diff mbox series

[v8,07/18] drm/msm/a6xx: Add a helper for software-resetting the GPU

Message ID 20230223-topic-gmuwrapper-v8-7-69c68206609e@linaro.org
State New
Headers
Series GMU-less A6xx support (A610, A619_holi) |

Commit Message

Konrad Dybcio May 29, 2023, 1:52 p.m. UTC 
  Introduce a6xx_gpu_sw_reset() in preparation for adding GMU wrapper
GPUs and reuse it in a6xx_gmu_force_off().

This helper, contrary to the original usage in GMU code paths, adds
a write memory barrier which together with the necessary delay should
ensure that the reset is never deasserted too quickly due to e.g. OoO
execution going crazy.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c |  3 +--
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++++++++++
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

Comments

Akhil P Oommen June 6, 2023, 5:18 p.m. UTC | #1 
On Mon, May 29, 2023 at 03:52:26PM +0200, Konrad Dybcio wrote:
> 
> Introduce a6xx_gpu_sw_reset() in preparation for adding GMU wrapper
> GPUs and reuse it in a6xx_gmu_force_off().
> 
> This helper, contrary to the original usage in GMU code paths, adds
> a write memory barrier which together with the necessary delay should
> ensure that the reset is never deasserted too quickly due to e.g. OoO
> execution going crazy.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c |  3 +--
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++++++++++
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  1 +
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index b86be123ecd0..5ba8cba69383 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -899,8 +899,7 @@ static void a6xx_gmu_force_off(struct a6xx_gmu *gmu)
>  	a6xx_bus_clear_pending_transactions(adreno_gpu, true);
>  
>  	/* Reset GPU core blocks */
> -	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, 1);
> -	udelay(100);
> +	a6xx_gpu_sw_reset(gpu, true);
>  }
>  
>  static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index e3ac3f045665..083ccb5bcb4e 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1634,6 +1634,17 @@ void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_
>  	gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0);
>  }
>  
> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert)
> +{
> +	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, assert);
> +	/* Add a barrier to avoid bad surprises */
Can you please make this comment a bit more clear? Highlight that we
should ensure the register is posted at hw before polling.

I think this barrier is required only during assert.

-Akhil.
> +	mb();
> +
> +	/* The reset line needs to be asserted for at least 100 us */
> +	if (assert)
> +		udelay(100);
> +}
> +
>  static int a6xx_pm_resume(struct msm_gpu *gpu)
>  {
>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> index 9580def06d45..aa70390ee1c6 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> @@ -89,5 +89,6 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu);
>  int a6xx_gpu_state_put(struct msm_gpu_state *state);
>  
>  void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_off);
> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert);
>  
>  #endif /* __A6XX_GPU_H__ */
> 
> -- 
> 2.40.1
>
Konrad Dybcio June 15, 2023, 10:34 a.m. UTC | #2 
On 6.06.2023 19:18, Akhil P Oommen wrote:
> On Mon, May 29, 2023 at 03:52:26PM +0200, Konrad Dybcio wrote:
>>
>> Introduce a6xx_gpu_sw_reset() in preparation for adding GMU wrapper
>> GPUs and reuse it in a6xx_gmu_force_off().
>>
>> This helper, contrary to the original usage in GMU code paths, adds
>> a write memory barrier which together with the necessary delay should
>> ensure that the reset is never deasserted too quickly due to e.g. OoO
>> execution going crazy.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c |  3 +--
>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++++++++++
>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  1 +
>>  3 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> index b86be123ecd0..5ba8cba69383 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> @@ -899,8 +899,7 @@ static void a6xx_gmu_force_off(struct a6xx_gmu *gmu)
>>  	a6xx_bus_clear_pending_transactions(adreno_gpu, true);
>>  
>>  	/* Reset GPU core blocks */
>> -	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, 1);
>> -	udelay(100);
>> +	a6xx_gpu_sw_reset(gpu, true);
>>  }
>>  
>>  static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> index e3ac3f045665..083ccb5bcb4e 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> @@ -1634,6 +1634,17 @@ void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_
>>  	gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0);
>>  }
>>  
>> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert)
>> +{
>> +	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, assert);
>> +	/* Add a barrier to avoid bad surprises */
> Can you please make this comment a bit more clear? Highlight that we
> should ensure the register is posted at hw before polling.
> 
> I think this barrier is required only during assert.
Generally it should not be strictly required at all, but I'm thinking
that it'd be good to keep it in both cases, so that:

if (assert)
	we don't keep writing things to the GPU if it's in reset
else
	we don't start writing things to the GPU becomes it comes
	out of reset

Also, if you squint hard enough at the commit message, you'll notice
I intended for this so only be a wmb, but for some reason generalized
it.. Perhaps that's another thing I should fix!
for v9..

Konrad
> 
> -Akhil.
>> +	mb();
>> +
>> +	/* The reset line needs to be asserted for at least 100 us */
>> +	if (assert)
>> +		udelay(100);
>> +}
>> +
>>  static int a6xx_pm_resume(struct msm_gpu *gpu)
>>  {
>>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> index 9580def06d45..aa70390ee1c6 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> @@ -89,5 +89,6 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu);
>>  int a6xx_gpu_state_put(struct msm_gpu_state *state);
>>  
>>  void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_off);
>> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert);
>>  
>>  #endif /* __A6XX_GPU_H__ */
>>
>> -- 
>> 2.40.1
>>
Akhil P Oommen June 15, 2023, 8:11 p.m. UTC | #3 
On Thu, Jun 15, 2023 at 12:34:06PM +0200, Konrad Dybcio wrote:
> 
> On 6.06.2023 19:18, Akhil P Oommen wrote:
> > On Mon, May 29, 2023 at 03:52:26PM +0200, Konrad Dybcio wrote:
> >>
> >> Introduce a6xx_gpu_sw_reset() in preparation for adding GMU wrapper
> >> GPUs and reuse it in a6xx_gmu_force_off().
> >>
> >> This helper, contrary to the original usage in GMU code paths, adds
> >> a write memory barrier which together with the necessary delay should
> >> ensure that the reset is never deasserted too quickly due to e.g. OoO
> >> execution going crazy.
> >>
> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> ---
> >>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c |  3 +--
> >>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++++++++++
> >>  drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  1 +
> >>  3 files changed, 13 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >> index b86be123ecd0..5ba8cba69383 100644
> >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >> @@ -899,8 +899,7 @@ static void a6xx_gmu_force_off(struct a6xx_gmu *gmu)
> >>  	a6xx_bus_clear_pending_transactions(adreno_gpu, true);
> >>  
> >>  	/* Reset GPU core blocks */
> >> -	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, 1);
> >> -	udelay(100);
> >> +	a6xx_gpu_sw_reset(gpu, true);
> >>  }
> >>  
> >>  static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >> index e3ac3f045665..083ccb5bcb4e 100644
> >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >> @@ -1634,6 +1634,17 @@ void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_
> >>  	gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0);
> >>  }
> >>  
> >> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert)
> >> +{
> >> +	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, assert);
> >> +	/* Add a barrier to avoid bad surprises */
> > Can you please make this comment a bit more clear? Highlight that we
> > should ensure the register is posted at hw before polling.
> > 
> > I think this barrier is required only during assert.
> Generally it should not be strictly required at all, but I'm thinking
> that it'd be good to keep it in both cases, so that:
> 
> if (assert)
> 	we don't keep writing things to the GPU if it's in reset
> else
> 	we don't start writing things to the GPU becomes it comes
> 	out of reset
> 
> Also, if you squint hard enough at the commit message, you'll notice
> I intended for this so only be a wmb, but for some reason generalized
> it.. Perhaps that's another thing I should fix!
> for v9..

wmb() doesn't provide any ordering guarantee with the delay loop.
A common practice is to just read back the same register before
the loop because a readl followed by delay() is guaranteed to be ordered.

-Akhil.
> 
> Konrad
> > 
> > -Akhil.
> >> +	mb();
> >> +
> >> +	/* The reset line needs to be asserted for at least 100 us */
> >> +	if (assert)
> >> +		udelay(100);
> >> +}
> >> +
> >>  static int a6xx_pm_resume(struct msm_gpu *gpu)
> >>  {
> >>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> >> index 9580def06d45..aa70390ee1c6 100644
> >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> >> @@ -89,5 +89,6 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu);
> >>  int a6xx_gpu_state_put(struct msm_gpu_state *state);
> >>  
> >>  void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_off);
> >> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert);
> >>  
> >>  #endif /* __A6XX_GPU_H__ */
> >>
> >> -- 
> >> 2.40.1
> >>
Konrad Dybcio June 15, 2023, 8:59 p.m. UTC | #4 
On 15.06.2023 22:11, Akhil P Oommen wrote:
> On Thu, Jun 15, 2023 at 12:34:06PM +0200, Konrad Dybcio wrote:
>>
>> On 6.06.2023 19:18, Akhil P Oommen wrote:
>>> On Mon, May 29, 2023 at 03:52:26PM +0200, Konrad Dybcio wrote:
>>>>
>>>> Introduce a6xx_gpu_sw_reset() in preparation for adding GMU wrapper
>>>> GPUs and reuse it in a6xx_gmu_force_off().
>>>>
>>>> This helper, contrary to the original usage in GMU code paths, adds
>>>> a write memory barrier which together with the necessary delay should
>>>> ensure that the reset is never deasserted too quickly due to e.g. OoO
>>>> execution going crazy.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c |  3 +--
>>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++++++++++
>>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  1 +
>>>>  3 files changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>> index b86be123ecd0..5ba8cba69383 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>> @@ -899,8 +899,7 @@ static void a6xx_gmu_force_off(struct a6xx_gmu *gmu)
>>>>  	a6xx_bus_clear_pending_transactions(adreno_gpu, true);
>>>>  
>>>>  	/* Reset GPU core blocks */
>>>> -	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, 1);
>>>> -	udelay(100);
>>>> +	a6xx_gpu_sw_reset(gpu, true);
>>>>  }
>>>>  
>>>>  static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>> index e3ac3f045665..083ccb5bcb4e 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>> @@ -1634,6 +1634,17 @@ void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_
>>>>  	gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0);
>>>>  }
>>>>  
>>>> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert)
>>>> +{
>>>> +	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, assert);
>>>> +	/* Add a barrier to avoid bad surprises */
>>> Can you please make this comment a bit more clear? Highlight that we
>>> should ensure the register is posted at hw before polling.
>>>
>>> I think this barrier is required only during assert.
>> Generally it should not be strictly required at all, but I'm thinking
>> that it'd be good to keep it in both cases, so that:
>>
>> if (assert)
>> 	we don't keep writing things to the GPU if it's in reset
>> else
>> 	we don't start writing things to the GPU becomes it comes
>> 	out of reset
>>
>> Also, if you squint hard enough at the commit message, you'll notice
>> I intended for this so only be a wmb, but for some reason generalized
>> it.. Perhaps that's another thing I should fix!
>> for v9..
> 
> wmb() doesn't provide any ordering guarantee with the delay loop.
Hm, fair.. I'm still not as fluent with memory access knowledge as I'd
like to be..

> A common practice is to just read back the same register before
> the loop because a readl followed by delay() is guaranteed to be ordered.
So, how should I proceed? Keep the r/w barrier, or add a readback and
a tiiiny (perhaps even using ndelay instead of udelay?) delay on de-assert?

Konrad
> 
> -Akhil.
>>
>> Konrad
>>>
>>> -Akhil.
>>>> +	mb();
>>>> +
>>>> +	/* The reset line needs to be asserted for at least 100 us */
>>>> +	if (assert)
>>>> +		udelay(100);
>>>> +}
>>>> +
>>>>  static int a6xx_pm_resume(struct msm_gpu *gpu)
>>>>  {
>>>>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>>>> index 9580def06d45..aa70390ee1c6 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>>>> @@ -89,5 +89,6 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu);
>>>>  int a6xx_gpu_state_put(struct msm_gpu_state *state);
>>>>  
>>>>  void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_off);
>>>> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert);
>>>>  
>>>>  #endif /* __A6XX_GPU_H__ */
>>>>
>>>> -- 
>>>> 2.40.1
>>>>
Akhil P Oommen June 15, 2023, 9:17 p.m. UTC | #5 
On Thu, Jun 15, 2023 at 10:59:23PM +0200, Konrad Dybcio wrote:
> 
> On 15.06.2023 22:11, Akhil P Oommen wrote:
> > On Thu, Jun 15, 2023 at 12:34:06PM +0200, Konrad Dybcio wrote:
> >>
> >> On 6.06.2023 19:18, Akhil P Oommen wrote:
> >>> On Mon, May 29, 2023 at 03:52:26PM +0200, Konrad Dybcio wrote:
> >>>>
> >>>> Introduce a6xx_gpu_sw_reset() in preparation for adding GMU wrapper
> >>>> GPUs and reuse it in a6xx_gmu_force_off().
> >>>>
> >>>> This helper, contrary to the original usage in GMU code paths, adds
> >>>> a write memory barrier which together with the necessary delay should
> >>>> ensure that the reset is never deasserted too quickly due to e.g. OoO
> >>>> execution going crazy.
> >>>>
> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>> ---
> >>>>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c |  3 +--
> >>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++++++++++
> >>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  1 +
> >>>>  3 files changed, 13 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >>>> index b86be123ecd0..5ba8cba69383 100644
> >>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >>>> @@ -899,8 +899,7 @@ static void a6xx_gmu_force_off(struct a6xx_gmu *gmu)
> >>>>  	a6xx_bus_clear_pending_transactions(adreno_gpu, true);
> >>>>  
> >>>>  	/* Reset GPU core blocks */
> >>>> -	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, 1);
> >>>> -	udelay(100);
> >>>> +	a6xx_gpu_sw_reset(gpu, true);
> >>>>  }
> >>>>  
> >>>>  static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
> >>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>>> index e3ac3f045665..083ccb5bcb4e 100644
> >>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>>> @@ -1634,6 +1634,17 @@ void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_
> >>>>  	gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0);
> >>>>  }
> >>>>  
> >>>> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert)
> >>>> +{
> >>>> +	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, assert);
> >>>> +	/* Add a barrier to avoid bad surprises */
> >>> Can you please make this comment a bit more clear? Highlight that we
> >>> should ensure the register is posted at hw before polling.
> >>>
> >>> I think this barrier is required only during assert.
> >> Generally it should not be strictly required at all, but I'm thinking
> >> that it'd be good to keep it in both cases, so that:
> >>
> >> if (assert)
> >> 	we don't keep writing things to the GPU if it's in reset
> >> else
> >> 	we don't start writing things to the GPU becomes it comes
> >> 	out of reset
> >>
> >> Also, if you squint hard enough at the commit message, you'll notice
> >> I intended for this so only be a wmb, but for some reason generalized
> >> it.. Perhaps that's another thing I should fix!
> >> for v9..
> > 
> > wmb() doesn't provide any ordering guarantee with the delay loop.
> Hm, fair.. I'm still not as fluent with memory access knowledge as I'd
> like to be..
> 
> > A common practice is to just read back the same register before
> > the loop because a readl followed by delay() is guaranteed to be ordered.
> So, how should I proceed? Keep the r/w barrier, or add a readback and
> a tiiiny (perhaps even using ndelay instead of udelay?) delay on de-assert?

readback + delay (similar value as downstream). This path is exercised
rarely.

-Akhil.

> 
> Konrad
> > 
> > -Akhil.
> >>
> >> Konrad
> >>>
> >>> -Akhil.
> >>>> +	mb();
> >>>> +
> >>>> +	/* The reset line needs to be asserted for at least 100 us */
> >>>> +	if (assert)
> >>>> +		udelay(100);
> >>>> +}
> >>>> +
> >>>>  static int a6xx_pm_resume(struct msm_gpu *gpu)
> >>>>  {
> >>>>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> >>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> >>>> index 9580def06d45..aa70390ee1c6 100644
> >>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> >>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> >>>> @@ -89,5 +89,6 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu);
> >>>>  int a6xx_gpu_state_put(struct msm_gpu_state *state);
> >>>>  
> >>>>  void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_off);
> >>>> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert);
> >>>>  
> >>>>  #endif /* __A6XX_GPU_H__ */
> >>>>
> >>>> -- 
> >>>> 2.40.1
> >>>>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index b86be123ecd0..5ba8cba69383 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -899,8 +899,7 @@  static void a6xx_gmu_force_off(struct a6xx_gmu *gmu)
 	a6xx_bus_clear_pending_transactions(adreno_gpu, true);
 
 	/* Reset GPU core blocks */
-	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, 1);
-	udelay(100);
+	a6xx_gpu_sw_reset(gpu, true);
 }
 
 static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index e3ac3f045665..083ccb5bcb4e 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1634,6 +1634,17 @@  void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_
 	gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0);
 }
 
+void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert)
+{
+	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, assert);
+	/* Add a barrier to avoid bad surprises */
+	mb();
+
+	/* The reset line needs to be asserted for at least 100 us */
+	if (assert)
+		udelay(100);
+}
+
 static int a6xx_pm_resume(struct msm_gpu *gpu)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
index 9580def06d45..aa70390ee1c6 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
@@ -89,5 +89,6 @@  struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu);
 int a6xx_gpu_state_put(struct msm_gpu_state *state);
 
 void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_off);
+void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert);
 
 #endif /* __A6XX_GPU_H__ */