On 04/14, Mario Limonciello wrote:
> On 4/12/2025 3:37 PM, Rodrigo Siqueira wrote:
> > Add some random documentation associated with the ring buffer
> > manipulations and writeback.
> >
> > Signed-off-by: Rodrigo Siqueira <sique...@igalia.com>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 29 ++++++++++++++++++-
> > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 37 ++++++++++++++++++++++++
> > 2 files changed, 65 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index 804d37703709..a8206ad210b4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -522,9 +522,36 @@ 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 access directly by using the offset as an
>
> 'accessed' not 'access '
>
> right?
>
> > + * 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 actually reserved for amdgpu.
>
> I don't think you need the word actually
>
> > + */
> > + 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..82972978c546 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 DWord for ring allocation. This information is
>
> DWords
>
> > + * 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;
>
Thanks Mario, I'll send a V2 with your suggestions.
--
Rodrigo Siqueira
