[PATCH v2 7/9] drm/amdgpu/gfx: Clean up gfx_v7_0_get_csb_buffer

[Alex Deucher]

On Mon, Apr 14, 2025 at 3:38 PM Rodrigo Siqueira <siqueira at igalia.com> wrote:
>
> On 04/13, Alex Deucher wrote:
> > On Sat, Apr 12, 2025 at 4:22 PM Rodrigo Siqueira <siqueira at igalia.com> wrote:
> > >
> > > CHIP_KAVERI, CHIP_KABINI, and CHIP_MULLINS have the same buffer
> > > manipulation as the default option in the switch case. Remove those
> > > specific manipulations and rely on the default behavior for them.
> > >
> > > Signed-off-by: Rodrigo Siqueira <siqueira at igalia.com>
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 9 ---------
> > >  1 file changed, 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > index 16b94ff5a959..4d8d68b737d1 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > @@ -3902,15 +3902,6 @@ static void gfx_v7_0_get_csb_buffer(struct amdgpu_device *adev,
> > >                 buffer[count++] = cpu_to_le32(0x16000012);
> > >                 buffer[count++] = cpu_to_le32(0x00000000);
> > >                 break;
> > > -       case CHIP_KAVERI:
> > > -               buffer[count++] = cpu_to_le32(0x00000000); /* XXX */
> > > -               buffer[count++] = cpu_to_le32(0x00000000);
> > > -               break;
> > > -       case CHIP_KABINI:
> > > -       case CHIP_MULLINS:
> > > -               buffer[count++] = cpu_to_le32(0x00000000); /* XXX */
> > > -               buffer[count++] = cpu_to_le32(0x00000000);
> > > -               break;
> >
> > These should be fixed rather than removed.  Actually, all of these
> > should be fixed up for all chips.  We should use the values calculated
> > by the driver similar to what we do in gfx_v8.  E.g.,
> >         buffer[count++] =
> > cpu_to_le32(adev->gfx.config.rb_config[0][0].raster_config);
> >         buffer[count++] =
> > cpu_to_le32(adev->gfx.config.rb_config[0][0].raster_config_1);
> >
>
> ok, I checked the code sequence, and fwiu gfx_v7_0_setup_rb() ->
> gfx_v7_0_raster_config() is called before gfx_v7_0_get_csb_buffer().
>  gfx_v7_0_raster_config() has the setup for all of the ASICs in this
> part. I also have a HAWAII and confirmed that using raster_config
> directly had the same value as the one hardcoded in this part. Anyway,
> I'll send a v3 with this fix.
>
> When I was looking into the code, it was not clear to me these things:
>
> 1. What is this config RB? Why is there a field for User RB? Also, what
>    RB stands for?

RB = Render Backends.  These are the render backends (color and depth
buffer handling) on the 3D engine (also called ROPs by a lot of
people).  Bad RBs are fused off and there is a harvest register the
driver reads to determine which RB(s) are fused off so that the driver
can configure the hardware state so that nothing gets sent to them.
There are also user harvest registers that the driver can program to
disable additional RBs, etc. for testing purposes.

> 2. Why there is raster_config and raster_config_1? Is raster_config_1
>    some sort of fallback?

The configuration just happens to require two registers to hold all of
the state.

Alex

>
> Thanks
>
> > Alex
> >
> > >         case CHIP_HAWAII:
> > >                 buffer[count++] = cpu_to_le32(0x3a00161a);
> > >                 buffer[count++] = cpu_to_le32(0x0000002e);
> > > --
> > > 2.49.0
> > >
>
> --
> Rodrigo Siqueira