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

Tue Aug 11 00:46:29 PDT 2015

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.

> 
> 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))) {
> >> > +		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