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.
> 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.
Reviewed-by: Juston Li <juston.li@xxxxxxxxx>
> 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;
