[Intel-gfx] [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware

[Ville Syrjälä]

On Tue, Feb 07, 2017 at 05:03:09PM +0200, Martin Peres wrote:
> On 07/02/17 15:22, Uwe Kleine-König wrote:
> > Hello,
> >
> > On 02/01/2017 03:37 PM, Ville Syrjälä wrote:
> >> On Wed, Feb 01, 2017 at 01:41:08PM +0100, Maarten Lankhorst wrote:
> >>> Op 31-01-17 om 20:13 schreef Uwe Kleine-König:
> >>>> On Tue, Jan 31, 2017 at 10:03:26AM +0100, Maarten Lankhorst wrote:
> >>>>> Op 31-01-17 om 09:09 schreef Uwe Kleine-König:
> >>>>> Just curious, does this help?
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>>> index ae2c0bb4b2e8..13de4c526ca6 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>>> @@ -1838,7 +1838,7 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
> >>>>>  	 * this is necessary to avoid flickering.
> >>>>>  	 */
> >>>>>  	int cpp = 4;
> >>>>> -	int width = pstate->base.visible ? pstate->base.crtc_w : 64;
> >>>>> +	int width = 256;
> >>>>>
> >>>>>  	if (!cstate->base.active)
> >>>>>  		return 0;
> >>>>>
> >>>> On a kernel with this patch applied I cannot reproduce the flickering. I
> >>>> keep that kernel running but expect that it also fixes the crash.
> >>>
> >>> Ok that's good news.
> >>>
> >>> Maybe ville or matt can comment whether this patch is the right fix?
> >>
> >> Well, it's just extending the hack even further. The right fix would be
> >> to fix the wm programming sequence to respect the frame boundaries
> >> correctly (ie. my old vblank based wm stuff).
> >
> > so I wonder how this goes forward. The situation seems to be well
> > understood, but other than testing patches I don't know what to do (and
> > there is currently no patch to test).
> >
> > Best regards
> > Uwe
> >
> 
> The way I understand this is that no-one wants to restrict the 
> capabilities exposed by the kernel and would like a proper fix for this. 
> However, I agree with Uwe, given the low priority status of Gen5 (people 
> would rather work on hw that is used by a lot of people), we should 
> probably accept the patch proposed by Maarten as it fixes someone's 
> workflow and does not regress anything meaningful.

The same code is used for ILK-BDW, so it's not just ILK. And other other
platform suffers from the same problem of cursor vs. watermarks. It just
seems that most people are lucky enough to not be seriously affected by
this problem.

Also it can regress some things, at least theoretically. Power consumption
with < 256x256 for one, and potentially it could also end up rejecting
some display modes that previously used to work with smaller cursor
sizes (or no cursors). That last part may not be 100% true, but I was
too lazy to go through the math to see if the cursor FIFO could end up
being the limiting factor in some cases.

I was thinking Maarten's intel_legacy_cursor_update() hack should have
"fixed" this, but now I'm not sure since it still sets the
legacy_cursor_update flag in the slow path, and the commit message
didn't quite manage to tell me what the purpose of this function 
was supposed to be. The logic for picking the slow path also seems a
little wonky to me (assuming I deduced the purpose of the function
correctly).

So, we might want something like:

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 88689a0b4183..307ee4f7bd58 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15053,8 +15053,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);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ec16f3d6dd2e..660990a3f276 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1865,20 +1865,26 @@ 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 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(ilk_pipe_pixel_rate(cstate),
 			      cstate->base.adjusted_mode.crtc_htotal,
-			      width, cpp, mem_value);
+			      pstate->base.crtc_w,
+			      cpp, mem_value);
 }
 
 /* Only for WM_LP. */

and fix up the legacy_cursor_update flag problem with the slow path...

-- 
Ville Syrjälä
Intel OTC