[PATCH 1/5] drm/amdgpu: Expand kernel-doc in amdgpu_ring

[Rodrigo Siqueira]

On 08/18, Christian König wrote:
> On 16.08.25 17:31, Rodrigo Siqueira wrote:
> > Expand the kernel-doc about amdgpu_ring and add some tiny improvements.
> > 
> > Cc: Alex Deucher <alexander.deucher at amd.com>
> > Cc: Christian König <christian.koenig at amd.com>
> > Cc: Timur Kristóf <timur.kristof at gmail.com>
> > Signed-off-by: Rodrigo Siqueira <siqueira at igalia.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 15 ++++++++++++---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  2 ++
> >  2 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > index 6379bb25bf5c..78fd324c84e9 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > @@ -75,8 +75,16 @@ unsigned int amdgpu_ring_max_ibs(enum amdgpu_ring_type type)
> >   * @ring: amdgpu_ring structure holding ring information
> >   * @ndw: number of dwords to allocate in the ring buffer
> >   *
> > - * Allocate @ndw dwords in the ring buffer (all asics).
> > - * Returns 0 on success, error on failure.
> > + * Allocate @ndw dwords in the ring buffer (it works in all ASICs). When
> > + * inspecting the code, you may encounter places where this function is called
> > + * amdgpu_ring_alloc(ring, X + Y + Z) where X, Y, and Z are integer numbers.
> > + * This is a way to show how many dwords operations will be inserted in the
> > + * ring. For example, if gfx_v9_0_wait_reg_mem(), amdgpu_ring_emit_reg_wait(),
> > + * amdgpu_ring_emit_wreg(), and amdgpu_ring_emit_fence() will be called, before
> > + * that you will see amdgpu_ring_alloc(ring, 7 + 7 + 5 + 8).
> 
> Well the rest of the patch is certainly useful, but that here made me chuckle. Isn't that obvious?

Hi,

Interesting point. Tbh, it wasn't obvious to me the first time I saw it,
and I had to ask about it in one of the patch reviews. Perhaps this was
not evident to me because I'm new to this part of the code. I think a
kernel-doc in this function with this explanation can be helpful to some
other people looking into the code base for the first time, and might
also reduce one extra question to the maintainers. However, if you think
this is not necessary, I can drop it in the V2.

> 
> > + *
> > + * Returns:
> > + * 0 on success, error on failure.
> 
> We should probably adjust the return value of the function from -ENOMEM to -EINVAL when the number of the requested DW exceeds the maximum and document that here.

Since this will change the function return, I'll address it in another
patch.

Thanks

> 
> Apart from that looks good to me.
> 
> Christian.
> 
> >   */
> >  int amdgpu_ring_alloc(struct amdgpu_ring *ring, unsigned int ndw)
> >  {
> > @@ -122,7 +130,8 @@ static void amdgpu_ring_alloc_reemit(struct amdgpu_ring *ring, unsigned int ndw)
> >  		ring->funcs->begin_use(ring);
> >  }
> >  
> > -/** amdgpu_ring_insert_nop - insert NOP packets
> > +/**
> > + * amdgpu_ring_insert_nop - insert NOP packets
> >   *
> >   * @ring: amdgpu_ring structure holding ring information
> >   * @count: the number of NOP packets to insert
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > index 7670f5d82b9e..d27dbb3c109f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > @@ -62,6 +62,8 @@ enum amdgpu_ring_priority_level {
> >  #define AMDGPU_FENCE_FLAG_64BIT         (1 << 0)
> >  #define AMDGPU_FENCE_FLAG_INT           (1 << 1)
> >  #define AMDGPU_FENCE_FLAG_TC_WB_ONLY    (1 << 2)
> > +
> > +/* Ensure the execution in case of preemption or reset */
> >  #define AMDGPU_FENCE_FLAG_EXEC          (1 << 3)
> >  
> >  #define to_amdgpu_ring(s) container_of((s), struct amdgpu_ring, sched)
> 

-- 
Rodrigo Siqueira