[PATCH 2/2] drm/amdkfd: Eliminate ops_asic_specific in kernel queue
Yong Zhao
yong.zhao at amd.com
Wed Nov 13 21:30:50 UTC 2019
I will change it to CHIP_MULLINS.
Yes , I also spotted the kq->ops cleanup, will send it out shortly.
Regards,
Yong
On 2019-11-13 2:31 p.m., Felix Kuehling wrote:
> See one comment inline. With that fixed, the series is
>
> Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>
>
> I could think of more follow-up cleanup while you're at it:
>
> 1. Can you see any reason why the kq->ops need to be function pointers.
> Looks to me like they are the same for all kernel queues, so those
> functions could be called without the pointer indirection.
> 2. The only think left in the ASIC-specific kfd_kernel_queue_*.c files
> is the PM4 packet writer functions that are called by the
> kfd_packet_manager. It may make sense to rename them to reflect
> that. Maybe kfd_packet_manager_*.c
>
> Regards,
> Felix
>
> On 2019-11-12 5:18 p.m., Yong Zhao wrote:
>> The ops_asic_specific function pointers are actually quite generic after
>> using a simple if condition. Eliminate it by code refactoring.
>>
>> Change-Id: Icb891289cca31acdbe2d2eea76a426f1738b9c08
>> Signed-off-by: Yong Zhao <Yong.Zhao at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 63 ++++++++-----------
>> drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.h | 4 --
>> .../gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c | 36 -----------
>> .../gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c | 48 --------------
>> 4 files changed, 26 insertions(+), 125 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>> index a750b1d110eb..59ee9053498c 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>> @@ -87,9 +87,17 @@ static bool initialize(struct kernel_queue *kq,
>> struct kfd_dev *dev,
>> kq->pq_kernel_addr = kq->pq->cpu_ptr;
>> kq->pq_gpu_addr = kq->pq->gpu_addr;
>> - retval = kq->ops_asic_specific.initialize(kq, dev, type,
>> queue_size);
>> - if (!retval)
>> - goto err_eop_allocate_vidmem;
>> + /* For CIK family asics, kq->eop_mem is not needed */
>> + if (dev->device_info->asic_family > CHIP_HAWAII) {
>
> This is not the correct condition to distinguish GFXv7 (CIK) vs v8
> (VI). CHIP_MULLINS comes after Hawaii, but it is also GFXv7 (CIK),
> even though KFD current doesn't support it.
>
>
>> + retval = kfd_gtt_sa_allocate(dev, PAGE_SIZE, &kq->eop_mem);
>> + if (retval != 0)
>> + goto err_eop_allocate_vidmem;
>> +
>> + 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);
>> + }
>> retval = kfd_gtt_sa_allocate(dev, sizeof(*kq->rptr_kernel),
>> &kq->rptr_mem);
>> @@ -200,7 +208,12 @@ static void uninitialize(struct kernel_queue *kq)
>> kfd_gtt_sa_free(kq->dev, kq->rptr_mem);
>> kfd_gtt_sa_free(kq->dev, kq->wptr_mem);
>> - kq->ops_asic_specific.uninitialize(kq);
>> +
>> + /* For CIK family asics, kq->eop_mem is Null, kfd_gtt_sa_free()
>> + * is able to handle NULL properly.
>> + */
>> + kfd_gtt_sa_free(kq->dev, kq->eop_mem);
>> +
>> kfd_gtt_sa_free(kq->dev, kq->pq);
>> kfd_release_kernel_doorbell(kq->dev,
>> kq->queue->properties.doorbell_ptr);
>> @@ -280,8 +293,15 @@ static void submit_packet(struct kernel_queue *kq)
>> }
>> pr_debug("\n");
>> #endif
>> -
>> - kq->ops_asic_specific.submit_packet(kq);
>> + if (kq->dev->device_info->doorbell_size == 8) {
>> + *kq->wptr64_kernel = kq->pending_wptr64;
>> + write_kernel_doorbell64(kq->queue->properties.doorbell_ptr,
>> + kq->pending_wptr64);
>> + } else {
>> + *kq->wptr_kernel = kq->pending_wptr;
>> + write_kernel_doorbell(kq->queue->properties.doorbell_ptr,
>> + kq->pending_wptr);
>> + }
>> }
>> static void rollback_packet(struct kernel_queue *kq)
>> @@ -310,42 +330,11 @@ struct kernel_queue *kernel_queue_init(struct
>> kfd_dev *dev,
>> kq->ops.submit_packet = submit_packet;
>> kq->ops.rollback_packet = rollback_packet;
>> - switch (dev->device_info->asic_family) {
>> - case CHIP_KAVERI:
>> - case CHIP_HAWAII:
>> - case CHIP_CARRIZO:
>> - case CHIP_TONGA:
>> - case CHIP_FIJI:
>> - case CHIP_POLARIS10:
>> - case CHIP_POLARIS11:
>> - case CHIP_POLARIS12:
>> - case CHIP_VEGAM:
>> - kernel_queue_init_vi(&kq->ops_asic_specific);
>> - break;
>> -
>> - case CHIP_VEGA10:
>> - case CHIP_VEGA12:
>> - case CHIP_VEGA20:
>> - case CHIP_RAVEN:
>> - case CHIP_RENOIR:
>> - case CHIP_ARCTURUS:
>> - case CHIP_NAVI10:
>> - case CHIP_NAVI12:
>> - case CHIP_NAVI14:
>> - kernel_queue_init_v9(&kq->ops_asic_specific);
>> - break;
>> - default:
>> - WARN(1, "Unexpected ASIC family %u",
>> - dev->device_info->asic_family);
>> - goto out_free;
>> - }
>> -
>> if (kq->ops.initialize(kq, dev, type, KFD_KERNEL_QUEUE_SIZE))
>> return kq;
>> pr_err("Failed to init kernel queue\n");
>> -out_free:
>> kfree(kq);
>> return NULL;
>> }
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.h
>> b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.h
>> index a9a35897d8b7..475e9499c0af 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.h
>> @@ -66,7 +66,6 @@ struct kernel_queue_ops {
>> struct kernel_queue {
>> struct kernel_queue_ops ops;
>> - struct kernel_queue_ops ops_asic_specific;
>> /* data */
>> struct kfd_dev *dev;
>> @@ -99,7 +98,4 @@ struct kernel_queue {
>> struct list_head list;
>> };
>> -void kernel_queue_init_vi(struct kernel_queue_ops *ops);
>> -void kernel_queue_init_v9(struct kernel_queue_ops *ops);
>> -
>> #endif /* KFD_KERNEL_QUEUE_H_ */
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
>> index 9e0eaf446bab..2de01009f1b6 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
>> @@ -27,42 +27,6 @@
>> #include "kfd_pm4_opcodes.h"
>> #include "gc/gc_10_1_0_sh_mask.h"
>> -static bool initialize_v9(struct kernel_queue *kq, struct kfd_dev
>> *dev,
>> - enum kfd_queue_type type, unsigned int queue_size)
>> -{
>> - int retval;
>> -
>> - retval = kfd_gtt_sa_allocate(dev, PAGE_SIZE, &kq->eop_mem);
>> - if (retval)
>> - 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);
>> -
>> - return true;
>> -}
>> -
>> -static void uninitialize_v9(struct kernel_queue *kq)
>> -{
>> - kfd_gtt_sa_free(kq->dev, kq->eop_mem);
>> -}
>> -
>> -static void submit_packet_v9(struct kernel_queue *kq)
>> -{
>> - *kq->wptr64_kernel = kq->pending_wptr64;
>> - write_kernel_doorbell64(kq->queue->properties.doorbell_ptr,
>> - kq->pending_wptr64);
>> -}
>> -
>> -void kernel_queue_init_v9(struct kernel_queue_ops *ops)
>> -{
>> - ops->initialize = initialize_v9;
>> - ops->uninitialize = uninitialize_v9;
>> - ops->submit_packet = submit_packet_v9;
>> -}
>> -
>> static int pm_map_process_v9(struct packet_manager *pm,
>> uint32_t *buffer, struct qcm_process_device *qpd)
>> {
>> 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 64f13f34d819..bed4d0ccb6b1 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c
>> @@ -26,54 +26,6 @@
>> #include "kfd_pm4_headers_vi.h"
>> #include "kfd_pm4_opcodes.h"
>> -static bool initialize_vi(struct kernel_queue *kq, struct kfd_dev
>> *dev,
>> - enum kfd_queue_type type, unsigned int queue_size);
>> -static void uninitialize_vi(struct kernel_queue *kq);
>> -static void submit_packet_vi(struct kernel_queue *kq);
>> -
>> -void kernel_queue_init_vi(struct kernel_queue_ops *ops)
>> -{
>> - ops->initialize = initialize_vi;
>> - ops->uninitialize = uninitialize_vi;
>> - ops->submit_packet = submit_packet_vi;
>> -}
>> -
>> -static bool initialize_vi(struct kernel_queue *kq, struct kfd_dev *dev,
>> - enum kfd_queue_type type, unsigned int queue_size)
>> -{
>> - int retval;
>> -
>> - /*For CIK family asics, kq->eop_mem is not needed */
>> - if (dev->device_info->asic_family <= CHIP_HAWAII)
>> - return true;
>> -
>> - retval = kfd_gtt_sa_allocate(dev, PAGE_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);
>> -
>> - return true;
>> -}
>> -
>> -static void uninitialize_vi(struct kernel_queue *kq)
>> -{
>> - /* For CIK family asics, kq->eop_mem is Null, kfd_gtt_sa_free()
>> - * is able to handle NULL properly.
>> - */
>> - kfd_gtt_sa_free(kq->dev, kq->eop_mem);
>> -}
>> -
>> -static void submit_packet_vi(struct kernel_queue *kq)
>> -{
>> - *kq->wptr_kernel = kq->pending_wptr;
>> - write_kernel_doorbell(kq->queue->properties.doorbell_ptr,
>> - kq->pending_wptr);
>> -}
>> -
>> unsigned int pm_build_pm4_header(unsigned int opcode, size_t
>> packet_size)
>> {
>> union PM4_MES_TYPE_3_HEADER header;
More information about the amd-gfx
mailing list