From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christopher James Halse Rogers Subject: Re: [PATCH] drm/i915: Unset cursor if out-of-bounds upon mode change (v4) Date: Fri, 09 Jul 2010 20:23:09 +1000 Message-ID: <1278670989.4430.8.camel@Ed> References: <8u3s8d@orsmga001.jf.intel.com> <1278661504-2189-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0038730546==" Return-path: Received: from adelie.canonical.com (adelie.canonical.com [91.189.90.139]) by gabe.freedesktop.org (Postfix) with ESMTP id 6F0579E710 for ; Fri, 9 Jul 2010 03:23:26 -0700 (PDT) In-Reply-To: <1278661504-2189-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Mime-version: 1.0 Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, stable@kernel.org List-Id: intel-gfx@lists.freedesktop.org --===============0038730546== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-iMJfaivA0BChtRfRbOI+" --=-iMJfaivA0BChtRfRbOI+ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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. >=20 > "For normal high resolution display modes, the cursor must have at least = a > single pixel positioned over the active screen.=E2=80=9D (p143, p148 of t= he hardware > registers docs). >=20 > Fixes: >=20 > Bug 24748 - [965G] Graphics crashes when resolution is changed with KMS > enabled > https://bugs.freedesktop.org/show_bug.cgi?id=3D24748 >=20 > 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. >=20 > Signed-off-by: Chris Wilson > Reported-by: Christian Eggers > Cc: Christopher James Halse Rogers > 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(-) >=20 > 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); > =20 > typedef struct { > /* given values */ > @@ -3532,6 +3533,9 @@ static int intel_crtc_mode_set(struct drm_crtc *crt= c, > return -EINVAL; > } > =20 > + /* 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 =3D limit->find_pll(limit, crtc, > dev_priv->lvds_downclock, > @@ -4069,6 +4073,85 @@ void intel_crtc_load_lut(struct drm_crtc *crtc) > } > } > =20 > +/* 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 =3D crtc->dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > + int pipe =3D intel_crtc->pipe; > + int x =3D intel_crtc->cursor_x; > + int y =3D intel_crtc->cursor_y; > + uint32_t base, pos; > + bool visible; > + > + pos =3D 0; > + > + if (crtc->fb) { > + base =3D intel_crtc->cursor_addr; > + if (x > (int) crtc->fb->width) > + base =3D 0; > + > + if (y > (int) crtc->fb->height) > + base =3D 0; > + } else > + base =3D 0; > + > + if (x < 0) { > + if (x + intel_crtc->cursor_width < 0) > + base =3D 0; > + > + pos |=3D CURSOR_POS_SIGN << CURSOR_X_SHIFT; > + x =3D -x; > + } > + pos |=3D x << CURSOR_X_SHIFT; > + > + if (y < 0) { > + if (y + intel_crtc->cursor_height < 0) > + base =3D 0; > + > + pos |=3D CURSOR_POS_SIGN << CURSOR_Y_SHIFT; > + y =3D -y; > + } > + pos |=3D y << CURSOR_Y_SHIFT; > + > + visible =3D base !=3D 0; > + if (!visible && !intel_crtc->cursor_visble) > + return; > + > + I915_WRITE(pipe =3D=3D 0 ? CURAPOS : CURBPOS, pos); > + if (intel_crtc->cursor_visble !=3D visible) { > + uint32_t cntl =3D I915_READ(pipe =3D=3D 0 ? CURACNTR : CURBCNTR); > + if (base) { > + /* Hooray for CUR*CNTR differences */ > + if (IS_MOBILE(dev) || IS_I9XX(dev)) { > + cntl &=3D ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); > + cntl |=3D CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + cntl |=3D pipe << 28; /* Connect to correct pipe */ > + } else { > + cntl &=3D ~(CURSOR_FORMAT_MASK); > + cntl |=3D CURSOR_ENABLE; > + cntl |=3D CURSOR_FORMAT_ARGB | CURSOR_GAMMA_ENABLE; > + } > + } else { > + if (IS_MOBILE(dev) || IS_I9XX(dev)) { > + cntl &=3D ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > + cntl |=3D CURSOR_MODE_DISABLE; > + } else { > + cntl &=3D ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE); > + } > + } > + I915_WRITE(pipe =3D=3D 0 ? CURACNTR : CURBCNTR, cntl); > + > + intel_crtc->cursor_visble =3D visible; > + } > + /* and commit changes on next vblank */ > + I915_WRITE(pipe =3D=3D 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 =3D to_intel_crtc(crtc); > struct drm_gem_object *bo; > struct drm_i915_gem_object *obj_priv; > - int pipe =3D intel_crtc->pipe; > - uint32_t control =3D (pipe =3D=3D 0) ? CURACNTR : CURBCNTR; > - uint32_t base =3D (pipe =3D=3D 0) ? CURABASE : CURBBASE; > - uint32_t temp =3D I915_READ(control); > - size_t addr; > + uint32_t addr; > int ret; > =20 > 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 &=3D ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > - temp |=3D CURSOR_MODE_DISABLE; > - } else { > - temp &=3D ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE); > - } > addr =3D 0; > bo =3D NULL; > mutex_lock(&dev->struct_mutex); > @@ -4138,7 +4211,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *c= rtc, > =20 > addr =3D obj_priv->gtt_offset; > } else { > - ret =3D i915_gem_attach_phys_object(dev, bo, (pipe =3D=3D 0) ? I915_GE= M_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1); > + ret =3D i915_gem_attach_phys_object(dev, bo, > + (intel_crtc->pipe =3D=3D 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); > =20 > - /* Hooray for CUR*CNTR differences */ > - if (IS_MOBILE(dev) || IS_I9XX(dev)) { > - temp &=3D ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); > - temp |=3D CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > - temp |=3D (pipe << 28); /* Connect to correct pipe */ > - } else { > - temp &=3D ~(CURSOR_FORMAT_MASK); > - temp |=3D CURSOR_ENABLE; > - temp |=3D 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 !=3D bo) > @@ -4177,6 +4237,10 @@ static int intel_crtc_cursor_set(struct drm_crtc *= crtc, > =20 > intel_crtc->cursor_addr =3D addr; > intel_crtc->cursor_bo =3D bo; > + intel_crtc->cursor_width =3D width; > + intel_crtc->cursor_height =3D height; > + > + intel_crtc_update_cursor(crtc); > =20 > return 0; > fail_unpin: > @@ -4190,34 +4254,12 @@ fail: > =20 > static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) > { > - struct drm_device *dev =3D crtc->dev; > - struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > - struct intel_framebuffer *intel_fb; > - int pipe =3D intel_crtc->pipe; > - uint32_t temp =3D 0; > - uint32_t adder; > - > - if (crtc->fb) { > - intel_fb =3D to_intel_framebuffer(crtc->fb); > - intel_mark_busy(dev, intel_fb->obj); > - } > - > - if (x < 0) { > - temp |=3D CURSOR_POS_SIGN << CURSOR_X_SHIFT; > - x =3D -x; > - } > - if (y < 0) { > - temp |=3D CURSOR_POS_SIGN << CURSOR_Y_SHIFT; > - y =3D -y; > - } > =20 > - temp |=3D x << CURSOR_X_SHIFT; > - temp |=3D y << CURSOR_Y_SHIFT; > + intel_crtc->cursor_x =3D x; > + intel_crtc->cursor_y =3D y; > =20 > - adder =3D intel_crtc->cursor_addr; > - I915_WRITE((pipe =3D=3D 0) ? CURAPOS : CURBPOS, temp); > - I915_WRITE((pipe =3D=3D 0) ? CURABASE : CURBBASE, adder); > + intel_crtc_update_cursor(crtc); > =20 > return 0; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/inte= l_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; > }; > =20 > #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 Reviewed-by: Christopher James Halse Rogers --=-iMJfaivA0BChtRfRbOI+ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) iQEcBAABAgAGBQJMNviFAAoJEBUHnPSAqAygNFkH/RGsDY4BlYCtscu2hwlbBQL8 KwAastwYdLRjD1mWhnBkxdGBllAaP3kyM8D1a1O+T0Pr+4UqCuB9L/zbSFWbu2T9 1L2fSErUmo8lTWhSmMmKjHh5uEX+OLHxC+UtfpD7ypEyCM/ve6YJwiDY64pI2ahT 9OnQl8XR8it9sGraRCxXt0Y2ZNckCjXYiAMjzHl5KIxeAJ3Hdh/CDrPjTA4XVeMp E4TknoOvZ/2XfuUM///zp+VL2Z0qzPdEgZ3CdnwMyoyiTOWp7Gc8nSgv/Dsyr728 qMBsTsqCXuZbDY0l+5THjyt1r3jPus67nGTknEZkgEMZOM0dsfYLEdcwekZTmyg= =Gk0y -----END PGP SIGNATURE----- --=-iMJfaivA0BChtRfRbOI+-- --===============0038730546== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0038730546==--