diff mbox series

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

Message ID 20200720113117.16131-3-karthik.b.s@intel.com (mailing list archive)
State New, archived
Headers show
Series Asynchronous flip implementation for i915 | expand

Commit Message

Karthik B S July 20, 2020, 11:31 a.m. UTC 
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(+)

Comments

Zanoni, Paulo R July 24, 2020, 11:26 p.m. UTC | #1 
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 */
Karthik B S July 28, 2020, 7:37 a.m. UTC | #2 
On 7/25/2020 4:56 AM, Paulo Zanoni wrote:
> 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.
> 

Thanks for the review.
Yes the check for other updates are done before.

So I could either do read-modify-write and return early, or,
keep the existing code flow as is, since the are checks already present.

I will keep the existing flow and remove the early return in the next 
revision.
> 
>> +	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.

Will initialize the plane_ctl variable to zero.
> 
> 
>> +		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.
> 

Since we only allow async flips on planes that are already enabled,
I assumed this would not be needed. Also, other than bit 9 (async 
address update enable), this register is double buffered and cannot be 
updated asynchronously.
> 
>>   
>>   	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.
> 
>

Will move this change to the end of the series.

Thanks,
Karthik.B.S

>> +
>>   	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 */
>
diff mbox series

Patch

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 */
+	if (crtc_state->uapi.async_flip) {
+		plane_ctl |= PLANE_CTL_ASYNC_FLIP;
+		return plane_ctl;
+	}
+
 	plane_ctl = PLANE_CTL_ENABLE;
 
 	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;
+	}
+
 	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 */