On Wed, 02 Sep 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, September 02, 2015 4:20 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com Cc: Yang, Libin Subject: Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset
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.
Yes, we need. David's patch will not trigger the sync_audio_rate callback.
Okay.
Some other comments inline.
[1] http://mid.gmane.org/1440781350-12053-1-git-send-email- david.henningsson@canonical.com
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.
OK, I will add the bit definition.
#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.
Do you mean it should be like: _PIPE(cvt, _HSW_AUD_STR_DESC_1, ...) ?
Yes.
#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.
Yes, I did lot of test to confirm it.
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.
We found there is something wrong/or vague in the spec.
Indeed.
- tmp = I915_READ(AUD_STR_DESC(cvt_idx));
- base_rate = tmp & (1 << 14);
Nitpick. We prefer (tmp & MASK) >> SHIFT.
OK, I will change it.
- 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.
OK, I will check it and change the patch.
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.
OK, I will add the msg. As you and Ville are insisting on sharing code, I will do it in next version.
Well, really, I'm fine with having that part duplicated as-is for now, we can fix it later. More important to focus on getting audio_config_get_rate() right.
I don't know if you're still targeting v4.3 with this (up to Takashi I guess) we'll really need to wrap this up soon.
BR, Jani.
Regards, Libin
}}
I915_WRITE(HSW_AUD_CFG(pipe), tmp);
mutex_unlock(&dev_priv->av_mutex);
-- 1.9.1
-- Jani Nikula, Intel Open Source Technology Center