[PATCH v2] drm/xe: Use enum instead of hard coding
Ghimiray, Himal Prasad
himal.prasad.ghimiray at intel.com
Tue Mar 26 07:08:52 UTC 2024
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 at intel.com>
>> Signed-off-by: Himal Prasad Ghimiray<himal.prasad.ghimiray at intel.com>
>> Reviewed-by: Rodrigo Vivi<rodrigo.vivi at 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
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20240326/3de78dee/attachment.htm>
More information about the Intel-xe
mailing list