Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 05, 2013 at 01:45:47PM +0200, Daniel Vetter wrote:
> On Thu, Sep 05, 2013 at 02:33:20PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 04, 2013 at 08:50:13PM +0200, Daniel Vetter wrote:
> > > On Fri, Aug 30, 2013 at 1:50 AM,  <mengdong.lin@intel.com> wrote:
> > > > +       /* Wait for 2 vertical blanks */
> > > > +       intel_wait_for_vblank(dev, pipe);
> > > > +       intel_wait_for_vblank(dev, pipe);
> > > > +
> > > > +       /* Disable audio PD. This is optional as per Bspec.  */
> > > > +       temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > > > +       temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> > > > +       I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> > > 
> > > If this is optional do we really need the two vblank waits above?
> > > Adding them just for fun when we generally try to rip out as many
> > > vblank waits as possible from the modeset code isn't all that great
> > > ...
> > 
> > One idea I had for these kinds of vblank waits (there also one required
> > for IPS for instance) is that we might just sample a vblank counter
> > after the first step, then at the latest point we can, we'd wait for the
> > frame counter to have passed the sampled vaoue + whatever extra is
> > needed. That might allow us to do other stuff in parallel while the
> > required number of vblanks will elapese.
> 
> My solution for this is to have vblank work items that we can use to chain
> off all these things. We also need them for pageflips e.g. when
> re-enabling fbc or similar stuff. The problem is a bit that for switching
> things off like in a modeset the synchronization can get hairy and will be
> little-tested. For enabling as long as we share the code with the nuclear
> pageflip code we should be fine though ...
> 
> Hence why I think we should try rather hard to avoid these vblank waits in
> the first place.

Yeah, for enabling we definitely want to execute all that stuff
asynchronously.

The disable case is more problematic. For atomic pageflip we might get
away with returning -EAGAIN or something, but we have to block in the
full modeset case unless we want to move the entire modeset to a work
queue. So this trick would mostly be relevant for the disable during
modeset case.

But of course we could do that stuff via some kind of
schedule_vblank_work(currentl_vbl_count + X) and synchronize_vblank_work()
just before the critical step that needs the vblank work to be done. But
the locking and bugs might be more complicated in this case.

-- 
Ville Syrjälä
Intel OTC