On Wed, 02 Sep 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 values, the N/CTS need to be set manually if audio is playing.
Do we still need this patch after David Henningsson's series [1]? IIUC you will now always get the callback on modesets, so you should be able to, uh, callback on the callback to set audio rate. Granted, with this I suppose you reduce the time there might be wrong N/CTS after enable.
Some other comments inline.
[1] http://mid.gmane.org/1440781350-12053-1-git-send-email-david.henningsson@can...
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ drivers/gpu/drm/i915/intel_audio.c | 40 +++++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ae7fa3e..5bdee36 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7058,6 +7058,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
Nitpick. The bit fields are not defined.
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ @@ -7072,6 +7074,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)
Nitpick. The bit fields are not defined. I think it's fine to use _PIPE macro, but you should probably use "converter" or "cvt" or something for the parameter name to not mislead people into thinking this is pipe based.
#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 a021720..acdb68c 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -140,6 +140,27 @@ static bool audio_rate_need_prog(struct intel_crtc *crtc, return false; }
+static int audio_config_get_rate(struct drm_i915_private *dev_priv,
enum pipe pipe)+{
- uint32_t tmp;
- int cvt_idx;
- int base_rate, mul, div, rate;
- tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
- cvt_idx = (tmp >> (pipe * 8)) & 0xff;
This one needs to be addressed. Are you sure it's indexed by pipe? The spec is vague.
Bits 7:0 are "control B", 15:8 are "control C", and so on, while your (tmp >> (pipe * 8)) maps pipe A to control B, pipe B to control C, etc. This would speak for port, not pipe, as pipe A should be valid while port A not.
- tmp = I915_READ(AUD_STR_DESC(cvt_idx));
- base_rate = tmp & (1 << 14);
Nitpick. We prefer (tmp & MASK) >> SHIFT.
- if (base_rate == 0)
rate = 48000;- else
rate = 44100;- mul = (tmp & (0x7 << 11)) + 1;
- div = (tmp & (0x7 << 8)) + 1;
This one needs to be addressed. This is bogus. The +1 would work if you actually did (tmp & MASK) >> SHIFT, but now you add it to the shifted value. Your multipliers and divisors are *way* off.
NAK on this patch.
- rate = rate * mul / div;
- return rate;
+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -265,6 +286,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int n_low, n_up, n;
int rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n", pipe_name(pipe), drm_eld_size(eld));
@@ -302,12 +325,27 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE; tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) tmp |= AUD_CONFIG_N_VALUE_INDEX; else tmp |= audio_config_hdmi_pixel_clock(mode);
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
- if (audio_rate_need_prog(intel_crtc, mode)) {
rate = audio_config_get_rate(dev_priv, pipe);n = audio_config_get_n(mode, rate);if (n != 0) {n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp &= ~(AUD_CONFIG_UPPER_N_MASK |AUD_CONFIG_LOWER_N_MASK);tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);
Nitpick. I'd prefer some sharing with the similar blocks from the earlier patch. Also a debug message on n == 0 would be nice; you probably didn't notice your audio_config_get_rate() wasn't working right because this silently fell back to the automatic mode here.
}}
I915_WRITE(HSW_AUD_CFG(pipe), tmp);
mutex_unlock(&dev_priv->av_mutex);
-- 1.9.1