| Message ID | 1408622786-19318-2-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + 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
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. > >> + 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
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
On Fri, Aug 22, 2014 at 11:00:15AM +0300, Ville Syrjälä wrote: > 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. Iirc it's also not used for edp on older platforms, but only for lvds. There's definitely a lot of fun in this area ... -Daniel
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 */ + 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 */ + 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));
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(-)