Re: [PATCH v2] drm/xe: Use enum instead of hard coding

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2819 bytes --]


On 26-03-2024 11:19, Matthew Brost wrote:
> On Fri, Mar 22, 2024 at 08:45:58AM +0530, Himal Prasad Ghimiray wrote:
>> Instead of hardcoding the value 0, pass DRM_SCHED_PRIORITY_KERNEL as an
>> argument to drm_sched_entity_init.
>>
>> v2
>> - Make changes in xe_execlist too. (Rodrigo)
>>
>> Cc: Matthew Brost<matthew.brost@intel.com>
>> Signed-off-by: Himal Prasad Ghimiray<himal.prasad.ghimiray@intel.com>
>> Reviewed-by: Rodrigo Vivi<rodrigo.vivi@intel.com>
> Hold on, this is intentionally set to 0 not DRM_SCHED_PRIORITY_KERNEL.
> In Xe we configure the scheduler with 1 priority level, thus the value
> has to be zero. i.e. if enum DRM_SCHED_PRIORITY_KERNEL changes to
> non-zero we are broken.
>
> Better leave as is, or add new define like XE_SCHED_PRIORITY_DEFAULT == 0.
>
> Matt

The function expects a parameter of type |drm_sched_priority|, and a 
static analyzer flagged our usage of hard-coded values.
Upon reviewing the implementation of |drm_sched_entity_init|, I 
discovered that the function is already safeguarded to handle
priority levels based on the scheduler's run_queues. Notably, altering 
|DRM_SCHED_PRIORITY_KERNEL| to a non-zero value could
potentially disrupt scheduler initialization itself (i = 
DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) .

Therefore, utilizing it seemed like a safe approach.

However, defining |XE_SCHED_PRIORITY_DEFAULT == 0| and employing it 
doesn't seem logical.

Hence, if you perceive this change as risky or unsafe, lets drop it.

BR
Himal
>> ---
>>   drivers/gpu/drm/xe/xe_execlist.c      | 2 +-
>>   drivers/gpu/drm/xe/xe_gpu_scheduler.h | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c
>> index dece2785933c..57d3c11da591 100644
>> --- a/drivers/gpu/drm/xe/xe_execlist.c
>> +++ b/drivers/gpu/drm/xe/xe_execlist.c
>> @@ -341,7 +341,7 @@ static int execlist_exec_queue_init(struct xe_exec_queue *q)
>>   		goto err_free;
>>   
>>   	sched = &exl->sched;
>> -	err = drm_sched_entity_init(&exl->entity, 0, &sched, 1, NULL);
>> +	err = drm_sched_entity_init(&exl->entity, DRM_SCHED_PRIORITY_KERNEL, &sched, 1, NULL);
>>   	if (err)
>>   		goto err_sched;
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>> index 10c6bb9c9386..1f712f4fc76a 100644
>> --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>> +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>> @@ -63,7 +63,7 @@ static inline int
>>   xe_sched_entity_init(struct xe_sched_entity *entity,
>>   		     struct xe_gpu_scheduler *sched)
>>   {
>> -	return drm_sched_entity_init(entity, 0,
>> +	return drm_sched_entity_init(entity, DRM_SCHED_PRIORITY_KERNEL,
>>   				     (struct drm_gpu_scheduler **)&sched,
>>   				     1, NULL);
>>   }
>> -- 
>> 2.25.1
>>

[-- Attachment #2: Type: text/html, Size: 9449 bytes --]