On Wed, 02 Sep 2015, "Yang, Libin" <libin.yang@xxxxxxxxx> wrote: > Hi Jani, > >> -----Original Message----- >> From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >> Sent: Wednesday, September 02, 2015 4:20 PM >> To: Yang, Libin; alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; intel- >> gfx@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx; >> ville.syrjala@xxxxxxxxxxxxxxx >> Cc: Yang, Libin >> Subject: Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset >> >> On Wed, 02 Sep 2015, libin.yang@xxxxxxxxx wrote: >> > From: Libin Yang <libin.yang@xxxxxxxxx> >> > >> > 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@xxxxxxxxxxxxx >> >> > >> > Signed-off-by: Libin Yang <libin.yang@xxxxxxxxx> >> > --- >> > 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
