[Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Yang, Libin]

Hi Daniel,

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Wednesday, August 12, 2015 9:43 PM
> To: Jani Nikula
> Cc: Daniel Vetter; Yang, Libin; alsa-devel at alsa-project.org;
> tiwai at suse.de; intel-gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch
> Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in
> modeset
> 
> On Wed, Aug 12, 2015 at 04:23:12PM +0300, Jani Nikula wrote:
> > On Wed, 12 Aug 2015, Daniel Vetter <daniel at ffwll.ch> wrote:
> > > On Wed, Aug 12, 2015 at 09:20:17AM +0300, Jani Nikula wrote:
> > >> On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang at intel.com> wrote:
> > >> > Hi Jani,
> > >> >
> > >> >> -----Original Message-----
> > >> >> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> > >> >> Sent: Tuesday, August 11, 2015 4:14 PM
> > >> >> To: Yang, Libin; alsa-devel at alsa-project.org; tiwai at suse.de;
> intel-
> > >> >> gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch
> > >> >> Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper
> N/CTS in
> > >> >> modeset
> > >> >>
> > >> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang at intel.com>
> wrote:
> > >> >> > Hi Jani,
> > >> >> >
> > >> >> >> -----Original Message-----
> > >> >> >> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> > >> >> >> Sent: Tuesday, August 11, 2015 3:14 PM
> > >> >> >> To: Yang, Libin; alsa-devel at alsa-project.org; tiwai at suse.de;
> intel-
> > >> >> >> gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch
> > >> >> >> Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper
> N/CTS in
> > >> >> >> modeset
> > >> >> >>
> > >> >> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang at intel.com>
> wrote:
> > >> >> >> > Hi Jani,
> > >> >> >> >
> > >> >> >> > Thanks for careful reviewing for all the patches, please
> > >> >> >> > see my comments.
> > >> >> >> >
> > >> >> >> >> -----Original Message-----
> > >> >> >> >> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> > >> >> >> >> Sent: Monday, August 10, 2015 8:10 PM
> > >> >> >> >> To: Yang, Libin; alsa-devel at alsa-project.org;
> tiwai at suse.de; intel-
> > >> >> >> >> gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch
> > >> >> >> >> Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set
> proper N/CTS
> > >> >> in
> > >> >> >> >> modeset
> > >> >> >> >>
> > >> >> >> >> On Mon, 10 Aug 2015, libin.yang at intel.com wrote:
> > >> >> >> >> > From: Libin Yang <libin.yang at intel.com>
> > >> >> >> >> >
> > >> >> >> >> > When modeset occurs and the TMDS frequency is set
> to some
> > >> >> >> >> > speical value, the N/CTS need to be set manually if
> audio
> > >> >> >> >> > is playing.
> > >> >> >> >>
> > >> >> >> >> When modeset occurs, we disable everything, and then
> re-enable
> > >> >> >> >> everything, and notify the audio driver every step of the
> way. IIUC
> > >> >> >> >> there should be no audio playing across the modeset,
> and when
> > >> >> the
> > >> >> >> >> modeset is complete and we've set the valid ELD, the
> audio driver
> > >> >> >> >> should
> > >> >> >> >> call the callback from the earlier patches to set the
> correct audio
> > >> >> >> >> rate.
> > >> >> >> >>
> > >> >> >> >> Why is this patch needed? Please enlighten me.
> > >> >> >> >
> > >> >> >> > Please image this scenario: when audio is playing, the gfx
> driver
> > >> >> >> > does the modeset. In this situation, after modeset, audio
> driver
> > >> >> >> > will do nothing and continue playing. And audio driver will
> not call
> > >> >> >> > set_ncts.
> > >> >> >>
> > >> >> >> Why does the audio driver not do anything here? Whenever
> there's
> > >> >> a
> > >> >> >> modeset, the graphics driver will disable audio and
> invalidate the
> > >> >> ELD,
> > >> >> >> which should cause an interrupt to notify the audio driver
> about
> > >> >> >> this. This is no different from a hot-unplug, in fact I can't see
> how
> > >> >> >> the audio driver could even detect whether there's been a
> hotplug
> > >> >> or
> > >> >> >> not
> > >> >> >> when there's a codec disable/enable.
> > >> >> >
> > >> >> > Yes, it will trigger an interrupt to audio driver when unplug
> > >> >> > and another interrupt for hotplug. But from audio driver,
> > >> >> > we will not stop the playing and resume the playing based
> > >> >> > on the hotplug or modeset. The audio is always playing
> during
> > >> >> > the hotplug/modeset.
> > >> >>
> > >> >> But the whole display, and consequently ELD, might have
> changed
> > >> >> during
> > >> >> the hotplug, why do you assume you can just keep playing?!
> The
> > >> >> display
> > >> >> might even have changed from HDMI to DP or vice versa.
> > >> >
> > >> > The eld info changing will not impact the audio playing.
> > >> > Actually, you can image the monitor as a digital speaker, just
> > >> > like the headphone to the analog audio. Unplug the speaker
> > >> > will not impact on the audio playing. Of course , there is a
> > >> > little difference, when monitor hotplug, in the interrupt,
> > >> > we may change the codec configuration dynamically.
> > >> >
> > >> > And audio driver supply the mechanism to stop the
> > >> > audio playing when hotplug if the HW requires.
> > >> >
> > >> >>
> > >> >> We've also been putting a lot of effort into not depending on
> previous
> > >> >> state when enabling the outputs, for more reliability. We
> generally
> > >> >> don't do partial changes, we disable everything and then
> enable
> > >> >> again. And now you're suggesting we look at some register
> which for all
> > >> >> we know may have been valid for some other display?
> > >> >
> > >> > In the patch, it will not depend on previous state, I think. We
> > >> > read the register value which is based on the state after
> modeset.
> > >>
> > >> Okay, let's have the patches cleaned up and see. It'll be easier and
> > >> quicker to solicit more review on that than this expanding thread.
> > >
> > > It sounds like we actually need to ping the audio side _before_ we
> do the
> >
> > Do you mean "read _HSW_AUD_STR_DESC_*" by "ping the audio
> side" above?
> 
> No, I mean call into snd-hda-intel before we do the modeset to ask it
> for
> what it actually wants to have set up. At least it feels a bit like only
> reacting to a modeset after it's done leads to confusion. Or maybe we
> just
> need to update the register values we want at compute_time instead
> of
> doing rwm fun ...

Actually we read the _HSW_AUD_STR_DESC_* register to get
what value we are going to set. This is similar with calling audio
driver to ask for the parameters. The _HSW_AUD_STR_DESC_*
will tell us what audio sample rates it's using. :-)

Regards,
Libin
> 
> Pure gut feeling comment, I didn't look into details ;-)
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch