Re: [PATCH 2/2] drm/i915: improve assert_panel_unlocked

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Aug 21, 2014 at 12:01:07PM -0300, Paulo Zanoni wrote:
> 2014-08-21 11:56 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > 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 <jani.nikula@intel.com>
> >>
> >> ---
> >>
> >> 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/i915/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 = dev_priv->dev;
> >> +     int pp_reg;
> >>       u32 val;
> >>       enum pipe panel_pipe = PIPE_A;
> >>       bool locked = 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 = PCH_PP_CONTROL;
> >> -             lvds_reg = PCH_LVDS;
> >> +             port_sel = I915_READ(PCH_PP_ON_DELAYS) & PANEL_PORT_SELECT_MASK;
> >> +
> >> +             if (port_sel == PANEL_PORT_SELECT_LVDS &&
> >> +                 I915_READ(PCH_LVDS) & LVDS_PIPEB_SELECT)
> >> +                     panel_pipe = PIPE_B;
> >> +             /* XXX: else fix for eDP */
> >> +     } else if (IS_VALLEYVIEW(dev)) {
> >> +             /* presumably write lock depends on pipe, not port select */
> >
> > 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 locked
> > to the port. For that we'd probably just have to check both power sequencers
> > 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älä <ville.syrjala@linux.intel.com>
> >
> >> +             pp_reg = VLV_PIPE_PP_CONTROL(pipe);
> >> +             panel_pipe = pipe;
> >>       } else {
> >>               pp_reg = PP_CONTROL;
> >> -             lvds_reg = LVDS;
> >> +             if (I915_READ(LVDS) & LVDS_PIPEB_SELECT)
> >> +                     panel_pipe = PIPE_B;
> >>       }
> >>
> >>       val = I915_READ(pp_reg);
> >> @@ -1211,9 +1227,6 @@ static void assert_panel_unlocked(struct drm_i915_private *dev_priv,
> >>           ((val & PANEL_UNLOCK_MASK) == PANEL_UNLOCK_REGS))
> >>               locked = false;
> >>
> >> -     if (I915_READ(lvds_reg) & LVDS_PIPEB_SELECT)
> >> -             panel_pipe = PIPE_B;
> >> -
> >>       WARN(panel_pipe == 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älä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC