Re: [Intel-gfx] [PATCH v1] drm/i915/gsc: Fix intel_gsc_uc_fw_proxy_init_done with directed wakerefs

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

On 6/15/2023 2:19 PM, Alan Previn wrote:
> intel_gsc_uc_fw_proxy_init_done is used by a few code paths
> and usages. However, certain paths need a wakeref while others
> can't take a wakeref such as from the runtime_pm_resume callstack.
>
> Add a param into this helper to allow callers to direct whether
> to take the wakeref or not. This resolves the following bug:
>
>     INFO: task sh:2607 blocked for more than 61 seconds.
>     Not tainted 6.3.0-pxp-gsc-final-jun14+ #297
>     "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>     task:sh              state:D stack:13016 pid:2607  ppid:2602   flags:0x00004000
>     Call Trace:
>        <TASK>
>        __schedule+0x47b/0xe10
>        schedule+0x58/0xd0
>        rpm_resume+0x1cc/0x800
>        ? __pfx_autoremove_wake_function+0x10/0x10
>        __pm_runtime_resume+0x42/0x80
>        __intel_runtime_pm_get+0x19/0x80 [i915]
>        gsc_uc_get_fw_status+0x10/0x50 [i915]
>        intel_gsc_uc_fw_init_done+0x9/0x20 [i915]
>        intel_gsc_uc_load_start+0x5b/0x130 [i915]
>        __uc_resume+0xa5/0x280 [i915]
>        intel_runtime_resume+0xd4/0x250 [i915]
>        ? __pfx_pci_pm_runtime_resume+0x10/0x10
>     __rpm_callback+0x3c/0x160
>
> Fixes: 8c33c3755b75 ("drm/i915/gsc: take a wakeref for the proxy-init-completion check")
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

I'm going to send a trybot of this patch with the FW definition patch, 
just to make sure there aren't any other issues that kick in once the FW 
is defined and the code starts being executed, and merge if the results 
are ok.

Daniele

> ---
>   drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c  | 17 +++++++++++------
>   drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h  |  2 +-
>   drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c  |  2 +-
>   drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c |  2 +-
>   4 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> index 856de9af1e3a..ab1a456f833d 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -22,27 +22,32 @@ static bool gsc_is_in_reset(struct intel_uncore *uncore)
>   			HECI1_FWSTS1_CURRENT_STATE_RESET;
>   }
>   
> -static u32 gsc_uc_get_fw_status(struct intel_uncore *uncore)
> +static u32 gsc_uc_get_fw_status(struct intel_uncore *uncore, bool needs_wakeref)
>   {
>   	intel_wakeref_t wakeref;
>   	u32 fw_status = 0;
>   
> -	with_intel_runtime_pm(uncore->rpm, wakeref)
> -		fw_status = intel_uncore_read(uncore, HECI_FWSTS(MTL_GSC_HECI1_BASE, 1));
> +	if (needs_wakeref)
> +		wakeref = intel_runtime_pm_get(uncore->rpm);
>   
> +	fw_status = intel_uncore_read(uncore, HECI_FWSTS(MTL_GSC_HECI1_BASE, 1));
> +
> +	if (needs_wakeref)
> +		intel_runtime_pm_put(uncore->rpm, wakeref);
>   	return fw_status;
>   }
>   
> -bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
> +bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc, bool needs_wakeref)
>   {
>   	return REG_FIELD_GET(HECI1_FWSTS1_CURRENT_STATE,
> -			     gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore)) ==
> +			     gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore,
> +						  needs_wakeref)) ==
>   	       HECI1_FWSTS1_PROXY_STATE_NORMAL;
>   }
>   
>   bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)
>   {
> -	return gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore) &
> +	return gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore, false) &
>   	       HECI1_FWSTS1_INIT_COMPLETE;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> index 8d7b9e4f1ffc..ad2167ce9137 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> @@ -15,6 +15,6 @@ struct intel_uncore;
>   int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, size_t size);
>   int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc);
>   bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc);
> -bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc);
> +bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc, bool needs_wakeref);
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> index 85d90f0a15e3..75a3a0790ef3 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> @@ -72,7 +72,7 @@ static void gsc_work(struct work_struct *work)
>   			 * complete the request handling cleanly, so we need to check the
>   			 * status register to check if the proxy init was actually successful
>   			 */
> -			if (intel_gsc_uc_fw_proxy_init_done(gsc)) {
> +			if (intel_gsc_uc_fw_proxy_init_done(gsc, false)) {
>   				drm_dbg(&gt->i915->drm, "GSC Proxy initialized\n");
>   				intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_RUNNING);
>   			} else {
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> index f13890ec7db1..c7df47364013 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> @@ -197,7 +197,7 @@ bool intel_pxp_gsccs_is_ready_for_sessions(struct intel_pxp *pxp)
>   	 * are out of order) will suffice.
>   	 */
>   	if (intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc, INTEL_HUC_AUTH_BY_GSC) &&
> -	    intel_gsc_uc_fw_proxy_init_done(&pxp->ctrl_gt->uc.gsc))
> +	    intel_gsc_uc_fw_proxy_init_done(&pxp->ctrl_gt->uc.gsc, true))
>   		return true;
>   
>   	return false;
>
> base-commit: 134d180cacae82fadbc5ee32f86014cc290f5e0c