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

[Jani Nikula <jani.nikula@linux.intel.com>]

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