Re: [PATCH] drm/amdkfd: Retrieve SDMA numbers from amdgpu

[Felix Kuehling <felix.kuehling@amd.com>]

On 2021-11-17 3:36 p.m., Amber Lin wrote:
> Instead of hard coding the number of sdma engines and the number of
> sdma_xgmi engines in the device_info table, get the number of toal SDMA
> instances from amdgpu. The first two engines are sdma engines and the
> rest are sdma-xgmi engines unless the ASIC doesn't support XGMI.
>
> v2: Move get_num_*_sdma_engines to global and shared by queues manager
>      and topology.
> v3: Use gmc.xgmi.supported to justify the SDMA PCIe/XGMI assignment

You can remove this version history, since this is the first public review.

One more nit-pick inline.


>
> Signed-off-by: Amber Lin <Amber.Lin@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_device.c       | 20 ++++++++++++
>   .../drm/amd/amdkfd/kfd_device_queue_manager.c | 31 +++++++------------
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h         |  3 ++
>   drivers/gpu/drm/amd/amdkfd/kfd_topology.c     |  5 ++-
>   4 files changed, 36 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index ce9f4e562bac..ec1f6bacb61e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -1516,6 +1516,26 @@ void kgd2kfd_smi_event_throttle(struct kfd_dev *kfd, uint64_t throttle_bitmask)
>   		kfd_smi_event_update_thermal_throttling(kfd, throttle_bitmask);
>   }
>   
> +/* get_num_sdma_engines returns the number of PCIe optimized SDMA and
> + * get_num_xgmi_sdma_engines returns the number of XGMI SDMA.
> + * When the device has more than two engines, we reserve two for PCIe to enable
> + * full-duplex and the rest are used as XGMI.
> + */
> +unsigned int get_num_sdma_engines(struct kfd_dev *kdev)

These two non-static functions should have a proper kfd_ name prefix.

Regards,
   Felix


> +{
> +	/* If XGMI is not supported, all SDMA engines are PCIe */
> +	if (!kdev->adev->gmc.xgmi.supported)
> +		return kdev->adev->sdma.num_instances;
> +
> +	return min(kdev->adev->sdma.num_instances, 2);
> +}
> +
> +unsigned int get_num_xgmi_sdma_engines(struct kfd_dev *kdev)
> +{
> +	/* After reserved for PCIe, the rest of engines are XGMI */
> +	return kdev->adev->sdma.num_instances - get_num_sdma_engines(kdev);
> +}
> +
>   #if defined(CONFIG_DEBUG_FS)
>   
>   /* This function will send a package to HIQ to hang the HWS
> 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 62fe28244a80..5f2886cf4d7e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -99,31 +99,22 @@ unsigned int get_pipes_per_mec(struct device_queue_manager *dqm)
>   	return dqm->dev->shared_resources.num_pipe_per_mec;
>   }
>   
> -static unsigned int get_num_sdma_engines(struct device_queue_manager *dqm)
> -{
> -	return dqm->dev->device_info->num_sdma_engines;
> -}
> -
> -static unsigned int get_num_xgmi_sdma_engines(struct device_queue_manager *dqm)
> -{
> -	return dqm->dev->device_info->num_xgmi_sdma_engines;
> -}
> -
>   static unsigned int get_num_all_sdma_engines(struct device_queue_manager *dqm)
>   {
> -	return get_num_sdma_engines(dqm) + get_num_xgmi_sdma_engines(dqm);
> +	return get_num_sdma_engines(dqm->dev) +
> +		get_num_xgmi_sdma_engines(dqm->dev);
>   }
>   
>   unsigned int get_num_sdma_queues(struct device_queue_manager *dqm)
>   {
> -	return dqm->dev->device_info->num_sdma_engines
> -			* dqm->dev->device_info->num_sdma_queues_per_engine;
> +	return get_num_sdma_engines(dqm->dev) *
> +		dqm->dev->device_info->num_sdma_queues_per_engine;
>   }
>   
>   unsigned int get_num_xgmi_sdma_queues(struct device_queue_manager *dqm)
>   {
> -	return dqm->dev->device_info->num_xgmi_sdma_engines
> -			* dqm->dev->device_info->num_sdma_queues_per_engine;
> +	return get_num_xgmi_sdma_engines(dqm->dev) *
> +		dqm->dev->device_info->num_sdma_queues_per_engine;
>   }
>   
>   void program_sh_mem_settings(struct device_queue_manager *dqm,
> @@ -1054,9 +1045,9 @@ static int allocate_sdma_queue(struct device_queue_manager *dqm,
>   		dqm->sdma_bitmap &= ~(1ULL << bit);
>   		q->sdma_id = bit;
>   		q->properties.sdma_engine_id = q->sdma_id %
> -				get_num_sdma_engines(dqm);
> +				get_num_sdma_engines(dqm->dev);
>   		q->properties.sdma_queue_id = q->sdma_id /
> -				get_num_sdma_engines(dqm);
> +				get_num_sdma_engines(dqm->dev);
>   	} else if (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) {
>   		if (dqm->xgmi_sdma_bitmap == 0) {
>   			pr_err("No more XGMI SDMA queue to allocate\n");
> @@ -1071,10 +1062,10 @@ static int allocate_sdma_queue(struct device_queue_manager *dqm,
>   		 * assumes the first N engines are always
>   		 * PCIe-optimized ones
>   		 */
> -		q->properties.sdma_engine_id = get_num_sdma_engines(dqm) +
> -				q->sdma_id % get_num_xgmi_sdma_engines(dqm);
> +		q->properties.sdma_engine_id = get_num_sdma_engines(dqm->dev) +
> +			q->sdma_id % get_num_xgmi_sdma_engines(dqm->dev);
>   		q->properties.sdma_queue_id = q->sdma_id /
> -				get_num_xgmi_sdma_engines(dqm);
> +			get_num_xgmi_sdma_engines(dqm->dev);
>   	}
>   
>   	pr_debug("SDMA engine id: %d\n", q->properties.sdma_engine_id);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 1d3f012bcd2a..85efa100a80d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -213,6 +213,9 @@ struct kfd_device_info {
>   	unsigned int num_sdma_queues_per_engine;
>   };
>   
> +unsigned int get_num_sdma_engines(struct kfd_dev *kdev);
> +unsigned int get_num_xgmi_sdma_engines(struct kfd_dev *kdev);
> +
>   struct kfd_mem_obj {
>   	uint32_t range_start;
>   	uint32_t range_end;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index a3f590e17973..fff4cc99fb5d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -1392,9 +1392,8 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>   		gpu->shared_resources.drm_render_minor;
>   
>   	dev->node_props.hive_id = gpu->hive_id;
> -	dev->node_props.num_sdma_engines = gpu->device_info->num_sdma_engines;
> -	dev->node_props.num_sdma_xgmi_engines =
> -				gpu->device_info->num_xgmi_sdma_engines;
> +	dev->node_props.num_sdma_engines = get_num_sdma_engines(gpu);
> +	dev->node_props.num_sdma_xgmi_engines = get_num_xgmi_sdma_engines(gpu);
>   	dev->node_props.num_sdma_queues_per_engine =
>   				gpu->device_info->num_sdma_queues_per_engine;
>   	dev->node_props.num_gws = (dev->gpu->gws &&