On Wed, Jan 15, 2014 at 10:25:13AM -0800, Jesse Barnes wrote:
> On Thu, 19 Dec 2013 19:12:29 -0200
> Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
>
> > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >
> > Depending on the HW gen and the connector type, the pipe won't start
> > running right after we call intel_enable_pipe, so that
> > intel_wait_for_vblank call we currently have will just sit there for
> > the full 50ms timeout. So this patch adds an argument that will allow
> > us to avoid the vblank wait in case we want. Currently all the callers
> > still request for the vblank wait, so the behavior should still be the
> > same.
> >
> > We also added a POSTING_READ on the register: previously
> > intel_wait_for_vblank was acting as a POSTING_READ, but now if
> > wait_for_vblank is false we'll stkip it, so we need an explicit
> > POSTING_READ.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 869be78..6865fa2 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1753,7 +1753,7 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
> > * returning.
> > */
> > static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
> > - bool pch_port, bool dsi)
> > + bool pch_port, bool dsi, bool wait_for_vblank)
> > {
> > enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
> > pipe);
> > @@ -1796,7 +1796,9 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
> > return;
> >
> > I915_WRITE(reg, val | PIPECONF_ENABLE);
> > - intel_wait_for_vblank(dev_priv->dev, pipe);
> > + POSTING_READ(reg);
> > + if (wait_for_vblank)
> > + intel_wait_for_vblank(dev_priv->dev, pipe);
> > }
> >
> > /**
> > @@ -3558,7 +3560,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >
> > intel_update_watermarks(crtc);
> > intel_enable_pipe(dev_priv, pipe,
> > - intel_crtc->config.has_pch_encoder, false);
> > + intel_crtc->config.has_pch_encoder, false, true);
> > intel_enable_primary_plane(dev_priv, plane, pipe);
> > intel_enable_planes(crtc);
> > intel_crtc_update_cursor(crtc, true);
> > @@ -3704,7 +3706,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >
> > intel_update_watermarks(crtc);
> > intel_enable_pipe(dev_priv, pipe,
> > - intel_crtc->config.has_pch_encoder, false);
> > + intel_crtc->config.has_pch_encoder, false, true);
> >
> > if (intel_crtc->config.has_pch_encoder)
> > lpt_pch_enable(crtc);
> > @@ -4145,7 +4147,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> > intel_crtc_load_lut(crtc);
> >
> > intel_update_watermarks(crtc);
> > - intel_enable_pipe(dev_priv, pipe, false, is_dsi);
> > + intel_enable_pipe(dev_priv, pipe, false, is_dsi, true);
> > intel_enable_primary_plane(dev_priv, plane, pipe);
> > intel_enable_planes(crtc);
> > intel_crtc_update_cursor(crtc, true);
> > @@ -4183,7 +4185,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> > intel_crtc_load_lut(crtc);
> >
> > intel_update_watermarks(crtc);
> > - intel_enable_pipe(dev_priv, pipe, false, false);
> > + intel_enable_pipe(dev_priv, pipe, false, false, true);
> > intel_enable_primary_plane(dev_priv, plane, pipe);
> > intel_enable_planes(crtc);
> > /* The fixup needs to happen before cursor is enabled */
>
> Looks fine, though I echo Jani's comment. Might be better to have an
> explicitly named intel_enable_pipe_wait variant to make the code more
> readable (likewise for DSI and PCH probably, if we don't need every
> combination), but that can be a separate cleanup.
Yeah, the 3 bool arguments aren't really fun any more imo. Simplest way
would be to move the vblank wait out into the corresponding platform
crtc_enable implementation. The same could be done with the asserts, which
atm are the only reason we have the other two booleans. With that we have
sanity again and a simpel intel_enable_pipe(dev_priv, pipe).
-Daniel
>
> Reviewed-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
>
> --
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
