Re: [Intel-gfx] [PATCH] drm/i915: Skip pxp init if gt is wedged

Re: [Intel-gfx] [PATCH] drm/i915: Skip pxp init if gt is wedged

[Jani Nikula]

On Thu, 26 Oct 2023, Zhanjun Dong <zhanjun.dong@intel.com> wrote:
> gt wedged is fatal error, skip the pxp init on this situation.

More information is needed in the commit message. When do you encounter
this situation?

I'll note that nobody checks intel_pxp_init() return status, so this
silently skips PXP.

BR,
Jani.

>
> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index dc327cf40b5a..923f233c91e1 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -212,6 +212,9 @@ int intel_pxp_init(struct drm_i915_private *i915)
>  	if (!gt)
>  		return -ENODEV;
>  
> +	if (intel_gt_is_wedged(gt))
> +		return -ENODEV;
> +
>  	/*
>  	 * At this point, we will either enable full featured PXP capabilities
>  	 * including session and object management, or we will init the backend tee

-- 
Jani Nikula, Intel

Re: [Intel-gfx] [PATCH] drm/i915: Skip pxp init if gt is wedged

[Teres Alexis, Alan Previn]

On Fri, 2023-10-27 at 10:13 +0300, Jani Nikula wrote:
> On Thu, 26 Oct 2023, Zhanjun Dong <zhanjun.dong@intel.com> wrote:
> 
alan:snip
> I'll note that nobody checks intel_pxp_init() return status, so this
> silently skips PXP.
> 
> BR,
> Jani.

alan:snip
> > +	if (intel_gt_is_wedged(gt))
> > +		return -ENODEV;
> > +

alan: wondering if we can add a drm_dbg in the caller of intel_pxp_init and
use a unique return value for the case of gt_is_wedged (for example: -ENXIO.).
As we know gt being wedged at startup basically means all gt usage is dead
and therefore we cant enable PXP (along with everything else that needs submission/
guc/ etc). With a drm-debug in the caller that prints that return value, it
helps us to differentiate between gt-is-wedged vs platform config doesnt support
PXP. However, this would mean new drm-debug 'noise' for platforms that i915 just
doesn't support PXP on at all which would be okay if dont use drm_warn or drm_err
and use 'softer' message like "PXP skipped with %d".

Please treat above comment as a "nit" - i.e. existing patch is good enough for me...
(after addressing Jani's request for more commit message info). ...alan

Re: [Intel-gfx] [PATCH] drm/i915: Skip pxp init if gt is wedged

[Dong, Zhanjun]

On 2023-10-31 5:38 p.m., Teres Alexis, Alan Previn wrote:
> On Fri, 2023-10-27 at 10:13 +0300, Jani Nikula wrote:
>> On Thu, 26 Oct 2023, Zhanjun Dong <zhanjun.dong@intel.com> wrote:
>>
> alan:snip
>> I'll note that nobody checks intel_pxp_init() return status, so this
>> silently skips PXP.
>>
>> BR,
>> Jani.
> 
> alan:snip
>>> +	if (intel_gt_is_wedged(gt))
>>> +		return -ENODEV;
>>> +
> 
> alan: wondering if we can add a drm_dbg in the caller of intel_pxp_init and
> use a unique return value for the case of gt_is_wedged (for example: -ENXIO.).
> As we know gt being wedged at startup basically means all gt usage is dead
> and therefore we cant enable PXP (along with everything else that needs submission/
> guc/ etc). With a drm-debug in the caller that prints that return value, it
> helps us to differentiate between gt-is-wedged vs platform config doesnt support
> PXP. However, this would mean new drm-debug 'noise' for platforms that i915 just
> doesn't support PXP on at all which would be okay if dont use drm_warn or drm_err
> and use 'softer' message like "PXP skipped with %d".
> 
> Please treat above comment as a "nit" - i.e. existing patch is good enough for me...
> (after addressing Jani's request for more commit message info). ...alan
> 
> 

Thanks Alan.
I agree, add more drm-debug looks like add noise in case of 
gt_is_wedged, existing code already output useful info.

If logs already let us know gt_wedged happens and we are not expect pxp 
init running on gt wedged condition, then silently skip pxp_init looks 
like match the expectation.

I will re-post with updated commit message later.

Regards,
Zhanjun

[PATCH] drm/i915: Skip pxp init if gt is wedged

[Zhanjun Dong]

The gt wedged could be triggered by missing guc firmware file, HW not
working, etc. Once triggered, it means all gt usage is dead, therefore we
can't enable pxp under this fatal error condition.

v2: Updated commit message.

Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
---
 drivers/gpu/drm/i915/pxp/intel_pxp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index dc327cf40b5a..923f233c91e1 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -212,6 +212,9 @@ int intel_pxp_init(struct drm_i915_private *i915)
 	if (!gt)
 		return -ENODEV;
 
+	if (intel_gt_is_wedged(gt))
+		return -ENODEV;
+
 	/*
 	 * At this point, we will either enable full featured PXP capabilities
 	 * including session and object management, or we will init the backend tee
-- 
2.34.1

Re: [Intel-gfx] [PATCH] drm/i915: Skip pxp init if gt is wedged

[Rodrigo Vivi]

On Wed, Nov 01, 2023 at 02:44:46PM -0700, Zhanjun Dong wrote:
> The gt wedged could be triggered by missing guc firmware file, HW not
> working, etc. Once triggered, it means all gt usage is dead, therefore we
> can't enable pxp under this fatal error condition.
> 
> v2: Updated commit message.
> 
> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index dc327cf40b5a..923f233c91e1 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -212,6 +212,9 @@ int intel_pxp_init(struct drm_i915_private *i915)
>  	if (!gt)
>  		return -ENODEV;
>  
> +	if (intel_gt_is_wedged(gt))
> +		return -ENODEV;

we need some error message.
also we need to fix the fact that these returns are entirely ignored.

or you convert this function to void and then add a message here before
return; on this case.

Or you return some different error and at probe if error but error different
then ENODEV you print a message saying pxp init failed.

but we need some log/error information. I honestly would prefer to convert to
void and print the msg here.

with some changes like this you can count with my rv-b

> +
>  	/*
>  	 * At this point, we will either enable full featured PXP capabilities
>  	 * including session and object management, or we will init the backend tee
> -- 
> 2.34.1
>