Re: [PATCH 1/1] drm/amdkfd: Do not ignore requested queue size during allocation

[Felix Kuehling <felix.kuehling-5C7GfCeVMHo@public.gmane.org>]

On 2017-11-21 06:44 AM, Oded Gabbay wrote:
> Thanks Felix for catching that, For some reason I remembered  EOP
> buffer should be the same size of the queue.

The EOP queue size is hard-coded to prop.eop_ring_buffer_size =
PAGE_SIZE for kernel queues in initialize in kfd_kernel_queue.c. I'm not
too familiar with the HW/FW details. But I see this comment in
kfd_mqd_manager_vi.c:

        /*
         * HW does not clamp this field correctly. Maximum EOP queue size
         * is constrained by per-SE EOP done signal count, which is 8-bit.
         * Limit is 0xFF EOP entries (= 0x7F8 dwords). CP will not submit
         * more than (EOP entry count - 1) so a queue size of 0x800 dwords
         * is safe, giving a maximum field value of 0xA.
         */

With that the maximum possible EOP queue size would be two pages,
regardless of the queue size.

> Then we can remove the queue size parameter from that function ?

Not the way the code is currently organized. Currently struct
kernel_queue_ops is shared for ASIC-independent and ASIC-specific queue
ops. The ASIC-independent initialize function in kfd_kernel_queue.c
still needs this parameter.

That said, the kernel_queue stuff could be cleaned up a bit in general.
IMO the hardware-independent functions don't really need to be called
through function pointers. The ASIC-specific function pointers don't
need to be in the kernel_queue structure, they could be in kfd_dev.

Regards,
  Felix

>
> On Mon, Nov 20, 2017 at 9:22 PM, Felix Kuehling <felix.kuehling@amd.com> wrote:
>> I think this patch is not correct. The EOP-mem is not associated with
>> the queue size. The EOP buffer is a separate buffer used by the firmware
>> to handle command completion. As I understand it, this allows more
>> concurrency, while still making it look like all commands in the queue
>> are completing in order.
>>
>> Regards,
>>   Felix
>>
>>
>> On 2017-11-19 03:19 AM, Oded Gabbay wrote:
>>> On Thu, Nov 16, 2017 at 11:36 PM, Jan Vesely <jan.vesely@rutgers.edu> wrote:
>>>> Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c
>>>> index f1d48281e322..b3bee39661ab 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c
>>>> @@ -37,15 +37,16 @@ static bool initialize_vi(struct kernel_queue *kq, struct kfd_dev *dev,
>>>>                         enum kfd_queue_type type, unsigned int queue_size)
>>>>  {
>>>>         int retval;
>>>> +       unsigned int size = ALIGN(queue_size, PAGE_SIZE);
>>>>
>>>> -       retval = kfd_gtt_sa_allocate(dev, PAGE_SIZE, &kq->eop_mem);
>>>> +       retval = kfd_gtt_sa_allocate(dev, size, &kq->eop_mem);
>>>>         if (retval != 0)
>>>>                 return false;
>>>>
>>>>         kq->eop_gpu_addr = kq->eop_mem->gpu_addr;
>>>>         kq->eop_kernel_addr = kq->eop_mem->cpu_ptr;
>>>>
>>>> -       memset(kq->eop_kernel_addr, 0, PAGE_SIZE);
>>>> +       memset(kq->eop_kernel_addr, 0, size);
>>>>
>>>>         return true;
>>>>  }
>>>> --
>>>> 2.13.6
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> Thanks!
>>> Applied to -next tree
>>> Oded
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx