Re: [PATCH v5 2/5] drm/i915: Add support for async flips in I915

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em seg, 2020-07-20 às 17:01 +0530, Karthik B S escreveu:
> Set the Async Address Update Enable bit in plane ctl
> when async flip is requested.
> 
> v2: -Move the Async flip enablement to individual patch (Paulo)
> 
> v3: -Rebased.
> 
> v4: -Add separate plane hook for async flip case (Ville)
> 
> v5: -Rebased.
> 
> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  6 +++++
>  drivers/gpu/drm/i915/display/intel_sprite.c  | 25 ++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h              |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index b8ff032195d9..4773f39e5924 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4766,6 +4766,12 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>  	u32 plane_ctl;
>  
> +	/* During Async flip, no other updates are allowed */

My understanding is that this function is fully setting the right bits
based on the chosen config (instead of doing read-modify-write), and
the checks for "other updates" were done before. So the logic
implemented here of early returning doesn't make sense.


> +	if (crtc_state->uapi.async_flip) {
> +		plane_ctl |= PLANE_CTL_ASYNC_FLIP;

I wonder why gcc does not complain we're ORing with an unitialized
value.


> +		return plane_ctl;
> +	}
> +
>  	plane_ctl = PLANE_CTL_ENABLE;

It seems to be the return above means we'll never even try to enable
the plane, we're only relying on the fact that plane_ctl is not zero
initialize so maybe  bit 31 is already set.


>  
>  	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) {
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index c26ca029fc0a..3747482e8fa3 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -603,6 +603,24 @@ icl_program_input_csc(struct intel_plane *plane,
>  			  PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0);
>  }
>  
> +static void
> +skl_program_async_surface_address(struct drm_i915_private *dev_priv,
> +				  const struct intel_plane_state *plane_state,
> +				  enum pipe pipe, enum plane_id plane_id,
> +				  u32 surf_addr)
> +{
> +	unsigned long irqflags;
> +	u32 plane_ctl = plane_state->ctl;
> +
> +	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +
> +	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);
> +
> +	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +}
> +
>  static void
>  skl_program_plane(struct intel_plane *plane,
>  		  const struct intel_crtc_state *crtc_state,
> @@ -631,6 +649,13 @@ skl_program_plane(struct intel_plane *plane,
>  	u32 keymsk, keymax;
>  	u32 plane_ctl = plane_state->ctl;
>  
> +	/* During Async flip, no other updates are allowed */
> +	if (crtc_state->uapi.async_flip) {
> +		skl_program_async_surface_address(dev_priv, plane_state,
> +						  pipe, plane_id, surf_addr);
> +		return;
> +	}


I'd vote for us to keep the "don't rewrite registers that shouldn't
change" part on its own commit, since it's just an optimization. It
could even go at the end of the series. But perhaps this is simple
enough and not needed.


> +
>  	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
>  
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8cee06314d5d..19aad4199874 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6935,6 +6935,7 @@ enum {
>  #define   PLANE_CTL_TILED_X			(1 << 10)
>  #define   PLANE_CTL_TILED_Y			(4 << 10)
>  #define   PLANE_CTL_TILED_YF			(5 << 10)
> +#define   PLANE_CTL_ASYNC_FLIP			(1 << 9)
>  #define   PLANE_CTL_FLIP_HORIZONTAL		(1 << 8)
>  #define   PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE	(1 << 4) /* TGL+ */
>  #define   PLANE_CTL_ALPHA_MASK			(0x3 << 4) /* Pre-GLK */

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel