On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@xxxxxxxxx> wrote: > 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. Why does the audio driver not do anything here? Whenever there's a modeset, the graphics driver will disable audio and invalidate the ELD, which should cause an interrupt to notify the audio driver about this. This is no different from a hot-unplug, in fact I can't see how the audio driver could even detect whether there's been a hotplug or not when there's a codec disable/enable. BR, Jani. > >> >> 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 -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
