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

[Helen Koike]

Hi Jani,

On 3/8/19 6:55 AM, Jani Nikula wrote:
> 
> Please send drm/i915 patches also to intel-gfx at 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 at collabora.com> wrote:
>> From: Gustavo Padovan <gustavo.padovan at collabora.com>
>>
>> Add implementation for async plane update callbacks
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.com>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo at collabora.com>
>> Signed-off-by: Tina Zhang <tina.zhang at intel.com>
>> Signed-off-by: Helen Koike <helen.koike at 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);
>