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

[Jani Nikula <jani.nikula@linux.intel.com>]

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