[PATCH v2 4/8] drm/xe: add function to convert xe hw engine class to user class

[Riana Tauro]

Hi Umesh

On 12/11/2024 5:18 AM, Umesh Nerlige Ramappa wrote:
> On Thu, Nov 21, 2024 at 12:09:00PM +0530, Riana Tauro wrote:
>> Avoid duplication of code in PMU by moving hw engine to user class
>> to a separate function. Refactor existing code to use
>> this new function.
>>
>> v2: replace with array (Umesh)
>>    fix commit message (Rodrigo)
>>
>> Signed-off-by: Riana Tauro <riana.tauro at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_hw_engine.c | 21 +++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_hw_engine.h |  1 +
>> drivers/gpu/drm/xe/xe_query.c     | 12 ++----------
>> 3 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/ 
>> xe_hw_engine.c
>> index c4b0dc3be39c..c41b4038dbb3 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
>> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
>> @@ -979,6 +979,27 @@ const char *xe_hw_engine_class_to_str(enum 
>> xe_engine_class class)
>>     return NULL;
>> }
>>
>> +/**
>> + * xe_hw_engine_to_user_class - converts xe hw engine to user engine 
>> class
>> + * @engine_class: hw engine class
>> + *
>> + * Returns: user engine class on success, -1 on error
> 
> 1) Depends on what you want to return on error since you return u16. If 
> the WARN_ON fires below, you may just want to return a default like:
> DRM_XE_ENGINE_CLASS_RENDER or 0 and change the comment above accordingly.

This is called as a part of iterating through hw engines both in query 
as well as pmu.  So it shouldn't hit the warn on condition.

Returning render would cause wrong mapping, how about 
XE_ENGINE_CLASS_OTHER or XE_ENGINE_CLASS_MAX ? or do i just remove the 
warn on?
> 
> 2) Returning -1 means use an int return type and make sure all callers 
> check for the return and act accordingly.
> 
> I would recommend 1.
> 
>> + */
>> +u16 xe_hw_engine_to_user_class(enum xe_engine_class engine_class)
> 
> since now this is a xe_hw_engine api, pass the hwe to this function 
> instead of the engine_class.

There is one other function in that file with the same parameter. So 
used this.

*xe_hw_engine_class_to_str(enum xe_engine_class class)

Thanks,
Riana Tauro
> 
>> +{
>> +    const u16 xe_to_user_engine_class[] = {
>> +        [XE_ENGINE_CLASS_RENDER] = DRM_XE_ENGINE_CLASS_RENDER,
>> +        [XE_ENGINE_CLASS_COPY] = DRM_XE_ENGINE_CLASS_COPY,
>> +        [XE_ENGINE_CLASS_VIDEO_DECODE] = 
>> DRM_XE_ENGINE_CLASS_VIDEO_DECODE,
>> +        [XE_ENGINE_CLASS_VIDEO_ENHANCE] = 
>> DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE,
>> +        [XE_ENGINE_CLASS_COMPUTE] = DRM_XE_ENGINE_CLASS_COMPUTE,
>> +    };
>> +
>> +    WARN_ON(engine_class >= ARRAY_SIZE(xe_to_user_engine_class));
> 
> Revisiting this, it should not index into the array if the engine_class 
> is invalid.
> 
>      if(WARN_ON(engine_class >= ARRAY_SIZE(xe_to_user_engine_class)))
>          return <something>;
> 
> Thanks,
> Umesh
> 
> 
>> +
>> +    return xe_to_user_engine_class[engine_class];
>> +}
>> +
>> u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
>> {
>>     return xe_mmio_read64_2x32(&hwe->gt->mmio, RING_TIMESTAMP(hwe- 
>> >mmio_base));
>> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/ 
>> xe_hw_engine.h
>> index 6b5f9fa2a594..06b39cb1a434 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
>> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
>> @@ -74,6 +74,7 @@ static inline bool xe_hw_engine_is_valid(struct 
>> xe_hw_engine *hwe)
>>
>> const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
>> u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
>> +u16 xe_hw_engine_to_user_class(enum xe_engine_class engine_class);
>> enum xe_force_wake_domains xe_hw_engine_to_fw_domain(struct 
>> xe_hw_engine *hwe);
>>
>> void xe_hw_engine_mmio_write32(struct xe_hw_engine *hwe, struct xe_reg 
>> reg, u32 val);
>> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/ 
>> xe_query.c
>> index 3eda616f1502..a260c43f13f4 100644
>> --- a/drivers/gpu/drm/xe/xe_query.c
>> +++ b/drivers/gpu/drm/xe/xe_query.c
>> @@ -27,14 +27,6 @@
>> #include "xe_ttm_vram_mgr.h"
>> #include "xe_wa.h"
>>
>> -static const u16 xe_to_user_engine_class[] = {
>> -    [XE_ENGINE_CLASS_RENDER] = DRM_XE_ENGINE_CLASS_RENDER,
>> -    [XE_ENGINE_CLASS_COPY] = DRM_XE_ENGINE_CLASS_COPY,
>> -    [XE_ENGINE_CLASS_VIDEO_DECODE] = DRM_XE_ENGINE_CLASS_VIDEO_DECODE,
>> -    [XE_ENGINE_CLASS_VIDEO_ENHANCE] = DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE,
>> -    [XE_ENGINE_CLASS_COMPUTE] = DRM_XE_ENGINE_CLASS_COMPUTE,
>> -};
>> -
>> static const enum xe_engine_class user_to_xe_engine_class[] = {
>>     [DRM_XE_ENGINE_CLASS_RENDER] = XE_ENGINE_CLASS_RENDER,
>>     [DRM_XE_ENGINE_CLASS_COPY] = XE_ENGINE_CLASS_COPY,
>> @@ -207,7 +199,7 @@ static int query_engines(struct xe_device *xe,
>>                 continue;
>>
>>             engines->engines[i].instance.engine_class =
>> -                xe_to_user_engine_class[hwe->class];
>> +                xe_hw_engine_to_user_class(hwe->class);
>>             engines->engines[i].instance.engine_instance =
>>                 hwe->logical_instance;
>>             engines->engines[i].instance.gt_id = gt->info.id;
>> @@ -678,7 +670,7 @@ static int query_oa_units(struct xe_device *xe,
>>                 if (!xe_hw_engine_is_reserved(hwe) &&
>>                     xe_oa_unit_id(hwe) == u->oa_unit_id) {
>>                     du->eci[j].engine_class =
>> -                        xe_to_user_engine_class[hwe->class];
>> +                        xe_hw_engine_to_user_class(hwe->class);
>>                     du->eci[j].engine_instance = hwe->logical_instance;
>>                     du->eci[j].gt_id = gt->info.id;
>>                     j++;
>> -- 
>> 2.40.0
>>