[Intel-gfx] [PATCH V2 3/4] drm/i915: Haswell HDMI audio enable

[Paulo Zanoni]

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