On Tue, Apr 22, 2025 at 9:59 AM Rodrigo Siqueira <sique...@igalia.com> wrote:
>
> On 04/22, Alex Deucher wrote:
> > On Mon, Apr 21, 2025 at 6:24 PM Rodrigo Siqueira <sique...@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
I just tried this with `make htmldocs` and there was no warning, so
maybe it's not an issue.
>
> 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?
In this case the name of the data structure came from the initial use
case. It is really just a tiny sub-allocator when you need few bytes
of GPU accessible memory. We originally used it for the write back
mechanism supported by various engines on the GPU. The writeback
mechanism is basically a way for the engine to shadow certain state to
memory. E.g., the engine may store its ring buffer read pointer in a
register, but it can shadow this value to memory for easier access by
the CPU. In addition to easier CPU access, the driver would use these
shadowed values to avoid register access all together, (e.g., if the
block is power gated, reading the register may be problematic).
>
> > better to make then as regular comments rather than kerneldoc.
> >
> > Alex
> >
> > >
> > > Signed-off-by: Rodrigo Siqueira <sique...@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?
XCC = Accelerator Core Complex. Similar to a CPU Core Complex on CPU.
XCP = Accelerator Core Partition. A subset of the GPU cores that can
be used independently from the rest.
Alex
>
> Thanks
>
> > > --
> > > 2.49.0
> > >
>
> --
> Rodrigo Siqueira
