[Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

Mon Feb 20 14:30:58 UTC 2017

Op 20-02-17 om 14:38 schreef Ville Syrjälä:
> On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote:
>> Op 17-02-17 om 16:01 schreef ville.syrjala at linux.intel.com:
>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>
>>> In order to make cursor updates actually safe wrt. watermark programming
>>> we have to clear the legacy_cursor_update flag in the atomic state. That
>>> will cause the regular atomic update path to do the necessary vblank
>>> wait after the plane update if needed, otherwise the vblank wait would
>>> be skipped and we'd feed the optimal watermarks to the hardware before
>>> the plane update has actually happened.
>>>
>>> To make the slow vs. fast path determination in
>>> intel_legacy_cursor_update() a little simpler we can ignore the actual
>>> visibility of the plane (which can only get computed once we've already
>>> chosen out path) and instead we simply check whether the fb is being
>>> set or cleared by the user. This means a fully clipped but logically
>>> visible cursor will be considered visible as far as watermark
>>> programming is concerned. We can do that for the cursor since it's a
>>> fixed size plane and the clipped size doesn't play a role in the
>>> watermark computation.
>>>
>>> This should fix underruns that can occur when the cursor gets
>>> enable/disabled or the size gets changed. Hopefully it's good enough
>>> that only pure cursor movement and flips go through unthrottled.
>>>
>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>> Cc: Uwe Kleine-König <uwe at kleine-koenig.org>
>>> Reported-by: Uwe Kleine-König <uwe at kleine-koenig.org>
>>> Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting legacy page flip to atomic, v3.")
>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++-----------
>>>  drivers/gpu/drm/i915/intel_pm.c      | 20 ++++++++++++--------
>>>  2 files changed, 30 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index b05d9c85384b..356ac04093e8 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct drm_device *dev,
>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>  	int ret = 0;
>>>  
>>> +	/*
>>> +	 * 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.
>>> +	 */
>>> +	if (INTEL_GEN(dev_priv) < 9)
>>> +		state->legacy_cursor_update = false;
>> Could we perhaps add a check in ilk_compute_pipe_wm which unsets the legacy_cursor_update flag so we keep things unsynced as much as possible?
> I'd have to sprinkle that stuff everywhere but the SKL code
> eventually. Seems a little pointless when I can just plop it
> there.
Ah indeed. Lets hope it doesn't slow things down too much.
>>>  	ret = drm_atomic_helper_setup_commit(state, nonblock);
>>>  	if (ret)
>>>  		return ret;
>>> @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>>  	    old_plane_state->src_h != src_h ||
>>>  	    old_plane_state->crtc_w != crtc_w ||
>>>  	    old_plane_state->crtc_h != crtc_h ||
>>> -	    !old_plane_state->visible ||
>>> -	    old_plane_state->fb->modifier != fb->modifier)
>>> +	    !old_plane_state->fb != !fb)
>>>  		goto slow;
>>>  
>>>  	new_plane_state = intel_plane_duplicate_state(plane);
>>> @@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>>  	if (ret)
>>>  		goto out_free;
>>>  
>>> -	/* Visibility changed, must take slowpath. */
>>> -	if (!new_plane_state->visible)
>>> -		goto slow_free;
>>> -
>>>  	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
>>>  	if (ret)
>>>  		goto out_free;
>> Those 2 hunks are needed. If you move the cursor out of the visible area or back you need to update.
> No. I changed the wm code to consider a non-visible but logicall active
> cursor as needing proper watermarks. That avoids needing this fallback
> path here.
Ah indeed. But one thing you dropped is the fb modifier check.
I suppose there's no harm with no support for using prite planes as cursor plane yet, but might be nice to keep it in.

Cc'ing Ristovski for testing the patch. :)
>
>> Easiest way to trigger a bug is load a 256 width cursor out of the visible area, move it back in and you get
>> a fifo underrun.
>>
>> Why is switching fb's synced?
> It is not.
>
>> Identical sized fb should be unsynced if possible.
>>
>>> @@ -13522,9 +13528,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>>  	new_plane_state->fb = old_fb;
>>>  	to_intel_plane_state(new_plane_state)->vma = old_vma;
>>>  
>>> -	intel_plane->update_plane(plane,
>>> -				  to_intel_crtc_state(crtc->state),
>>> -				  to_intel_plane_state(plane->state));
>>> +	if (plane->state->visible)
>>> +		intel_plane->update_plane(plane,
>>> +					  to_intel_crtc_state(crtc->state),
>>> +					  to_intel_plane_state(plane->state));
>>> +	else
>>> +		intel_plane->disable_plane(plane, crtc);
>>>  
>>>  	intel_cleanup_plane_fb(plane, new_plane_state);
>>>  
>>> @@ -13534,8 +13543,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>>  	intel_plane_destroy_state(plane, new_plane_state);
>>>  	return ret;
>>>  
>>> -slow_free:
>>> -	intel_plane_destroy_state(plane, new_plane_state);
>>>  slow:
>>>  	return drm_atomic_helper_update_plane(plane, crtc, fb,
>>>  					      crtc_x, crtc_y, crtc_w, crtc_h,
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index fe243c65de1a..4de8c40acc7e 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -1831,20 +1831,24 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
>>>  				   const struct intel_plane_state *pstate,
>>>  				   uint32_t mem_value)
>>>  {
>>> +	int cpp;
>>> +
>>>  	/*
>>> -	 * We treat the cursor plane as always-on for the purposes of watermark
>>> -	 * calculation.  Until we have two-stage watermark programming merged,
>>> -	 * this is necessary to avoid flickering.
>>> +	 * Treat cursor with fb as always visible since cursor updates
>>> +	 * 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
>>> +	 * around this problem with the watermark code.
>>>  	 */
>>> -	int cpp = 4;
>>> -	int width = pstate->base.visible ? pstate->base.crtc_w : 64;
>>> -
>>> -	if (!cstate->base.active)
>>> +	if (!cstate->base.active || !pstate->base.fb)
>>>  		return 0;
>>>  
>>> +	cpp = pstate->base.fb->format->cpp[0];
>>> +
>>>  	return ilk_wm_method2(cstate->pixel_rate,
>>>  			      cstate->base.adjusted_mode.crtc_htotal,
>>> -			      width, cpp, mem_value);
>>> +			      pstate->base.crtc_w, cpp, mem_value);
>>>  }
>>>  
>>>  /* Only for WM_LP. */