| Message ID | 1477438955-20345-1-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Mixing up git rebase and reset was not a good idea, will send the corrected patch. Please ignore this. On Tue, 2016-10-25 at 16:42 -0700, Dhinakaran Pandiyan wrote: > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So, > let's set this bit right before enabling the audio codec. Playing audio > without setting this bit results in pipe FIFO underruns. > > This workaround is applicable only for audio sample rates up to 96kHz. For > frequencies above 96kHz, this is insufficient and cdclk should be increased > to at least 432 MHz, just like BDW. Since, the audio driver does not > support sample rates < 48 kHz, we are safe with this fix for now. > > v2: Inlined the code change within hsw_audio_codec_enable() (Jani) > Fixed the port clock typo > Added TODO comment > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 5 +++ > drivers/gpu/drm/i915/intel_audio.c | 62 +++++++++++++++++++++++++++++++++++--- > 2 files changed, 62 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 00efaa1..76dac48 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6236,6 +6236,11 @@ enum { > #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) > #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) > > +#define _CHICKEN_TRANS_A 0x420C0 > +#define _CHICKEN_TRANS_B 0x420C4 > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, _CHICKEN_TRANS_B) > +#define SPARE_13 (1<<13) > + > /* WaCatErrorRejectionIssue */ > #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030) > #define GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11) > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > index 7093cfb..413dd50 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > + struct intel_crtc_state *crtc_config = intel_crtc->config; > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > enum pipe pipe = intel_crtc->pipe; > uint32_t tmp; > > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > > mutex_lock(&dev_priv->av_mutex); > > + /*Disable DP audio stall fix for HBR2*/ > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > + crtc_config->port_clock >= 540000) { > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > + tmp &= ~SPARE_13; > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > + } > + > /* Disable timestamps */ > tmp = I915_READ(HSW_AUD_CFG(pipe)); > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > tmp |= AUD_CONFIG_N_PROG_ENABLE; > tmp &= ~AUD_CONFIG_UPPER_N_MASK; > tmp &= ~AUD_CONFIG_LOWER_N_MASK; > - if (intel_crtc_has_dp_encoder(intel_crtc->config)) > + if (intel_crtc_has_dp_encoder(crtc_config)) > tmp |= AUD_CONFIG_N_VALUE_INDEX; > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > { > struct drm_i915_private *dev_priv = to_i915(connector->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc); > + struct intel_crtc_state *crtc_config = intel_crtc->config; > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > enum pipe pipe = intel_crtc->pipe; > enum port port = intel_encoder->port; > const uint8_t *eld = connector->eld; > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > > mutex_lock(&dev_priv->av_mutex); > > + /* Enable DP audio stall fix for HBR2 > + * > + * TODO: This workaround is applicable only for audio sample rates up > + * to 96kHz. For frequencies above 96kHz, this is insufficient and > + * cdclk should be increased to at least 432 MHz, just like BDW. Since, > + * the audio driver does not support sample rates < 48 kHz, we are safe > + * with this fix for now. > + */ > + > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > + crtc_config->port_clock >= 540000) { > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > + tmp |= SPARE_13; > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > + } > + > /* Enable audio presence detect, invalidate ELD */ > tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > tmp |= AUDIO_OUTPUT_ENABLE(pipe); > @@ -642,25 +670,49 @@ static int i915_audio_component_get_cdclk_freq(struct device *kdev) > return dev_priv->cdclk_freq; > } > > +/* > + * get the intel_encoder according to the parameter port and pipe > + * intel_encoder is saved by the index of pipe > + * MST & (pipe >= 0): return the av_enc_map[pipe], > + * when port is matched > + * MST & (pipe < 0): this is invalid > + * Non-MST & (pipe >= 0): only pipe = 0 (the first device entry) > + * will get the right intel_encoder with port matched > + * Non-MST & (pipe < 0): get the right intel_encoder with port matched > + */ > static struct intel_encoder *get_saved_enc(struct drm_i915_private *dev_priv, > int port, int pipe) > { > + struct intel_encoder *encoder; > > if (WARN_ON(pipe >= I915_MAX_PIPES)) > return NULL; > > /* MST */ > - if (pipe >= 0) > - return dev_priv->av_enc_map[pipe]; > + if (pipe >= 0) { > + encoder = dev_priv->av_enc_map[pipe]; > + /* > + * when bootup, audio driver may not know it is > + * MST or not. So it will poll all the port & pipe > + * combinations > + */ > + if (encoder != NULL && encoder->port == port && > + encoder->type == INTEL_OUTPUT_DP_MST) > + return encoder; > + } > > /* Non-MST */ > - for_each_pipe(dev_priv, pipe) { > - struct intel_encoder *encoder; > + if (pipe > 0) > + return NULL; > > + for_each_pipe(dev_priv, pipe) { > encoder = dev_priv->av_enc_map[pipe]; > if (encoder == NULL) > continue; > > + if (encoder->type == INTEL_OUTPUT_DP_MST) > + continue; > + > if (port == encoder->port) > return encoder; > }
On Wed, 26 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So, > let's set this bit right before enabling the audio codec. Playing audio > without setting this bit results in pipe FIFO underruns. > > This workaround is applicable only for audio sample rates up to 96kHz. For > frequencies above 96kHz, this is insufficient and cdclk should be increased > to at least 432 MHz, just like BDW. Since, the audio driver does not > support sample rates > 48 kHz, we are safe with this fix for now. > > v2: Inlined the code change within hsw_audio_codec_enable() (Jani) > Fixed the port clock typo > Added TODO comment > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 5 +++++ > drivers/gpu/drm/i915/intel_audio.c | 30 +++++++++++++++++++++++++++++- > 2 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 00efaa1..76dac48 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6236,6 +6236,11 @@ enum { > #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) > #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) > > +#define _CHICKEN_TRANS_A 0x420C0 > +#define _CHICKEN_TRANS_B 0x420C4 > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, _CHICKEN_TRANS_B) > +#define SPARE_13 (1<<13) > + > /* WaCatErrorRejectionIssue */ > #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030) > #define GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11) > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > index 7093cfb..894f11e 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > + struct intel_crtc_state *crtc_config = intel_crtc->config; > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > enum pipe pipe = intel_crtc->pipe; > uint32_t tmp; > > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > > mutex_lock(&dev_priv->av_mutex); > > + /*Disable DP audio stall fix for HBR2*/ Nitpick, spaces after /* and before */. > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > + crtc_config->port_clock >= 540000) { > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > + tmp &= ~SPARE_13; > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > + } Hmm. Why don't we just do this unconditionally? > + > /* Disable timestamps */ > tmp = I915_READ(HSW_AUD_CFG(pipe)); > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > tmp |= AUD_CONFIG_N_PROG_ENABLE; > tmp &= ~AUD_CONFIG_UPPER_N_MASK; > tmp &= ~AUD_CONFIG_LOWER_N_MASK; > - if (intel_crtc_has_dp_encoder(intel_crtc->config)) > + if (intel_crtc_has_dp_encoder(crtc_config)) > tmp |= AUD_CONFIG_N_VALUE_INDEX; > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > { > struct drm_i915_private *dev_priv = to_i915(connector->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc); > + struct intel_crtc_state *crtc_config = intel_crtc->config; > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > enum pipe pipe = intel_crtc->pipe; > enum port port = intel_encoder->port; > const uint8_t *eld = connector->eld; > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > > mutex_lock(&dev_priv->av_mutex); > > + /* Enable DP audio stall fix for HBR2 > + * > + * TODO: This workaround is applicable only for audio sample rates up > + * to 96kHz. For frequencies above 96kHz, this is insufficient and > + * cdclk should be increased to at least 432 MHz, just like BDW. Since, > + * the audio driver does not support sample rates > 48 kHz, we are safe > + * with this fix for now. > + */ Is this TODO required if you already have that check in patch 2/2? > + > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > + crtc_config->port_clock >= 540000) { > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > + tmp |= SPARE_13; > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > + } > + > /* Enable audio presence detect, invalidate ELD */ > tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > tmp |= AUDIO_OUTPUT_ENABLE(pipe);
On Tue, Oct 25, 2016 at 07:37:36PM -0700, Dhinakaran Pandiyan wrote: > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So, > let's set this bit right before enabling the audio codec. Playing audio > without setting this bit results in pipe FIFO underruns. > > This workaround is applicable only for audio sample rates up to 96kHz. For > frequencies above 96kHz, this is insufficient and cdclk should be increased > to at least 432 MHz, just like BDW. Since, the audio driver does not > support sample rates > 48 kHz, we are safe with this fix for now. > > v2: Inlined the code change within hsw_audio_codec_enable() (Jani) > Fixed the port clock typo > Added TODO comment > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 5 +++++ > drivers/gpu/drm/i915/intel_audio.c | 30 +++++++++++++++++++++++++++++- > 2 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 00efaa1..76dac48 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6236,6 +6236,11 @@ enum { > #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) > #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) > > +#define _CHICKEN_TRANS_A 0x420C0 > +#define _CHICKEN_TRANS_B 0x420C4 > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, _CHICKEN_TRANS_B) > +#define SPARE_13 (1<<13) > + > /* WaCatErrorRejectionIssue */ > #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030) > #define GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11) > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > index 7093cfb..894f11e 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > + struct intel_crtc_state *crtc_config = intel_crtc->config; > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > enum pipe pipe = intel_crtc->pipe; > uint32_t tmp; > > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > > mutex_lock(&dev_priv->av_mutex); > > + /*Disable DP audio stall fix for HBR2*/ > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > + crtc_config->port_clock >= 540000) { > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > + tmp &= ~SPARE_13; > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > + } > + > /* Disable timestamps */ > tmp = I915_READ(HSW_AUD_CFG(pipe)); > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > tmp |= AUD_CONFIG_N_PROG_ENABLE; > tmp &= ~AUD_CONFIG_UPPER_N_MASK; > tmp &= ~AUD_CONFIG_LOWER_N_MASK; > - if (intel_crtc_has_dp_encoder(intel_crtc->config)) > + if (intel_crtc_has_dp_encoder(crtc_config)) > tmp |= AUD_CONFIG_N_VALUE_INDEX; > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > { > struct drm_i915_private *dev_priv = to_i915(connector->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc); > + struct intel_crtc_state *crtc_config = intel_crtc->config; > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > enum pipe pipe = intel_crtc->pipe; > enum port port = intel_encoder->port; > const uint8_t *eld = connector->eld; > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > > mutex_lock(&dev_priv->av_mutex); > > + /* Enable DP audio stall fix for HBR2 > + * > + * TODO: This workaround is applicable only for audio sample rates up > + * to 96kHz. For frequencies above 96kHz, this is insufficient and > + * cdclk should be increased to at least 432 MHz, just like BDW. Since, > + * the audio driver does not support sample rates > 48 kHz, we are safe > + * with this fix for now. Where in the sound driver is this supposed 96kHz limit? I see a lot of stuff for >96kHz in the code at least. > + */ > + > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > + crtc_config->port_clock >= 540000) { > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > + tmp |= SPARE_13; > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > + } > + > /* Enable audio presence detect, invalidate ELD */ > tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > tmp |= AUDIO_OUTPUT_ENABLE(pipe); > -- > 2.7.4
On Wed, 2016-10-26 at 11:57 +0300, Jani Nikula wrote: > On Wed, 26 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So, > > let's set this bit right before enabling the audio codec. Playing audio > > without setting this bit results in pipe FIFO underruns. > > > > This workaround is applicable only for audio sample rates up to 96kHz. For > > frequencies above 96kHz, this is insufficient and cdclk should be increased > > to at least 432 MHz, just like BDW. Since, the audio driver does not > > support sample rates > 48 kHz, we are safe with this fix for now. > > > > v2: Inlined the code change within hsw_audio_codec_enable() (Jani) > > Fixed the port clock typo > > Added TODO comment > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 5 +++++ > > drivers/gpu/drm/i915/intel_audio.c | 30 +++++++++++++++++++++++++++++- > > 2 files changed, 34 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 00efaa1..76dac48 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -6236,6 +6236,11 @@ enum { > > #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) > > #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) > > > > +#define _CHICKEN_TRANS_A 0x420C0 > > +#define _CHICKEN_TRANS_B 0x420C4 > > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, _CHICKEN_TRANS_B) > > +#define SPARE_13 (1<<13) > > + > > /* WaCatErrorRejectionIssue */ > > #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030) > > #define GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11) > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > > index 7093cfb..894f11e 100644 > > --- a/drivers/gpu/drm/i915/intel_audio.c > > +++ b/drivers/gpu/drm/i915/intel_audio.c > > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > > { > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > > + struct intel_crtc_state *crtc_config = intel_crtc->config; > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > > enum pipe pipe = intel_crtc->pipe; > > uint32_t tmp; > > > > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > > > > mutex_lock(&dev_priv->av_mutex); > > > > + /*Disable DP audio stall fix for HBR2*/ > > Nitpick, spaces after /* and before */. > > > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > > + crtc_config->port_clock >= 540000) { > > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > > + tmp &= ~SPARE_13; > > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > > + } > > Hmm. Why don't we just do this unconditionally? > That bit is disabled by default, so avoiding two MMIO ops. that are not required. > > + > > /* Disable timestamps */ > > tmp = I915_READ(HSW_AUD_CFG(pipe)); > > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > > tmp |= AUD_CONFIG_N_PROG_ENABLE; > > tmp &= ~AUD_CONFIG_UPPER_N_MASK; > > tmp &= ~AUD_CONFIG_LOWER_N_MASK; > > - if (intel_crtc_has_dp_encoder(intel_crtc->config)) > > + if (intel_crtc_has_dp_encoder(crtc_config)) > > tmp |= AUD_CONFIG_N_VALUE_INDEX; > > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > > > > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > > { > > struct drm_i915_private *dev_priv = to_i915(connector->dev); > > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc); > > + struct intel_crtc_state *crtc_config = intel_crtc->config; > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > > enum pipe pipe = intel_crtc->pipe; > > enum port port = intel_encoder->port; > > const uint8_t *eld = connector->eld; > > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > > > > mutex_lock(&dev_priv->av_mutex); > > > > + /* Enable DP audio stall fix for HBR2 > > + * > > + * TODO: This workaround is applicable only for audio sample rates up > > + * to 96kHz. For frequencies above 96kHz, this is insufficient and > > + * cdclk should be increased to at least 432 MHz, just like BDW. Since, > > + * the audio driver does not support sample rates > 48 kHz, we are safe > > + * with this fix for now. > > + */ > > Is this TODO required if you already have that check in patch 2/2? > In fact, this patch itself is not required if we are increasing the cdclk to 432 MHz for gen9 platforms (like patch 2/2). Having this workaround gives us the option running the pipeline at a lower cdclk frequency i.e., 337.5 MHz, which I believe should be better in terms of power. > > + > > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > > + crtc_config->port_clock >= 540000) { > > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > > + tmp |= SPARE_13; > > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > > + } > > + > > /* Enable audio presence detect, invalidate ELD */ > > tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > > tmp |= AUDIO_OUTPUT_ENABLE(pipe); >
On Wed, 2016-10-26 at 12:11 +0300, Ville Syrjälä wrote: > On Tue, Oct 25, 2016 at 07:37:36PM -0700, Dhinakaran Pandiyan wrote: > > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So, > > let's set this bit right before enabling the audio codec. Playing audio > > without setting this bit results in pipe FIFO underruns. > > > > This workaround is applicable only for audio sample rates up to 96kHz. For > > frequencies above 96kHz, this is insufficient and cdclk should be increased > > to at least 432 MHz, just like BDW. Since, the audio driver does not > > support sample rates > 48 kHz, we are safe with this fix for now. > > > > v2: Inlined the code change within hsw_audio_codec_enable() (Jani) > > Fixed the port clock typo > > Added TODO comment > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 5 +++++ > > drivers/gpu/drm/i915/intel_audio.c | 30 +++++++++++++++++++++++++++++- > > 2 files changed, 34 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 00efaa1..76dac48 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -6236,6 +6236,11 @@ enum { > > #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) > > #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) > > > > +#define _CHICKEN_TRANS_A 0x420C0 > > +#define _CHICKEN_TRANS_B 0x420C4 > > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, _CHICKEN_TRANS_B) > > +#define SPARE_13 (1<<13) > > + > > /* WaCatErrorRejectionIssue */ > > #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030) > > #define GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11) > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > > index 7093cfb..894f11e 100644 > > --- a/drivers/gpu/drm/i915/intel_audio.c > > +++ b/drivers/gpu/drm/i915/intel_audio.c > > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > > { > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > > + struct intel_crtc_state *crtc_config = intel_crtc->config; > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > > enum pipe pipe = intel_crtc->pipe; > > uint32_t tmp; > > > > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > > > > mutex_lock(&dev_priv->av_mutex); > > > > + /*Disable DP audio stall fix for HBR2*/ > > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > > + crtc_config->port_clock >= 540000) { > > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > > + tmp &= ~SPARE_13; > > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > > + } > > + > > /* Disable timestamps */ > > tmp = I915_READ(HSW_AUD_CFG(pipe)); > > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > > tmp |= AUD_CONFIG_N_PROG_ENABLE; > > tmp &= ~AUD_CONFIG_UPPER_N_MASK; > > tmp &= ~AUD_CONFIG_LOWER_N_MASK; > > - if (intel_crtc_has_dp_encoder(intel_crtc->config)) > > + if (intel_crtc_has_dp_encoder(crtc_config)) > > tmp |= AUD_CONFIG_N_VALUE_INDEX; > > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > > > > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > > { > > struct drm_i915_private *dev_priv = to_i915(connector->dev); > > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc); > > + struct intel_crtc_state *crtc_config = intel_crtc->config; > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > > enum pipe pipe = intel_crtc->pipe; > > enum port port = intel_encoder->port; > > const uint8_t *eld = connector->eld; > > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > > > > mutex_lock(&dev_priv->av_mutex); > > > > + /* Enable DP audio stall fix for HBR2 > > + * > > + * TODO: This workaround is applicable only for audio sample rates up > > + * to 96kHz. For frequencies above 96kHz, this is insufficient and > > + * cdclk should be increased to at least 432 MHz, just like BDW. Since, > > + * the audio driver does not support sample rates > 48 kHz, we are safe > > + * with this fix for now. > > Where in the sound driver is this supposed 96kHz limit? I see a lot of > stuff for >96kHz in the code at least. > Libin/Jeeja can you help me here? > > + */ > > + > > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > > + crtc_config->port_clock >= 540000) { > > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > > + tmp |= SPARE_13; > > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > > + } > > + > > /* Enable audio presence detect, invalidate ELD */ > > tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > > tmp |= AUDIO_OUTPUT_ENABLE(pipe); > > -- > > 2.7.4 >
On Wed, 26 Oct 2016, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> wrote: > On Wed, 2016-10-26 at 11:57 +0300, Jani Nikula wrote: >> On Wed, 26 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: >> > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So, >> > let's set this bit right before enabling the audio codec. Playing audio >> > without setting this bit results in pipe FIFO underruns. >> > >> > This workaround is applicable only for audio sample rates up to 96kHz. For >> > frequencies above 96kHz, this is insufficient and cdclk should be increased >> > to at least 432 MHz, just like BDW. Since, the audio driver does not >> > support sample rates > 48 kHz, we are safe with this fix for now. >> > >> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani) >> > Fixed the port clock typo >> > Added TODO comment >> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_reg.h | 5 +++++ >> > drivers/gpu/drm/i915/intel_audio.c | 30 +++++++++++++++++++++++++++++- >> > 2 files changed, 34 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> > index 00efaa1..76dac48 100644 >> > --- a/drivers/gpu/drm/i915/i915_reg.h >> > +++ b/drivers/gpu/drm/i915/i915_reg.h >> > @@ -6236,6 +6236,11 @@ enum { >> > #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) >> > #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) >> > >> > +#define _CHICKEN_TRANS_A 0x420C0 >> > +#define _CHICKEN_TRANS_B 0x420C4 >> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, _CHICKEN_TRANS_B) >> > +#define SPARE_13 (1<<13) >> > + >> > /* WaCatErrorRejectionIssue */ >> > #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030) >> > #define GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11) >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c >> > index 7093cfb..894f11e 100644 >> > --- a/drivers/gpu/drm/i915/intel_audio.c >> > +++ b/drivers/gpu/drm/i915/intel_audio.c >> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) >> > { >> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >> > + struct intel_crtc_state *crtc_config = intel_crtc->config; >> > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; >> > enum pipe pipe = intel_crtc->pipe; >> > uint32_t tmp; >> > >> > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) >> > >> > mutex_lock(&dev_priv->av_mutex); >> > >> > + /*Disable DP audio stall fix for HBR2*/ >> >> Nitpick, spaces after /* and before */. >> >> > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && >> > + crtc_config->port_clock >= 540000) { >> > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); >> > + tmp &= ~SPARE_13; >> > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); >> > + } >> >> Hmm. Why don't we just do this unconditionally? >> > > That bit is disabled by default, so avoiding two MMIO ops. that are not > required. That makes no difference on the modeset path. > >> > + >> > /* Disable timestamps */ >> > tmp = I915_READ(HSW_AUD_CFG(pipe)); >> > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; >> > tmp |= AUD_CONFIG_N_PROG_ENABLE; >> > tmp &= ~AUD_CONFIG_UPPER_N_MASK; >> > tmp &= ~AUD_CONFIG_LOWER_N_MASK; >> > - if (intel_crtc_has_dp_encoder(intel_crtc->config)) >> > + if (intel_crtc_has_dp_encoder(crtc_config)) >> > tmp |= AUD_CONFIG_N_VALUE_INDEX; >> > I915_WRITE(HSW_AUD_CFG(pipe), tmp); >> > >> > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, >> > { >> > struct drm_i915_private *dev_priv = to_i915(connector->dev); >> > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc); >> > + struct intel_crtc_state *crtc_config = intel_crtc->config; >> > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; >> > enum pipe pipe = intel_crtc->pipe; >> > enum port port = intel_encoder->port; >> > const uint8_t *eld = connector->eld; >> > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, >> > >> > mutex_lock(&dev_priv->av_mutex); >> > >> > + /* Enable DP audio stall fix for HBR2 >> > + * >> > + * TODO: This workaround is applicable only for audio sample rates up >> > + * to 96kHz. For frequencies above 96kHz, this is insufficient and >> > + * cdclk should be increased to at least 432 MHz, just like BDW. Since, >> > + * the audio driver does not support sample rates > 48 kHz, we are safe >> > + * with this fix for now. >> > + */ >> >> Is this TODO required if you already have that check in patch 2/2? >> > > In fact, this patch itself is not required if we are increasing the > cdclk to 432 MHz for gen9 platforms (like patch 2/2). Having this > workaround gives us the option running the pipeline at a lower cdclk > frequency i.e., 337.5 MHz, which I believe should be better in terms of > power. The conditions for requiring higher cdclk are different in patch 2/2, right? That one also requires 4 lanes. So with 1 or 2 lanes this is still needed. BR, Jani. > > >> > + >> > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && >> > + crtc_config->port_clock >= 540000) { >> > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); >> > + tmp |= SPARE_13; >> > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); >> > + } >> > + >> > /* Enable audio presence detect, invalidate ELD */ >> > tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); >> > tmp |= AUDIO_OUTPUT_ENABLE(pipe); >> >
On Wed, 2016-10-26 at 22:06 +0300, Jani Nikula wrote: > On Wed, 26 Oct 2016, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> wrote: > > On Wed, 2016-10-26 at 11:57 +0300, Jani Nikula wrote: > >> On Wed, 26 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > >> > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So, > >> > let's set this bit right before enabling the audio codec. Playing audio > >> > without setting this bit results in pipe FIFO underruns. > >> > > >> > This workaround is applicable only for audio sample rates up to 96kHz. For > >> > frequencies above 96kHz, this is insufficient and cdclk should be increased > >> > to at least 432 MHz, just like BDW. Since, the audio driver does not > >> > support sample rates > 48 kHz, we are safe with this fix for now. > >> > > >> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani) > >> > Fixed the port clock typo > >> > Added TODO comment > >> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/i915_reg.h | 5 +++++ > >> > drivers/gpu/drm/i915/intel_audio.c | 30 +++++++++++++++++++++++++++++- > >> > 2 files changed, 34 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> > index 00efaa1..76dac48 100644 > >> > --- a/drivers/gpu/drm/i915/i915_reg.h > >> > +++ b/drivers/gpu/drm/i915/i915_reg.h > >> > @@ -6236,6 +6236,11 @@ enum { > >> > #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) > >> > #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) > >> > > >> > +#define _CHICKEN_TRANS_A 0x420C0 > >> > +#define _CHICKEN_TRANS_B 0x420C4 > >> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, _CHICKEN_TRANS_B) > >> > +#define SPARE_13 (1<<13) > >> > + > >> > /* WaCatErrorRejectionIssue */ > >> > #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030) > >> > #define GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11) > >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > >> > index 7093cfb..894f11e 100644 > >> > --- a/drivers/gpu/drm/i915/intel_audio.c > >> > +++ b/drivers/gpu/drm/i915/intel_audio.c > >> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > >> > { > >> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > >> > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > >> > + struct intel_crtc_state *crtc_config = intel_crtc->config; > >> > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > >> > enum pipe pipe = intel_crtc->pipe; > >> > uint32_t tmp; > >> > > >> > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > >> > > >> > mutex_lock(&dev_priv->av_mutex); > >> > > >> > + /*Disable DP audio stall fix for HBR2*/ > >> > >> Nitpick, spaces after /* and before */. > >> > >> > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > >> > + crtc_config->port_clock >= 540000) { > >> > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > >> > + tmp &= ~SPARE_13; > >> > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > >> > + } > >> > >> Hmm. Why don't we just do this unconditionally? > >> > > > > That bit is disabled by default, so avoiding two MMIO ops. that are not > > required. > > That makes no difference on the modeset path. > Because we do a lot of MMIO ops. anyway? > > > >> > + > >> > /* Disable timestamps */ > >> > tmp = I915_READ(HSW_AUD_CFG(pipe)); > >> > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > >> > tmp |= AUD_CONFIG_N_PROG_ENABLE; > >> > tmp &= ~AUD_CONFIG_UPPER_N_MASK; > >> > tmp &= ~AUD_CONFIG_LOWER_N_MASK; > >> > - if (intel_crtc_has_dp_encoder(intel_crtc->config)) > >> > + if (intel_crtc_has_dp_encoder(crtc_config)) > >> > tmp |= AUD_CONFIG_N_VALUE_INDEX; > >> > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > >> > > >> > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > >> > { > >> > struct drm_i915_private *dev_priv = to_i915(connector->dev); > >> > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc); > >> > + struct intel_crtc_state *crtc_config = intel_crtc->config; > >> > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > >> > enum pipe pipe = intel_crtc->pipe; > >> > enum port port = intel_encoder->port; > >> > const uint8_t *eld = connector->eld; > >> > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > >> > > >> > mutex_lock(&dev_priv->av_mutex); > >> > > >> > + /* Enable DP audio stall fix for HBR2 > >> > + * > >> > + * TODO: This workaround is applicable only for audio sample rates up > >> > + * to 96kHz. For frequencies above 96kHz, this is insufficient and > >> > + * cdclk should be increased to at least 432 MHz, just like BDW. Since, > >> > + * the audio driver does not support sample rates > 48 kHz, we are safe > >> > + * with this fix for now. > >> > + */ > >> > >> Is this TODO required if you already have that check in patch 2/2? > >> > > > > In fact, this patch itself is not required if we are increasing the > > cdclk to 432 MHz for gen9 platforms (like patch 2/2). Having this > > workaround gives us the option running the pipeline at a lower cdclk > > frequency i.e., 337.5 MHz, which I believe should be better in terms of > > power. > > The conditions for requiring higher cdclk are different in patch 2/2, > right? That one also requires 4 lanes. So with 1 or 2 lanes this is > still needed. > > BR, > Jani. > > I have not tested the 1 or 2 lane, HBR2 configuration myself. And BSpec doesn't exactly clarify that this is *needed* for 1 or 2 lanes HBR2, but I see no harm in having this workaround just based on HBR2. > > > > > >> > + > >> > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > >> > + crtc_config->port_clock >= 540000) { > >> > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > >> > + tmp |= SPARE_13; > >> > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > >> > + } > >> > + > >> > /* Enable audio presence detect, invalidate ELD */ > >> > tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > >> > tmp |= AUDIO_OUTPUT_ENABLE(pipe); > >> > > >
On Wed, 2016-10-26 at 18:14 +0000, Pandiyan, Dhinakaran wrote: > On Wed, 2016-10-26 at 12:11 +0300, Ville Syrjälä wrote: > > On Tue, Oct 25, 2016 at 07:37:36PM -0700, Dhinakaran Pandiyan wrote: > > > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So, > > > let's set this bit right before enabling the audio codec. Playing audio > > > without setting this bit results in pipe FIFO underruns. > > > > > > This workaround is applicable only for audio sample rates up to 96kHz. For > > > frequencies above 96kHz, this is insufficient and cdclk should be increased > > > to at least 432 MHz, just like BDW. Since, the audio driver does not > > > support sample rates > 48 kHz, we are safe with this fix for now. > > > > > > v2: Inlined the code change within hsw_audio_codec_enable() (Jani) > > > Fixed the port clock typo > > > Added TODO comment > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 5 +++++ > > > drivers/gpu/drm/i915/intel_audio.c | 30 +++++++++++++++++++++++++++++- > > > 2 files changed, 34 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index 00efaa1..76dac48 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -6236,6 +6236,11 @@ enum { > > > #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) > > > #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) > > > > > > +#define _CHICKEN_TRANS_A 0x420C0 > > > +#define _CHICKEN_TRANS_B 0x420C4 > > > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, _CHICKEN_TRANS_B) > > > +#define SPARE_13 (1<<13) > > > + > > > /* WaCatErrorRejectionIssue */ > > > #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030) > > > #define GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11) > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > > > index 7093cfb..894f11e 100644 > > > --- a/drivers/gpu/drm/i915/intel_audio.c > > > +++ b/drivers/gpu/drm/i915/intel_audio.c > > > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > > > { > > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > > > + struct intel_crtc_state *crtc_config = intel_crtc->config; > > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > > > enum pipe pipe = intel_crtc->pipe; > > > uint32_t tmp; > > > > > > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) > > > > > > mutex_lock(&dev_priv->av_mutex); > > > > > > + /*Disable DP audio stall fix for HBR2*/ > > > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > > > + crtc_config->port_clock >= 540000) { > > > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > > > + tmp &= ~SPARE_13; > > > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > > > + } > > > + > > > /* Disable timestamps */ > > > tmp = I915_READ(HSW_AUD_CFG(pipe)); > > > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > > > tmp |= AUD_CONFIG_N_PROG_ENABLE; > > > tmp &= ~AUD_CONFIG_UPPER_N_MASK; > > > tmp &= ~AUD_CONFIG_LOWER_N_MASK; > > > - if (intel_crtc_has_dp_encoder(intel_crtc->config)) > > > + if (intel_crtc_has_dp_encoder(crtc_config)) > > > tmp |= AUD_CONFIG_N_VALUE_INDEX; > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > > > > > > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > > > { > > > struct drm_i915_private *dev_priv = to_i915(connector->dev); > > > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc); > > > + struct intel_crtc_state *crtc_config = intel_crtc->config; > > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > > > enum pipe pipe = intel_crtc->pipe; > > > enum port port = intel_encoder->port; > > > const uint8_t *eld = connector->eld; > > > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, > > > > > > mutex_lock(&dev_priv->av_mutex); > > > > > > + /* Enable DP audio stall fix for HBR2 > > > + * > > > + * TODO: This workaround is applicable only for audio sample rates up > > > + * to 96kHz. For frequencies above 96kHz, this is insufficient and > > > + * cdclk should be increased to at least 432 MHz, just like BDW. Since, > > > + * the audio driver does not support sample rates > 48 kHz, we are safe > > > + * with this fix for now. > > > > Where in the sound driver is this supposed 96kHz limit? I see a lot of > > stuff for >96kHz in the code at least. > > > > Libin/Jeeja can you help me here? > > I see another problem, we cannot change cdclk without a full modeset. So choosing the workarounds based on audio sampling rate is going to be tricky. I guess, it makes sense to use the BDW cdclk workaround to SKL too. -DK > > > + */ > > > + > > > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > > > + crtc_config->port_clock >= 540000) { > > > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > > > + tmp |= SPARE_13; > > > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > > > + } > > > + > > > /* Enable audio presence detect, invalidate ELD */ > > > tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > > > tmp |= AUDIO_OUTPUT_ENABLE(pipe); > > > -- > > > 2.7.4 > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > Pandiyan, Dhinakaran > Sent: Thursday, October 27, 2016 2:14 AM > To: ville.syrjala@linux.intel.com > Cc: Nikula, Jani <jani.nikula@intel.com>; Kp, Jeeja <jeeja.kp@intel.com>; > intel-gfx@lists.freedesktop.org; libin.yang@linux.intel.com > Subject: Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix > for gen9 platforms > > On Wed, 2016-10-26 at 12:11 +0300, Ville Syrjälä wrote: > > On Tue, Oct 25, 2016 at 07:37:36PM -0700, Dhinakaran Pandiyan wrote: > > > Enabling DP audio stall fix is necessary to play audio over DP HBR2. > > > So, let's set this bit right before enabling the audio codec. > > > Playing audio without setting this bit results in pipe FIFO underruns. > > > > > > This workaround is applicable only for audio sample rates up to > > > 96kHz. For frequencies above 96kHz, this is insufficient and cdclk > > > should be increased to at least 432 MHz, just like BDW. Since, the > > > audio driver does not support sample rates > 48 kHz, we are safe with > this fix for now. > > > > > > v2: Inlined the code change within hsw_audio_codec_enable() (Jani) > > > Fixed the port clock typo > > > Added TODO comment > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 5 +++++ > > > drivers/gpu/drm/i915/intel_audio.c | 30 > > > +++++++++++++++++++++++++++++- > > > 2 files changed, 34 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > > b/drivers/gpu/drm/i915/i915_reg.h index 00efaa1..76dac48 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -6236,6 +6236,11 @@ enum { > > > #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) > > > #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) > > > > > > +#define _CHICKEN_TRANS_A 0x420C0 > > > +#define _CHICKEN_TRANS_B 0x420C4 > > > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, > _CHICKEN_TRANS_A, _CHICKEN_TRANS_B) > > > +#define SPARE_13 (1<<13) > > > + > > > /* WaCatErrorRejectionIssue */ > > > #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG > _MMIO(0x9030) > > > #define GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11) > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c > > > b/drivers/gpu/drm/i915/intel_audio.c > > > index 7093cfb..894f11e 100644 > > > --- a/drivers/gpu/drm/i915/intel_audio.c > > > +++ b/drivers/gpu/drm/i915/intel_audio.c > > > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct > > > intel_encoder *encoder) { > > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > > > + struct intel_crtc_state *crtc_config = intel_crtc->config; > > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > > > enum pipe pipe = intel_crtc->pipe; > > > uint32_t tmp; > > > > > > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct > > > intel_encoder *encoder) > > > > > > mutex_lock(&dev_priv->av_mutex); > > > > > > + /*Disable DP audio stall fix for HBR2*/ > > > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > > > + crtc_config->port_clock >= 540000) { > > > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > > > + tmp &= ~SPARE_13; > > > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > > > + } > > > + > > > /* Disable timestamps */ > > > tmp = I915_READ(HSW_AUD_CFG(pipe)); > > > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > > > tmp |= AUD_CONFIG_N_PROG_ENABLE; > > > tmp &= ~AUD_CONFIG_UPPER_N_MASK; > > > tmp &= ~AUD_CONFIG_LOWER_N_MASK; > > > - if (intel_crtc_has_dp_encoder(intel_crtc->config)) > > > + if (intel_crtc_has_dp_encoder(crtc_config)) > > > tmp |= AUD_CONFIG_N_VALUE_INDEX; > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > > > > > > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct > > > drm_connector *connector, { > > > struct drm_i915_private *dev_priv = to_i915(connector->dev); > > > struct intel_crtc *intel_crtc = > > > to_intel_crtc(intel_encoder->base.crtc); > > > + struct intel_crtc_state *crtc_config = intel_crtc->config; > > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; > > > enum pipe pipe = intel_crtc->pipe; > > > enum port port = intel_encoder->port; > > > const uint8_t *eld = connector->eld; @@ -326,6 +338,22 @@ static > > > void hsw_audio_codec_enable(struct drm_connector *connector, > > > > > > mutex_lock(&dev_priv->av_mutex); > > > > > > + /* Enable DP audio stall fix for HBR2 > > > + * > > > + * TODO: This workaround is applicable only for audio sample rates > up > > > + * to 96kHz. For frequencies above 96kHz, this is insufficient and > > > + * cdclk should be increased to at least 432 MHz, just like BDW. Since, > > > + * the audio driver does not support sample rates > 48 kHz, we are > safe > > > + * with this fix for now. > > > > Where in the sound driver is this supposed 96kHz limit? I see a lot of > > stuff for >96kHz in the code at least. > > > > Libin/Jeeja can you help me here? From audio driver side, 96KHz should be similar with 48KHz. I have test 88KHz, it works. When I test 96KHz, it will be converted to 48KHz. Our audio QA didn't test 88KHz and 96KHz and it is not in our support list. How do you get the 'stuff for >96kHz in the code' message? It seems I can't reproduce this message. Regards, Libin > > > > > + */ > > > + > > > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && > > > + crtc_config->port_clock >= 540000) { > > > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); > > > + tmp |= SPARE_13; > > > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); > > > + } > > > + > > > /* Enable audio presence detect, invalidate ELD */ > > > tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > > > tmp |= AUDIO_OUTPUT_ENABLE(pipe); > > > -- > > > 2.7.4 > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 00efaa1..76dac48 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6236,6 +6236,11 @@ enum { #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) +#define _CHICKEN_TRANS_A 0x420C0 +#define _CHICKEN_TRANS_B 0x420C4 +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, _CHICKEN_TRANS_B) +#define SPARE_13 (1<<13) + /* WaCatErrorRejectionIssue */ #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030) #define GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11) diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 7093cfb..413dd50 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) { struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); + struct intel_crtc_state *crtc_config = intel_crtc->config; + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; enum pipe pipe = intel_crtc->pipe; uint32_t tmp; @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) mutex_lock(&dev_priv->av_mutex); + /*Disable DP audio stall fix for HBR2*/ + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && + crtc_config->port_clock >= 540000) { + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); + tmp &= ~SPARE_13; + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); + } + /* Disable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX; tmp |= AUD_CONFIG_N_PROG_ENABLE; tmp &= ~AUD_CONFIG_UPPER_N_MASK; tmp &= ~AUD_CONFIG_LOWER_N_MASK; - if (intel_crtc_has_dp_encoder(intel_crtc->config)) + if (intel_crtc_has_dp_encoder(crtc_config)) tmp |= AUD_CONFIG_N_VALUE_INDEX; I915_WRITE(HSW_AUD_CFG(pipe), tmp); @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, { struct drm_i915_private *dev_priv = to_i915(connector->dev); struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc); + struct intel_crtc_state *crtc_config = intel_crtc->config; + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder; enum pipe pipe = intel_crtc->pipe; enum port port = intel_encoder->port; const uint8_t *eld = connector->eld; @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, mutex_lock(&dev_priv->av_mutex); + /* Enable DP audio stall fix for HBR2 + * + * TODO: This workaround is applicable only for audio sample rates up + * to 96kHz. For frequencies above 96kHz, this is insufficient and + * cdclk should be increased to at least 432 MHz, just like BDW. Since, + * the audio driver does not support sample rates < 48 kHz, we are safe + * with this fix for now. + */ + + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) && + crtc_config->port_clock >= 540000) { + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder)); + tmp |= SPARE_13; + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp); + } + /* Enable audio presence detect, invalidate ELD */ tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); tmp |= AUDIO_OUTPUT_ENABLE(pipe); @@ -642,25 +670,49 @@ static int i915_audio_component_get_cdclk_freq(struct device *kdev) return dev_priv->cdclk_freq; } +/* + * get the intel_encoder according to the parameter port and pipe + * intel_encoder is saved by the index of pipe + * MST & (pipe >= 0): return the av_enc_map[pipe], + * when port is matched + * MST & (pipe < 0): this is invalid + * Non-MST & (pipe >= 0): only pipe = 0 (the first device entry) + * will get the right intel_encoder with port matched + * Non-MST & (pipe < 0): get the right intel_encoder with port matched + */ static struct intel_encoder *get_saved_enc(struct drm_i915_private *dev_priv, int port, int pipe) { + struct intel_encoder *encoder; if (WARN_ON(pipe >= I915_MAX_PIPES)) return NULL; /* MST */ - if (pipe >= 0) - return dev_priv->av_enc_map[pipe]; + if (pipe >= 0) { + encoder = dev_priv->av_enc_map[pipe]; + /* + * when bootup, audio driver may not know it is + * MST or not. So it will poll all the port & pipe + * combinations + */ + if (encoder != NULL && encoder->port == port && + encoder->type == INTEL_OUTPUT_DP_MST) + return encoder; + } /* Non-MST */ - for_each_pipe(dev_priv, pipe) { - struct intel_encoder *encoder; + if (pipe > 0) + return NULL; + for_each_pipe(dev_priv, pipe) { encoder = dev_priv->av_enc_map[pipe]; if (encoder == NULL) continue; + if (encoder->type == INTEL_OUTPUT_DP_MST) + continue; + if (port == encoder->port) return encoder; }
Enabling DP audio stall fix is necessary to play audio over DP HBR2. So, let's set this bit right before enabling the audio codec. Playing audio without setting this bit results in pipe FIFO underruns. This workaround is applicable only for audio sample rates up to 96kHz. For frequencies above 96kHz, this is insufficient and cdclk should be increased to at least 432 MHz, just like BDW. Since, the audio driver does not support sample rates < 48 kHz, we are safe with this fix for now. v2: Inlined the code change within hsw_audio_codec_enable() (Jani) Fixed the port clock typo Added TODO comment Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 5 +++ drivers/gpu/drm/i915/intel_audio.c | 62 +++++++++++++++++++++++++++++++++++--- 2 files changed, 62 insertions(+), 5 deletions(-)