Re: [PATCH 03/11] drm/amdgpu/gfx: add generic handling for disable_kq

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

On 2025-03-05 15:47, Alex Deucher wrote:
> Add proper checks for disable_kq functionality in
> gfx helper functions.  Add special logic for families
> that require the clear state setup.
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 92 +++++++++++++++++--------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h |  2 +
>   2 files changed, 67 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index a194bf3347cbc..af3f8b62f6fd5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -371,6 +371,18 @@ int amdgpu_gfx_kiq_init(struct amdgpu_device *adev,
>   	return 0;
>   }
>   
> +static bool amdgpu_gfx_disable_gfx_kq(struct amdgpu_device *adev)
> +{
> +	if (adev->gfx.disable_kq) {
> +		/* GFX11 needs the GFX ring for clear buffer */
> +		if (amdgpu_ip_version(adev, GC_HWIP, 0) <= IP_VERSION(12, 0, 0))

Should this be < instead of <=?

Regards,
   Felix

> +			return false;
> +		else
> +			return true;
> +	}
> +	return false;
> +}
> +
>   /* create MQD for each compute/gfx queue */
>   int amdgpu_gfx_mqd_sw_init(struct amdgpu_device *adev,
>   			   unsigned int mqd_size, int xcc_id)
> @@ -379,6 +391,7 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device *adev,
>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_id];
>   	struct amdgpu_ring *ring = &kiq->ring;
>   	u32 domain = AMDGPU_GEM_DOMAIN_GTT;
> +	bool disable_kq_gfx = amdgpu_gfx_disable_gfx_kq(adev);
>   
>   #if !defined(CONFIG_ARM) && !defined(CONFIG_ARM64)
>   	/* Only enable on gfx10 and 11 for now to avoid changing behavior on older chips */
> @@ -413,7 +426,8 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device *adev,
>   		}
>   	}
>   
> -	if (adev->asic_type >= CHIP_NAVI10 && amdgpu_async_gfx_ring) {
> +	if (adev->asic_type >= CHIP_NAVI10 && amdgpu_async_gfx_ring &&
> +	    !disable_kq_gfx) {
>   		/* create MQD for each KGQ */
>   		for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
>   			ring = &adev->gfx.gfx_ring[i];
> @@ -437,25 +451,28 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device *adev,
>   		}
>   	}
>   
> -	/* create MQD for each KCQ */
> -	for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> -		j = i + xcc_id * adev->gfx.num_compute_rings;
> -		ring = &adev->gfx.compute_ring[j];
> -		if (!ring->mqd_obj) {
> -			r = amdgpu_bo_create_kernel(adev, mqd_size, PAGE_SIZE,
> -						    domain, &ring->mqd_obj,
> -						    &ring->mqd_gpu_addr, &ring->mqd_ptr);
> -			if (r) {
> -				dev_warn(adev->dev, "failed to create ring mqd bo (%d)", r);
> -				return r;
> -			}
> +	if (!adev->gfx.disable_kq) {

Maybe just set adev->gfx.num_compute_rings to 0 somewhere, then you 
don't need this condition.


> +		/* create MQD for each KCQ */
> +		for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> +			j = i + xcc_id * adev->gfx.num_compute_rings;
> +			ring = &adev->gfx.compute_ring[j];
> +			if (!ring->mqd_obj) {
> +				r = amdgpu_bo_create_kernel(adev, mqd_size, PAGE_SIZE,
> +							    domain, &ring->mqd_obj,
> +							    &ring->mqd_gpu_addr, &ring->mqd_ptr);
> +				if (r) {
> +					dev_warn(adev->dev, "failed to create ring mqd bo (%d)", r);
> +					return r;
> +				}
>   
> -			ring->mqd_size = mqd_size;
> -			/* prepare MQD backup */
> -			adev->gfx.mec.mqd_backup[j] = kzalloc(mqd_size, GFP_KERNEL);
> -			if (!adev->gfx.mec.mqd_backup[j]) {
> -				dev_warn(adev->dev, "no memory to create MQD backup for ring %s\n", ring->name);
> -				return -ENOMEM;
> +				ring->mqd_size = mqd_size;
> +				/* prepare MQD backup */
> +				adev->gfx.mec.mqd_backup[j] = kzalloc(mqd_size, GFP_KERNEL);
> +				if (!adev->gfx.mec.mqd_backup[j]) {
> +					dev_warn(adev->dev, "no memory to create MQD backup for ring %s\n",
> +						 ring->name);
> +					return -ENOMEM;
> +				}
>   			}
>   		}
>   	}
> @@ -468,8 +485,10 @@ void amdgpu_gfx_mqd_sw_fini(struct amdgpu_device *adev, int xcc_id)
>   	struct amdgpu_ring *ring = NULL;
>   	int i, j;
>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_id];
> +	bool disable_kq_gfx = amdgpu_gfx_disable_gfx_kq(adev);
>   
> -	if (adev->asic_type >= CHIP_NAVI10 && amdgpu_async_gfx_ring) {
> +	if (adev->asic_type >= CHIP_NAVI10 && amdgpu_async_gfx_ring &&
> +	    !disable_kq_gfx) {
>   		for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
>   			ring = &adev->gfx.gfx_ring[i];
>   			kfree(adev->gfx.me.mqd_backup[i]);
> @@ -479,13 +498,15 @@ void amdgpu_gfx_mqd_sw_fini(struct amdgpu_device *adev, int xcc_id)
>   		}
>   	}
>   
> -	for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> -		j = i + xcc_id * adev->gfx.num_compute_rings;
> -		ring = &adev->gfx.compute_ring[j];
> -		kfree(adev->gfx.mec.mqd_backup[j]);
> -		amdgpu_bo_free_kernel(&ring->mqd_obj,
> -				      &ring->mqd_gpu_addr,
> -				      &ring->mqd_ptr);
> +	if (!adev->gfx.disable_kq) {

Same as above.


> +		for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> +			j = i + xcc_id * adev->gfx.num_compute_rings;
> +			ring = &adev->gfx.compute_ring[j];
> +			kfree(adev->gfx.mec.mqd_backup[j]);
> +			amdgpu_bo_free_kernel(&ring->mqd_obj,
> +					      &ring->mqd_gpu_addr,
> +					      &ring->mqd_ptr);
> +		}
>   	}
>   
>   	ring = &kiq->ring;
> @@ -502,6 +523,9 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device *adev, int xcc_id)
>   	int i, r = 0;
>   	int j;
>   
> +	if (adev->gfx.disable_kq)

Same as above.


> +		return 0;
> +
>   	if (adev->enable_mes) {
>   		for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>   			j = i + xcc_id * adev->gfx.num_compute_rings;
> @@ -547,11 +571,15 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device *adev, int xcc_id)
>   
>   int amdgpu_gfx_disable_kgq(struct amdgpu_device *adev, int xcc_id)
>   {
> +	bool disable_kq_gfx = amdgpu_gfx_disable_gfx_kq(adev);
>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_id];
>   	struct amdgpu_ring *kiq_ring = &kiq->ring;
>   	int i, r = 0;
>   	int j;
>   
> +	if (disable_kq_gfx)
> +		return 0;
Maybe just set adev->gfx.num_gfx_rings to 0 somewhere, then you don't 
need this condition.

Regards,
   Felix


> +
>   	if (adev->enable_mes) {
>   		if (amdgpu_gfx_is_master_xcc(adev, xcc_id)) {
>   			for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
> @@ -657,6 +685,9 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device *adev, int xcc_id)
>   	uint64_t queue_mask = 0;
>   	int r, i, j;
>   
> +	if (adev->gfx.disable_kq)
> +		return 0;
> +
>   	if (adev->mes.enable_legacy_queue_map)
>   		return amdgpu_gfx_mes_enable_kcq(adev, xcc_id);
>   
> @@ -716,10 +747,14 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device *adev, int xcc_id)
>   
>   int amdgpu_gfx_enable_kgq(struct amdgpu_device *adev, int xcc_id)
>   {
> +	bool disable_kq_gfx = amdgpu_gfx_disable_gfx_kq(adev);
>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_id];
>   	struct amdgpu_ring *kiq_ring = &kiq->ring;
>   	int r, i, j;
>   
> +	if (disable_kq_gfx)
> +		return 0;
> +
>   	if (!kiq->pmf || !kiq->pmf->kiq_map_queues)
>   		return -EINVAL;
>   
> @@ -1544,6 +1579,9 @@ static ssize_t amdgpu_gfx_set_run_cleaner_shader(struct device *dev,
>   	if (adev->in_suspend && !adev->in_runpm)
>   		return -EPERM;
>   
> +	if (adev->gfx.disable_kq)
> +		return -ENOTSUPP;
> +
>   	ret = kstrtol(buf, 0, &value);
>   
>   	if (ret)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index ddf4533614bac..8fa68a4ac34f1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -483,6 +483,8 @@ struct amdgpu_gfx {
>   
>   	atomic_t			total_submission_cnt;
>   	struct delayed_work		idle_work;
> +
> +	bool				disable_kq;
>   };
>   
>   struct amdgpu_gfx_ras_reg_entry {