Re: [RFC PATCH v2 1/5] drm/amdgpu: add typed helpers for ring writeback slots

[Christian König]

On 7/17/26 05:30, Runyu Xiao wrote:
> AMDGPU ring writeback slots are currently accessed with a mix of direct
> pointer dereferences, casted u64 accesses, READ_ONCE()/WRITE_ONCE() on
> casted pointers,

Clear NAK to that.

Those differentiation are completely intentional and exits for documentation purposes.

> and atomic64_t casts.

Well that is clearly problematic, where do you see that?

> Add small typed helpers for reading and writing 32-bit and 64-bit ring
> writeback slots. This makes the intended slot width explicit at each
> call site and provides a single access model for the later cleanup
> patches in this series.

That's just nonsense.

The writeback pointers not only vary between 32 and 64bits but are sometimes full 256bits with all kind of information in them.

In general it should be a 32bit pointer which can be indexed and/or cast to 64bit if necessary.

Regards,
Christian.

> 
> This patch only introduces the helpers and does not change any existing
> call site behavior.
> 
> Signed-off-by: Runyu Xiao <runyu.xiao@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 8f28b3bd7..cdc855285 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -457,6 +457,26 @@ struct amdgpu_ring {
>  #define amdgpu_ring_patch_de(r, o) ((r)->funcs->patch_de((r), (o)))
>  #define amdgpu_ring_reset(r, v, f) (r)->funcs->reset((r), (v), (f))
>  
> +static inline u32 amdgpu_ring_wb_read32(const void *cpu_addr)
> +{
> +	return READ_ONCE(*(const u32 *)cpu_addr);
> +}
> +
> +static inline void amdgpu_ring_wb_write32(void *cpu_addr, u32 value)
> +{
> +	WRITE_ONCE(*(u32 *)cpu_addr, value);
> +}
> +
> +static inline u64 amdgpu_ring_wb_read64(const void *cpu_addr)
> +{
> +	return READ_ONCE(*(const u64 *)cpu_addr);
> +}
> +
> +static inline void amdgpu_ring_wb_write64(void *cpu_addr, u64 value)
> +{
> +	WRITE_ONCE(*(u64 *)cpu_addr, value);
> +}
> +
>  unsigned int amdgpu_ring_max_ibs(enum amdgpu_ring_type type);
>  int amdgpu_ring_alloc(struct amdgpu_ring *ring, unsigned ndw);
>  void amdgpu_ring_ib_begin(struct amdgpu_ring *ring);