[RFC-v23,13/13] drm/i915/pxp: Add plane decryption support

Commit Message

Huang, Sean Z Jan. 19, 2021, 7:43 a.m. UTC

From: Anshuman Gupta <[e-mail]>

Add support to enable/disable PLANE_SURF Decryption Request bit.
It requires only to enable plane decryption support when following
condition met.
1. PXP session is enabled.
2. Buffer object is protected.

v2:
- Rebased to libva_cp-drm-tip_tgl_cp tree.
- Used gen fb obj user_flags instead gem_object_metadata. [Krishna]

v3:
- intel_pxp_gem_object_status() API changes.

Cc: Bommu Krishnaiah <[e-mail]>
Cc: Huang Sean Z <[e-mail]>
Cc: Gaurav Kumar <[e-mail]>
Signed-off-by: Anshuman Gupta <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_sprite.c | 21 ++++++++++++++++++---
 drivers/gpu/drm/i915/i915_reg.h             |  1 +
 2 files changed, 19 insertions(+), 3 deletions(-)

Comments

Gupta, Anshuman Jan. 19, 2021, 9:35 a.m. UTC | #1

Jani/Ville
I had received an offline comment form Gaurav on this patch,
See below,
> -----Original Message-----
> From: Huang, Sean Z <[e-mail]>
> Sent: Tuesday, January 19, 2021 1:13 PM
> To: [e-mail]
> Cc: Gaurav, Kumar <[e-mail]>; Gupta, Anshuman
> <[e-mail]>; Bommu, Krishnaiah
> <[e-mail]>; Huang, Sean Z <[e-mail]>
> Subject: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support
> 
> From: Anshuman Gupta <[e-mail]>
> 
> Add support to enable/disable PLANE_SURF Decryption Request bit.
> It requires only to enable plane decryption support when following
> condition met.
> 1. PXP session is enabled.
> 2. Buffer object is protected.
> 
> v2:
> - Rebased to libva_cp-drm-tip_tgl_cp tree.
> - Used gen fb obj user_flags instead gem_object_metadata. [Krishna]
> 
> v3:
> - intel_pxp_gem_object_status() API changes.
> 
> Cc: Bommu Krishnaiah <[e-mail]>
> Cc: Huang Sean Z <[e-mail]>
> Cc: Gaurav Kumar <[e-mail]>
> Signed-off-by: Anshuman Gupta <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_sprite.c | 21 ++++++++++++++++++---
>  drivers/gpu/drm/i915/i915_reg.h             |  1 +
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> b/drivers/gpu/drm/i915/display/intel_sprite.c
> index cf3589fd0ddb..39f8c922ce66 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -39,6 +39,8 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_rect.h>
> 
> +#include "pxp/intel_pxp.h"
> +
>  #include "i915_drv.h"
>  #include "i915_trace.h"
>  #include "i915_vgpu.h"
> @@ -768,6 +770,11 @@ icl_program_input_csc(struct intel_plane *plane,
>  			  PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2),
> 0x0);  }
> 
> +static bool intel_fb_obj_protected(const struct drm_i915_gem_object
> +*obj) {
> +	return obj->user_flags & I915_BO_PROTECTED ? true : false; }
> +
>  static void
>  skl_plane_async_flip(struct intel_plane *plane,
>  		     const struct intel_crtc_state *crtc_state, @@ -804,6
> +811,7 @@ skl_program_plane(struct intel_plane *plane,
>  	u32 surf_addr = plane_state->color_plane[color_plane].offset;
>  	u32 stride = skl_plane_stride(plane_state, color_plane);
>  	const struct drm_framebuffer *fb = plane_state->hw.fb;
> +	const struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	int aux_plane = intel_main_to_aux_plane(fb, color_plane);
>  	int crtc_x = plane_state->uapi.dst.x1;
>  	int crtc_y = plane_state->uapi.dst.y1; @@ -814,7 +822,7 @@
> skl_program_plane(struct intel_plane *plane,
>  	u8 alpha = plane_state->hw.alpha >> 8;
>  	u32 plane_color_ctl = 0, aux_dist = 0;
>  	unsigned long irqflags;
> -	u32 keymsk, keymax;
> +	u32 keymsk, keymax, plane_surf;
>  	u32 plane_ctl = plane_state->ctl;
> 
>  	plane_ctl |= skl_plane_ctl_crtc(crtc_state); @@ -890,8 +898,15
> @@ skl_program_plane(struct intel_plane *plane,
>  	 * the control register just before the surface register.
>  	 */
>  	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> -	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> -			  intel_plane_ggtt_offset(plane_state) + surf_addr);
> +	plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
> +
> +	if (intel_pxp_gem_object_status(dev_priv) &&
> +	    intel_fb_obj_protected(obj))
> +		plane_surf |= PLANE_SURF_DECRYPTION_ENABLED;
Here in case of if fb obj is protected but pxp session is not enabled i.e intel_pxp_gem_object_status() returns false, request to show the black frame buffer on display instead of corrupted data.
                            plane_surf = 0xXXX; //Pointer to black framebuffer
But above approach would be a hack. 
@Jani and @Ville could you please guide with the general way of handling this as pxp session keys can be invalidated at any time.

Thanks,
Anshuman Gupta.
> +	else
> +		plane_surf &= ~PLANE_SURF_DECRYPTION_ENABLED;
> +
> +	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> plane_surf);
> 
>  	if (plane_state->scaler_id >= 0)
>  		skl_program_scaler(plane, crtc_state, plane_state); diff --git
> a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 97bcecada87f..7a4817054bc8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7208,6 +7208,7 @@ enum {
>  #define _PLANE_SURF_3(pipe)	_PIPE(pipe, _PLANE_SURF_3_A,
> _PLANE_SURF_3_B)
>  #define PLANE_SURF(pipe, plane)	\
>  	_MMIO_PLANE(plane, _PLANE_SURF_1(pipe),
> _PLANE_SURF_2(pipe))
> +#define   PLANE_SURF_DECRYPTION_ENABLED		REG_BIT(2)
> 
>  #define _PLANE_OFFSET_1_B			0x711a4
>  #define _PLANE_OFFSET_2_B			0x712a4
> --
> 2.17.1

Ville Syrjälä Jan. 21, 2021, 8:30 p.m. UTC | #2

On Tue, Jan 19, 2021 at 09:35:18AM +0000, Gupta, Anshuman wrote:
> Jani/Ville
> I had received an offline comment form Gaurav on this patch,
> See below,
> > -----Original Message-----
> > From: Huang, Sean Z <[e-mail]>
> > Sent: Tuesday, January 19, 2021 1:13 PM
> > To: [e-mail]
> > Cc: Gaurav, Kumar <[e-mail]>; Gupta, Anshuman
> > <[e-mail]>; Bommu, Krishnaiah
> > <[e-mail]>; Huang, Sean Z <[e-mail]>
> > Subject: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support
> > 
> > From: Anshuman Gupta <[e-mail]>
> > 
> > Add support to enable/disable PLANE_SURF Decryption Request bit.
> > It requires only to enable plane decryption support when following
> > condition met.
> > 1. PXP session is enabled.
> > 2. Buffer object is protected.
> > 
> > v2:
> > - Rebased to libva_cp-drm-tip_tgl_cp tree.
> > - Used gen fb obj user_flags instead gem_object_metadata. [Krishna]
> > 
> > v3:
> > - intel_pxp_gem_object_status() API changes.
> > 
> > Cc: Bommu Krishnaiah <[e-mail]>
> > Cc: Huang Sean Z <[e-mail]>
> > Cc: Gaurav Kumar <[e-mail]>
> > Signed-off-by: Anshuman Gupta <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_sprite.c | 21 ++++++++++++++++++---
> >  drivers/gpu/drm/i915/i915_reg.h             |  1 +
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index cf3589fd0ddb..39f8c922ce66 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -39,6 +39,8 @@
> >  #include <drm/drm_plane_helper.h>
> >  #include <drm/drm_rect.h>
> > 
> > +#include "pxp/intel_pxp.h"
> > +
> >  #include "i915_drv.h"
> >  #include "i915_trace.h"
> >  #include "i915_vgpu.h"
> > @@ -768,6 +770,11 @@ icl_program_input_csc(struct intel_plane *plane,
> >  			  PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2),
> > 0x0);  }
> > 
> > +static bool intel_fb_obj_protected(const struct drm_i915_gem_object
> > +*obj) {
> > +	return obj->user_flags & I915_BO_PROTECTED ? true : false; }
> > +
> >  static void
> >  skl_plane_async_flip(struct intel_plane *plane,
> >  		     const struct intel_crtc_state *crtc_state, @@ -804,6
> > +811,7 @@ skl_program_plane(struct intel_plane *plane,
> >  	u32 surf_addr = plane_state->color_plane[color_plane].offset;
> >  	u32 stride = skl_plane_stride(plane_state, color_plane);
> >  	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > +	const struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >  	int aux_plane = intel_main_to_aux_plane(fb, color_plane);
> >  	int crtc_x = plane_state->uapi.dst.x1;
> >  	int crtc_y = plane_state->uapi.dst.y1; @@ -814,7 +822,7 @@
> > skl_program_plane(struct intel_plane *plane,
> >  	u8 alpha = plane_state->hw.alpha >> 8;
> >  	u32 plane_color_ctl = 0, aux_dist = 0;
> >  	unsigned long irqflags;
> > -	u32 keymsk, keymax;
> > +	u32 keymsk, keymax, plane_surf;
> >  	u32 plane_ctl = plane_state->ctl;
> > 
> >  	plane_ctl |= skl_plane_ctl_crtc(crtc_state); @@ -890,8 +898,15
> > @@ skl_program_plane(struct intel_plane *plane,
> >  	 * the control register just before the surface register.
> >  	 */
> >  	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> > -	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> > -			  intel_plane_ggtt_offset(plane_state) + surf_addr);
> > +	plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
> > +
> > +	if (intel_pxp_gem_object_status(dev_priv) &&
> > +	    intel_fb_obj_protected(obj))
> > +		plane_surf |= PLANE_SURF_DECRYPTION_ENABLED;
> Here in case of if fb obj is protected but pxp session is not enabled i.e intel_pxp_gem_object_status() returns false, request to show the black frame buffer on display instead of corrupted data.
>                             plane_surf = 0xXXX; //Pointer to black framebuffer
> But above approach would be a hack. 
> @Jani and @Ville could you please guide with the general way of handling this as pxp session keys can be invalidated at any time.

Would need such a black buffer to be always pinned into the gtt, which
is seems a bit wasteful. We could perhaps just force the plane to output
black eg. by using the plane gamma. I think we should always have the
per-plane gamma available on skl+ universal planes. Cursor may be a
different story.

Gaurav, Kumar Jan. 21, 2021, 8:50 p.m. UTC | #3

Thanks Anshuman for adding me for review.

Actually, using plane Gamma is good idea to show black frame. Another option could be alpha value since we know for ChromeOS protected buffer will always be flipped on overlays.

Below explanation captures need for black frame in i915 Display for HWDRM protected surfaces -
Problem Statement -
There is race condition between Ring3 and Ring0 where encrypted frame could be flipped by i915 Display despite Ring3 checking if HWDRM session keys are valid for encrypted frame.  

Google Bug -
BUG1 -[Intel] i915 framebuffer tracking (protected surfaces that can't be decrypted are being rendered as encrypted) -b/155511255

Background -
There are 4 high level pipelines working together in HWDRM playback.
1. CDM Pipeline -
App CDM SW Stack -> LibVA/iHD -> i915 -> MEI -> CSME-FW 

2. Media(Audio/Video) Pipeline
App Media SW Stack -> LibVA/iHD -> i915 -> GPU 

3. 3D Pipeline in Compositor
App Composition SW Stack -> OpenGL/MESA/MiniGBM -> i915 -> GPU/Display

4. Display Pipeline in Compositor
App Composition SW Stack -> Ozone/MiniGBM -> i915 -> Display

Discussion Point -
Even after Pipeline #4 is context robustness compliant there is a corner case/race condition for corruption as following  - BUG1
App's Composition SW Stack -> Creates Protected Context and Protected Buffer(MiniGBM)
App's Composition SW Stack -> Supplies Protected Buffer to LibVA/iHD -> i915 -> GPU -> Encrypted decoded output
App's Composition SW Stack -> Gets back decode output -> Checks for context robustness -> Submits frame for flip -> i915 Display(by the time i915 Display gets flip PAVP session is invalid despite being atomic since invalidation of PAVP is HW async event) -> Display HW -> Shows corruption


-----Original Message-----
From: Ville Syrjälä <[e-mail]> 
Sent: Thursday, January 21, 2021 12:31 PM
To: Gupta, Anshuman <[e-mail]>
Cc: Huang, Sean Z <[e-mail]>; [e-mail]; Nikula, Jani <[e-mail]>; Gaurav, Kumar <[e-mail]>; Bommu, Krishnaiah <[e-mail]>; Vetter, Daniel <[e-mail]>
Subject: Re: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support

On Tue, Jan 19, 2021 at 09:35:18AM +0000, Gupta, Anshuman wrote:
> Jani/Ville
> I had received an offline comment form Gaurav on this patch, See 
> below,
> > -----Original Message-----
> > From: Huang, Sean Z <[e-mail]>
> > Sent: Tuesday, January 19, 2021 1:13 PM
> > To: [e-mail]
> > Cc: Gaurav, Kumar <[e-mail]>; Gupta, Anshuman 
> > <[e-mail]>; Bommu, Krishnaiah 
> > <[e-mail]>; Huang, Sean Z <[e-mail]>
> > Subject: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support
> > 
> > From: Anshuman Gupta <[e-mail]>
> > 
> > Add support to enable/disable PLANE_SURF Decryption Request bit.
> > It requires only to enable plane decryption support when following 
> > condition met.
> > 1. PXP session is enabled.
> > 2. Buffer object is protected.
> > 
> > v2:
> > - Rebased to libva_cp-drm-tip_tgl_cp tree.
> > - Used gen fb obj user_flags instead gem_object_metadata. [Krishna]
> > 
> > v3:
> > - intel_pxp_gem_object_status() API changes.
> > 
> > Cc: Bommu Krishnaiah <[e-mail]>
> > Cc: Huang Sean Z <[e-mail]>
> > Cc: Gaurav Kumar <[e-mail]>
> > Signed-off-by: Anshuman Gupta <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_sprite.c | 21 ++++++++++++++++++---
> >  drivers/gpu/drm/i915/i915_reg.h             |  1 +
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index cf3589fd0ddb..39f8c922ce66 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -39,6 +39,8 @@
> >  #include <drm/drm_plane_helper.h>
> >  #include <drm/drm_rect.h>
> > 
> > +#include "pxp/intel_pxp.h"
> > +
> >  #include "i915_drv.h"
> >  #include "i915_trace.h"
> >  #include "i915_vgpu.h"
> > @@ -768,6 +770,11 @@ icl_program_input_csc(struct intel_plane *plane,
> >  			  PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0);  }
> > 
> > +static bool intel_fb_obj_protected(const struct drm_i915_gem_object
> > +*obj) {
> > +	return obj->user_flags & I915_BO_PROTECTED ? true : false; }
> > +
> >  static void
> >  skl_plane_async_flip(struct intel_plane *plane,
> >  		     const struct intel_crtc_state *crtc_state, @@ -804,6
> > +811,7 @@ skl_program_plane(struct intel_plane *plane,
> >  	u32 surf_addr = plane_state->color_plane[color_plane].offset;
> >  	u32 stride = skl_plane_stride(plane_state, color_plane);
> >  	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > +	const struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >  	int aux_plane = intel_main_to_aux_plane(fb, color_plane);
> >  	int crtc_x = plane_state->uapi.dst.x1;
> >  	int crtc_y = plane_state->uapi.dst.y1; @@ -814,7 +822,7 @@ 
> > skl_program_plane(struct intel_plane *plane,
> >  	u8 alpha = plane_state->hw.alpha >> 8;
> >  	u32 plane_color_ctl = 0, aux_dist = 0;
> >  	unsigned long irqflags;
> > -	u32 keymsk, keymax;
> > +	u32 keymsk, keymax, plane_surf;
> >  	u32 plane_ctl = plane_state->ctl;
> > 
> >  	plane_ctl |= skl_plane_ctl_crtc(crtc_state); @@ -890,8 +898,15 @@ 
> > skl_program_plane(struct intel_plane *plane,
> >  	 * the control register just before the surface register.
> >  	 */
> >  	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> > -	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> > -			  intel_plane_ggtt_offset(plane_state) + surf_addr);
> > +	plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
> > +
> > +	if (intel_pxp_gem_object_status(dev_priv) &&
> > +	    intel_fb_obj_protected(obj))
> > +		plane_surf |= PLANE_SURF_DECRYPTION_ENABLED;
> Here in case of if fb obj is protected but pxp session is not enabled i.e intel_pxp_gem_object_status() returns false, request to show the black frame buffer on display instead of corrupted data.
>                             plane_surf = 0xXXX; //Pointer to black 
> framebuffer But above approach would be a hack.
> @Jani and @Ville could you please guide with the general way of handling this as pxp session keys can be invalidated at any time.

Would need such a black buffer to be always pinned into the gtt, which is seems a bit wasteful. We could perhaps just force the plane to output black eg. by using the plane gamma. I think we should always have the per-plane gamma available on skl+ universal planes. Cursor may be a different story.

--
Ville Syrjälä
Intel

Ville Syrjälä Jan. 21, 2021, 9 p.m. UTC | #4

On Thu, Jan 21, 2021 at 08:50:18PM +0000, Gaurav, Kumar wrote:
> Thanks Anshuman for adding me for review.
> 
> Actually, using plane Gamma is good idea to show black frame. Another option could be alpha value since we know for ChromeOS protected buffer will always be flipped on overlays.
> 
> Below explanation captures need for black frame in i915 Display for HWDRM protected surfaces -
> Problem Statement -
> There is race condition between Ring3 and Ring0 where encrypted frame could be flipped by i915 Display despite Ring3 checking if HWDRM session keys are valid for encrypted frame.  
> 
> Google Bug -
> BUG1 -[Intel] i915 framebuffer tracking (protected surfaces that can't be decrypted are being rendered as encrypted) -b/155511255
> 
> Background -
> There are 4 high level pipelines working together in HWDRM playback.
> 1. CDM Pipeline -
> App CDM SW Stack -> LibVA/iHD -> i915 -> MEI -> CSME-FW 
> 
> 2. Media(Audio/Video) Pipeline
> App Media SW Stack -> LibVA/iHD -> i915 -> GPU 
> 
> 3. 3D Pipeline in Compositor
> App Composition SW Stack -> OpenGL/MESA/MiniGBM -> i915 -> GPU/Display
> 
> 4. Display Pipeline in Compositor
> App Composition SW Stack -> Ozone/MiniGBM -> i915 -> Display
> 
> Discussion Point -
> Even after Pipeline #4 is context robustness compliant there is a corner case/race condition for corruption as following  - BUG1
> App's Composition SW Stack -> Creates Protected Context and Protected Buffer(MiniGBM)
> App's Composition SW Stack -> Supplies Protected Buffer to LibVA/iHD -> i915 -> GPU -> Encrypted decoded output
> App's Composition SW Stack -> Gets back decode output -> Checks for context robustness -> Submits frame for flip -> i915 Display(by the time i915 Display gets flip PAVP session is invalid despite being atomic since invalidation of PAVP is HW async event) -> Display HW -> Shows corruption

It'll always be racy unless the invalidation becomes a two stage process
that first tells display to stop scanning out the thing and then waits
for the display to finish scanning out the current frame.

> 
> 
> -----Original Message-----
> From: Ville Syrjälä <[e-mail]> 
> Sent: Thursday, January 21, 2021 12:31 PM
> To: Gupta, Anshuman <[e-mail]>
> Cc: Huang, Sean Z <[e-mail]>; [e-mail]; Nikula, Jani <[e-mail]>; Gaurav, Kumar <[e-mail]>; Bommu, Krishnaiah <[e-mail]>; Vetter, Daniel <[e-mail]>
> Subject: Re: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support
> 
> On Tue, Jan 19, 2021 at 09:35:18AM +0000, Gupta, Anshuman wrote:
> > Jani/Ville
> > I had received an offline comment form Gaurav on this patch, See 
> > below,
> > > -----Original Message-----
> > > From: Huang, Sean Z <[e-mail]>
> > > Sent: Tuesday, January 19, 2021 1:13 PM
> > > To: [e-mail]
> > > Cc: Gaurav, Kumar <[e-mail]>; Gupta, Anshuman 
> > > <[e-mail]>; Bommu, Krishnaiah 
> > > <[e-mail]>; Huang, Sean Z <[e-mail]>
> > > Subject: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support
> > > 
> > > From: Anshuman Gupta <[e-mail]>
> > > 
> > > Add support to enable/disable PLANE_SURF Decryption Request bit.
> > > It requires only to enable plane decryption support when following 
> > > condition met.
> > > 1. PXP session is enabled.
> > > 2. Buffer object is protected.
> > > 
> > > v2:
> > > - Rebased to libva_cp-drm-tip_tgl_cp tree.
> > > - Used gen fb obj user_flags instead gem_object_metadata. [Krishna]
> > > 
> > > v3:
> > > - intel_pxp_gem_object_status() API changes.
> > > 
> > > Cc: Bommu Krishnaiah <[e-mail]>
> > > Cc: Huang Sean Z <[e-mail]>
> > > Cc: Gaurav Kumar <[e-mail]>
> > > Signed-off-by: Anshuman Gupta <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_sprite.c | 21 ++++++++++++++++++---
> > >  drivers/gpu/drm/i915/i915_reg.h             |  1 +
> > >  2 files changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > index cf3589fd0ddb..39f8c922ce66 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > @@ -39,6 +39,8 @@
> > >  #include <drm/drm_plane_helper.h>
> > >  #include <drm/drm_rect.h>
> > > 
> > > +#include "pxp/intel_pxp.h"
> > > +
> > >  #include "i915_drv.h"
> > >  #include "i915_trace.h"
> > >  #include "i915_vgpu.h"
> > > @@ -768,6 +770,11 @@ icl_program_input_csc(struct intel_plane *plane,
> > >  			  PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0);  }
> > > 
> > > +static bool intel_fb_obj_protected(const struct drm_i915_gem_object
> > > +*obj) {
> > > +	return obj->user_flags & I915_BO_PROTECTED ? true : false; }
> > > +
> > >  static void
> > >  skl_plane_async_flip(struct intel_plane *plane,
> > >  		     const struct intel_crtc_state *crtc_state, @@ -804,6
> > > +811,7 @@ skl_program_plane(struct intel_plane *plane,
> > >  	u32 surf_addr = plane_state->color_plane[color_plane].offset;
> > >  	u32 stride = skl_plane_stride(plane_state, color_plane);
> > >  	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > +	const struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > >  	int aux_plane = intel_main_to_aux_plane(fb, color_plane);
> > >  	int crtc_x = plane_state->uapi.dst.x1;
> > >  	int crtc_y = plane_state->uapi.dst.y1; @@ -814,7 +822,7 @@ 
> > > skl_program_plane(struct intel_plane *plane,
> > >  	u8 alpha = plane_state->hw.alpha >> 8;
> > >  	u32 plane_color_ctl = 0, aux_dist = 0;
> > >  	unsigned long irqflags;
> > > -	u32 keymsk, keymax;
> > > +	u32 keymsk, keymax, plane_surf;
> > >  	u32 plane_ctl = plane_state->ctl;
> > > 
> > >  	plane_ctl |= skl_plane_ctl_crtc(crtc_state); @@ -890,8 +898,15 @@ 
> > > skl_program_plane(struct intel_plane *plane,
> > >  	 * the control register just before the surface register.
> > >  	 */
> > >  	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> > > -	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> > > -			  intel_plane_ggtt_offset(plane_state) + surf_addr);
> > > +	plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
> > > +
> > > +	if (intel_pxp_gem_object_status(dev_priv) &&
> > > +	    intel_fb_obj_protected(obj))
> > > +		plane_surf |= PLANE_SURF_DECRYPTION_ENABLED;
> > Here in case of if fb obj is protected but pxp session is not enabled i.e intel_pxp_gem_object_status() returns false, request to show the black frame buffer on display instead of corrupted data.
> >                             plane_surf = 0xXXX; //Pointer to black 
> > framebuffer But above approach would be a hack.
> > @Jani and @Ville could you please guide with the general way of handling this as pxp session keys can be invalidated at any time.
> 
> Would need such a black buffer to be always pinned into the gtt, which is seems a bit wasteful. We could perhaps just force the plane to output black eg. by using the plane gamma. I think we should always have the per-plane gamma available on skl+ universal planes. Cursor may be a different story.
> 
> --
> Ville Syrjälä
> Intel

Gaurav, Kumar Jan. 21, 2021, 9:34 p.m. UTC | #5

So the idea is to perform HWDRM session check in flip IRQL and i915 PXP will guarantee that IRQL is not blocked. 
We have done similar check in Windows flip IRQL.  

-----Original Message-----
From: Ville Syrjälä <[e-mail]> 
Sent: Thursday, January 21, 2021 1:00 PM
To: Gaurav, Kumar <[e-mail]>
Cc: Gupta, Anshuman <[e-mail]>; Huang, Sean Z <[e-mail]>; [e-mail]; Nikula, Jani <[e-mail]>; Bommu, Krishnaiah <[e-mail]>; Vetter, Daniel <[e-mail]>
Subject: Re: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support

On Thu, Jan 21, 2021 at 08:50:18PM +0000, Gaurav, Kumar wrote:
> Thanks Anshuman for adding me for review.
> 
> Actually, using plane Gamma is good idea to show black frame. Another option could be alpha value since we know for ChromeOS protected buffer will always be flipped on overlays.
> 
> Below explanation captures need for black frame in i915 Display for 
> HWDRM protected surfaces - Problem Statement - There is race condition 
> between Ring3 and Ring0 where encrypted frame could be flipped by i915 Display despite Ring3 checking if HWDRM session keys are valid for encrypted frame.
> 
> Google Bug -
> BUG1 -[Intel] i915 framebuffer tracking (protected surfaces that can't 
> be decrypted are being rendered as encrypted) -b/155511255
> 
> Background -
> There are 4 high level pipelines working together in HWDRM playback.
> 1. CDM Pipeline -
> App CDM SW Stack -> LibVA/iHD -> i915 -> MEI -> CSME-FW
> 
> 2. Media(Audio/Video) Pipeline
> App Media SW Stack -> LibVA/iHD -> i915 -> GPU
> 
> 3. 3D Pipeline in Compositor
> App Composition SW Stack -> OpenGL/MESA/MiniGBM -> i915 -> GPU/Display
> 
> 4. Display Pipeline in Compositor
> App Composition SW Stack -> Ozone/MiniGBM -> i915 -> Display
> 
> Discussion Point -
> Even after Pipeline #4 is context robustness compliant there is a 
> corner case/race condition for corruption as following  - BUG1 App's 
> Composition SW Stack -> Creates Protected Context and Protected 
> Buffer(MiniGBM) App's Composition SW Stack -> Supplies Protected 
> Buffer to LibVA/iHD -> i915 -> GPU -> Encrypted decoded output App's 
> Composition SW Stack -> Gets back decode output -> Checks for context 
> robustness -> Submits frame for flip -> i915 Display(by the time i915 
> Display gets flip PAVP session is invalid despite being atomic since 
> invalidation of PAVP is HW async event) -> Display HW -> Shows 
> corruption

It'll always be racy unless the invalidation becomes a two stage process that first tells display to stop scanning out the thing and then waits for the display to finish scanning out the current frame.

> 
> 
> -----Original Message-----
> From: Ville Syrjälä <[e-mail]>
> Sent: Thursday, January 21, 2021 12:31 PM
> To: Gupta, Anshuman <[e-mail]>
> Cc: Huang, Sean Z <[e-mail]>; 
> [e-mail]; Nikula, Jani <[e-mail]>; 
> Gaurav, Kumar <[e-mail]>; Bommu, Krishnaiah 
> <[e-mail]>; Vetter, Daniel <[e-mail]>
> Subject: Re: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption 
> support
> 
> On Tue, Jan 19, 2021 at 09:35:18AM +0000, Gupta, Anshuman wrote:
> > Jani/Ville
> > I had received an offline comment form Gaurav on this patch, See 
> > below,
> > > -----Original Message-----
> > > From: Huang, Sean Z <[e-mail]>
> > > Sent: Tuesday, January 19, 2021 1:13 PM
> > > To: [e-mail]
> > > Cc: Gaurav, Kumar <[e-mail]>; Gupta, Anshuman 
> > > <[e-mail]>; Bommu, Krishnaiah 
> > > <[e-mail]>; Huang, Sean Z 
> > > <[e-mail]>
> > > Subject: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption 
> > > support
> > > 
> > > From: Anshuman Gupta <[e-mail]>
> > > 
> > > Add support to enable/disable PLANE_SURF Decryption Request bit.
> > > It requires only to enable plane decryption support when following 
> > > condition met.
> > > 1. PXP session is enabled.
> > > 2. Buffer object is protected.
> > > 
> > > v2:
> > > - Rebased to libva_cp-drm-tip_tgl_cp tree.
> > > - Used gen fb obj user_flags instead gem_object_metadata. 
> > > [Krishna]
> > > 
> > > v3:
> > > - intel_pxp_gem_object_status() API changes.
> > > 
> > > Cc: Bommu Krishnaiah <[e-mail]>
> > > Cc: Huang Sean Z <[e-mail]>
> > > Cc: Gaurav Kumar <[e-mail]>
> > > Signed-off-by: Anshuman Gupta <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_sprite.c | 21 ++++++++++++++++++---
> > >  drivers/gpu/drm/i915/i915_reg.h             |  1 +
> > >  2 files changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > index cf3589fd0ddb..39f8c922ce66 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > @@ -39,6 +39,8 @@
> > >  #include <drm/drm_plane_helper.h>  #include <drm/drm_rect.h>
> > > 
> > > +#include "pxp/intel_pxp.h"
> > > +
> > >  #include "i915_drv.h"
> > >  #include "i915_trace.h"
> > >  #include "i915_vgpu.h"
> > > @@ -768,6 +770,11 @@ icl_program_input_csc(struct intel_plane *plane,
> > >  			  PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0);  }
> > > 
> > > +static bool intel_fb_obj_protected(const struct 
> > > +drm_i915_gem_object
> > > +*obj) {
> > > +	return obj->user_flags & I915_BO_PROTECTED ? true : false; }
> > > +
> > >  static void
> > >  skl_plane_async_flip(struct intel_plane *plane,
> > >  		     const struct intel_crtc_state *crtc_state, @@ -804,6
> > > +811,7 @@ skl_program_plane(struct intel_plane *plane,
> > >  	u32 surf_addr = plane_state->color_plane[color_plane].offset;
> > >  	u32 stride = skl_plane_stride(plane_state, color_plane);
> > >  	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > +	const struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > >  	int aux_plane = intel_main_to_aux_plane(fb, color_plane);
> > >  	int crtc_x = plane_state->uapi.dst.x1;
> > >  	int crtc_y = plane_state->uapi.dst.y1; @@ -814,7 +822,7 @@ 
> > > skl_program_plane(struct intel_plane *plane,
> > >  	u8 alpha = plane_state->hw.alpha >> 8;
> > >  	u32 plane_color_ctl = 0, aux_dist = 0;
> > >  	unsigned long irqflags;
> > > -	u32 keymsk, keymax;
> > > +	u32 keymsk, keymax, plane_surf;
> > >  	u32 plane_ctl = plane_state->ctl;
> > > 
> > >  	plane_ctl |= skl_plane_ctl_crtc(crtc_state); @@ -890,8 +898,15 
> > > @@ skl_program_plane(struct intel_plane *plane,
> > >  	 * the control register just before the surface register.
> > >  	 */
> > >  	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> > > -	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> > > -			  intel_plane_ggtt_offset(plane_state) + surf_addr);
> > > +	plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
> > > +
> > > +	if (intel_pxp_gem_object_status(dev_priv) &&
> > > +	    intel_fb_obj_protected(obj))
> > > +		plane_surf |= PLANE_SURF_DECRYPTION_ENABLED;
> > Here in case of if fb obj is protected but pxp session is not enabled i.e intel_pxp_gem_object_status() returns false, request to show the black frame buffer on display instead of corrupted data.
> >                             plane_surf = 0xXXX; //Pointer to black 
> > framebuffer But above approach would be a hack.
> > @Jani and @Ville could you please guide with the general way of handling this as pxp session keys can be invalidated at any time.
> 
> Would need such a black buffer to be always pinned into the gtt, which is seems a bit wasteful. We could perhaps just force the plane to output black eg. by using the plane gamma. I think we should always have the per-plane gamma available on skl+ universal planes. Cursor may be a different story.
> 
> --
> Ville Syrjälä
> Intel

--
Ville Syrjälä
Intel

Ville Syrjälä Jan. 22, 2021, 11:58 a.m. UTC | #6

On Thu, Jan 21, 2021 at 09:34:53PM +0000, Gaurav, Kumar wrote:
> So the idea is to perform HWDRM session check in flip IRQL and i915 PXP will guarantee that IRQL is not blocked. 
> We have done similar check in Windows flip IRQL.  
> 

What's an flip IRQL? Some sort of flip related irq handler?

a) we don't use the flip done interrupt (except for async flips), if
   that's what you're referring to
b) how would doing anything there help against the race?

> -----Original Message-----
> From: Ville Syrjälä <[e-mail]> 
> Sent: Thursday, January 21, 2021 1:00 PM
> To: Gaurav, Kumar <[e-mail]>
> Cc: Gupta, Anshuman <[e-mail]>; Huang, Sean Z <[e-mail]>; [e-mail]; Nikula, Jani <[e-mail]>; Bommu, Krishnaiah <[e-mail]>; Vetter, Daniel <[e-mail]>
> Subject: Re: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support
> 
> On Thu, Jan 21, 2021 at 08:50:18PM +0000, Gaurav, Kumar wrote:
> > Thanks Anshuman for adding me for review.
> > 
> > Actually, using plane Gamma is good idea to show black frame. Another option could be alpha value since we know for ChromeOS protected buffer will always be flipped on overlays.
> > 
> > Below explanation captures need for black frame in i915 Display for 
> > HWDRM protected surfaces - Problem Statement - There is race condition 
> > between Ring3 and Ring0 where encrypted frame could be flipped by i915 Display despite Ring3 checking if HWDRM session keys are valid for encrypted frame.
> > 
> > Google Bug -
> > BUG1 -[Intel] i915 framebuffer tracking (protected surfaces that can't 
> > be decrypted are being rendered as encrypted) -b/155511255
> > 
> > Background -
> > There are 4 high level pipelines working together in HWDRM playback.
> > 1. CDM Pipeline -
> > App CDM SW Stack -> LibVA/iHD -> i915 -> MEI -> CSME-FW
> > 
> > 2. Media(Audio/Video) Pipeline
> > App Media SW Stack -> LibVA/iHD -> i915 -> GPU
> > 
> > 3. 3D Pipeline in Compositor
> > App Composition SW Stack -> OpenGL/MESA/MiniGBM -> i915 -> GPU/Display
> > 
> > 4. Display Pipeline in Compositor
> > App Composition SW Stack -> Ozone/MiniGBM -> i915 -> Display
> > 
> > Discussion Point -
> > Even after Pipeline #4 is context robustness compliant there is a 
> > corner case/race condition for corruption as following  - BUG1 App's 
> > Composition SW Stack -> Creates Protected Context and Protected 
> > Buffer(MiniGBM) App's Composition SW Stack -> Supplies Protected 
> > Buffer to LibVA/iHD -> i915 -> GPU -> Encrypted decoded output App's 
> > Composition SW Stack -> Gets back decode output -> Checks for context 
> > robustness -> Submits frame for flip -> i915 Display(by the time i915 
> > Display gets flip PAVP session is invalid despite being atomic since 
> > invalidation of PAVP is HW async event) -> Display HW -> Shows 
> > corruption
> 
> It'll always be racy unless the invalidation becomes a two stage process that first tells display to stop scanning out the thing and then waits for the display to finish scanning out the current frame.
> 
> > 
> > 
> > -----Original Message-----
> > From: Ville Syrjälä <[e-mail]>
> > Sent: Thursday, January 21, 2021 12:31 PM
> > To: Gupta, Anshuman <[e-mail]>
> > Cc: Huang, Sean Z <[e-mail]>; 
> > [e-mail]; Nikula, Jani <[e-mail]>; 
> > Gaurav, Kumar <[e-mail]>; Bommu, Krishnaiah 
> > <[e-mail]>; Vetter, Daniel <[e-mail]>
> > Subject: Re: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption 
> > support
> > 
> > On Tue, Jan 19, 2021 at 09:35:18AM +0000, Gupta, Anshuman wrote:
> > > Jani/Ville
> > > I had received an offline comment form Gaurav on this patch, See 
> > > below,
> > > > -----Original Message-----
> > > > From: Huang, Sean Z <[e-mail]>
> > > > Sent: Tuesday, January 19, 2021 1:13 PM
> > > > To: [e-mail]
> > > > Cc: Gaurav, Kumar <[e-mail]>; Gupta, Anshuman 
> > > > <[e-mail]>; Bommu, Krishnaiah 
> > > > <[e-mail]>; Huang, Sean Z 
> > > > <[e-mail]>
> > > > Subject: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption 
> > > > support
> > > > 
> > > > From: Anshuman Gupta <[e-mail]>
> > > > 
> > > > Add support to enable/disable PLANE_SURF Decryption Request bit.
> > > > It requires only to enable plane decryption support when following 
> > > > condition met.
> > > > 1. PXP session is enabled.
> > > > 2. Buffer object is protected.
> > > > 
> > > > v2:
> > > > - Rebased to libva_cp-drm-tip_tgl_cp tree.
> > > > - Used gen fb obj user_flags instead gem_object_metadata. 
> > > > [Krishna]
> > > > 
> > > > v3:
> > > > - intel_pxp_gem_object_status() API changes.
> > > > 
> > > > Cc: Bommu Krishnaiah <[e-mail]>
> > > > Cc: Huang Sean Z <[e-mail]>
> > > > Cc: Gaurav Kumar <[e-mail]>
> > > > Signed-off-by: Anshuman Gupta <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_sprite.c | 21 ++++++++++++++++++---
> > > >  drivers/gpu/drm/i915/i915_reg.h             |  1 +
> > > >  2 files changed, 19 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > index cf3589fd0ddb..39f8c922ce66 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > @@ -39,6 +39,8 @@
> > > >  #include <drm/drm_plane_helper.h>  #include <drm/drm_rect.h>
> > > > 
> > > > +#include "pxp/intel_pxp.h"
> > > > +
> > > >  #include "i915_drv.h"
> > > >  #include "i915_trace.h"
> > > >  #include "i915_vgpu.h"
> > > > @@ -768,6 +770,11 @@ icl_program_input_csc(struct intel_plane *plane,
> > > >  			  PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0);  }
> > > > 
> > > > +static bool intel_fb_obj_protected(const struct 
> > > > +drm_i915_gem_object
> > > > +*obj) {
> > > > +	return obj->user_flags & I915_BO_PROTECTED ? true : false; }
> > > > +
> > > >  static void
> > > >  skl_plane_async_flip(struct intel_plane *plane,
> > > >  		     const struct intel_crtc_state *crtc_state, @@ -804,6
> > > > +811,7 @@ skl_program_plane(struct intel_plane *plane,
> > > >  	u32 surf_addr = plane_state->color_plane[color_plane].offset;
> > > >  	u32 stride = skl_plane_stride(plane_state, color_plane);
> > > >  	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > > +	const struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > > >  	int aux_plane = intel_main_to_aux_plane(fb, color_plane);
> > > >  	int crtc_x = plane_state->uapi.dst.x1;
> > > >  	int crtc_y = plane_state->uapi.dst.y1; @@ -814,7 +822,7 @@ 
> > > > skl_program_plane(struct intel_plane *plane,
> > > >  	u8 alpha = plane_state->hw.alpha >> 8;
> > > >  	u32 plane_color_ctl = 0, aux_dist = 0;
> > > >  	unsigned long irqflags;
> > > > -	u32 keymsk, keymax;
> > > > +	u32 keymsk, keymax, plane_surf;
> > > >  	u32 plane_ctl = plane_state->ctl;
> > > > 
> > > >  	plane_ctl |= skl_plane_ctl_crtc(crtc_state); @@ -890,8 +898,15 
> > > > @@ skl_program_plane(struct intel_plane *plane,
> > > >  	 * the control register just before the surface register.
> > > >  	 */
> > > >  	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> > > > -	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> > > > -			  intel_plane_ggtt_offset(plane_state) + surf_addr);
> > > > +	plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
> > > > +
> > > > +	if (intel_pxp_gem_object_status(dev_priv) &&
> > > > +	    intel_fb_obj_protected(obj))
> > > > +		plane_surf |= PLANE_SURF_DECRYPTION_ENABLED;
> > > Here in case of if fb obj is protected but pxp session is not enabled i.e intel_pxp_gem_object_status() returns false, request to show the black frame buffer on display instead of corrupted data.
> > >                             plane_surf = 0xXXX; //Pointer to black 
> > > framebuffer But above approach would be a hack.
> > > @Jani and @Ville could you please guide with the general way of handling this as pxp session keys can be invalidated at any time.
> > 
> > Would need such a black buffer to be always pinned into the gtt, which is seems a bit wasteful. We could perhaps just force the plane to output black eg. by using the plane gamma. I think we should always have the per-plane gamma available on skl+ universal planes. Cursor may be a different story.
> > 
> > --
> > Ville Syrjälä
> > Intel
> 
> --
> Ville Syrjälä
> Intel

Gaurav, Kumar Jan. 22, 2021, 5:25 p.m. UTC | #7

a) How are you scheduling flips for ChromeOS HWDRM overlays(not primary planes)?

b) For this you need to understand race. The race is between - Display HW detecting that there is HDCP display config change and deciding to destroy crypto context vs. Display HW flipping HWDRM protected surface.
     If Display HW decided to destroy crypto context then display HW does flip user will corruption but the check before flip makes sure that order is reversed i.e. no invalid crypto context flips are scheduled in Display HW.

-----Original Message-----
From: Ville Syrjälä <[e-mail]> 
Sent: Friday, January 22, 2021 3:59 AM
To: Gaurav, Kumar <[e-mail]>
Cc: Gupta, Anshuman <[e-mail]>; Huang, Sean Z <[e-mail]>; [e-mail]; Nikula, Jani <[e-mail]>; Bommu, Krishnaiah <[e-mail]>; Vetter, Daniel <[e-mail]>
Subject: Re: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support

On Thu, Jan 21, 2021 at 09:34:53PM +0000, Gaurav, Kumar wrote:
> So the idea is to perform HWDRM session check in flip IRQL and i915 PXP will guarantee that IRQL is not blocked. 
> We have done similar check in Windows flip IRQL.  
> 

What's an flip IRQL? Some sort of flip related irq handler?

a) we don't use the flip done interrupt (except for async flips), if
   that's what you're referring to
b) how would doing anything there help against the race?

> -----Original Message-----
> From: Ville Syrjälä <[e-mail]>
> Sent: Thursday, January 21, 2021 1:00 PM
> To: Gaurav, Kumar <[e-mail]>
> Cc: Gupta, Anshuman <[e-mail]>; Huang, Sean Z 
> <[e-mail]>; [e-mail]; Nikula, 
> Jani <[e-mail]>; Bommu, Krishnaiah 
> <[e-mail]>; Vetter, Daniel <[e-mail]>
> Subject: Re: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption 
> support
> 
> On Thu, Jan 21, 2021 at 08:50:18PM +0000, Gaurav, Kumar wrote:
> > Thanks Anshuman for adding me for review.
> > 
> > Actually, using plane Gamma is good idea to show black frame. Another option could be alpha value since we know for ChromeOS protected buffer will always be flipped on overlays.
> > 
> > Below explanation captures need for black frame in i915 Display for 
> > HWDRM protected surfaces - Problem Statement - There is race 
> > condition between Ring3 and Ring0 where encrypted frame could be flipped by i915 Display despite Ring3 checking if HWDRM session keys are valid for encrypted frame.
> > 
> > Google Bug -
> > BUG1 -[Intel] i915 framebuffer tracking (protected surfaces that 
> > can't be decrypted are being rendered as encrypted) -b/155511255
> > 
> > Background -
> > There are 4 high level pipelines working together in HWDRM playback.
> > 1. CDM Pipeline -
> > App CDM SW Stack -> LibVA/iHD -> i915 -> MEI -> CSME-FW
> > 
> > 2. Media(Audio/Video) Pipeline
> > App Media SW Stack -> LibVA/iHD -> i915 -> GPU
> > 
> > 3. 3D Pipeline in Compositor
> > App Composition SW Stack -> OpenGL/MESA/MiniGBM -> i915 -> 
> > GPU/Display
> > 
> > 4. Display Pipeline in Compositor
> > App Composition SW Stack -> Ozone/MiniGBM -> i915 -> Display
> > 
> > Discussion Point -
> > Even after Pipeline #4 is context robustness compliant there is a 
> > corner case/race condition for corruption as following  - BUG1 App's 
> > Composition SW Stack -> Creates Protected Context and Protected
> > Buffer(MiniGBM) App's Composition SW Stack -> Supplies Protected 
> > Buffer to LibVA/iHD -> i915 -> GPU -> Encrypted decoded output App's 
> > Composition SW Stack -> Gets back decode output -> Checks for 
> > context robustness -> Submits frame for flip -> i915 Display(by the 
> > time i915 Display gets flip PAVP session is invalid despite being 
> > atomic since invalidation of PAVP is HW async event) -> Display HW 
> > -> Shows corruption
> 
> It'll always be racy unless the invalidation becomes a two stage process that first tells display to stop scanning out the thing and then waits for the display to finish scanning out the current frame.
> 
> > 
> > 
> > -----Original Message-----
> > From: Ville Syrjälä <[e-mail]>
> > Sent: Thursday, January 21, 2021 12:31 PM
> > To: Gupta, Anshuman <[e-mail]>
> > Cc: Huang, Sean Z <[e-mail]>; 
> > [e-mail]; Nikula, Jani 
> > <[e-mail]>; Gaurav, Kumar <[e-mail]>; 
> > Bommu, Krishnaiah <[e-mail]>; Vetter, Daniel 
> > <[e-mail]>
> > Subject: Re: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption 
> > support
> > 
> > On Tue, Jan 19, 2021 at 09:35:18AM +0000, Gupta, Anshuman wrote:
> > > Jani/Ville
> > > I had received an offline comment form Gaurav on this patch, See 
> > > below,
> > > > -----Original Message-----
> > > > From: Huang, Sean Z <[e-mail]>
> > > > Sent: Tuesday, January 19, 2021 1:13 PM
> > > > To: [e-mail]
> > > > Cc: Gaurav, Kumar <[e-mail]>; Gupta, Anshuman 
> > > > <[e-mail]>; Bommu, Krishnaiah 
> > > > <[e-mail]>; Huang, Sean Z 
> > > > <[e-mail]>
> > > > Subject: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption 
> > > > support
> > > > 
> > > > From: Anshuman Gupta <[e-mail]>
> > > > 
> > > > Add support to enable/disable PLANE_SURF Decryption Request bit.
> > > > It requires only to enable plane decryption support when 
> > > > following condition met.
> > > > 1. PXP session is enabled.
> > > > 2. Buffer object is protected.
> > > > 
> > > > v2:
> > > > - Rebased to libva_cp-drm-tip_tgl_cp tree.
> > > > - Used gen fb obj user_flags instead gem_object_metadata. 
> > > > [Krishna]
> > > > 
> > > > v3:
> > > > - intel_pxp_gem_object_status() API changes.
> > > > 
> > > > Cc: Bommu Krishnaiah <[e-mail]>
> > > > Cc: Huang Sean Z <[e-mail]>
> > > > Cc: Gaurav Kumar <[e-mail]>
> > > > Signed-off-by: Anshuman Gupta <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_sprite.c | 21 ++++++++++++++++++---
> > > >  drivers/gpu/drm/i915/i915_reg.h             |  1 +
> > > >  2 files changed, 19 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > index cf3589fd0ddb..39f8c922ce66 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > @@ -39,6 +39,8 @@
> > > >  #include <drm/drm_plane_helper.h>  #include <drm/drm_rect.h>
> > > > 
> > > > +#include "pxp/intel_pxp.h"
> > > > +
> > > >  #include "i915_drv.h"
> > > >  #include "i915_trace.h"
> > > >  #include "i915_vgpu.h"
> > > > @@ -768,6 +770,11 @@ icl_program_input_csc(struct intel_plane *plane,
> > > >  			  PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0);  }
> > > > 
> > > > +static bool intel_fb_obj_protected(const struct 
> > > > +drm_i915_gem_object
> > > > +*obj) {
> > > > +	return obj->user_flags & I915_BO_PROTECTED ? true : false; }
> > > > +
> > > >  static void
> > > >  skl_plane_async_flip(struct intel_plane *plane,
> > > >  		     const struct intel_crtc_state *crtc_state, @@ -804,6
> > > > +811,7 @@ skl_program_plane(struct intel_plane *plane,
> > > >  	u32 surf_addr = plane_state->color_plane[color_plane].offset;
> > > >  	u32 stride = skl_plane_stride(plane_state, color_plane);
> > > >  	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > > +	const struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > > >  	int aux_plane = intel_main_to_aux_plane(fb, color_plane);
> > > >  	int crtc_x = plane_state->uapi.dst.x1;
> > > >  	int crtc_y = plane_state->uapi.dst.y1; @@ -814,7 +822,7 @@ 
> > > > skl_program_plane(struct intel_plane *plane,
> > > >  	u8 alpha = plane_state->hw.alpha >> 8;
> > > >  	u32 plane_color_ctl = 0, aux_dist = 0;
> > > >  	unsigned long irqflags;
> > > > -	u32 keymsk, keymax;
> > > > +	u32 keymsk, keymax, plane_surf;
> > > >  	u32 plane_ctl = plane_state->ctl;
> > > > 
> > > >  	plane_ctl |= skl_plane_ctl_crtc(crtc_state); @@ -890,8 +898,15 
> > > > @@ skl_program_plane(struct intel_plane *plane,
> > > >  	 * the control register just before the surface register.
> > > >  	 */
> > > >  	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> > > > -	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> > > > -			  intel_plane_ggtt_offset(plane_state) + surf_addr);
> > > > +	plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
> > > > +
> > > > +	if (intel_pxp_gem_object_status(dev_priv) &&
> > > > +	    intel_fb_obj_protected(obj))
> > > > +		plane_surf |= PLANE_SURF_DECRYPTION_ENABLED;
> > > Here in case of if fb obj is protected but pxp session is not enabled i.e intel_pxp_gem_object_status() returns false, request to show the black frame buffer on display instead of corrupted data.
> > >                             plane_surf = 0xXXX; //Pointer to black 
> > > framebuffer But above approach would be a hack.
> > > @Jani and @Ville could you please guide with the general way of handling this as pxp session keys can be invalidated at any time.
> > 
> > Would need such a black buffer to be always pinned into the gtt, which is seems a bit wasteful. We could perhaps just force the plane to output black eg. by using the plane gamma. I think we should always have the per-plane gamma available on skl+ universal planes. Cursor may be a different story.
> > 
> > --
> > Ville Syrjälä
> > Intel
> 
> --
> Ville Syrjälä
> Intel

--
Ville Syrjälä
Intel

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index cf3589fd0ddb..39f8c922ce66 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -39,6 +39,8 @@ 
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_rect.h>
 
+#include "pxp/intel_pxp.h"
+
 #include "i915_drv.h"
 #include "i915_trace.h"
 #include "i915_vgpu.h"
@@ -768,6 +770,11 @@  icl_program_input_csc(struct intel_plane *plane,
 			  PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0);
 }
 
+static bool intel_fb_obj_protected(const struct drm_i915_gem_object *obj)
+{
+	return obj->user_flags & I915_BO_PROTECTED ? true : false;
+}
+
 static void
 skl_plane_async_flip(struct intel_plane *plane,
 		     const struct intel_crtc_state *crtc_state,
@@ -804,6 +811,7 @@  skl_program_plane(struct intel_plane *plane,
 	u32 surf_addr = plane_state->color_plane[color_plane].offset;
 	u32 stride = skl_plane_stride(plane_state, color_plane);
 	const struct drm_framebuffer *fb = plane_state->hw.fb;
+	const struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int aux_plane = intel_main_to_aux_plane(fb, color_plane);
 	int crtc_x = plane_state->uapi.dst.x1;
 	int crtc_y = plane_state->uapi.dst.y1;
@@ -814,7 +822,7 @@  skl_program_plane(struct intel_plane *plane,
 	u8 alpha = plane_state->hw.alpha >> 8;
 	u32 plane_color_ctl = 0, aux_dist = 0;
 	unsigned long irqflags;
-	u32 keymsk, keymax;
+	u32 keymsk, keymax, plane_surf;
 	u32 plane_ctl = plane_state->ctl;
 
 	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
@@ -890,8 +898,15 @@  skl_program_plane(struct intel_plane *plane,
 	 * the control register just before the surface register.
 	 */
 	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
-	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
-			  intel_plane_ggtt_offset(plane_state) + surf_addr);
+	plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
+
+	if (intel_pxp_gem_object_status(dev_priv) &&
+	    intel_fb_obj_protected(obj))
+		plane_surf |= PLANE_SURF_DECRYPTION_ENABLED;
+	else
+		plane_surf &= ~PLANE_SURF_DECRYPTION_ENABLED;
+
+	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), plane_surf);
 
 	if (plane_state->scaler_id >= 0)
 		skl_program_scaler(plane, crtc_state, plane_state);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 97bcecada87f..7a4817054bc8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7208,6 +7208,7 @@  enum {
 #define _PLANE_SURF_3(pipe)	_PIPE(pipe, _PLANE_SURF_3_A, _PLANE_SURF_3_B)
 #define PLANE_SURF(pipe, plane)	\
 	_MMIO_PLANE(plane, _PLANE_SURF_1(pipe), _PLANE_SURF_2(pipe))
+#define   PLANE_SURF_DECRYPTION_ENABLED		REG_BIT(2)
 
 #define _PLANE_OFFSET_1_B			0x711a4
 #define _PLANE_OFFSET_2_B			0x712a4