2014/1/15 Daniel Vetter <daniel@xxxxxxxx>:
> 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 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@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
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
