drm/i915: Fix screen flickering on X

Submitted by Ismael Luceno on April 11, 2015, 10:17 p.m.

Details

Message ID 1428790644-6812-1-git-send-email-ismael@iodev.co.uk
State New
Headers show

Not browsing as part of any series.

Commit Message

Ismael Luceno April 11, 2015, 10:17 p.m.
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(-)

Patch hide | download patch | download mbox

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,

Comments

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