Re: [Intel-gfx] [PATCH 2/9] drm/i915: Fix async flip with decryption and/or DPT

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Mon, Oct 18, 2021 at 02:50:23PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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@intel.com>

> 
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Juston Li <juston.li@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Karthik B S <karthik.b.s@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../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
>