| Message ID | 1343702825-15439-4-git-send-email-xingchao.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > /* 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
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
Hey Daniel, I had v3 patches under review and will send them out later. > > > 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. I'd removed this debug message in v3. > > > > /* PCH only available on I > > > > 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. They are replaced with macro in v3 too. > > > > @@ -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. Hmm, I think I need write a separate patch to place the haswell_write_eld. Thanks --xingchao
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)); /* 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; + + 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); + 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; @@ -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);
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(+)