[Intel-gfx] [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 at lists.freedesktop.org
> [mailto:intel-gfx-bounces+mengdong.lin=intel.com at lists.freedesktop.org] On
> Behalf Of Daniel Vetter
> Sent: Friday, September 20, 2013 4:18 PM
> To: Paulo Zanoni
> Cc: intel-gfx at 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 at 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 at 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 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx