[PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v2)

[PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v2)

[Chris Wilson]

The docs warn that to position the cursor such that no part of it is
visible on the pipe is an undefined operation. Avoid such circumstances
upon changing the mode, or at any other time, by unsetting the cursor if
it moves out of bounds.

"For normal high resolution display modes, the cursor must have at least a
single pixel positioned over the active screen.” (p143, p148 of the hardware
registers docs).

Fixes:

  Bug 24748 - [965G] Graphics crashes when resolution is changed with KMS
              enabled
  https://bugs.freedesktop.org/show_bug.cgi?id=24748

v2: Only update the cursor registers if they change.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reported-by: Christian Eggers <ceggers@gmx.de>
Cc: Christopher James Halse Rogers  <chalserogers@gmail.com>
Tested-by: Christopher James Halse Rogers  <chalserogers@gmail.com>
Cc: stable@kernel.org
---
 drivers/gpu/drm/i915/intel_display.c |  140 +++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     |    9 ++-
 2 files changed, 96 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d3397eb..15a6840 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -42,6 +42,7 @@
 bool intel_pipe_has_type (struct drm_crtc *crtc, int type);
 static void intel_update_watermarks(struct drm_device *dev);
 static void intel_increase_pllclock(struct drm_crtc *crtc, bool schedule);
+static void intel_crtc_update_cursor(struct drm_crtc *crtc);
 
 typedef struct {
     /* given values */
@@ -3533,6 +3534,9 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
+	/* Ensure that the cursor is valid for the new mode before changing... */
+	intel_crtc_update_cursor(crtc);
+
 	if (is_lvds && dev_priv->lvds_downclock_avail) {
 		has_reduced_clock = limit->find_pll(limit, crtc,
 							    dev_priv->lvds_downclock,
@@ -4068,6 +4072,81 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
 	}
 }
 
+/* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
+static void intel_crtc_update_cursor(struct drm_crtc *crtc)
+{
+	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;
+	int x = intel_crtc->cursor_x;
+	int y = intel_crtc->cursor_y;
+	uint32_t base, pos;
+
+	pos = 0;
+
+	if (crtc->fb) {
+		base = intel_crtc->cursor_addr;
+		if (x > crtc->fb->width)
+			base = 0;
+
+		if (y > crtc->fb->height)
+			base = 0;
+	} else
+		base = 0;
+
+	if (x < 0) {
+		if (x + intel_crtc->cursor_width < 0)
+			base = 0;
+
+		pos |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
+		x = -x;
+	}
+	pos |= x << CURSOR_X_SHIFT;
+
+	if (y < 0) {
+		if (y + intel_crtc->cursor_height < 0)
+			base = 0;
+
+		pos |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
+		y = -y;
+	}
+	pos |= y << CURSOR_Y_SHIFT;
+
+	if (pos != intel_crtc->cursor_current_pos) {
+		I915_WRITE((pipe == 0) ? CURAPOS : CURBPOS, pos);
+		intel_crtc->cursor_current_pos = pos;
+	}
+
+	if (base != intel_crtc->cursor_current_base) {
+		uint32_t cntl = I915_READ((pipe == 0) ? CURACNTR : CURBCNTR);
+		if (base) {
+			intel_mark_busy(dev, to_intel_framebuffer(crtc->fb)->obj);
+
+			/* Hooray for CUR*CNTR differences */
+			if (IS_MOBILE(dev) || IS_I9XX(dev)) {
+				cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
+				cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
+				cntl |= (pipe << 28); /* Connect to correct pipe */
+			} else {
+				cntl &= ~(CURSOR_FORMAT_MASK);
+				cntl |= CURSOR_ENABLE;
+				cntl |= CURSOR_FORMAT_ARGB | CURSOR_GAMMA_ENABLE;
+			}
+		} else {
+			if (IS_MOBILE(dev) || IS_I9XX(dev)) {
+				cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
+				cntl |= CURSOR_MODE_DISABLE;
+			} else {
+				cntl &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
+			}
+		}
+		I915_WRITE((pipe == 0) ? CURACNTR : CURBCNTR, cntl);
+		I915_WRITE((pipe == 0) ? CURABASE : CURBBASE, base);
+		intel_crtc->cursor_current_base = base;
+	}
+}
+
 static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 				 struct drm_file *file_priv,
 				 uint32_t handle,
@@ -4078,11 +4157,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_gem_object *bo;
 	struct drm_i915_gem_object *obj_priv;
-	int pipe = intel_crtc->pipe;
-	uint32_t control = (pipe == 0) ? CURACNTR : CURBCNTR;
-	uint32_t base = (pipe == 0) ? CURABASE : CURBBASE;
-	uint32_t temp = I915_READ(control);
-	size_t addr;
+	uint32_t addr;
 	int ret;
 
 	DRM_DEBUG_KMS("\n");
@@ -4090,12 +4165,6 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	/* if we want to turn off the cursor ignore width and height */
 	if (!handle) {
 		DRM_DEBUG_KMS("cursor off\n");
-		if (IS_MOBILE(dev) || IS_I9XX(dev)) {
-			temp &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
-			temp |= CURSOR_MODE_DISABLE;
-		} else {
-			temp &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
-		}
 		addr = 0;
 		bo = NULL;
 		mutex_lock(&dev->struct_mutex);
@@ -4137,7 +4206,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 
 		addr = obj_priv->gtt_offset;
 	} else {
-		ret = i915_gem_attach_phys_object(dev, bo, (pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1);
+		ret = i915_gem_attach_phys_object(dev, bo,
+						  (intel_crtc->pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1);
 		if (ret) {
 			DRM_ERROR("failed to attach phys object\n");
 			goto fail_locked;
@@ -4148,21 +4218,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	if (!IS_I9XX(dev))
 		I915_WRITE(CURSIZE, (height << 12) | width);
 
-	/* Hooray for CUR*CNTR differences */
-	if (IS_MOBILE(dev) || IS_I9XX(dev)) {
-		temp &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
-		temp |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
-		temp |= (pipe << 28); /* Connect to correct pipe */
-	} else {
-		temp &= ~(CURSOR_FORMAT_MASK);
-		temp |= CURSOR_ENABLE;
-		temp |= CURSOR_FORMAT_ARGB | CURSOR_GAMMA_ENABLE;
-	}
-
  finish:
-	I915_WRITE(control, temp);
-	I915_WRITE(base, addr);
-
 	if (intel_crtc->cursor_bo) {
 		if (dev_priv->info->cursor_needs_physical) {
 			if (intel_crtc->cursor_bo != bo)
@@ -4176,6 +4232,10 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 
 	intel_crtc->cursor_addr = addr;
 	intel_crtc->cursor_bo = bo;
+	intel_crtc->cursor_width = width;
+	intel_crtc->cursor_height = height;
+
+	intel_crtc_update_cursor(crtc);
 
 	return 0;
 fail_unpin:
@@ -4189,34 +4249,12 @@ fail:
 
 static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 {
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_framebuffer *intel_fb;
-	int pipe = intel_crtc->pipe;
-	uint32_t temp = 0;
-	uint32_t adder;
-
-	if (crtc->fb) {
-		intel_fb = to_intel_framebuffer(crtc->fb);
-		intel_mark_busy(dev, intel_fb->obj);
-	}
-
-	if (x < 0) {
-		temp |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
-		x = -x;
-	}
-	if (y < 0) {
-		temp |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
-		y = -y;
-	}
 
-	temp |= x << CURSOR_X_SHIFT;
-	temp |= y << CURSOR_Y_SHIFT;
+	intel_crtc->cursor_x = x;
+	intel_crtc->cursor_y = y;
 
-	adder = intel_crtc->cursor_addr;
-	I915_WRITE((pipe == 0) ? CURAPOS : CURBPOS, temp);
-	I915_WRITE((pipe == 0) ? CURABASE : CURBBASE, adder);
+	intel_crtc_update_cursor(crtc);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 77a71a4..22c259f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -143,8 +143,6 @@ struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
 	enum plane plane;
-	struct drm_gem_object *cursor_bo;
-	uint32_t cursor_addr;
 	u8 lut_r[256], lut_g[256], lut_b[256];
 	int dpms_mode;
 	bool busy; /* is scanout buffer being updated frequently? */
@@ -153,6 +151,13 @@ struct intel_crtc {
 	struct intel_overlay *overlay;
 	struct intel_unpin_work *unpin_work;
 	int fdi_lanes;
+
+	struct drm_gem_object *cursor_bo;
+	uint32_t cursor_addr;
+	int16_t cursor_x, cursor_y;
+	int16_t cursor_width, cursor_height;
+	uint32_t cursor_current_base;
+	uint32_t cursor_current_pos;
 };
 
 #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
-- 
1.7.1

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

[PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v3)

[Chris Wilson]

The docs warn that to position the cursor such that no part of it is
visible on the pipe is an undefined operation. Avoid such circumstances
upon changing the mode, or at any other time, by unsetting the cursor if
it moves out of bounds.

"For normal high resolution display modes, the cursor must have at least a
single pixel positioned over the active screen.” (p143, p148 of the hardware
registers docs).

Fixes:

  Bug 24748 - [965G] Graphics crashes when resolution is changed with KMS
              enabled
  https://bugs.freedesktop.org/show_bug.cgi?id=24748

v2: Only update the cursor registers if they change.
v3: Fix the unsigned comparision of x,y against width,height.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reported-by: Christian Eggers <ceggers@gmx.de>
Cc: Christopher James Halse Rogers  <chalserogers@gmail.com>
Tested-by: Christopher James Halse Rogers  <chalserogers@gmail.com>
Cc: stable@kernel.org
---
 drivers/gpu/drm/i915/intel_display.c |  140 +++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     |    9 ++-
 2 files changed, 96 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d3397eb..c426a6a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -42,6 +42,7 @@
 bool intel_pipe_has_type (struct drm_crtc *crtc, int type);
 static void intel_update_watermarks(struct drm_device *dev);
 static void intel_increase_pllclock(struct drm_crtc *crtc, bool schedule);
+static void intel_crtc_update_cursor(struct drm_crtc *crtc);
 
 typedef struct {
     /* given values */
@@ -3533,6 +3534,9 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
+	/* Ensure that the cursor is valid for the new mode before changing... */
+	intel_crtc_update_cursor(crtc);
+
 	if (is_lvds && dev_priv->lvds_downclock_avail) {
 		has_reduced_clock = limit->find_pll(limit, crtc,
 							    dev_priv->lvds_downclock,
@@ -4068,6 +4072,81 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
 	}
 }
 
+/* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
+static void intel_crtc_update_cursor(struct drm_crtc *crtc)
+{
+	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;
+	int x = intel_crtc->cursor_x;
+	int y = intel_crtc->cursor_y;
+	uint32_t base, pos;
+
+	pos = 0;
+
+	if (crtc->fb) {
+		base = intel_crtc->cursor_addr;
+		if (x > (int) crtc->fb->width)
+			base = 0;
+
+		if (y > (int) crtc->fb->height)
+			base = 0;
+	} else
+		base = 0;
+
+	if (x < 0) {
+		if (x + intel_crtc->cursor_width < 0)
+			base = 0;
+
+		pos |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
+		x = -x;
+	}
+	pos |= x << CURSOR_X_SHIFT;
+
+	if (y < 0) {
+		if (y + intel_crtc->cursor_height < 0)
+			base = 0;
+
+		pos |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
+		y = -y;
+	}
+	pos |= y << CURSOR_Y_SHIFT;
+
+	if (pos != intel_crtc->cursor_current_pos) {
+		I915_WRITE((pipe == 0) ? CURAPOS : CURBPOS, pos);
+		intel_crtc->cursor_current_pos = pos;
+	}
+
+	if (base != intel_crtc->cursor_current_base) {
+		uint32_t cntl = I915_READ((pipe == 0) ? CURACNTR : CURBCNTR);
+		if (base) {
+			intel_mark_busy(dev, to_intel_framebuffer(crtc->fb)->obj);
+
+			/* Hooray for CUR*CNTR differences */
+			if (IS_MOBILE(dev) || IS_I9XX(dev)) {
+				cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
+				cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
+				cntl |= (pipe << 28); /* Connect to correct pipe */
+			} else {
+				cntl &= ~(CURSOR_FORMAT_MASK);
+				cntl |= CURSOR_ENABLE;
+				cntl |= CURSOR_FORMAT_ARGB | CURSOR_GAMMA_ENABLE;
+			}
+		} else {
+			if (IS_MOBILE(dev) || IS_I9XX(dev)) {
+				cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
+				cntl |= CURSOR_MODE_DISABLE;
+			} else {
+				cntl &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
+			}
+		}
+		I915_WRITE((pipe == 0) ? CURACNTR : CURBCNTR, cntl);
+		I915_WRITE((pipe == 0) ? CURABASE : CURBBASE, base);
+		intel_crtc->cursor_current_base = base;
+	}
+}
+
 static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 				 struct drm_file *file_priv,
 				 uint32_t handle,
@@ -4078,11 +4157,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_gem_object *bo;
 	struct drm_i915_gem_object *obj_priv;
-	int pipe = intel_crtc->pipe;
-	uint32_t control = (pipe == 0) ? CURACNTR : CURBCNTR;
-	uint32_t base = (pipe == 0) ? CURABASE : CURBBASE;
-	uint32_t temp = I915_READ(control);
-	size_t addr;
+	uint32_t addr;
 	int ret;
 
 	DRM_DEBUG_KMS("\n");
@@ -4090,12 +4165,6 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	/* if we want to turn off the cursor ignore width and height */
 	if (!handle) {
 		DRM_DEBUG_KMS("cursor off\n");
-		if (IS_MOBILE(dev) || IS_I9XX(dev)) {
-			temp &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
-			temp |= CURSOR_MODE_DISABLE;
-		} else {
-			temp &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
-		}
 		addr = 0;
 		bo = NULL;
 		mutex_lock(&dev->struct_mutex);
@@ -4137,7 +4206,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 
 		addr = obj_priv->gtt_offset;
 	} else {
-		ret = i915_gem_attach_phys_object(dev, bo, (pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1);
+		ret = i915_gem_attach_phys_object(dev, bo,
+						  (intel_crtc->pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1);
 		if (ret) {
 			DRM_ERROR("failed to attach phys object\n");
 			goto fail_locked;
@@ -4148,21 +4218,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	if (!IS_I9XX(dev))
 		I915_WRITE(CURSIZE, (height << 12) | width);
 
-	/* Hooray for CUR*CNTR differences */
-	if (IS_MOBILE(dev) || IS_I9XX(dev)) {
-		temp &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
-		temp |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
-		temp |= (pipe << 28); /* Connect to correct pipe */
-	} else {
-		temp &= ~(CURSOR_FORMAT_MASK);
-		temp |= CURSOR_ENABLE;
-		temp |= CURSOR_FORMAT_ARGB | CURSOR_GAMMA_ENABLE;
-	}
-
  finish:
-	I915_WRITE(control, temp);
-	I915_WRITE(base, addr);
-
 	if (intel_crtc->cursor_bo) {
 		if (dev_priv->info->cursor_needs_physical) {
 			if (intel_crtc->cursor_bo != bo)
@@ -4176,6 +4232,10 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 
 	intel_crtc->cursor_addr = addr;
 	intel_crtc->cursor_bo = bo;
+	intel_crtc->cursor_width = width;
+	intel_crtc->cursor_height = height;
+
+	intel_crtc_update_cursor(crtc);
 
 	return 0;
 fail_unpin:
@@ -4189,34 +4249,12 @@ fail:
 
 static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 {
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_framebuffer *intel_fb;
-	int pipe = intel_crtc->pipe;
-	uint32_t temp = 0;
-	uint32_t adder;
-
-	if (crtc->fb) {
-		intel_fb = to_intel_framebuffer(crtc->fb);
-		intel_mark_busy(dev, intel_fb->obj);
-	}
-
-	if (x < 0) {
-		temp |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
-		x = -x;
-	}
-	if (y < 0) {
-		temp |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
-		y = -y;
-	}
 
-	temp |= x << CURSOR_X_SHIFT;
-	temp |= y << CURSOR_Y_SHIFT;
+	intel_crtc->cursor_x = x;
+	intel_crtc->cursor_y = y;
 
-	adder = intel_crtc->cursor_addr;
-	I915_WRITE((pipe == 0) ? CURAPOS : CURBPOS, temp);
-	I915_WRITE((pipe == 0) ? CURABASE : CURBBASE, adder);
+	intel_crtc_update_cursor(crtc);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 77a71a4..22c259f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -143,8 +143,6 @@ struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
 	enum plane plane;
-	struct drm_gem_object *cursor_bo;
-	uint32_t cursor_addr;
 	u8 lut_r[256], lut_g[256], lut_b[256];
 	int dpms_mode;
 	bool busy; /* is scanout buffer being updated frequently? */
@@ -153,6 +151,13 @@ struct intel_crtc {
 	struct intel_overlay *overlay;
 	struct intel_unpin_work *unpin_work;
 	int fdi_lanes;
+
+	struct drm_gem_object *cursor_bo;
+	uint32_t cursor_addr;
+	int16_t cursor_x, cursor_y;
+	int16_t cursor_width, cursor_height;
+	uint32_t cursor_current_base;
+	uint32_t cursor_current_pos;
 };
 
 #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
-- 
1.7.1

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

Re: [PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v2)

[Christopher James Halse Rogers]

[-- Attachment #1.1: Type: text/plain, Size: 9927 bytes --]

On Thu, 2010-07-08 at 10:41 +0100, Chris Wilson wrote:
> The docs warn that to position the cursor such that no part of it is
> visible on the pipe is an undefined operation. Avoid such circumstances
> upon changing the mode, or at any other time, by unsetting the cursor if
> it moves out of bounds.
> 
> "For normal high resolution display modes, the cursor must have at least a
> single pixel positioned over the active screen.” (p143, p148 of the hardware
> registers docs).
> 
> Fixes:
> 
>   Bug 24748 - [965G] Graphics crashes when resolution is changed with KMS
>               enabled
>   https://bugs.freedesktop.org/show_bug.cgi?id=24748
> 
> v2: Only update the cursor registers if they change.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reported-by: Christian Eggers <ceggers@gmx.de>
> Cc: Christopher James Halse Rogers  <chalserogers@gmail.com>
> Tested-by: Christopher James Halse Rogers  <chalserogers@gmail.com>
> Cc: stable@kernel.org

Gah.  Not quit sufficiently well tested-by me.  Although this fixes the
resolution change problem, it introduces a regression where the cursor
is hidden when it's on the extreme top or left edges of the screen.


> ---
>  drivers/gpu/drm/i915/intel_display.c |  140 +++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h     |    9 ++-
>  2 files changed, 96 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d3397eb..15a6840 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -42,6 +42,7 @@
>  bool intel_pipe_has_type (struct drm_crtc *crtc, int type);
>  static void intel_update_watermarks(struct drm_device *dev);
>  static void intel_increase_pllclock(struct drm_crtc *crtc, bool schedule);
> +static void intel_crtc_update_cursor(struct drm_crtc *crtc);
>  
>  typedef struct {
>      /* given values */
> @@ -3533,6 +3534,9 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> +	/* Ensure that the cursor is valid for the new mode before changing... */
> +	intel_crtc_update_cursor(crtc);
> +
>  	if (is_lvds && dev_priv->lvds_downclock_avail) {
>  		has_reduced_clock = limit->find_pll(limit, crtc,
>  							    dev_priv->lvds_downclock,
> @@ -4068,6 +4072,81 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
>  	}
>  }
>  
> +/* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> +static void intel_crtc_update_cursor(struct drm_crtc *crtc)
> +{
> +	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;
> +	int x = intel_crtc->cursor_x;
> +	int y = intel_crtc->cursor_y;
> +	uint32_t base, pos;
> +
> +	pos = 0;
> +
> +	if (crtc->fb) {
> +		base = intel_crtc->cursor_addr;
> +		if (x > crtc->fb->width)

crtc->fb->width is unsigned, so this evaluates to true when x is
negative, such as when the cursor is close to the left-hand edge of the
screen.

> +			base = 0;
> +
> +		if (y > crtc->fb->height)

Similarly here.

> +			base = 0;
> +	} else
> +		base = 0;
> +
> +	if (x < 0) {
> +		if (x + intel_crtc->cursor_width < 0)
> +			base = 0;
> +
> +		pos |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
> +		x = -x;
> +	}
> +	pos |= x << CURSOR_X_SHIFT;
> +
> +	if (y < 0) {
> +		if (y + intel_crtc->cursor_height < 0)
> +			base = 0;
> +
> +		pos |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
> +		y = -y;
> +	}
> +	pos |= y << CURSOR_Y_SHIFT;
> +
> +	if (pos != intel_crtc->cursor_current_pos) {
> +		I915_WRITE((pipe == 0) ? CURAPOS : CURBPOS, pos);
> +		intel_crtc->cursor_current_pos = pos;
> +	}
> +
> +	if (base != intel_crtc->cursor_current_base) {
> +		uint32_t cntl = I915_READ((pipe == 0) ? CURACNTR : CURBCNTR);
> +		if (base) {
> +			intel_mark_busy(dev, to_intel_framebuffer(crtc->fb)->obj);
> +
> +			/* Hooray for CUR*CNTR differences */
> +			if (IS_MOBILE(dev) || IS_I9XX(dev)) {
> +				cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
> +				cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> +				cntl |= (pipe << 28); /* Connect to correct pipe */
> +			} else {
> +				cntl &= ~(CURSOR_FORMAT_MASK);
> +				cntl |= CURSOR_ENABLE;
> +				cntl |= CURSOR_FORMAT_ARGB | CURSOR_GAMMA_ENABLE;
> +			}
> +		} else {
> +			if (IS_MOBILE(dev) || IS_I9XX(dev)) {
> +				cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> +				cntl |= CURSOR_MODE_DISABLE;
> +			} else {
> +				cntl &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
> +			}
> +		}
> +		I915_WRITE((pipe == 0) ? CURACNTR : CURBCNTR, cntl);
> +		I915_WRITE((pipe == 0) ? CURABASE : CURBBASE, base);
> +		intel_crtc->cursor_current_base = base;
> +	}
> +}

The 965 docs say (vol 3, p142, 147) that CUR?BASE should be written to
last when updating any of the cursor regs even if the base value hasn't
changed to trigger an update on the next VBLANK.

I'm not sure whether my reading of that documentation is correct,
though, because the cursor seemed to update just fine with the code in
this patch.

> +
>  static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  				 struct drm_file *file_priv,
>  				 uint32_t handle,
> @@ -4078,11 +4157,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_gem_object *bo;
>  	struct drm_i915_gem_object *obj_priv;
> -	int pipe = intel_crtc->pipe;
> -	uint32_t control = (pipe == 0) ? CURACNTR : CURBCNTR;
> -	uint32_t base = (pipe == 0) ? CURABASE : CURBBASE;
> -	uint32_t temp = I915_READ(control);
> -	size_t addr;
> +	uint32_t addr;
>  	int ret;
>  
>  	DRM_DEBUG_KMS("\n");
> @@ -4090,12 +4165,6 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	/* if we want to turn off the cursor ignore width and height */
>  	if (!handle) {
>  		DRM_DEBUG_KMS("cursor off\n");
> -		if (IS_MOBILE(dev) || IS_I9XX(dev)) {
> -			temp &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> -			temp |= CURSOR_MODE_DISABLE;
> -		} else {
> -			temp &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
> -		}
>  		addr = 0;
>  		bo = NULL;
>  		mutex_lock(&dev->struct_mutex);
> @@ -4137,7 +4206,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  
>  		addr = obj_priv->gtt_offset;
>  	} else {
> -		ret = i915_gem_attach_phys_object(dev, bo, (pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1);
> +		ret = i915_gem_attach_phys_object(dev, bo,
> +						  (intel_crtc->pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1);
>  		if (ret) {
>  			DRM_ERROR("failed to attach phys object\n");
>  			goto fail_locked;
> @@ -4148,21 +4218,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	if (!IS_I9XX(dev))
>  		I915_WRITE(CURSIZE, (height << 12) | width);
>  
> -	/* Hooray for CUR*CNTR differences */
> -	if (IS_MOBILE(dev) || IS_I9XX(dev)) {
> -		temp &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
> -		temp |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> -		temp |= (pipe << 28); /* Connect to correct pipe */
> -	} else {
> -		temp &= ~(CURSOR_FORMAT_MASK);
> -		temp |= CURSOR_ENABLE;
> -		temp |= CURSOR_FORMAT_ARGB | CURSOR_GAMMA_ENABLE;
> -	}
> -
>   finish:
> -	I915_WRITE(control, temp);
> -	I915_WRITE(base, addr);
> -
>  	if (intel_crtc->cursor_bo) {
>  		if (dev_priv->info->cursor_needs_physical) {
>  			if (intel_crtc->cursor_bo != bo)
> @@ -4176,6 +4232,10 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  
>  	intel_crtc->cursor_addr = addr;
>  	intel_crtc->cursor_bo = bo;
> +	intel_crtc->cursor_width = width;
> +	intel_crtc->cursor_height = height;
> +
> +	intel_crtc_update_cursor(crtc);
>  
>  	return 0;
>  fail_unpin:
> @@ -4189,34 +4249,12 @@ fail:
>  
>  static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  {
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_framebuffer *intel_fb;
> -	int pipe = intel_crtc->pipe;
> -	uint32_t temp = 0;
> -	uint32_t adder;
> -
> -	if (crtc->fb) {
> -		intel_fb = to_intel_framebuffer(crtc->fb);
> -		intel_mark_busy(dev, intel_fb->obj);
> -	}
> -
> -	if (x < 0) {
> -		temp |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
> -		x = -x;
> -	}
> -	if (y < 0) {
> -		temp |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
> -		y = -y;
> -	}
>  
> -	temp |= x << CURSOR_X_SHIFT;
> -	temp |= y << CURSOR_Y_SHIFT;
> +	intel_crtc->cursor_x = x;
> +	intel_crtc->cursor_y = y;
>  
> -	adder = intel_crtc->cursor_addr;
> -	I915_WRITE((pipe == 0) ? CURAPOS : CURBPOS, temp);
> -	I915_WRITE((pipe == 0) ? CURABASE : CURBBASE, adder);
> +	intel_crtc_update_cursor(crtc);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 77a71a4..22c259f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -143,8 +143,6 @@ struct intel_crtc {
>  	struct drm_crtc base;
>  	enum pipe pipe;
>  	enum plane plane;
> -	struct drm_gem_object *cursor_bo;
> -	uint32_t cursor_addr;
>  	u8 lut_r[256], lut_g[256], lut_b[256];
>  	int dpms_mode;
>  	bool busy; /* is scanout buffer being updated frequently? */
> @@ -153,6 +151,13 @@ struct intel_crtc {
>  	struct intel_overlay *overlay;
>  	struct intel_unpin_work *unpin_work;
>  	int fdi_lanes;
> +
> +	struct drm_gem_object *cursor_bo;
> +	uint32_t cursor_addr;
> +	int16_t cursor_x, cursor_y;
> +	int16_t cursor_width, cursor_height;
> +	uint32_t cursor_current_base;
> +	uint32_t cursor_current_pos;
>  };
>  
>  #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)



[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v2)

[Chris Wilson]

On Thu, 08 Jul 2010 22:56:34 +1000, Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> wrote:
> The 965 docs say (vol 3, p142, 147) that CUR?BASE should be written to
> last when updating any of the cursor regs even if the base value hasn't
> changed to trigger an update on the next VBLANK.
> 
> I'm not sure whether my reading of that documentation is correct,
> though, because the cursor seemed to update just fine with the code in
> this patch.

CUR?POS:
"This register can be loaded atomically (requires that the base address be
written) and is double buffered."

As the code has been previously moving the cursor for many years without
updating CUR?BASE, I think we are safe.
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v2)

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 978 bytes --]

On Thu, 08 Jul 2010 14:23:26 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, 08 Jul 2010 22:56:34 +1000, Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> wrote:
> > The 965 docs say (vol 3, p142, 147) that CUR?BASE should be written to
> > last when updating any of the cursor regs even if the base value hasn't
> > changed to trigger an update on the next VBLANK.
> > 
> > I'm not sure whether my reading of that documentation is correct,
> > though, because the cursor seemed to update just fine with the code in
> > this patch.
> 
> CUR?POS:
> "This register can be loaded atomically (requires that the base address be
> written) and is double buffered."
> 
> As the code has been previously moving the cursor for many years without
> updating CUR?BASE, I think we are safe.
> -ickle

Cursors are working fine on my system with this patch, but I'll wait for
a resolution on the regression when cursor at top/left.


[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v3)

[Christopher James Halse Rogers]

[-- Attachment #1.1: Type: text/plain, Size: 9575 bytes --]

On Thu, 2010-07-08 at 13:56 +0100, Chris Wilson wrote:
> The docs warn that to position the cursor such that no part of it is
> visible on the pipe is an undefined operation. Avoid such circumstances
> upon changing the mode, or at any other time, by unsetting the cursor if
> it moves out of bounds.
> 
> "For normal high resolution display modes, the cursor must have at least a
> single pixel positioned over the active screen.” (p143, p148 of the hardware
> registers docs).
> 
> Fixes:
> 
>   Bug 24748 - [965G] Graphics crashes when resolution is changed with KMS
>               enabled
>   https://bugs.freedesktop.org/show_bug.cgi?id=24748
> 
> v2: Only update the cursor registers if they change.
> v3: Fix the unsigned comparision of x,y against width,height.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reported-by: Christian Eggers <ceggers@gmx.de>
> Cc: Christopher James Halse Rogers  <chalserogers@gmail.com>
> Tested-by: Christopher James Halse Rogers  <chalserogers@gmail.com>
> Cc: stable@kernel.org
> ---
>  drivers/gpu/drm/i915/intel_display.c |  140 +++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h     |    9 ++-
>  2 files changed, 96 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d3397eb..c426a6a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -42,6 +42,7 @@
>  bool intel_pipe_has_type (struct drm_crtc *crtc, int type);
>  static void intel_update_watermarks(struct drm_device *dev);
>  static void intel_increase_pllclock(struct drm_crtc *crtc, bool schedule);
> +static void intel_crtc_update_cursor(struct drm_crtc *crtc);
>  
>  typedef struct {
>      /* given values */
> @@ -3533,6 +3534,9 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> +	/* Ensure that the cursor is valid for the new mode before changing... */
> +	intel_crtc_update_cursor(crtc);
> +
>  	if (is_lvds && dev_priv->lvds_downclock_avail) {
>  		has_reduced_clock = limit->find_pll(limit, crtc,
>  							    dev_priv->lvds_downclock,
> @@ -4068,6 +4072,81 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
>  	}
>  }
>  
> +/* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> +static void intel_crtc_update_cursor(struct drm_crtc *crtc)
> +{
> +	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;
> +	int x = intel_crtc->cursor_x;
> +	int y = intel_crtc->cursor_y;
> +	uint32_t base, pos;
> +
> +	pos = 0;
> +
> +	if (crtc->fb) {
> +		base = intel_crtc->cursor_addr;
> +		if (x > (int) crtc->fb->width)
> +			base = 0;
> +
> +		if (y > (int) crtc->fb->height)
> +			base = 0;
> +	} else
> +		base = 0;
> +
> +	if (x < 0) {
> +		if (x + intel_crtc->cursor_width < 0)
> +			base = 0;
> +
> +		pos |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
> +		x = -x;
> +	}
> +	pos |= x << CURSOR_X_SHIFT;
> +
> +	if (y < 0) {
> +		if (y + intel_crtc->cursor_height < 0)
> +			base = 0;
> +
> +		pos |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
> +		y = -y;
> +	}
> +	pos |= y << CURSOR_Y_SHIFT;
> +
> +	if (pos != intel_crtc->cursor_current_pos) {
> +		I915_WRITE((pipe == 0) ? CURAPOS : CURBPOS, pos);
> +		intel_crtc->cursor_current_pos = pos;
> +	}
> +
> +	if (base != intel_crtc->cursor_current_base) {
> +		uint32_t cntl = I915_READ((pipe == 0) ? CURACNTR : CURBCNTR);
> +		if (base) {
> +			intel_mark_busy(dev, to_intel_framebuffer(crtc->fb)->obj);
> +
> +			/* Hooray for CUR*CNTR differences */
> +			if (IS_MOBILE(dev) || IS_I9XX(dev)) {
> +				cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
> +				cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> +				cntl |= (pipe << 28); /* Connect to correct pipe */
> +			} else {
> +				cntl &= ~(CURSOR_FORMAT_MASK);
> +				cntl |= CURSOR_ENABLE;
> +				cntl |= CURSOR_FORMAT_ARGB | CURSOR_GAMMA_ENABLE;
> +			}
> +		} else {
> +			if (IS_MOBILE(dev) || IS_I9XX(dev)) {
> +				cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> +				cntl |= CURSOR_MODE_DISABLE;
> +			} else {
> +				cntl &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
> +			}
> +		}
> +		I915_WRITE((pipe == 0) ? CURACNTR : CURBCNTR, cntl);
> +		I915_WRITE((pipe == 0) ? CURABASE : CURBBASE, base);
> +		intel_crtc->cursor_current_base = base;
> +	}
> +}
> +
>  static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  				 struct drm_file *file_priv,
>  				 uint32_t handle,
> @@ -4078,11 +4157,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_gem_object *bo;
>  	struct drm_i915_gem_object *obj_priv;
> -	int pipe = intel_crtc->pipe;
> -	uint32_t control = (pipe == 0) ? CURACNTR : CURBCNTR;
> -	uint32_t base = (pipe == 0) ? CURABASE : CURBBASE;
> -	uint32_t temp = I915_READ(control);
> -	size_t addr;
> +	uint32_t addr;
>  	int ret;
>  
>  	DRM_DEBUG_KMS("\n");
> @@ -4090,12 +4165,6 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	/* if we want to turn off the cursor ignore width and height */
>  	if (!handle) {
>  		DRM_DEBUG_KMS("cursor off\n");
> -		if (IS_MOBILE(dev) || IS_I9XX(dev)) {
> -			temp &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> -			temp |= CURSOR_MODE_DISABLE;
> -		} else {
> -			temp &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
> -		}
>  		addr = 0;
>  		bo = NULL;
>  		mutex_lock(&dev->struct_mutex);
> @@ -4137,7 +4206,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  
>  		addr = obj_priv->gtt_offset;
>  	} else {
> -		ret = i915_gem_attach_phys_object(dev, bo, (pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1);
> +		ret = i915_gem_attach_phys_object(dev, bo,
> +						  (intel_crtc->pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1);
>  		if (ret) {
>  			DRM_ERROR("failed to attach phys object\n");
>  			goto fail_locked;
> @@ -4148,21 +4218,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	if (!IS_I9XX(dev))
>  		I915_WRITE(CURSIZE, (height << 12) | width);
>  
> -	/* Hooray for CUR*CNTR differences */
> -	if (IS_MOBILE(dev) || IS_I9XX(dev)) {
> -		temp &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
> -		temp |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> -		temp |= (pipe << 28); /* Connect to correct pipe */
> -	} else {
> -		temp &= ~(CURSOR_FORMAT_MASK);
> -		temp |= CURSOR_ENABLE;
> -		temp |= CURSOR_FORMAT_ARGB | CURSOR_GAMMA_ENABLE;
> -	}
> -
>   finish:
> -	I915_WRITE(control, temp);
> -	I915_WRITE(base, addr);
> -
>  	if (intel_crtc->cursor_bo) {
>  		if (dev_priv->info->cursor_needs_physical) {
>  			if (intel_crtc->cursor_bo != bo)
> @@ -4176,6 +4232,10 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  
>  	intel_crtc->cursor_addr = addr;
>  	intel_crtc->cursor_bo = bo;
> +	intel_crtc->cursor_width = width;
> +	intel_crtc->cursor_height = height;
> +
> +	intel_crtc_update_cursor(crtc);
>  
>  	return 0;
>  fail_unpin:
> @@ -4189,34 +4249,12 @@ fail:
>  
>  static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  {
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_framebuffer *intel_fb;
> -	int pipe = intel_crtc->pipe;
> -	uint32_t temp = 0;
> -	uint32_t adder;
> -
> -	if (crtc->fb) {
> -		intel_fb = to_intel_framebuffer(crtc->fb);
> -		intel_mark_busy(dev, intel_fb->obj);
> -	}
> -
> -	if (x < 0) {
> -		temp |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
> -		x = -x;
> -	}
> -	if (y < 0) {
> -		temp |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
> -		y = -y;
> -	}
>  
> -	temp |= x << CURSOR_X_SHIFT;
> -	temp |= y << CURSOR_Y_SHIFT;
> +	intel_crtc->cursor_x = x;
> +	intel_crtc->cursor_y = y;
>  
> -	adder = intel_crtc->cursor_addr;
> -	I915_WRITE((pipe == 0) ? CURAPOS : CURBPOS, temp);
> -	I915_WRITE((pipe == 0) ? CURABASE : CURBBASE, adder);
> +	intel_crtc_update_cursor(crtc);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 77a71a4..22c259f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -143,8 +143,6 @@ struct intel_crtc {
>  	struct drm_crtc base;
>  	enum pipe pipe;
>  	enum plane plane;
> -	struct drm_gem_object *cursor_bo;
> -	uint32_t cursor_addr;
>  	u8 lut_r[256], lut_g[256], lut_b[256];
>  	int dpms_mode;
>  	bool busy; /* is scanout buffer being updated frequently? */
> @@ -153,6 +151,13 @@ struct intel_crtc {
>  	struct intel_overlay *overlay;
>  	struct intel_unpin_work *unpin_work;
>  	int fdi_lanes;
> +
> +	struct drm_gem_object *cursor_bo;
> +	uint32_t cursor_addr;
> +	int16_t cursor_x, cursor_y;
> +	int16_t cursor_width, cursor_height;
> +	uint32_t cursor_current_base;
> +	uint32_t cursor_current_pos;
>  };
>  
>  #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)

This fixes the bug, and the mouse is correctly visible along all four
edges of the screen.

However, it seems that about half the time when unhiding the cursor, the
cursor is full of garbage - black, with red, vaguely kanji-looking
scribbles.  It's the same garbage each time.

I'll see if I can track it down.

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v2)

[Chris Wilson]

On Thu, 08 Jul 2010 11:27:14 -0700, Eric Anholt <eric@anholt.net> wrote:
> Cursors are working fine on my system with this patch, but I'll wait for
> a resolution on the regression when cursor at top/left.

Eric, you were wise to wait! Christopher found a further issue where the
cursor may come back corrupt...
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre