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. > > 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, ...) ? > > > + > > #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. > > > + 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. 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
