[PATCH] drm/i915/gt: Handle INTEL_WAKEREF_DEF return value in gen8_ggtt_bind_get_ce

[PATCH] drm/i915/gt: Handle INTEL_WAKEREF_DEF return value in gen8_ggtt_bind_get_ce

[Ingyu Jang]

The function 'gen8_ggtt_bind_get_ce' previously did not handle the case
where 'intel_gt_pm_get_if_awake()' returns 'INTEL_WAKEREF_DEF'.
This value is returned when the 'intel_ref_tracker_alloc()' call within
'intel_gt_pm_get_if_awake()' fails to allocate due
to memory pressure or other constraints.

'intel_ref_tracker_alloc()' uses 'ref_tracker_alloc()' with the
'GFP_NOWAIT' flag, which can return 'NULL' if allocation fails.
In this case, the function explicitly returns 'INTEL_WAKEREF_DEF'.

This patch adds a check for 'INTEL_WAKEREF_DEF' in
'gen8_ggtt_bind_get_ce' to ensure that such errors are properly handled.
If 'INTEL_WAKEREF_DEF' is returned, the function returns 'NULL' .

Signed-off-by: Ingyu Jang <ingyujang25@gmail.com>
---
 drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index d60a6ca0cae5..8d22d8f2243d 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -311,7 +311,7 @@ static struct intel_context *gen8_ggtt_bind_get_ce(struct i915_ggtt *ggtt, intel
 	 * doing rpm_resume().
 	 */
 	*wakeref = intel_gt_pm_get_if_awake(gt);
-	if (!*wakeref)
+	if (!*wakeref || *wakeref == INTEL_WAKEREF_DEF)
 		return NULL;
 
 	intel_engine_pm_get(ce->engine);
-- 
2.34.1

Re: [PATCH] drm/i915/gt: Handle INTEL_WAKEREF_DEF return value in gen8_ggtt_bind_get_ce

[Jani Nikula]

On Fri, 17 Jan 2025, Ingyu Jang <ingyujang25@gmail.com> wrote:
> The function 'gen8_ggtt_bind_get_ce' previously did not handle the case
> where 'intel_gt_pm_get_if_awake()' returns 'INTEL_WAKEREF_DEF'.
> This value is returned when the 'intel_ref_tracker_alloc()' call within
> 'intel_gt_pm_get_if_awake()' fails to allocate due
> to memory pressure or other constraints.
>
> 'intel_ref_tracker_alloc()' uses 'ref_tracker_alloc()' with the
> 'GFP_NOWAIT' flag, which can return 'NULL' if allocation fails.
> In this case, the function explicitly returns 'INTEL_WAKEREF_DEF'.
>
> This patch adds a check for 'INTEL_WAKEREF_DEF' in
> 'gen8_ggtt_bind_get_ce' to ensure that such errors are properly handled.
> If 'INTEL_WAKEREF_DEF' is returned, the function returns 'NULL' .

No.

The callers should only handle NULL vs. non-NULL, and never make any
other assumptions about the value.

If intel_ref_tracker_alloc() fails, we'll only lose the wakeref tracking
debug aid, but everything else goes fine. The patch at hand conflates
the returned "asleep" (NULL) with "ref tracker fail", and that's wrong.

See how intel_ref_tracker_free() handles INTEL_WAKEREF_DEF.

BR,
Jani.


>
> Signed-off-by: Ingyu Jang <ingyujang25@gmail.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index d60a6ca0cae5..8d22d8f2243d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -311,7 +311,7 @@ static struct intel_context *gen8_ggtt_bind_get_ce(struct i915_ggtt *ggtt, intel
>  	 * doing rpm_resume().
>  	 */
>  	*wakeref = intel_gt_pm_get_if_awake(gt);
> -	if (!*wakeref)
> +	if (!*wakeref || *wakeref == INTEL_WAKEREF_DEF)
>  		return NULL;
>  
>  	intel_engine_pm_get(ce->engine);

-- 
Jani Nikula, Intel

✗ i915.CI.BAT: failure for drm/i915/gt: Handle INTEL_WAKEREF_DEF return value in gen8_ggtt_bind_get_ce

[Patchwork]

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

== Series Details ==

Series: drm/i915/gt: Handle INTEL_WAKEREF_DEF return value in gen8_ggtt_bind_get_ce
URL   : https://patchwork.freedesktop.org/series/143797/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_15993 -> Patchwork_143797v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_143797v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_143797v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

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

Participating hosts (39 -> 38)
------------------------------

  Missing    (1): fi-bsw-nick 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_143797v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live:
    - bat-mtlp-8:         [PASS][1] -> [INCOMPLETE][2] +1 other test incomplete
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15993/bat-mtlp-8/igt@i915_selftest@live.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143797v1/bat-mtlp-8/igt@i915_selftest@live.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@module-reload:
    - bat-rpls-4:         [PASS][3] -> [FAIL][4] ([i915#13401])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15993/bat-rpls-4/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143797v1/bat-rpls-4/igt@i915_pm_rpm@module-reload.html

  
#### Possible fixes ####

  * igt@dmabuf@all-tests:
    - bat-apl-1:          [INCOMPLETE][5] ([i915#12904]) -> [PASS][6] +1 other test pass
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15993/bat-apl-1/igt@dmabuf@all-tests.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143797v1/bat-apl-1/igt@dmabuf@all-tests.html

  * igt@i915_selftest@live@workarounds:
    - bat-arls-5:         [DMESG-FAIL][7] ([i915#12061]) -> [PASS][8] +1 other test pass
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15993/bat-arls-5/igt@i915_selftest@live@workarounds.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143797v1/bat-arls-5/igt@i915_selftest@live@workarounds.html
    - bat-arlh-2:         [DMESG-FAIL][9] ([i915#12061]) -> [PASS][10] +1 other test pass
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15993/bat-arlh-2/igt@i915_selftest@live@workarounds.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143797v1/bat-arlh-2/igt@i915_selftest@live@workarounds.html

  
  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [i915#12904]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12904
  [i915#13401]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13401


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

  * Linux: CI_DRM_15993 -> Patchwork_143797v1

  CI-20190529: 20190529
  CI_DRM_15993: 44010d7f04695da899c2860de32625a71011fbab @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8203: 3dce04be4ed4c0e729d80f70caff8ae8a869b5c0 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_143797v1: 44010d7f04695da899c2860de32625a71011fbab @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

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

Re: [PATCH] drm/i915/gt: Handle INTEL_WAKEREF_DEF return value in gen8_ggtt_bind_get_ce

[Krzysztof Karas]

Hi Ingyu,

> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index d60a6ca0cae5..8d22d8f2243d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -311,7 +311,7 @@ static struct intel_context *gen8_ggtt_bind_get_ce(struct i915_ggtt *ggtt, intel
>  	 * doing rpm_resume().
>  	 */
>  	*wakeref = intel_gt_pm_get_if_awake(gt);
> -	if (!*wakeref)
> +	if (!*wakeref || *wakeref == INTEL_WAKEREF_DEF)
INTEL_WAKEREF_DEF is a wrapper for an error pointer - how about
IS_ERR_OR_NULL() macro? Without going a bit deeper into the code
it is not apparent that INTEL_WAKEREF_DEF is indicating an error.

Nice catch nevertheless.

Krzysztof

>  		return NULL;
>  
>  	intel_engine_pm_get(ce->engine);
> -- 
> 2.34.1
> 

Re: [PATCH] drm/i915/gt: Handle INTEL_WAKEREF_DEF return value in gen8_ggtt_bind_get_ce

[Jani Nikula]

On Wed, 22 Jan 2025, Krzysztof Karas <krzysztof.karas@intel.com> wrote:
> Hi Ingyu,
>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
>> index d60a6ca0cae5..8d22d8f2243d 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
>> @@ -311,7 +311,7 @@ static struct intel_context *gen8_ggtt_bind_get_ce(struct i915_ggtt *ggtt, intel
>>  	 * doing rpm_resume().
>>  	 */
>>  	*wakeref = intel_gt_pm_get_if_awake(gt);
>> -	if (!*wakeref)
>> +	if (!*wakeref || *wakeref == INTEL_WAKEREF_DEF)
> INTEL_WAKEREF_DEF is a wrapper for an error pointer - how about
> IS_ERR_OR_NULL() macro? Without going a bit deeper into the code
> it is not apparent that INTEL_WAKEREF_DEF is indicating an error.
>
> Nice catch nevertheless.

It's not a nice catch. It's wrong [1].

BR,
Jani.


[1] https://lore.kernel.org/r/87cyglg9w2.fsf@intel.com


>
> Krzysztof
>
>>  		return NULL;
>>  
>>  	intel_engine_pm_get(ce->engine);
>> -- 
>> 2.34.1
>> 

-- 
Jani Nikula, Intel