On Mon, Oct 18, 2021 at 02:50:23PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> We're currently forgetting to set the PLANE_SURF_DECRYPT
> flag in the async flip path. So if the hardware were to
> latch that bit despite this being an async flip we'd start
> scanning out garbage. And if it doesn't latch it then I
> guess we'd just end up with a weird register value that
> doesn't actually match the hardware state, which isn't
> great for anyone starting at register dumps.
>
> Similarly the async flip path also forgets to call
> skl_surf_address() which means the DPT address space to
> GGTT address space downshift is not being applied to
> the offset. Which means we are pointing PLANE_SURF
> at some random location in GGTT instead of the correct
> DPT page.
>
> So let's fix two birds with one stone and extract the
> PLANE_SURF calculation from skl_program_plane() into
> a small helper and use it in the async flip path as well.
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
>
> 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>
> Cc: Karthik B S <karthik.b.s@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> .../drm/i915/display/skl_universal_plane.c | 30 ++++++++++++-------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 7444b88829ea..e2f024449149 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1011,6 +1011,20 @@ static u32 skl_surf_address(const struct intel_plane_state *plane_state,
> }
> }
>
> +static u32 skl_plane_surf(const struct intel_plane_state *plane_state,
> + int color_plane)
> +{
> + u32 plane_surf;
> +
> + plane_surf = intel_plane_ggtt_offset(plane_state) +
> + skl_surf_address(plane_state, color_plane);
> +
> + if (plane_state->decrypt)
> + plane_surf |= PLANE_SURF_DECRYPT;
> +
> + return plane_surf;
> +}
> +
> static void icl_plane_csc_load_black(struct intel_plane *plane)
> {
> struct drm_i915_private *i915 = to_i915(plane->base.dev);
> @@ -1045,7 +1059,6 @@ skl_program_plane(struct intel_plane *plane,
> enum plane_id plane_id = plane->id;
> enum pipe pipe = plane->pipe;
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> - u32 surf_addr = skl_surf_address(plane_state, color_plane);
> u32 stride = skl_plane_stride(plane_state, color_plane);
> const struct drm_framebuffer *fb = plane_state->hw.fb;
> int aux_plane = skl_main_to_aux_plane(fb, color_plane);
> @@ -1058,7 +1071,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, plane_surf;
> + u32 keymsk, keymax;
> u32 plane_ctl = plane_state->ctl;
>
> plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> @@ -1084,16 +1097,13 @@ skl_program_plane(struct intel_plane *plane,
> }
>
> if (aux_plane) {
> - aux_dist = skl_surf_address(plane_state, aux_plane) - surf_addr;
> + aux_dist = skl_surf_address(plane_state, aux_plane) -
> + skl_surf_address(plane_state, color_plane);
>
> if (DISPLAY_VER(dev_priv) < 12)
> aux_dist |= skl_plane_stride(plane_state, aux_plane);
> }
>
> - plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
> - if (plane_state->decrypt)
> - plane_surf |= PLANE_SURF_DECRYPT;
> -
> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>
> /*
> @@ -1157,7 +1167,8 @@ 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), plane_surf);
> + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> + skl_plane_surf(plane_state, color_plane));
>
> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }
> @@ -1172,7 +1183,6 @@ skl_plane_async_flip(struct intel_plane *plane,
> unsigned long irqflags;
> enum plane_id plane_id = plane->id;
> enum pipe pipe = plane->pipe;
> - u32 surf_addr = plane_state->view.color_plane[0].offset;
> u32 plane_ctl = plane_state->ctl;
>
> plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> @@ -1184,7 +1194,7 @@ skl_plane_async_flip(struct intel_plane *plane,
>
> 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);
> + skl_plane_surf(plane_state, 0));
>
> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }
> --
> 2.32.0
>
