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

Ceraolo Spurio, Daniele daniele.ceraolospurio at intel.com
Tue Jun 20 18:05:31 UTC 2023



On 6/16/2023 8:54 AM, Ceraolo Spurio, Daniele wrote:
>
>
> 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 at intel.com>
>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at 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.

Trybot came back green 
(https://patchwork.freedesktop.org/series/119471/), so pushed.

Daniele

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



More information about the Intel-gfx mailing list