From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 2/2] drm/i915: improve assert_panel_unlocked Date: Fri, 22 Aug 2014 11:00:15 +0300 Message-ID: <20140822080015.GQ4193@intel.com> References: <1408622786-19318-1-git-send-email-jani.nikula@intel.com> <1408622786-19318-2-git-send-email-jani.nikula@intel.com> <20140821145640.GO4193@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 7407E6E36C for ; Fri, 22 Aug 2014 01:00:19 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: Jani Nikula , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Thu, Aug 21, 2014 at 12:01:07PM -0300, Paulo Zanoni wrote: > 2014-08-21 11:56 GMT-03:00 Ville Syrj=E4l=E4 : > > On Thu, Aug 21, 2014 at 03:06:26PM +0300, Jani Nikula wrote: > >> Fix assert_panel_unlocked for vlv/chv, and improve it a bit for > >> non-LVDS. Also don't pretend it works for DDI. There's still work to do > >> to get this right for eDP on PCH platforms, but this is a start. > >> > >> Signed-off-by: Jani Nikula > >> > >> --- > >> > >> So I wanted to quickly fix assert_panel_unlocked, but for such a short > >> piece of code it's too involved to _quickly_ get right across all > >> platforms. I think this is a worthwhile improvement though. > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++++++++------- > >> 1 file changed, 20 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i9= 15/intel_display.c > >> index fe1d00dc9ef5..d6b48496d7f4 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -1193,17 +1193,33 @@ void assert_fdi_rx_pll(struct drm_i915_private= *dev_priv, > >> static void assert_panel_unlocked(struct drm_i915_private *dev_priv, > >> enum pipe pipe) > >> { > >> - int pp_reg, lvds_reg; > >> + struct drm_device *dev =3D dev_priv->dev; > >> + int pp_reg; > >> u32 val; > >> enum pipe panel_pipe =3D PIPE_A; > >> bool locked =3D true; > >> > >> - if (HAS_PCH_SPLIT(dev_priv->dev)) { > >> + if (HAS_DDI(dev)) { > >> + /* XXX: this neither works nor gets called for DDI */ > > > > Not sure why the XXX here. Seems to me there's nothing to fix here for > > DDI. Maybe just make that a WARN_ON(HAS_DDI()) or just remove the check > > entirely. > = > As far as I remember, the "abcd" stuff is not even used/needed on DDI. > But this is just what my memory tells me, it may be wrong. Someone > needs to double-check. Bspec just says "spare" for those bits. > = > > > >> + return; > >> + } else if (HAS_PCH_SPLIT(dev)) { > >> + u32 port_sel; > >> + > >> pp_reg =3D PCH_PP_CONTROL; > >> - lvds_reg =3D PCH_LVDS; > >> + port_sel =3D I915_READ(PCH_PP_ON_DELAYS) & PANEL_PORT_SE= LECT_MASK; > >> + > >> + if (port_sel =3D=3D PANEL_PORT_SELECT_LVDS && > >> + I915_READ(PCH_LVDS) & LVDS_PIPEB_SELECT) > >> + panel_pipe =3D PIPE_B; > >> + /* XXX: else fix for eDP */ > >> + } else if (IS_VALLEYVIEW(dev)) { > >> + /* presumably write lock depends on pipe, not port selec= t */ > > > > Hmm. This is a good question. Needs a bit if testing I suppose. In the > > worst case it somehow gets tied in with how the power sequencer gets lo= cked > > to the port. For that we'd probably just have to check both power seque= ncers > > here and complain if either has the registers locked. Or maybe we should > > just do that anyway because it's such a simple solution? But we could > > do that simply by calling assert_panel_unlocked() twice (once for each = pipe) > > from VLV specific code, so this patch seems to be exactly what we need = as > > a first step. > > > > Apart from the XXX in the comment: > > Reviewed-by: Ville Syrj=E4l=E4 > > > >> + pp_reg =3D VLV_PIPE_PP_CONTROL(pipe); > >> + panel_pipe =3D pipe; > >> } else { > >> pp_reg =3D PP_CONTROL; > >> - lvds_reg =3D LVDS; > >> + if (I915_READ(LVDS) & LVDS_PIPEB_SELECT) > >> + panel_pipe =3D PIPE_B; > >> } > >> > >> val =3D I915_READ(pp_reg); > >> @@ -1211,9 +1227,6 @@ static void assert_panel_unlocked(struct drm_i91= 5_private *dev_priv, > >> ((val & PANEL_UNLOCK_MASK) =3D=3D PANEL_UNLOCK_REGS)) > >> locked =3D false; > >> > >> - if (I915_READ(lvds_reg) & LVDS_PIPEB_SELECT) > >> - panel_pipe =3D PIPE_B; > >> - > >> WARN(panel_pipe =3D=3D pipe && locked, > >> "panel assertion failure, pipe %c regs locked\n", > >> pipe_name(pipe)); > >> -- > >> 1.9.1 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrj=E4l=E4 > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > = > = > = > -- = > Paulo Zanoni -- = Ville Syrj=E4l=E4 Intel OTC