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

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

On Wed, 02 Sep 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> Hi Jani,
>
>> -----Original Message-----
>> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> Sent: Wednesday, September 02, 2015 4:20 PM
>> To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
>> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch;
>> ville.syrjala@linux.intel.com
>> Cc: Yang, Libin
>> Subject: Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset
>> 
>> On Wed, 02 Sep 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 values, the N/CTS need to be set manually if audio
>> > is playing.
>> 
>> Do we still need this patch after David Henningsson's series [1]? IIUC
>> you will now always get the callback on modesets, so you should be
>> able
>> to, uh, callback on the callback to set audio rate. Granted, with this I
>> suppose you reduce the time there might be wrong N/CTS after enable.
>
> Yes, we need. David's patch will not trigger the sync_audio_rate
> callback.

Okay.

>
>> 
>> Some other comments inline.
>> 
>> [1] http://mid.gmane.org/1440781350-12053-1-git-send-email-
>> david.henningsson@canonical.com
>> 
>> >
>> > Signed-off-by: Libin Yang <libin.yang@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_reg.h    |  8 ++++++++
>> >  drivers/gpu/drm/i915/intel_audio.c | 40
>> +++++++++++++++++++++++++++++++++++++-
>> >  2 files changed, 47 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> > index ae7fa3e..5bdee36 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -7058,6 +7058,8 @@ enum skl_disp_power_wells {
>> >  					_HSW_AUD_MISC_CTRL_A, \
>> >  					_HSW_AUD_MISC_CTRL_B)
>> >
>> > +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
>> 
>> Nitpick. The bit fields are not defined.
>
> OK, I will add the bit definition. 
>
>> 
>> > +
>> >  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
>> >  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
>> >  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
>> > @@ -7072,6 +7074,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)
>> 
>> Nitpick. The bit fields are not defined. I think it's fine to use _PIPE
>> macro, but you should probably use "converter" or "cvt" or something
>> for
>> the parameter name to not mislead people into thinking this is pipe
>> based.
>
> Do you mean it should be like:
> _PIPE(cvt, _HSW_AUD_STR_DESC_1, ...) ?

Yes.

>
>> 
>> > +
>> >  #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 a021720..acdb68c 100644
>> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> > @@ -140,6 +140,27 @@ static bool audio_rate_need_prog(struct
>> intel_crtc *crtc,
>> >  		return false;
>> >  }
>> >
>> > +static int audio_config_get_rate(struct drm_i915_private *dev_priv,
>> > +						enum pipe pipe)
>> > +{
>> > +	uint32_t tmp;
>> > +	int cvt_idx;
>> > +	int base_rate, mul, div, rate;
>> > +
>> > +	tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
>> > +	cvt_idx = (tmp >> (pipe * 8)) & 0xff;
>> 
>> This one needs to be addressed. Are you sure it's indexed by pipe? The
>> spec is vague.
>
> Yes, I did lot of test to confirm it.
>
>> 
>> Bits 7:0 are "control B", 15:8 are "control C", and so on, while your
>> (tmp >> (pipe * 8)) maps pipe A to control B, pipe B to control C,
>> etc. This would speak for port, not pipe, as pipe A should be valid
>> while port A not.
>
> We found there is something wrong/or vague in the spec.

Indeed.

>
>> 
>> > +	tmp = I915_READ(AUD_STR_DESC(cvt_idx));
>> > +	base_rate = tmp & (1 << 14);
>> 
>> Nitpick. We prefer (tmp & MASK) >> SHIFT.
>
> OK, I will change it.
>
>> 
>> > +	if (base_rate == 0)
>> > +		rate = 48000;
>> > +	else
>> > +		rate = 44100;
>> > +	mul = (tmp & (0x7 << 11)) + 1;
>> > +	div = (tmp & (0x7 << 8)) + 1;
>> 
>> This one needs to be addressed. This is bogus. The +1 would work if
>> you
>> actually did (tmp & MASK) >> SHIFT, but now you add it to the shifted
>> value. Your multipliers and divisors are *way* off.
>
> OK, I will check it and change the patch.
>> 
>> NAK on this patch.
>> 
>> > +	rate = rate * mul / div;
>> > +	return rate;
>> > +}
>> > +
>> >  static bool intel_eld_uptodate(struct drm_connector *connector,
>> >  			       int reg_eldv, uint32_t bits_eldv,
>> >  			       int reg_elda, uint32_t bits_elda,
>> > @@ -265,6 +286,8 @@ static void hsw_audio_codec_enable(struct
>> drm_connector *connector,
>> >  	const uint8_t *eld = connector->eld;
>> >  	uint32_t tmp;
>> >  	int len, i;
>> > +	int n_low, n_up, n;
>> > +	int rate;
>> >
>> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
>> ELD\n",
>> >  		      pipe_name(pipe), drm_eld_size(eld));
>> > @@ -302,12 +325,27 @@ static void
>> hsw_audio_codec_enable(struct drm_connector *connector,
>> >  	/* Enable timestamps */
>> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> > -	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>> >  	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
>> >  	if (intel_pipe_has_type(intel_crtc,
>> INTEL_OUTPUT_DISPLAYPORT))
>> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >  	else
>> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
>> > +
>> > +	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>> > +	if (audio_rate_need_prog(intel_crtc, mode)) {
>> > +		rate = audio_config_get_rate(dev_priv, pipe);
>> > +		n = audio_config_get_n(mode, rate);
>> > +		if (n != 0) {
>> > +			n_low = n & 0xfff;
>> > +			n_up = (n >> 12) & 0xff;
>> > +			tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
>> > +
>> AUD_CONFIG_LOWER_N_MASK);
>> > +			tmp |= ((n_up <<
>> AUD_CONFIG_UPPER_N_SHIFT) |
>> > +					(n_low <<
>> AUD_CONFIG_LOWER_N_SHIFT) |
>> > +
>> 	AUD_CONFIG_N_PROG_ENABLE);
>> 
>> Nitpick. I'd prefer some sharing with the similar blocks from the
>> earlier patch. Also a debug message on n == 0 would be nice; you
>> probably didn't notice your audio_config_get_rate() wasn't working
>> right
>> because this silently fell back to the automatic mode here.
>
> OK, I will add the msg. As you and Ville are insisting on
> sharing code, I will do it in next version.

Well, really, I'm fine with having that part duplicated as-is for now,
we can fix it later. More important to focus on getting
audio_config_get_rate() right.

I don't know if you're still targeting v4.3 with this (up to Takashi I
guess) we'll really need to wrap this up soon.

BR,
Jani.

>
> Regards,
> Libin
>> 
>> > +		}
>> > +	}
>> > +
>> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> >
>> >  	mutex_unlock(&dev_priv->av_mutex);
>> > --
>> > 1.9.1
>> >
>> 
>> --
>> 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