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