On Wed, Aug 01, 2012 at 11:12:28PM -0300, Paulo Zanoni wrote:
> 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?
... 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 at ffwll.ch
Mobile: +41 (0)79 365 57 48
