| Message ID | 1580935697-28195-1-git-send-email-jcrouse@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9cc68ee1d92e3ab5bd5c821e5c1f387b0e16a669 |
| Headers | show |
| Series | [v2] drm/msm: Fix a6xx GMU shutdown sequence | expand |
On Wed, Feb 5, 2020 at 12:48 PM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > Commit e812744c5f95 ("drm: msm: a6xx: Add support for A618") missed > updating the VBIF flush in a6xx_gmu_shutdown and instead > inserted the new sequence into a6xx_pm_suspend along with a redundant > GMU idle. > > Move a6xx_bus_clear_pending_transactions to a6xx_gmu.c and use it in > the appropriate place in the shutdown routine and remove the redundant > idle call. > > v2: Remove newly unused variable that was triggering a warning > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> Reviewed-by: Rob Clark <robdclark@gmail.com> > --- > > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 37 +++++++++++++++++++++++++----- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 43 ----------------------------------- > 2 files changed, 31 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > index 983afea..748cd37 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > @@ -796,12 +796,41 @@ bool a6xx_gmu_isidle(struct a6xx_gmu *gmu) > return true; > } > > +#define GBIF_CLIENT_HALT_MASK BIT(0) > +#define GBIF_ARB_HALT_MASK BIT(1) > + > +static void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu) > +{ > + struct msm_gpu *gpu = &adreno_gpu->base; > + > + if (!a6xx_has_gbif(adreno_gpu)) { > + gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0xf); > + spin_until((gpu_read(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL1) & > + 0xf) == 0xf); > + gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0); > + > + return; > + } > + > + /* Halt new client requests on GBIF */ > + gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_CLIENT_HALT_MASK); > + spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) & > + (GBIF_CLIENT_HALT_MASK)) == GBIF_CLIENT_HALT_MASK); > + > + /* Halt all AXI requests on GBIF */ > + gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_ARB_HALT_MASK); > + spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) & > + (GBIF_ARB_HALT_MASK)) == GBIF_ARB_HALT_MASK); > + > + /* The GBIF halt needs to be explicitly cleared */ > + gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0); > +} > + > /* Gracefully try to shut down the GMU and by extension the GPU */ > static void a6xx_gmu_shutdown(struct a6xx_gmu *gmu) > { > struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct a6xx_gpu, gmu); > struct adreno_gpu *adreno_gpu = &a6xx_gpu->base; > - struct msm_gpu *gpu = &adreno_gpu->base; > u32 val; > > /* > @@ -819,11 +848,7 @@ static void a6xx_gmu_shutdown(struct a6xx_gmu *gmu) > return; > } > > - /* Clear the VBIF pipe before shutting down */ > - gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0xf); > - spin_until((gpu_read(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL1) & 0xf) > - == 0xf); > - gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0); > + a6xx_bus_clear_pending_transactions(adreno_gpu); > > /* tell the GMU we want to slumber */ > a6xx_gmu_notify_slumber(gmu); > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index daf0780..b580e47 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -748,39 +748,6 @@ static const u32 a6xx_register_offsets[REG_ADRENO_REGISTER_MAX] = { > REG_ADRENO_DEFINE(REG_ADRENO_CP_RB_CNTL, REG_A6XX_CP_RB_CNTL), > }; > > -#define GBIF_CLIENT_HALT_MASK BIT(0) > -#define GBIF_ARB_HALT_MASK BIT(1) > - > -static void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu) > -{ > - struct msm_gpu *gpu = &adreno_gpu->base; > - > - if(!a6xx_has_gbif(adreno_gpu)){ > - gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0xf); > - spin_until((gpu_read(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL1) & > - 0xf) == 0xf); > - gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0); > - > - return; > - } > - > - /* Halt new client requests on GBIF */ > - gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_CLIENT_HALT_MASK); > - spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) & > - (GBIF_CLIENT_HALT_MASK)) == GBIF_CLIENT_HALT_MASK); > - > - /* Halt all AXI requests on GBIF */ > - gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_ARB_HALT_MASK); > - spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) & > - (GBIF_ARB_HALT_MASK)) == GBIF_ARB_HALT_MASK); > - > - /* > - * GMU needs DDR access in slumber path. Deassert GBIF halt now > - * to allow for GMU to access system memory. > - */ > - gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0); > -} > - > static int a6xx_pm_resume(struct msm_gpu *gpu) > { > struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > @@ -805,16 +772,6 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu) > > devfreq_suspend_device(gpu->devfreq.devfreq); > > - /* > - * Make sure the GMU is idle before continuing (because some transitions > - * may use VBIF > - */ > - a6xx_gmu_wait_for_idle(&a6xx_gpu->gmu); > - > - /* Clear the VBIF pipe before shutting down */ > - /* FIXME: This accesses the GPU - do we need to make sure it is on? */ > - a6xx_bus_clear_pending_transactions(adreno_gpu); > - > return a6xx_gmu_stop(a6xx_gpu); > } > > -- > 2.7.4
Hi, On Wed, Feb 5, 2020 at 1:00 PM Rob Clark <robdclark@gmail.com> wrote: > > On Wed, Feb 5, 2020 at 12:48 PM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > > > Commit e812744c5f95 ("drm: msm: a6xx: Add support for A618") missed > > updating the VBIF flush in a6xx_gmu_shutdown and instead > > inserted the new sequence into a6xx_pm_suspend along with a redundant > > GMU idle. > > > > Move a6xx_bus_clear_pending_transactions to a6xx_gmu.c and use it in > > the appropriate place in the shutdown routine and remove the redundant > > idle call. > > > > v2: Remove newly unused variable that was triggering a warning > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > Reviewed-by: Rob Clark <robdclark@gmail.com> Without this patch I'm seeing some really bad behavior where the whole system will pause for a bit, especially if it has been idle. After this patch things are much better. Thus: Fixes: e812744c5f95 ("drm: msm: a6xx: Add support for A618") Tested-by: Douglas Anderson <dianders@chromium.org> -Doug
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c index 983afea..748cd37 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c @@ -796,12 +796,41 @@ bool a6xx_gmu_isidle(struct a6xx_gmu *gmu) return true; } +#define GBIF_CLIENT_HALT_MASK BIT(0) +#define GBIF_ARB_HALT_MASK BIT(1) + +static void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu) +{ + struct msm_gpu *gpu = &adreno_gpu->base; + + if (!a6xx_has_gbif(adreno_gpu)) { + gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0xf); + spin_until((gpu_read(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL1) & + 0xf) == 0xf); + gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0); + + return; + } + + /* Halt new client requests on GBIF */ + gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_CLIENT_HALT_MASK); + spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) & + (GBIF_CLIENT_HALT_MASK)) == GBIF_CLIENT_HALT_MASK); + + /* Halt all AXI requests on GBIF */ + gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_ARB_HALT_MASK); + spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) & + (GBIF_ARB_HALT_MASK)) == GBIF_ARB_HALT_MASK); + + /* The GBIF halt needs to be explicitly cleared */ + gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0); +} + /* Gracefully try to shut down the GMU and by extension the GPU */ static void a6xx_gmu_shutdown(struct a6xx_gmu *gmu) { struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct a6xx_gpu, gmu); struct adreno_gpu *adreno_gpu = &a6xx_gpu->base; - struct msm_gpu *gpu = &adreno_gpu->base; u32 val; /* @@ -819,11 +848,7 @@ static void a6xx_gmu_shutdown(struct a6xx_gmu *gmu) return; } - /* Clear the VBIF pipe before shutting down */ - gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0xf); - spin_until((gpu_read(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL1) & 0xf) - == 0xf); - gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0); + a6xx_bus_clear_pending_transactions(adreno_gpu); /* tell the GMU we want to slumber */ a6xx_gmu_notify_slumber(gmu); diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index daf0780..b580e47 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -748,39 +748,6 @@ static const u32 a6xx_register_offsets[REG_ADRENO_REGISTER_MAX] = { REG_ADRENO_DEFINE(REG_ADRENO_CP_RB_CNTL, REG_A6XX_CP_RB_CNTL), }; -#define GBIF_CLIENT_HALT_MASK BIT(0) -#define GBIF_ARB_HALT_MASK BIT(1) - -static void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu) -{ - struct msm_gpu *gpu = &adreno_gpu->base; - - if(!a6xx_has_gbif(adreno_gpu)){ - gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0xf); - spin_until((gpu_read(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL1) & - 0xf) == 0xf); - gpu_write(gpu, REG_A6XX_VBIF_XIN_HALT_CTRL0, 0); - - return; - } - - /* Halt new client requests on GBIF */ - gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_CLIENT_HALT_MASK); - spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) & - (GBIF_CLIENT_HALT_MASK)) == GBIF_CLIENT_HALT_MASK); - - /* Halt all AXI requests on GBIF */ - gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_ARB_HALT_MASK); - spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) & - (GBIF_ARB_HALT_MASK)) == GBIF_ARB_HALT_MASK); - - /* - * GMU needs DDR access in slumber path. Deassert GBIF halt now - * to allow for GMU to access system memory. - */ - gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0); -} - static int a6xx_pm_resume(struct msm_gpu *gpu) { struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); @@ -805,16 +772,6 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu) devfreq_suspend_device(gpu->devfreq.devfreq); - /* - * Make sure the GMU is idle before continuing (because some transitions - * may use VBIF - */ - a6xx_gmu_wait_for_idle(&a6xx_gpu->gmu); - - /* Clear the VBIF pipe before shutting down */ - /* FIXME: This accesses the GPU - do we need to make sure it is on? */ - a6xx_bus_clear_pending_transactions(adreno_gpu); - return a6xx_gmu_stop(a6xx_gpu); }
Commit e812744c5f95 ("drm: msm: a6xx: Add support for A618") missed updating the VBIF flush in a6xx_gmu_shutdown and instead inserted the new sequence into a6xx_pm_suspend along with a redundant GMU idle. Move a6xx_bus_clear_pending_transactions to a6xx_gmu.c and use it in the appropriate place in the shutdown routine and remove the redundant idle call. v2: Remove newly unused variable that was triggering a warning Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> --- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 37 +++++++++++++++++++++++++----- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 43 ----------------------------------- 2 files changed, 31 insertions(+), 49 deletions(-)