Hi
Comments inline.
2012/7/30 Wang Xingchao <xingchao.wang at intel.com>:
> Configure the related HDMI audio register to generate an unsolicited
> response to the audio controller driver to indicate that the controller
> sequence should start.
>
> Signed-off-by: Wang Xingchao <xingchao.wang at intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 17020cd..7ddc446 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1649,6 +1649,7 @@ static void intel_enable_transcoder(struct drm_i915_private *dev_priv,
> u32 val, pipeconf_val;
> struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>
> + DRM_DEBUG_DRIVER("Enable transcoder %c\n", pipe_name(pipe));
Do we really want this?
> /* PCH only available on ILK+ */
> BUG_ON(dev_priv->info->gen < 5);
>
> @@ -5071,6 +5072,7 @@ static void ironlake_write_eld(struct drm_connector *connector,
> struct drm_crtc *crtc)
> {
> struct drm_i915_private *dev_priv = connector->dev->dev_private;
> + struct drm_device *dev = crtc->dev;
> uint8_t *eld = connector->eld;
> uint32_t eldv;
> uint32_t i;
> @@ -5085,6 +5087,11 @@ static void ironlake_write_eld(struct drm_connector *connector,
> aud_config = IBX_AUD_CONFIG_A;
> aud_cntl_st = IBX_AUD_CNTL_ST_A;
> aud_cntrl_st2 = IBX_AUD_CNTL_ST2;
> + } else if (IS_HASWELL(dev)) {
> + hdmiw_hdmiedid = HSW_AUD_EDID_DATA;
> + aud_cntl_st = HSW_AUD_DIP_ELD_CTRL_ST_A;
> + aud_config = HSW_AUD_CONFIG_A;
> + aud_cntrl_st2 = HSW_AUD_PIN_ELD_CP_VLD;
> } else {
> hdmiw_hdmiedid = CPT_HDMIW_HDMIEDID_A;
> aud_config = CPT_AUD_CONFIG_A;
> @@ -5092,6 +5099,55 @@ static void ironlake_write_eld(struct drm_connector *connector,
> aud_cntrl_st2 = CPT_AUD_CNTRL_ST2;
> }
>
> + if (IS_HASWELL(dev)) {
> + int tmp;
> + int aud_cfg;
> + int aud_vld;
> + int eld_ctr_st;
> + int audio_misc;
> + int transf_a;
> + int pipe;
Since the initialization is very simple, can't we just define the
variables and initialize them at the same time?
Example:
int tmp;
int aud_cfg = HSW_AUD_CFG(pipe);
int aud_vld = HSW_AUD_PIN_ELD_CP_VLD;
int eld_ctr_st = HSW_AUD_DIP_ELD_CTRL(pipe);
int auido_misc = HSW_AUD_MISC_CTRL(pipe);
int transf_a = TRANS_CONF_A
int pipe = to_intel_crtc(crtc)->pipe;
Also, after a quick look, these variables seem unused and maybe should
be removed: eld_ctr_st, audio_misc, transf_a.
Also, we have aud_cfg (defined by you) and aud_config (defined earlier
on the same function) which seem to have the same purpose. You should
probably use the already-defined aud_config, but then you'll have to
move your "if (IS_HASWELL())" statement a few lines down...
You should also move the "int pipe" definition to the beginning of the
function and use it instead of reusing the "i" variable for
everything.
> +
> + pipe = to_intel_crtc(crtc)->pipe;
> +
> + DRM_DEBUG_DRIVER("HDMI: Haswell Audio initialize....\n");
> +
> + /* Need first enable transcoder and port */
> + aud_cfg = HSW_AUD_CFG(pipe);
> + eld_ctr_st = HSW_AUD_DIP_ELD_CTRL(pipe);
> + aud_vld = HSW_AUD_PIN_ELD_CP_VLD;
> + audio_misc = HSW_AUD_MISC_CTRL(pipe);
> +
> + transf_a = TRANS_CONF_A;
> +
> + /* Audio output enable */
> + DRM_DEBUG_DRIVER("HDMI audio: enable codec\n");
> + tmp = I915_READ(aud_vld);
> + tmp |= (AUDIO_OUTPUT_ENABLE_AB | AUDIO_OUTPUT_ENABLE_BC | AUDIO_OUTPUT_ENABLE_CD);
> + I915_WRITE(aud_vld, tmp);
> +
> + /* Set ELD valid state */
> + tmp = I915_READ(aud_vld);
> + DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%8x\n", tmp);
> + tmp |= (AUDIO_ELD_VALID_AB | AUDIO_ELD_VALID_BC | AUDIO_ELD_VALID_CD);
> + I915_WRITE(aud_vld, tmp);
> + tmp = I915_READ(aud_vld);
> + DRM_DEBUG_DRIVER("HDMI audio: eld vld status=0x%8x\n", tmp);
> +
> + /* Enable HDMI mode */
> + tmp = I915_READ(aud_cfg);
> + DRM_DEBUG_DRIVER("HDMI audio: audio conf: 0x%8x\n", tmp);
> + /* clear N_programing_enable and N_value_index */
> + tmp &= ~(0x3<<29);
If you use #define instead of the magic "0x3 << 29" (just like we do
in the rest of our code), you will probably be able to remove these
comments.
> + I915_WRITE(aud_cfg, tmp);
> +
> + /*TODO:
> + * 1.enable sample fabrication
> + * 2.set Upper_N_value(27:20) and Lower_N_value(15:4)
> + * 3.enable timestamps
> + * */
> + }
> +
> i = to_intel_crtc(crtc)->pipe;
> hdmiw_hdmiedid += i * 0x100;
> aud_cntl_st += i * 0x100;
I know this is not part of your commit, but can't we just use the PIPE
macro instead of this "var += i * 0x100" ?
> @@ -5135,6 +5191,8 @@ static void ironlake_write_eld(struct drm_connector *connector,
> i = I915_READ(aud_cntl_st);
> i &= ~IBX_ELD_ADDRESS;
> I915_WRITE(aud_cntl_st, i);
> + i = (i >> 29) & 0x3; /* DIP_Port_Select, 0x1 = PortB */
> + DRM_DEBUG_DRIVER("port num:%d\n", i);
>
> len = min_t(uint8_t, eld[2], 21); /* 84 bytes of hw ELD buffer */
> DRM_DEBUG_DRIVER("ELD size %d\n", len);
ironlake_write_eld looks scary...
Thanks,
Paulo
--
Paulo Zanoni
