[PATCH] drm/radeon: drop register readback in cayman_cp_int_cntl_setup

[PATCH] drm/radeon: drop register readback in cayman_cp_int_cntl_setup

[Lucas Stach]

The read is taking a considerable amount of time (about 50us on this
machine). The register does not ever hold anything other than the ring
ID that is updated in this exact function, so there is no need for
the read modify write cycle.

This chops off a big chunk of the time spent in hardirq disabled
context, as this function is called multiple times in the interrupt
handler. With this change applied radeon won't show up in the list
of the worst IRQ latency offenders anymore, where it was a regular
before.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 drivers/gpu/drm/radeon/ni.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
index 103fc86..a0d4a05 100644
--- a/drivers/gpu/drm/radeon/ni.c
+++ b/drivers/gpu/drm/radeon/ni.c
@@ -1396,9 +1396,7 @@ static void cayman_pcie_gart_fini(struct radeon_device *rdev)
 void cayman_cp_int_cntl_setup(struct radeon_device *rdev,
 			      int ring, u32 cp_int_cntl)
 {
-	u32 srbm_gfx_cntl = RREG32(SRBM_GFX_CNTL) & ~3;
-
-	WREG32(SRBM_GFX_CNTL, srbm_gfx_cntl | (ring & 3));
+	WREG32(SRBM_GFX_CNTL, RINGID(ring));
 	WREG32(CP_INT_CNTL, cp_int_cntl);
 }
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: drop register readback in cayman_cp_int_cntl_setup

[Christian König]

Am 24.10.2016 um 23:32 schrieb Lucas Stach:
> The read is taking a considerable amount of time (about 50us on this
> machine). The register does not ever hold anything other than the ring
> ID that is updated in this exact function, so there is no need for
> the read modify write cycle.
>
> This chops off a big chunk of the time spent in hardirq disabled
> context, as this function is called multiple times in the interrupt
> handler. With this change applied radeon won't show up in the list
> of the worst IRQ latency offenders anymore, where it was a regular
> before.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>

Ups, and to make it even worse SRBM_GFX_CNTL is explicitly documented to 
be a write only register.

That it takes a considerable amount of time is probably because the SRBM 
runs into an error condition when you read it.

So patch is Reviewed-by: Christian König <christian.koenig@amd.com> and 
please also add a CC:stable on that.

Do we have other occasions where we try to use a read modify write cycle?

Regards,
Christian.

> ---
>   drivers/gpu/drm/radeon/ni.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
> index 103fc86..a0d4a05 100644
> --- a/drivers/gpu/drm/radeon/ni.c
> +++ b/drivers/gpu/drm/radeon/ni.c
> @@ -1396,9 +1396,7 @@ static void cayman_pcie_gart_fini(struct radeon_device *rdev)
>   void cayman_cp_int_cntl_setup(struct radeon_device *rdev,
>   			      int ring, u32 cp_int_cntl)
>   {
> -	u32 srbm_gfx_cntl = RREG32(SRBM_GFX_CNTL) & ~3;
> -
> -	WREG32(SRBM_GFX_CNTL, srbm_gfx_cntl | (ring & 3));
> +	WREG32(SRBM_GFX_CNTL, RINGID(ring));
>   	WREG32(CP_INT_CNTL, cp_int_cntl);
>   }
>   


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: drop register readback in cayman_cp_int_cntl_setup

[Lucas Stach]

Am Dienstag, den 25.10.2016, 09:21 +0200 schrieb Christian König:
> Am 24.10.2016 um 23:32 schrieb Lucas Stach:
> > 
> > The read is taking a considerable amount of time (about 50us on
> > this
> > machine). The register does not ever hold anything other than the
> > ring
> > ID that is updated in this exact function, so there is no need for
> > the read modify write cycle.
> > 
> > This chops off a big chunk of the time spent in hardirq disabled
> > context, as this function is called multiple times in the interrupt
> > handler. With this change applied radeon won't show up in the list
> > of the worst IRQ latency offenders anymore, where it was a regular
> > before.
> > 
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> 
> Ups, and to make it even worse SRBM_GFX_CNTL is explicitly documented
> to 
> be a write only register.
> 
> That it takes a considerable amount of time is probably because the
> SRBM 
> runs into an error condition when you read it.
> 
> So patch is Reviewed-by: Christian König <christian.koenig@amd.com>
> and 
> please also add a CC:stable on that.
> 
> Do we have other occasions where we try to use a read modify write
> cycle?

No, that's the only occasion where this specific register is read back
(including newer generations that use it like cik).

Regards,
Lucas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: drop register readback in cayman_cp_int_cntl_setup

[Alex Deucher]

On Tue, Oct 25, 2016 at 7:01 AM, Lucas Stach <dev@lynxeye.de> wrote:
> Am Dienstag, den 25.10.2016, 09:21 +0200 schrieb Christian König:
>> Am 24.10.2016 um 23:32 schrieb Lucas Stach:
>> >
>> > The read is taking a considerable amount of time (about 50us on
>> > this
>> > machine). The register does not ever hold anything other than the
>> > ring
>> > ID that is updated in this exact function, so there is no need for
>> > the read modify write cycle.
>> >
>> > This chops off a big chunk of the time spent in hardirq disabled
>> > context, as this function is called multiple times in the interrupt
>> > handler. With this change applied radeon won't show up in the list
>> > of the worst IRQ latency offenders anymore, where it was a regular
>> > before.
>> >
>> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
>>
>> Ups, and to make it even worse SRBM_GFX_CNTL is explicitly documented
>> to
>> be a write only register.
>>
>> That it takes a considerable amount of time is probably because the
>> SRBM
>> runs into an error condition when you read it.
>>
>> So patch is Reviewed-by: Christian König <christian.koenig@amd.com>
>> and
>> please also add a CC:stable on that.
>>
>> Do we have other occasions where we try to use a read modify write
>> cycle?
>
> No, that's the only occasion where this specific register is read back
> (including newer generations that use it like cik).
>

Applied, including cc to stable.

Thanks,

Alex
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel