RE: [PATCH] drm/amdgpu: disable ring_muxer if mcbp is off

[Zhu, Jiadong]

[AMD Official Use Only - General]

Acked-by: Jiadong Zhu 

-Original Message-
From: amd-gfx  On Behalf Of Christian 
König
Sent: Tuesday, February 27, 2024 12:02 AM
To: Alex Deucher ; Pelloux-Prayer, Pierre-Eric 

Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: disable ring_muxer if mcbp is off

Am 23.02.24 um 17:30 schrieb Alex Deucher:
> On Fri, Feb 23, 2024 at 4:48 AM Pierre-Eric Pelloux-Prayer
>  wrote:
>> Using the ring_muxer without preemption adds overhead for no reason
>> since mcbp cannot be triggered.
>>
>> Moving back to a single queue in this case also helps when high
>> priority app are used: in this case the gpu_scheduler priority
>> handling will work as expected - much better than ring_muxer with its
>> 2 independant schedulers competing for the same hardware queue.
>>
>> This change requires moving amdgpu_device_set_mcbp above
>> amdgpu_device_ip_early_init because we use adev->gfx.mcbp.
>>
>> Signed-off-by: Pierre-Eric Pelloux-Prayer
>> 
> Reviewed-by: Alex Deucher 

Acked-by: Christian König 

>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 ++--
>>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c  | 21 -
>>   2 files changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index d534e192e260..40516d24026c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -4054,13 +4054,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>  return r;
>>  }
>>
>> +   amdgpu_device_set_mcbp(adev);
>> +
>>  /* early init functions */
>>  r = amdgpu_device_ip_early_init(adev);
>>  if (r)
>>  return r;
>>
>> -   amdgpu_device_set_mcbp(adev);
>> -
>>  /* Get rid of things like offb */
>>  r = drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, 
>> &amdgpu_kms_driver);
>>  if (r)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> index 169d45268ef6..f682f830f7f6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> @@ -2080,7 +2080,7 @@ static int gfx_v9_0_sw_init(void *handle)
>>  ring->doorbell_index =
>> adev->doorbell_index.gfx_ring0 << 1;
>>
>>  /* disable scheduler on the real ring */
>> -   ring->no_scheduler = true;
>> +   ring->no_scheduler = adev->gfx.mcbp;
>>  ring->vm_hub = AMDGPU_GFXHUB(0);
>>  r = amdgpu_ring_init(adev, ring, 1024, &adev->gfx.eop_irq,
>>
>> AMDGPU_CP_IRQ_GFX_ME0_PIPE0_EOP, @@ -2090,7 +2090,7 @@ static int 
>> gfx_v9_0_sw_init(void *handle)
>>  }
>>
>>  /* set up the software rings */
>> -   if (adev->gfx.num_gfx_rings) {
>> +   if (adev->gfx.mcbp && adev->gfx.num_gfx_rings) {
>>  for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++) {
>>  ring = &adev->gfx.sw_gfx_ring[i];
>>  ring->ring_obj = NULL; @@ -2181,7 +2181,7 @@
>> static int gfx_v9_0_sw_fini(void *handle)
>>  int i;
>>  struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>
>> -   if (adev->gfx.num_gfx_rings) {
>> +   if (adev->gfx.mcbp && adev->gfx.num_gfx_rings) {
>>  for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++)
>>  amdgpu_ring_fini(&adev->gfx.sw_gfx_ring[i]);
>>  amdgpu_ring_mux_fini(&adev->gfx.muxer);
>> @@ -5910,11 +5910,14 @@ static int gfx_v9_0_eop_irq(struct
>> amdgpu_device *adev,
>>
>>  switch (me_id) {
>>  case 0:
>> -   if (adev->gfx.num_gfx_rings &&
>> -   
>> !amdgpu_mcbp_handle_trailing_fence_irq(&adev->gfx.muxer)) {
>> -   /* Fence signals are handled on the software rings*/
>> -   for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++)
>> -   
>> amdgpu_fence_process(&adev->gfx.sw_gfx_ring[i]);
>> +   if (adev->gfx.num_gfx_rings) {
>> +   if (!adev->gfx.mcbp) {
>> +   amdgpu_fence_process(&

Re: [PATCH] drm/amdgpu: disable ring_muxer if mcbp is off

[Christian König]

Am 23.02.24 um 17:30 schrieb Alex Deucher:

On Fri, Feb 23, 2024 at 4:48 AM Pierre-Eric Pelloux-Prayer
 wrote:

Using the ring_muxer without preemption adds overhead for no
reason since mcbp cannot be triggered.

Moving back to a single queue in this case also helps when
high priority app are used: in this case the gpu_scheduler
priority handling will work as expected - much better than
ring_muxer with its 2 independant schedulers competing for
the same hardware queue.

This change requires moving amdgpu_device_set_mcbp above
amdgpu_device_ip_early_init because we use adev->gfx.mcbp.

Signed-off-by: Pierre-Eric Pelloux-Prayer 

Reviewed-by: Alex Deucher 


Acked-by: Christian König 




---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 ++--
  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c  | 21 -
  2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index d534e192e260..40516d24026c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4054,13 +4054,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 return r;
 }

+   amdgpu_device_set_mcbp(adev);
+
 /* early init functions */
 r = amdgpu_device_ip_early_init(adev);
 if (r)
 return r;

-   amdgpu_device_set_mcbp(adev);
-
 /* Get rid of things like offb */
 r = drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, 
&amdgpu_kms_driver);
 if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index 169d45268ef6..f682f830f7f6 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -2080,7 +2080,7 @@ static int gfx_v9_0_sw_init(void *handle)
 ring->doorbell_index = adev->doorbell_index.gfx_ring0 << 1;

 /* disable scheduler on the real ring */
-   ring->no_scheduler = true;
+   ring->no_scheduler = adev->gfx.mcbp;
 ring->vm_hub = AMDGPU_GFXHUB(0);
 r = amdgpu_ring_init(adev, ring, 1024, &adev->gfx.eop_irq,
  AMDGPU_CP_IRQ_GFX_ME0_PIPE0_EOP,
@@ -2090,7 +2090,7 @@ static int gfx_v9_0_sw_init(void *handle)
 }

 /* set up the software rings */
-   if (adev->gfx.num_gfx_rings) {
+   if (adev->gfx.mcbp && adev->gfx.num_gfx_rings) {
 for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++) {
 ring = &adev->gfx.sw_gfx_ring[i];
 ring->ring_obj = NULL;
@@ -2181,7 +2181,7 @@ static int gfx_v9_0_sw_fini(void *handle)
 int i;
 struct amdgpu_device *adev = (struct amdgpu_device *)handle;

-   if (adev->gfx.num_gfx_rings) {
+   if (adev->gfx.mcbp && adev->gfx.num_gfx_rings) {
 for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++)
 amdgpu_ring_fini(&adev->gfx.sw_gfx_ring[i]);
 amdgpu_ring_mux_fini(&adev->gfx.muxer);
@@ -5910,11 +5910,14 @@ static int gfx_v9_0_eop_irq(struct amdgpu_device *adev,

 switch (me_id) {
 case 0:
-   if (adev->gfx.num_gfx_rings &&
-   !amdgpu_mcbp_handle_trailing_fence_irq(&adev->gfx.muxer)) {
-   /* Fence signals are handled on the software rings*/
-   for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++)
-   amdgpu_fence_process(&adev->gfx.sw_gfx_ring[i]);
+   if (adev->gfx.num_gfx_rings) {
+   if (!adev->gfx.mcbp) {
+   amdgpu_fence_process(&adev->gfx.gfx_ring[0]);
+   } else if 
(!amdgpu_mcbp_handle_trailing_fence_irq(&adev->gfx.muxer)) {
+   /* Fence signals are handled on the software 
rings*/
+   for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++)
+   
amdgpu_fence_process(&adev->gfx.sw_gfx_ring[i]);
+   }
 }
 break;
 case 1:
@@ -7051,7 +7054,7 @@ static void gfx_v9_0_set_ring_funcs(struct amdgpu_device 
*adev)
 for (i = 0; i < adev->gfx.num_gfx_rings; i++)
 adev->gfx.gfx_ring[i].funcs = &gfx_v9_0_ring_funcs_gfx;

-   if (adev->gfx.num_gfx_rings) {
+   if (adev->gfx.mcbp && adev->gfx.num_gfx_rings) {
 for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++)
 adev->gfx.sw_gfx_ring[i].funcs = 
&gfx_v9_0_sw_ring_funcs_gfx;
 }
--
2.40.1

Re: [PATCH] drm/amdgpu: disable ring_muxer if mcbp is off

[Alex Deucher]

On Fri, Feb 23, 2024 at 4:48 AM Pierre-Eric Pelloux-Prayer
 wrote:
>
> Using the ring_muxer without preemption adds overhead for no
> reason since mcbp cannot be triggered.
>
> Moving back to a single queue in this case also helps when
> high priority app are used: in this case the gpu_scheduler
> priority handling will work as expected - much better than
> ring_muxer with its 2 independant schedulers competing for
> the same hardware queue.
>
> This change requires moving amdgpu_device_set_mcbp above
> amdgpu_device_ip_early_init because we use adev->gfx.mcbp.
>
> Signed-off-by: Pierre-Eric Pelloux-Prayer 

Reviewed-by: Alex Deucher 

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 ++--
>  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c  | 21 -
>  2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index d534e192e260..40516d24026c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4054,13 +4054,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> return r;
> }
>
> +   amdgpu_device_set_mcbp(adev);
> +
> /* early init functions */
> r = amdgpu_device_ip_early_init(adev);
> if (r)
> return r;
>
> -   amdgpu_device_set_mcbp(adev);
> -
> /* Get rid of things like offb */
> r = drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, 
> &amdgpu_kms_driver);
> if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 169d45268ef6..f682f830f7f6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -2080,7 +2080,7 @@ static int gfx_v9_0_sw_init(void *handle)
> ring->doorbell_index = adev->doorbell_index.gfx_ring0 << 1;
>
> /* disable scheduler on the real ring */
> -   ring->no_scheduler = true;
> +   ring->no_scheduler = adev->gfx.mcbp;
> ring->vm_hub = AMDGPU_GFXHUB(0);
> r = amdgpu_ring_init(adev, ring, 1024, &adev->gfx.eop_irq,
>  AMDGPU_CP_IRQ_GFX_ME0_PIPE0_EOP,
> @@ -2090,7 +2090,7 @@ static int gfx_v9_0_sw_init(void *handle)
> }
>
> /* set up the software rings */
> -   if (adev->gfx.num_gfx_rings) {
> +   if (adev->gfx.mcbp && adev->gfx.num_gfx_rings) {
> for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++) {
> ring = &adev->gfx.sw_gfx_ring[i];
> ring->ring_obj = NULL;
> @@ -2181,7 +2181,7 @@ static int gfx_v9_0_sw_fini(void *handle)
> int i;
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> -   if (adev->gfx.num_gfx_rings) {
> +   if (adev->gfx.mcbp && adev->gfx.num_gfx_rings) {
> for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++)
> amdgpu_ring_fini(&adev->gfx.sw_gfx_ring[i]);
> amdgpu_ring_mux_fini(&adev->gfx.muxer);
> @@ -5910,11 +5910,14 @@ static int gfx_v9_0_eop_irq(struct amdgpu_device 
> *adev,
>
> switch (me_id) {
> case 0:
> -   if (adev->gfx.num_gfx_rings &&
> -   !amdgpu_mcbp_handle_trailing_fence_irq(&adev->gfx.muxer)) 
> {
> -   /* Fence signals are handled on the software rings*/
> -   for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++)
> -   
> amdgpu_fence_process(&adev->gfx.sw_gfx_ring[i]);
> +   if (adev->gfx.num_gfx_rings) {
> +   if (!adev->gfx.mcbp) {
> +   amdgpu_fence_process(&adev->gfx.gfx_ring[0]);
> +   } else if 
> (!amdgpu_mcbp_handle_trailing_fence_irq(&adev->gfx.muxer)) {
> +   /* Fence signals are handled on the software 
> rings*/
> +   for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++)
> +   
> amdgpu_fence_process(&adev->gfx.sw_gfx_ring[i]);
> +   }
> }
> break;
> case 1:
> @@ -7051,7 +7054,7 @@ static void gfx_v9_0_set_ring_funcs(struct 
> amdgpu_device *adev)
> for (i = 0; i < adev->gfx.num_gfx_rings; i++)
> adev->gfx.gfx_ring[i].funcs = &gfx_v9_0_ring_funcs_gfx;
>
> -   if (adev->gfx.num_gfx_rings) {
> +   if (adev->gfx.mcbp && adev->gfx.num_gfx_rings) {
> for (i = 0; i < GFX9_NUM_SW_GFX_RINGS; i++)
> adev->gfx.sw_gfx_ring[i].funcs = 
> &gfx_v9_0_sw_ring_funcs_gfx;
> }
> --
> 2.40.1
>