On Wed, 12 Aug 2015, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Wed, Aug 12, 2015 at 09:20:17AM +0300, Jani Nikula wrote: >> On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@xxxxxxxxx> wrote: >> > Hi Jani, >> > >> >> -----Original Message----- >> >> From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >> >> Sent: Tuesday, August 11, 2015 4:14 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 Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@xxxxxxxxx> wrote: >> >> > Hi Jani, >> >> > >> >> >> -----Original Message----- >> >> >> From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >> >> >> Sent: Tuesday, August 11, 2015 3:14 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 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. >> >> > >> >> > Yes, it will trigger an interrupt to audio driver when unplug >> >> > and another interrupt for hotplug. But from audio driver, >> >> > we will not stop the playing and resume the playing based >> >> > on the hotplug or modeset. The audio is always playing during >> >> > the hotplug/modeset. >> >> >> >> But the whole display, and consequently ELD, might have changed >> >> during >> >> the hotplug, why do you assume you can just keep playing?! The >> >> display >> >> might even have changed from HDMI to DP or vice versa. >> > >> > The eld info changing will not impact the audio playing. >> > Actually, you can image the monitor as a digital speaker, just >> > like the headphone to the analog audio. Unplug the speaker >> > will not impact on the audio playing. Of course , there is a >> > little difference, when monitor hotplug, in the interrupt, >> > we may change the codec configuration dynamically. >> > >> > And audio driver supply the mechanism to stop the >> > audio playing when hotplug if the HW requires. >> > >> >> >> >> We've also been putting a lot of effort into not depending on previous >> >> state when enabling the outputs, for more reliability. We generally >> >> don't do partial changes, we disable everything and then enable >> >> again. And now you're suggesting we look at some register which for all >> >> we know may have been valid for some other display? >> > >> > In the patch, it will not depend on previous state, I think. We >> > read the register value which is based on the state after modeset. >> >> Okay, let's have the patches cleaned up and see. It'll be easier and >> quicker to solicit more review on that than this expanding thread. > > It sounds like we actually need to ping the audio side _before_ we do the Do you mean "read _HSW_AUD_STR_DESC_*" by "ping the audio side" above? Jani. > hw modeset while computing the pipe_config. That would probably take care > of my rwm concerns, give this thing proper structure and also make sure > N/CTS never get out of sync. Also then we can track this stuff in the > pipe_config and throw the state checker at it to make sure it's all fine. > -Daniel > >> >> Please find one more code comment below. >> >> > >> >> >> >> Timeout. >> >> >> >> >> >> BR, >> >> Jani. >> >> >> >> >> >> > >> >> >> >> >> >> 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))) { >> >> Please abstract that condition into a helper and give it a helpful >> name. That's repeated many times now, in this and negated forms, and I >> want the compiler to ensure it's the same in each place. >> >> Thanks, >> Jani. >> >> >> >> >> > + 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 >> >> >> >> -- >> >> Jani Nikula, Intel Open Source Technology Center >> >> -- >> Jani Nikula, Intel Open Source Technology Center > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
