[PATCH v2 1/1] drm/i915/pxp: Add missing tag for Wa_14019159160

[PATCH v2 1/1] drm/i915/pxp: Add missing tag for Wa_14019159160

[Alan Previn]

Add missing tag for "Wa_14019159160 - Case 2" (for existing
PXP code that ensures run alone mode bit is set to allow
PxP-decryption.

 v2: - Fix WA id number (John Harrison).
     - Improve comments and code to be specific
       for the targetted platforms (John Harrison)

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 7c367ba8d9dc..2959dfed2aa0 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -863,11 +863,13 @@ static bool ctx_needs_runalone(const struct intel_context *ce)
 	bool ctx_is_protected = false;
 
 	/*
-	 * On MTL and newer platforms, protected contexts require setting
-	 * the LRC run-alone bit or else the encryption will not happen.
+	 * Wa_14019159160 - Case 2: mtl
+	 * On some platforms, protected contexts require setting
+	 * the LRC run-alone bit or else the encryption/decryption will not happen.
+	 * NOTE: Case 2 only applies to PXP use-case of said workaround.
 	 */
-	if (GRAPHICS_VER_FULL(ce->engine->i915) >= IP_VER(12, 70) &&
-	    (ce->engine->class == COMPUTE_CLASS || ce->engine->class == RENDER_CLASS)) {
+	if (IS_METEORLAKE(ce->engine->i915) && (ce->engine->class == COMPUTE_CLASS ||
+						ce->engine->class == RENDER_CLASS)) {
 		rcu_read_lock();
 		gem_ctx = rcu_dereference(ce->gem_context);
 		if (gem_ctx)

base-commit: 5429d55de723544dfc0630cf39d96392052b27a1
-- 
2.39.0

Re: [PATCH v2 1/1] drm/i915/pxp: Add missing tag for Wa_14019159160

[Rodrigo Vivi]

On Wed, Nov 22, 2023 at 12:30:03PM -0800, Alan Previn wrote:
> Add missing tag for "Wa_14019159160 - Case 2" (for existing
> PXP code that ensures run alone mode bit is set to allow
> PxP-decryption.
> 
>  v2: - Fix WA id number (John Harrison).
>      - Improve comments and code to be specific
>        for the targetted platforms (John Harrison)
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 7c367ba8d9dc..2959dfed2aa0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -863,11 +863,13 @@ static bool ctx_needs_runalone(const struct intel_context *ce)
>  	bool ctx_is_protected = false;
>  
>  	/*
> -	 * On MTL and newer platforms, protected contexts require setting
> -	 * the LRC run-alone bit or else the encryption will not happen.
> +	 * Wa_14019159160 - Case 2: mtl
> +	 * On some platforms, protected contexts require setting
> +	 * the LRC run-alone bit or else the encryption/decryption will not happen.
> +	 * NOTE: Case 2 only applies to PXP use-case of said workaround.
>  	 */

hmm, interesting enough, on the wa description I read that it is incomplete for MTL/ARL
and something about a fuse bit. We should probably chase for some clarification in the
HSD?!

> -	if (GRAPHICS_VER_FULL(ce->engine->i915) >= IP_VER(12, 70) &&
> -	    (ce->engine->class == COMPUTE_CLASS || ce->engine->class == RENDER_CLASS)) {
> +	if (IS_METEORLAKE(ce->engine->i915) && (ce->engine->class == COMPUTE_CLASS ||
> +						ce->engine->class == RENDER_CLASS)) {

This check now excludes the ARL with the same IP, no?!

>  		rcu_read_lock();
>  		gem_ctx = rcu_dereference(ce->gem_context);
>  		if (gem_ctx)
> 
> base-commit: 5429d55de723544dfc0630cf39d96392052b27a1
> -- 
> 2.39.0
> 

Re: [PATCH v2 1/1] drm/i915/pxp: Add missing tag for Wa_14019159160

[Teres Alexis, Alan Previn]

On Mon, 2023-11-27 at 15:24 -0500, Vivi, Rodrigo wrote:
> On Wed, Nov 22, 2023 at 12:30:03PM -0800, Alan Previn wrote:
alan:snip
alan: thanks for reviewing and apologize for replying to this late.

> >  	/*
> > -	 * On MTL and newer platforms, protected contexts require setting
> > -	 * the LRC run-alone bit or else the encryption will not happen.
> > +	 * Wa_14019159160 - Case 2: mtl
> > +	 * On some platforms, protected contexts require setting
> > +	 * the LRC run-alone bit or else the encryption/decryption will not happen.
> > +	 * NOTE: Case 2 only applies to PXP use-case of said workaround.
> >  	 */
> 
> hmm, interesting enough, on the wa description I read that it is incomplete for MTL/ARL
> and something about a fuse bit. We should probably chase for some clarification in the
> HSD?!
alan: yes, i went through the HSD description with the architect and we both had agreed that
that from the KMD's perspective. At that time, the checking of the fuse from KMD's
could be optimized out for Case-2-PXP: if the fuses was set a certain way, KMD can skip setting
the bit in lrc because hw will enforce runalone in pxp mode irrespective of what KMD does but
if fuse was was not set that way KMD needed to set the runalone in lrc. So for case2, its simpler
to just turn it on always when the context is protected. However, that said, i realized the
wording of the HSDs have changed since the original patch was implemented so i will reclarify
offline and reply back. NOTE: i believe John Harrison is working on case-3 through a
separate series.

> 
> > -	if (GRAPHICS_VER_FULL(ce->engine->i915) >= IP_VER(12, 70) &&
> > -	    (ce->engine->class == COMPUTE_CLASS || ce->engine->class == RENDER_CLASS)) {
> > +	if (IS_METEORLAKE(ce->engine->i915) && (ce->engine->class == COMPUTE_CLASS ||
> > +						ce->engine->class == RENDER_CLASS)) {
> 
> This check now excludes the ARL with the same IP, no?!
alan: yeah - re-reved this part just now.
> 
> >  		rcu_read_lock();
> >  		gem_ctx = rcu_dereference(ce->gem_context);
> >  		if (gem_ctx)
> > 
> > base-commit: 5429d55de723544dfc0630cf39d96392052b27a1
> > -- 
> > 2.39.0
> > 

Re: [PATCH v2 1/1] drm/i915/pxp: Add missing tag for Wa_14019159160

[Teres Alexis, Alan Previn]

On Wed, 2023-11-29 at 13:13 -0800, Teres Alexis, Alan Previn wrote:
> On Mon, 2023-11-27 at 15:24 -0500, Vivi, Rodrigo wrote:
> > On Wed, Nov 22, 2023 at 12:30:03PM -0800, Alan Previn wrote:
> alan:snip
> alan: thanks for reviewing and apologize for replying to this late.
> 
> > >  	/*
> > > -	 * On MTL and newer platforms, protected contexts require setting
> > > -	 * the LRC run-alone bit or else the encryption will not happen.
> > > +	 * Wa_14019159160 - Case 2: mtl
> > > +	 * On some platforms, protected contexts require setting
> > > +	 * the LRC run-alone bit or else the encryption/decryption will not happen.
> > > +	 * NOTE: Case 2 only applies to PXP use-case of said workaround.
> > >  	 */
> > 
> > hmm, interesting enough, on the wa description I read that it is incomplete for MTL/ARL
> > and something about a fuse bit. We should probably chase for some clarification in the
> > HSD?!
> alan: yes, i went through the HSD description with the architect and we both had agreed that
> that from the KMD's perspective. At that time, the checking of the fuse from KMD's
> could be optimized out for Case-2-PXP: if the fuses was set a certain way, KMD can skip setting
> the bit in lrc because hw will enforce runalone in pxp mode irrespective of what KMD does but
> if fuse was was not set that way KMD needed to set the runalone in lrc. So for case2, its simpler
> to just turn it on always when the context is protected. However, that said, i realized the
> wording of the HSDs have changed since the original patch was implemented so i will reclarify
> offline and reply back. NOTE: i believe John Harrison is working on case-3 through a
> separate series.
alan: based on conversations with the architect, a different WA number, 14019725520,
has the details of case-2 that explains the relationship the new fuse.
However, as before it can be optimized as explained above where PXP context
need to always set this bit to ensure encryption/decryption engages.