Re: [PATCH v2 17/30] drm/xe: Create scoped cleanup class for force_wake_get_any_engine()

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Matt Roper (2025-11-10 20:20:35-03:00)
>force_wake_get_any_engine() is a single-use function to pick any engine
>present on the platform and grab its forcewake.  The signature
>(returning a boolean success and both the engine pointer and a forcewake
>ref by reference) is a bit awkward.  Rewrite it such that the
>forcewake ref is the function's return value and the caller can
>determine success/failure by checking the engine pointer against NULL.
>
>With this new signature, the function can serve as a scoped cleanup
>class constructor, so define the corresponding class.  Note that if we
>fail to obtain forcewake (or if the platform somehow has no engines),
>the constructor can fail, returning an invalid fw_ref.  In such cases,
>fw_ref.fw will be NULL, making it clear that the reference is invalid;
>this fact can be used to create a thin wrapper around xe_force_wake_put
>that can be used as a destructor for this class.
>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> drivers/gpu/drm/xe/xe_drm_client.c | 52 +++++++++++++++++++-----------
> 1 file changed, 34 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_drm_client.c b/drivers/gpu/drm/xe/xe_drm_client.c
>index f931ff9b1ec0..9deb258ba204 100644
>--- a/drivers/gpu/drm/xe/xe_drm_client.c
>+++ b/drivers/gpu/drm/xe/xe_drm_client.c
>@@ -6,6 +6,7 @@
> 
> #include <drm/drm_print.h>
> #include <uapi/drm/xe_drm.h>
>+#include <linux/cleanup.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/types.h>
>@@ -285,34 +286,48 @@ static struct xe_hw_engine *any_engine(struct xe_device *xe)
>         return NULL;
> }
> 
>-static bool force_wake_get_any_engine(struct xe_device *xe,
>-                                      struct xe_hw_engine **phwe,
>-                                      unsigned int *pfw_ref)
>+/*
>+ * Pick any engine and grab its forcewake.  On error phwe will be NULL and
>+ * the returned forcewake reference will be invalid.  Callers should check
>+ * phwe against NULL.
>+ */
>+static struct xe_force_wake_ref force_wake_get_any_engine(struct xe_device *xe,
>+                                                          struct xe_hw_engine **phwe)
> {
>         enum xe_force_wake_domains domain;
>-        unsigned int fw_ref;
>+        struct xe_force_wake_ref fw_ref = {};
>         struct xe_hw_engine *hwe;
>-        struct xe_force_wake *fw;
>+
>+        *phwe = NULL;
> 
>         hwe = any_engine(xe);
>         if (!hwe)
>-                return false;
>+                return fw_ref;        /* will be invalid */
> 
>         domain = xe_hw_engine_to_fw_domain(hwe);
>-        fw = gt_to_fw(hwe->gt);
> 
>-        fw_ref = xe_force_wake_get(fw, domain);
>-        if (!xe_force_wake_ref_has_domain(fw_ref, domain)) {
>-                xe_force_wake_put(fw, fw_ref);
>-                return false;
>-        }
>+        fw_ref.fw = gt_to_fw(hwe->gt);
>+        fw_ref.domains = xe_force_wake_get(fw_ref.fw, domain);

I think we should use xe_force_wake_constructor() here, to future-proof
this for any modification that we might decide to do in the way we force
wake CLASS constructors are implemented.

>+        if (xe_force_wake_ref_has_domain(fw_ref.domains, domain))
>+                *phwe = hwe;        /* valid forcewake */
> 
>-        *phwe = hwe;
>-        *pfw_ref = fw_ref;
>+        return fw_ref;
>+}
> 
>-        return true;
>+static void drop_fw_if_valid(struct xe_force_wake_ref fw_ref)
>+{
>+        /*
>+         * If force_wake_get_any_engine() fails, there's no real forcewake
>+         * reference to drop, and fw_ref.fw will be NULL.
>+         */
>+        if (fw_ref.fw)
>+                xe_force_wake_put(fw_ref.fw, fw_ref.domains);
> }
> 
>+DEFINE_CLASS(xe_force_wake_any_engine, struct xe_force_wake_ref,
>+             drop_fw_if_valid(_T), force_wake_get_any_engine(xe, phwe),
>+             struct xe_device *xe, struct xe_hw_engine **phwe);
>+

An alternative approach could be for xe_force_wake.h to have:

    DEFINE_CLASS(xe_force_wake_put_only, struct xe_force_wake_ref,
                 xe_force_wake_put(_T.fw, _T.domains),
                 fw_ref,
                 struct xe_force_wake_ref fw_ref);

Then, in this file, we would have something like:

    static void show_run_ticks(struct drm_printer *p, struct drm_file *file)
    {
        ...
        ...
        ...
        ...
        CLASS(xe_force_wake_put_only, fw_ref)(force_wake_get_any_engine(xe, &hwe));
        ...
        ...
        ...
    }

With that, we wouldn't need to create custom classes for special one-off
cases like this one.  What do you think?

PS: I also thought of using a DEFINE_FREE(), but I don't like the fact that
    the variable declaration would be explicit in the middle of
    show_run_ticks(). Using DEFINE_CLASS() we can hide that.

--
Gustavo Sousa

> static void show_run_ticks(struct drm_printer *p, struct drm_file *file)
> {
>         unsigned long class, i, gt_id, capacity[XE_ENGINE_CLASS_MAX] = { };
>@@ -322,7 +337,7 @@ static void show_run_ticks(struct drm_printer *p, struct drm_file *file)
>         struct xe_hw_engine *hwe;
>         struct xe_exec_queue *q;
>         u64 gpu_timestamp;
>-        unsigned int fw_ref;
>+        struct xe_force_wake_ref fw_ref;
> 
>         /*
>          * RING_TIMESTAMP registers are inaccessible in VF mode.
>@@ -340,7 +355,8 @@ static void show_run_ticks(struct drm_printer *p, struct drm_file *file)
>                        !atomic_read(&xef->exec_queue.pending_removal));
> 
>         xe_pm_runtime_get(xe);
>-        if (!force_wake_get_any_engine(xe, &hwe, &fw_ref)) {
>+        fw_ref = force_wake_get_any_engine(xe, &hwe);
>+        if (!hwe) {
>                 xe_pm_runtime_put(xe);
>                 return;
>         }
>@@ -360,7 +376,7 @@ static void show_run_ticks(struct drm_printer *p, struct drm_file *file)
> 
>         gpu_timestamp = xe_hw_engine_read_timestamp(hwe);
> 
>-        xe_force_wake_put(gt_to_fw(hwe->gt), fw_ref);
>+        xe_force_wake_put(gt_to_fw(hwe->gt), fw_ref.domains);
>         xe_pm_runtime_put(xe);
> 
>         for (class = 0; class < XE_ENGINE_CLASS_MAX; class++) {
>-- 
>2.51.1
>