A bisect showed that commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
introduced the issue.
The issue starts as soon as X takes control of the screen, even if just
a plain X doing nothing, so based on the code touched by the commit I
thought it had to be related to the so called "hardware cursor". I
confirmed it when hiding the cursor made the flickering go away.
The aforementioned commit removed some suspicious code, and the
Programmer's Reference Manual confirmed my suspicion:
"Incorrectly programmed watermark values can result in screen corruption.
The watermarks should be calculated and programmed when any of the
watermark calculation inputs change. This includes planes enabling or
disabling, plane source format or size changing, etc."
So I'm re-adding the few lines that update the watermarks after a cursor
size change.
Signed-off-by: Ismael Luceno <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f75173c..e23f062 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12258,6 +12258,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
struct intel_crtc *intel_crtc;
struct intel_plane *intel_plane = to_intel_plane(plane);
struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
+ unsigned old_width;
uint32_t addr;
crtc = crtc ? crtc : plane->crtc;
@@ -12282,11 +12283,15 @@ intel_commit_cursor_plane(struct drm_plane *plane,
intel_crtc->cursor_addr = addr;
intel_crtc->cursor_bo = obj;
update:
+ old_width = intel_crtc->cursor_width;
intel_crtc->cursor_width = state->base.crtc_w;
intel_crtc->cursor_height = state->base.crtc_h;
- if (intel_crtc->active)
+ if (intel_crtc->active) {
+ if (old_width != intel_crtc->cursor_width)
+ intel_update_watermarks(crtc);
intel_crtc_update_cursor(crtc, state->visible);
+ }
}
static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
--
2.3.4
[cc'ing the authors]
On Sat, Apr 11, 2015 at 07:40:34PM -0300, Ismael Luceno wrote:
> A bisect showed that commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
> introduced the issue.
>
> The issue starts as soon as X takes control of the screen, even if just
> a plain X doing nothing, so based on the code touched by the commit I
> thought it had to be related to the so called "hardware cursor". I
> confirmed it when hiding the cursor made the flickering go away.
>
> The aforementioned commit removed some suspicious code, and the
> Programmer's Reference Manual confirmed my suspicion:
>
> "Incorrectly programmed watermark values can result in screen corruption.
>
> The watermarks should be calculated and programmed when any of the
> watermark calculation inputs change. This includes planes enabling or
> disabling, plane source format or size changing, etc."
>
> So I'm re-adding the few lines that update the watermarks after a cursor
> size change.
>
> Signed-off-by: Ismael Luceno <[email protected]>
> ---
> drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f75173c..e23f062 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12258,6 +12258,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> struct intel_crtc *intel_crtc;
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> + unsigned old_width;
> uint32_t addr;
>
> crtc = crtc ? crtc : plane->crtc;
> @@ -12282,11 +12283,15 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> intel_crtc->cursor_addr = addr;
> intel_crtc->cursor_bo = obj;
> update:
> + old_width = intel_crtc->cursor_width;
> intel_crtc->cursor_width = state->base.crtc_w;
> intel_crtc->cursor_height = state->base.crtc_h;
>
> - if (intel_crtc->active)
> + if (intel_crtc->active) {
> + if (old_width != intel_crtc->cursor_width)
> + intel_update_watermarks(crtc);
> intel_crtc_update_cursor(crtc, state->visible);
> + }
> }
>
> static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> --
> 2.3.4
--
Chris Wilson, Intel Open Source Technology Centre
On Thu, 23 Apr 2015, Chris Wilson <[email protected]> wrote:
> [cc'ing the authors]
This has been posted earlier [1] and it has review to be addressed [2].
BR,
Jani.
[1] http://mid.gmane.org/[email protected]
[2] http://mid.gmane.org/[email protected]
>
> On Sat, Apr 11, 2015 at 07:40:34PM -0300, Ismael Luceno wrote:
>> A bisect showed that commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
>> introduced the issue.
>>
>> The issue starts as soon as X takes control of the screen, even if just
>> a plain X doing nothing, so based on the code touched by the commit I
>> thought it had to be related to the so called "hardware cursor". I
>> confirmed it when hiding the cursor made the flickering go away.
>>
>> The aforementioned commit removed some suspicious code, and the
>> Programmer's Reference Manual confirmed my suspicion:
>>
>> "Incorrectly programmed watermark values can result in screen corruption.
>>
>> The watermarks should be calculated and programmed when any of the
>> watermark calculation inputs change. This includes planes enabling or
>> disabling, plane source format or size changing, etc."
>>
>> So I'm re-adding the few lines that update the watermarks after a cursor
>> size change.
>>
>> Signed-off-by: Ismael Luceno <[email protected]>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index f75173c..e23f062 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12258,6 +12258,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>> struct intel_crtc *intel_crtc;
>> struct intel_plane *intel_plane = to_intel_plane(plane);
>> struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
>> + unsigned old_width;
>> uint32_t addr;
>>
>> crtc = crtc ? crtc : plane->crtc;
>> @@ -12282,11 +12283,15 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>> intel_crtc->cursor_addr = addr;
>> intel_crtc->cursor_bo = obj;
>> update:
>> + old_width = intel_crtc->cursor_width;
>> intel_crtc->cursor_width = state->base.crtc_w;
>> intel_crtc->cursor_height = state->base.crtc_h;
>>
>> - if (intel_crtc->active)
>> + if (intel_crtc->active) {
>> + if (old_width != intel_crtc->cursor_width)
>> + intel_update_watermarks(crtc);
>> intel_crtc_update_cursor(crtc, state->visible);
>> + }
>> }
>>
>> static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>> --
>> 2.3.4
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
> On Thu, 23 Apr 2015, Chris Wilson <[email protected]> wrote:
> > [cc'ing the authors]
>
> This has been posted earlier [1] and it has review to be addressed [2].
>
> BR,
> Jani.
I agree with Ander's response in [2]...we can't call
intel_update_watermarks() in the commit function because we're under
vblank evasion. We should already be flagging the transaction as
needing a watermark update in intel_check_cursor_plane(), and that flag
will be acted upon immediately after the commit functions are done
running, once we've re-enabled interrupts.
Note that our current codebase looks a bit different since we've dropped
intel_crtc->cursor_{width,height}. So the relevant check in
intel_check_cursor_plane() now looks like:
if (plane->state->crtc_w != state->base.crtc_w)
intel_crtc->atomic.update_wm = true;
Is there a bugzilla open on this issue with more details?
Matt
>
>
> [1] http://mid.gmane.org/[email protected]
> [2] http://mid.gmane.org/[email protected]
>
>
> >
> > On Sat, Apr 11, 2015 at 07:40:34PM -0300, Ismael Luceno wrote:
> >> A bisect showed that commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
> >> introduced the issue.
> >>
> >> The issue starts as soon as X takes control of the screen, even if just
> >> a plain X doing nothing, so based on the code touched by the commit I
> >> thought it had to be related to the so called "hardware cursor". I
> >> confirmed it when hiding the cursor made the flickering go away.
> >>
> >> The aforementioned commit removed some suspicious code, and the
> >> Programmer's Reference Manual confirmed my suspicion:
> >>
> >> "Incorrectly programmed watermark values can result in screen corruption.
> >>
> >> The watermarks should be calculated and programmed when any of the
> >> watermark calculation inputs change. This includes planes enabling or
> >> disabling, plane source format or size changing, etc."
> >>
> >> So I'm re-adding the few lines that update the watermarks after a cursor
> >> size change.
> >>
> >> Signed-off-by: Ismael Luceno <[email protected]>
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index f75173c..e23f062 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12258,6 +12258,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> >> struct intel_crtc *intel_crtc;
> >> struct intel_plane *intel_plane = to_intel_plane(plane);
> >> struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> >> + unsigned old_width;
> >> uint32_t addr;
> >>
> >> crtc = crtc ? crtc : plane->crtc;
> >> @@ -12282,11 +12283,15 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> >> intel_crtc->cursor_addr = addr;
> >> intel_crtc->cursor_bo = obj;
> >> update:
> >> + old_width = intel_crtc->cursor_width;
> >> intel_crtc->cursor_width = state->base.crtc_w;
> >> intel_crtc->cursor_height = state->base.crtc_h;
> >>
> >> - if (intel_crtc->active)
> >> + if (intel_crtc->active) {
> >> + if (old_width != intel_crtc->cursor_width)
> >> + intel_update_watermarks(crtc);
> >> intel_crtc_update_cursor(crtc, state->visible);
> >> + }
> >> }
> >>
> >> static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> >> --
> >> 2.3.4
> >
> > --
> > Chris Wilson, Intel Open Source Technology Centre
> > _______________________________________________
> > Intel-gfx mailing list
> > [email protected]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
On Thu, 07 May 2015, Matt Roper <[email protected]> wrote:
> On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
>> On Thu, 23 Apr 2015, Chris Wilson <[email protected]> wrote:
>> > [cc'ing the authors]
>>
>> This has been posted earlier [1] and it has review to be addressed [2].
>>
>> BR,
>> Jani.
>
> I agree with Ander's response in [2]...we can't call
> intel_update_watermarks() in the commit function because we're under
> vblank evasion. We should already be flagging the transaction as
> needing a watermark update in intel_check_cursor_plane(), and that flag
> will be acted upon immediately after the commit functions are done
> running, once we've re-enabled interrupts.
>
> Note that our current codebase looks a bit different since we've dropped
> intel_crtc->cursor_{width,height}. So the relevant check in
> intel_check_cursor_plane() now looks like:
>
> if (plane->state->crtc_w != state->base.crtc_w)
> intel_crtc->atomic.update_wm = true;
>
> Is there a bugzilla open on this issue with more details?
Not that I know of. Ismael?
BR,
Jani.
>
>
> Matt
>
>>
>>
>> [1] http://mid.gmane.org/[email protected]
>> [2] http://mid.gmane.org/[email protected]
>>
>>
>> >
>> > On Sat, Apr 11, 2015 at 07:40:34PM -0300, Ismael Luceno wrote:
>> >> A bisect showed that commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
>> >> introduced the issue.
>> >>
>> >> The issue starts as soon as X takes control of the screen, even if just
>> >> a plain X doing nothing, so based on the code touched by the commit I
>> >> thought it had to be related to the so called "hardware cursor". I
>> >> confirmed it when hiding the cursor made the flickering go away.
>> >>
>> >> The aforementioned commit removed some suspicious code, and the
>> >> Programmer's Reference Manual confirmed my suspicion:
>> >>
>> >> "Incorrectly programmed watermark values can result in screen corruption.
>> >>
>> >> The watermarks should be calculated and programmed when any of the
>> >> watermark calculation inputs change. This includes planes enabling or
>> >> disabling, plane source format or size changing, etc."
>> >>
>> >> So I'm re-adding the few lines that update the watermarks after a cursor
>> >> size change.
>> >>
>> >> Signed-off-by: Ismael Luceno <[email protected]>
>> >> ---
>> >> drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
>> >> 1 file changed, 6 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> >> index f75173c..e23f062 100644
>> >> --- a/drivers/gpu/drm/i915/intel_display.c
>> >> +++ b/drivers/gpu/drm/i915/intel_display.c
>> >> @@ -12258,6 +12258,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>> >> struct intel_crtc *intel_crtc;
>> >> struct intel_plane *intel_plane = to_intel_plane(plane);
>> >> struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
>> >> + unsigned old_width;
>> >> uint32_t addr;
>> >>
>> >> crtc = crtc ? crtc : plane->crtc;
>> >> @@ -12282,11 +12283,15 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>> >> intel_crtc->cursor_addr = addr;
>> >> intel_crtc->cursor_bo = obj;
>> >> update:
>> >> + old_width = intel_crtc->cursor_width;
>> >> intel_crtc->cursor_width = state->base.crtc_w;
>> >> intel_crtc->cursor_height = state->base.crtc_h;
>> >>
>> >> - if (intel_crtc->active)
>> >> + if (intel_crtc->active) {
>> >> + if (old_width != intel_crtc->cursor_width)
>> >> + intel_update_watermarks(crtc);
>> >> intel_crtc_update_cursor(crtc, state->visible);
>> >> + }
>> >> }
>> >>
>> >> static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>> >> --
>> >> 2.3.4
>> >
>> > --
>> > Chris Wilson, Intel Open Source Technology Centre
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > [email protected]
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Jani Nikula, Intel Open Source Technology Center
On Thu, May 07, 2015 at 04:41:48PM +0300, Jani Nikula wrote:
> On Thu, 07 May 2015, Matt Roper <[email protected]> wrote:
> > On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
> >> On Thu, 23 Apr 2015, Chris Wilson <[email protected]> wrote:
> >> > [cc'ing the authors]
> >>
> >> This has been posted earlier [1] and it has review to be addressed [2].
> >>
> >> BR,
> >> Jani.
> >
> > I agree with Ander's response in [2]...we can't call
> > intel_update_watermarks() in the commit function because we're under
> > vblank evasion. We should already be flagging the transaction as
> > needing a watermark update in intel_check_cursor_plane(), and that flag
> > will be acted upon immediately after the commit functions are done
> > running, once we've re-enabled interrupts.
> >
> > Note that our current codebase looks a bit different since we've dropped
> > intel_crtc->cursor_{width,height}. So the relevant check in
> > intel_check_cursor_plane() now looks like:
> >
> > if (plane->state->crtc_w != state->base.crtc_w)
> > intel_crtc->atomic.update_wm = true;
> >
> > Is there a bugzilla open on this issue with more details?
>
> Not that I know of. Ismael?
>
Probably:
https://bugs.freedesktop.org/show_bug.cgi?id=88944
https://bugzilla.redhat.com/show_bug.cgi?id=1199890
are related.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
On Thu, 7 May 2015 15:18:27 +0100
Chris Wilson <[email protected]> wrote:
> On Thu, May 07, 2015 at 04:41:48PM +0300, Jani Nikula wrote:
> > On Thu, 07 May 2015, Matt Roper <[email protected]> wrote:
> > > On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
> > >> On Thu, 23 Apr 2015, Chris Wilson <[email protected]>
> > >> wrote:
> > >> > [cc'ing the authors]
> > >>
> > >> This has been posted earlier [1] and it has review to be
> > >> addressed [2].
> > >>
> > >> BR,
> > >> Jani.
> > >
> > > I agree with Ander's response in [2]...we can't call
> > > intel_update_watermarks() in the commit function because we're
> > > under vblank evasion. We should already be flagging the
> > > transaction as needing a watermark update in
> > > intel_check_cursor_plane(), and that flag will be acted upon
> > > immediately after the commit functions are done running, once
> > > we've re-enabled interrupts.
> > >
> > > Note that our current codebase looks a bit different since we've
> > > dropped intel_crtc->cursor_{width,height}. So the relevant check
> > > in intel_check_cursor_plane() now looks like:
> > >
> > > if (plane->state->crtc_w != state->base.crtc_w)
> > > intel_crtc->atomic.update_wm = true;
> > >
> > > Is there a bugzilla open on this issue with more details?
> >
> > Not that I know of. Ismael?
> >
>
> Probably:
> https://bugs.freedesktop.org/show_bug.cgi?id=88944
> https://bugzilla.redhat.com/show_bug.cgi?id=1199890
> are related.
> -Chris
>
I am experiencing neither of those things.
On Thu, 07 May 2015 16:41:48 +0300
Jani Nikula <[email protected]> wrote:
> On Thu, 07 May 2015, Matt Roper <[email protected]> wrote:
> > On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
> >> On Thu, 23 Apr 2015, Chris Wilson <[email protected]> wrote:
> >> > [cc'ing the authors]
> >>
> >> This has been posted earlier [1] and it has review to be addressed
> >> [2].
> >>
> >> BR,
> >> Jani.
> >
> > I agree with Ander's response in [2]...we can't call
> > intel_update_watermarks() in the commit function because we're under
> > vblank evasion. We should already be flagging the transaction as
> > needing a watermark update in intel_check_cursor_plane(), and that
> > flag will be acted upon immediately after the commit functions are
> > done running, once we've re-enabled interrupts.
> >
> > Note that our current codebase looks a bit different since we've
> > dropped intel_crtc->cursor_{width,height}. So the relevant check in
> > intel_check_cursor_plane() now looks like:
> >
> > if (plane->state->crtc_w != state->base.crtc_w)
> > intel_crtc->atomic.update_wm = true;
> >
> > Is there a bugzilla open on this issue with more details?
>
> Not that I know of. Ismael?
Didn't found one at the time.
I apologize for the lack of communication, have been too busy job
hunting these weeks.
Chris comments prompted me to double-check. It seems one of Matt's
commits solves the issue [0], it just didn't hit mainline until April
20 [1], long after v4.0.
[0] 3dd512fbda0d87d1c3fb44bf878b262baee98fb6
[1] 14aa02449064541217836b9f3d3295e241d5ae9c
On Fri, 8 May 2015 12:10:15 -0300
Ismael Luceno <[email protected]> wrote:
> On Thu, 07 May 2015 16:41:48 +0300
> Jani Nikula <[email protected]> wrote:
> > On Thu, 07 May 2015, Matt Roper <[email protected]> wrote:
> > > On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
> > >> On Thu, 23 Apr 2015, Chris Wilson <[email protected]>
> > >> wrote:
> > >> > [cc'ing the authors]
> > >>
> > >> This has been posted earlier [1] and it has review to be
> > >> addressed [2].
> > >>
> > >> BR,
> > >> Jani.
> > >
> > > I agree with Ander's response in [2]...we can't call
> > > intel_update_watermarks() in the commit function because we're
> > > under vblank evasion. We should already be flagging the
> > > transaction as needing a watermark update in
> > > intel_check_cursor_plane(), and that flag will be acted upon
> > > immediately after the commit functions are done running, once
> > > we've re-enabled interrupts.
> > >
> > > Note that our current codebase looks a bit different since we've
> > > dropped intel_crtc->cursor_{width,height}. So the relevant check
> > > in intel_check_cursor_plane() now looks like:
> > >
> > > if (plane->state->crtc_w != state->base.crtc_w)
> > > intel_crtc->atomic.update_wm = true;
> > >
> > > Is there a bugzilla open on this issue with more details?
> >
> > Not that I know of. Ismael?
>
> Didn't found one at the time.
>
> I apologize for the lack of communication, have been too busy job
> hunting these weeks.
>
> Chris comments prompted me to double-check. It seems one of Matt's
> commits solves the issue [0], it just didn't hit mainline until April
> 20 [1], long after v4.0.
>
> [0] 3dd512fbda0d87d1c3fb44bf878b262baee98fb6
> [1] 14aa02449064541217836b9f3d3295e241d5ae9c
Sorry, meant Matt's comment about the current codebase; my brain seems
not to be working well today.