[PATCH 1/3] drm/amdkfd: Use asic specific fn to configure grace period

[Chen, Xiaogang]

On 1/14/2025 1:52 PM, Elena Sakhnovitch wrote:
> From: Harish Kasiviswanathan<Harish.Kasiviswanathan at amd.com>
>
> Currently, grace period is modified only for gfx943 APU. In the future
> this might need to be set for other ASICs too. Either ways, asic
> specific values should be handled by asic specific functions.
>
> Signed-off-by: Harish Kasiviswanathan<Harish.Kasiviswanathan at amd.com>
> Signed-off-by: Elena Sakhnovitch<Elena.Sakhnovitch at amd.com>
> ---
>   .../drm/amd/amdkfd/kfd_device_queue_manager.c | 24 ++++++-------------
>   .../drm/amd/amdkfd/kfd_device_queue_manager.h |  3 ++-
>   .../gpu/drm/amd/amdkfd/kfd_packet_manager.c   |  3 +++
>   .../drm/amd/amdkfd/kfd_packet_manager_v9.c    | 10 ++++++++
>   4 files changed, 22 insertions(+), 18 deletions(-)
>
> 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 f157494bfdb1..4369308a74e7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -1859,26 +1859,16 @@ static int start_cpsch(struct device_queue_manager *dqm)
>   	/* clear hang status when driver try to start the hw scheduler */
>   	dqm->sched_running = true;
>   
> -	if (!dqm->dev->kfd->shared_resources.enable_mes)
> +	if (!dqm->dev->kfd->shared_resources.enable_mes) {
>   		execute_queues_cpsch(dqm, KFD_UNMAP_QUEUES_FILTER_DYNAMIC_QUEUES, 0, USE_DEFAULT_GRACE_PERIOD);
> -
> -	/* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
> -	if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&
> -	    (KFD_GC_VERSION(dqm->dev) == IP_VERSION(9, 4, 3))) {
> -		uint32_t reg_offset = 0;
> -		uint32_t grace_period = 1;
> -
> -		retval = pm_update_grace_period(&dqm->packet_mgr,
> -						grace_period);
> +		retval = pm_update_grace_period(&dqm->packet_mgr, SET_ASIC_OPTIMIZED_GRACE_PERIOD);
>   		if (retval)
> -			dev_err(dev, "Setting grace timeout failed\n");
> -		else if (dqm->dev->kfd2kgd->build_grace_period_packet_info)
> -			/* Update dqm->wait_times maintained in software */
> -			dqm->dev->kfd2kgd->build_grace_period_packet_info(
> -					dqm->dev->adev,	dqm->wait_times,
> -					grace_period, &reg_offset,
> -					&dqm->wait_times);
> +			dev_err(dev, "Setting optimized grace timeout failed\n");
>   	}
> +	if (dqm->dev->kfd2kgd->get_iq_wait_times)
> +		dqm->dev->kfd2kgd->get_iq_wait_times(dqm->dev->adev,
> +					&dqm->wait_times,
> +					ffs(dqm->dev->xcc_mask) - 1);
why put it here? it seems not related to this patch.
>   
>   	/* setup per-queue reset detection buffer  */
>   	num_hw_queue_slots =  dqm->dev->kfd->shared_resources.num_queue_per_pipe *
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> index 09ab36f8e8c6..fb3419993612 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> @@ -37,7 +37,8 @@
>   
>   #define KFD_MES_PROCESS_QUANTUM		100000
>   #define KFD_MES_GANG_QUANTUM		10000
> -#define USE_DEFAULT_GRACE_PERIOD 0xffffffff
> +#define USE_DEFAULT_GRACE_PERIOD	0xffffffff
what is difference between the two lines above, or just add spaces?
> +#define SET_ASIC_OPTIMIZED_GRACE_PERIOD	0xfffffffe
>   
>   struct device_process_node {
>   	struct qcm_process_device *qpd;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
> index 4984b41cd372..518c6ec23a75 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
> @@ -403,6 +403,9 @@ int pm_update_grace_period(struct packet_manager *pm, uint32_t grace_period)
>   	int retval = 0;
>   	uint32_t *buffer, size;
>   
> +	if (!pm->pmf->set_grace_period || !pm->pmf->set_grace_period_size)
> +		return 0;
> +
>   	size = pm->pmf->set_grace_period_size;
>   
>   	mutex_lock(&pm->lock);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> index d56525201155..fde212242129 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> @@ -302,9 +302,19 @@ static int pm_set_grace_period_v9(struct packet_manager *pm,
>   		uint32_t grace_period)
>   {
>   	struct pm4_mec_write_data_mmio *packet;
> +	struct device_queue_manager *dqm = pm->dqm;
>   	uint32_t reg_offset = 0;
>   	uint32_t reg_data = 0;
>   
> +	if (grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD) {
this patch purpose is setting grace_period at asic specific file. Do we 
need check "grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD"? I think we 
can set grace_period directly at asic specific file.
> +		/* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
> +		if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&
> +		    KFD_GC_VERSION(dqm->dev) == IP_VERSION(9, 4, 3))
> +			grace_period = 1;
> +		else
> +			return 0;

why return 0 here? that skip following build_grace_period_packet_info 
for no-GFX9.4.3 AP. As above, set grace_period directly is more straight 
forward at asic specific file.

Regards

Xiaogang

> +	}
> +
>   	pm->dqm->dev->kfd2kgd->build_grace_period_packet_info(
>   			pm->dqm->dev->adev,
>   			pm->dqm->wait_times,
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20250206/d01338d3/attachment-0001.htm>