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 <[email protected]> > Reported-by: Christian Eggers <[email protected]> > Cc: Christopher James Halse Rogers <[email protected]> > Cc: [email protected] > --- > 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 <[email protected]> Reviewed-by: Christopher James Halse Rogers <[email protected]>
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
