On Thu, Oct 07, 2021 at 05:52:47AM +0000, Li, Juston wrote:
> On Thu, 2021-10-07 at 02:57 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > No real reason to have this pxp state computation in
> > intel_atomic_check_planes(). Just stuff it into skl_plane_check().
> >
> > There was also some funny state copying being done from the
> > old plane state to the new plane state when the plane is anyway
> > disabled.
> >
> > The one thing we presumably must remember to do is copy
> > over the decrypt state when assigning a Y plane for planar
> > YCbCr scanout, so that the Y plane's PLANE_SURF will get the
> > appropriate bit set. The force_black thing should not matter
> > as I'm pretty sure all that stuff is ignored for the Y plane.
Hmm. Might even want to just clear force_black explicitly since
then we won't waste time writing the plane CSC registers if the flag
was previously set when the Y plane was used as an independent plane.
> > I suppose this was the reason for the odd placement for the
> > state computation, but I see no reason to deviate from the
> > standard way of doing these things. This also guarantees
> > that we don't calculate things differently between the
> > linked UV and Y plane.
> >
> > Cc: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Juston Li <juston.li@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 31 +----------------
> > --
> > .../drm/i915/display/skl_universal_plane.c | 15 +++++++++
> > 2 files changed, 16 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 4f0badb11bbb..bb45872cb619 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -71,8 +71,6 @@
> > #include "gt/intel_rps.h"
> > #include "gt/gen8_ppgtt.h"
> >
> > -#include "pxp/intel_pxp.h"
> > -
> > #include "g4x_dp.h"
> > #include "g4x_hdmi.h"
> > #include "i915_drv.h"
> > @@ -6662,6 +6660,7 @@ static int icl_check_nv12_planes(struct
> > intel_crtc_state *crtc_state)
> > linked_state->ctl = plane_state->ctl |
> > PLANE_CTL_YUV420_Y_PLANE;
> > linked_state->color_ctl = plane_state->color_ctl;
> > linked_state->view = plane_state->view;
> > + linked_state->decrypt = plane_state->decrypt;
>
> Ahh this was what we were missing before.
>
> The computation was originally in skl_plane_check() but to get it
> working I moved it to intel_atomic_check_planes() after
> icl_check_nv12_planes() so that it would set that plane's decrypt flag
> as well, not knowing I could just do this.
Right. Figured that was probably the reason.
>
> Reviewed-by: Juston Li <juston.li@xxxxxxxxx>
Ta.
>
> > intel_plane_copy_hw_state(linked_state, plane_state);
> > linked_state->uapi.src = plane_state->uapi.src;
> > @@ -9024,28 +9023,13 @@ static int
> > intel_bigjoiner_add_affected_planes(struct intel_atomic_state *state)
> > return 0;
> > }
> >
> > -static bool bo_has_valid_encryption(struct drm_i915_gem_object *obj)
> > -{
> > - struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > -
> > - return intel_pxp_key_check(&i915->gt.pxp, obj, false) == 0;
> > -}
> > -
> > -static bool pxp_is_borked(struct drm_i915_gem_object *obj)
> > -{
> > - return i915_gem_object_is_protected(obj) &&
> > !bo_has_valid_encryption(obj);
> > -}
> > -
> > static int intel_atomic_check_planes(struct intel_atomic_state *state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> > struct intel_plane_state *plane_state;
> > struct intel_plane *plane;
> > - struct intel_plane_state *new_plane_state;
> > - struct intel_plane_state *old_plane_state;
> > struct intel_crtc *crtc;
> > - const struct drm_framebuffer *fb;
> > int i, ret;
> >
> > ret = icl_add_linked_planes(state);
> > @@ -9093,19 +9077,6 @@ static int intel_atomic_check_planes(struct
> > intel_atomic_state *state)
> > return ret;
> > }
> >
> > - for_each_new_intel_plane_in_state(state, plane, plane_state, i)
> > {
> > - new_plane_state =
> > intel_atomic_get_new_plane_state(state, plane);
> > - old_plane_state =
> > intel_atomic_get_old_plane_state(state, plane);
> > - fb = new_plane_state->hw.fb;
> > - if (fb) {
> > - new_plane_state->decrypt =
> > bo_has_valid_encryption(intel_fb_obj(fb));
> > - new_plane_state->force_black =
> > pxp_is_borked(intel_fb_obj(fb));
> > - } else {
> > - new_plane_state->decrypt = old_plane_state-
> > >decrypt;
> > - new_plane_state->force_black = old_plane_state-
> > >force_black;
> > - }
> > - }
> > -
> > return 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index a0e53a3b267a..1fcb41942c7e 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1737,6 +1737,18 @@ static bool skl_fb_scalable(const struct
> > drm_framebuffer *fb)
> > }
> > }
> >
> > +static bool bo_has_valid_encryption(struct drm_i915_gem_object *obj)
> > +{
> > + struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > +
> > + return intel_pxp_key_check(&i915->gt.pxp, obj, false) == 0;
> > +}
> > +
> > +static bool pxp_is_borked(struct drm_i915_gem_object *obj)
> > +{
> > + return i915_gem_object_is_protected(obj) &&
> > !bo_has_valid_encryption(obj);
> > +}
> > +
> > static int skl_plane_check(struct intel_crtc_state *crtc_state,
> > struct intel_plane_state *plane_state)
> > {
> > @@ -1781,6 +1793,9 @@ static int skl_plane_check(struct
> > intel_crtc_state *crtc_state,
> > if (ret)
> > return ret;
> >
> > + plane_state->decrypt =
> > bo_has_valid_encryption(intel_fb_obj(fb));
> > + plane_state->force_black = pxp_is_borked(intel_fb_obj(fb));
> > +
> > /* HW only has 8 bits pixel precision, disable plane if
> > invisible */
> > if (!(plane_state->hw.alpha >> 8))
> > plane_state->uapi.visible = false;
>
--
Ville Syrjälä
Intel
