[PATCH 0/2] drm/i915: Fix the invisible cursor syndrome

[PATCH 0/2] drm/i915: Fix the invisible cursor syndrome

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

These two should provide the minimal fix for the invisible cursor
problem that's been plaguing me and some other people. It's caused
by the cursor bo getting bound at ggtt offset 0, which the code then
considers to mean "not enabled". These patches get rid of that assumption,
with the caveat that we may do some needless cursor register writes.
I have a more generic fix for that (and a ton of other cursor improvements)
at [1], but I figured that I'll just post the critical fixes now and allow
the others to stew a bit more in the branch until maybe early next year.

I assume the problem is also related to the lack of fbdev pinning that
Chris has fixed [2]. With fbdev permanalty pinned at offset 0, there's
no opportunity for the cursor bo to travel there. Without fbdev I suppose
something else rings, status page, etc. could get permanently placed
at offset 0, so not sure if this can be hit in that case (didn't try it).

I'm a bit wary about adding cc stable to these since I suspect that
backporting them too far will cause regressions, on account of the
atomic/plane stuff probably not being up to the task in older kernels.
So maybe better get these in w/o cc stable and consider a backport again
later if it looks necessary.

[1] git://github.com/vsyrjala/linux.git cursor_improvements
[2] http://lists.freedesktop.org/archives/intel-gfx/2015-December/082114.html

Ville Syrjälä (2):
  drm/i915: Kill intel_crtc->cursor_bo
  drm/i915: Drop the broken cursor base==0 special casing

 drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++----------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 12 insertions(+), 23 deletions(-)

Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
-- 
2.4.10

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

[PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The vma may have been rebound between the last time the cursor was
enabled and now, so skipping the cursor gtt offset deduction is not
safe unless we would also reset cursor_bo to NULL when disabling the
cursor. Just thow cursor_bo to the bin instead since it's lost all
other uses thanks to universal plane support.

Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 -----
 drivers/gpu/drm/i915/intel_drv.h     | 1 -
 2 files changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e9bb860a55dc..f2a0151b3f14 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14105,9 +14105,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
-	if (intel_crtc->cursor_bo == obj)
-		goto update;
-
 	if (!obj)
 		addr = 0;
 	else if (!INTEL_INFO(dev)->cursor_needs_physical)
@@ -14116,9 +14113,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 		addr = obj->phys_handle->busaddr;
 
 	intel_crtc->cursor_addr = addr;
-	intel_crtc->cursor_bo = obj;
 
-update:
 	intel_crtc_update_cursor(crtc, state->visible);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 76dfa286cd95..6324c782d062 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -568,7 +568,6 @@ struct intel_crtc {
 	int adjusted_x;
 	int adjusted_y;
 
-	struct drm_i915_gem_object *cursor_bo;
 	uint32_t cursor_addr;
 	uint32_t cursor_cntl;
 	uint32_t cursor_size;
-- 
2.4.10

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

[PATCH 2/2] drm/i915: Drop the broken cursor base==0 special casing

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The cursor code tries to treat base==0 to mean disabled. That fails
when the cursor bo gets bound at ggtt offset 0, and the user is left
looking at an invisible cursor.

We lose the disabled->disabled optimization, but that seems like
something better handled at a slightly higher level.

Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f2a0151b3f14..ee84b517eb74 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10013,14 +10013,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	return true;
 }
 
-static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
+static void i845_update_cursor(struct drm_crtc *crtc, u32 base, bool on)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint32_t cntl = 0, size = 0;
 
-	if (base) {
+	if (on) {
 		unsigned int width = intel_crtc->base.cursor->state->crtc_w;
 		unsigned int height = intel_crtc->base.cursor->state->crtc_h;
 		unsigned int stride = roundup_pow_of_two(width) * 4;
@@ -10075,16 +10075,15 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
 	}
 }
 
-static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
+static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base, bool on)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
-	uint32_t cntl;
+	uint32_t cntl = 0;
 
-	cntl = 0;
-	if (base) {
+	if (on) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (intel_crtc->base.cursor->state->crtc_w) {
 			case 64:
@@ -10135,18 +10134,17 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	int y = cursor_state->crtc_y;
 	u32 base = 0, pos = 0;
 
-	if (on)
-		base = intel_crtc->cursor_addr;
+	base = intel_crtc->cursor_addr;
 
 	if (x >= intel_crtc->config->pipe_src_w)
-		base = 0;
+		on = false;
 
 	if (y >= intel_crtc->config->pipe_src_h)
-		base = 0;
+		on = false;
 
 	if (x < 0) {
 		if (x + cursor_state->crtc_w <= 0)
-			base = 0;
+			on = false;
 
 		pos |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
 		x = -x;
@@ -10155,16 +10153,13 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 
 	if (y < 0) {
 		if (y + cursor_state->crtc_h <= 0)
-			base = 0;
+			on = false;
 
 		pos |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
 		y = -y;
 	}
 	pos |= y << CURSOR_Y_SHIFT;
 
-	if (base == 0 && intel_crtc->cursor_base == 0)
-		return;
-
 	I915_WRITE(CURPOS(pipe), pos);
 
 	/* ILK+ do this automagically */
@@ -10175,9 +10170,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	}
 
 	if (IS_845G(dev) || IS_I865G(dev))
-		i845_update_cursor(crtc, base);
+		i845_update_cursor(crtc, base, on);
 	else
-		i9xx_update_cursor(crtc, base);
+		i9xx_update_cursor(crtc, base, on);
 }
 
 static bool cursor_size_ok(struct drm_device *dev,
-- 
2.4.10

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

Re: [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo

[Chris Wilson]

On Mon, Dec 14, 2015 at 01:16:47PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The vma may have been rebound between the last time the cursor was
> enabled and now, so skipping the cursor gtt offset deduction is not
> safe unless we would also reset cursor_bo to NULL when disabling the
> cursor. Just thow cursor_bo to the bin instead since it's lost all
> other uses thanks to universal plane support.

You could also mention that since updating the cursor is so slow now
through the universal-plane support, that a fast path to avoid a few
writes/checks is also irrelevant.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Drop the broken cursor base==0 special casing

[Chris Wilson]

On Mon, Dec 14, 2015 at 01:16:48PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The cursor code tries to treat base==0 to mean disabled. That fails
> when the cursor bo gets bound at ggtt offset 0, and the user is left
> looking at an invisible cursor.
> 
> We lose the disabled->disabled optimization, but that seems like
> something better handled at a slightly higher level.
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 11:32:23AM +0000, Chris Wilson wrote:
> On Mon, Dec 14, 2015 at 01:16:47PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The vma may have been rebound between the last time the cursor was
> > enabled and now, so skipping the cursor gtt offset deduction is not
> > safe unless we would also reset cursor_bo to NULL when disabling the
> > cursor. Just thow cursor_bo to the bin instead since it's lost all
> > other uses thanks to universal plane support.
> 
> You could also mention that since updating the cursor is so slow now
> through the universal-plane support, that a fast path to avoid a few
> writes/checks is also irrelevant.

I can add a note.

At some point we really should start measuring things and try to get
some of the speed back for common stuff like cursor movement and page
flips.

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

[PATCH v2 1/2] drm/i915: Kill intel_crtc->cursor_bo

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The vma may have been rebound between the last time the cursor was
enabled and now, so skipping the cursor gtt offset deduction is not
safe unless we would also reset cursor_bo to NULL when disabling the
cursor. Just thow cursor_bo to the bin instead since it's lost all
other uses thanks to universal plane support.

Chris pointed out that cursor updates are currently too slow
via universal planes that micro optimizations like these wouldn't
even help.

v2: Add a note about futility of micro optimizations (Chris)

References: http://lists.freedesktop.org/archives/intel-gfx/2015-December/082976.html
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 -----
 drivers/gpu/drm/i915/intel_drv.h     | 1 -
 2 files changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e9bb860a55dc..f2a0151b3f14 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14105,9 +14105,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
-	if (intel_crtc->cursor_bo == obj)
-		goto update;
-
 	if (!obj)
 		addr = 0;
 	else if (!INTEL_INFO(dev)->cursor_needs_physical)
@@ -14116,9 +14113,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 		addr = obj->phys_handle->busaddr;
 
 	intel_crtc->cursor_addr = addr;
-	intel_crtc->cursor_bo = obj;
 
-update:
 	intel_crtc_update_cursor(crtc, state->visible);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 76dfa286cd95..6324c782d062 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -568,7 +568,6 @@ struct intel_crtc {
 	int adjusted_x;
 	int adjusted_y;
 
-	struct drm_i915_gem_object *cursor_bo;
 	uint32_t cursor_addr;
 	uint32_t cursor_cntl;
 	uint32_t cursor_size;
-- 
2.4.10

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

Re: [PATCH v2 1/2] drm/i915: Kill intel_crtc->cursor_bo

[Chris Wilson]

On Mon, Dec 14, 2015 at 05:35:02PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The vma may have been rebound between the last time the cursor was
> enabled and now, so skipping the cursor gtt offset deduction is not
> safe unless we would also reset cursor_bo to NULL when disabling the
> cursor. Just thow cursor_bo to the bin instead since it's lost all
> other uses thanks to universal plane support.
> 
> Chris pointed out that cursor updates are currently too slow
> via universal planes that micro optimizations like these wouldn't
> even help.
> 
> v2: Add a note about futility of micro optimizations (Chris)
> 
> References: http://lists.freedesktop.org/archives/intel-gfx/2015-December/082976.html
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 01:16:46PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> These two should provide the minimal fix for the invisible cursor
> problem that's been plaguing me and some other people. It's caused
> by the cursor bo getting bound at ggtt offset 0, which the code then
> considers to mean "not enabled". These patches get rid of that assumption,
> with the caveat that we may do some needless cursor register writes.
> I have a more generic fix for that (and a ton of other cursor improvements)
> at [1], but I figured that I'll just post the critical fixes now and allow
> the others to stew a bit more in the branch until maybe early next year.
> 
> I assume the problem is also related to the lack of fbdev pinning that
> Chris has fixed [2]. With fbdev permanalty pinned at offset 0, there's
> no opportunity for the cursor bo to travel there. Without fbdev I suppose
> something else rings, status page, etc. could get permanently placed
> at offset 0, so not sure if this can be hit in that case (didn't try it).
> 
> I'm a bit wary about adding cc stable to these since I suspect that
> backporting them too far will cause regressions, on account of the
> atomic/plane stuff probably not being up to the task in older kernels.
> So maybe better get these in w/o cc stable and consider a backport again
> later if it looks necessary.
> 
> [1] git://github.com/vsyrjala/linux.git cursor_improvements
> [2] http://lists.freedesktop.org/archives/intel-gfx/2015-December/082114.html
> 
> Ville Syrjälä (2):
>   drm/i915: Kill intel_crtc->cursor_bo
>   drm/i915: Drop the broken cursor base==0 special casing

Both patches pushed to dinq with cc: fixes. Thanks for the reviews.

> 
>  drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++----------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 12 insertions(+), 23 deletions(-)
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> -- 
> 2.4.10

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