Hi Jani, Thanks for careful reviewing for all the patches, please see my comments. > -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] > Sent: Monday, August 10, 2015 8:10 PM > To: Yang, Libin; alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx > Subject: Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in > modeset > > On Mon, 10 Aug 2015, libin.yang@xxxxxxxxx wrote: > > From: Libin Yang <libin.yang@xxxxxxxxx> > > > > 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. > > Also some comments in-line, provided you've convinced me first. ;) > > > Signed-off-by: Libin Yang <libin.yang@xxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
