On 10/14/2024 9:55 AM, Himal Prasad Ghimiray wrote:
> xe_force_wake_get() now returns the reference count-incremented domain
> mask. If it fails for individual domains, the return value will always
> be 0. However, for XE_FORCEWAKE_ALL, it may return a non-zero value even
> in the event of failure. Update the return handling of xe_force_wake_get()
> to reflect this behavior, and ensure that the return value is passed as
> input to xe_force_wake_put().
>
> v3
> - return xe_wakeref_t instead of int in xe_force_wake_get()
>
> v5
> - return unsigned int for xe_force_wake_get()
> - No need to WARN from caller in case of forcewake get failure.
>
> v7
> - Fix commit message
>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> Reviewed-by: Badal Nilawar <badal.nilawar at intel.com>
> ---
> drivers/gpu/drm/xe/xe_gsc.c | 23 +++++++++++------------
> drivers/gpu/drm/xe/xe_gsc_proxy.c | 9 ++++-----
> 2 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gsc.c b/drivers/gpu/drm/xe/xe_gsc.c
> index 783b09bf3681..1eb791ddc375 100644
> --- a/drivers/gpu/drm/xe/xe_gsc.c
> +++ b/drivers/gpu/drm/xe/xe_gsc.c
> @@ -261,19 +261,17 @@ static int gsc_upload_and_init(struct xe_gsc *gsc)
> {
> struct xe_gt *gt = gsc_to_gt(gsc);
> struct xe_tile *tile = gt_to_tile(gt);
> + unsigned int fw_ref;
> int ret;
>
> if (XE_WA(tile->primary_gt, 14018094691)) {
> - ret = xe_force_wake_get(gt_to_fw(tile->primary_gt), XE_FORCEWAKE_ALL);
> + fw_ref = xe_force_wake_get(gt_to_fw(tile->primary_gt), XE_FORCEWAKE_ALL);
>
> /*
> * If the forcewake fails we want to keep going, because the worst
> * case outcome in failing to apply the WA is that PXP won't work,
> - * which is not fatal. We still throw a warning so the issue is
> - * seen if it happens.
> + * which is not fatal. Forcewake get warns implicitly in case of failure
> */
> - xe_gt_WARN_ON(tile->primary_gt, ret);
> -
> xe_gt_mcr_multicast_write(tile->primary_gt,
> EU_SYSTOLIC_LIC_THROTTLE_CTL_WITH_LOCK,
> EU_SYSTOLIC_LIC_THROTTLE_CTL_LOCK_BIT);
> @@ -282,7 +280,7 @@ static int gsc_upload_and_init(struct xe_gsc *gsc)
> ret = gsc_upload(gsc);
>
> if (XE_WA(tile->primary_gt, 14018094691))
> - xe_force_wake_put(gt_to_fw(tile->primary_gt), XE_FORCEWAKE_ALL);
> + xe_force_wake_put(gt_to_fw(tile->primary_gt), fw_ref);
>
> if (ret)
> return ret;
> @@ -352,6 +350,7 @@ static void gsc_work(struct work_struct *work)
> struct xe_gsc *gsc = container_of(work, typeof(*gsc), work);
> struct xe_gt *gt = gsc_to_gt(gsc);
> struct xe_device *xe = gt_to_xe(gt);
> + unsigned int fw_ref;
> u32 actions;
> int ret;
>
> @@ -361,7 +360,7 @@ static void gsc_work(struct work_struct *work)
> spin_unlock_irq(&gsc->lock);
>
> xe_pm_runtime_get(xe);
> - xe_gt_WARN_ON(gt, xe_force_wake_get(gt_to_fw(gt), XE_FW_GSC));
> + fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GSC);
>
> if (actions & GSC_ACTION_ER_COMPLETE) {
> ret = gsc_er_complete(gt);
> @@ -381,7 +380,7 @@ static void gsc_work(struct work_struct *work)
> xe_gsc_proxy_request_handler(gsc);
>
> out:
> - xe_force_wake_put(gt_to_fw(gt), XE_FW_GSC);
> + xe_force_wake_put(gt_to_fw(gt), fw_ref);
> xe_pm_runtime_put(xe);
> }
>
> @@ -601,7 +600,7 @@ void xe_gsc_print_info(struct xe_gsc *gsc, struct drm_printer *p)
> {
> struct xe_gt *gt = gsc_to_gt(gsc);
> struct xe_mmio *mmio = >->mmio;
> - int err;
> + unsigned int fw_ref;
>
> xe_uc_fw_print(&gsc->fw, p);
>
> @@ -610,8 +609,8 @@ void xe_gsc_print_info(struct xe_gsc *gsc, struct drm_printer *p)
> if (!xe_uc_fw_is_enabled(&gsc->fw))
> return;
>
> - err = xe_force_wake_get(gt_to_fw(gt), XE_FW_GSC);
> - if (err)
> + fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GSC);
> + if (!fw_ref)
> return;
>
> drm_printf(p, "\nHECI1 FWSTS: 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x\n",
> @@ -622,5 +621,5 @@ void xe_gsc_print_info(struct xe_gsc *gsc, struct drm_printer *p)
> xe_mmio_read32(mmio, HECI_FWSTS5(MTL_GSC_HECI1_BASE)),
> xe_mmio_read32(mmio, HECI_FWSTS6(MTL_GSC_HECI1_BASE)));
>
> - xe_force_wake_put(gt_to_fw(gt), XE_FW_GSC);
> + xe_force_wake_put(gt_to_fw(gt), fw_ref);
> }
> diff --git a/drivers/gpu/drm/xe/xe_gsc_proxy.c b/drivers/gpu/drm/xe/xe_gsc_proxy.c
> index 6d89c22ae811..fc64b45d324b 100644
> --- a/drivers/gpu/drm/xe/xe_gsc_proxy.c
> +++ b/drivers/gpu/drm/xe/xe_gsc_proxy.c
> @@ -450,22 +450,21 @@ void xe_gsc_proxy_remove(struct xe_gsc *gsc)
> {
> struct xe_gt *gt = gsc_to_gt(gsc);
> struct xe_device *xe = gt_to_xe(gt);
> - int err = 0;
> + unsigned int fw_ref = 0;
nit: harmless init but not really needed
Reviewed-by: Nirmoy Das <nirmoy.das at intel.com>
>
> if (!gsc->proxy.component_added)
> return;
>
> /* disable HECI2 IRQs */
> xe_pm_runtime_get(xe);
> - err = xe_force_wake_get(gt_to_fw(gt), XE_FW_GSC);
> - if (err)
> + fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GSC);
> + if (!fw_ref)
> xe_gt_err(gt, "failed to get forcewake to disable GSC interrupts\n");
>
> /* try do disable irq even if forcewake failed */
> gsc_proxy_irq_toggle(gsc, false);
>
> - if (!err)
> - xe_force_wake_put(gt_to_fw(gt), XE_FW_GSC);
> + xe_force_wake_put(gt_to_fw(gt), fw_ref);
> xe_pm_runtime_put(xe);
>
> xe_gsc_wait_for_worker_completion(gsc);