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

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

[Uwe Kleine-König]

From: Chris Wilson <chris@chris-wilson.co.uk>

As the introduced comment admits this is clearly a workaround, but for
me this is the only known way to make my Lenovo X201 work without
flickering and crashing.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
[uwe: added changelog, comment and restrict to GEN5]
---
Hello,

as I don't like having to compile my own kernel (which has this workaround) I
suggest to apply this patch until someone with more knowledge than me about
i915 finds the muse and time to work on this.

If applying this patch means that I will become i915 maintainer, then please
don't apply; I'm not ready for this :-)

Best regards
Uwe

 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f0b9aa7a0483..126825c207b3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -16488,6 +16488,17 @@ int intel_modeset_init(struct drm_device *dev)
 	} else if (IS_GEN2(dev_priv)) {
 		dev->mode_config.cursor_width = GEN2_CURSOR_WIDTH;
 		dev->mode_config.cursor_height = GEN2_CURSOR_HEIGHT;
+	} else if (IS_GEN5(dev_priv)) {
+		/*
+		 * actually the hardware should be capable to handle
+		 * MAX_CURSOR_{WIDTH,HEIGHT} (i.e. 256), but on some GEN 5
+		 * hardware this results in fifo underruns, occasional
+		 * hardware lockups and display artifacts.
+		 * See https://bugs.freedesktop.org/show_bug.cgi?id=98742 for
+		 * more details.
+		 */
+		dev->mode_config.cursor_width = 64;
+		dev->mode_config.cursor_height = 64;
 	} else {
 		dev->mode_config.cursor_width = MAX_CURSOR_WIDTH;
 		dev->mode_config.cursor_height = MAX_CURSOR_HEIGHT;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware

[Maarten Lankhorst]

Op 31-01-17 om 09:09 schreef Uwe Kleine-König:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> As the introduced comment admits this is clearly a workaround, but for
> me this is the only known way to make my Lenovo X201 work without
> flickering and crashing.
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> [uwe: added changelog, comment and restrict to GEN5]
> ---
> Hello,
>
> as I don't like having to compile my own kernel (which has this workaround) I
> suggest to apply this patch until someone with more knowledge than me about
> i915 finds the muse and time to work on this.
>
> If applying this patch means that I will become i915 maintainer, then please
> don't apply; I'm not ready for this :-)
>
> Best regards
> Uwe
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;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1454 bytes --]

Hello,

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:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > As the introduced comment admits this is clearly a workaround, but for
> > me this is the only known way to make my Lenovo X201 work without
> > flickering and crashing.
> >
> > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > [uwe: added changelog, comment and restrict to GEN5]
> > ---
> > Hello,
> >
> > as I don't like having to compile my own kernel (which has this workaround) I
> > suggest to apply this patch until someone with more knowledge than me about
> > i915 finds the muse and time to work on this.
>
> 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.

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware

[Maarten Lankhorst]

Op 31-01-17 om 20:13 schreef Uwe Kleine-König:
> Hello,
>
> 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:
>>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>>
>>> As the introduced comment admits this is clearly a workaround, but for
>>> me this is the only known way to make my Lenovo X201 work without
>>> flickering and crashing.
>>>
>>> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>>> [uwe: added changelog, comment and restrict to GEN5]
>>> ---
>>> Hello,
>>>
>>> as I don't like having to compile my own kernel (which has this workaround) I
>>> suggest to apply this patch until someone with more knowledge than me about
>>> i915 finds the muse and time to work on this.
>> 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.
>
> Best regards
> Uwe

Ok that's good news.

Maybe ville or matt can comment whether this patch is the right fix?

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware

[Ville Syrjälä]

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:
> > Hello,
> >
> > 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:
> >>> From: Chris Wilson <chris@chris-wilson.co.uk>
> >>>
> >>> As the introduced comment admits this is clearly a workaround, but for
> >>> me this is the only known way to make my Lenovo X201 work without
> >>> flickering and crashing.
> >>>
> >>> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> >>> [uwe: added changelog, comment and restrict to GEN5]
> >>> ---
> >>> Hello,
> >>>
> >>> as I don't like having to compile my own kernel (which has this workaround) I
> >>> suggest to apply this patch until someone with more knowledge than me about
> >>> i915 finds the muse and time to work on this.
> >> 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.
> >
> > Best regards
> > Uwe
> 
> 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).

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware

[Uwe Kleine-König]

[-- Attachment #1.1.1: Type: text/plain, Size: 1566 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware

[Martin Peres]

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 proper fix for watermark computation can be worked on as time 
permits, later on.

Again, I would like to thanks Uwe for pushing hard for this, we are 
definitely not active-enough on this issue, flashing screens should be a 
big NO-NO, yet we seem to be OK with it :s

Martin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware

[Maarten Lankhorst]

Op 07-02-17 om 16:35 schreef 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).
Hey,

This patch shouldn't fix anything, maybe wait for a vblank on changing to a smaller stride?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware

[Ville Syrjälä]

On Tue, Feb 07, 2017 at 06:51:25PM +0100, Maarten Lankhorst wrote:
> Op 07-02-17 om 16:35 schreef 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).
> Hey,
> 
> This patch shouldn't fix anything, maybe wait for a vblank on changing to a smaller stride?

Hmm. If it doesn't fix anything, then why does it exist?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware

[Uwe Kleine-König]

[-- Attachment #1.1.1: Type: text/plain, Size: 5942 bytes --]

On 02/07/2017 04:35 PM, Ville Syrjälä wrote:
> 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...

Would it be sensible to test this patch on my hardware? You seem to
think it's incomplete ...?!

I can already predict that several more people will be affected by this
when Debian Stretch is released (currently in freeze) and people start
upgrading to this.

Best regards
Uwe



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
Reported-by: Uwe Kleine-König <uwe@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@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;
+
 	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;
@@ -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. */
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1328 bytes --]

Hello Ville,

On Fri, Feb 17, 2017 at 05:01:59PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@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.
> 
> [...]
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> Reported-by: Uwe Kleine-König <uwe@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@linux.intel.com>

Is this supposed to fix
https://bugs.freedesktop.org/show_bug.cgi?id=98742 ? If so, the Fixes:
line seems wrong because f79f26921ee1 isn't in 4.9 where I see the
issue.

If I want to fix 4.9---my ultimate goal is to fix the kernel that will
go into the next stable release---I have to cherry-pick f79f26921ee1
first, I assume?

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 09:04:44PM +0100, Uwe Kleine-König wrote:
> Hello Ville,
> 
> On Fri, Feb 17, 2017 at 05:01:59PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@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.
> > 
> > [...]
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> > Reported-by: Uwe Kleine-König <uwe@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@linux.intel.com>
> 
> Is this supposed to fix
> https://bugs.freedesktop.org/show_bug.cgi?id=98742 ? If so, the Fixes:
> line seems wrong because f79f26921ee1 isn't in 4.9 where I see the
> issue.
> 
> If I want to fix 4.9---my ultimate goal is to fix the kernel that will
> go into the next stable release---I have to cherry-pick f79f26921ee1
> first, I assume?

Argh. The fact is that it has actually been broken since forever,
but I suppose *something* must have been masking the problem for
you on earlier kernels, well, assuming you didn't actually hit the
problem before 4.9.

You can give backporting the custom legacy cursor path a try,
but I'm not sure how badly it depends on other things. So not
sure a backport is entirely trivial. Maarten, any thoughts?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

Op 17-02-17 om 16:01 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> Reported-by: Uwe Kleine-König <uwe@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@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?
>  	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.
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? 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. */


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> > Reported-by: Uwe Kleine-König <uwe@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@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.

> >  	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.

> 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. */
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

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@linux.intel.com:
>>> From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
>>> Reported-by: Uwe Kleine-König <uwe@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@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. */


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
> 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@linux.intel.com:
> >>> From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> >>> Reported-by: Uwe Kleine-König <uwe@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@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.

We'd have bigger problems than the modifier if we want to use a sprite
plane as the cursor because for sprite planes the watermarks are
computed based on the clipped size. So the wm code would need some
surgery as well.

> 
> Cc'ing Ristovski for testing the patch. :)

17:57 < Ristovski> mlankhorst: So I tested the patch and it passed all my
manual tests, including vigorous_pointer_movement and
spastic_window_repositioning, good enough for me!

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

Op 24-02-17 om 14:11 schreef Ville Syrjälä:
> On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
>> 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@linux.intel.com:
>>>>> From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
>>>>> Reported-by: Uwe Kleine-König <uwe@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@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.
> We'd have bigger problems than the modifier if we want to use a sprite
> plane as the cursor because for sprite planes the watermarks are
> computed based on the clipped size. So the wm code would need some
> surgery as well.
>
>> Cc'ing Ristovski for testing the patch. :)
> 17:57 < Ristovski> mlankhorst: So I tested the patch and it passed all my
> manual tests, including vigorous_pointer_movement and
> spastic_window_repositioning, good enough for me!
>
Fair enough.

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Sat, Feb 25, 2017 at 04:31:04PM +0100, Maarten Lankhorst wrote:
> Op 24-02-17 om 14:11 schreef Ville Syrjälä:
> > On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
> >> 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@linux.intel.com:
> >>>>> From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> >>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> >>>>> Reported-by: Uwe Kleine-König <uwe@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@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.
> > We'd have bigger problems than the modifier if we want to use a sprite
> > plane as the cursor because for sprite planes the watermarks are
> > computed based on the clipped size. So the wm code would need some
> > surgery as well.
> >
> >> Cc'ing Ristovski for testing the patch. :)
> > 17:57 < Ristovski> mlankhorst: So I tested the patch and it passed all my
> > manual tests, including vigorous_pointer_movement and
> > spastic_window_repositioning, good enough for me!
> >
> Fair enough.
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Pushed to dinq. Thanks for the review and testing.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ander Conselvan De Oliveira]

On Thu, 2017-03-02 at 16:58 +0200, Ville Syrjälä wrote:
> On Sat, Feb 25, 2017 at 04:31:04PM +0100, Maarten Lankhorst wrote:
> > Op 24-02-17 om 14:11 schreef Ville Syrjälä:
> > > On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
> > > > 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@linux.intel.com:
> > > > > > > From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > > Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > > > Reported-by: Uwe Kleine-König <uwe@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@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.
> > > 
> > > We'd have bigger problems than the modifier if we want to use a sprite
> > > plane as the cursor because for sprite planes the watermarks are
> > > computed based on the clipped size. So the wm code would need some
> > > surgery as well.
> > > 
> > > > Cc'ing Ristovski for testing the patch. :)
> > > 
> > > 17:57 < Ristovski> mlankhorst: So I tested the patch and it passed all my
> > > manual tests, including vigorous_pointer_movement and
> > > spastic_window_repositioning, good enough for me!
> > > 
> > 
> > Fair enough.
> > 
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Pushed to dinq. Thanks for the review and testing.

This patch causes FIFO underruns on gen9 devices with kms_cursor_crc *-random
tests. The problem seems to happen when the cursor plane gets enabled after
being disabled because the cursor was completely offscreen. With the patch
reverted the enable goes through the slow path, which doesn't cause an underrun.


Ander

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Tue, Mar 21, 2017 at 03:42:53PM +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2017-03-02 at 16:58 +0200, Ville Syrjälä wrote:
> > On Sat, Feb 25, 2017 at 04:31:04PM +0100, Maarten Lankhorst wrote:
> > > Op 24-02-17 om 14:11 schreef Ville Syrjälä:
> > > > On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
> > > > > 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@linux.intel.com:
> > > > > > > > From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> > > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > > > Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > > > > Reported-by: Uwe Kleine-König <uwe@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@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.
> > > > 
> > > > We'd have bigger problems than the modifier if we want to use a sprite
> > > > plane as the cursor because for sprite planes the watermarks are
> > > > computed based on the clipped size. So the wm code would need some
> > > > surgery as well.
> > > > 
> > > > > Cc'ing Ristovski for testing the patch. :)
> > > > 
> > > > 17:57 < Ristovski> mlankhorst: So I tested the patch and it passed all my
> > > > manual tests, including vigorous_pointer_movement and
> > > > spastic_window_repositioning, good enough for me!
> > > > 
> > > 
> > > Fair enough.
> > > 
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> > Pushed to dinq. Thanks for the review and testing.
> 
> This patch causes FIFO underruns on gen9 devices with kms_cursor_crc *-random
> tests. The problem seems to happen when the cursor plane gets enabled after
> being disabled because the cursor was completely offscreen. With the patch
> reverted the enable goes through the slow path, which doesn't cause an underrun.

https://patchwork.freedesktop.org/series/21226/#rev1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: reduce cursor size for GEN5 hardware

[Patchwork]

== Series Details ==

Series: drm/i915: reduce cursor size for GEN5 hardware
URL   : https://patchwork.freedesktop.org/series/18822/
State : warning

== Summary ==

Series 18822v1 drm/i915: reduce cursor size for GEN5 hardware
https://patchwork.freedesktop.org/api/1.0/series/18822/revisions/1/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                dmesg-warn -> PASS       (fi-snb-2520m)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                dmesg-warn -> PASS       (fi-snb-2520m)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a:
                dmesg-warn -> PASS       (fi-snb-2520m)
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> DMESG-WARN (fi-skl-6770hq)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:78   pass:65   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:223  dwarn:0   dfail:0   fail:2   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:221  dwarn:4   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:232  dwarn:1   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

123d798c350471aba7e0625c154c6d9e395756c8 drm-tip: 2017y-01m-30d-21h-14m-37s UTC integration manifest
2ed3050 drm/i915: reduce cursor size for GEN5 hardware

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3649/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: reduce cursor size for GEN5 hardware (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: reduce cursor size for GEN5 hardware (rev4)
URL   : https://patchwork.freedesktop.org/series/18822/
State : success

== Summary ==

Series 18822v4 drm/i915: reduce cursor size for GEN5 hardware
https://patchwork.freedesktop.org/api/1.0/series/18822/revisions/4/mbox/

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

02022d17a5787709617b7897de3906970e2b0721 drm-tip: 2017y-02m-17d-15h-15m-45s UTC integration manifest
1ad4f51 drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3883/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx