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

Posted by Ingyu Jang 1 year ago

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

Posted by Krzysztof Karas 1 year ago

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

Posted by Jani Nikula 1 year ago

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

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

Posted by Jani Nikula 1 year ago

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