[PATCH v2] drm/amdgpu: Add documentation to some parts of the AMDGPU ring and wb

[Rodrigo Siqueira]

On 04/22, Alex Deucher wrote:
> On Mon, Apr 21, 2025 at 6:24 PM Rodrigo Siqueira <siqueira at igalia.com> wrote:
> >
> > Add some random documentation associated with the ring buffer
> > manipulations and writeback.
> 
> I think this will result in documentation warnings if not all of the
> elements in the structure are documented?  If so, maybe it would be

This warning will likely be triggered only per struct, right? For the
case of the struct amdgpu_wb I can try to complete all the missing
fields for the next version. Regarding the writeback struct, I'm only
familiar with the display writeback where DCN writes the same data from
scanout in a memory buffer (at a scanout time). Does this writeback
behave similarly to the one from DCN?

> better to make then as regular comments rather than kerneldoc.
> 
> Alex
> 
> >
> > Signed-off-by: Rodrigo Siqueira <siqueira at igalia.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu.h      | 28 +++++++++++++++++-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 37 ++++++++++++++++++++++++
> >  2 files changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index cc26cf1bd843..6d2ae8d027e5 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -522,9 +522,35 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv);
> >
> >  struct amdgpu_wb {
> >         struct amdgpu_bo        *wb_obj;
> > +
> > +       /**
> > +        * @wb:
> > +        *
> > +        * Pointer to the first writeback slot. In terms of CPU address
> > +        * this value can be accessed directly by using the offset as an index.
> > +        * For the GPU address, it is necessary to use gpu_addr and the offset.
> > +        */
> >         volatile uint32_t       *wb;
> > +
> > +       /**
> > +        * @gpu_addr:
> > +        *
> > +        * Writeback base address in the GPU.
> > +        */
> >         uint64_t                gpu_addr;
> > -       u32                     num_wb; /* Number of wb slots actually reserved for amdgpu. */
> > +
> > +       /**
> > +        * @num_wb:
> > +        *
> > +        * Number of writeback slots reserved for amdgpu.
> > +        */
> > +       u32                     num_wb;
> > +
> > +       /**
> > +        * @used:
> > +        *
> > +        * Track the writeback slot already used.
> > +        */
> >         unsigned long           used[DIV_ROUND_UP(AMDGPU_MAX_WB, BITS_PER_LONG)];
> >         spinlock_t              lock;
> >  };
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > index ec4de8df34e7..20805dacd66c 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > @@ -241,6 +241,9 @@ struct amdgpu_ring_funcs {
> >         bool (*is_guilty)(struct amdgpu_ring *ring);
> >  };
> >
> > +/**
> > + * amdgpu_ring - Holds ring information
> > + */
> >  struct amdgpu_ring {
> >         struct amdgpu_device            *adev;
> >         const struct amdgpu_ring_funcs  *funcs;
> > @@ -255,10 +258,44 @@ struct amdgpu_ring {
> >         u64                     wptr;
> >         u64                     wptr_old;
> >         unsigned                ring_size;
> > +
> > +       /**
> > +        * @max_dw:
> > +        *
> > +        * Maximum number of DWords for ring allocation. This information is
> > +        * provided at the ring initialization time, and each IP block can
> > +        * specify a specific value.
> > +        */
> >         unsigned                max_dw;
> > +
> > +       /**
> > +        * @count_dw:
> > +        *
> > +        * Count DWords: this value starts with the maximum amount of DWords
> > +        * supported by the ring. This value is updated based on the ring
> > +        * manipulation.
> > +        */
> >         int                     count_dw;
> >         uint64_t                gpu_addr;
> > +
> > +       /**
> > +        * @ptr_mask:
> > +        *
> > +        * Some IPs provide support for 64-bit pointers and others for 32-bit
> > +        * only; this behavior is component-specific and defined by the field
> > +        * support_64bit_ptr. If the IP block supports 64-bits, the mask
> > +        * 0xffffffffffffffff is set; otherwise, this value assumes buf_mask.
> > +        * Notice that this field is used to keep wptr under a valid range.
> > +        */
> >         uint64_t                ptr_mask;
> > +
> > +       /**
> > +        * @buf_mask:
> > +        *
> > +        * Buffer mask is a value used to keep wptr count under its
> > +        * thresholding. Buffer mask initialized during the ring buffer
> > +        * initialization time, and it is defined as (ring_size / 4) -1.
> > +        */
> >         uint32_t                buf_mask;
> >         u32                     idx;
> >         u32                     xcc_id;

Since we are here, what is this XCC and XCP? I guess those are focused
on datacenter GPUs, right? Also, what do those acronyms stand by?

Thanks

> > --
> > 2.49.0
> >

-- 
Rodrigo Siqueira