diff mbox series

[1/7] drm/msm: Fix bv_fence being used as bv_rptr

Message ID 20240815-preemption-a750-t-v1-1-7bda26c34037@gmail.com (mailing list archive)
State New, archived
Headers show
Series Preemption support for A7XX | expand

Commit Message

Antonino Maniscalco Aug. 15, 2024, 6:26 p.m. UTC
The bv_fence field of rbmemptrs was being used incorrectly as the BV
rptr shadow pointer in some places.

Add a bv_rptr field and change the code to use that instead.

Signed-off-by: Antonino Maniscalco <antomani103@gmail.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
 drivers/gpu/drm/msm/msm_ringbuffer.h  | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Akhil P Oommen Aug. 19, 2024, 7:59 p.m. UTC | #1
On Thu, Aug 15, 2024 at 08:26:11PM +0200, Antonino Maniscalco wrote:
> The bv_fence field of rbmemptrs was being used incorrectly as the BV
> rptr shadow pointer in some places.
> 
> Add a bv_rptr field and change the code to use that instead.
> 
> Signed-off-by: Antonino Maniscalco <antomani103@gmail.com>

Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>

-Akhil.

> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
>  drivers/gpu/drm/msm/msm_ringbuffer.h  | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index bcaec86ac67a..32a4faa93d7f 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1132,7 +1132,7 @@ static int hw_init(struct msm_gpu *gpu)
>  	/* ..which means "always" on A7xx, also for BV shadow */
>  	if (adreno_is_a7xx(adreno_gpu)) {
>  		gpu_write64(gpu, REG_A7XX_CP_BV_RB_RPTR_ADDR,
> -			    rbmemptr(gpu->rb[0], bv_fence));
> +			    rbmemptr(gpu->rb[0], bv_rptr));
>  	}
>  
>  	/* Always come up on rb 0 */
> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h b/drivers/gpu/drm/msm/msm_ringbuffer.h
> index 0d6beb8cd39a..40791b2ade46 100644
> --- a/drivers/gpu/drm/msm/msm_ringbuffer.h
> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.h
> @@ -31,6 +31,7 @@ struct msm_rbmemptrs {
>  	volatile uint32_t rptr;
>  	volatile uint32_t fence;
>  	/* Introduced on A7xx */
> +	volatile uint32_t bv_rptr;
>  	volatile uint32_t bv_fence;
>  
>  	volatile struct msm_gpu_submit_stats stats[MSM_GPU_SUBMIT_STATS_COUNT];
> 
> -- 
> 2.46.0
> 
>
Konrad Dybcio Aug. 20, 2024, 10:09 a.m. UTC | #2
On 15.08.2024 8:26 PM, Antonino Maniscalco wrote:
> The bv_fence field of rbmemptrs was being used incorrectly as the BV
> rptr shadow pointer in some places.
> 
> Add a bv_rptr field and change the code to use that instead.
> 
> Signed-off-by: Antonino Maniscalco <antomani103@gmail.com>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
>  drivers/gpu/drm/msm/msm_ringbuffer.h  | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index bcaec86ac67a..32a4faa93d7f 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1132,7 +1132,7 @@ static int hw_init(struct msm_gpu *gpu)
>  	/* ..which means "always" on A7xx, also for BV shadow */
>  	if (adreno_is_a7xx(adreno_gpu)) {
>  		gpu_write64(gpu, REG_A7XX_CP_BV_RB_RPTR_ADDR,
> -			    rbmemptr(gpu->rb[0], bv_fence));
> +			    rbmemptr(gpu->rb[0], bv_rptr));
>  	}
>  
>  	/* Always come up on rb 0 */
> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h b/drivers/gpu/drm/msm/msm_ringbuffer.h
> index 0d6beb8cd39a..40791b2ade46 100644
> --- a/drivers/gpu/drm/msm/msm_ringbuffer.h
> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.h
> @@ -31,6 +31,7 @@ struct msm_rbmemptrs {
>  	volatile uint32_t rptr;
>  	volatile uint32_t fence;
>  	/* Introduced on A7xx */
> +	volatile uint32_t bv_rptr;

This is never initialized or assigned any value, no?

Konrad
Connor Abbott Aug. 20, 2024, 10:45 a.m. UTC | #3
On Tue, Aug 20, 2024 at 11:15 AM Konrad Dybcio <konradybcio@kernel.org> wrote:
>
> On 15.08.2024 8:26 PM, Antonino Maniscalco wrote:
> > The bv_fence field of rbmemptrs was being used incorrectly as the BV
> > rptr shadow pointer in some places.
> >
> > Add a bv_rptr field and change the code to use that instead.
> >
> > Signed-off-by: Antonino Maniscalco <antomani103@gmail.com>
> > ---
> >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
> >  drivers/gpu/drm/msm/msm_ringbuffer.h  | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > index bcaec86ac67a..32a4faa93d7f 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > @@ -1132,7 +1132,7 @@ static int hw_init(struct msm_gpu *gpu)
> >       /* ..which means "always" on A7xx, also for BV shadow */
> >       if (adreno_is_a7xx(adreno_gpu)) {
> >               gpu_write64(gpu, REG_A7XX_CP_BV_RB_RPTR_ADDR,
> > -                         rbmemptr(gpu->rb[0], bv_fence));
> > +                         rbmemptr(gpu->rb[0], bv_rptr));
> >       }
> >
> >       /* Always come up on rb 0 */
> > diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h b/drivers/gpu/drm/msm/msm_ringbuffer.h
> > index 0d6beb8cd39a..40791b2ade46 100644
> > --- a/drivers/gpu/drm/msm/msm_ringbuffer.h
> > +++ b/drivers/gpu/drm/msm/msm_ringbuffer.h
> > @@ -31,6 +31,7 @@ struct msm_rbmemptrs {
> >       volatile uint32_t rptr;
> >       volatile uint32_t fence;
> >       /* Introduced on A7xx */
> > +     volatile uint32_t bv_rptr;
>
> This is never initialized or assigned any value, no?
>
> Konrad

Neither is the original (retroactively BR) shadow RPTR, except
apparently on suspend (no idea why). It's written by the GPU as it
reads the ringbuffer, because CP_BV_RPTR_ADDR is set to its address.
For the BV shadow RPTR, we aren't really using it for anything (and
neither is kgsl) so we just need to point the register to a valid
"dummy" address that isn't used by anything else.

Connor
Konrad Dybcio Aug. 20, 2024, 11:25 a.m. UTC | #4
On 20.08.2024 12:45 PM, Connor Abbott wrote:
> On Tue, Aug 20, 2024 at 11:15 AM Konrad Dybcio <konradybcio@kernel.org> wrote:
>>
>> On 15.08.2024 8:26 PM, Antonino Maniscalco wrote:
>>> The bv_fence field of rbmemptrs was being used incorrectly as the BV
>>> rptr shadow pointer in some places.
>>>
>>> Add a bv_rptr field and change the code to use that instead.
>>>
>>> Signed-off-by: Antonino Maniscalco <antomani103@gmail.com>
>>> ---
>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
>>>  drivers/gpu/drm/msm/msm_ringbuffer.h  | 1 +
>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> index bcaec86ac67a..32a4faa93d7f 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> @@ -1132,7 +1132,7 @@ static int hw_init(struct msm_gpu *gpu)
>>>       /* ..which means "always" on A7xx, also for BV shadow */
>>>       if (adreno_is_a7xx(adreno_gpu)) {
>>>               gpu_write64(gpu, REG_A7XX_CP_BV_RB_RPTR_ADDR,
>>> -                         rbmemptr(gpu->rb[0], bv_fence));
>>> +                         rbmemptr(gpu->rb[0], bv_rptr));
>>>       }
>>>
>>>       /* Always come up on rb 0 */
>>> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h b/drivers/gpu/drm/msm/msm_ringbuffer.h
>>> index 0d6beb8cd39a..40791b2ade46 100644
>>> --- a/drivers/gpu/drm/msm/msm_ringbuffer.h
>>> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.h
>>> @@ -31,6 +31,7 @@ struct msm_rbmemptrs {
>>>       volatile uint32_t rptr;
>>>       volatile uint32_t fence;
>>>       /* Introduced on A7xx */
>>> +     volatile uint32_t bv_rptr;
>>
>> This is never initialized or assigned any value, no?
>>
>> Konrad
> 
> Neither is the original (retroactively BR) shadow RPTR, except
> apparently on suspend (no idea why). It's written by the GPU as it
> reads the ringbuffer, because CP_BV_RPTR_ADDR is set to its address.
> For the BV shadow RPTR, we aren't really using it for anything (and
> neither is kgsl) so we just need to point the register to a valid
> "dummy" address that isn't used by anything else.

Alright, thanks

Konrad
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index bcaec86ac67a..32a4faa93d7f 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1132,7 +1132,7 @@  static int hw_init(struct msm_gpu *gpu)
 	/* ..which means "always" on A7xx, also for BV shadow */
 	if (adreno_is_a7xx(adreno_gpu)) {
 		gpu_write64(gpu, REG_A7XX_CP_BV_RB_RPTR_ADDR,
-			    rbmemptr(gpu->rb[0], bv_fence));
+			    rbmemptr(gpu->rb[0], bv_rptr));
 	}
 
 	/* Always come up on rb 0 */
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h b/drivers/gpu/drm/msm/msm_ringbuffer.h
index 0d6beb8cd39a..40791b2ade46 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.h
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.h
@@ -31,6 +31,7 @@  struct msm_rbmemptrs {
 	volatile uint32_t rptr;
 	volatile uint32_t fence;
 	/* Introduced on A7xx */
+	volatile uint32_t bv_rptr;
 	volatile uint32_t bv_fence;
 
 	volatile struct msm_gpu_submit_stats stats[MSM_GPU_SUBMIT_STATS_COUNT];