All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Fix screen flickering on X
@ 2015-04-11 22:17 Ismael Luceno
  2015-04-13 12:33 ` Ander Conselvan De Oliveira
  0 siblings, 1 reply; 11+ messages in thread
From: Ismael Luceno @ 2015-04-11 22:17 UTC (permalink / raw)
  To: Matt Roper; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, Ismael Luceno

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 <ismael@iodev.co.uk>
---
 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

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Fix screen flickering on X
  2015-04-11 22:17 [PATCH] drm/i915: Fix screen flickering on X Ismael Luceno
@ 2015-04-13 12:33 ` Ander Conselvan De Oliveira
  0 siblings, 0 replies; 11+ messages in thread
From: Ander Conselvan De Oliveira @ 2015-04-13 12:33 UTC (permalink / raw)
  To: Ismael Luceno; +Cc: Jani Nikula, Daniel Vetter, intel-gfx

On Sat, 2015-04-11 at 19:17 -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 <ismael@iodev.co.uk>
> ---
>  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);
> +	}


We can't call intel_update_watermarks() from here because we commit
under vblank evasion (with interrupts disabled) and some parts of that
function might sleep. That's why that call to it was moved to
intel_begin_crtc_commit(). The check for change in width was moved to
intel_check_cursor_plane() (where the intel_crtc->atomic.update_wm flag
is set).

So the question here is why that logic is not working. Could you check
if update_wm is being set and intel_update_watermarks() called from
intel_begin_crtc_commit()?

Thanks,
Ander

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Fix screen flickering on X
  2015-05-08 15:10         ` Ismael Luceno
@ 2015-05-08 15:16           ` Ismael Luceno
  0 siblings, 0 replies; 11+ messages in thread
From: Ismael Luceno @ 2015-05-08 15:16 UTC (permalink / raw)
  To: Jani Nikula, Chris Wilson; +Cc: intel-gfx, linux-kernel

On Fri, 8 May 2015 12:10:15 -0300
Ismael Luceno <ismael@iodev.co.uk> wrote:
> On Thu, 07 May 2015 16:41:48 +0300
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Thu, 07 May 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> > > On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
> > >> On Thu, 23 Apr 2015, Chris Wilson <chris@chris-wilson.co.uk>
> > >> 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Fix screen flickering on X
  2015-05-07 13:41       ` Jani Nikula
  2015-05-07 14:18         ` [Intel-gfx] " Chris Wilson
@ 2015-05-08 15:10         ` Ismael Luceno
  2015-05-08 15:16           ` Ismael Luceno
  1 sibling, 1 reply; 11+ messages in thread
From: Ismael Luceno @ 2015-05-08 15:10 UTC (permalink / raw)
  To: Jani Nikula, Chris Wilson; +Cc: intel-gfx, linux-kernel

On Thu, 07 May 2015 16:41:48 +0300
Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 07 May 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> > On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
> >> On Thu, 23 Apr 2015, Chris Wilson <chris@chris-wilson.co.uk> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Fix screen flickering on X
  2015-05-07 14:18         ` [Intel-gfx] " Chris Wilson
@ 2015-05-07 21:23           ` Ismael Luceno
  0 siblings, 0 replies; 11+ messages in thread
From: Ismael Luceno @ 2015-05-07 21:23 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, linux-kernel

On Thu, 7 May 2015 15:18:27 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, May 07, 2015 at 04:41:48PM +0300, Jani Nikula wrote:
> > On Thu, 07 May 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> > > On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
> > >> On Thu, 23 Apr 2015, Chris Wilson <chris@chris-wilson.co.uk>
> > >> 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Fix screen flickering on X
  2015-05-07 13:27     ` Matt Roper
@ 2015-05-07 13:41       ` Jani Nikula
  2015-05-07 14:18         ` [Intel-gfx] " Chris Wilson
  2015-05-08 15:10         ` Ismael Luceno
  0 siblings, 2 replies; 11+ messages in thread
From: Jani Nikula @ 2015-05-07 13:41 UTC (permalink / raw)
  To: Matt Roper, Ismael Luceno; +Cc: intel-gfx, linux-kernel

On Thu, 07 May 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
>> On Thu, 23 Apr 2015, Chris Wilson <chris@chris-wilson.co.uk> 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/1428790644-6812-1-git-send-email-ismael@iodev.co.uk
>> [2] http://mid.gmane.org/1428928418.2654.8.camel@gmail.com
>> 
>> 
>> >
>> > 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 <ismael@iodev.co.uk>
>> >> ---
>> >>  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
>> > Intel-gfx@lists.freedesktop.org
>> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Fix screen flickering on X
  2015-05-07  9:12   ` Jani Nikula
@ 2015-05-07 13:27     ` Matt Roper
  2015-05-07 13:41       ` Jani Nikula
  0 siblings, 1 reply; 11+ messages in thread
From: Matt Roper @ 2015-05-07 13:27 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, linux-kernel

On Thu, May 07, 2015 at 12:12:18PM +0300, Jani Nikula wrote:
> On Thu, 23 Apr 2015, Chris Wilson <chris@chris-wilson.co.uk> 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/1428790644-6812-1-git-send-email-ismael@iodev.co.uk
> [2] http://mid.gmane.org/1428928418.2654.8.camel@gmail.com
> 
> 
> >
> > 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 <ismael@iodev.co.uk>
> >> ---
> >>  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
> > Intel-gfx@lists.freedesktop.org
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Fix screen flickering on X
  2015-04-23  9:15   ` Chris Wilson
  (?)
@ 2015-05-07  9:12   ` Jani Nikula
  2015-05-07 13:27     ` Matt Roper
  -1 siblings, 1 reply; 11+ messages in thread
From: Jani Nikula @ 2015-05-07  9:12 UTC (permalink / raw)
  To: Chris Wilson, Matt Roper; +Cc: intel-gfx, linux-kernel

On Thu, 23 Apr 2015, Chris Wilson <chris@chris-wilson.co.uk> 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/1428790644-6812-1-git-send-email-ismael@iodev.co.uk
[2] http://mid.gmane.org/1428928418.2654.8.camel@gmail.com


>
> 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 <ismael@iodev.co.uk>
>> ---
>>  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
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Fix screen flickering on X
  2015-04-11 22:40 Ismael Luceno
@ 2015-04-23  9:15   ` Chris Wilson
  0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2015-04-23  9:15 UTC (permalink / raw)
  To: Matt Roper; +Cc: linux-kernel, intel-gfx

[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 <ismael@iodev.co.uk>
> ---
>  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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Fix screen flickering on X
@ 2015-04-23  9:15   ` Chris Wilson
  0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2015-04-23  9:15 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx, linux-kernel

[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 <ismael@iodev.co.uk>
> ---
>  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
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] drm/i915: Fix screen flickering on X
@ 2015-04-11 22:40 Ismael Luceno
  2015-04-23  9:15   ` Chris Wilson
  0 siblings, 1 reply; 11+ messages in thread
From: Ismael Luceno @ 2015-04-11 22:40 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ismael Luceno

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 <ismael@iodev.co.uk>
---
 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-05-08 15:17 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-11 22:17 [PATCH] drm/i915: Fix screen flickering on X Ismael Luceno
2015-04-13 12:33 ` Ander Conselvan De Oliveira
2015-04-11 22:40 Ismael Luceno
2015-04-23  9:15 ` Chris Wilson
2015-04-23  9:15   ` Chris Wilson
2015-05-07  9:12   ` Jani Nikula
2015-05-07 13:27     ` Matt Roper
2015-05-07 13:41       ` Jani Nikula
2015-05-07 14:18         ` [Intel-gfx] " Chris Wilson
2015-05-07 21:23           ` Ismael Luceno
2015-05-08 15:10         ` Ismael Luceno
2015-05-08 15:16           ` Ismael Luceno

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.