| Message ID | 20220730150952.v3.5.I176567525af2b9439a7e485d0ca130528666a55c@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve GPU Recovery | expand |
On 30/07/2022 12:40, Akhil P Oommen wrote: > Because there could be transient votes from other drivers/tz/hyp which > may keep the cx gdsc enabled, we should poll until cx gdsc collapses. > We can use the reset framework to poll for cx gdsc collapse from gpucc > clk driver. > > This feature requires support from the platform's gpucc driver. > > Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > --- > > Changes in v3: > - Use reset interface from gpucc driver to poll for cx gdsc collapse > https://patchwork.freedesktop.org/series/106860/ > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++++ > drivers/gpu/drm/msm/msm_gpu.c | 4 ++++ > drivers/gpu/drm/msm/msm_gpu.h | 4 ++++ > 3 files changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 1b049c5..721d5e6 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -10,6 +10,7 @@ > > #include <linux/bitfield.h> > #include <linux/devfreq.h> > +#include <linux/reset.h> > #include <linux/soc/qcom/llcc-qcom.h> > > #define GPU_PAS_ID 13 > @@ -1224,6 +1225,9 @@ static void a6xx_recover(struct msm_gpu *gpu) > /* And the final one from recover worker */ > pm_runtime_put_sync(&gpu->pdev->dev); > > + /* Call into gpucc driver to poll for cx gdsc collapse */ > + reset_control_reset(gpu->cx_collapse); Do we have a race between the last pm_runtime_put_sync(), this polling and other voters removing their votes beforehand? > + > pm_runtime_use_autosuspend(&gpu->pdev->dev); > > if (active_submits) > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > index 07e55a6..4a57627 100644 > --- a/drivers/gpu/drm/msm/msm_gpu.c > +++ b/drivers/gpu/drm/msm/msm_gpu.c > @@ -14,6 +14,7 @@ > #include <generated/utsrelease.h> > #include <linux/string_helpers.h> > #include <linux/devcoredump.h> > +#include <linux/reset.h> > #include <linux/sched/task.h> > > /* > @@ -903,6 +904,9 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev, > if (IS_ERR(gpu->gpu_cx)) > gpu->gpu_cx = NULL; > > + gpu->cx_collapse = devm_reset_control_get_optional(&pdev->dev, > + "cx_collapse"); > + > gpu->pdev = pdev; > platform_set_drvdata(pdev, &gpu->adreno_smmu); > > diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h > index 6def008..ab59fd2 100644 > --- a/drivers/gpu/drm/msm/msm_gpu.h > +++ b/drivers/gpu/drm/msm/msm_gpu.h > @@ -13,6 +13,7 @@ > #include <linux/interconnect.h> > #include <linux/pm_opp.h> > #include <linux/regulator/consumer.h> > +#include <linux/reset.h> > > #include "msm_drv.h" > #include "msm_fence.h" > @@ -268,6 +269,9 @@ struct msm_gpu { > bool hw_apriv; > > struct thermal_cooling_device *cooling; > + > + /* To poll for cx gdsc collapse during gpu recovery */ > + struct reset_control *cx_collapse; > }; > > static inline struct msm_gpu *dev_to_gpu(struct device *dev)
On 8/2/2022 12:44 PM, Dmitry Baryshkov wrote: > On 30/07/2022 12:40, Akhil P Oommen wrote: >> Because there could be transient votes from other drivers/tz/hyp which >> may keep the cx gdsc enabled, we should poll until cx gdsc collapses. >> We can use the reset framework to poll for cx gdsc collapse from gpucc >> clk driver. >> >> This feature requires support from the platform's gpucc driver. >> >> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >> --- >> >> Changes in v3: >> - Use reset interface from gpucc driver to poll for cx gdsc collapse >> https://patchwork.freedesktop.org/series/106860/ >> >> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++++ >> drivers/gpu/drm/msm/msm_gpu.c | 4 ++++ >> drivers/gpu/drm/msm/msm_gpu.h | 4 ++++ >> 3 files changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> index 1b049c5..721d5e6 100644 >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> @@ -10,6 +10,7 @@ >> #include <linux/bitfield.h> >> #include <linux/devfreq.h> >> +#include <linux/reset.h> >> #include <linux/soc/qcom/llcc-qcom.h> >> #define GPU_PAS_ID 13 >> @@ -1224,6 +1225,9 @@ static void a6xx_recover(struct msm_gpu *gpu) >> /* And the final one from recover worker */ >> pm_runtime_put_sync(&gpu->pdev->dev); >> + /* Call into gpucc driver to poll for cx gdsc collapse */ >> + reset_control_reset(gpu->cx_collapse); > > Do we have a race between the last pm_runtime_put_sync(), this polling > and other voters removing their votes beforehand? I can't see any issue with a race here. reset_control_reset() will return immediately in that case. -Akhil. > >> + >> pm_runtime_use_autosuspend(&gpu->pdev->dev); >> if (active_submits) >> diff --git a/drivers/gpu/drm/msm/msm_gpu.c >> b/drivers/gpu/drm/msm/msm_gpu.c >> index 07e55a6..4a57627 100644 >> --- a/drivers/gpu/drm/msm/msm_gpu.c >> +++ b/drivers/gpu/drm/msm/msm_gpu.c >> @@ -14,6 +14,7 @@ >> #include <generated/utsrelease.h> >> #include <linux/string_helpers.h> >> #include <linux/devcoredump.h> >> +#include <linux/reset.h> >> #include <linux/sched/task.h> >> /* >> @@ -903,6 +904,9 @@ int msm_gpu_init(struct drm_device *drm, struct >> platform_device *pdev, >> if (IS_ERR(gpu->gpu_cx)) >> gpu->gpu_cx = NULL; >> + gpu->cx_collapse = devm_reset_control_get_optional(&pdev->dev, >> + "cx_collapse"); >> + >> gpu->pdev = pdev; >> platform_set_drvdata(pdev, &gpu->adreno_smmu); >> diff --git a/drivers/gpu/drm/msm/msm_gpu.h >> b/drivers/gpu/drm/msm/msm_gpu.h >> index 6def008..ab59fd2 100644 >> --- a/drivers/gpu/drm/msm/msm_gpu.h >> +++ b/drivers/gpu/drm/msm/msm_gpu.h >> @@ -13,6 +13,7 @@ >> #include <linux/interconnect.h> >> #include <linux/pm_opp.h> >> #include <linux/regulator/consumer.h> >> +#include <linux/reset.h> >> #include "msm_drv.h" >> #include "msm_fence.h" >> @@ -268,6 +269,9 @@ struct msm_gpu { >> bool hw_apriv; >> struct thermal_cooling_device *cooling; >> + >> + /* To poll for cx gdsc collapse during gpu recovery */ >> + struct reset_control *cx_collapse; >> }; >> static inline struct msm_gpu *dev_to_gpu(struct device *dev) > >
On Wed, 3 Aug 2022 at 13:15, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > > On 8/2/2022 12:44 PM, Dmitry Baryshkov wrote: > > On 30/07/2022 12:40, Akhil P Oommen wrote: > >> Because there could be transient votes from other drivers/tz/hyp which > >> may keep the cx gdsc enabled, we should poll until cx gdsc collapses. > >> We can use the reset framework to poll for cx gdsc collapse from gpucc > >> clk driver. > >> > >> This feature requires support from the platform's gpucc driver. > >> > >> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > >> --- > >> > >> Changes in v3: > >> - Use reset interface from gpucc driver to poll for cx gdsc collapse > >> https://patchwork.freedesktop.org/series/106860/ > >> > >> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++++ > >> drivers/gpu/drm/msm/msm_gpu.c | 4 ++++ > >> drivers/gpu/drm/msm/msm_gpu.h | 4 ++++ > >> 3 files changed, 12 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >> index 1b049c5..721d5e6 100644 > >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >> @@ -10,6 +10,7 @@ > >> #include <linux/bitfield.h> > >> #include <linux/devfreq.h> > >> +#include <linux/reset.h> > >> #include <linux/soc/qcom/llcc-qcom.h> > >> #define GPU_PAS_ID 13 > >> @@ -1224,6 +1225,9 @@ static void a6xx_recover(struct msm_gpu *gpu) > >> /* And the final one from recover worker */ > >> pm_runtime_put_sync(&gpu->pdev->dev); > >> + /* Call into gpucc driver to poll for cx gdsc collapse */ > >> + reset_control_reset(gpu->cx_collapse); > > > > Do we have a race between the last pm_runtime_put_sync(), this polling > > and other voters removing their votes beforehand? > I can't see any issue with a race here. reset_control_reset() will > return immediately in that case. Ack, ok then. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index 1b049c5..721d5e6 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -10,6 +10,7 @@ #include <linux/bitfield.h> #include <linux/devfreq.h> +#include <linux/reset.h> #include <linux/soc/qcom/llcc-qcom.h> #define GPU_PAS_ID 13 @@ -1224,6 +1225,9 @@ static void a6xx_recover(struct msm_gpu *gpu) /* And the final one from recover worker */ pm_runtime_put_sync(&gpu->pdev->dev); + /* Call into gpucc driver to poll for cx gdsc collapse */ + reset_control_reset(gpu->cx_collapse); + pm_runtime_use_autosuspend(&gpu->pdev->dev); if (active_submits) diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 07e55a6..4a57627 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -14,6 +14,7 @@ #include <generated/utsrelease.h> #include <linux/string_helpers.h> #include <linux/devcoredump.h> +#include <linux/reset.h> #include <linux/sched/task.h> /* @@ -903,6 +904,9 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev, if (IS_ERR(gpu->gpu_cx)) gpu->gpu_cx = NULL; + gpu->cx_collapse = devm_reset_control_get_optional(&pdev->dev, + "cx_collapse"); + gpu->pdev = pdev; platform_set_drvdata(pdev, &gpu->adreno_smmu); diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h index 6def008..ab59fd2 100644 --- a/drivers/gpu/drm/msm/msm_gpu.h +++ b/drivers/gpu/drm/msm/msm_gpu.h @@ -13,6 +13,7 @@ #include <linux/interconnect.h> #include <linux/pm_opp.h> #include <linux/regulator/consumer.h> +#include <linux/reset.h> #include "msm_drv.h" #include "msm_fence.h" @@ -268,6 +269,9 @@ struct msm_gpu { bool hw_apriv; struct thermal_cooling_device *cooling; + + /* To poll for cx gdsc collapse during gpu recovery */ + struct reset_control *cx_collapse; }; static inline struct msm_gpu *dev_to_gpu(struct device *dev)
Because there could be transient votes from other drivers/tz/hyp which may keep the cx gdsc enabled, we should poll until cx gdsc collapses. We can use the reset framework to poll for cx gdsc collapse from gpucc clk driver. This feature requires support from the platform's gpucc driver. Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> --- Changes in v3: - Use reset interface from gpucc driver to poll for cx gdsc collapse https://patchwork.freedesktop.org/series/106860/ drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++++ drivers/gpu/drm/msm/msm_gpu.c | 4 ++++ drivers/gpu/drm/msm/msm_gpu.h | 4 ++++ 3 files changed, 12 insertions(+)