[v8,1/2] drm/i915: Introduce async plane update to i915

Submitted by Helen Koike on March 7, 2019, 4:49 p.m.

Details

Message ID 20190307164948.12138-1-helen.koike@collabora.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Helen Koike March 7, 2019, 4:49 p.m.
From: Gustavo Padovan <gustavo.padovan@collabora.com>

Add implementation for async plane update callbacks

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
Signed-off-by: Helen Koike <helen.koike@collabora.com>

---
Hi,

This patch just adds the callbacks for atomic async update, the next in
the series will replace the legacy cursor implementation with these
callbacks.

I tested with igt (ksm_cursor_legacy and plane_cursor_legacy).

This patch depends on "[PATCH] drm: don't block fb changes for async plane updates",
otherwise there will be a regression on igt tests:

        cursor-vs-flip-atomic-transitions-varying-size
        cursor-vs-flip-toggle
        cursor-vs-flip-varying-size

with errors of type:

"CRITICAL: completed 97 cursor updated in a period of 30 flips, we
expect to complete approximately 15360 updates, with the threshold set
at 7680"

This happens because what should be async updates are being turned into
syncronous updates.

Thanks
Helen

Changes in v8:
 - v7: https://lkml.org/lkml/2018/6/8/168
 - v7 was splited in two, one that adds the async callbacks and another
 that updates the cursor.
 - rebase with drm-intel
 - allow async update in all types of planes, not only cursor
 - add watermark checks in async update
 - remove bypass of intel_prepare_plane_fb() in case of async update
 - add missing drm_atomic_helper_cleanup_planes(dev, state) call in
 intel_atomic_commit().
 - use swap() function in async update to set the old_fb in the
 new_state object.
 - use helpers intel_update_plane()/intel_disable_plane()

Changes in v7:
- Rebase on top of drm-intel repository. Hopefully now will play
  nicely with autobuilders.

Changes in v6:
- Rework the intel_plane_atomic_async_update due driver changed from
  last time.
- Removed the mutex_lock/unlock as causes a deadlock.

Changes in v5:
- Call drm_atomic_helper_async_check() from the check hook

Changes in v4:
- Set correct vma to new state for cleanup
- Move size checks back to drivers (Ville Syrjälä)

Changes in v3:
- Move fb setting to core and use new state (Eric Anholt)

 drivers/gpu/drm/i915/intel_atomic_plane.c | 71 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c      |  9 +++
 2 files changed, 80 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 7961cf0e6951..f516b227dba9 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -318,10 +318,81 @@  void i9xx_update_planes_on_crtc(struct intel_atomic_state *state,
 	}
 }
 
+static int intel_plane_atomic_async_check(struct drm_plane *plane,
+					  struct drm_plane_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+
+	crtc_state = drm_atomic_get_existing_crtc_state(state->state,
+							state->crtc);
+	if (WARN_ON(!crtc_state))
+		return -EINVAL;
+
+	/*
+	 * When crtc is inactive or there is a modeset pending,
+	 * wait for it to complete in the slowpath
+	 */
+	if (!crtc_state->active || to_intel_crtc_state(crtc_state)->update_pipe)
+		return -EINVAL;
+
+	/*
+	 * If any parameters change that may affect watermarks,
+	 * take the slowpath. Only changing fb or position should be
+	 * in the fastpath.
+	 */
+	if (plane->state->crtc != state->crtc ||
+	    plane->state->src_w != state->src_w ||
+	    plane->state->src_h != state->src_h ||
+	    plane->state->crtc_w != state->crtc_w ||
+	    plane->state->crtc_h != state->crtc_h ||
+	    !plane->state->fb != !state->fb)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void intel_plane_atomic_async_update(struct drm_plane *plane,
+					    struct drm_plane_state *new_state)
+{
+	struct intel_atomic_state *intel_new_state =
+		to_intel_atomic_state(new_state->state);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_crtc *crtc = plane->state->crtc;
+	struct intel_crtc_state *new_crtc_state;
+	struct intel_crtc *intel_crtc;
+	int i;
+
+	for_each_new_intel_crtc_in_state(intel_new_state, intel_crtc,
+					 new_crtc_state, i)
+		WARN_ON(new_crtc_state->wm.need_postvbl_update ||
+			new_crtc_state->update_wm_post);
+
+	i915_gem_track_fb(intel_fb_obj(plane->state->fb),
+			  intel_fb_obj(new_state->fb),
+			  intel_plane->frontbuffer_bit);
+
+	plane->state->src_x = new_state->src_x;
+	plane->state->src_y = new_state->src_y;
+	plane->state->crtc_x = new_state->crtc_x;
+	plane->state->crtc_y = new_state->crtc_y;
+
+	swap(plane->state->fb, new_state->fb);
+
+	if (plane->state->visible)
+		intel_update_plane(intel_plane,
+				   to_intel_crtc_state(crtc->state),
+				   to_intel_plane_state(plane->state));
+	else
+		intel_disable_plane(intel_plane,
+				    to_intel_crtc_state(crtc->state));
+}
+
 const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
 	.prepare_fb = intel_prepare_plane_fb,
 	.cleanup_fb = intel_cleanup_plane_fb,
 	.atomic_check = intel_plane_atomic_check,
+	.atomic_async_check = intel_plane_atomic_async_check,
+	.atomic_async_update = intel_plane_atomic_async_update,
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 415d8968f2c5..244e1c94277d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13464,6 +13464,15 @@  static int intel_atomic_commit(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	int ret = 0;
 
+	if (state->async_update) {
+		ret = drm_atomic_helper_prepare_planes(dev, state);
+		if (ret)
+			return ret;
+		drm_atomic_helper_async_commit(dev, state);
+		drm_atomic_helper_cleanup_planes(dev, state);
+		return 0;
+	}
+
 	drm_atomic_state_get(state);
 	i915_sw_fence_init(&intel_state->commit_ready,
 			   intel_atomic_commit_ready);

Comments

Tested-by: Tina Zhang <tina.zhang@intel.com>


> -----Original Message-----

> From: Helen Koike [mailto:helen.koike@collabora.com]

> Sent: Friday, March 8, 2019 12:50 AM

> To: dri-devel@lists.freedesktop.org

> Cc: tfiga@chromium.org; mcasas@google.com; Wang, Zhenyu Z

> <zhenyu.z.wang@intel.com>; daniel.vetter@ffwll.ch; Zhang, Tina

> <tina.zhang@intel.com>; kernel@collabora.com; ville.syrjala@linux.intel.com

> Subject: [PATCH v8 1/2] drm/i915: Introduce async plane update to i915

> 

> From: Gustavo Padovan <gustavo.padovan@collabora.com>

> 

> Add implementation for async plane update callbacks

> 

> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>

> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

> Signed-off-by: Tina Zhang <tina.zhang@intel.com>

> Signed-off-by: Helen Koike <helen.koike@collabora.com>

> 

> ---

> Hi,

> 

> This patch just adds the callbacks for atomic async update, the next in the series

> will replace the legacy cursor implementation with these callbacks.

> 

> I tested with igt (ksm_cursor_legacy and plane_cursor_legacy).

> 

> This patch depends on "[PATCH] drm: don't block fb changes for async plane

> updates", otherwise there will be a regression on igt tests:

> 

>         cursor-vs-flip-atomic-transitions-varying-size

>         cursor-vs-flip-toggle

>         cursor-vs-flip-varying-size

> 

> with errors of type:

> 

> "CRITICAL: completed 97 cursor updated in a period of 30 flips, we expect to

> complete approximately 15360 updates, with the threshold set at 7680"

> 

> This happens because what should be async updates are being turned into

> syncronous updates.

> 

> Thanks

> Helen

> 

> Changes in v8:

>  - v7: https://lkml.org/lkml/2018/6/8/168

>  - v7 was splited in two, one that adds the async callbacks and another  that

> updates the cursor.

>  - rebase with drm-intel

>  - allow async update in all types of planes, not only cursor

>  - add watermark checks in async update

>  - remove bypass of intel_prepare_plane_fb() in case of async update

>  - add missing drm_atomic_helper_cleanup_planes(dev, state) call in

> intel_atomic_commit().

>  - use swap() function in async update to set the old_fb in the  new_state object.

>  - use helpers intel_update_plane()/intel_disable_plane()

> 

> Changes in v7:

> - Rebase on top of drm-intel repository. Hopefully now will play

>   nicely with autobuilders.

> 

> Changes in v6:

> - Rework the intel_plane_atomic_async_update due driver changed from

>   last time.

> - Removed the mutex_lock/unlock as causes a deadlock.

> 

> Changes in v5:

> - Call drm_atomic_helper_async_check() from the check hook

> 

> Changes in v4:

> - Set correct vma to new state for cleanup

> - Move size checks back to drivers (Ville Syrjälä)

> 

> Changes in v3:

> - Move fb setting to core and use new state (Eric Anholt)

> 

>  drivers/gpu/drm/i915/intel_atomic_plane.c | 71 +++++++++++++++++++++++

>  drivers/gpu/drm/i915/intel_display.c      |  9 +++

>  2 files changed, 80 insertions(+)

> 

> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c

> b/drivers/gpu/drm/i915/intel_atomic_plane.c

> index 7961cf0e6951..f516b227dba9 100644

> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c

> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c

> @@ -318,10 +318,81 @@ void i9xx_update_planes_on_crtc(struct

> intel_atomic_state *state,

>  	}

>  }

> 

> +static int intel_plane_atomic_async_check(struct drm_plane *plane,

> +					  struct drm_plane_state *state)

> +{

> +	struct drm_crtc_state *crtc_state;

> +

> +	crtc_state = drm_atomic_get_existing_crtc_state(state->state,

> +							state->crtc);

> +	if (WARN_ON(!crtc_state))

> +		return -EINVAL;

> +

> +	/*

> +	 * When crtc is inactive or there is a modeset pending,

> +	 * wait for it to complete in the slowpath

> +	 */

> +	if (!crtc_state->active || to_intel_crtc_state(crtc_state)->update_pipe)

> +		return -EINVAL;

> +

> +	/*

> +	 * If any parameters change that may affect watermarks,

> +	 * take the slowpath. Only changing fb or position should be

> +	 * in the fastpath.

> +	 */

> +	if (plane->state->crtc != state->crtc ||

> +	    plane->state->src_w != state->src_w ||

> +	    plane->state->src_h != state->src_h ||

> +	    plane->state->crtc_w != state->crtc_w ||

> +	    plane->state->crtc_h != state->crtc_h ||

> +	    !plane->state->fb != !state->fb)

> +		return -EINVAL;

> +

> +	return 0;

> +}

> +

> +static void intel_plane_atomic_async_update(struct drm_plane *plane,

> +					    struct drm_plane_state *new_state)

> {

> +	struct intel_atomic_state *intel_new_state =

> +		to_intel_atomic_state(new_state->state);

> +	struct intel_plane *intel_plane = to_intel_plane(plane);

> +	struct drm_crtc *crtc = plane->state->crtc;

> +	struct intel_crtc_state *new_crtc_state;

> +	struct intel_crtc *intel_crtc;

> +	int i;

> +

> +	for_each_new_intel_crtc_in_state(intel_new_state, intel_crtc,

> +					 new_crtc_state, i)

> +		WARN_ON(new_crtc_state->wm.need_postvbl_update ||

> +			new_crtc_state->update_wm_post);

> +

> +	i915_gem_track_fb(intel_fb_obj(plane->state->fb),

> +			  intel_fb_obj(new_state->fb),

> +			  intel_plane->frontbuffer_bit);

> +

> +	plane->state->src_x = new_state->src_x;

> +	plane->state->src_y = new_state->src_y;

> +	plane->state->crtc_x = new_state->crtc_x;

> +	plane->state->crtc_y = new_state->crtc_y;

> +

> +	swap(plane->state->fb, new_state->fb);

> +

> +	if (plane->state->visible)

> +		intel_update_plane(intel_plane,

> +				   to_intel_crtc_state(crtc->state),

> +				   to_intel_plane_state(plane->state));

> +	else

> +		intel_disable_plane(intel_plane,

> +				    to_intel_crtc_state(crtc->state));

> +}

> +

>  const struct drm_plane_helper_funcs intel_plane_helper_funcs = {

>  	.prepare_fb = intel_prepare_plane_fb,

>  	.cleanup_fb = intel_cleanup_plane_fb,

>  	.atomic_check = intel_plane_atomic_check,

> +	.atomic_async_check = intel_plane_atomic_async_check,

> +	.atomic_async_update = intel_plane_atomic_async_update,

>  };

> 

>  /**

> diff --git a/drivers/gpu/drm/i915/intel_display.c

> b/drivers/gpu/drm/i915/intel_display.c

> index 415d8968f2c5..244e1c94277d 100644

> --- a/drivers/gpu/drm/i915/intel_display.c

> +++ b/drivers/gpu/drm/i915/intel_display.c

> @@ -13464,6 +13464,15 @@ static int intel_atomic_commit(struct

> drm_device *dev,

>  	struct drm_i915_private *dev_priv = to_i915(dev);

>  	int ret = 0;

> 

> +	if (state->async_update) {

> +		ret = drm_atomic_helper_prepare_planes(dev, state);

> +		if (ret)

> +			return ret;

> +		drm_atomic_helper_async_commit(dev, state);

> +		drm_atomic_helper_cleanup_planes(dev, state);

> +		return 0;

> +	}

> +

>  	drm_atomic_state_get(state);

>  	i915_sw_fence_init(&intel_state->commit_ready,

>  			   intel_atomic_commit_ready);

> --

> 2.20.1
Please send drm/i915 patches also to intel-gfx@lists.freedesktop.org to
trigger CI on the patches among other things.

BR,
Jani.


On Thu, 07 Mar 2019, Helen Koike <helen.koike@collabora.com> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>
> Add implementation for async plane update callbacks
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>
> ---
> Hi,
>
> This patch just adds the callbacks for atomic async update, the next in
> the series will replace the legacy cursor implementation with these
> callbacks.
>
> I tested with igt (ksm_cursor_legacy and plane_cursor_legacy).
>
> This patch depends on "[PATCH] drm: don't block fb changes for async plane updates",
> otherwise there will be a regression on igt tests:
>
>         cursor-vs-flip-atomic-transitions-varying-size
>         cursor-vs-flip-toggle
>         cursor-vs-flip-varying-size
>
> with errors of type:
>
> "CRITICAL: completed 97 cursor updated in a period of 30 flips, we
> expect to complete approximately 15360 updates, with the threshold set
> at 7680"
>
> This happens because what should be async updates are being turned into
> syncronous updates.
>
> Thanks
> Helen
>
> Changes in v8:
>  - v7: https://lkml.org/lkml/2018/6/8/168
>  - v7 was splited in two, one that adds the async callbacks and another
>  that updates the cursor.
>  - rebase with drm-intel
>  - allow async update in all types of planes, not only cursor
>  - add watermark checks in async update
>  - remove bypass of intel_prepare_plane_fb() in case of async update
>  - add missing drm_atomic_helper_cleanup_planes(dev, state) call in
>  intel_atomic_commit().
>  - use swap() function in async update to set the old_fb in the
>  new_state object.
>  - use helpers intel_update_plane()/intel_disable_plane()
>
> Changes in v7:
> - Rebase on top of drm-intel repository. Hopefully now will play
>   nicely with autobuilders.
>
> Changes in v6:
> - Rework the intel_plane_atomic_async_update due driver changed from
>   last time.
> - Removed the mutex_lock/unlock as causes a deadlock.
>
> Changes in v5:
> - Call drm_atomic_helper_async_check() from the check hook
>
> Changes in v4:
> - Set correct vma to new state for cleanup
> - Move size checks back to drivers (Ville Syrjälä)
>
> Changes in v3:
> - Move fb setting to core and use new state (Eric Anholt)
>
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 71 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c      |  9 +++
>  2 files changed, 80 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 7961cf0e6951..f516b227dba9 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -318,10 +318,81 @@ void i9xx_update_planes_on_crtc(struct intel_atomic_state *state,
>  	}
>  }
>  
> +static int intel_plane_atomic_async_check(struct drm_plane *plane,
> +					  struct drm_plane_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +
> +	crtc_state = drm_atomic_get_existing_crtc_state(state->state,
> +							state->crtc);
> +	if (WARN_ON(!crtc_state))
> +		return -EINVAL;
> +
> +	/*
> +	 * When crtc is inactive or there is a modeset pending,
> +	 * wait for it to complete in the slowpath
> +	 */
> +	if (!crtc_state->active || to_intel_crtc_state(crtc_state)->update_pipe)
> +		return -EINVAL;
> +
> +	/*
> +	 * If any parameters change that may affect watermarks,
> +	 * take the slowpath. Only changing fb or position should be
> +	 * in the fastpath.
> +	 */
> +	if (plane->state->crtc != state->crtc ||
> +	    plane->state->src_w != state->src_w ||
> +	    plane->state->src_h != state->src_h ||
> +	    plane->state->crtc_w != state->crtc_w ||
> +	    plane->state->crtc_h != state->crtc_h ||
> +	    !plane->state->fb != !state->fb)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static void intel_plane_atomic_async_update(struct drm_plane *plane,
> +					    struct drm_plane_state *new_state)
> +{
> +	struct intel_atomic_state *intel_new_state =
> +		to_intel_atomic_state(new_state->state);
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_crtc *crtc = plane->state->crtc;
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *intel_crtc;
> +	int i;
> +
> +	for_each_new_intel_crtc_in_state(intel_new_state, intel_crtc,
> +					 new_crtc_state, i)
> +		WARN_ON(new_crtc_state->wm.need_postvbl_update ||
> +			new_crtc_state->update_wm_post);
> +
> +	i915_gem_track_fb(intel_fb_obj(plane->state->fb),
> +			  intel_fb_obj(new_state->fb),
> +			  intel_plane->frontbuffer_bit);
> +
> +	plane->state->src_x = new_state->src_x;
> +	plane->state->src_y = new_state->src_y;
> +	plane->state->crtc_x = new_state->crtc_x;
> +	plane->state->crtc_y = new_state->crtc_y;
> +
> +	swap(plane->state->fb, new_state->fb);
> +
> +	if (plane->state->visible)
> +		intel_update_plane(intel_plane,
> +				   to_intel_crtc_state(crtc->state),
> +				   to_intel_plane_state(plane->state));
> +	else
> +		intel_disable_plane(intel_plane,
> +				    to_intel_crtc_state(crtc->state));
> +}
> +
>  const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
>  	.prepare_fb = intel_prepare_plane_fb,
>  	.cleanup_fb = intel_cleanup_plane_fb,
>  	.atomic_check = intel_plane_atomic_check,
> +	.atomic_async_check = intel_plane_atomic_async_check,
> +	.atomic_async_update = intel_plane_atomic_async_update,
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 415d8968f2c5..244e1c94277d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13464,6 +13464,15 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	int ret = 0;
>  
> +	if (state->async_update) {
> +		ret = drm_atomic_helper_prepare_planes(dev, state);
> +		if (ret)
> +			return ret;
> +		drm_atomic_helper_async_commit(dev, state);
> +		drm_atomic_helper_cleanup_planes(dev, state);
> +		return 0;
> +	}
> +
>  	drm_atomic_state_get(state);
>  	i915_sw_fence_init(&intel_state->commit_ready,
>  			   intel_atomic_commit_ready);
Hi Jani,

On 3/8/19 6:55 AM, Jani Nikula wrote:
> 
> Please send drm/i915 patches also to intel-gfx@lists.freedesktop.org to
> trigger CI on the patches among other things.

Sure! Is there a way to tell the CI to pick up the dependency [1] as well?

[1] "[PATCH] drm: don't block fb changes for async plane updates"

Regards,
Helen

> 
> BR,
> Jani.
> 
> 
> On Thu, 07 Mar 2019, Helen Koike <helen.koike@collabora.com> wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>
>> Add implementation for async plane update callbacks
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>> Hi,
>>
>> This patch just adds the callbacks for atomic async update, the next in
>> the series will replace the legacy cursor implementation with these
>> callbacks.
>>
>> I tested with igt (ksm_cursor_legacy and plane_cursor_legacy).
>>
>> This patch depends on "[PATCH] drm: don't block fb changes for async plane updates",
>> otherwise there will be a regression on igt tests:
>>
>>         cursor-vs-flip-atomic-transitions-varying-size
>>         cursor-vs-flip-toggle
>>         cursor-vs-flip-varying-size
>>
>> with errors of type:
>>
>> "CRITICAL: completed 97 cursor updated in a period of 30 flips, we
>> expect to complete approximately 15360 updates, with the threshold set
>> at 7680"
>>
>> This happens because what should be async updates are being turned into
>> syncronous updates.
>>
>> Thanks
>> Helen
>>
>> Changes in v8:
>>  - v7: https://lkml.org/lkml/2018/6/8/168
>>  - v7 was splited in two, one that adds the async callbacks and another
>>  that updates the cursor.
>>  - rebase with drm-intel
>>  - allow async update in all types of planes, not only cursor
>>  - add watermark checks in async update
>>  - remove bypass of intel_prepare_plane_fb() in case of async update
>>  - add missing drm_atomic_helper_cleanup_planes(dev, state) call in
>>  intel_atomic_commit().
>>  - use swap() function in async update to set the old_fb in the
>>  new_state object.
>>  - use helpers intel_update_plane()/intel_disable_plane()
>>
>> Changes in v7:
>> - Rebase on top of drm-intel repository. Hopefully now will play
>>   nicely with autobuilders.
>>
>> Changes in v6:
>> - Rework the intel_plane_atomic_async_update due driver changed from
>>   last time.
>> - Removed the mutex_lock/unlock as causes a deadlock.
>>
>> Changes in v5:
>> - Call drm_atomic_helper_async_check() from the check hook
>>
>> Changes in v4:
>> - Set correct vma to new state for cleanup
>> - Move size checks back to drivers (Ville Syrjälä)
>>
>> Changes in v3:
>> - Move fb setting to core and use new state (Eric Anholt)
>>
>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 71 +++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_display.c      |  9 +++
>>  2 files changed, 80 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> index 7961cf0e6951..f516b227dba9 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> @@ -318,10 +318,81 @@ void i9xx_update_planes_on_crtc(struct intel_atomic_state *state,
>>  	}
>>  }
>>  
>> +static int intel_plane_atomic_async_check(struct drm_plane *plane,
>> +					  struct drm_plane_state *state)
>> +{
>> +	struct drm_crtc_state *crtc_state;
>> +
>> +	crtc_state = drm_atomic_get_existing_crtc_state(state->state,
>> +							state->crtc);
>> +	if (WARN_ON(!crtc_state))
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * When crtc is inactive or there is a modeset pending,
>> +	 * wait for it to complete in the slowpath
>> +	 */
>> +	if (!crtc_state->active || to_intel_crtc_state(crtc_state)->update_pipe)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * If any parameters change that may affect watermarks,
>> +	 * take the slowpath. Only changing fb or position should be
>> +	 * in the fastpath.
>> +	 */
>> +	if (plane->state->crtc != state->crtc ||
>> +	    plane->state->src_w != state->src_w ||
>> +	    plane->state->src_h != state->src_h ||
>> +	    plane->state->crtc_w != state->crtc_w ||
>> +	    plane->state->crtc_h != state->crtc_h ||
>> +	    !plane->state->fb != !state->fb)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static void intel_plane_atomic_async_update(struct drm_plane *plane,
>> +					    struct drm_plane_state *new_state)
>> +{
>> +	struct intel_atomic_state *intel_new_state =
>> +		to_intel_atomic_state(new_state->state);
>> +	struct intel_plane *intel_plane = to_intel_plane(plane);
>> +	struct drm_crtc *crtc = plane->state->crtc;
>> +	struct intel_crtc_state *new_crtc_state;
>> +	struct intel_crtc *intel_crtc;
>> +	int i;
>> +
>> +	for_each_new_intel_crtc_in_state(intel_new_state, intel_crtc,
>> +					 new_crtc_state, i)
>> +		WARN_ON(new_crtc_state->wm.need_postvbl_update ||
>> +			new_crtc_state->update_wm_post);
>> +
>> +	i915_gem_track_fb(intel_fb_obj(plane->state->fb),
>> +			  intel_fb_obj(new_state->fb),
>> +			  intel_plane->frontbuffer_bit);
>> +
>> +	plane->state->src_x = new_state->src_x;
>> +	plane->state->src_y = new_state->src_y;
>> +	plane->state->crtc_x = new_state->crtc_x;
>> +	plane->state->crtc_y = new_state->crtc_y;
>> +
>> +	swap(plane->state->fb, new_state->fb);
>> +
>> +	if (plane->state->visible)
>> +		intel_update_plane(intel_plane,
>> +				   to_intel_crtc_state(crtc->state),
>> +				   to_intel_plane_state(plane->state));
>> +	else
>> +		intel_disable_plane(intel_plane,
>> +				    to_intel_crtc_state(crtc->state));
>> +}
>> +
>>  const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
>>  	.prepare_fb = intel_prepare_plane_fb,
>>  	.cleanup_fb = intel_cleanup_plane_fb,
>>  	.atomic_check = intel_plane_atomic_check,
>> +	.atomic_async_check = intel_plane_atomic_async_check,
>> +	.atomic_async_update = intel_plane_atomic_async_update,
>>  };
>>  
>>  /**
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 415d8968f2c5..244e1c94277d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13464,6 +13464,15 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>  	int ret = 0;
>>  
>> +	if (state->async_update) {
>> +		ret = drm_atomic_helper_prepare_planes(dev, state);
>> +		if (ret)
>> +			return ret;
>> +		drm_atomic_helper_async_commit(dev, state);
>> +		drm_atomic_helper_cleanup_planes(dev, state);
>> +		return 0;
>> +	}
>> +
>>  	drm_atomic_state_get(state);
>>  	i915_sw_fence_init(&intel_state->commit_ready,
>>  			   intel_atomic_commit_ready);
>
On Fri, 08 Mar 2019, Helen Koike <helen.koike@collabora.com> wrote:
> Hi Jani,
>
> On 3/8/19 6:55 AM, Jani Nikula wrote:
>> 
>> Please send drm/i915 patches also to intel-gfx@lists.freedesktop.org to
>> trigger CI on the patches among other things.
>
> Sure! Is there a way to tell the CI to pick up the dependency [1] as well?

No. You should post these as part of a self-contained series. Looks like
you already have other driver changes in the series.

BR,
Jani.


>
> [1] "[PATCH] drm: don't block fb changes for async plane updates"
>
> Regards,
> Helen
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> On Thu, 07 Mar 2019, Helen Koike <helen.koike@collabora.com> wrote:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>
>>> Add implementation for async plane update callbacks
>>>
>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>
>>> ---
>>> Hi,
>>>
>>> This patch just adds the callbacks for atomic async update, the next in
>>> the series will replace the legacy cursor implementation with these
>>> callbacks.
>>>
>>> I tested with igt (ksm_cursor_legacy and plane_cursor_legacy).
>>>
>>> This patch depends on "[PATCH] drm: don't block fb changes for async plane updates",
>>> otherwise there will be a regression on igt tests:
>>>
>>>         cursor-vs-flip-atomic-transitions-varying-size
>>>         cursor-vs-flip-toggle
>>>         cursor-vs-flip-varying-size
>>>
>>> with errors of type:
>>>
>>> "CRITICAL: completed 97 cursor updated in a period of 30 flips, we
>>> expect to complete approximately 15360 updates, with the threshold set
>>> at 7680"
>>>
>>> This happens because what should be async updates are being turned into
>>> syncronous updates.
>>>
>>> Thanks
>>> Helen
>>>
>>> Changes in v8:
>>>  - v7: https://lkml.org/lkml/2018/6/8/168
>>>  - v7 was splited in two, one that adds the async callbacks and another
>>>  that updates the cursor.
>>>  - rebase with drm-intel
>>>  - allow async update in all types of planes, not only cursor
>>>  - add watermark checks in async update
>>>  - remove bypass of intel_prepare_plane_fb() in case of async update
>>>  - add missing drm_atomic_helper_cleanup_planes(dev, state) call in
>>>  intel_atomic_commit().
>>>  - use swap() function in async update to set the old_fb in the
>>>  new_state object.
>>>  - use helpers intel_update_plane()/intel_disable_plane()
>>>
>>> Changes in v7:
>>> - Rebase on top of drm-intel repository. Hopefully now will play
>>>   nicely with autobuilders.
>>>
>>> Changes in v6:
>>> - Rework the intel_plane_atomic_async_update due driver changed from
>>>   last time.
>>> - Removed the mutex_lock/unlock as causes a deadlock.
>>>
>>> Changes in v5:
>>> - Call drm_atomic_helper_async_check() from the check hook
>>>
>>> Changes in v4:
>>> - Set correct vma to new state for cleanup
>>> - Move size checks back to drivers (Ville Syrjälä)
>>>
>>> Changes in v3:
>>> - Move fb setting to core and use new state (Eric Anholt)
>>>
>>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 71 +++++++++++++++++++++++
>>>  drivers/gpu/drm/i915/intel_display.c      |  9 +++
>>>  2 files changed, 80 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> index 7961cf0e6951..f516b227dba9 100644
>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> @@ -318,10 +318,81 @@ void i9xx_update_planes_on_crtc(struct intel_atomic_state *state,
>>>  	}
>>>  }
>>>  
>>> +static int intel_plane_atomic_async_check(struct drm_plane *plane,
>>> +					  struct drm_plane_state *state)
>>> +{
>>> +	struct drm_crtc_state *crtc_state;
>>> +
>>> +	crtc_state = drm_atomic_get_existing_crtc_state(state->state,
>>> +							state->crtc);
>>> +	if (WARN_ON(!crtc_state))
>>> +		return -EINVAL;
>>> +
>>> +	/*
>>> +	 * When crtc is inactive or there is a modeset pending,
>>> +	 * wait for it to complete in the slowpath
>>> +	 */
>>> +	if (!crtc_state->active || to_intel_crtc_state(crtc_state)->update_pipe)
>>> +		return -EINVAL;
>>> +
>>> +	/*
>>> +	 * If any parameters change that may affect watermarks,
>>> +	 * take the slowpath. Only changing fb or position should be
>>> +	 * in the fastpath.
>>> +	 */
>>> +	if (plane->state->crtc != state->crtc ||
>>> +	    plane->state->src_w != state->src_w ||
>>> +	    plane->state->src_h != state->src_h ||
>>> +	    plane->state->crtc_w != state->crtc_w ||
>>> +	    plane->state->crtc_h != state->crtc_h ||
>>> +	    !plane->state->fb != !state->fb)
>>> +		return -EINVAL;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void intel_plane_atomic_async_update(struct drm_plane *plane,
>>> +					    struct drm_plane_state *new_state)
>>> +{
>>> +	struct intel_atomic_state *intel_new_state =
>>> +		to_intel_atomic_state(new_state->state);
>>> +	struct intel_plane *intel_plane = to_intel_plane(plane);
>>> +	struct drm_crtc *crtc = plane->state->crtc;
>>> +	struct intel_crtc_state *new_crtc_state;
>>> +	struct intel_crtc *intel_crtc;
>>> +	int i;
>>> +
>>> +	for_each_new_intel_crtc_in_state(intel_new_state, intel_crtc,
>>> +					 new_crtc_state, i)
>>> +		WARN_ON(new_crtc_state->wm.need_postvbl_update ||
>>> +			new_crtc_state->update_wm_post);
>>> +
>>> +	i915_gem_track_fb(intel_fb_obj(plane->state->fb),
>>> +			  intel_fb_obj(new_state->fb),
>>> +			  intel_plane->frontbuffer_bit);
>>> +
>>> +	plane->state->src_x = new_state->src_x;
>>> +	plane->state->src_y = new_state->src_y;
>>> +	plane->state->crtc_x = new_state->crtc_x;
>>> +	plane->state->crtc_y = new_state->crtc_y;
>>> +
>>> +	swap(plane->state->fb, new_state->fb);
>>> +
>>> +	if (plane->state->visible)
>>> +		intel_update_plane(intel_plane,
>>> +				   to_intel_crtc_state(crtc->state),
>>> +				   to_intel_plane_state(plane->state));
>>> +	else
>>> +		intel_disable_plane(intel_plane,
>>> +				    to_intel_crtc_state(crtc->state));
>>> +}
>>> +
>>>  const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
>>>  	.prepare_fb = intel_prepare_plane_fb,
>>>  	.cleanup_fb = intel_cleanup_plane_fb,
>>>  	.atomic_check = intel_plane_atomic_check,
>>> +	.atomic_async_check = intel_plane_atomic_async_check,
>>> +	.atomic_async_update = intel_plane_atomic_async_update,
>>>  };
>>>  
>>>  /**
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 415d8968f2c5..244e1c94277d 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -13464,6 +13464,15 @@ static int intel_atomic_commit(struct drm_device *dev,
>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>  	int ret = 0;
>>>  
>>> +	if (state->async_update) {
>>> +		ret = drm_atomic_helper_prepare_planes(dev, state);
>>> +		if (ret)
>>> +			return ret;
>>> +		drm_atomic_helper_async_commit(dev, state);
>>> +		drm_atomic_helper_cleanup_planes(dev, state);
>>> +		return 0;
>>> +	}
>>> +
>>>  	drm_atomic_state_get(state);
>>>  	i915_sw_fence_init(&intel_state->commit_ready,
>>>  			   intel_atomic_commit_ready);
>>