Re: [PATCH V2 3/4] drm/i915: Haswell HDMI audio enable

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 01, 2012 at 11:12:28PM -0300, Paulo Zanoni wrote:
> Hi
> 
> Comments inline.
> 
> 2012/7/30 Wang Xingchao <xingchao.wang@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@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?

... and in kms code we use DRM_DEBUG_KMS.

> >         /* 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" ?

Yeah, and there are a few other places in the eld code that would welcome
some similar love. Renaming the temporary var i to tmp or reg would also
look a bit better. But such cleanups would need to happen in a separate
patch.

> > @@ -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...

Imo the hsw code is sufficiently different to varant a haswell_write_eld.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48