* [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware @ 2017-01-31 8:09 Uwe Kleine-König 2017-01-31 9:03 ` Maarten Lankhorst ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Uwe Kleine-König @ 2017-01-31 8:09 UTC (permalink / raw) To: Daniel Vetter, Jani Nikula; +Cc: intel-gfx, Martin Peres 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware 2017-01-31 8:09 [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware Uwe Kleine-König @ 2017-01-31 9:03 ` Maarten Lankhorst 2017-01-31 19:13 ` Uwe Kleine-König 2017-01-31 9:25 ` ✗ Fi.CI.BAT: warning for drm/i915: reduce cursor size for GEN5 hardware Patchwork 2017-02-17 17:52 ` ✓ Fi.CI.BAT: success for drm/i915: reduce cursor size for GEN5 hardware (rev4) Patchwork 2 siblings, 1 reply; 24+ messages in thread From: Maarten Lankhorst @ 2017-01-31 9:03 UTC (permalink / raw) To: Uwe Kleine-König, Daniel Vetter, Jani Nikula; +Cc: intel-gfx, Martin Peres 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware 2017-01-31 9:03 ` Maarten Lankhorst @ 2017-01-31 19:13 ` Uwe Kleine-König 2017-02-01 12:41 ` Maarten Lankhorst 0 siblings, 1 reply; 24+ messages in thread From: Uwe Kleine-König @ 2017-01-31 19:13 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx, Martin Peres, Daniel Vetter [-- 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware 2017-01-31 19:13 ` Uwe Kleine-König @ 2017-02-01 12:41 ` Maarten Lankhorst 2017-02-01 14:37 ` Ville Syrjälä 0 siblings, 1 reply; 24+ messages in thread From: Maarten Lankhorst @ 2017-02-01 12:41 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: intel-gfx, Martin Peres, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware 2017-02-01 12:41 ` Maarten Lankhorst @ 2017-02-01 14:37 ` Ville Syrjälä 2017-02-07 13:22 ` Uwe Kleine-König 0 siblings, 1 reply; 24+ messages in thread From: Ville Syrjälä @ 2017-02-01 14:37 UTC (permalink / raw) To: Maarten Lankhorst Cc: Uwe Kleine-König, intel-gfx, Martin Peres, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware 2017-02-01 14:37 ` Ville Syrjälä @ 2017-02-07 13:22 ` Uwe Kleine-König 2017-02-07 15:03 ` Martin Peres 0 siblings, 1 reply; 24+ messages in thread From: Uwe Kleine-König @ 2017-02-07 13:22 UTC (permalink / raw) To: Ville Syrjälä, Maarten Lankhorst Cc: intel-gfx, Martin Peres, Daniel Vetter [-- 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware 2017-02-07 13:22 ` Uwe Kleine-König @ 2017-02-07 15:03 ` Martin Peres 2017-02-07 15:35 ` Ville Syrjälä 0 siblings, 1 reply; 24+ messages in thread From: Martin Peres @ 2017-02-07 15:03 UTC (permalink / raw) To: Uwe Kleine-König, Ville Syrjälä, Maarten Lankhorst Cc: Daniel Vetter, intel-gfx 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware 2017-02-07 15:03 ` Martin Peres @ 2017-02-07 15:35 ` Ville Syrjälä 2017-02-07 17:51 ` Maarten Lankhorst 2017-02-17 11:10 ` Uwe Kleine-König 0 siblings, 2 replies; 24+ messages in thread From: Ville Syrjälä @ 2017-02-07 15:35 UTC (permalink / raw) To: Martin Peres; +Cc: Uwe Kleine-König, intel-gfx, Daniel Vetter 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware 2017-02-07 15:35 ` Ville Syrjälä @ 2017-02-07 17:51 ` Maarten Lankhorst 2017-02-07 17:58 ` Ville Syrjälä 2017-02-17 11:10 ` Uwe Kleine-König 1 sibling, 1 reply; 24+ messages in thread From: Maarten Lankhorst @ 2017-02-07 17:51 UTC (permalink / raw) To: Ville Syrjälä, Martin Peres Cc: Uwe Kleine-König, intel-gfx, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware 2017-02-07 17:51 ` Maarten Lankhorst @ 2017-02-07 17:58 ` Ville Syrjälä 0 siblings, 0 replies; 24+ messages in thread From: Ville Syrjälä @ 2017-02-07 17:58 UTC (permalink / raw) To: Maarten Lankhorst Cc: Uwe Kleine-König, intel-gfx, Martin Peres, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware 2017-02-07 15:35 ` Ville Syrjälä 2017-02-07 17:51 ` Maarten Lankhorst @ 2017-02-17 11:10 ` Uwe Kleine-König 2017-02-17 15:01 ` [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW ville.syrjala 1 sibling, 1 reply; 24+ messages in thread From: Uwe Kleine-König @ 2017-02-17 11:10 UTC (permalink / raw) To: Ville Syrjälä, Martin Peres; +Cc: intel-gfx, Daniel Vetter [-- 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-02-17 11:10 ` Uwe Kleine-König @ 2017-02-17 15:01 ` ville.syrjala 2017-02-17 20:04 ` Uwe Kleine-König 2017-02-20 9:04 ` Maarten Lankhorst 0 siblings, 2 replies; 24+ messages in thread From: ville.syrjala @ 2017-02-17 15:01 UTC (permalink / raw) To: intel-gfx; +Cc: Uwe Kleine-König, Daniel Vetter, Martin Peres 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-02-17 15:01 ` [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW ville.syrjala @ 2017-02-17 20:04 ` Uwe Kleine-König 2017-02-17 20:18 ` Ville Syrjälä 2017-02-20 9:04 ` Maarten Lankhorst 1 sibling, 1 reply; 24+ messages in thread From: Uwe Kleine-König @ 2017-02-17 20:04 UTC (permalink / raw) To: ville.syrjala; +Cc: Daniel Vetter, intel-gfx, Martin Peres [-- 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-02-17 20:04 ` Uwe Kleine-König @ 2017-02-17 20:18 ` Ville Syrjälä 0 siblings, 0 replies; 24+ messages in thread From: Ville Syrjälä @ 2017-02-17 20:18 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Daniel Vetter, intel-gfx, Martin Peres 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-02-17 15:01 ` [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW ville.syrjala 2017-02-17 20:04 ` Uwe Kleine-König @ 2017-02-20 9:04 ` Maarten Lankhorst 2017-02-20 13:38 ` Ville Syrjälä 1 sibling, 1 reply; 24+ messages in thread From: Maarten Lankhorst @ 2017-02-20 9:04 UTC (permalink / raw) To: ville.syrjala, intel-gfx Cc: Uwe Kleine-König, Daniel Vetter, Martin Peres 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-02-20 9:04 ` Maarten Lankhorst @ 2017-02-20 13:38 ` Ville Syrjälä 2017-02-20 14:30 ` Maarten Lankhorst 0 siblings, 1 reply; 24+ messages in thread From: Ville Syrjälä @ 2017-02-20 13:38 UTC (permalink / raw) To: Maarten Lankhorst Cc: Uwe Kleine-König, Daniel Vetter, intel-gfx, Martin Peres 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-02-20 13:38 ` Ville Syrjälä @ 2017-02-20 14:30 ` Maarten Lankhorst 2017-02-24 13:11 ` Ville Syrjälä 0 siblings, 1 reply; 24+ messages in thread From: Maarten Lankhorst @ 2017-02-20 14:30 UTC (permalink / raw) To: Ville Syrjälä Cc: Uwe Kleine-König, Daniel Vetter, intel-gfx, Martin Peres, Rafael Ristovski 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-02-20 14:30 ` Maarten Lankhorst @ 2017-02-24 13:11 ` Ville Syrjälä 2017-02-25 15:31 ` Maarten Lankhorst 0 siblings, 1 reply; 24+ messages in thread From: Ville Syrjälä @ 2017-02-24 13:11 UTC (permalink / raw) To: Maarten Lankhorst Cc: Uwe Kleine-König, Daniel Vetter, intel-gfx, Martin Peres, Rafael Ristovski 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-02-24 13:11 ` Ville Syrjälä @ 2017-02-25 15:31 ` Maarten Lankhorst 2017-03-02 14:58 ` Ville Syrjälä 0 siblings, 1 reply; 24+ messages in thread From: Maarten Lankhorst @ 2017-02-25 15:31 UTC (permalink / raw) To: Ville Syrjälä Cc: Uwe Kleine-König, Daniel Vetter, intel-gfx, Martin Peres, Rafael Ristovski 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-02-25 15:31 ` Maarten Lankhorst @ 2017-03-02 14:58 ` Ville Syrjälä 2017-03-21 13:42 ` Ander Conselvan De Oliveira 0 siblings, 1 reply; 24+ messages in thread From: Ville Syrjälä @ 2017-03-02 14:58 UTC (permalink / raw) To: Maarten Lankhorst Cc: Uwe Kleine-König, Daniel Vetter, intel-gfx, Martin Peres, Rafael Ristovski 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-03-02 14:58 ` Ville Syrjälä @ 2017-03-21 13:42 ` Ander Conselvan De Oliveira 2017-03-21 14:43 ` Ville Syrjälä 0 siblings, 1 reply; 24+ messages in thread From: Ander Conselvan De Oliveira @ 2017-03-21 13:42 UTC (permalink / raw) To: Ville Syrjälä, Maarten Lankhorst Cc: Daniel Vetter, intel-gfx, Rafael Ristovski, Martin Peres, Uwe Kleine-König 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW 2017-03-21 13:42 ` Ander Conselvan De Oliveira @ 2017-03-21 14:43 ` Ville Syrjälä 0 siblings, 0 replies; 24+ messages in thread From: Ville Syrjälä @ 2017-03-21 14:43 UTC (permalink / raw) To: Ander Conselvan De Oliveira Cc: Uwe Kleine-König, Daniel Vetter, intel-gfx, Martin Peres, Rafael Ristovski 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915: reduce cursor size for GEN5 hardware 2017-01-31 8:09 [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware Uwe Kleine-König 2017-01-31 9:03 ` Maarten Lankhorst @ 2017-01-31 9:25 ` Patchwork 2017-02-17 17:52 ` ✓ Fi.CI.BAT: success for drm/i915: reduce cursor size for GEN5 hardware (rev4) Patchwork 2 siblings, 0 replies; 24+ messages in thread From: Patchwork @ 2017-01-31 9:25 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: reduce cursor size for GEN5 hardware (rev4) 2017-01-31 8:09 [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware Uwe Kleine-König 2017-01-31 9:03 ` Maarten Lankhorst 2017-01-31 9:25 ` ✗ Fi.CI.BAT: warning for drm/i915: reduce cursor size for GEN5 hardware Patchwork @ 2017-02-17 17:52 ` Patchwork 2 siblings, 0 replies; 24+ messages in thread From: Patchwork @ 2017-02-17 17:52 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-03-21 14:43 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-31 8:09 [PATCH RFC] drm/i915: reduce cursor size for GEN5 hardware Uwe Kleine-König 2017-01-31 9:03 ` Maarten Lankhorst 2017-01-31 19:13 ` Uwe Kleine-König 2017-02-01 12:41 ` Maarten Lankhorst 2017-02-01 14:37 ` Ville Syrjälä 2017-02-07 13:22 ` Uwe Kleine-König 2017-02-07 15:03 ` Martin Peres 2017-02-07 15:35 ` Ville Syrjälä 2017-02-07 17:51 ` Maarten Lankhorst 2017-02-07 17:58 ` Ville Syrjälä 2017-02-17 11:10 ` Uwe Kleine-König 2017-02-17 15:01 ` [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW ville.syrjala 2017-02-17 20:04 ` Uwe Kleine-König 2017-02-17 20:18 ` Ville Syrjälä 2017-02-20 9:04 ` Maarten Lankhorst 2017-02-20 13:38 ` Ville Syrjälä 2017-02-20 14:30 ` Maarten Lankhorst 2017-02-24 13:11 ` Ville Syrjälä 2017-02-25 15:31 ` Maarten Lankhorst 2017-03-02 14:58 ` Ville Syrjälä 2017-03-21 13:42 ` Ander Conselvan De Oliveira 2017-03-21 14:43 ` Ville Syrjälä 2017-01-31 9:25 ` ✗ Fi.CI.BAT: warning for drm/i915: reduce cursor size for GEN5 hardware Patchwork 2017-02-17 17:52 ` ✓ Fi.CI.BAT: success for drm/i915: reduce cursor size for GEN5 hardware (rev4) Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.