From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 1/3] drm/i915: add asserts for cursor state in crtc mode set Date: Wed, 4 Sep 2013 15:11:22 +0300 Message-ID: <20130904121122.GQ11428@intel.com> References: <1378293610-26631-1-git-send-email-jani.nikula@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 2012CE66AD for ; Wed, 4 Sep 2013 05:11:45 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1378293610-26631-1-git-send-email-jani.nikula@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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: Jani Nikula Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Sep 04, 2013 at 02:20:08PM +0300, Jani Nikula wrote: > The cursor is supposed to be disabled during crtc mode set (disabled by > ctrc disable). Assert this is the case. > = > Suggested-by: Chris Wilson > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index d88057e..89243cc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1069,6 +1069,26 @@ static void assert_panel_unlocked(struct drm_i915_= private *dev_priv, > pipe_name(pipe)); > } > = > +static void assert_cursor(struct drm_i915_private *dev_priv, > + enum pipe pipe, bool state) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + bool cur_state; > + > + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) This could be (gen >=3D7 && !VLV), but we have the same check in = intel_crtc_update_cursor() so that should be changed as well. Can be left for another patch I suppose. > + cur_state =3D I915_READ(CURCNTR_IVB(pipe)) & CURSOR_MODE; > + else if (IS_845G(dev) || IS_I865G(dev)) > + cur_state =3D I915_READ(_CURACNTR) & CURSOR_ENABLE; > + else > + cur_state =3D I915_READ(CURCNTR(pipe)) & CURSOR_MODE; First I was thingking that can't be right, but looks like it is. Weird how they stuffed the cursor registers differently on mobile and desktop during the gen2 days. > + > + WARN(cur_state !=3D state, > + "cursor on pipe %c assertion failure (expected %s, current %s)\n", > + pipe_name(pipe), state_string(state), state_string(cur_state)); > +} > +#define assert_cursor_enabled(d, p) assert_cursor(d, p, true) > +#define assert_cursor_disabled(d, p) assert_cursor(d, p, false) I guess we don't really need assert_cursor_enabled() but no real harm in having it. > + > void assert_pipe(struct drm_i915_private *dev_priv, > enum pipe pipe, bool state) > { > @@ -4856,6 +4876,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > const intel_limit_t *limit; > int ret; > = > + assert_cursor_disabled(dev_priv, pipe); > + > for_each_encoder_on_crtc(dev, crtc, encoder) { > switch (encoder->type) { > case INTEL_OUTPUT_LVDS: > @@ -5754,6 +5776,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *= crtc, > struct intel_shared_dpll *pll; > int ret; > = > + assert_cursor_disabled(dev_priv, pipe); > + > for_each_encoder_on_crtc(dev, crtc, encoder) { > switch (encoder->type) { > case INTEL_OUTPUT_LVDS: > @@ -6270,6 +6294,8 @@ static int haswell_crtc_mode_set(struct drm_crtc *c= rtc, > int plane =3D intel_crtc->plane; > int ret; > = > + assert_cursor_disabled(dev_priv, intel_crtc->pipe); > + > if (!intel_ddi_pll_mode_set(crtc)) > return -EINVAL; I'd slap the asserts to the same places as the asserts for other planes. But maybe we'd want to have pipe and plane asserts in mode_set as well, just in case we manage to mess things up and call mode_set w/o disabling the pipe first. Also looks like the plane assert should be killed from ironlake_fdi_link_train(). We shouldn't need a plain to train the FDI link as the pipe will anyway pump out pixels. -- = Ville Syrj=E4l=E4 Intel OTC