Re: [PATCH] drm/amdkfd: enable cooperative groups for gfx11

[Felix Kuehling <felix.kuehling@xxxxxxx>]

Am 2023-07-19 um 10:36 schrieb Jonathan Kim:
> MES can concurrently schedule queues on the device that require
> exclusive device access if marked exclusively_scheduled without the
> requirement of GWS.  Similar to the F32 HWS, MES will manage
> quality of service for these queues.
> Use this for cooperative groups since cooperative groups are device
> occupancy limited.
>
> Since some GFX11 devices can only be debugged with partial CUs, do not
> allow the debugging of cooperative groups on these devices as the CU
> occupancy limit will change on attach.
>
> In addition, zero initialize the MES add queue submission vector for MES
> initialization tests as we do not want these to be cooperative
> dispatches.
>
> v2: fix up indentation and comments.
> remove unnecessary perf warning on oversubscription.
> change 0 init to 0 memset to deal with padding.
>
> Signed-off-by: Jonathan Kim <jonathan.kim@xxxxxxx>

Sorry. More indentation nit-picks inline. With those fixed, the patch is

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


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c              |  2 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h              |  1 +
>   drivers/gpu/drm/amd/amdgpu/mes_v11_0.c               |  2 ++
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c             |  3 ++-
>   drivers/gpu/drm/amd/amdkfd/kfd_debug.c               |  3 ++-
>   drivers/gpu/drm/amd/amdkfd/kfd_device.c              |  6 +++++-
>   .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c    |  7 ++-----
>   .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c   | 12 ++++++++----
>   drivers/gpu/drm/amd/include/mes_v11_api_def.h        |  4 +++-
>   9 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> index f808841310fd..72ab6a838bb6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> @@ -642,6 +642,8 @@ int amdgpu_mes_add_hw_queue(struct amdgpu_device *adev, int gang_id,
>   	unsigned long flags;
>   	int r;
>   
> +	memset(&queue_input, 0, sizeof(struct mes_add_queue_input));
> +
>   	/* allocate the mes queue buffer */
>   	queue = kzalloc(sizeof(struct amdgpu_mes_queue), GFP_KERNEL);
>   	if (!queue) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> index 2d6ac30b7135..2053954a235c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> @@ -224,6 +224,7 @@ struct mes_add_queue_input {
>   	uint32_t	is_kfd_process;
>   	uint32_t	is_aql_queue;
>   	uint32_t	queue_size;
> +	uint32_t	exclusively_scheduled;
>   };
>   
>   struct mes_remove_queue_input {
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> index 1bdaa00c0b46..8e67e965f7ea 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> @@ -214,6 +214,8 @@ static int mes_v11_0_add_hw_queue(struct amdgpu_mes *mes,
>   	mes_add_queue_pkt.is_aql_queue = input->is_aql_queue;
>   	mes_add_queue_pkt.gds_size = input->queue_size;
>   
> +	mes_add_queue_pkt.exclusively_scheduled = input->exclusively_scheduled;
> +
>   	return mes_v11_0_submit_pkt_and_poll_completion(mes,
>   			&mes_add_queue_pkt, sizeof(mes_add_queue_pkt),
>   			offsetof(union MESAPI__ADD_QUEUE, api_status));
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 40ac093b5035..e0f9cf6dd8fd 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1487,7 +1487,8 @@ static int kfd_ioctl_alloc_queue_gws(struct file *filep,
>   		goto out_unlock;
>   	}
>   
> -	if (!kfd_dbg_has_gws_support(dev) && p->debug_trap_enabled) {
> +	if (p->debug_trap_enabled && (!kfd_dbg_has_gws_support(dev) ||
> +				      kfd_dbg_has_cwsr_workaround(dev))) {
>   		retval = -EBUSY;
>   		goto out_unlock;
>   	}
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
> index ccfc81f085ce..1f82caea59ba 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
> @@ -753,7 +753,8 @@ int kfd_dbg_trap_enable(struct kfd_process *target, uint32_t fd,
>   		if (!KFD_IS_SOC15(pdd->dev))
>   			return -ENODEV;
>   
> -		if (!kfd_dbg_has_gws_support(pdd->dev) && pdd->qpd.num_gws)
> +		if (pdd->qpd.num_gws && (!kfd_dbg_has_gws_support(pdd->dev) ||
> +					 kfd_dbg_has_cwsr_workaround(pdd->dev)))
>   			return -EBUSY;
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 0b3dc754e06b..ebc9674d3ce1 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -508,6 +508,7 @@ static int kfd_gws_init(struct kfd_node *node)
>   {
>   	int ret = 0;
>   	struct kfd_dev *kfd = node->kfd;
> +	uint32_t mes_rev = node->adev->mes.sched_version & AMDGPU_MES_VERSION_MASK;
>   
>   	if (node->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS)
>   		return 0;
> @@ -524,7 +525,10 @@ static int kfd_gws_init(struct kfd_node *node)
>   		(KFD_GC_VERSION(node) == IP_VERSION(9, 4, 3)) ||
>   		(KFD_GC_VERSION(node) >= IP_VERSION(10, 3, 0)
>   			&& KFD_GC_VERSION(node) < IP_VERSION(11, 0, 0)
> -			&& kfd->mec2_fw_version >= 0x6b))))
> +			&& kfd->mec2_fw_version >= 0x6b) ||
> +		(KFD_GC_VERSION(node) >= IP_VERSION(11, 0, 0)
> +			&& KFD_GC_VERSION(node) < IP_VERSION(12, 0, 0)
> +			&& mes_rev >= 68))))
>   		ret = amdgpu_amdkfd_alloc_gws(node->adev,
>   				node->adev->gds.gws_size, &node->gws);
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index 761963ad6154..71b7f16c0173 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -237,10 +237,7 @@ static int add_queue_mes(struct device_queue_manager *dqm, struct queue *q,
>   	}
>   	queue_input.queue_type = (uint32_t)queue_type;
>   
> -	if (q->gws) {
> -		queue_input.gws_base = 0;
> -		queue_input.gws_size = qpd->num_gws;
> -	}
> +	queue_input.exclusively_scheduled = q->properties.is_gws;
>   
>   	amdgpu_mes_lock(&adev->mes);
>   	r = adev->mes.funcs->add_hw_queue(&adev->mes, &queue_input);
> @@ -250,7 +247,7 @@ static int add_queue_mes(struct device_queue_manager *dqm, struct queue *q,
>   			q->properties.doorbell_off);
>   		pr_err("MES might be in unrecoverable state, issue a GPU reset\n");
>   		kfd_hws_hang(dqm);
> -}
> +	}
>   
>   	return r;
>   }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> index ba9d69054119..e8ee52d70a19 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> @@ -123,7 +123,7 @@ int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
>   	if (!gws && pdd->qpd.num_gws == 0)
>   		return -EINVAL;
>   
> -	if (KFD_GC_VERSION(dev) != IP_VERSION(9, 4, 3)) {
> +	if (KFD_GC_VERSION(dev) != IP_VERSION(9, 4, 3) && !dev->kfd->shared_resources.enable_mes) {
>   		if (gws)
>   			ret = amdgpu_amdkfd_add_gws_to_process(pdd->process->kgd_process_info,
>   				gws, &mem);
> @@ -136,7 +136,9 @@ int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
>   	} else {
>   		/*
>   		 * Intentionally set GWS to a non-NULL value
> -		 * for GFX 9.4.3.
> +		 * for devices that do not use GWS for global wave
> +		 * synchronization but require the formality
> +		 * of setting GWS for cooperative groups.
>   		 */
>   		pqn->q->gws = gws ? ERR_PTR(-ENOMEM) : NULL;
>   	}
> @@ -173,7 +175,8 @@ void pqm_uninit(struct process_queue_manager *pqm)
>   
>   	list_for_each_entry_safe(pqn, next, &pqm->queues, process_queue_list) {
>   		if (pqn->q && pqn->q->gws &&
> -		    KFD_GC_VERSION(pqn->q->device) != IP_VERSION(9, 4, 3))
> +				KFD_GC_VERSION(pqn->q->device) != IP_VERSION(9, 4, 3) &&
> +					!pqn->q->device->kfd->shared_resources.enable_mes)

This still looks wrong. You have pieces of the same condition indented 3 
different ways here. I'd prefer to see this (and hoping Thunderbird 
won't mess it up):

 		if (pqn->q && pqn->q->gws &&
		    KFD_GC_VERSION(pqn->q->device) != IP_VERSION(9, 4, 3) &&
		    !pqn->q->device->kfd->shared_resources.enable_mes)

Everything indented to line up just behind the open ( on the first line.


>   			amdgpu_amdkfd_remove_gws_from_process(pqm->process->kgd_process_info,
>   				pqn->q->gws);
>   		kfd_procfs_del_queue(pqn->q);
> @@ -455,7 +458,8 @@ int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid)
>   		}
>   
>   		if (pqn->q->gws) {
> -			if (KFD_GC_VERSION(pqn->q->device) != IP_VERSION(9, 4, 3))
> +			if (KFD_GC_VERSION(pqn->q->device) != IP_VERSION(9, 4, 3) &&
> +						!dev->kfd->shared_resources.enable_mes)

Same as above.

Thanks,
  Felix


>   				amdgpu_amdkfd_remove_gws_from_process(
>   						pqm->process->kgd_process_info,
>   						pqn->q->gws);
> diff --git a/drivers/gpu/drm/amd/include/mes_v11_api_def.h b/drivers/gpu/drm/amd/include/mes_v11_api_def.h
> index 0997e999416a..b1db2b190187 100644
> --- a/drivers/gpu/drm/amd/include/mes_v11_api_def.h
> +++ b/drivers/gpu/drm/amd/include/mes_v11_api_def.h
> @@ -275,7 +275,9 @@ union MESAPI__ADD_QUEUE {
>   			uint32_t trap_en		: 1;
>   			uint32_t is_aql_queue		: 1;
>   			uint32_t skip_process_ctx_clear : 1;
> -			uint32_t reserved		: 19;
> +			uint32_t map_legacy_kq		: 1;
> +			uint32_t exclusively_scheduled	: 1;
> +			uint32_t reserved		: 17;
>   		};
>   		struct MES_API_STATUS		api_status;
>   		uint64_t                        tma_addr;