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

Jani Nikula jani.nikula at linux.intel.com
Wed Aug 12 06:23:12 PDT 2015 

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?

Jani.

> hw modeset while computing the pipe_config. That would probably take care
> of my rwm concerns, give this thing proper structure and also make sure
> N/CTS never get out of sync. Also then we can track this stuff in the
> pipe_config and throw the state checker at it to make sure it's all fine.
> -Daniel
>
>> 
>> Please find one more code comment below.
>> 
>> >
>> >> 
>> >> Timeout.
>> >> 
>> >> 
>> >> BR,
>> >> Jani.
>> >> 
>> >> 
>> >> >
>> >> >>
>> >> >> BR,
>> >> >> Jani.
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >>
>> >> >> >> Also some comments in-line, provided you've convinced me first. ;)
>> >> >> >>
>> >> >> >> > Signed-off-by: Libin Yang <libin.yang at intel.com>
>> >> >> >> > ---
>> >> >> >> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
>> >> >> >> >  drivers/gpu/drm/i915/intel_audio.c | 42
>> >> >> >> ++++++++++++++++++++++++++++++++++++++
>> >> >> >> >  2 files changed, 48 insertions(+)
>> >> >> >> >
>> >> >> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> >> >> >> b/drivers/gpu/drm/i915/i915_reg.h
>> >> >> >> > index da2d128..85f3beb 100644
>> >> >> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> >> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> >> >> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
>> >> >> >> >
>> >> 	_HSW_AUD_DIG_CNVT_2)
>> >> >> >> >  #define DIP_PORT_SEL_MASK		0x3
>> >> >> >> >
>> >> >> >> > +#define _HSW_AUD_STR_DESC_1		0x65084
>> >> >> >> > +#define _HSW_AUD_STR_DESC_2		0x65184
>> >> >> >> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
>> >> >> >> > +
>> >> _HSW_AUD_STR_DESC_1,
>> >> >> >> 	\
>> >> >> >> > +
>> >> _HSW_AUD_STR_DESC_2)
>> >> >> >> > +
>> >> >> >> >  #define _HSW_AUD_EDID_DATA_A		0x65050
>> >> >> >> >  #define _HSW_AUD_EDID_DATA_B		0x65150
>> >> >> >> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
>> >> >> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> >> >> >> b/drivers/gpu/drm/i915/intel_audio.c
>> >> >> >> > index eddf37f..082f96d 100644
>> >> >> >> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> >> >> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> >> >> >> > @@ -235,6 +235,9 @@ static void
>> >> >> hsw_audio_codec_enable(struct
>> >> >> >> drm_connector *connector,
>> >> >> >> >  	const uint8_t *eld = connector->eld;
>> >> >> >> >  	uint32_t tmp;
>> >> >> >> >  	int len, i;
>> >> >> >> > +	int cvt_idx;
>> >> >> >> > +	int n_low, n_up, n;
>> >> >> >> > +	int base_rate, mul, div, rate;
>> >> >> >> >
>> >> >> >> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
>> >> bytes
>> >> >> >> ELD\n",
>> >> >> >> >  		      pipe_name(pipe), drm_eld_size(eld));
>> >> >> >> > @@ -267,6 +270,21 @@ static void
>> >> >> hsw_audio_codec_enable(struct
>> >> >> >> drm_connector *connector,
>> >> >> >> >  	tmp |= AUDIO_ELD_VALID(pipe);
>> >> >> >> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>> >> >> >> >
>> >> >> >> > +	if ((mode->clock == 297000) ||
>> >> >> >> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000,
>> >> >> >> 1001))) {
>> 
>> Please abstract that condition into a helper and give it a helpful
>> name. That's repeated many times now, in this and negated forms, and I
>> want the compiler to ensure it's the same in each place.
>> 
>> Thanks,
>> Jani.
>> 
>> >> >> >> > +		tmp =
>> >> I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
>> >> >> >> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
>> >> >> >> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
>> >> >> >> > +		base_rate = tmp & (1 << 14);
>> >> >> >> > +		if (base_rate == 0)
>> >> >> >> > +			rate = 48000;
>> >> >> >> > +		else
>> >> >> >> > +			rate = 44100;
>> >> >> >> > +		mul = (tmp & (0x7 << 11)) + 1;
>> >> >> >> > +		div = (tmp & (0x7 << 8)) + 1;
>> >> >> >> > +		rate = rate * mul / div;
>> >> >> >> > +	}
>> >> >> >>
>> >> >> >> This should be abstracted to a separate function.
>> >> >> >
>> >> >> > Yes. I will do it.
>> >> >> >
>> >> >> >>
>> >> >> >> > +
>> >> >> >> >  	/* Enable timestamps */
>> >> >> >> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >> >> >> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> >> >> >> > @@ -276,6 +294,30 @@ static void
>> >> >> hsw_audio_codec_enable(struct
>> >> >> >> drm_connector *connector,
>> >> >> >> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >> >> >> >  	else
>> >> >> >> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
>> >> >> >> > +
>> >> >> >> > +	if ((mode->clock != 297000) &&
>> >> >> >> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000,
>> >> >> >> 1001))) {
>> >> >> >> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>> >> >> >> > +	} else {
>> >> >> >> > +		for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
>> >> >> >> > +			if ((rate == aud_ncts[i].sample_rate) &&
>> >> >> >> > +				(mode->clock ==
>> >> aud_ncts[i].clock)) {
>> >> >> >> > +				n = aud_ncts[i].n;
>> >> >> >> > +				break;
>> >> >> >> > +			}
>> >> >> >> > +		}
>> >> >> >> > +		if (n != 0) {
>> >> >> >> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
>> >> >> >> > +			n_low = n & 0xfff;
>> >> >> >> > +			n_up = (n >> 12) & 0xff;
>> >> >> >> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
>> >> >> >> > +			tmp &= ~AUD_CONFIG_UPPER_N_MASK;
>> >> >> >> > +			tmp |= (n_up <<
>> >> >> >> AUD_CONFIG_UPPER_N_SHIFT);
>> >> >> >> > +			tmp &=
>> >> ~AUD_CONFIG_LOWER_N_MASK;
>> >> >> >> > +			tmp |= (n_low <<
>> >> >> >> AUD_CONFIG_LOWER_N_SHIFT);
>> >> >> >> > +		}
>> >> >> >> > +	}
>> >> >> >>
>> >> >> >> Please de-duplicate the copy-paste from patch 2. At the very least
>> >> >> you
>> >> >> >> should use a helper for finding the parameters from the table.
>> >> >> >
>> >> >> > OK. I see.
>> >> >> >
>> >> >> > Regards,
>> >> >> > Libin
>> >> >> >
>> >> >> >>
>> >> >> >> > +
>> >> >> >> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> >> >> >> >  }
>> >> >> >> >
>> >> >> >> > --
>> >> >> >> > 1.9.1
>> >> >> >> >
>> >> >> >> > _______________________________________________
>> >> >> >> > Intel-gfx mailing list
>> >> >> >> > Intel-gfx at lists.freedesktop.org
>> >> >> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >> >> >>
>> >> >> >> --
>> >> >> >> Jani Nikula, Intel Open Source Technology Center
>> >> >>
>> >> >> --
>> >> >> Jani Nikula, Intel Open Source Technology Center
>> >> 
>> >> --
>> >> Jani Nikula, Intel Open Source Technology Center
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center

More information about the Intel-gfx mailing list