diff mbox

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

Message ID 1379621249-1816-5-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni Sept. 19, 2013, 8:07 p.m. UTC 
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(-)

Comments

Chris Wilson Sept. 19, 2013, 8:28 p.m. UTC | #1 
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
Daniel Vetter Sept. 20, 2013, 8:17 a.m. UTC | #2 
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
Lin, Mengdong Sept. 22, 2013, 10:37 a.m. UTC | #3 
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
Paulo Zanoni Oct. 10, 2013, 6:33 p.m. UTC | #4 
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
Daniel Vetter Oct. 10, 2013, 7:07 p.m. UTC | #5 
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
diff mbox

Patch

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);