Re: [Intel-gfx] [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support

["Gaurav, Kumar" <kumar.gaurav@intel.com>]

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ä <ville.syrjala@linux.intel.com> 
Sent: Thursday, January 21, 2021 12:31 PM
To: Gupta, Anshuman <anshuman.gupta@intel.com>
Cc: Huang, Sean Z <sean.z.huang@intel.com>; Intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com>; Bommu, Krishnaiah <krishnaiah.bommu@intel.com>; Vetter, Daniel <daniel.vetter@intel.com>
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 <sean.z.huang@intel.com>
> > Sent: Tuesday, January 19, 2021 1:13 PM
> > To: Intel-gfx@lists.freedesktop.org
> > Cc: Gaurav, Kumar <kumar.gaurav@intel.com>; Gupta, Anshuman 
> > <anshuman.gupta@intel.com>; Bommu, Krishnaiah 
> > <krishnaiah.bommu@intel.com>; Huang, Sean Z <sean.z.huang@intel.com>
> > Subject: [RFC-v23 13/13] drm/i915/pxp: Add plane decryption support
> > 
> > From: Anshuman Gupta <anshuman.gupta@intel.com>
> > 
> > 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 <krishnaiah.bommu@intel.com>
> > Cc: Huang Sean Z <sean.z.huang@intel.com>
> > Cc: Gaurav Kumar <kumar.gaurav@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx