On 2024-11-28 10:21, Lazar, Lijo wrote: > > > On 11/28/2024 8:12 PM, Felix Kuehling wrote: >> >> >> On 2024-11-27 22:45, Lazar, Lijo wrote: >>> >>> >>> On 11/28/2024 5:43 AM, Felix Kuehling wrote: >>>> >>>> On 2024-11-18 00:34, Lijo Lazar wrote: >>>>> Write pointer could be 32-bit or 64-bit. Use the correct size during >>>>> initialization. >>>>> >>>>> Signed-off-by: Lijo Lazar <lijo.lazar@xxxxxxx> >>>>> --- >>>>> drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c b/drivers/ >>>>> gpu/drm/amd/amdkfd/kfd_kernel_queue.c >>>>> index 4843dcb9a5f7..d6037577c532 100644 >>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c >>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c >>>>> @@ -125,7 +125,7 @@ static bool kq_initialize(struct kernel_queue *kq, >>>>> struct kfd_node *dev, >>>>> memset(kq->pq_kernel_addr, 0, queue_size); >>>>> memset(kq->rptr_kernel, 0, sizeof(*kq->rptr_kernel)); >>>>> - memset(kq->wptr_kernel, 0, sizeof(*kq->wptr_kernel)); >>>>> + memset(kq->wptr_kernel, 0, dev->kfd->device_info.doorbell_size); >>>> >>>> It would be safer and cleaner to just initialize kq->wptr64_kernel, >>>> which is always 64 bit and aliases kq->wptr_kernel. >>>> >>> >>> It's done this way because of aliasing. The size requested is >>> doorbell_size which could be 4 or 8 bytes. >>> >>> By safer, do you mean to have an if..else check in case the aliasing is >>> taken out? >> >> Cleaner because the size of the memset would match the size of the variable. And safer because it clears the entire wptr, regardless of the doorbell size. >> >> That said, it doesn't really matter because the whole kq structure is allocated with kzalloc just before calling kq_initialize. So maybe we should just remove all those redundant memset(kq->field, 0, size) calls. >> > > This is not related to kzalloc of kq structure. This is actually > initializing write pointer value to 0. > > The sequence is - > Allocate memory for write pointer (kfd_gtt_sa_allocate) > Assign kq wptr to the allocated pointer > Set wptr to 0 (Initial write pointer value). > > The last step should actually be based on 4 byte or 8byte, this patch is > for that. If gtt_sa_allocate is already getting a cleared memory, then > this can be skipped as well. OK, sorry, I was reading that wrong. You're right. kfd_gtt_sa_allocate does not clear memory. And clearing it with the same size as the allocation is the right thing to do. The patch is Reviewed-by: Felix Kuehling <felix.kuehling@xxxxxxx> > > Thanks, > Lijo > >> Regards, >> Felix >> >>> >>> Thanks, >>> Lijo >>> >>>> Regards, >>>> Felix >>>> >>>> >>>> >>>>> prop.queue_size = queue_size; >>>>> prop.is_interop = false; >>> >> >
