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

[Daniel Vetter]

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 ...

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