[Intel-gfx] [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset
Jani Nikula
jani.nikula at linux.intel.com
Wed Sep 2 02:02:42 PDT 2015
On Wed, 02 Sep 2015, "Yang, Libin" <libin.yang at intel.com> wrote:
> Hi Jani,
>
>> -----Original Message-----
>> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
>> Sent: Wednesday, September 02, 2015 4:20 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;
>> ville.syrjala at 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 at intel.com wrote:
>> > From: Libin Yang <libin.yang at 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 at canonical.com
>>
>> >
>> > Signed-off-by: Libin Yang <libin.yang at 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
More information about the Intel-gfx
mailing list