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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Wed, Dec 18, 2024 at 01:03:19PM +0530, Riana Tauro wrote:
>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@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.

true, but now this is an internal api, so we cannot guarantee that 
everyone will use it within valid bounds. We need some check to ensure 
valid array access. Maybe you could use an int return type and return an 
error. For code that iterates within the proper bounds, don't check the 
return value. Not sure how else to do this cleanly.

@Lucas - any thoughts here?

Thanks,
Umesh

>
>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
>>>
>