[PATCH 0/4] Remove useless vblank waits on Haswell

[PATCH 0/4] Remove useless vblank waits on Haswell

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

This series gets rid of those messages saying that the vblank wait timed out on
my Haswell machine. It saves 2 timeouts per pipe (one at mode_set, one at
crtc_enable), so it's 100ms we save per modeset.

The big idea exercised by this patch is that the pipe doesn't really start
running after we enable PIPECONF, so we had some useless waits. This idea
probably also applies to ILK/SNB/IVB and perhaps we could try to save some
vblanks on those Gens too.

I tested these patches on top of the 4 patches that fix the Haswell underruns +
the 2 IPS patches I just sent.

Thanks,
Paulo

Paulo Zanoni (4):
  drm/i915: WARN in case PIPECONF is already enabled
  drm/i915: don't intel_wait_for_vblank inside intel_enable_pipe
  drm/i915: remove useless vblank wait form haswell_crtc_enable
  drm/i915: skip useless vblank wait on Haswell audio sequence

 drivers/gpu/drm/i915/intel_display.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] drm/i915: WARN in case PIPECONF is already enabled

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

After the modeset rework this really shouldn't be happening, so
transform it into a WARN. A stuck pipe is a bad signal and is one of
the things that can lead to full machine hangs.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8fd13ab..5f1399d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1737,7 +1737,7 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
 
 	reg = PIPECONF(cpu_transcoder);
 	val = I915_READ(reg);
-	if (val & PIPECONF_ENABLE)
+	if (WARN_ON(val & PIPECONF_ENABLE))
 		return;
 
 	I915_WRITE(reg, val | PIPECONF_ENABLE);
-- 
1.8.3.1

Re: [PATCH 1/4] drm/i915: WARN in case PIPECONF is already enabled

[Ville Syrjälä]

On Thu, Sep 19, 2013 at 05:07:26PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> After the modeset rework this really shouldn't be happening, so
> transform it into a WARN. A stuck pipe is a bad signal and is one of
> the things that can lead to full machine hangs.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8fd13ab..5f1399d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1737,7 +1737,7 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
>  
>  	reg = PIPECONF(cpu_transcoder);
>  	val = I915_READ(reg);
> -	if (val & PIPECONF_ENABLE)
> +	if (WARN_ON(val & PIPECONF_ENABLE))
>  		return;

You're forgetting QUIRK_PIPEA_FORCE.

>  
>  	I915_WRITE(reg, val | PIPECONF_ENABLE);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/4] drm/i915: WARN in case PIPECONF is already enabled

[Paulo Zanoni]

2013/9/20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Sep 19, 2013 at 05:07:26PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> After the modeset rework this really shouldn't be happening, so
>> transform it into a WARN. A stuck pipe is a bad signal and is one of
>> the things that can lead to full machine hangs.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8fd13ab..5f1399d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -1737,7 +1737,7 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
>>
>>       reg = PIPECONF(cpu_transcoder);
>>       val = I915_READ(reg);
>> -     if (val & PIPECONF_ENABLE)
>> +     if (WARN_ON(val & PIPECONF_ENABLE))
>>               return;
>
> You're forgetting QUIRK_PIPEA_FORCE.

Oh, I only did this to reinforce my point that 49% of the gen-agnostic
Display patches break some random Gen :)

Patches 2 and 3 depended on this patch, so for now let's ignore
patches 1-2-3 of the series (leaving only the Audio patch). I'll
resend this after we merge the current stuff.

Thanks for the review,
Paulo

>
>>
>>       I915_WRITE(reg, val | PIPECONF_ENABLE);
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni

[PATCH 2/4] drm/i915: don't intel_wait_for_vblank inside intel_enable_pipe

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Depending on the Gen and encoder, the pipe won't really start after we
enable PIPECONF, so waiting for the vblank inside intel_enable_pipe
doesn't really make sense for these cases. This patch removes the
vblank wait from intel_enable_pipe, but adds vblank waiting code after
each call to intel_enable_pipe, so we'll still have the "useless"
vblank waits. Future patches will properly deal with each of the
vblank waits.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5f1399d..2f546f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1694,8 +1694,6 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
  *
  * @pipe should be %PIPE_A or %PIPE_B.
  *
- * Will wait until the pipe is actually running (i.e. first vblank) before
- * returning.
  */
 static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
 			      bool pch_port, bool dsi)
@@ -1741,7 +1739,6 @@ 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);
 }
 
 /**
@@ -3327,6 +3324,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_wait_for_vblank(dev, pipe);
 	intel_enable_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
@@ -3511,6 +3509,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_wait_for_vblank(dev, pipe);
 
 	if (intel_crtc->config.has_pch_encoder)
 		lpt_pch_enable(crtc);
@@ -3784,6 +3783,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe, false, is_dsi);
+	intel_wait_for_vblank(dev, pipe);
 	intel_enable_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
@@ -3822,6 +3822,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe, false, false);
+	intel_wait_for_vblank(dev, pipe);
 	intel_enable_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	/* The fixup needs to happen before cursor is enabled */
-- 
1.8.3.1

[PATCH 3/4] drm/i915: remove useless vblank wait form haswell_crtc_enable

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This function is called in a point where the pipe is not really
running (the fact that PIPECONF is enabled doesn't mean that the pipe
is actually running), so we pay the full 50ms timeout. Also, I
couldn't find a reason why this is needed, so just skip it.

I do have to notice we still have other vblank waits at later points
of the mode set sequence, and at least some of them make sense.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2f546f7..69e8bb6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3509,7 +3509,6 @@ 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_wait_for_vblank(dev, pipe);
 
 	if (intel_crtc->config.has_pch_encoder)
 		lpt_pch_enable(crtc);
-- 
1.8.3.1

[PATCH 4/4] drm/i915: skip useless vblank wait on Haswell audio sequence

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We call haswell_write_eld at mode_set time, not at crtc_enable time,
so the pipes are stopped, and it doesn't really make sense to wait for
a vblank: it just delays the modeset in 50ms.

Leave the code there (commented with FIXME) for now since maybe we
need a bigger rework.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 69e8bb6..b891a0c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6662,7 +6662,6 @@ static void haswell_write_eld(struct drm_connector *connector,
 {
 	struct drm_i915_private *dev_priv = connector->dev->dev_private;
 	uint8_t *eld = connector->eld;
-	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint32_t eldv;
 	uint32_t i;
@@ -6684,8 +6683,17 @@ static void haswell_write_eld(struct drm_connector *connector,
 	tmp |= (AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
 	I915_WRITE(aud_cntrl_st2, tmp);
 
-	/* Wait for 1 vertical blank */
-	intel_wait_for_vblank(dev, pipe);
+	/*
+	 * Wait for 1 vertical blank
+	 *
+	 * FIXME: We call this function at mode_set time, when the pipes are all
+	 * stopped, so it doesn't really make any sense to wait for a vblank
+	 * here: it just delays the modeset in 50ms. I'll leave the code here
+	 * because since the wait doesn't make sense at this point, maybe we
+	 * need a bigger rework. We need an Audio authority to audit this code.
+	 *
+	 * intel_wait_for_vblank(dev_priv->dev, pipe);
+	 */
 
 	/* Set ELD valid state */
 	tmp = I915_READ(aud_cntrl_st2);
-- 
1.8.3.1

Re: [PATCH 4/4] drm/i915: skip useless vblank wait on Haswell audio sequence

[Chris Wilson]

On Thu, Sep 19, 2013 at 05:07:29PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We call haswell_write_eld at mode_set time, not at crtc_enable time,
> so the pipes are stopped, and it doesn't really make sense to wait for
> a vblank: it just delays the modeset in 50ms.
> 
> Leave the code there (commented with FIXME) for now since maybe we
> need a bigger rework.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

For the series:
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: skip useless vblank wait on Haswell audio sequence

[Daniel Vetter]

On Thu, Sep 19, 2013 at 05:07:29PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We call haswell_write_eld at mode_set time, not at crtc_enable time,
> so the pipes are stopped, and it doesn't really make sense to wait for
> a vblank: it just delays the modeset in 50ms.
> 
> Leave the code there (commented with FIXME) for now since maybe we
> need a bigger rework.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 69e8bb6..b891a0c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6662,7 +6662,6 @@ static void haswell_write_eld(struct drm_connector *connector,
>  {
>  	struct drm_i915_private *dev_priv = connector->dev->dev_private;
>  	uint8_t *eld = connector->eld;
> -	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	uint32_t eldv;
>  	uint32_t i;
> @@ -6684,8 +6683,17 @@ static void haswell_write_eld(struct drm_connector *connector,
>  	tmp |= (AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
>  	I915_WRITE(aud_cntrl_st2, tmp);
>  
> -	/* Wait for 1 vertical blank */
> -	intel_wait_for_vblank(dev, pipe);
> +	/*
> +	 * Wait for 1 vertical blank
> +	 *
> +	 * FIXME: We call this function at mode_set time, when the pipes are all
> +	 * stopped, so it doesn't really make any sense to wait for a vblank
> +	 * here: it just delays the modeset in 50ms. I'll leave the code here
> +	 * because since the wait doesn't make sense at this point, maybe we
> +	 * need a bigger rework. We need an Audio authority to audit this code.
> +	 *
> +	 * intel_wait_for_vblank(dev_priv->dev, pipe);
> +	 */

This might be due to the generic recommendation that infoframes and
related stuff (audio also gets transmitted when infoframes are) should
only be changed after the vblank completed when the pipe is on.

Imo we should just ditch this (and cc: the audio guys on the patch so
they're aware).
-Daniel

>  
>  	/* Set ELD valid state */
>  	tmp = I915_READ(aud_cntrl_st2);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@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

[PATCH 4/4] drm/i915: skip useless vblank wait on Haswell audio sequence

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We call haswell_write_eld at mode_set time, not at crtc_enable time,
so the pipes are stopped, and it doesn't really make sense to wait for
a vblank: it just delays the modeset in 50ms.

v2: - Remove the big comment
    - CC Audio developers

Cc: Wang Xingchao <xingchao.wang@intel.com>
Cc: Mengdong Lin <mengdong.lin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 61621ce..e79dd45 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6663,7 +6663,6 @@ static void haswell_write_eld(struct drm_connector *connector,
 {
 	struct drm_i915_private *dev_priv = connector->dev->dev_private;
 	uint8_t *eld = connector->eld;
-	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint32_t eldv;
 	uint32_t i;
@@ -6685,9 +6684,6 @@ static void haswell_write_eld(struct drm_connector *connector,
 	tmp |= (AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
 	I915_WRITE(aud_cntrl_st2, tmp);
 
-	/* Wait for 1 vertical blank */
-	intel_wait_for_vblank(dev, pipe);
-
 	/* Set ELD valid state */
 	tmp = I915_READ(aud_cntrl_st2);
 	DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%08x\n", tmp);
-- 
1.8.3.1

Re: [PATCH 4/4] drm/i915: skip useless vblank wait on Haswell audio sequence

[Lin, Mengdong]

Hi Daniel and Paulo,

I think we need a confirmation from HW owner whether vblank wait can be skipped in audio enabling and disabling. I'll ping HW owner and involve you.
The original code just follows b-spec but there is no explanation why.

Thanks
Mengdong

> -----Original Message-----
> From: intel-gfx-bounces+mengdong.lin=intel.com@lists.freedesktop.org
> [mailto:intel-gfx-bounces+mengdong.lin=intel.com@lists.freedesktop.org] On
> Behalf Of Daniel Vetter
> Sent: Friday, September 20, 2013 4:18 PM
> To: Paulo Zanoni
> Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R
> Subject: Re: [Intel-gfx] [PATCH 4/4] drm/i915: skip useless vblank wait on
> Haswell audio sequence
> 
> On Thu, Sep 19, 2013 at 05:07:29PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > We call haswell_write_eld at mode_set time, not at crtc_enable time,
> > so the pipes are stopped, and it doesn't really make sense to wait for
> > a vblank: it just delays the modeset in 50ms.
> >
> > Leave the code there (commented with FIXME) for now since maybe we
> > need a bigger rework.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 69e8bb6..b891a0c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6662,7 +6662,6 @@ static void haswell_write_eld(struct
> > drm_connector *connector,  {
> >  	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> >  	uint8_t *eld = connector->eld;
> > -	struct drm_device *dev = crtc->dev;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	uint32_t eldv;
> >  	uint32_t i;
> > @@ -6684,8 +6683,17 @@ static void haswell_write_eld(struct
> drm_connector *connector,
> >  	tmp |= (AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> >  	I915_WRITE(aud_cntrl_st2, tmp);
> >
> > -	/* Wait for 1 vertical blank */
> > -	intel_wait_for_vblank(dev, pipe);
> > +	/*
> > +	 * Wait for 1 vertical blank
> > +	 *
> > +	 * FIXME: We call this function at mode_set time, when the pipes are all
> > +	 * stopped, so it doesn't really make any sense to wait for a vblank
> > +	 * here: it just delays the modeset in 50ms. I'll leave the code here
> > +	 * because since the wait doesn't make sense at this point, maybe we
> > +	 * need a bigger rework. We need an Audio authority to audit this code.
> > +	 *
> > +	 * intel_wait_for_vblank(dev_priv->dev, pipe);
> > +	 */
> 
> This might be due to the generic recommendation that infoframes and related
> stuff (audio also gets transmitted when infoframes are) should only be changed
> after the vblank completed when the pipe is on.
> 
> Imo we should just ditch this (and cc: the audio guys on the patch so they're
> aware).
> -Daniel
> 
> >
> >  	/* Set ELD valid state */
> >  	tmp = I915_READ(aud_cntrl_st2);
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: skip useless vblank wait on Haswell audio sequence

[Paulo Zanoni]

2013/9/22 Lin, Mengdong <mengdong.lin@intel.com>:
> Hi Daniel and Paulo,
>
> I think we need a confirmation from HW owner whether vblank wait can be skipped in audio enabling and disabling. I'll ping HW owner and involve you.
> The original code just follows b-spec but there is no explanation why.

Hi, any news on this?

Please notice that even though the spec says we need to wait for a
vblank, this code runs on a place where the pipe is disabled, so
there's no vblank to wait. Bspec also suggests Audio should only be
enabled at the very end of the mode set sequence, so the "proper" fix
would probably be to move the place where haswell_write_eld is called.
While this is not really done, I think we could probably merge this
patch because the 50ms delay here seems pointless.

Thanks,
Paulo

>
> Thanks
> Mengdong
>
>> -----Original Message-----
>> From: intel-gfx-bounces+mengdong.lin=intel.com@lists.freedesktop.org
>> [mailto:intel-gfx-bounces+mengdong.lin=intel.com@lists.freedesktop.org] On
>> Behalf Of Daniel Vetter
>> Sent: Friday, September 20, 2013 4:18 PM
>> To: Paulo Zanoni
>> Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R
>> Subject: Re: [Intel-gfx] [PATCH 4/4] drm/i915: skip useless vblank wait on
>> Haswell audio sequence
>>
>> On Thu, Sep 19, 2013 at 05:07:29PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > We call haswell_write_eld at mode_set time, not at crtc_enable time,
>> > so the pipes are stopped, and it doesn't really make sense to wait for
>> > a vblank: it just delays the modeset in 50ms.
>> >
>> > Leave the code there (commented with FIXME) for now since maybe we
>> > need a bigger rework.
>> >
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++---
>> >  1 file changed, 11 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c
>> > b/drivers/gpu/drm/i915/intel_display.c
>> > index 69e8bb6..b891a0c 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -6662,7 +6662,6 @@ static void haswell_write_eld(struct
>> > drm_connector *connector,  {
>> >     struct drm_i915_private *dev_priv = connector->dev->dev_private;
>> >     uint8_t *eld = connector->eld;
>> > -   struct drm_device *dev = crtc->dev;
>> >     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> >     uint32_t eldv;
>> >     uint32_t i;
>> > @@ -6684,8 +6683,17 @@ static void haswell_write_eld(struct
>> drm_connector *connector,
>> >     tmp |= (AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
>> >     I915_WRITE(aud_cntrl_st2, tmp);
>> >
>> > -   /* Wait for 1 vertical blank */
>> > -   intel_wait_for_vblank(dev, pipe);
>> > +   /*
>> > +    * Wait for 1 vertical blank
>> > +    *
>> > +    * FIXME: We call this function at mode_set time, when the pipes are all
>> > +    * stopped, so it doesn't really make any sense to wait for a vblank
>> > +    * here: it just delays the modeset in 50ms. I'll leave the code here
>> > +    * because since the wait doesn't make sense at this point, maybe we
>> > +    * need a bigger rework. We need an Audio authority to audit this code.
>> > +    *
>> > +    * intel_wait_for_vblank(dev_priv->dev, pipe);
>> > +    */
>>
>> This might be due to the generic recommendation that infoframes and related
>> stuff (audio also gets transmitted when infoframes are) should only be changed
>> after the vblank completed when the pipe is on.
>>
>> Imo we should just ditch this (and cc: the audio guys on the patch so they're
>> aware).
>> -Daniel
>>
>> >
>> >     /* Set ELD valid state */
>> >     tmp = I915_READ(aud_cntrl_st2);
>> > --
>> > 1.8.3.1
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@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
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 4/4] drm/i915: skip useless vblank wait on Haswell audio sequence

[Daniel Vetter]

On Thu, Oct 10, 2013 at 8:33 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2013/9/22 Lin, Mengdong <mengdong.lin@intel.com>:
>> Hi Daniel and Paulo,
>>
>> I think we need a confirmation from HW owner whether vblank wait can be skipped in audio enabling and disabling. I'll ping HW owner and involve you.
>> The original code just follows b-spec but there is no explanation why.
>
> Hi, any news on this?
>
> Please notice that even though the spec says we need to wait for a
> vblank, this code runs on a place where the pipe is disabled, so
> there's no vblank to wait. Bspec also suggests Audio should only be
> enabled at the very end of the mode set sequence, so the "proper" fix
> would probably be to move the place where haswell_write_eld is called.
> While this is not really done, I think we could probably merge this
> patch because the 50ms delay here seems pointless.

I think updating the ELD is one of those things we can do any time, as
long as it's early enough (similar to setting new pipe timings). Maybe
we need to move the updating of the eld_valid bits (which is what will
generate the unsolictid interrupt event on the audio driver side
afaik). But we have a sparate bit (plus interrupt type on the audio
side) for "audio is now enabled" and I think that's the really
important part. And that one is enabled at the right spot in the
modeset sequence afaics.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch