| Message ID | 20210427104311.2664-1-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote: > 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: > - Used gen fb obj user_flags instead gem_object_metadata. [Krishna] > > v3: > - intel_pxp_gem_object_status() API changes. > > v4: use intel_pxp_is_active (Daniele) > > v5: rebase and use the new protected object status checker (Daniele) > > v6: used plane state for plane_decryption to handle async flip > as suggested by Ville. > > Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> > Cc: Huang Sean Z <sean.z.huang@intel.com> > Cc: Gaurav Kumar <kumar.gaurav@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > .../gpu/drm/i915/display/intel_atomic_plane.c | 3 ++ > drivers/gpu/drm/i915/display/intel_display.c | 5 +++ > .../drm/i915/display/intel_display_types.h | 3 ++ > .../drm/i915/display/skl_universal_plane.c | 32 +++++++++++++++++-- > .../drm/i915/display/skl_universal_plane.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 1 + > 6 files changed, 42 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index 7bfb26ca0bd0..7057077a2b71 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct intel_atomic_state *state, > intel_atomic_get_old_crtc_state(state, crtc); > struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > + const struct drm_framebuffer *fb = new_plane_state->hw.fb; > > if (new_crtc_state && new_crtc_state->bigjoiner_slave) { > struct intel_plane *master_plane = > @@ -409,6 +410,8 @@ int intel_plane_atomic_check(struct intel_atomic_state *state, > intel_plane_copy_uapi_to_hw_state(new_plane_state, > new_master_plane_state, > crtc); > + new_plane_state->plane_decryption = > + i915_gem_object_has_valid_protection(intel_fb_obj(fb)); > > new_plane_state->uapi.visible = false; > if (!new_crtc_state) > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index a10e26380ef3..55ab2d0b92d8 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct intel_atomic_state *state) > drm_dbg_kms(&i915->drm, "Color range cannot be changed in async flip\n"); > return -EINVAL; > } > + > + /* plane decryption is allow to change only in synchronous flips */ > + if (old_plane_state->plane_decryption != new_plane_state->plane_decryption) > + return -EINVAL; > } > > return 0; > @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct drm_i915_private *dev_priv) > > crtc = intel_get_crtc_for_pipe(dev_priv, pipe); > crtc_state = to_intel_crtc_state(crtc->base.state); > + intel_plane_read_hw_decryption(plane_state); We don't have real plane state readout anyway, so seems pointless. > > intel_set_plane_visible(crtc_state, plane_state, visible); > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index e2e707c4dff5..76b3bb64a36a 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -617,6 +617,9 @@ struct intel_plane_state { > > struct intel_fb_view view; > > + /* Plane pxp decryption state */ > + bool plane_decryption; > + It's all about the plane, so the plane_ prefix is entirely redundant. Could just call it "decrypt" I guess. > /* plane control register */ > u32 ctl; > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c > index 75d3ca3dbb37..74489217e580 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > @@ -17,6 +17,7 @@ > #include "intel_sprite.h" > #include "skl_scaler.h" > #include "skl_universal_plane.h" > +#include "pxp/intel_pxp.h" > > static const u32 skl_plane_formats[] = { > DRM_FORMAT_C8, > @@ -956,7 +957,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); > @@ -1037,8 +1038,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_is_active(&dev_priv->gt.pxp) && That should all be part of the state computation. And you're missing this in the .async_flip path totally. > + plane_state->plane_decryption) > + plane_surf |= PLANE_SURF_DECRYPTION_ENABLED; > + else > + plane_surf &= ~PLANE_SURF_DECRYPTION_ENABLED; This &=~ stuff is still 100% pointless. So just if (plane_state->decrypt) surf_addr |= ...; should do it. And I'd do it around the top of the function where we calculate a bunch of the other register values. That keeps most of the function clean of random ifs. Just after the aux_dist calculation would seem like the correct spot to me. > + > + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), plane_surf); > > if (plane_state->scaler_id >= 0) > skl_program_plane_scaler(plane, crtc_state, plane_state); > @@ -2242,3 +2250,21 @@ skl_get_initial_plane_config(struct intel_crtc *crtc, > kfree(intel_fb); > } > > +void intel_plane_read_hw_decryption(struct intel_plane_state *plane_state) > +{ > + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); > + struct drm_i915_private *i915 = to_i915(plane->base.dev); > + enum intel_display_power_domain power_domain; > + enum plane_id plane_id = plane->id; > + intel_wakeref_t wakeref; > + > + power_domain = POWER_DOMAIN_PIPE(plane->pipe); > + wakeref = intel_display_power_get_if_enabled(i915, power_domain); > + if (drm_WARN_ON(&i915->drm, !wakeref)) > + return; > + > + plane_state->plane_decryption = > + intel_de_read(i915, PLANE_SURF(plane->pipe, plane_id)) & PLANE_SURF_DECRYPTION_ENABLED; > + > + intel_display_power_put(i915, power_domain, wakeref); > +} > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.h b/drivers/gpu/drm/i915/display/skl_universal_plane.h > index 351040b64dc7..2b1d673f8bf5 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.h > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.h > @@ -31,5 +31,6 @@ int skl_calc_main_surface_offset(const struct intel_plane_state *plane_state, > bool icl_is_nv12_y_plane(struct drm_i915_private *dev_priv, > enum plane_id plane_id); > bool icl_is_hdr_plane(struct drm_i915_private *dev_priv, enum plane_id plane_id); > +void intel_plane_read_hw_decryption(struct intel_plane_state *plane_state); > > #endif > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 740e97663fec..fbaf9199001d 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7240,6 +7240,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) Could be just PLANE_SURF_DECRYPT or something along those lines. > > #define _PLANE_OFFSET_1_B 0x711a4 > #define _PLANE_OFFSET_2_B 0x712a4 > -- > 2.26.2
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Wednesday, April 28, 2021 12:25 AM > To: Gupta, Anshuman <anshuman.gupta@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; > Bommu, Krishnaiah <krishnaiah.bommu@intel.com>; Huang Sean Z > <sean.z.huang@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com>; > Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> > Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support Thanks Ville for review, Below are some doubts with respect to pxp state. > > On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote: > > 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: > > - Used gen fb obj user_flags instead gem_object_metadata. [Krishna] > > > > v3: > > - intel_pxp_gem_object_status() API changes. > > > > v4: use intel_pxp_is_active (Daniele) > > > > v5: rebase and use the new protected object status checker (Daniele) > > > > v6: used plane state for plane_decryption to handle async flip > > as suggested by Ville. > > > > Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> > > Cc: Huang Sean Z <sean.z.huang@intel.com> > > Cc: Gaurav Kumar <kumar.gaurav@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > Signed-off-by: Daniele Ceraolo Spurio > > <daniele.ceraolospurio@intel.com> > > --- > > .../gpu/drm/i915/display/intel_atomic_plane.c | 3 ++ > > drivers/gpu/drm/i915/display/intel_display.c | 5 +++ > > .../drm/i915/display/intel_display_types.h | 3 ++ > > .../drm/i915/display/skl_universal_plane.c | 32 +++++++++++++++++-- > > .../drm/i915/display/skl_universal_plane.h | 1 + > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 6 files changed, 42 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > index 7bfb26ca0bd0..7057077a2b71 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct > intel_atomic_state *state, > > intel_atomic_get_old_crtc_state(state, crtc); > > struct intel_crtc_state *new_crtc_state = > > intel_atomic_get_new_crtc_state(state, crtc); > > + const struct drm_framebuffer *fb = new_plane_state->hw.fb; > > > > if (new_crtc_state && new_crtc_state->bigjoiner_slave) { > > struct intel_plane *master_plane = > > @@ -409,6 +410,8 @@ int intel_plane_atomic_check(struct > intel_atomic_state *state, > > intel_plane_copy_uapi_to_hw_state(new_plane_state, > > new_master_plane_state, > > crtc); > > + new_plane_state->plane_decryption = > > + i915_gem_object_has_valid_protection(intel_fb_obj(fb)); > > > > new_plane_state->uapi.visible = false; > > if (!new_crtc_state) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index a10e26380ef3..55ab2d0b92d8 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct > intel_atomic_state *state) > > drm_dbg_kms(&i915->drm, "Color range cannot be > changed in async flip\n"); > > return -EINVAL; > > } > > + > > + /* plane decryption is allow to change only in synchronous flips > */ > > + if (old_plane_state->plane_decryption != new_plane_state- > >plane_decryption) > > + return -EINVAL; > > } > > > > return 0; > > @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct > > drm_i915_private *dev_priv) > > > > crtc = intel_get_crtc_for_pipe(dev_priv, pipe); > > crtc_state = to_intel_crtc_state(crtc->base.state); > > + intel_plane_read_hw_decryption(plane_state); > > We don't have real plane state readout anyway, so seems pointless. > > > > > intel_set_plane_visible(crtc_state, plane_state, visible); > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index e2e707c4dff5..76b3bb64a36a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -617,6 +617,9 @@ struct intel_plane_state { > > > > struct intel_fb_view view; > > > > + /* Plane pxp decryption state */ > > + bool plane_decryption; > > + > > It's all about the plane, so the plane_ prefix is entirely redundant. > Could just call it "decrypt" I guess. > > > /* plane control register */ > > u32 ctl; > > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > index 75d3ca3dbb37..74489217e580 100644 > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > @@ -17,6 +17,7 @@ > > #include "intel_sprite.h" > > #include "skl_scaler.h" > > #include "skl_universal_plane.h" > > +#include "pxp/intel_pxp.h" > > > > static const u32 skl_plane_formats[] = { > > DRM_FORMAT_C8, > > @@ -956,7 +957,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); @@ -1037,8 +1038,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_is_active(&dev_priv->gt.pxp) && > > That should all be part of the state computation. And you're missing this in the > .async_flip path totally. Hi Ville / Rodrigo / Daniele, Is it possible to check intel_pxp_is_active() in plane atomic check function to compute plane decryption state? with my best knowledge session can be invalid at any time, Let's say in plane atomic check function pxp session was enabled but while in atomic commit pxp session can be still disabled. Please provide your inputs. Thanks, Anshuman Gupta. > > > + plane_state->plane_decryption) > > + plane_surf |= PLANE_SURF_DECRYPTION_ENABLED; > > + else > > + plane_surf &= ~PLANE_SURF_DECRYPTION_ENABLED; > > This &=~ stuff is still 100% pointless. > > So just > if (plane_state->decrypt) > surf_addr |= ...; > > should do it. And I'd do it around the top of the function where we calculate a > bunch of the other register values. That keeps most of the function clean of > random ifs. Just after the aux_dist calculation would seem like the correct spot > to me. > > > + > > + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), plane_surf); > > > > if (plane_state->scaler_id >= 0) > > skl_program_plane_scaler(plane, crtc_state, plane_state); @@ > > -2242,3 +2250,21 @@ skl_get_initial_plane_config(struct intel_crtc *crtc, > > kfree(intel_fb); > > } > > > > +void intel_plane_read_hw_decryption(struct intel_plane_state > > +*plane_state) { > > + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); > > + struct drm_i915_private *i915 = to_i915(plane->base.dev); > > + enum intel_display_power_domain power_domain; > > + enum plane_id plane_id = plane->id; > > + intel_wakeref_t wakeref; > > + > > + power_domain = POWER_DOMAIN_PIPE(plane->pipe); > > + wakeref = intel_display_power_get_if_enabled(i915, power_domain); > > + if (drm_WARN_ON(&i915->drm, !wakeref)) > > + return; > > + > > + plane_state->plane_decryption = > > + intel_de_read(i915, PLANE_SURF(plane->pipe, plane_id)) & > > +PLANE_SURF_DECRYPTION_ENABLED; > > + > > + intel_display_power_put(i915, power_domain, wakeref); } > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.h > > b/drivers/gpu/drm/i915/display/skl_universal_plane.h > > index 351040b64dc7..2b1d673f8bf5 100644 > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.h > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.h > > @@ -31,5 +31,6 @@ int skl_calc_main_surface_offset(const struct > > intel_plane_state *plane_state, bool icl_is_nv12_y_plane(struct > drm_i915_private *dev_priv, > > enum plane_id plane_id); > > bool icl_is_hdr_plane(struct drm_i915_private *dev_priv, enum > > plane_id plane_id); > > +void intel_plane_read_hw_decryption(struct intel_plane_state > > +*plane_state); > > > > #endif > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h index 740e97663fec..fbaf9199001d > > 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7240,6 +7240,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) > > Could be just PLANE_SURF_DECRYPT or something along those lines. > > > > > #define _PLANE_OFFSET_1_B 0x711a4 > > #define _PLANE_OFFSET_2_B 0x712a4 > > -- > > 2.26.2 > > -- > Ville Syrjälä > Intel
On Wed, Apr 28, 2021 at 11:25:23AM +0000, Gupta, Anshuman wrote: > > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: Wednesday, April 28, 2021 12:25 AM > > To: Gupta, Anshuman <anshuman.gupta@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; > > Bommu, Krishnaiah <krishnaiah.bommu@intel.com>; Huang Sean Z > > <sean.z.huang@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com>; > > Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> > > Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support > Thanks Ville for review, Below are some doubts with respect to pxp state. > > > > On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote: > > > 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: > > > - Used gen fb obj user_flags instead gem_object_metadata. [Krishna] > > > > > > v3: > > > - intel_pxp_gem_object_status() API changes. > > > > > > v4: use intel_pxp_is_active (Daniele) > > > > > > v5: rebase and use the new protected object status checker (Daniele) > > > > > > v6: used plane state for plane_decryption to handle async flip > > > as suggested by Ville. > > > > > > Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> > > > Cc: Huang Sean Z <sean.z.huang@intel.com> > > > Cc: Gaurav Kumar <kumar.gaurav@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > > Signed-off-by: Daniele Ceraolo Spurio > > > <daniele.ceraolospurio@intel.com> > > > --- > > > .../gpu/drm/i915/display/intel_atomic_plane.c | 3 ++ > > > drivers/gpu/drm/i915/display/intel_display.c | 5 +++ > > > .../drm/i915/display/intel_display_types.h | 3 ++ > > > .../drm/i915/display/skl_universal_plane.c | 32 +++++++++++++++++-- > > > .../drm/i915/display/skl_universal_plane.h | 1 + > > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > > 6 files changed, 42 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > index 7bfb26ca0bd0..7057077a2b71 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct > > intel_atomic_state *state, > > > intel_atomic_get_old_crtc_state(state, crtc); > > > struct intel_crtc_state *new_crtc_state = > > > intel_atomic_get_new_crtc_state(state, crtc); > > > + const struct drm_framebuffer *fb = new_plane_state->hw.fb; > > > > > > if (new_crtc_state && new_crtc_state->bigjoiner_slave) { > > > struct intel_plane *master_plane = > > > @@ -409,6 +410,8 @@ int intel_plane_atomic_check(struct > > intel_atomic_state *state, > > > intel_plane_copy_uapi_to_hw_state(new_plane_state, > > > new_master_plane_state, > > > crtc); > > > + new_plane_state->plane_decryption = > > > + i915_gem_object_has_valid_protection(intel_fb_obj(fb)); > > > > > > new_plane_state->uapi.visible = false; > > > if (!new_crtc_state) > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index a10e26380ef3..55ab2d0b92d8 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct > > intel_atomic_state *state) > > > drm_dbg_kms(&i915->drm, "Color range cannot be > > changed in async flip\n"); > > > return -EINVAL; > > > } > > > + > > > + /* plane decryption is allow to change only in synchronous flips > > */ > > > + if (old_plane_state->plane_decryption != new_plane_state- > > >plane_decryption) > > > + return -EINVAL; > > > } > > > > > > return 0; > > > @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct > > > drm_i915_private *dev_priv) > > > > > > crtc = intel_get_crtc_for_pipe(dev_priv, pipe); > > > crtc_state = to_intel_crtc_state(crtc->base.state); > > > + intel_plane_read_hw_decryption(plane_state); > > > > We don't have real plane state readout anyway, so seems pointless. > > > > > > > > intel_set_plane_visible(crtc_state, plane_state, visible); > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index e2e707c4dff5..76b3bb64a36a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -617,6 +617,9 @@ struct intel_plane_state { > > > > > > struct intel_fb_view view; > > > > > > + /* Plane pxp decryption state */ > > > + bool plane_decryption; > > > + > > > > It's all about the plane, so the plane_ prefix is entirely redundant. > > Could just call it "decrypt" I guess. > > > > > /* plane control register */ > > > u32 ctl; > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > > index 75d3ca3dbb37..74489217e580 100644 > > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > > @@ -17,6 +17,7 @@ > > > #include "intel_sprite.h" > > > #include "skl_scaler.h" > > > #include "skl_universal_plane.h" > > > +#include "pxp/intel_pxp.h" > > > > > > static const u32 skl_plane_formats[] = { > > > DRM_FORMAT_C8, > > > @@ -956,7 +957,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); @@ -1037,8 +1038,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_is_active(&dev_priv->gt.pxp) && > > > > That should all be part of the state computation. And you're missing this in the > > .async_flip path totally. > Hi Ville / Rodrigo / Daniele, > Is it possible to check intel_pxp_is_active() in plane atomic check function to compute plane decryption state? > with my best knowledge session can be invalid at any time, Let's say in plane atomic check function pxp session was enabled > but while in atomic commit pxp session can be still disabled. I can be invalidated any time after the commit anyway. What happens in that case?
On 4/28/2021 5:03 AM, Ville Syrjälä wrote: > On Wed, Apr 28, 2021 at 11:25:23AM +0000, Gupta, Anshuman wrote: >> >>> -----Original Message----- >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Sent: Wednesday, April 28, 2021 12:25 AM >>> To: Gupta, Anshuman <anshuman.gupta@intel.com> >>> Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; >>> Bommu, Krishnaiah <krishnaiah.bommu@intel.com>; Huang Sean Z >>> <sean.z.huang@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com>; >>> Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> >>> Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support >> Thanks Ville for review, Below are some doubts with respect to pxp state. >>> On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote: >>>> 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: >>>> - Used gen fb obj user_flags instead gem_object_metadata. [Krishna] >>>> >>>> v3: >>>> - intel_pxp_gem_object_status() API changes. >>>> >>>> v4: use intel_pxp_is_active (Daniele) >>>> >>>> v5: rebase and use the new protected object status checker (Daniele) >>>> >>>> v6: used plane state for plane_decryption to handle async flip >>>> as suggested by Ville. >>>> >>>> Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> >>>> Cc: Huang Sean Z <sean.z.huang@intel.com> >>>> Cc: Gaurav Kumar <kumar.gaurav@intel.com> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> >>>> Signed-off-by: Daniele Ceraolo Spurio >>>> <daniele.ceraolospurio@intel.com> >>>> --- >>>> .../gpu/drm/i915/display/intel_atomic_plane.c | 3 ++ >>>> drivers/gpu/drm/i915/display/intel_display.c | 5 +++ >>>> .../drm/i915/display/intel_display_types.h | 3 ++ >>>> .../drm/i915/display/skl_universal_plane.c | 32 +++++++++++++++++-- >>>> .../drm/i915/display/skl_universal_plane.h | 1 + >>>> drivers/gpu/drm/i915/i915_reg.h | 1 + >>>> 6 files changed, 42 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>> index 7bfb26ca0bd0..7057077a2b71 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>> @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct >>> intel_atomic_state *state, >>>> intel_atomic_get_old_crtc_state(state, crtc); >>>> struct intel_crtc_state *new_crtc_state = >>>> intel_atomic_get_new_crtc_state(state, crtc); >>>> + const struct drm_framebuffer *fb = new_plane_state->hw.fb; >>>> >>>> if (new_crtc_state && new_crtc_state->bigjoiner_slave) { >>>> struct intel_plane *master_plane = >>>> @@ -409,6 +410,8 @@ int intel_plane_atomic_check(struct >>> intel_atomic_state *state, >>>> intel_plane_copy_uapi_to_hw_state(new_plane_state, >>>> new_master_plane_state, >>>> crtc); >>>> + new_plane_state->plane_decryption = >>>> + i915_gem_object_has_valid_protection(intel_fb_obj(fb)); >>>> >>>> new_plane_state->uapi.visible = false; >>>> if (!new_crtc_state) >>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c >>>> b/drivers/gpu/drm/i915/display/intel_display.c >>>> index a10e26380ef3..55ab2d0b92d8 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c >>>> @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct >>> intel_atomic_state *state) >>>> drm_dbg_kms(&i915->drm, "Color range cannot be >>> changed in async flip\n"); >>>> return -EINVAL; >>>> } >>>> + >>>> + /* plane decryption is allow to change only in synchronous flips >>> */ >>>> + if (old_plane_state->plane_decryption != new_plane_state- >>>> plane_decryption) >>>> + return -EINVAL; >>>> } >>>> >>>> return 0; >>>> @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct >>>> drm_i915_private *dev_priv) >>>> >>>> crtc = intel_get_crtc_for_pipe(dev_priv, pipe); >>>> crtc_state = to_intel_crtc_state(crtc->base.state); >>>> + intel_plane_read_hw_decryption(plane_state); >>> We don't have real plane state readout anyway, so seems pointless. >>> >>>> intel_set_plane_visible(crtc_state, plane_state, visible); >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h >>>> b/drivers/gpu/drm/i915/display/intel_display_types.h >>>> index e2e707c4dff5..76b3bb64a36a 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >>>> @@ -617,6 +617,9 @@ struct intel_plane_state { >>>> >>>> struct intel_fb_view view; >>>> >>>> + /* Plane pxp decryption state */ >>>> + bool plane_decryption; >>>> + >>> It's all about the plane, so the plane_ prefix is entirely redundant. >>> Could just call it "decrypt" I guess. >>> >>>> /* plane control register */ >>>> u32 ctl; >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c >>>> b/drivers/gpu/drm/i915/display/skl_universal_plane.c >>>> index 75d3ca3dbb37..74489217e580 100644 >>>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c >>>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c >>>> @@ -17,6 +17,7 @@ >>>> #include "intel_sprite.h" >>>> #include "skl_scaler.h" >>>> #include "skl_universal_plane.h" >>>> +#include "pxp/intel_pxp.h" >>>> >>>> static const u32 skl_plane_formats[] = { >>>> DRM_FORMAT_C8, >>>> @@ -956,7 +957,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); @@ -1037,8 +1038,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_is_active(&dev_priv->gt.pxp) && >>> That should all be part of the state computation. And you're missing this in the >>> .async_flip path totally. >> Hi Ville / Rodrigo / Daniele, >> Is it possible to check intel_pxp_is_active() in plane atomic check function to compute plane decryption state? Yes, it should be possible to call that function from anywhere. >> with my best knowledge session can be invalid at any time, Let's say in plane atomic check function pxp session was enabled >> but while in atomic commit pxp session can be still disabled. > I can be invalidated any time after the commit anyway. What happens in > that case? If we flip a PXP object after the relevant key has been invalidated we just get garbage on the screen. Note that it is understood that this is a race we can't completely close given that the session invalidation can hit us at any time, the aim here is just to make the window as small as we can and replace invalid objects with a black frame where possible. Daniele
On Wed, Apr 28, 2021 at 10:32:46AM -0700, Daniele Ceraolo Spurio wrote: > > > On 4/28/2021 5:03 AM, Ville Syrjälä wrote: > > On Wed, Apr 28, 2021 at 11:25:23AM +0000, Gupta, Anshuman wrote: > >> > >>> -----Original Message----- > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> Sent: Wednesday, April 28, 2021 12:25 AM > >>> To: Gupta, Anshuman <anshuman.gupta@intel.com> > >>> Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; > >>> Bommu, Krishnaiah <krishnaiah.bommu@intel.com>; Huang Sean Z > >>> <sean.z.huang@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com>; > >>> Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> > >>> Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support > >> Thanks Ville for review, Below are some doubts with respect to pxp state. > >>> On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote: > >>>> 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: > >>>> - Used gen fb obj user_flags instead gem_object_metadata. [Krishna] > >>>> > >>>> v3: > >>>> - intel_pxp_gem_object_status() API changes. > >>>> > >>>> v4: use intel_pxp_is_active (Daniele) > >>>> > >>>> v5: rebase and use the new protected object status checker (Daniele) > >>>> > >>>> v6: used plane state for plane_decryption to handle async flip > >>>> as suggested by Ville. > >>>> > >>>> Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> > >>>> Cc: Huang Sean Z <sean.z.huang@intel.com> > >>>> Cc: Gaurav Kumar <kumar.gaurav@intel.com> > >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > >>>> Signed-off-by: Daniele Ceraolo Spurio > >>>> <daniele.ceraolospurio@intel.com> > >>>> --- > >>>> .../gpu/drm/i915/display/intel_atomic_plane.c | 3 ++ > >>>> drivers/gpu/drm/i915/display/intel_display.c | 5 +++ > >>>> .../drm/i915/display/intel_display_types.h | 3 ++ > >>>> .../drm/i915/display/skl_universal_plane.c | 32 +++++++++++++++++-- > >>>> .../drm/i915/display/skl_universal_plane.h | 1 + > >>>> drivers/gpu/drm/i915/i915_reg.h | 1 + > >>>> 6 files changed, 42 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>> index 7bfb26ca0bd0..7057077a2b71 100644 > >>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>> @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct > >>> intel_atomic_state *state, > >>>> intel_atomic_get_old_crtc_state(state, crtc); > >>>> struct intel_crtc_state *new_crtc_state = > >>>> intel_atomic_get_new_crtc_state(state, crtc); > >>>> + const struct drm_framebuffer *fb = new_plane_state->hw.fb; > >>>> > >>>> if (new_crtc_state && new_crtc_state->bigjoiner_slave) { > >>>> struct intel_plane *master_plane = > >>>> @@ -409,6 +410,8 @@ int intel_plane_atomic_check(struct > >>> intel_atomic_state *state, > >>>> intel_plane_copy_uapi_to_hw_state(new_plane_state, > >>>> new_master_plane_state, > >>>> crtc); > >>>> + new_plane_state->plane_decryption = > >>>> + i915_gem_object_has_valid_protection(intel_fb_obj(fb)); > >>>> > >>>> new_plane_state->uapi.visible = false; > >>>> if (!new_crtc_state) > >>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c > >>>> b/drivers/gpu/drm/i915/display/intel_display.c > >>>> index a10e26380ef3..55ab2d0b92d8 100644 > >>>> --- a/drivers/gpu/drm/i915/display/intel_display.c > >>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >>>> @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct > >>> intel_atomic_state *state) > >>>> drm_dbg_kms(&i915->drm, "Color range cannot be > >>> changed in async flip\n"); > >>>> return -EINVAL; > >>>> } > >>>> + > >>>> + /* plane decryption is allow to change only in synchronous flips > >>> */ > >>>> + if (old_plane_state->plane_decryption != new_plane_state- > >>>> plane_decryption) > >>>> + return -EINVAL; > >>>> } > >>>> > >>>> return 0; > >>>> @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct > >>>> drm_i915_private *dev_priv) > >>>> > >>>> crtc = intel_get_crtc_for_pipe(dev_priv, pipe); > >>>> crtc_state = to_intel_crtc_state(crtc->base.state); > >>>> + intel_plane_read_hw_decryption(plane_state); > >>> We don't have real plane state readout anyway, so seems pointless. > >>> > >>>> intel_set_plane_visible(crtc_state, plane_state, visible); > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > >>>> b/drivers/gpu/drm/i915/display/intel_display_types.h > >>>> index e2e707c4dff5..76b3bb64a36a 100644 > >>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h > >>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > >>>> @@ -617,6 +617,9 @@ struct intel_plane_state { > >>>> > >>>> struct intel_fb_view view; > >>>> > >>>> + /* Plane pxp decryption state */ > >>>> + bool plane_decryption; > >>>> + > >>> It's all about the plane, so the plane_ prefix is entirely redundant. > >>> Could just call it "decrypt" I guess. > >>> > >>>> /* plane control register */ > >>>> u32 ctl; > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > >>>> b/drivers/gpu/drm/i915/display/skl_universal_plane.c > >>>> index 75d3ca3dbb37..74489217e580 100644 > >>>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > >>>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > >>>> @@ -17,6 +17,7 @@ > >>>> #include "intel_sprite.h" > >>>> #include "skl_scaler.h" > >>>> #include "skl_universal_plane.h" > >>>> +#include "pxp/intel_pxp.h" > >>>> > >>>> static const u32 skl_plane_formats[] = { > >>>> DRM_FORMAT_C8, > >>>> @@ -956,7 +957,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); @@ -1037,8 +1038,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_is_active(&dev_priv->gt.pxp) && > >>> That should all be part of the state computation. And you're missing this in the > >>> .async_flip path totally. > >> Hi Ville / Rodrigo / Daniele, > >> Is it possible to check intel_pxp_is_active() in plane atomic check function to compute plane decryption state? > > Yes, it should be possible to call that function from anywhere. > > >> with my best knowledge session can be invalid at any time, Let's say in plane atomic check function pxp session was enabled > >> but while in atomic commit pxp session can be still disabled. > > I can be invalidated any time after the commit anyway. What happens in > > that case? > > If we flip a PXP object after the relevant key has been invalidated we > just get garbage on the screen. > Note that it is understood that this is a race we can't completely close > given that the session invalidation can hit us at any time, It should be possible if the invalidation thingy gave us a warning ahead of time and then waited for us to actually stop scanout. > the aim here > is just to make the window as small as we can and replace invalid > objects with a black frame where possible. > > Daniele >
On 4/28/2021 1:04 PM, Ville Syrjälä wrote: > On Wed, Apr 28, 2021 at 10:32:46AM -0700, Daniele Ceraolo Spurio wrote: >> >> On 4/28/2021 5:03 AM, Ville Syrjälä wrote: >>> On Wed, Apr 28, 2021 at 11:25:23AM +0000, Gupta, Anshuman wrote: >>>>> -----Original Message----- >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>>> Sent: Wednesday, April 28, 2021 12:25 AM >>>>> To: Gupta, Anshuman <anshuman.gupta@intel.com> >>>>> Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; >>>>> Bommu, Krishnaiah <krishnaiah.bommu@intel.com>; Huang Sean Z >>>>> <sean.z.huang@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com>; >>>>> Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> >>>>> Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support >>>> Thanks Ville for review, Below are some doubts with respect to pxp state. >>>>> On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote: >>>>>> 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: >>>>>> - Used gen fb obj user_flags instead gem_object_metadata. [Krishna] >>>>>> >>>>>> v3: >>>>>> - intel_pxp_gem_object_status() API changes. >>>>>> >>>>>> v4: use intel_pxp_is_active (Daniele) >>>>>> >>>>>> v5: rebase and use the new protected object status checker (Daniele) >>>>>> >>>>>> v6: used plane state for plane_decryption to handle async flip >>>>>> as suggested by Ville. >>>>>> >>>>>> Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> >>>>>> Cc: Huang Sean Z <sean.z.huang@intel.com> >>>>>> Cc: Gaurav Kumar <kumar.gaurav@intel.com> >>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> >>>>>> Signed-off-by: Daniele Ceraolo Spurio >>>>>> <daniele.ceraolospurio@intel.com> >>>>>> --- >>>>>> .../gpu/drm/i915/display/intel_atomic_plane.c | 3 ++ >>>>>> drivers/gpu/drm/i915/display/intel_display.c | 5 +++ >>>>>> .../drm/i915/display/intel_display_types.h | 3 ++ >>>>>> .../drm/i915/display/skl_universal_plane.c | 32 +++++++++++++++++-- >>>>>> .../drm/i915/display/skl_universal_plane.h | 1 + >>>>>> drivers/gpu/drm/i915/i915_reg.h | 1 + >>>>>> 6 files changed, 42 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>>>> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>>>> index 7bfb26ca0bd0..7057077a2b71 100644 >>>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>>>> @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct >>>>> intel_atomic_state *state, >>>>>> intel_atomic_get_old_crtc_state(state, crtc); >>>>>> struct intel_crtc_state *new_crtc_state = >>>>>> intel_atomic_get_new_crtc_state(state, crtc); >>>>>> + const struct drm_framebuffer *fb = new_plane_state->hw.fb; >>>>>> >>>>>> if (new_crtc_state && new_crtc_state->bigjoiner_slave) { >>>>>> struct intel_plane *master_plane = >>>>>> @@ -409,6 +410,8 @@ int intel_plane_atomic_check(struct >>>>> intel_atomic_state *state, >>>>>> intel_plane_copy_uapi_to_hw_state(new_plane_state, >>>>>> new_master_plane_state, >>>>>> crtc); >>>>>> + new_plane_state->plane_decryption = >>>>>> + i915_gem_object_has_valid_protection(intel_fb_obj(fb)); >>>>>> >>>>>> new_plane_state->uapi.visible = false; >>>>>> if (!new_crtc_state) >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c >>>>>> b/drivers/gpu/drm/i915/display/intel_display.c >>>>>> index a10e26380ef3..55ab2d0b92d8 100644 >>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c >>>>>> @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct >>>>> intel_atomic_state *state) >>>>>> drm_dbg_kms(&i915->drm, "Color range cannot be >>>>> changed in async flip\n"); >>>>>> return -EINVAL; >>>>>> } >>>>>> + >>>>>> + /* plane decryption is allow to change only in synchronous flips >>>>> */ >>>>>> + if (old_plane_state->plane_decryption != new_plane_state- >>>>>> plane_decryption) >>>>>> + return -EINVAL; >>>>>> } >>>>>> >>>>>> return 0; >>>>>> @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct >>>>>> drm_i915_private *dev_priv) >>>>>> >>>>>> crtc = intel_get_crtc_for_pipe(dev_priv, pipe); >>>>>> crtc_state = to_intel_crtc_state(crtc->base.state); >>>>>> + intel_plane_read_hw_decryption(plane_state); >>>>> We don't have real plane state readout anyway, so seems pointless. >>>>> >>>>>> intel_set_plane_visible(crtc_state, plane_state, visible); >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h >>>>>> b/drivers/gpu/drm/i915/display/intel_display_types.h >>>>>> index e2e707c4dff5..76b3bb64a36a 100644 >>>>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >>>>>> @@ -617,6 +617,9 @@ struct intel_plane_state { >>>>>> >>>>>> struct intel_fb_view view; >>>>>> >>>>>> + /* Plane pxp decryption state */ >>>>>> + bool plane_decryption; >>>>>> + >>>>> It's all about the plane, so the plane_ prefix is entirely redundant. >>>>> Could just call it "decrypt" I guess. >>>>> >>>>>> /* plane control register */ >>>>>> u32 ctl; >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c >>>>>> b/drivers/gpu/drm/i915/display/skl_universal_plane.c >>>>>> index 75d3ca3dbb37..74489217e580 100644 >>>>>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c >>>>>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c >>>>>> @@ -17,6 +17,7 @@ >>>>>> #include "intel_sprite.h" >>>>>> #include "skl_scaler.h" >>>>>> #include "skl_universal_plane.h" >>>>>> +#include "pxp/intel_pxp.h" >>>>>> >>>>>> static const u32 skl_plane_formats[] = { >>>>>> DRM_FORMAT_C8, >>>>>> @@ -956,7 +957,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); @@ -1037,8 +1038,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_is_active(&dev_priv->gt.pxp) && >>>>> That should all be part of the state computation. And you're missing this in the >>>>> .async_flip path totally. >>>> Hi Ville / Rodrigo / Daniele, >>>> Is it possible to check intel_pxp_is_active() in plane atomic check function to compute plane decryption state? >> Yes, it should be possible to call that function from anywhere. >> >>>> with my best knowledge session can be invalid at any time, Let's say in plane atomic check function pxp session was enabled >>>> but while in atomic commit pxp session can be still disabled. >>> I can be invalidated any time after the commit anyway. What happens in >>> that case? >> If we flip a PXP object after the relevant key has been invalidated we >> just get garbage on the screen. >> Note that it is understood that this is a race we can't completely close >> given that the session invalidation can hit us at any time, > It should be possible if the invalidation thingy gave us a warning > ahead of time and then waited for us to actually stop scanout. Apart from suspend/resume scenarios, invalidations mainly occur when there is an event that the HW perceives as something that could compromise the security, so the keys are revoked immediately. There is a GuC-controlled timer that can be enabled to have a grace period for the user to remove the problem before revoking the keys (I was planning to send that as a follow-up), but AFAIU there is no way to control that from the CPU. Daniele > >> the aim here >> is just to make the window as small as we can and replace invalid >> objects with a black frame where possible. >> >> Daniele >>
> -----Original Message----- > From: Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> > Sent: Thursday, April 29, 2021 2:10 AM > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Vivi, Rodrigo > <rodrigo.vivi@intel.com>; intel-gfx@lists.freedesktop.org; Bommu, Krishnaiah > <krishnaiah.bommu@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com> > Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support > > > > On 4/28/2021 1:04 PM, Ville Syrjälä wrote: > > On Wed, Apr 28, 2021 at 10:32:46AM -0700, Daniele Ceraolo Spurio wrote: > >> > >> On 4/28/2021 5:03 AM, Ville Syrjälä wrote: > >>> On Wed, Apr 28, 2021 at 11:25:23AM +0000, Gupta, Anshuman wrote: > >>>>> -----Original Message----- > >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>>>> Sent: Wednesday, April 28, 2021 12:25 AM > >>>>> To: Gupta, Anshuman <anshuman.gupta@intel.com> > >>>>> Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo > >>>>> <rodrigo.vivi@intel.com>; Bommu, Krishnaiah > >>>>> <krishnaiah.bommu@intel.com>; Huang Sean Z > >>>>> <sean.z.huang@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com>; > >>>>> Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> > >>>>> Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption > >>>>> support > >>>> Thanks Ville for review, Below are some doubts with respect to pxp state. > >>>>> On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote: > >>>>>> 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: > >>>>>> - Used gen fb obj user_flags instead gem_object_metadata. > >>>>>> [Krishna] > >>>>>> > >>>>>> v3: > >>>>>> - intel_pxp_gem_object_status() API changes. > >>>>>> > >>>>>> v4: use intel_pxp_is_active (Daniele) > >>>>>> > >>>>>> v5: rebase and use the new protected object status checker > >>>>>> (Daniele) > >>>>>> > >>>>>> v6: used plane state for plane_decryption to handle async flip > >>>>>> as suggested by Ville. > >>>>>> > >>>>>> Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> > >>>>>> Cc: Huang Sean Z <sean.z.huang@intel.com> > >>>>>> Cc: Gaurav Kumar <kumar.gaurav@intel.com> > >>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > >>>>>> Signed-off-by: Daniele Ceraolo Spurio > >>>>>> <daniele.ceraolospurio@intel.com> > >>>>>> --- > >>>>>> .../gpu/drm/i915/display/intel_atomic_plane.c | 3 ++ > >>>>>> drivers/gpu/drm/i915/display/intel_display.c | 5 +++ > >>>>>> .../drm/i915/display/intel_display_types.h | 3 ++ > >>>>>> .../drm/i915/display/skl_universal_plane.c | 32 +++++++++++++++++- > - > >>>>>> .../drm/i915/display/skl_universal_plane.h | 1 + > >>>>>> drivers/gpu/drm/i915/i915_reg.h | 1 + > >>>>>> 6 files changed, 42 insertions(+), 3 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>>>> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>>>> index 7bfb26ca0bd0..7057077a2b71 100644 > >>>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>>>> @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct > >>>>> intel_atomic_state *state, > >>>>>> intel_atomic_get_old_crtc_state(state, crtc); > >>>>>> struct intel_crtc_state *new_crtc_state = > >>>>>> intel_atomic_get_new_crtc_state(state, crtc); > >>>>>> + const struct drm_framebuffer *fb = new_plane_state->hw.fb; > >>>>>> > >>>>>> if (new_crtc_state && new_crtc_state->bigjoiner_slave) { > >>>>>> struct intel_plane *master_plane = @@ -409,6 +410,8 > @@ int > >>>>>> intel_plane_atomic_check(struct > >>>>> intel_atomic_state *state, > >>>>>> intel_plane_copy_uapi_to_hw_state(new_plane_state, > >>>>>> new_master_plane_state, > >>>>>> crtc); > >>>>>> + new_plane_state->plane_decryption = > >>>>>> + > i915_gem_object_has_valid_protection(intel_fb_obj(fb)); > >>>>>> > >>>>>> new_plane_state->uapi.visible = false; > >>>>>> if (!new_crtc_state) > >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c > >>>>>> b/drivers/gpu/drm/i915/display/intel_display.c > >>>>>> index a10e26380ef3..55ab2d0b92d8 100644 > >>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c > >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >>>>>> @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct > >>>>> intel_atomic_state *state) > >>>>>> drm_dbg_kms(&i915->drm, "Color range > cannot be > >>>>> changed in async flip\n"); > >>>>>> return -EINVAL; > >>>>>> } > >>>>>> + > >>>>>> + /* plane decryption is allow to change only in > synchronous > >>>>>> +flips > >>>>> */ > >>>>>> + if (old_plane_state->plane_decryption != > new_plane_state- > >>>>>> plane_decryption) > >>>>>> + return -EINVAL; > >>>>>> } > >>>>>> > >>>>>> return 0; > >>>>>> @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct > >>>>>> drm_i915_private *dev_priv) > >>>>>> > >>>>>> crtc = intel_get_crtc_for_pipe(dev_priv, pipe); > >>>>>> crtc_state = to_intel_crtc_state(crtc->base.state); > >>>>>> + intel_plane_read_hw_decryption(plane_state); > >>>>> We don't have real plane state readout anyway, so seems pointless. > >>>>> > >>>>>> intel_set_plane_visible(crtc_state, plane_state, visible); > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > >>>>>> b/drivers/gpu/drm/i915/display/intel_display_types.h > >>>>>> index e2e707c4dff5..76b3bb64a36a 100644 > >>>>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h > >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > >>>>>> @@ -617,6 +617,9 @@ struct intel_plane_state { > >>>>>> > >>>>>> struct intel_fb_view view; > >>>>>> > >>>>>> + /* Plane pxp decryption state */ > >>>>>> + bool plane_decryption; > >>>>>> + > >>>>> It's all about the plane, so the plane_ prefix is entirely redundant. > >>>>> Could just call it "decrypt" I guess. > >>>>> > >>>>>> /* plane control register */ > >>>>>> u32 ctl; > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > >>>>>> b/drivers/gpu/drm/i915/display/skl_universal_plane.c > >>>>>> index 75d3ca3dbb37..74489217e580 100644 > >>>>>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > >>>>>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > >>>>>> @@ -17,6 +17,7 @@ > >>>>>> #include "intel_sprite.h" > >>>>>> #include "skl_scaler.h" > >>>>>> #include "skl_universal_plane.h" > >>>>>> +#include "pxp/intel_pxp.h" > >>>>>> > >>>>>> static const u32 skl_plane_formats[] = { > >>>>>> DRM_FORMAT_C8, > >>>>>> @@ -956,7 +957,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); @@ -1037,8 > >>>>>> +1038,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_is_active(&dev_priv->gt.pxp) && > >>>>> That should all be part of the state computation. And you're > >>>>> missing this in the .async_flip path totally. > >>>> Hi Ville / Rodrigo / Daniele, > >>>> Is it possible to check intel_pxp_is_active() in plane atomic check function > to compute plane decryption state? > >> Yes, it should be possible to call that function from anywhere. > >> > >>>> with my best knowledge session can be invalid at any time, Let's > >>>> say in plane atomic check function pxp session was enabled but while in > atomic commit pxp session can be still disabled. > >>> I can be invalidated any time after the commit anyway. What happens > >>> in that case? > >> If we flip a PXP object after the relevant key has been invalidated > >> we just get garbage on the screen. > >> Note that it is understood that this is a race we can't completely > >> close given that the session invalidation can hit us at any time, > > It should be possible if the invalidation thingy gave us a warning > > ahead of time and then waited for us to actually stop scanout. > > Apart from suspend/resume scenarios, invalidations mainly occur when there is > an event that the HW perceives as something that could compromise the > security, so the keys are revoked immediately. There is a GuC-controlled timer > that can be enabled to have a grace period for the user to remove the problem > before revoking the keys (I was planning to send that as a follow-up), but AFAIU > there is no way to control that from the CPU. > > Daniele AFAIU with respect to inputs from Ville and Daniele, I can think of something like below , Compute plane state by checking both PXP session is enabled and Buffer object is protected plane_state->plane_decryption = intel_pxp_is_active() && i915_gem_object_has_valid_protection() and let's check the whether pxp session is still valid before setting the plane decryption bit. If (plane_state->plane_decryption = = intel_pxp_is_active ) plane_surf |= PLANE_SURF_DECRYPTION_ENABLED else display black pixels Please provide yours opinion if above approach is correct ? Thanks, Anshuman Gupta. > > > > >> the aim here > >> is just to make the window as small as we can and replace invalid > >> objects with a black frame where possible. > >> > >> Daniele > >>
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Wednesday, April 28, 2021 12:25 AM > To: Gupta, Anshuman <anshuman.gupta@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; > Bommu, Krishnaiah <krishnaiah.bommu@intel.com>; Huang Sean Z > <sean.z.huang@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com>; > Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> > Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support > > On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote: > > 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: > > - Used gen fb obj user_flags instead gem_object_metadata. [Krishna] > > > > v3: > > - intel_pxp_gem_object_status() API changes. > > > > v4: use intel_pxp_is_active (Daniele) > > > > v5: rebase and use the new protected object status checker (Daniele) > > > > v6: used plane state for plane_decryption to handle async flip > > as suggested by Ville. > > > > Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> > > Cc: Huang Sean Z <sean.z.huang@intel.com> > > Cc: Gaurav Kumar <kumar.gaurav@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > Signed-off-by: Daniele Ceraolo Spurio > > <daniele.ceraolospurio@intel.com> > > --- > > .../gpu/drm/i915/display/intel_atomic_plane.c | 3 ++ > > drivers/gpu/drm/i915/display/intel_display.c | 5 +++ > > .../drm/i915/display/intel_display_types.h | 3 ++ > > .../drm/i915/display/skl_universal_plane.c | 32 +++++++++++++++++-- > > .../drm/i915/display/skl_universal_plane.h | 1 + > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 6 files changed, 42 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > index 7bfb26ca0bd0..7057077a2b71 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct > intel_atomic_state *state, > > intel_atomic_get_old_crtc_state(state, crtc); > > struct intel_crtc_state *new_crtc_state = > > intel_atomic_get_new_crtc_state(state, crtc); > > + const struct drm_framebuffer *fb = new_plane_state->hw.fb; > > > > if (new_crtc_state && new_crtc_state->bigjoiner_slave) { > > struct intel_plane *master_plane = > > @@ -409,6 +410,8 @@ int intel_plane_atomic_check(struct > intel_atomic_state *state, > > intel_plane_copy_uapi_to_hw_state(new_plane_state, > > new_master_plane_state, > > crtc); > > + new_plane_state->plane_decryption = > > + i915_gem_object_has_valid_protection(intel_fb_obj(fb)); > > > > new_plane_state->uapi.visible = false; > > if (!new_crtc_state) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index a10e26380ef3..55ab2d0b92d8 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct > intel_atomic_state *state) > > drm_dbg_kms(&i915->drm, "Color range cannot be > changed in async flip\n"); > > return -EINVAL; > > } > > + > > + /* plane decryption is allow to change only in synchronous flips > */ > > + if (old_plane_state->plane_decryption != new_plane_state- > >plane_decryption) > > + return -EINVAL; > > } > > > > return 0; > > @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct > > drm_i915_private *dev_priv) > > > > crtc = intel_get_crtc_for_pipe(dev_priv, pipe); > > crtc_state = to_intel_crtc_state(crtc->base.state); > > + intel_plane_read_hw_decryption(plane_state); > > We don't have real plane state readout anyway, so seems pointless. could u please guide me the correct place to read the old plane decrypt State, some where we do need to populate the plane decrypt state with plane surface register decrypt bit. How about plane->get_hw_state ? Thanks, Anshuman Gupta. > > > > > intel_set_plane_visible(crtc_state, plane_state, visible); > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index e2e707c4dff5..76b3bb64a36a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -617,6 +617,9 @@ struct intel_plane_state { > > > > struct intel_fb_view view; > > > > + /* Plane pxp decryption state */ > > + bool plane_decryption; > > + > > It's all about the plane, so the plane_ prefix is entirely redundant. > Could just call it "decrypt" I guess. > > > /* plane control register */ > > u32 ctl; > > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > index 75d3ca3dbb37..74489217e580 100644 > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > @@ -17,6 +17,7 @@ > > #include "intel_sprite.h" > > #include "skl_scaler.h" > > #include "skl_universal_plane.h" > > +#include "pxp/intel_pxp.h" > > > > static const u32 skl_plane_formats[] = { > > DRM_FORMAT_C8, > > @@ -956,7 +957,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); @@ -1037,8 +1038,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_is_active(&dev_priv->gt.pxp) && > > That should all be part of the state computation. And you're missing this in the > .async_flip path totally. > > > + plane_state->plane_decryption) > > + plane_surf |= PLANE_SURF_DECRYPTION_ENABLED; > > + else > > + plane_surf &= ~PLANE_SURF_DECRYPTION_ENABLED; > > This &=~ stuff is still 100% pointless. > > So just > if (plane_state->decrypt) > surf_addr |= ...; > > should do it. And I'd do it around the top of the function where we calculate a > bunch of the other register values. That keeps most of the function clean of > random ifs. Just after the aux_dist calculation would seem like the correct spot > to me. > > > + > > + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), plane_surf); > > > > if (plane_state->scaler_id >= 0) > > skl_program_plane_scaler(plane, crtc_state, plane_state); @@ > > -2242,3 +2250,21 @@ skl_get_initial_plane_config(struct intel_crtc *crtc, > > kfree(intel_fb); > > } > > > > +void intel_plane_read_hw_decryption(struct intel_plane_state > > +*plane_state) { > > + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); > > + struct drm_i915_private *i915 = to_i915(plane->base.dev); > > + enum intel_display_power_domain power_domain; > > + enum plane_id plane_id = plane->id; > > + intel_wakeref_t wakeref; > > + > > + power_domain = POWER_DOMAIN_PIPE(plane->pipe); > > + wakeref = intel_display_power_get_if_enabled(i915, power_domain); > > + if (drm_WARN_ON(&i915->drm, !wakeref)) > > + return; > > + > > + plane_state->plane_decryption = > > + intel_de_read(i915, PLANE_SURF(plane->pipe, plane_id)) & > > +PLANE_SURF_DECRYPTION_ENABLED; > > + > > + intel_display_power_put(i915, power_domain, wakeref); } > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.h > > b/drivers/gpu/drm/i915/display/skl_universal_plane.h > > index 351040b64dc7..2b1d673f8bf5 100644 > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.h > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.h > > @@ -31,5 +31,6 @@ int skl_calc_main_surface_offset(const struct > > intel_plane_state *plane_state, bool icl_is_nv12_y_plane(struct > drm_i915_private *dev_priv, > > enum plane_id plane_id); > > bool icl_is_hdr_plane(struct drm_i915_private *dev_priv, enum > > plane_id plane_id); > > +void intel_plane_read_hw_decryption(struct intel_plane_state > > +*plane_state); > > > > #endif > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h index 740e97663fec..fbaf9199001d > > 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7240,6 +7240,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) > > Could be just PLANE_SURF_DECRYPT or something along those lines. > > > > > #define _PLANE_OFFSET_1_B 0x711a4 > > #define _PLANE_OFFSET_2_B 0x712a4 > > -- > > 2.26.2 > > -- > Ville Syrjälä > Intel
On Fri, Apr 30, 2021 at 06:56:10AM +0000, Gupta, Anshuman wrote: > > > > -----Original Message----- > > From: Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> > > Sent: Thursday, April 29, 2021 2:10 AM > > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Vivi, Rodrigo > > <rodrigo.vivi@intel.com>; intel-gfx@lists.freedesktop.org; Bommu, Krishnaiah > > <krishnaiah.bommu@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com> > > Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support > > > > > > > > On 4/28/2021 1:04 PM, Ville Syrjälä wrote: > > > On Wed, Apr 28, 2021 at 10:32:46AM -0700, Daniele Ceraolo Spurio wrote: > > >> > > >> On 4/28/2021 5:03 AM, Ville Syrjälä wrote: > > >>> On Wed, Apr 28, 2021 at 11:25:23AM +0000, Gupta, Anshuman wrote: > > >>>>> -----Original Message----- > > >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >>>>> Sent: Wednesday, April 28, 2021 12:25 AM > > >>>>> To: Gupta, Anshuman <anshuman.gupta@intel.com> > > >>>>> Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo > > >>>>> <rodrigo.vivi@intel.com>; Bommu, Krishnaiah > > >>>>> <krishnaiah.bommu@intel.com>; Huang Sean Z > > >>>>> <sean.z.huang@intel.com>; Gaurav, Kumar <kumar.gaurav@intel.com>; > > >>>>> Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> > > >>>>> Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption > > >>>>> support > > >>>> Thanks Ville for review, Below are some doubts with respect to pxp state. > > >>>>> On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote: > > >>>>>> 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: > > >>>>>> - Used gen fb obj user_flags instead gem_object_metadata. > > >>>>>> [Krishna] > > >>>>>> > > >>>>>> v3: > > >>>>>> - intel_pxp_gem_object_status() API changes. > > >>>>>> > > >>>>>> v4: use intel_pxp_is_active (Daniele) > > >>>>>> > > >>>>>> v5: rebase and use the new protected object status checker > > >>>>>> (Daniele) > > >>>>>> > > >>>>>> v6: used plane state for plane_decryption to handle async flip > > >>>>>> as suggested by Ville. > > >>>>>> > > >>>>>> Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> > > >>>>>> Cc: Huang Sean Z <sean.z.huang@intel.com> > > >>>>>> Cc: Gaurav Kumar <kumar.gaurav@intel.com> > > >>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >>>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > >>>>>> Signed-off-by: Daniele Ceraolo Spurio > > >>>>>> <daniele.ceraolospurio@intel.com> > > >>>>>> --- > > >>>>>> .../gpu/drm/i915/display/intel_atomic_plane.c | 3 ++ > > >>>>>> drivers/gpu/drm/i915/display/intel_display.c | 5 +++ > > >>>>>> .../drm/i915/display/intel_display_types.h | 3 ++ > > >>>>>> .../drm/i915/display/skl_universal_plane.c | 32 +++++++++++++++++- > > - > > >>>>>> .../drm/i915/display/skl_universal_plane.h | 1 + > > >>>>>> drivers/gpu/drm/i915/i915_reg.h | 1 + > > >>>>>> 6 files changed, 42 insertions(+), 3 deletions(-) > > >>>>>> > > >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > >>>>>> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > >>>>>> index 7bfb26ca0bd0..7057077a2b71 100644 > > >>>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > >>>>>> @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct > > >>>>> intel_atomic_state *state, > > >>>>>> intel_atomic_get_old_crtc_state(state, crtc); > > >>>>>> struct intel_crtc_state *new_crtc_state = > > >>>>>> intel_atomic_get_new_crtc_state(state, crtc); > > >>>>>> + const struct drm_framebuffer *fb = new_plane_state->hw.fb; > > >>>>>> > > >>>>>> if (new_crtc_state && new_crtc_state->bigjoiner_slave) { > > >>>>>> struct intel_plane *master_plane = @@ -409,6 +410,8 > > @@ int > > >>>>>> intel_plane_atomic_check(struct > > >>>>> intel_atomic_state *state, > > >>>>>> intel_plane_copy_uapi_to_hw_state(new_plane_state, > > >>>>>> new_master_plane_state, > > >>>>>> crtc); > > >>>>>> + new_plane_state->plane_decryption = > > >>>>>> + > > i915_gem_object_has_valid_protection(intel_fb_obj(fb)); > > >>>>>> > > >>>>>> new_plane_state->uapi.visible = false; > > >>>>>> if (!new_crtc_state) > > >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > >>>>>> b/drivers/gpu/drm/i915/display/intel_display.c > > >>>>>> index a10e26380ef3..55ab2d0b92d8 100644 > > >>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c > > >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c > > >>>>>> @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct > > >>>>> intel_atomic_state *state) > > >>>>>> drm_dbg_kms(&i915->drm, "Color range > > cannot be > > >>>>> changed in async flip\n"); > > >>>>>> return -EINVAL; > > >>>>>> } > > >>>>>> + > > >>>>>> + /* plane decryption is allow to change only in > > synchronous > > >>>>>> +flips > > >>>>> */ > > >>>>>> + if (old_plane_state->plane_decryption != > > new_plane_state- > > >>>>>> plane_decryption) > > >>>>>> + return -EINVAL; > > >>>>>> } > > >>>>>> > > >>>>>> return 0; > > >>>>>> @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct > > >>>>>> drm_i915_private *dev_priv) > > >>>>>> > > >>>>>> crtc = intel_get_crtc_for_pipe(dev_priv, pipe); > > >>>>>> crtc_state = to_intel_crtc_state(crtc->base.state); > > >>>>>> + intel_plane_read_hw_decryption(plane_state); > > >>>>> We don't have real plane state readout anyway, so seems pointless. > > >>>>> > > >>>>>> intel_set_plane_visible(crtc_state, plane_state, visible); > > >>>>>> > > >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > >>>>>> b/drivers/gpu/drm/i915/display/intel_display_types.h > > >>>>>> index e2e707c4dff5..76b3bb64a36a 100644 > > >>>>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > >>>>>> @@ -617,6 +617,9 @@ struct intel_plane_state { > > >>>>>> > > >>>>>> struct intel_fb_view view; > > >>>>>> > > >>>>>> + /* Plane pxp decryption state */ > > >>>>>> + bool plane_decryption; > > >>>>>> + > > >>>>> It's all about the plane, so the plane_ prefix is entirely redundant. > > >>>>> Could just call it "decrypt" I guess. > > >>>>> > > >>>>>> /* plane control register */ > > >>>>>> u32 ctl; > > >>>>>> > > >>>>>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > >>>>>> b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > >>>>>> index 75d3ca3dbb37..74489217e580 100644 > > >>>>>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > >>>>>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > >>>>>> @@ -17,6 +17,7 @@ > > >>>>>> #include "intel_sprite.h" > > >>>>>> #include "skl_scaler.h" > > >>>>>> #include "skl_universal_plane.h" > > >>>>>> +#include "pxp/intel_pxp.h" > > >>>>>> > > >>>>>> static const u32 skl_plane_formats[] = { > > >>>>>> DRM_FORMAT_C8, > > >>>>>> @@ -956,7 +957,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); @@ -1037,8 > > >>>>>> +1038,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_is_active(&dev_priv->gt.pxp) && > > >>>>> That should all be part of the state computation. And you're > > >>>>> missing this in the .async_flip path totally. > > >>>> Hi Ville / Rodrigo / Daniele, > > >>>> Is it possible to check intel_pxp_is_active() in plane atomic check function > > to compute plane decryption state? > > >> Yes, it should be possible to call that function from anywhere. > > >> > > >>>> with my best knowledge session can be invalid at any time, Let's > > >>>> say in plane atomic check function pxp session was enabled but while in > > atomic commit pxp session can be still disabled. > > >>> I can be invalidated any time after the commit anyway. What happens > > >>> in that case? > > >> If we flip a PXP object after the relevant key has been invalidated > > >> we just get garbage on the screen. > > >> Note that it is understood that this is a race we can't completely > > >> close given that the session invalidation can hit us at any time, > > > It should be possible if the invalidation thingy gave us a warning > > > ahead of time and then waited for us to actually stop scanout. > > > > Apart from suspend/resume scenarios, invalidations mainly occur when there is > > an event that the HW perceives as something that could compromise the > > security, so the keys are revoked immediately. There is a GuC-controlled timer > > that can be enabled to have a grace period for the user to remove the problem > > before revoking the keys (I was planning to send that as a follow-up), but AFAIU > > there is no way to control that from the CPU. > > > > Daniele > AFAIU with respect to inputs from Ville and Daniele, I can think of something like below , > Compute plane state by checking both PXP session is enabled and Buffer object is protected > > plane_state->plane_decryption = intel_pxp_is_active() && i915_gem_object_has_valid_protection() > > and let's check the whether pxp session is still valid before setting the plane decryption bit. > > If (plane_state->plane_decryption = = intel_pxp_is_active ) > plane_surf |= PLANE_SURF_DECRYPTION_ENABLED > else > display black pixels > > Please provide yours opinion if above approach is correct ? I think we need something like plane_state->decrypt = bo_is_encrypted(); plane_state->force_black = plane_state->decrypt && pxp_is_bork; The other problem with this approach is that it still totally depends on userspace submitting a new flip/update on this specific plane. To make it a bit more robust we should perhaps track which planes are scanning out encrypted stuff and force all of them to update if things got revoked. But we can think about that more later.
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index 7bfb26ca0bd0..7057077a2b71 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct intel_atomic_state *state, intel_atomic_get_old_crtc_state(state, crtc); struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); + const struct drm_framebuffer *fb = new_plane_state->hw.fb; if (new_crtc_state && new_crtc_state->bigjoiner_slave) { struct intel_plane *master_plane = @@ -409,6 +410,8 @@ int intel_plane_atomic_check(struct intel_atomic_state *state, intel_plane_copy_uapi_to_hw_state(new_plane_state, new_master_plane_state, crtc); + new_plane_state->plane_decryption = + i915_gem_object_has_valid_protection(intel_fb_obj(fb)); new_plane_state->uapi.visible = false; if (!new_crtc_state) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index a10e26380ef3..55ab2d0b92d8 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct intel_atomic_state *state) drm_dbg_kms(&i915->drm, "Color range cannot be changed in async flip\n"); return -EINVAL; } + + /* plane decryption is allow to change only in synchronous flips */ + if (old_plane_state->plane_decryption != new_plane_state->plane_decryption) + return -EINVAL; } return 0; @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct drm_i915_private *dev_priv) crtc = intel_get_crtc_for_pipe(dev_priv, pipe); crtc_state = to_intel_crtc_state(crtc->base.state); + intel_plane_read_hw_decryption(plane_state); intel_set_plane_visible(crtc_state, plane_state, visible); diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index e2e707c4dff5..76b3bb64a36a 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -617,6 +617,9 @@ struct intel_plane_state { struct intel_fb_view view; + /* Plane pxp decryption state */ + bool plane_decryption; + /* plane control register */ u32 ctl; diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c index 75d3ca3dbb37..74489217e580 100644 --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c @@ -17,6 +17,7 @@ #include "intel_sprite.h" #include "skl_scaler.h" #include "skl_universal_plane.h" +#include "pxp/intel_pxp.h" static const u32 skl_plane_formats[] = { DRM_FORMAT_C8, @@ -956,7 +957,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); @@ -1037,8 +1038,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_is_active(&dev_priv->gt.pxp) && + plane_state->plane_decryption) + 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_plane_scaler(plane, crtc_state, plane_state); @@ -2242,3 +2250,21 @@ skl_get_initial_plane_config(struct intel_crtc *crtc, kfree(intel_fb); } +void intel_plane_read_hw_decryption(struct intel_plane_state *plane_state) +{ + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); + struct drm_i915_private *i915 = to_i915(plane->base.dev); + enum intel_display_power_domain power_domain; + enum plane_id plane_id = plane->id; + intel_wakeref_t wakeref; + + power_domain = POWER_DOMAIN_PIPE(plane->pipe); + wakeref = intel_display_power_get_if_enabled(i915, power_domain); + if (drm_WARN_ON(&i915->drm, !wakeref)) + return; + + plane_state->plane_decryption = + intel_de_read(i915, PLANE_SURF(plane->pipe, plane_id)) & PLANE_SURF_DECRYPTION_ENABLED; + + intel_display_power_put(i915, power_domain, wakeref); +} diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.h b/drivers/gpu/drm/i915/display/skl_universal_plane.h index 351040b64dc7..2b1d673f8bf5 100644 --- a/drivers/gpu/drm/i915/display/skl_universal_plane.h +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.h @@ -31,5 +31,6 @@ int skl_calc_main_surface_offset(const struct intel_plane_state *plane_state, bool icl_is_nv12_y_plane(struct drm_i915_private *dev_priv, enum plane_id plane_id); bool icl_is_hdr_plane(struct drm_i915_private *dev_priv, enum plane_id plane_id); +void intel_plane_read_hw_decryption(struct intel_plane_state *plane_state); #endif diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 740e97663fec..fbaf9199001d 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7240,6 +7240,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
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: - Used gen fb obj user_flags instead gem_object_metadata. [Krishna] v3: - intel_pxp_gem_object_status() API changes. v4: use intel_pxp_is_active (Daniele) v5: rebase and use the new protected object status checker (Daniele) v6: used plane state for plane_decryption to handle async flip as suggested by Ville. Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> Cc: Huang Sean Z <sean.z.huang@intel.com> Cc: Gaurav Kumar <kumar.gaurav@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- .../gpu/drm/i915/display/intel_atomic_plane.c | 3 ++ drivers/gpu/drm/i915/display/intel_display.c | 5 +++ .../drm/i915/display/intel_display_types.h | 3 ++ .../drm/i915/display/skl_universal_plane.c | 32 +++++++++++++++++-- .../drm/i915/display/skl_universal_plane.h | 1 + drivers/gpu/drm/i915/i915_reg.h | 1 + 6 files changed, 42 insertions(+), 3 deletions(-)