[PATCH 1/2] drm/amdkfd: update SIMD distribution algo for GFXIP 9.4.2 onwards

[PATCH 1/2] drm/amdkfd: update SIMD distribution algo for GFXIP 9.4.2 onwards

[Rajneesh Bhardwaj]

In certain cooperative group dispatch scenarios the default SPI resource
allocation may cause reduced per-CU workgroup occupancy. Set
COMPUTE_RESOURCE_LIMITS.FORCE_SIMD_DIST=1 to mitigate soft hang
scenarions.

Suggested-by: Joseph Greathouse <Joseph.Greathouse@amd.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
---
* Incorporate review feedback from Felix from
  https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg102840.html
  and split one of the suggested gfx11 changes as a seperate patch.


 drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c        | 9 +++++++++
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h                  | 1 +
 drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 4 +++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
index 42d881809dc7..697b6d530d12 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
@@ -303,6 +303,15 @@ static void update_mqd(struct mqd_manager *mm, void *mqd,
 		update_cu_mask(mm, mqd, minfo, 0);
 	set_priority(m, q);
 
+	if (minfo && KFD_GC_VERSION(mm->dev) >= IP_VERSION(9, 4, 2)) {
+		if (minfo->update_flag & UPDATE_FLAG_IS_GWS)
+			m->compute_resource_limits |=
+				COMPUTE_RESOURCE_LIMITS__FORCE_SIMD_DIST_MASK;
+		else
+			m->compute_resource_limits &=
+				~COMPUTE_RESOURCE_LIMITS__FORCE_SIMD_DIST_MASK;
+	}
+
 	q->is_active = QUEUE_IS_ACTIVE(*q);
 }
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 677281c0793e..65b504813576 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -532,6 +532,7 @@ struct queue_properties {
 enum mqd_update_flag {
 	UPDATE_FLAG_DBG_WA_ENABLE = 1,
 	UPDATE_FLAG_DBG_WA_DISABLE = 2,
+	UPDATE_FLAG_IS_GWS = 3, /* quirk for gfx9 IP */
 };
 
 struct mqd_update_info {
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 43eff221eae5..4858112f9a53 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
@@ -95,6 +95,7 @@ void kfd_process_dequeue_from_device(struct kfd_process_device *pdd)
 int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
 			void *gws)
 {
+	struct mqd_update_info minfo = {0};
 	struct kfd_node *dev = NULL;
 	struct process_queue_node *pqn;
 	struct kfd_process_device *pdd;
@@ -146,9 +147,10 @@ int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
 	}
 
 	pdd->qpd.num_gws = gws ? dev->adev->gds.gws_size : 0;
+	minfo.update_flag = gws ? UPDATE_FLAG_IS_GWS : 0;
 
 	return pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
-							pqn->q, NULL);
+							pqn->q, &minfo);
 }
 
 void kfd_process_dequeue_from_all_devices(struct kfd_process *p)
-- 
2.34.1

[PATCH 2/2] drm/amdgpu: Fix implicit assumtion in gfx11 debug flags

[Rajneesh Bhardwaj]

Gfx11 debug flags mask is currently set with an implicit assumption that
no other mqd update flags exist. This needs to be fixed with newly
introduced flag UPDATE_FLAG_IS_GWS by the previous patch.

Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
index d722cbd31783..826bc4f6c8a7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
@@ -55,8 +55,8 @@ static void update_cu_mask(struct mqd_manager *mm, void *mqd,
 	m = get_mqd(mqd);
 
 	if (has_wa_flag) {
-		uint32_t wa_mask = minfo->update_flag == UPDATE_FLAG_DBG_WA_ENABLE ?
-						0xffff : 0xffffffff;
+		uint32_t wa_mask =
+			(minfo->update_flag & UPDATE_FLAG_DBG_WA_ENABLE) ? 0xffff : 0xffffffff;
 
 		m->compute_static_thread_mgmt_se0 = wa_mask;
 		m->compute_static_thread_mgmt_se1 = wa_mask;
-- 
2.34.1

Re: [PATCH 1/2] drm/amdkfd: update SIMD distribution algo for GFXIP 9.4.2 onwards

[Felix Kuehling]

On 2024-02-09 20:49, Rajneesh Bhardwaj wrote:
> In certain cooperative group dispatch scenarios the default SPI resource
> allocation may cause reduced per-CU workgroup occupancy. Set
> COMPUTE_RESOURCE_LIMITS.FORCE_SIMD_DIST=1 to mitigate soft hang
> scenarions.
>
> Suggested-by: Joseph Greathouse <Joseph.Greathouse@amd.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
> ---
> * Incorporate review feedback from Felix from
>    https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg102840.html
>    and split one of the suggested gfx11 changes as a seperate patch.
>
>
>   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c        | 9 +++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h                  | 1 +
>   drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 4 +++-
>   3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> index 42d881809dc7..697b6d530d12 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> @@ -303,6 +303,15 @@ static void update_mqd(struct mqd_manager *mm, void *mqd,
>   		update_cu_mask(mm, mqd, minfo, 0);
>   	set_priority(m, q);
>   
> +	if (minfo && KFD_GC_VERSION(mm->dev) >= IP_VERSION(9, 4, 2)) {
> +		if (minfo->update_flag & UPDATE_FLAG_IS_GWS)
> +			m->compute_resource_limits |=
> +				COMPUTE_RESOURCE_LIMITS__FORCE_SIMD_DIST_MASK;
> +		else
> +			m->compute_resource_limits &=
> +				~COMPUTE_RESOURCE_LIMITS__FORCE_SIMD_DIST_MASK;
> +	}
> +
>   	q->is_active = QUEUE_IS_ACTIVE(*q);
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 677281c0793e..65b504813576 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -532,6 +532,7 @@ struct queue_properties {
>   enum mqd_update_flag {
>   	UPDATE_FLAG_DBG_WA_ENABLE = 1,
>   	UPDATE_FLAG_DBG_WA_DISABLE = 2,
> +	UPDATE_FLAG_IS_GWS = 3, /* quirk for gfx9 IP */

This flat needs to be a separate bit. So it should be defined as 4. 
Otherwise it looks just like UPDATE_FLAG_DBG_WA_ENABLE | 
UPDATE_FLAG_DBG_WA_DISABLE. I agree that defining bit-masks in an enum 
is not ideal, but I've seen the same in other places.

Regards,
   Felix


>   };
>   
>   struct mqd_update_info {
> 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 43eff221eae5..4858112f9a53 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> @@ -95,6 +95,7 @@ void kfd_process_dequeue_from_device(struct kfd_process_device *pdd)
>   int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
>   			void *gws)
>   {
> +	struct mqd_update_info minfo = {0};
>   	struct kfd_node *dev = NULL;
>   	struct process_queue_node *pqn;
>   	struct kfd_process_device *pdd;
> @@ -146,9 +147,10 @@ int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
>   	}
>   
>   	pdd->qpd.num_gws = gws ? dev->adev->gds.gws_size : 0;
> +	minfo.update_flag = gws ? UPDATE_FLAG_IS_GWS : 0;
>   
>   	return pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
> -							pqn->q, NULL);
> +							pqn->q, &minfo);
>   }
>   
>   void kfd_process_dequeue_from_all_devices(struct kfd_process *p)

Re: [PATCH 2/2] drm/amdgpu: Fix implicit assumtion in gfx11 debug flags

[Felix Kuehling]

On 2024-02-09 20:49, Rajneesh Bhardwaj wrote:
> Gfx11 debug flags mask is currently set with an implicit assumption that
> no other mqd update flags exist. This needs to be fixed with newly
> introduced flag UPDATE_FLAG_IS_GWS by the previous patch.
>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>

Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>


> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
> index d722cbd31783..826bc4f6c8a7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
> @@ -55,8 +55,8 @@ static void update_cu_mask(struct mqd_manager *mm, void *mqd,
>   	m = get_mqd(mqd);
>   
>   	if (has_wa_flag) {
> -		uint32_t wa_mask = minfo->update_flag == UPDATE_FLAG_DBG_WA_ENABLE ?
> -						0xffff : 0xffffffff;
> +		uint32_t wa_mask =
> +			(minfo->update_flag & UPDATE_FLAG_DBG_WA_ENABLE) ? 0xffff : 0xffffffff;
>   
>   		m->compute_static_thread_mgmt_se0 = wa_mask;
>   		m->compute_static_thread_mgmt_se1 = wa_mask;

Re: [PATCH 1/2] drm/amdkfd: update SIMD distribution algo for GFXIP 9.4.2 onwards

[Bhardwaj, Rajneesh]

On 2/13/2024 3:52 PM, Felix Kuehling wrote:
>
> On 2024-02-09 20:49, Rajneesh Bhardwaj wrote:
>> In certain cooperative group dispatch scenarios the default SPI resource
>> allocation may cause reduced per-CU workgroup occupancy. Set
>> COMPUTE_RESOURCE_LIMITS.FORCE_SIMD_DIST=1 to mitigate soft hang
>> scenarions.
>>
>> Suggested-by: Joseph Greathouse <Joseph.Greathouse@amd.com>
>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
>> ---
>> * Incorporate review feedback from Felix from
>> https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg102840.html
>>    and split one of the suggested gfx11 changes as a seperate patch.
>>
>>
>>   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c        | 9 +++++++++
>>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h                  | 1 +
>>   drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 4 +++-
>>   3 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
>> index 42d881809dc7..697b6d530d12 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
>> @@ -303,6 +303,15 @@ static void update_mqd(struct mqd_manager *mm, 
>> void *mqd,
>>           update_cu_mask(mm, mqd, minfo, 0);
>>       set_priority(m, q);
>>   +    if (minfo && KFD_GC_VERSION(mm->dev) >= IP_VERSION(9, 4, 2)) {
>> +        if (minfo->update_flag & UPDATE_FLAG_IS_GWS)
>> +            m->compute_resource_limits |=
>> +                COMPUTE_RESOURCE_LIMITS__FORCE_SIMD_DIST_MASK;
>> +        else
>> +            m->compute_resource_limits &=
>> +                ~COMPUTE_RESOURCE_LIMITS__FORCE_SIMD_DIST_MASK;
>> +    }
>> +
>>       q->is_active = QUEUE_IS_ACTIVE(*q);
>>   }
>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> index 677281c0793e..65b504813576 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> @@ -532,6 +532,7 @@ struct queue_properties {
>>   enum mqd_update_flag {
>>       UPDATE_FLAG_DBG_WA_ENABLE = 1,
>>       UPDATE_FLAG_DBG_WA_DISABLE = 2,
>> +    UPDATE_FLAG_IS_GWS = 3, /* quirk for gfx9 IP */
>
> This flat needs to be a separate bit. So it should be defined as 4. 
> Otherwise it looks just like UPDATE_FLAG_DBG_WA_ENABLE | 
> UPDATE_FLAG_DBG_WA_DISABLE. I agree that defining bit-masks in an enum 
> is not ideal, but I've seen the same in other places.


Yes, I agree. I am have sent the updated patches. When we extend this in 
future if required, it would be 8, 16 and so on...not a great way to 
extend/define an enum IMO.


>
> Regards,
>   Felix
>
>
>>   };
>>     struct mqd_update_info {
>> 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 43eff221eae5..4858112f9a53 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> @@ -95,6 +95,7 @@ void kfd_process_dequeue_from_device(struct 
>> kfd_process_device *pdd)
>>   int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
>>               void *gws)
>>   {
>> +    struct mqd_update_info minfo = {0};
>>       struct kfd_node *dev = NULL;
>>       struct process_queue_node *pqn;
>>       struct kfd_process_device *pdd;
>> @@ -146,9 +147,10 @@ int pqm_set_gws(struct process_queue_manager 
>> *pqm, unsigned int qid,
>>       }
>>         pdd->qpd.num_gws = gws ? dev->adev->gds.gws_size : 0;
>> +    minfo.update_flag = gws ? UPDATE_FLAG_IS_GWS : 0;
>>         return pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
>> -                            pqn->q, NULL);
>> +                            pqn->q, &minfo);
>>   }
>>     void kfd_process_dequeue_from_all_devices(struct kfd_process *p)