[Intel-gfx] [PATCH v1] drm/i915/gsc: take a wakeref for the proxy-init-completion check

[Intel-gfx] [PATCH v1] drm/i915/gsc: take a wakeref for the proxy-init-completion check

[Alan Previn]

Ensure intel_gsc_uc_fw_init_done and intel_gsc_uc_fw_proxy_init
takes a wakeref before reading GSC Shim registers.

NOTE: another patch in review also adds a call from selftest to
this same function. (https://patchwork.freedesktop.org/series/117713/)
which is why i am adding the wakeref inside the callee, not the
caller.

Fixes: 99afb7cc8c44 ("drm/i915/pxp: Add ARB session creation and cleanup")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 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 f46eb17a7a98..1e5a8b2bdac9 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -27,7 +27,11 @@ static bool gsc_is_in_reset(struct intel_uncore *uncore)
 bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
 {
 	struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore;
-	u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
+	intel_wakeref_t wakeref;
+	u32 fw_status;
+
+	with_intel_runtime_pm(uncore->rpm, wakeref)
+		fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
 
 	return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) ==
 	       GSC_FW_PROXY_STATE_NORMAL;
@@ -36,7 +40,11 @@ bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
 bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)
 {
 	struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore;
-	u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
+	intel_wakeref_t wakeref;
+	u32 fw_status;
+
+	with_intel_runtime_pm(uncore->rpm, wakeref)
+		fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
 
 	return fw_status & GSC_FW_INIT_COMPLETE_BIT;
 }

base-commit: 27187d09511e1d47dbaaf91c7332319551a8edab
-- 
2.39.0

Re: [Intel-gfx] [PATCH v1] drm/i915/gsc: take a wakeref for the proxy-init-completion check

[Ceraolo Spurio, Daniele]

On 6/8/2023 11:04 AM, Alan Previn wrote:
> Ensure intel_gsc_uc_fw_init_done and intel_gsc_uc_fw_proxy_init
> takes a wakeref before reading GSC Shim registers.
>
> NOTE: another patch in review also adds a call from selftest to
> this same function. (https://patchwork.freedesktop.org/series/117713/)
> which is why i am adding the wakeref inside the callee, not the
> caller.
>
> Fixes: 99afb7cc8c44 ("drm/i915/pxp: Add ARB session creation and cleanup")
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 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 f46eb17a7a98..1e5a8b2bdac9 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -27,7 +27,11 @@ static bool gsc_is_in_reset(struct intel_uncore *uncore)
>   bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
>   {
>   	struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore;
> -	u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
> +	intel_wakeref_t wakeref;
> +	u32 fw_status;
> +
> +	with_intel_runtime_pm(uncore->rpm, wakeref)
> +		fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);

I think this could be moved to an helper (gsc_uc_get_fw_status?), so we 
don't have to re-do the wakeref in all the callers.

Daniele

>   
>   	return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) ==
>   	       GSC_FW_PROXY_STATE_NORMAL;
> @@ -36,7 +40,11 @@ bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
>   bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)
>   {
>   	struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore;
> -	u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
> +	intel_wakeref_t wakeref;
> +	u32 fw_status;
> +
> +	with_intel_runtime_pm(uncore->rpm, wakeref)
> +		fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
>   
>   	return fw_status & GSC_FW_INIT_COMPLETE_BIT;
>   }
>
> base-commit: 27187d09511e1d47dbaaf91c7332319551a8edab

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/gsc: take a wakeref for the proxy-init-completion check

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3964 bytes --]

== Series Details ==

Series: drm/i915/gsc: take a wakeref for the proxy-init-completion check
URL   : https://patchwork.freedesktop.org/series/119092/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13253 -> Patchwork_119092v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_119092v1/index.html

Participating hosts (35 -> 34)
------------------------------

  Missing    (1): fi-kbl-soraka 

Known issues
------------

  Here are the changes found in Patchwork_119092v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@reset:
    - bat-rpls-2:         NOTRUN -> [ABORT][1] ([i915#4983] / [i915#7461] / [i915#7913] / [i915#8347])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_119092v1/bat-rpls-2/igt@i915_selftest@live@reset.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - fi-glk-j4005:       NOTRUN -> [SKIP][2] ([fdo#109271])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_119092v1/fi-glk-j4005/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [DMESG-FAIL][3] ([i915#5334]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13253/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_119092v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@late_gt_pm:
    - fi-glk-j4005:       [ABORT][5] ([i915#6217]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13253/fi-glk-j4005/igt@i915_selftest@live@late_gt_pm.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_119092v1/fi-glk-j4005/igt@i915_selftest@live@late_gt_pm.html

  * igt@i915_selftest@live@mman:
    - bat-rpls-2:         [TIMEOUT][7] ([i915#6794] / [i915#7392]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13253/bat-rpls-2/igt@i915_selftest@live@mman.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_119092v1/bat-rpls-2/igt@i915_selftest@live@mman.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#4309]: https://gitlab.freedesktop.org/drm/intel/issues/4309
  [i915#4423]: https://gitlab.freedesktop.org/drm/intel/issues/4423
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6121]: https://gitlab.freedesktop.org/drm/intel/issues/6121
  [i915#6217]: https://gitlab.freedesktop.org/drm/intel/issues/6217
  [i915#6794]: https://gitlab.freedesktop.org/drm/intel/issues/6794
  [i915#6868]: https://gitlab.freedesktop.org/drm/intel/issues/6868
  [i915#7392]: https://gitlab.freedesktop.org/drm/intel/issues/7392
  [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#8347]: https://gitlab.freedesktop.org/drm/intel/issues/8347


Build changes
-------------

  * Linux: CI_DRM_13253 -> Patchwork_119092v1

  CI-20190529: 20190529
  CI_DRM_13253: b5faf562ea160e9dcd7f95b737753ce44134b266 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7322: 2dd77d6d827a308caae49ce3eba759c2bab394ed @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_119092v1: b5faf562ea160e9dcd7f95b737753ce44134b266 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

b75258a38534 drm/i915/gsc: take a wakeref for the proxy-init-completion check

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_119092v1/index.html

[-- Attachment #2: Type: text/html, Size: 4345 bytes --]

Re: [Intel-gfx] [PATCH v1] drm/i915/gsc: take a wakeref for the proxy-init-completion check

[Teres Alexis, Alan Previn]

On Thu, 2023-06-08 at 11:19 -0700, Ceraolo Spurio, Daniele wrote:
> On 6/8/2023 11:04 AM, Alan Previn wrote:
> > Ensure intel_gsc_uc_fw_init_done and intel_gsc_uc_fw_proxy_init
> > takes a wakeref before reading GSC Shim registers.
alan:snip
> 
> >   bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
> >   {
> >   	struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore;
> > -	u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
> > +	intel_wakeref_t wakeref;
> > +	u32 fw_status;
> > +
> > +	with_intel_runtime_pm(uncore->rpm, wakeref)
> > +		fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
> 
> I think this could be moved to an helper (gsc_uc_get_fw_status?), so we 
> don't have to re-do the wakeref in all the callers.
alan: thanks - will fix.