[Intel-gfx] [PATCH 1/3] drm/i915: add wait_for_vblank argument to intel_enable_pipe

[Paulo Zanoni]

2014/1/15 Daniel Vetter <daniel at ffwll.ch>:
> 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 at gmail.com> wrote:
>>
>> > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >
>> > 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 at intel.com>
>> > ---
>> >  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 problem with moving the vblank_wait to the caller is that early
return in case the pipe is already enabled, which is needed for the
"force pipe A quirk". This was my first attempt, when I sent this
patch a few months ago.

I wrote a few patches on top of these 3 that should remove the 3
boolean arguments. I'll send them in a few minutes. I hope they make
everybody happy :)

> 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 at virtuousgeek.org>
>>
>> --
>> Jesse Barnes, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni