[Intel-gfx] [PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v2)
Christopher James Halse Rogers
christopher.halse.rogers at canonical.com
Thu Jul 8 14:56:34 CEST 2010
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 at chris-wilson.co.uk>
> Reported-by: Christian Eggers <ceggers at gmx.de>
> Cc: Christopher James Halse Rogers <chalserogers at gmail.com>
> Tested-by: Christopher James Halse Rogers <chalserogers at gmail.com>
> Cc: stable at 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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20100708/9e71ab1f/attachment.sig>
More information about the Intel-gfx
mailing list