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@xxxxxxxxxxxxxxx> > >>> Sent: Wednesday, April 28, 2021 12:25 AM > >>> To: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx> > >>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; > >>> Bommu, Krishnaiah <krishnaiah.bommu@xxxxxxxxx>; Huang Sean Z > >>> <sean.z.huang@xxxxxxxxx>; Gaurav, Kumar <kumar.gaurav@xxxxxxxxx>; > >>> Ceraolo Spurio, Daniele <daniele.ceraolospurio@xxxxxxxxx> > >>> 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@xxxxxxxxx> > >>>> Cc: Huang Sean Z <sean.z.huang@xxxxxxxxx> > >>>> Cc: Gaurav Kumar <kumar.gaurav@xxxxxxxxx> > >>>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx> > >>>> Signed-off-by: Daniele Ceraolo Spurio > >>>> <daniele.ceraolospurio@xxxxxxxxx> > >>>> --- > >>>> .../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 > -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx