Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@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@linux.intel.com]
>> Sent: Monday, August 10, 2015 8:10 PM
>> To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
>> gfx@lists.freedesktop.org; daniel.vetter@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@intel.com wrote:
>> > From: Libin Yang <libin.yang@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.

BR,
Jani.


>
>> 
>> Also some comments in-line, provided you've convinced me first. ;)
>> 
>> > Signed-off-by: Libin Yang <libin.yang@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx