Re: [PATCH v2 02/30] drm/xe/eustall: Store forcewake reference in stream structure

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

Quoting Matt Roper (2025-11-10 20:20:20-03:00)
>Calls to xe_force_wake_put() should generally pass the exact reference
>returned by xe_force_wake_get().  Since EU stall grabs and releases
>forcewake in different functions, xe_eu_stall_disable_locked() is
>currently calling put with a hardcoded RENDER domain.  Although this
>works for now, it's somewhat fragile in case the power domain(s)
>required by stall sampling change in the future, or if workarounds show
>up that require us to obtain additional domains.
>
>Stash the original reference obtained during stream enable inside the
>stream structure so that we can use it directly when the stream is
>disabled.
>
>Cc: Harish Chegondi <harish.chegondi@intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> drivers/gpu/drm/xe/xe_eu_stall.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_eu_stall.c b/drivers/gpu/drm/xe/xe_eu_stall.c
>index 650e45f6a7c7..97dfb7945b7a 100644
>--- a/drivers/gpu/drm/xe/xe_eu_stall.c
>+++ b/drivers/gpu/drm/xe/xe_eu_stall.c
>@@ -49,6 +49,7 @@ struct xe_eu_stall_data_stream {
>         wait_queue_head_t poll_wq;
>         size_t data_record_size;
>         size_t per_xecore_buf_size;
>+        unsigned int fw_ref;
> 
>         struct xe_gt *gt;
>         struct xe_bo *bo;
>@@ -660,13 +661,12 @@ static int xe_eu_stall_stream_enable(struct xe_eu_stall_data_stream *stream)
>         struct per_xecore_buf *xecore_buf;
>         struct xe_gt *gt = stream->gt;
>         u16 group, instance;
>-        unsigned int fw_ref;
>         int xecore;
> 
>         /* Take runtime pm ref and forcewake to disable RC6 */
>         xe_pm_runtime_get(gt_to_xe(gt));
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_RENDER);
>-        if (!xe_force_wake_ref_has_domain(fw_ref, XE_FW_RENDER)) {
>+        stream->fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_RENDER);
>+        if (!xe_force_wake_ref_has_domain(stream->fw_ref, XE_FW_RENDER)) {
>                 xe_gt_err(gt, "Failed to get RENDER forcewake\n");
>                 xe_pm_runtime_put(gt_to_xe(gt));
>                 return -ETIMEDOUT;
>@@ -832,7 +832,7 @@ static int xe_eu_stall_disable_locked(struct xe_eu_stall_data_stream *stream)
>                 xe_gt_mcr_multicast_write(gt, ROW_CHICKEN2,
>                                           _MASKED_BIT_DISABLE(DISABLE_DOP_GATING));
> 
>-        xe_force_wake_put(gt_to_fw(gt), XE_FW_RENDER);
>+        xe_force_wake_put(gt_to_fw(gt), stream->fw_ref);

I did a quick look and I would even say that this is also good to avoid
decrementing the refcount if the corresponding get failed and we somehow
ended up making this put call.

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>         xe_pm_runtime_put(gt_to_xe(gt));
> 
>         return 0;
>-- 
>2.51.1
>