diff mbox series

[v8,5/8] drm/i915: Add dedicated plane hook for async flip case

Message ID 20200914055633.21109-6-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
This hook is added to avoid writing other plane registers in case of
async flips, so that we do not write the double buffered registers
during async surface address update.

v7: -Plane ctl needs bits from skl_plane_ctl_crtc as well. (Ville)
    -Add a vfunc for skl_program_async_surface_address
     and call it from intel_update_plane. (Ville)

v8: -Rebased.

Signed-off-by: Karthik B S <karthik.b.s@intel.com>
Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
---
 .../gpu/drm/i915/display/intel_atomic_plane.c |  7 ++++++
 .../drm/i915/display/intel_display_types.h    |  3 +++
 drivers/gpu/drm/i915/display/intel_sprite.c   | 24 +++++++++++++++++++
 3 files changed, 34 insertions(+)

Comments

Ville Syrjala Sept. 15, 2020, 2:10 p.m. UTC | #1
On Mon, Sep 14, 2020 at 11:26:30AM +0530, Karthik B S wrote:
> This hook is added to avoid writing other plane registers in case of
> async flips, so that we do not write the double buffered registers
> during async surface address update.
> 
> v7: -Plane ctl needs bits from skl_plane_ctl_crtc as well. (Ville)
>     -Add a vfunc for skl_program_async_surface_address
>      and call it from intel_update_plane. (Ville)
> 
> v8: -Rebased.
> 
> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_atomic_plane.c |  7 ++++++
>  .../drm/i915/display/intel_display_types.h    |  3 +++
>  drivers/gpu/drm/i915/display/intel_sprite.c   | 24 +++++++++++++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 79032701873a..fdc633020255 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -408,6 +408,13 @@ void intel_update_plane(struct intel_plane *plane,
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  
>  	trace_intel_update_plane(&plane->base, crtc);
> +
> +	if (crtc_state->uapi.async_flip) {
> +		plane->program_async_surface_address(plane,
> +						     crtc_state, plane_state);
> +		return;
> +	}

if
	.async_flip()
else
	.update_plane()

should do

> +
>  	plane->update_plane(plane, crtc_state, plane_state);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index b2d0edacc58c..d2ae781e4d81 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1190,6 +1190,9 @@ struct intel_plane {
>  			   struct intel_plane_state *plane_state);
>  	int (*min_cdclk)(const struct intel_crtc_state *crtc_state,
>  			 const struct intel_plane_state *plane_state);
> +	void (*program_async_surface_address)(struct intel_plane *plane,

That's a mouthful. I'd simplify it to eg. just .async_flip().

> +					      const struct intel_crtc_state *crtc_state,
> +					      const struct intel_plane_state *plane_state);
>  };
>  
>  struct intel_watermark_params {
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index f0c89418d2e1..69407dfcebf6 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -609,6 +609,29 @@ 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 intel_plane *plane,
> +				  const struct intel_crtc_state *crtc_state,
> +				  const struct intel_plane_state *plane_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	unsigned long irqflags;
> +	enum plane_id plane_id = plane->id;
> +	enum pipe pipe = plane->pipe;
> +	u32 surf_addr = plane_state->color_plane[0].offset;
> +	u32 plane_ctl = plane_state->ctl;
> +
> +	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> +
> +	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,
> @@ -3096,6 +3119,7 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  	plane->get_hw_state = skl_plane_get_hw_state;
>  	plane->check_plane = skl_plane_check;
>  	plane->min_cdclk = skl_plane_min_cdclk;
> +	plane->program_async_surface_address = skl_program_async_surface_address;
>  
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		formats = icl_get_plane_formats(dev_priv, pipe,
> -- 
> 2.22.0
Ville Syrjala Sept. 15, 2020, 2:41 p.m. UTC | #2
On Mon, Sep 14, 2020 at 11:26:30AM +0530, Karthik B S wrote:
> This hook is added to avoid writing other plane registers in case of
> async flips, so that we do not write the double buffered registers
> during async surface address update.
> 
> v7: -Plane ctl needs bits from skl_plane_ctl_crtc as well. (Ville)
>     -Add a vfunc for skl_program_async_surface_address
>      and call it from intel_update_plane. (Ville)
> 
> v8: -Rebased.
> 
> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_atomic_plane.c |  7 ++++++
>  .../drm/i915/display/intel_display_types.h    |  3 +++
>  drivers/gpu/drm/i915/display/intel_sprite.c   | 24 +++++++++++++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 79032701873a..fdc633020255 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -408,6 +408,13 @@ void intel_update_plane(struct intel_plane *plane,
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  
>  	trace_intel_update_plane(&plane->base, crtc);
> +
> +	if (crtc_state->uapi.async_flip) {

Hmm. Now I'm starting to wonder how this is actually going to interact
with legacy cursor updates. The crtc_state we use there I think comes
from the previous update and so will have this flag set it if was an
async flip. Which means the cursor ioctl will oops.

We may want the igt to check this particular combination of ioctls
actually.

> +		plane->program_async_surface_address(plane,
> +						     crtc_state, plane_state);
> +		return;
> +	}
> +
>  	plane->update_plane(plane, crtc_state, plane_state);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index b2d0edacc58c..d2ae781e4d81 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1190,6 +1190,9 @@ struct intel_plane {
>  			   struct intel_plane_state *plane_state);
>  	int (*min_cdclk)(const struct intel_crtc_state *crtc_state,
>  			 const struct intel_plane_state *plane_state);
> +	void (*program_async_surface_address)(struct intel_plane *plane,
> +					      const struct intel_crtc_state *crtc_state,
> +					      const struct intel_plane_state *plane_state);
>  };
>  
>  struct intel_watermark_params {
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index f0c89418d2e1..69407dfcebf6 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -609,6 +609,29 @@ 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 intel_plane *plane,
> +				  const struct intel_crtc_state *crtc_state,
> +				  const struct intel_plane_state *plane_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	unsigned long irqflags;
> +	enum plane_id plane_id = plane->id;
> +	enum pipe pipe = plane->pipe;
> +	u32 surf_addr = plane_state->color_plane[0].offset;
> +	u32 plane_ctl = plane_state->ctl;
> +
> +	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> +
> +	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,
> @@ -3096,6 +3119,7 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  	plane->get_hw_state = skl_plane_get_hw_state;
>  	plane->check_plane = skl_plane_check;
>  	plane->min_cdclk = skl_plane_min_cdclk;
> +	plane->program_async_surface_address = skl_program_async_surface_address;
>  
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		formats = icl_get_plane_formats(dev_priv, pipe,
> -- 
> 2.22.0
Karthik B S Sept. 16, 2020, 12:48 p.m. UTC | #3
On 9/15/2020 7:40 PM, Ville Syrjälä wrote:
> On Mon, Sep 14, 2020 at 11:26:30AM +0530, Karthik B S wrote:
>> This hook is added to avoid writing other plane registers in case of
>> async flips, so that we do not write the double buffered registers
>> during async surface address update.
>>
>> v7: -Plane ctl needs bits from skl_plane_ctl_crtc as well. (Ville)
>>      -Add a vfunc for skl_program_async_surface_address
>>       and call it from intel_update_plane. (Ville)
>>
>> v8: -Rebased.
>>
>> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
>> ---
>>   .../gpu/drm/i915/display/intel_atomic_plane.c |  7 ++++++
>>   .../drm/i915/display/intel_display_types.h    |  3 +++
>>   drivers/gpu/drm/i915/display/intel_sprite.c   | 24 +++++++++++++++++++
>>   3 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>> index 79032701873a..fdc633020255 100644
>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>> @@ -408,6 +408,13 @@ void intel_update_plane(struct intel_plane *plane,
>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>   
>>   	trace_intel_update_plane(&plane->base, crtc);
>> +
>> +	if (crtc_state->uapi.async_flip) {
>> +		plane->program_async_surface_address(plane,
>> +						     crtc_state, plane_state);
>> +		return;
>> +	}
> 
> if
> 	.async_flip()
> else
> 	.update_plane()
> 
> should do
> 

Thanks for the review.
Sure, I'll update this.
>> +
>>   	plane->update_plane(plane, crtc_state, plane_state);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index b2d0edacc58c..d2ae781e4d81 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1190,6 +1190,9 @@ struct intel_plane {
>>   			   struct intel_plane_state *plane_state);
>>   	int (*min_cdclk)(const struct intel_crtc_state *crtc_state,
>>   			 const struct intel_plane_state *plane_state);
>> +	void (*program_async_surface_address)(struct intel_plane *plane,
> 
> That's a mouthful. I'd simplify it to eg. just .async_flip().
> 

Sure, I'll change the name.

Thanks,
Karthik.B.S
>> +					      const struct intel_crtc_state *crtc_state,
>> +					      const struct intel_plane_state *plane_state);
>>   };
>>   
>>   struct intel_watermark_params {
>> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
>> index f0c89418d2e1..69407dfcebf6 100644
>> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
>> @@ -609,6 +609,29 @@ 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 intel_plane *plane,
>> +				  const struct intel_crtc_state *crtc_state,
>> +				  const struct intel_plane_state *plane_state)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>> +	unsigned long irqflags;
>> +	enum plane_id plane_id = plane->id;
>> +	enum pipe pipe = plane->pipe;
>> +	u32 surf_addr = plane_state->color_plane[0].offset;
>> +	u32 plane_ctl = plane_state->ctl;
>> +
>> +	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
>> +
>> +	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,
>> @@ -3096,6 +3119,7 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>>   	plane->get_hw_state = skl_plane_get_hw_state;
>>   	plane->check_plane = skl_plane_check;
>>   	plane->min_cdclk = skl_plane_min_cdclk;
>> +	plane->program_async_surface_address = skl_program_async_surface_address;
>>   
>>   	if (INTEL_GEN(dev_priv) >= 11)
>>   		formats = icl_get_plane_formats(dev_priv, pipe,
>> -- 
>> 2.22.0
>
Karthik B S Sept. 16, 2020, 1 p.m. UTC | #4
On 9/15/2020 8:11 PM, Ville Syrjälä wrote:
> On Mon, Sep 14, 2020 at 11:26:30AM +0530, Karthik B S wrote:
>> This hook is added to avoid writing other plane registers in case of
>> async flips, so that we do not write the double buffered registers
>> during async surface address update.
>>
>> v7: -Plane ctl needs bits from skl_plane_ctl_crtc as well. (Ville)
>>      -Add a vfunc for skl_program_async_surface_address
>>       and call it from intel_update_plane. (Ville)
>>
>> v8: -Rebased.
>>
>> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
>> ---
>>   .../gpu/drm/i915/display/intel_atomic_plane.c |  7 ++++++
>>   .../drm/i915/display/intel_display_types.h    |  3 +++
>>   drivers/gpu/drm/i915/display/intel_sprite.c   | 24 +++++++++++++++++++
>>   3 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>> index 79032701873a..fdc633020255 100644
>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>> @@ -408,6 +408,13 @@ void intel_update_plane(struct intel_plane *plane,
>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>   
>>   	trace_intel_update_plane(&plane->base, crtc);
>> +
>> +	if (crtc_state->uapi.async_flip) {
> 
> Hmm. Now I'm starting to wonder how this is actually going to interact
> with legacy cursor updates. The crtc_state we use there I think comes
> from the previous update and so will have this flag set it if was an
> async flip. Which means the cursor ioctl will oops.
> 
> We may want the igt to check this particular combination of ioctls
> actually.
> 

I tried this out locally by using the DRM_IOCTL_MODE_CURSOR ioctl after 
an async flip. And looks like its working fine. During the cursor commit 
it actually takes the 'else' path.

I'll send out the new version of the IGT shortly with this subtest 
added. Please let me know if I'm missing something there.

Thanks,
Karthik.B.S
>> +		plane->program_async_surface_address(plane,
>> +						     crtc_state, plane_state);
>> +		return;
>> +	}
>> +
>>   	plane->update_plane(plane, crtc_state, plane_state);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index b2d0edacc58c..d2ae781e4d81 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1190,6 +1190,9 @@ struct intel_plane {
>>   			   struct intel_plane_state *plane_state);
>>   	int (*min_cdclk)(const struct intel_crtc_state *crtc_state,
>>   			 const struct intel_plane_state *plane_state);
>> +	void (*program_async_surface_address)(struct intel_plane *plane,
>> +					      const struct intel_crtc_state *crtc_state,
>> +					      const struct intel_plane_state *plane_state);
>>   };
>>   
>>   struct intel_watermark_params {
>> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
>> index f0c89418d2e1..69407dfcebf6 100644
>> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
>> @@ -609,6 +609,29 @@ 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 intel_plane *plane,
>> +				  const struct intel_crtc_state *crtc_state,
>> +				  const struct intel_plane_state *plane_state)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>> +	unsigned long irqflags;
>> +	enum plane_id plane_id = plane->id;
>> +	enum pipe pipe = plane->pipe;
>> +	u32 surf_addr = plane_state->color_plane[0].offset;
>> +	u32 plane_ctl = plane_state->ctl;
>> +
>> +	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
>> +
>> +	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,
>> @@ -3096,6 +3119,7 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>>   	plane->get_hw_state = skl_plane_get_hw_state;
>>   	plane->check_plane = skl_plane_check;
>>   	plane->min_cdclk = skl_plane_min_cdclk;
>> +	plane->program_async_surface_address = skl_program_async_surface_address;
>>   
>>   	if (INTEL_GEN(dev_priv) >= 11)
>>   		formats = icl_get_plane_formats(dev_priv, pipe,
>> -- 
>> 2.22.0
>
Karthik B S Sept. 16, 2020, 3:52 p.m. UTC | #5
On 9/16/2020 6:30 PM, Karthik B S wrote:
> 
> 
> On 9/15/2020 8:11 PM, Ville Syrjälä wrote:
>> On Mon, Sep 14, 2020 at 11:26:30AM +0530, Karthik B S wrote:
>>> This hook is added to avoid writing other plane registers in case of
>>> async flips, so that we do not write the double buffered registers
>>> during async surface address update.
>>>
>>> v7: -Plane ctl needs bits from skl_plane_ctl_crtc as well. (Ville)
>>>      -Add a vfunc for skl_program_async_surface_address
>>>       and call it from intel_update_plane. (Ville)
>>>
>>> v8: -Rebased.
>>>
>>> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
>>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
>>> ---
>>>   .../gpu/drm/i915/display/intel_atomic_plane.c |  7 ++++++
>>>   .../drm/i915/display/intel_display_types.h    |  3 +++
>>>   drivers/gpu/drm/i915/display/intel_sprite.c   | 24 +++++++++++++++++++
>>>   3 files changed, 34 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c 
>>> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>> index 79032701873a..fdc633020255 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>> @@ -408,6 +408,13 @@ void intel_update_plane(struct intel_plane *plane,
>>>       struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>>       trace_intel_update_plane(&plane->base, crtc);
>>> +
>>> +    if (crtc_state->uapi.async_flip) {
>>
>> Hmm. Now I'm starting to wonder how this is actually going to interact
>> with legacy cursor updates. The crtc_state we use there I think comes
>> from the previous update and so will have this flag set it if was an
>> async flip. Which means the cursor ioctl will oops.
>>
>> We may want the igt to check this particular combination of ioctls
>> actually.
>>
> 
> I tried this out locally by using the DRM_IOCTL_MODE_CURSOR ioctl after 
> an async flip. And looks like its working fine. During the cursor commit 
> it actually takes the 'else' path.
> 
> I'll send out the new version of the IGT shortly with this subtest 
> added. Please let me know if I'm missing something there.
> 

I've pushed the IGT with the cursor subtest added.
https://patchwork.freedesktop.org/series/79701/

Thanks,
Karthik.B.S
> Thanks,
> Karthik.B.S
>>> +        plane->program_async_surface_address(plane,
>>> +                             crtc_state, plane_state);
>>> +        return;
>>> +    }
>>> +
>>>       plane->update_plane(plane, crtc_state, plane_state);
>>>   }
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h 
>>> b/drivers/gpu/drm/i915/display/intel_display_types.h
>>> index b2d0edacc58c..d2ae781e4d81 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>>> @@ -1190,6 +1190,9 @@ struct intel_plane {
>>>                  struct intel_plane_state *plane_state);
>>>       int (*min_cdclk)(const struct intel_crtc_state *crtc_state,
>>>                const struct intel_plane_state *plane_state);
>>> +    void (*program_async_surface_address)(struct intel_plane *plane,
>>> +                          const struct intel_crtc_state *crtc_state,
>>> +                          const struct intel_plane_state *plane_state);
>>>   };
>>>   struct intel_watermark_params {
>>> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c 
>>> b/drivers/gpu/drm/i915/display/intel_sprite.c
>>> index f0c89418d2e1..69407dfcebf6 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
>>> @@ -609,6 +609,29 @@ 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 intel_plane *plane,
>>> +                  const struct intel_crtc_state *crtc_state,
>>> +                  const struct intel_plane_state *plane_state)
>>> +{
>>> +    struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>> +    unsigned long irqflags;
>>> +    enum plane_id plane_id = plane->id;
>>> +    enum pipe pipe = plane->pipe;
>>> +    u32 surf_addr = plane_state->color_plane[0].offset;
>>> +    u32 plane_ctl = plane_state->ctl;
>>> +
>>> +    plane_ctl |= skl_plane_ctl_crtc(crtc_state);
>>> +
>>> +    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,
>>> @@ -3096,6 +3119,7 @@ skl_universal_plane_create(struct 
>>> drm_i915_private *dev_priv,
>>>       plane->get_hw_state = skl_plane_get_hw_state;
>>>       plane->check_plane = skl_plane_check;
>>>       plane->min_cdclk = skl_plane_min_cdclk;
>>> +    plane->program_async_surface_address = 
>>> skl_program_async_surface_address;
>>>       if (INTEL_GEN(dev_priv) >= 11)
>>>           formats = icl_get_plane_formats(dev_priv, pipe,
>>> -- 
>>> 2.22.0
>>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index 79032701873a..fdc633020255 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -408,6 +408,13 @@  void intel_update_plane(struct intel_plane *plane,
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 
 	trace_intel_update_plane(&plane->base, crtc);
+
+	if (crtc_state->uapi.async_flip) {
+		plane->program_async_surface_address(plane,
+						     crtc_state, plane_state);
+		return;
+	}
+
 	plane->update_plane(plane, crtc_state, plane_state);
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index b2d0edacc58c..d2ae781e4d81 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1190,6 +1190,9 @@  struct intel_plane {
 			   struct intel_plane_state *plane_state);
 	int (*min_cdclk)(const struct intel_crtc_state *crtc_state,
 			 const struct intel_plane_state *plane_state);
+	void (*program_async_surface_address)(struct intel_plane *plane,
+					      const struct intel_crtc_state *crtc_state,
+					      const struct intel_plane_state *plane_state);
 };
 
 struct intel_watermark_params {
diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index f0c89418d2e1..69407dfcebf6 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -609,6 +609,29 @@  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 intel_plane *plane,
+				  const struct intel_crtc_state *crtc_state,
+				  const struct intel_plane_state *plane_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	unsigned long irqflags;
+	enum plane_id plane_id = plane->id;
+	enum pipe pipe = plane->pipe;
+	u32 surf_addr = plane_state->color_plane[0].offset;
+	u32 plane_ctl = plane_state->ctl;
+
+	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
+
+	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,
@@ -3096,6 +3119,7 @@  skl_universal_plane_create(struct drm_i915_private *dev_priv,
 	plane->get_hw_state = skl_plane_get_hw_state;
 	plane->check_plane = skl_plane_check;
 	plane->min_cdclk = skl_plane_min_cdclk;
+	plane->program_async_surface_address = skl_program_async_surface_address;
 
 	if (INTEL_GEN(dev_priv) >= 11)
 		formats = icl_get_plane_formats(dev_priv, pipe,