[Intel-gfx] [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Wed Apr 28 17:32:46 UTC 2021
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 at linux.intel.com>
>>> Sent: Wednesday, April 28, 2021 12:25 AM
>>> To: Gupta, Anshuman <anshuman.gupta at intel.com>
>>> Cc: intel-gfx at lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi at intel.com>;
>>> Bommu, Krishnaiah <krishnaiah.bommu at intel.com>; Huang Sean Z
>>> <sean.z.huang at intel.com>; Gaurav, Kumar <kumar.gaurav at intel.com>;
>>> Ceraolo Spurio, Daniele <daniele.ceraolospurio at 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 at intel.com>
>>>> Cc: Huang Sean Z <sean.z.huang at intel.com>
>>>> Cc: Gaurav Kumar <kumar.gaurav at intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
>>>> Signed-off-by: Daniele Ceraolo Spurio
>>>> <daniele.ceraolospurio at 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
More information about the Intel-gfx
mailing list