Hi Christian,
Thanks, that makes sense. I will rework this around typed rptr/wptr_cpu_addr accesses instead of the atomic64_t cast, and I will include the related rptr fixes in the same patch set. Thanks, Runyu >> >> >> Hi Christian, >> >> Thanks, understood. >> >> To make sure I rework this in the right direction: would you expect >> this reset path to do >> >> ring->wptr = 0; >> amdgpu_ring_set_wptr(ring); >> >> instead of writing wptr_cpu_addr directly? >> >> I am asking because amdgpu_ring_set_wptr() also updates the doorbell, >> so I want to confirm that this is the intended sequence for the >> reset/suspend case here. > >Yeah I was wondering the same thing. > >I think the correct approach would be to make both rptr_cpu_addr and >wptr_cpu_addr void* in the amdgpu_ring.h structure instead of u32* and then >cast that to either (u64*) or (u32*) depending on the ring type. > >The atomic64_t hack should really be removed. > >BTW Reading the rptr is wrong on multiple instances as well and should >probably be fixed in the same patch set. > >Regards, >Christian. > >> >> Thanks, >> Runyu >> >> On Tue, Jun 2, 2026 at 11:49:05AM +0200, Christian König wrote: >>> Clear NAK. >>> >>> The atomic64_t cast hack is just something we did for older >>> generations and is not something which is necessary nor should >>> be done here. >>> >>> What could be possible is that we need to use amdgpu_ring_set_wptr() >>> here to correctly distinguish between queues with 32bit and 64bit >>> wptrs. >> > > >> >> >> Hi Christian, >> >> Thanks, understood. >> >> To make sure I rework this in the right direction: would you expect >> this reset path to do >> >> ring->wptr = 0; >> amdgpu_ring_set_wptr(ring); >> >> instead of writing wptr_cpu_addr directly? >> >> I am asking because amdgpu_ring_set_wptr() also updates the doorbell, >> so I want to confirm that this is the intended sequence for the >> reset/suspend case here. > >Yeah I was wondering the same thing. > >I think the correct approach would be to make both rptr_cpu_addr and >wptr_cpu_addr void* in the amdgpu_ring.h structure instead of u32* and then >cast that to either (u64*) or (u32*) depending on the ring type. > >The atomic64_t hack should really be removed. > >BTW Reading the rptr is wrong on multiple instances as well and should >probably be fixed in the same patch set. > >Regards, >Christian. > >> >> Thanks, >> Runyu >> >> On Tue, Jun 2, 2026 at 11:49:05AM +0200, Christian König wrote: >>> Clear NAK. >>> >>> The atomic64_t cast hack is just something we did for older >>> generations and is not something which is necessary nor should >>> be done here. >>> >>> What could be possible is that we need to use amdgpu_ring_set_wptr() >>> here to correctly distinguish between queues with 32bit and 64bit >>> wptrs. >> > >
