Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, August 12, 2015 2:20 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Wed, 12 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 4:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 3:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS
in
modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
Thanks for careful reviewing for all the patches, please see my comments.
> -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Monday, August 10, 2015 8:10 PM > To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de;
intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch > Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper
N/CTS
in
> modeset > > On Mon, 10 Aug 2015, libin.yang@intel.com wrote: > > From: Libin Yang libin.yang@intel.com > > > > 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.
OK. Thanks.
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@intel.com > > --- > > 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.
OK. I will do it.
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@lists.freedesktop.org > > 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