Re: [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS

2023-06-29 Thread Teres Alexis, Alan Previn 
On Tue, 2023-06-20 at 09:30 -0500, Balasubrawmanian, Vivaik wrote:
> On 6/1/2023 12:45 PM, Alan Previn wrote:
> > After recent discussions with Mesa folks, it was requested
> > that we optimize i915's GET_PARAM for the PXP_STATUS without
> > changing the UAPI spec.
> > 
> > This patch adds this additional optimizations:
> > - If any PXP initializatoin flow failed, then ensure that
> >   we catch it so that we can change the returned PXP_STATUS
> >   from "2" (i.e. 'PXP is supported but not yet ready')
> >   to "-ENODEV". This typically should not happen and if it
> >   does, we have a platform configuration.
> > - If a PXP arbitration session creation event failed
> >   due to incorrect firmware version or blocking SOC fusing
> >   or blocking BIOS configuration (platform reasons that won't
> >   change if we retry), then reflect that blockage by also
> >   returning -ENODEV in the GET_PARAM-PXP_STATUS.
> > - GET_PARAM:PXP_STATUS should not wait at all if PXP is
> >   supported but non-i915 dependencies (component-driver /
> >   firmware) we are still pending to complete the init flows.
> >   In this case, just return "2" immediately (i.e. 'PXP is
> >   supported but not yet ready').
> > 
> > Signed-off-by: Alan Previn 
> > ---
> >   drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c  | 11 +-
> >   drivers/gpu/drm/i915/i915_getparam.c   |  2 +-
> >   drivers/gpu/drm/i915/pxp/intel_pxp.c   | 25 ++
> >   drivers/gpu/drm/i915/pxp/intel_pxp.h   |  2 +-
> >   drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c |  7 +++---
> >   drivers/gpu/drm/i915/pxp/intel_pxp_tee.c   |  7 +++---
> >   drivers/gpu/drm/i915/pxp/intel_pxp_types.h |  9 
> >   7 files changed, 50 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> > index fb0984f875f9..4dd744c96a37 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> > @@ -42,8 +42,17 @@ static void gsc_work(struct work_struct *work)
> > }
> >   
> > ret = intel_gsc_proxy_request_handler(gsc);
> > -   if (ret)
> > +   if (ret) {
> > +   if (actions & GSC_ACTION_FW_LOAD) {
> > +   /*
> > +* a proxy request failure that came together 
> > with the
> > +* firmware load action means the last part of 
> > init has
> > +* failed so GSC fw won't be usable after this
> > +*/
> > +   intel_uc_fw_change_status(>fw, 
> > INTEL_UC_FIRMWARE_LOAD_FAIL);
> > +   }
> > goto out_put;
> > +   }
> >   
> > /* mark the GSC FW init as done the first time we run this */
> > if (actions & GSC_ACTION_FW_LOAD) {
> 
> On the huc authentication comment block above this snippet, the last 
> statement: "Note that we can only do the GSC auth if the GuC auth was" 
> is confusing as the code below is only dealing with HuC Authentication.
alan: i believe what he meant was "can only do the GSC-based auth if
the GuC-based auth"... but I can't change that code as part
of this patch - I believe the rules for kernel patch is to ensure each
single patch is modular (not mixing unrelated changes) and focuses just
on what its described to do. IIRC, we would need to create a separate
patch review for that change.

> 
> This function seems to have a section to deal with FW load action and 
> another to deal with SW Proxy requests, but we seem to be mixing both 
> actions in the SW proxy section. instead, can we move this call to 
> intel_gsc_proxy_request_handler to the FW load section itself instead of 
> handling it as an additional check in the SW_proxy section? In the same 
> vein, we should also move the intel_uc_fw_change_status() call into the 
> above FW Load action section. i think that way the code reads better.
alan: GSC_ACTION_FW_LOAD is used for loading the GSC firmware which is a
one-time thing per i915 load. However, GSC_ACTION_SW_PROXY events can happen
any time the GSC fw needs to communicate with CSE firmware (or vice versa)
due to platform events that may have not been triggered by i915 long after
init. However, the rule is after GSC FW is loaded, i915 is required
to do a 1-time proxy-init step to prime both GSC and CSE fws that proxy
comms is avail. without this step, we can't use the gsc-fw for other ops.

So to recap the rules:
1. we launch the worker to do the one-time the GSC firmware load.
2. after the GSC firmware load is successful, we have to do a one-time SW-proxy 
init.
-> this is why we add the GSC_ACTION_SW_PROXY flag successful load 
completion.
3. If we are doing proxy-handling for the very first time, we ensure
   -> FW status is only set to RUNNING if proxy int was

Re: [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS

2023-06-20 Thread Balasubrawmanian, Vivaik 

On 6/1/2023 12:45 PM, Alan Previn wrote:

After recent discussions with Mesa folks, it was requested
that we optimize i915's GET_PARAM for the PXP_STATUS without
changing the UAPI spec.

This patch adds this additional optimizations:
- If any PXP initializatoin flow failed, then ensure that
  we catch it so that we can change the returned PXP_STATUS
  from "2" (i.e. 'PXP is supported but not yet ready')
  to "-ENODEV". This typically should not happen and if it
  does, we have a platform configuration.
- If a PXP arbitration session creation event failed
  due to incorrect firmware version or blocking SOC fusing
  or blocking BIOS configuration (platform reasons that won't
  change if we retry), then reflect that blockage by also
  returning -ENODEV in the GET_PARAM-PXP_STATUS.
- GET_PARAM:PXP_STATUS should not wait at all if PXP is
  supported but non-i915 dependencies (component-driver /
  firmware) we are still pending to complete the init flows.
  In this case, just return "2" immediately (i.e. 'PXP is
  supported but not yet ready').

Signed-off-by: Alan Previn 
---
  drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c  | 11 +-
  drivers/gpu/drm/i915/i915_getparam.c   |  2 +-
  drivers/gpu/drm/i915/pxp/intel_pxp.c   | 25 ++
  drivers/gpu/drm/i915/pxp/intel_pxp.h   |  2 +-
  drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c |  7 +++---
  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c   |  7 +++---
  drivers/gpu/drm/i915/pxp/intel_pxp_types.h |  9 
  7 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c 
b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index fb0984f875f9..4dd744c96a37 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -42,8 +42,17 @@ static void gsc_work(struct work_struct *work)
}
  
  		ret = intel_gsc_proxy_request_handler(gsc);

-   if (ret)
+   if (ret) {
+   if (actions & GSC_ACTION_FW_LOAD) {
+   /*
+* a proxy request failure that came together 
with the
+* firmware load action means the last part of 
init has
+* failed so GSC fw won't be usable after this
+*/
+   intel_uc_fw_change_status(>fw, 
INTEL_UC_FIRMWARE_LOAD_FAIL);
+   }
goto out_put;
+   }
  
  		/* mark the GSC FW init as done the first time we run this */

if (actions & GSC_ACTION_FW_LOAD) {


On the huc authentication comment block above this snippet, the last 
statement: "Note that we can only do the GSC auth if the GuC auth was" 
is confusing as the code below is only dealing with HuC Authentication.


This function seems to have a section to deal with FW load action and 
another to deal with SW Proxy requests, but we seem to be mixing both 
actions in the SW proxy section. instead, can we move this call to 
intel_gsc_proxy_request_handler to the FW load section itself instead of 
handling it as an additional check in the SW_proxy section? In the same 
vein, we should also move the intel_uc_fw_change_status() call into the 
above FW Load action section. i think that way the code reads better.



diff --git a/drivers/gpu/drm/i915/i915_getparam.c 
b/drivers/gpu/drm/i915/i915_getparam.c
index 6f11d7eaa91a..1b2ee98a158a 100644
--- a/drivers/gpu/drm/i915/i915_getparam.c
+++ b/drivers/gpu/drm/i915/i915_getparam.c
@@ -105,7 +105,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
return value;
break;
case I915_PARAM_PXP_STATUS:
-   value = intel_pxp_get_readiness_status(i915->pxp);
+   value = intel_pxp_get_readiness_status(i915->pxp, 1);
if (value < 0)
return value;
break;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index bb2e15329f34..1478bb9b4e26 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -359,21 +359,38 @@ void intel_pxp_end(struct intel_pxp *pxp)
intel_runtime_pm_put(>runtime_pm, wakeref);
  }
  
+static bool pxp_required_fw_failed(struct intel_pxp *pxp)

+{
+   if (__intel_uc_fw_status(>ctrl_gt->uc.huc.fw) == 
INTEL_UC_FIRMWARE_LOAD_FAIL)
+   return true;
+   if (HAS_ENGINE(pxp->ctrl_gt, GSC0) &&
+   __intel_uc_fw_status(>ctrl_gt->uc.gsc.fw) == 
INTEL_UC_FIRMWARE_LOAD_FAIL)
+   return true;
+
+   return false;
+}
+
  /*
   * this helper is used by both intel_pxp_start and by
   * the GET_PARAM IOCTL that user space calls. Thus, the
   * return values here should match the UAPI spec.
   */
-int intel_pxp_get_readiness_status(struct

Re: [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS

2023-06-02 Thread Teres Alexis, Alan Previn 
Thanks Jani - will rev this up and fix these.

On Fri, 2023-06-02 at 16:03 +0300, Jani Nikula wrote:
> On Thu, 01 Jun 2023, Alan Previn  wrote:
> > After recent discussions with Mesa folks, it was requested
> > that we optimize i915's GET_PARAM for the PXP_STATUS without
> > changing the UAPI spec.
> > 
> > This patch adds this additional optimizations:
> 
> Nitpick, please avoid "This patch". It's redundant, and after the patch
> gets applied it becomes a commit, not a patch.
> 
> Instead, use the imperative mood, e.g. "Add these additional
> optimizations".
> 
> See 
> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
alan:snip

> 
> > -int intel_pxp_get_readiness_status(struct intel_pxp *pxp)
> > +int intel_pxp_get_readiness_status(struct intel_pxp *pxp, int timeout)
> 
> It would help the reader if you named the parameter timeout_ms. Assuming
> the unit is ms.
alan:snip

> > -is_fw_err_platform_config(u32 type)
> > +is_fw_err_platform_config(u32 type, struct intel_pxp *pxp)
> 
> It's customary to have the parameters ordered from higher context to
> lower.
> 

alan:snip

Re: [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS

2023-06-02 Thread Jani Nikula 
On Thu, 01 Jun 2023, Alan Previn  wrote:
> After recent discussions with Mesa folks, it was requested
> that we optimize i915's GET_PARAM for the PXP_STATUS without
> changing the UAPI spec.
>
> This patch adds this additional optimizations:

Nitpick, please avoid "This patch". It's redundant, and after the patch
gets applied it becomes a commit, not a patch.

Instead, use the imperative mood, e.g. "Add these additional
optimizations".

See 
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

>- If any PXP initializatoin flow failed, then ensure that
>  we catch it so that we can change the returned PXP_STATUS
>  from "2" (i.e. 'PXP is supported but not yet ready')
>  to "-ENODEV". This typically should not happen and if it
>  does, we have a platform configuration.
>- If a PXP arbitration session creation event failed
>  due to incorrect firmware version or blocking SOC fusing
>  or blocking BIOS configuration (platform reasons that won't
>  change if we retry), then reflect that blockage by also
>  returning -ENODEV in the GET_PARAM-PXP_STATUS.
>- GET_PARAM:PXP_STATUS should not wait at all if PXP is
>  supported but non-i915 dependencies (component-driver /
>  firmware) we are still pending to complete the init flows.
>  In this case, just return "2" immediately (i.e. 'PXP is
>  supported but not yet ready').
>
> Signed-off-by: Alan Previn 
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c  | 11 +-
>  drivers/gpu/drm/i915/i915_getparam.c   |  2 +-
>  drivers/gpu/drm/i915/pxp/intel_pxp.c   | 25 ++
>  drivers/gpu/drm/i915/pxp/intel_pxp.h   |  2 +-
>  drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c |  7 +++---
>  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c   |  7 +++---
>  drivers/gpu/drm/i915/pxp/intel_pxp_types.h |  9 
>  7 files changed, 50 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c 
> b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> index fb0984f875f9..4dd744c96a37 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> @@ -42,8 +42,17 @@ static void gsc_work(struct work_struct *work)
>   }
>  
>   ret = intel_gsc_proxy_request_handler(gsc);
> - if (ret)
> + if (ret) {
> + if (actions & GSC_ACTION_FW_LOAD) {
> + /*
> +  * a proxy request failure that came together 
> with the
> +  * firmware load action means the last part of 
> init has
> +  * failed so GSC fw won't be usable after this
> +  */
> + intel_uc_fw_change_status(>fw, 
> INTEL_UC_FIRMWARE_LOAD_FAIL);
> + }
>   goto out_put;
> + }
>  
>   /* mark the GSC FW init as done the first time we run this */
>   if (actions & GSC_ACTION_FW_LOAD) {
> diff --git a/drivers/gpu/drm/i915/i915_getparam.c 
> b/drivers/gpu/drm/i915/i915_getparam.c
> index 6f11d7eaa91a..1b2ee98a158a 100644
> --- a/drivers/gpu/drm/i915/i915_getparam.c
> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> @@ -105,7 +105,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void 
> *data,
>   return value;
>   break;
>   case I915_PARAM_PXP_STATUS:
> - value = intel_pxp_get_readiness_status(i915->pxp);
> + value = intel_pxp_get_readiness_status(i915->pxp, 1);
>   if (value < 0)
>   return value;
>   break;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c 
> b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index bb2e15329f34..1478bb9b4e26 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -359,21 +359,38 @@ void intel_pxp_end(struct intel_pxp *pxp)
>   intel_runtime_pm_put(>runtime_pm, wakeref);
>  }
>  
> +static bool pxp_required_fw_failed(struct intel_pxp *pxp)
> +{
> + if (__intel_uc_fw_status(>ctrl_gt->uc.huc.fw) == 
> INTEL_UC_FIRMWARE_LOAD_FAIL)
> + return true;
> + if (HAS_ENGINE(pxp->ctrl_gt, GSC0) &&
> + __intel_uc_fw_status(>ctrl_gt->uc.gsc.fw) == 
> INTEL_UC_FIRMWARE_LOAD_FAIL)
> + return true;
> +
> + return false;
> +}
> +
>  /*
>   * this helper is used by both intel_pxp_start and by
>   * the GET_PARAM IOCTL that user space calls. Thus, the
>   * return values here should match the UAPI spec.
>   */
> -int intel_pxp_get_readiness_status(struct intel_pxp *pxp)
> +int intel_pxp_get_readiness_status(struct intel_pxp *pxp, int timeout)

It would help the reader if you named the parameter timeout_ms. Assuming
the unit is ms.

>  {
>   if (!intel_pxp_is_enabled(pxp))
>   return -ENODEV;
>  
> + if (pxp_required_fw_failed(pxp))
> +