[PATCH 0/6] Introduce a generic function to get the CSB buffer

[Rodrigo Siqueira]

On 04/07, Alex Deucher wrote:
> On Sun, Apr 6, 2025 at 7:07 PM Rodrigo Siqueira <siqueira at igalia.com> wrote:
> >
> > This patchset was inspired and made on top of the below series:
> >
> > https://lore.kernel.org/amd-gfx/20250319162225.3775315-1-alexander.deucher@amd.com/
> >
> > In the above series, there is a bug fix in many functions named
> > gfx_vX_0_get_csb_buffer (where X ranges from 6 to 11). After closely
> > looking at those functions, it became clear that most of the code could
> > be shared from gfx6 to gfx11. Aside from the code duplication removal,
> > this also improves maintainability since a bug fix in a shared function
> > can be propagated to all ASICs.
> >
> > The first patch of this series created one dedicated file for
> > encapsulating common GC registers (gc_common_offset.h); this series only
> > adds registers associated with the CSB. In the future, this file can
> > keep growing as we identify common registers to be shared in the
> > gc_common_offset.h.
> >
> > The second patch introduces the generic gfx_get_csb_buffer function,
> > which has the same implementation found in gfx_v10_0_get_csb_buffer and
> > gfx_v11_0_get_csb_buffer (these two functions have the same code). After
> > that, every patch is dedicated to absorbing one of the csb_buffer
> > functions from gfx from 9 to 6; notice that some adaptations were
> > required.
> 
> I don't really like the register header changes and moving all of the
> IP version specific logic into the common code.  These register
> headers are used in other places as well and moving some registers
> into a common header can get confusing and may lead to bugs later if
> other chips change the offset of these registers.

In that case, what do you think if I just abstract the first part of the
function for a V2? The first part is the same for all gfx from 6 to 11.
Something like this:

int gfx_get_pre_setup_csb_buffer(struct amdgpu_device *adev, volatile u32 *buffer)
{
       u32 count = 0, i;
       const struct cs_section_def *sect = NULL;
       const struct cs_extent_def *ext = NULL;
       int ctx_reg_offset;

       if (adev->gfx.rlc.cs_data == NULL)
               return 1;
       if (buffer == NULL)
               return 1;

       buffer[count++] = cpu_to_le32(PACKET3(PACKET3_PREAMBLE_CNTL, 0));
       buffer[count++] = cpu_to_le32(PACKET3_PREAMBLE_BEGIN_CLEAR_STATE);

       buffer[count++] = cpu_to_le32(PACKET3(PACKET3_CONTEXT_CONTROL, 1));
       buffer[count++] = cpu_to_le32(0x80000000);
       buffer[count++] = cpu_to_le32(0x80000000);

       for (sect = adev->gfx.rlc.cs_data; sect->section != NULL; ++sect) {
               for (ext = sect->section; ext->extent != NULL; ++ext) {
                       if (sect->id == SECT_CONTEXT) {
                               buffer[count++] =
                                       cpu_to_le32(PACKET3(PACKET3_SET_CONTEXT_REG, ext->reg_count));
                               buffer[count++] = cpu_to_le32(ext->reg_index -
                                               PACKET3_SET_CONTEXT_REG_START);
                               for (i = 0; i < ext->reg_count; i++)
                                       buffer[count++] = cpu_to_le32(ext->extent[i]);
                       }
               }
       }

       return 0;
}

Thanks

> 
> Alex
> 
> >
> > Thanks
> >
> > Rodrigo Siqueira (6):
> >   drm/amd/amdgpu: Create a headears to keep some common GC registers
> >   drm/amdgpu/gfx: Introduce generic gfx_get_csb_buffer
> >   drm/amdgpu/gfx: Integrate gfx_v9_0_get_csb_buffer into
> >     gfx_get_csb_buffer
> >   drm/amdgpu/gfx: Absorb gfx_v8_0_get_csb_buffer into gfx_get_csb_buffer
> >   drm/amdgpu/gfx: Assimilate gfx_v7_0_get_csb_buffer into
> >     gfx_get_csb_buffer
> >   drm/amdgpu/gfx: Merge gfx_v6_0_get_csb_buffer into gfx_get_csb_buffer
> >
> >  Documentation/gpu/amdgpu/amdgpu-glossary.rst  |   3 +
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c       | 101 ++++++++++++++++++
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h       |   1 +
> >  drivers/gpu/drm/amd/amdgpu/cik.c              |   2 +
> >  drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c        |  51 +--------
> >  drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c        |  53 +--------
> >  drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c         |  46 +-------
> >  drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c         |  70 +-----------
> >  drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c         |  51 +--------
> >  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c         |  43 +-------
> >  drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c         |   1 +
> >  drivers/gpu/drm/amd/amdgpu/si.c               |   2 +
> >  drivers/gpu/drm/amd/amdgpu/vi.c               |   2 +
> >  .../include/asic_reg/gc/gc_10_1_0_offset.h    |   3 -
> >  .../include/asic_reg/gc/gc_10_3_0_offset.h    |   3 -
> >  .../include/asic_reg/gc/gc_11_0_0_offset.h    |   2 -
> >  .../include/asic_reg/gc/gc_11_0_3_offset.h    |   2 -
> >  .../include/asic_reg/gc/gc_11_5_0_offset.h    |   2 -
> >  .../include/asic_reg/gc/gc_12_0_0_offset.h    |   2 -
> >  .../amd/include/asic_reg/gc/gc_9_0_offset.h   |   3 -
> >  .../amd/include/asic_reg/gc/gc_9_1_offset.h   |   3 -
> >  .../amd/include/asic_reg/gc/gc_9_2_1_offset.h |   3 -
> >  .../amd/include/asic_reg/gc/gc_9_4_2_offset.h |   2 -
> >  .../amd/include/asic_reg/gc/gc_9_4_3_offset.h |   2 -
> >  .../include/asic_reg/gc/gc_common_offset.h    |  11 ++
> >  .../drm/amd/include/asic_reg/gca/gfx_6_0_d.h  |   1 -
> >  .../drm/amd/include/asic_reg/gca/gfx_7_0_d.h  |   1 -
> >  .../drm/amd/include/asic_reg/gca/gfx_7_2_d.h  |   1 -
> >  .../drm/amd/include/asic_reg/gca/gfx_8_0_d.h  |   1 -
> >  .../drm/amd/include/asic_reg/gca/gfx_8_1_d.h  |   1 -
> >  30 files changed, 141 insertions(+), 328 deletions(-)
> >  create mode 100644 drivers/gpu/drm/amd/include/asic_reg/gc/gc_common_offset.h
> >
> > --
> > 2.49.0
> >

-- 
Rodrigo Siqueira