* [PATCH v8 2/2] drm/i915: update cursors asynchronously through atomic
2019-03-07 16:49 [PATCH v8 1/2] drm/i915: Introduce async plane update to i915 Helen Koike
@ 2019-03-07 16:49 ` Helen Koike
2019-03-08 8:08 ` [PATCH v8 1/2] drm/i915: Introduce async plane update to i915 Zhang, Tina
2019-03-08 9:55 ` Jani Nikula
2 siblings, 0 replies; 6+ messages in thread
From: Helen Koike @ 2019-03-07 16:49 UTC (permalink / raw)
To: dri-devel; +Cc: tfiga, mcasas, zhenyu.z.wang, daniel.vetter, tina.zhang, kernel
From: Gustavo Padovan <gustavo.padovan@collabora.com>
Replace the legacy cursor implementation by the async callbacks
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---
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.
- Update comment in intel_pm.c that was referencing
intel_plane_atomic_async_update()
drivers/gpu/drm/i915/intel_display.c | 165 +--------------------------
drivers/gpu/drm/i915/intel_pm.c | 2 +-
2 files changed, 6 insertions(+), 161 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 244e1c94277d..bd84d5b9c441 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13005,6 +13005,10 @@ static int intel_atomic_check(struct drm_device *dev,
if (ret)
return ret;
+ if (state->legacy_cursor_update)
+ state->async_update = !drm_atomic_helper_async_check(dev,
+ state);
+
intel_fbc_choose_crtc(dev_priv, intel_state);
return calc_watermark_data(intel_state);
}
@@ -13477,34 +13481,6 @@ static int intel_atomic_commit(struct drm_device *dev,
i915_sw_fence_init(&intel_state->commit_ready,
intel_atomic_commit_ready);
- /*
- * The intel_legacy_cursor_update() fast path takes care
- * of avoiding the vblank waits for simple cursor
- * movement and flips. For cursor on/off and size changes,
- * we want to perform the vblank waits so that watermark
- * updates happen during the correct frames. Gen9+ have
- * double buffered watermarks and so shouldn't need this.
- *
- * Unset state->legacy_cursor_update before the call to
- * drm_atomic_helper_setup_commit() because otherwise
- * drm_atomic_helper_wait_for_flip_done() is a noop and
- * we get FIFO underruns because we didn't wait
- * for vblank.
- *
- * FIXME doing watermarks and fb cleanup from a vblank worker
- * (assuming we had any) would solve these problems.
- */
- if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) {
- struct intel_crtc_state *new_crtc_state;
- struct intel_crtc *crtc;
- int i;
-
- for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
- if (new_crtc_state->wm.need_postvbl_update ||
- new_crtc_state->update_wm_post)
- state->legacy_cursor_update = false;
- }
-
ret = intel_atomic_prepare_commit(dev, state);
if (ret) {
DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
@@ -14012,139 +13988,8 @@ static const struct drm_plane_funcs i8xx_plane_funcs = {
.format_mod_supported = i8xx_plane_format_mod_supported,
};
-static int
-intel_legacy_cursor_update(struct drm_plane *plane,
- struct drm_crtc *crtc,
- struct drm_framebuffer *fb,
- int crtc_x, int crtc_y,
- unsigned int crtc_w, unsigned int crtc_h,
- u32 src_x, u32 src_y,
- u32 src_w, u32 src_h,
- struct drm_modeset_acquire_ctx *ctx)
-{
- struct drm_i915_private *dev_priv = to_i915(crtc->dev);
- int ret;
- struct drm_plane_state *old_plane_state, *new_plane_state;
- struct intel_plane *intel_plane = to_intel_plane(plane);
- struct drm_framebuffer *old_fb;
- struct intel_crtc_state *crtc_state =
- to_intel_crtc_state(crtc->state);
- struct intel_crtc_state *new_crtc_state;
-
- /*
- * When crtc is inactive or there is a modeset pending,
- * wait for it to complete in the slowpath
- */
- if (!crtc_state->base.active || needs_modeset(&crtc_state->base) ||
- crtc_state->update_pipe)
- goto slow;
-
- old_plane_state = plane->state;
- /*
- * Don't do an async update if there is an outstanding commit modifying
- * the plane. This prevents our async update's changes from getting
- * overridden by a previous synchronous update's state.
- */
- if (old_plane_state->commit &&
- !try_wait_for_completion(&old_plane_state->commit->hw_done))
- goto slow;
-
- /*
- * If any parameters change that may affect watermarks,
- * take the slowpath. Only changing fb or position should be
- * in the fastpath.
- */
- if (old_plane_state->crtc != crtc ||
- old_plane_state->src_w != src_w ||
- old_plane_state->src_h != src_h ||
- old_plane_state->crtc_w != crtc_w ||
- old_plane_state->crtc_h != crtc_h ||
- !old_plane_state->fb != !fb)
- goto slow;
-
- new_plane_state = intel_plane_duplicate_state(plane);
- if (!new_plane_state)
- return -ENOMEM;
-
- new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc));
- if (!new_crtc_state) {
- ret = -ENOMEM;
- goto out_free;
- }
-
- drm_atomic_set_fb_for_plane(new_plane_state, fb);
-
- new_plane_state->src_x = src_x;
- new_plane_state->src_y = src_y;
- new_plane_state->src_w = src_w;
- new_plane_state->src_h = src_h;
- new_plane_state->crtc_x = crtc_x;
- new_plane_state->crtc_y = crtc_y;
- new_plane_state->crtc_w = crtc_w;
- new_plane_state->crtc_h = crtc_h;
-
- ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state,
- to_intel_plane_state(old_plane_state),
- to_intel_plane_state(new_plane_state));
- if (ret)
- goto out_free;
-
- ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
- if (ret)
- goto out_free;
-
- ret = intel_plane_pin_fb(to_intel_plane_state(new_plane_state));
- if (ret)
- goto out_unlock;
-
- intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
-
- old_fb = old_plane_state->fb;
- i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
- intel_plane->frontbuffer_bit);
-
- /* Swap plane state */
- plane->state = new_plane_state;
-
- /*
- * We cannot swap crtc_state as it may be in use by an atomic commit or
- * page flip that's running simultaneously. If we swap crtc_state and
- * destroy the old state, we will cause a use-after-free there.
- *
- * Only update active_planes, which is needed for our internal
- * bookkeeping. Either value will do the right thing when updating
- * planes atomically. If the cursor was part of the atomic update then
- * we would have taken the slowpath.
- */
- crtc_state->active_planes = new_crtc_state->active_planes;
-
- if (plane->state->visible)
- intel_update_plane(intel_plane, crtc_state,
- to_intel_plane_state(plane->state));
- else
- intel_disable_plane(intel_plane, crtc_state);
-
- intel_plane_unpin_fb(to_intel_plane_state(old_plane_state));
-
-out_unlock:
- mutex_unlock(&dev_priv->drm.struct_mutex);
-out_free:
- if (new_crtc_state)
- intel_crtc_destroy_state(crtc, &new_crtc_state->base);
- if (ret)
- intel_plane_destroy_state(plane, new_plane_state);
- else
- intel_plane_destroy_state(plane, old_plane_state);
- return ret;
-
-slow:
- return drm_atomic_helper_update_plane(plane, crtc, fb,
- crtc_x, crtc_y, crtc_w, crtc_h,
- src_x, src_y, src_w, src_h, ctx);
-}
-
static const struct drm_plane_funcs intel_cursor_plane_funcs = {
- .update_plane = intel_legacy_cursor_update,
+ .update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
.destroy = intel_plane_destroy,
.atomic_get_property = intel_plane_atomic_get_property,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4c0e43caa5cd..1bfbb3ec39d4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -818,7 +818,7 @@ static bool intel_wm_plane_visible(const struct intel_crtc_state *crtc_state,
* can happen faster than the vrefresh rate, and the current
* watermark code doesn't handle that correctly. Cursor updates
* which set/clear the fb or change the cursor size are going
- * to get throttled by intel_legacy_cursor_update() to work
+ * to get throttled by intel_plane_atomic_async_update() to work
* around this problem with the watermark code.
*/
if (plane->id == PLANE_CURSOR)
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [PATCH v8 1/2] drm/i915: Introduce async plane update to i915
2019-03-07 16:49 [PATCH v8 1/2] drm/i915: Introduce async plane update to i915 Helen Koike
2019-03-07 16:49 ` [PATCH v8 2/2] drm/i915: update cursors asynchronously through atomic Helen Koike
@ 2019-03-08 8:08 ` Zhang, Tina
2019-03-08 9:55 ` Jani Nikula
2 siblings, 0 replies; 6+ messages in thread
From: Zhang, Tina @ 2019-03-08 8:08 UTC (permalink / raw)
To: Helen Koike, dri-devel
Cc: mcasas, Wang, Zhenyu Z, daniel.vetter, tfiga, kernel
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v8 1/2] drm/i915: Introduce async plane update to i915
2019-03-07 16:49 [PATCH v8 1/2] drm/i915: Introduce async plane update to i915 Helen Koike
2019-03-07 16:49 ` [PATCH v8 2/2] drm/i915: update cursors asynchronously through atomic Helen Koike
2019-03-08 8:08 ` [PATCH v8 1/2] drm/i915: Introduce async plane update to i915 Zhang, Tina
@ 2019-03-08 9:55 ` Jani Nikula
2019-03-08 14:51 ` Helen Koike
2 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2019-03-08 9:55 UTC (permalink / raw)
To: Helen Koike, dri-devel
Cc: tfiga, mcasas, zhenyu.z.wang, daniel.vetter, tina.zhang, kernel
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);
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v8 1/2] drm/i915: Introduce async plane update to i915
2019-03-08 9:55 ` Jani Nikula
@ 2019-03-08 14:51 ` Helen Koike
2019-03-08 15:34 ` Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Helen Koike @ 2019-03-08 14:51 UTC (permalink / raw)
To: Jani Nikula, dri-devel
Cc: tfiga, mcasas, zhenyu.z.wang, daniel.vetter, tina.zhang, kernel
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);
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v8 1/2] drm/i915: Introduce async plane update to i915
2019-03-08 14:51 ` Helen Koike
@ 2019-03-08 15:34 ` Jani Nikula
0 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2019-03-08 15:34 UTC (permalink / raw)
To: Helen Koike, dri-devel
Cc: tfiga, mcasas, zhenyu.z.wang, daniel.vetter, tina.zhang, kernel
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);
>>
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread