diff mbox series

[v8,6/8] drm/i915: WA for platforms with double buffered adress update enable bit

Message ID 20200914055633.21109-7-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 Sept. 14, 2020, 5:56 a.m. UTC
In Gen 9 and Gen 10 platforms, async address update enable bit is
double buffered. Due to this, during the transition from async flip
to sync flip we have to wait until this bit is updated before continuing
with the normal commit for sync flip.

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 | 44 ++++++++++++++++++++
 1 file changed, 44 insertions(+)

Comments

Ville Syrjala Sept. 15, 2020, 2:19 p.m. UTC | #1
On Mon, Sep 14, 2020 at 11:26:31AM +0530, Karthik B S wrote:
> In Gen 9 and Gen 10 platforms, async address update enable bit is
> double buffered. Due to this, during the transition from async flip
> to sync flip we have to wait until this bit is updated before continuing
> with the normal commit for sync flip.
> 
> 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 | 44 ++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a0c17d94daf3..b7e24dff0772 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -15360,6 +15360,42 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
>  	intel_crtc_enable_pipe_crc(crtc);
>  }
>  
> +static void skl_toggle_async_sync(struct intel_atomic_state *state,

skl_disable_async_flip_wa() maybe?

> +				  struct intel_crtc *crtc,
> +				  struct intel_crtc_state *new_crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct intel_plane *plane;
> +	struct intel_plane_state *new_plane_state;
> +	u32 update_mask = new_crtc_state->update_planes;
> +	u32 plane_ctl, surf_addr;
> +	enum plane_id plane_id;
> +	unsigned long irqflags;
> +	enum pipe pipe;

Most of these things are only needed within the loop, so that's where
the declarations should be.

> +	int i;
> +
> +	for_each_new_intel_plane_in_state(state, plane, new_plane_state, i) {
> +		if (crtc->pipe != plane->pipe ||
> +		    !(update_mask & BIT(plane->id)))
> +			continue;
> +
> +		plane_id = plane->id;
> +		pipe = plane->pipe;
> +

I'd take the lock here so the rmw is fully protected.

> +		plane_ctl = intel_de_read_fw(dev_priv, PLANE_CTL(pipe, plane_id));
> +		surf_addr = intel_de_read_fw(dev_priv, PLANE_SURF(pipe, plane_id));
> +
> +		plane_ctl &= ~PLANE_CTL_ASYNC_FLIP;
> +
> +		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), surf_addr);
> +		spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +	}
> +
> +	intel_wait_for_vblank(dev_priv, crtc->pipe);
> +}
> +
>  static void intel_update_crtc(struct intel_atomic_state *state,
>  			      struct intel_crtc *crtc)
>  {
> @@ -15387,6 +15423,14 @@ static void intel_update_crtc(struct intel_atomic_state *state,
>  	else
>  		intel_fbc_enable(state, crtc);
>  
> +	/* WA for older platforms where async address update enable bit

s/older//

> +	 * is double buffered.

"is double buffered and only latched at start of vblank" or
something. Otherwise one is left wondering what the fuss is about.

> +	 */

Multiline comment format should be
/*
 * blah
 * blah
 */

> +	if (old_crtc_state->uapi.async_flip &&
> +	    !new_crtc_state->uapi.async_flip &&
> +	    INTEL_GEN(dev_priv) <= 10 && INTEL_GEN(dev_priv) >= 9)

IS_GEN_RANGE(9, 10) or whatever it's called.

I guess we might want to put this call into intel_pre_plane_update()
since that's where all kinds of similar wait_for_vblank w/as live.

> +		skl_toggle_async_sync(state, crtc, new_crtc_state);
> +
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(new_crtc_state);
>  
> -- 
> 2.22.0
Karthik B S Sept. 16, 2020, 12:54 p.m. UTC | #2
On 9/15/2020 7:49 PM, Ville Syrjälä wrote:
> On Mon, Sep 14, 2020 at 11:26:31AM +0530, Karthik B S wrote:
>> In Gen 9 and Gen 10 platforms, async address update enable bit is
>> double buffered. Due to this, during the transition from async flip
>> to sync flip we have to wait until this bit is updated before continuing
>> with the normal commit for sync flip.
>>
>> 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 | 44 ++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index a0c17d94daf3..b7e24dff0772 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -15360,6 +15360,42 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
>>   	intel_crtc_enable_pipe_crc(crtc);
>>   }
>>   
>> +static void skl_toggle_async_sync(struct intel_atomic_state *state,
> 
> skl_disable_async_flip_wa() maybe?
> 

Thanks for the review.
I'll change the name.
>> +				  struct intel_crtc *crtc,
>> +				  struct intel_crtc_state *new_crtc_state)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> +	struct intel_plane *plane;
>> +	struct intel_plane_state *new_plane_state;
>> +	u32 update_mask = new_crtc_state->update_planes;
>> +	u32 plane_ctl, surf_addr;
>> +	enum plane_id plane_id;
>> +	unsigned long irqflags;
>> +	enum pipe pipe;
> 
> Most of these things are only needed within the loop, so that's where
> the declarations should be.
> 

Sure, I'll move it inside the loop.
>> +	int i;
>> +
>> +	for_each_new_intel_plane_in_state(state, plane, new_plane_state, i) {
>> +		if (crtc->pipe != plane->pipe ||
>> +		    !(update_mask & BIT(plane->id)))
>> +			continue;
>> +
>> +		plane_id = plane->id;
>> +		pipe = plane->pipe;
>> +
> 
> I'd take the lock here so the rmw is fully protected.
> 

Sure, I'll move it here.
>> +		plane_ctl = intel_de_read_fw(dev_priv, PLANE_CTL(pipe, plane_id));
>> +		surf_addr = intel_de_read_fw(dev_priv, PLANE_SURF(pipe, plane_id));
>> +
>> +		plane_ctl &= ~PLANE_CTL_ASYNC_FLIP;
>> +
>> +		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), surf_addr);
>> +		spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>> +	}
>> +
>> +	intel_wait_for_vblank(dev_priv, crtc->pipe);
>> +}
>> +
>>   static void intel_update_crtc(struct intel_atomic_state *state,
>>   			      struct intel_crtc *crtc)
>>   {
>> @@ -15387,6 +15423,14 @@ static void intel_update_crtc(struct intel_atomic_state *state,
>>   	else
>>   		intel_fbc_enable(state, crtc);
>>   
>> +	/* WA for older platforms where async address update enable bit
> 
> s/older//
> 

Noted.
>> +	 * is double buffered.
> 
> "is double buffered and only latched at start of vblank" or
> something. Otherwise one is left wondering what the fuss is about.
> 

Sure, I'll update this.
>> +	 */
> 
> Multiline comment format should be
> /*
>   * blah
>   * blah
>   */
> 

I'll fix this.
>> +	if (old_crtc_state->uapi.async_flip &&
>> +	    !new_crtc_state->uapi.async_flip &&
>> +	    INTEL_GEN(dev_priv) <= 10 && INTEL_GEN(dev_priv) >= 9)
> 
> IS_GEN_RANGE(9, 10) or whatever it's called.
> 

Sure, I'll update this.
> I guess we might want to put this call into intel_pre_plane_update()
> since that's where all kinds of similar wait_for_vblank w/as live.
> 

Sure, I'll move this to intel_pre_plane_update()

Thanks,
Karthik.B.S
>> +		skl_toggle_async_sync(state, crtc, new_crtc_state);
>> +
>>   	/* Perform vblank evasion around commit operation */
>>   	intel_pipe_update_start(new_crtc_state);
>>   
>> -- 
>> 2.22.0
>
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 a0c17d94daf3..b7e24dff0772 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -15360,6 +15360,42 @@  static void intel_enable_crtc(struct intel_atomic_state *state,
 	intel_crtc_enable_pipe_crc(crtc);
 }
 
+static void skl_toggle_async_sync(struct intel_atomic_state *state,
+				  struct intel_crtc *crtc,
+				  struct intel_crtc_state *new_crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct intel_plane *plane;
+	struct intel_plane_state *new_plane_state;
+	u32 update_mask = new_crtc_state->update_planes;
+	u32 plane_ctl, surf_addr;
+	enum plane_id plane_id;
+	unsigned long irqflags;
+	enum pipe pipe;
+	int i;
+
+	for_each_new_intel_plane_in_state(state, plane, new_plane_state, i) {
+		if (crtc->pipe != plane->pipe ||
+		    !(update_mask & BIT(plane->id)))
+			continue;
+
+		plane_id = plane->id;
+		pipe = plane->pipe;
+
+		plane_ctl = intel_de_read_fw(dev_priv, PLANE_CTL(pipe, plane_id));
+		surf_addr = intel_de_read_fw(dev_priv, PLANE_SURF(pipe, plane_id));
+
+		plane_ctl &= ~PLANE_CTL_ASYNC_FLIP;
+
+		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), surf_addr);
+		spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+	}
+
+	intel_wait_for_vblank(dev_priv, crtc->pipe);
+}
+
 static void intel_update_crtc(struct intel_atomic_state *state,
 			      struct intel_crtc *crtc)
 {
@@ -15387,6 +15423,14 @@  static void intel_update_crtc(struct intel_atomic_state *state,
 	else
 		intel_fbc_enable(state, crtc);
 
+	/* WA for older platforms where async address update enable bit
+	 * is double buffered.
+	 */
+	if (old_crtc_state->uapi.async_flip &&
+	    !new_crtc_state->uapi.async_flip &&
+	    INTEL_GEN(dev_priv) <= 10 && INTEL_GEN(dev_priv) >= 9)
+		skl_toggle_async_sync(state, crtc, new_crtc_state);
+
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(new_crtc_state);