* Re: [PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v4)
2010-07-09 7:45 ` [PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v4) Chris Wilson
@ 2010-07-09 10:23 ` Christopher James Halse Rogers
0 siblings, 0 replies; 2+ messages in thread
From: Christopher James Halse Rogers @ 2010-07-09 10:23 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, stable
[-- Attachment #1.1: Type: text/plain, Size: 9590 bytes --]
On Fri, 2010-07-09 at 08:45 +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.
> v4: Always set CUR.BASE or else the cursor may become corrupt.
>
> 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>
> Cc: stable@kernel.org
> ---
> drivers/gpu/drm/i915/intel_display.c | 144 ++++++++++++++++++++++------------
> drivers/gpu/drm/i915/intel_drv.h | 8 ++-
> 2 files changed, 99 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1f06f3f..5f51084 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 */
> @@ -3532,6 +3533,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,
> @@ -4069,6 +4073,85 @@ 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;
> + bool visible;
> +
> + 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;
> +
> + visible = base != 0;
> + if (!visible && !intel_crtc->cursor_visble)
> + return;
> +
> + I915_WRITE(pipe == 0 ? CURAPOS : CURBPOS, pos);
> + if (intel_crtc->cursor_visble != visible) {
> + uint32_t cntl = I915_READ(pipe == 0 ? CURACNTR : CURBCNTR);
> + if (base) {
> + /* 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);
> +
> + intel_crtc->cursor_visble = visible;
> + }
> + /* and commit changes on next vblank */
> + I915_WRITE(pipe == 0 ? CURABASE : CURBBASE, base);
> +
> + if (visible)
> + intel_mark_busy(dev, to_intel_framebuffer(crtc->fb)->obj);
> +}
> +
> static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> struct drm_file *file_priv,
> uint32_t handle,
> @@ -4079,11 +4162,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");
> @@ -4091,12 +4170,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);
> @@ -4138,7 +4211,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;
> @@ -4149,21 +4223,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)
> @@ -4177,6 +4237,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:
> @@ -4190,34 +4254,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 437233d..e3cad5c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -142,8 +142,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? */
> @@ -152,6 +150,12 @@ 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;
> + bool cursor_visble;
> };
>
> #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
Tested on my Q965 and GM45. Fixes the reported bug, hasn't introduced
any visible regressions.
This matches my reading of the hardware docs, too.
Tested-by: Christopher James Halse Rogers
<christopher.halse.rogers@canonical.com>
Reviewed-by: Christopher James Halse Rogers
<christopher.halse.rogers@canonical.com>
[-- 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
^ permalink raw reply [flat|nested] 2+ messages in thread