Re: [PATCH 2/2] drm/amdkfd: Eliminate ops_asic_specific in kernel queue

[Felix Kuehling <felix.kuehling@xxxxxxx>]

See one comment inline. With that fixed, the series is

Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>

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@xxxxxxx>
> ---
>   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;
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx