[Intel-gfx] [PATCH v3] drm/i915: start adding dp mst audio
Yang, Libin
libin.yang at intel.com
Sun Nov 27 01:33:09 UTC 2016
Hi Jani,
Thanks for review. I will split this patch.
Regards,
Libin
> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> Sent: Friday, November 25, 2016 10:23 PM
> To: Yang, Libin <libin.yang at intel.com>; intel-gfx at lists.freedesktop.org;
> ville.syrjala at linux.intel.com; Vetter, Daniel <daniel.vetter at intel.com>;
> tiwai at suse.de
> Cc: Yang, Libin <libin.yang at intel.com>; Libin Yang
> <libin.yang at linux.intel.com>; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Subject: Re: [PATCH v3] drm/i915: start adding dp mst audio
>
> On Mon, 14 Nov 2016, libin.yang at intel.com wrote:
> > From: Libin Yang <libin.yang at linux.intel.com>
> >
> > (This patch is developed by Dave Airlie <airlied at redhat.com>
> > originally)
> >
> > This patch adds support for DP MST audio in i915.
> >
> > Enable audio codec when DP MST is enabled if has_audio flag is set.
> > Disable audio codec when DP MST is disabled if has_audio flag is set.
> >
> > Another separated patches to support DP MST audio will be implemented
> > in audio driver.
> >
> > v2:
> > Rebased.
> > v3:
> > refine to support updated intel_audio_codec_enable()
>
> I view this patch different from usual because this regressed and was
> reverted before. Please split this up to three patches:
>
> 1) Do intel_dp_mst_info() in i915_debugfs.c. The information printed
> looks at the sink, not pipe config, so this is not dependent on the
> rest.
>
> 2) Abstract intel_ddi_is_audio_enabled(). It's a nice clean
> non-functional change.
>
> 3) The rest. The last patch *must* reference the original commit and the
> revert:
>
> 3708d5e082c3 ("drm/i915: start adding dp mst audio")
> be754b101f70 ("Revert "drm/i915: start adding dp mst audio"")
>
> Please rename the patch title to something like, "enable dp mst
> audio" to distinguish.
>
> If the split had been done originally, we would only have reverted part 3, not
> all of it. Please do it now.
>
> More comments inline.
>
> >
> > Signed-off-by: Libin Yang <libin.yang at linux.intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Reviewed-by: Lyude <cpaul at redhat.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Link:
> > http://patchwork.freedesktop.org/patch/msgid/1474334681-22690-6-git-se
> > nd-email-dhinakaran.pandiyan at intel.com
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 19 ++++++++++++++++++-
> > drivers/gpu/drm/i915/intel_ddi.c | 20 +++++++++++++++-----
> > drivers/gpu/drm/i915/intel_dp_mst.c | 18 ++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 2 ++
> > 4 files changed, 53 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index bce3880..3442381 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2893,6 +2893,20 @@ static void intel_dp_info(struct seq_file *m,
> > &intel_dp->aux);
> > }
> >
> > +static void intel_dp_mst_info(struct seq_file *m,
> > + struct intel_connector *intel_connector) {
> > + struct intel_encoder *intel_encoder = intel_connector->encoder;
> > + struct intel_dp_mst_encoder *intel_mst =
> > + enc_to_mst(&intel_encoder->base);
> > + struct intel_digital_port *intel_dig_port = intel_mst->primary;
> > + struct intel_dp *intel_dp = &intel_dig_port->dp;
> > + bool has_audio = drm_dp_mst_port_has_audio(&intel_dp->mst_mgr,
> > + intel_connector->port);
> > +
> > + seq_printf(m, "\taudio support: %s\n", yesno(has_audio)); }
> > +
> > static void intel_hdmi_info(struct seq_file *m,
> > struct intel_connector *intel_connector) { @@ -
> 2935,7
> > +2949,10 @@ static void intel_connector_info(struct seq_file *m,
> > switch (connector->connector_type) {
> > case DRM_MODE_CONNECTOR_DisplayPort:
> > case DRM_MODE_CONNECTOR_eDP:
> > - intel_dp_info(m, intel_connector);
> > + if (intel_encoder->type == INTEL_OUTPUT_DP_MST)
> > + intel_dp_mst_info(m, intel_connector);
> > + else
> > + intel_dp_info(m, intel_connector);
>
> The above two hunks make up patch 1/3.
>
> > break;
> > case DRM_MODE_CONNECTOR_LVDS:
> > if (intel_encoder->type == INTEL_OUTPUT_LVDS) diff --git
> > a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 0ad4e16..b1e3442 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1951,6 +1951,19 @@ void intel_ddi_prepare_link_retrain(struct
> intel_dp *intel_dp)
> > udelay(600);
> > }
> >
> > +bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
> > + struct intel_crtc *intel_crtc)
> > +{
> > + u32 temp;
> > +
> > + if (intel_display_power_is_enabled(dev_priv,
> POWER_DOMAIN_AUDIO)) {
> > + temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > + if (temp & AUDIO_OUTPUT_ENABLE(intel_crtc->pipe))
> > + return true;
> > + }
> > + return false;
> > +}
> > +
> > void intel_ddi_get_config(struct intel_encoder *encoder,
> > struct intel_crtc_state *pipe_config) { @@ -2016,11
> +2029,8 @@
> > void intel_ddi_get_config(struct intel_encoder *encoder,
> > break;
> > }
> >
> > - if (intel_display_power_is_enabled(dev_priv,
> POWER_DOMAIN_AUDIO)) {
> > - temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > - if (temp & AUDIO_OUTPUT_ENABLE(intel_crtc->pipe))
> > - pipe_config->has_audio = true;
> > - }
> > + pipe_config->has_audio =
> > + intel_ddi_is_audio_enabled(dev_priv, intel_crtc);
>
> The above two hunks + declaration in intel_drv.h make up patch 2/3.
>
> >
> > if (encoder->type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp.bpp
> &&
> > pipe_config->pipe_bpp > dev_priv->vbt.edp.bpp) { diff --git
> > a/drivers/gpu/drm/i915/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 3ffbd69..96ff6a9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -37,6 +37,8 @@ static bool intel_dp_mst_compute_config(struct
> intel_encoder *encoder,
> > struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder-
> >base);
> > struct intel_digital_port *intel_dig_port = intel_mst->primary;
> > struct intel_dp *intel_dp = &intel_dig_port->dp;
> > + struct intel_connector *connector =
> > + to_intel_connector(conn_state->connector);
> > struct drm_atomic_state *state;
> > int bpp;
> > int lane_count, slots;
> > @@ -59,6 +61,8 @@ static bool intel_dp_mst_compute_config(struct
> > intel_encoder *encoder,
> >
> > state = pipe_config->base.state;
> >
> > + if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector-
> >port))
> > + pipe_config->has_audio = true;
> > mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
> bpp);
> >
> > pipe_config->pbn = mst_pbn;
> > @@ -84,6 +88,7 @@ static void intel_mst_disable_dp(struct intel_encoder
> *encoder,
> > struct intel_dp *intel_dp = &intel_dig_port->dp;
> > struct intel_connector *connector =
> > to_intel_connector(old_conn_state->connector);
> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > int ret;
> >
> > DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links); @@ -94,6
> +99,10
> > @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
> > if (ret) {
> > DRM_ERROR("failed to update payload %d\n", ret);
> > }
> > + if (old_crtc_state->has_audio) {
> > + intel_audio_codec_disable(encoder);
> > + intel_display_power_put(dev_priv,
> POWER_DOMAIN_AUDIO);
> > + }
> > }
> >
> > static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
> > @@ -206,6 +215,12 @@ static void intel_mst_enable_dp(struct
> intel_encoder *encoder,
> > ret = drm_dp_check_act_status(&intel_dp->mst_mgr);
> >
> > ret = drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> > + if (pipe_config->has_audio) {
> > + DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
> > + pipe_name(intel_mst->pipe));
>
> Not thrilled about the debug logging here. Is the information in
> intel_audio_codec_enable() not enough? Can that be amended instead?
>
> BR,
> Jani.
>
> > + intel_display_power_get(dev_priv,
> POWER_DOMAIN_AUDIO);
> > + intel_audio_codec_enable(encoder, pipe_config, conn_state);
> > + }
> > }
> >
> > static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder
> > *encoder, @@ -228,6 +243,9 @@ static void
> intel_dp_mst_enc_get_config(struct intel_encoder *encoder,
> > enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> > u32 temp, flags = 0;
> >
> > + pipe_config->has_audio =
> > + intel_ddi_is_audio_enabled(dev_priv, crtc);
> > +
> > temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > if (temp & TRANS_DDI_PHSYNC)
> > flags |= DRM_MODE_FLAG_PHSYNC;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 003afb8..8c9aa0a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1166,6 +1166,8 @@ bool intel_ddi_pll_select(struct intel_crtc
> > *crtc, void intel_ddi_set_pipe_settings(struct drm_crtc *crtc); void
> > intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp); bool
> > intel_ddi_connector_get_hw_state(struct intel_connector
> > *intel_connector);
> > +bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
> > + struct intel_crtc *intel_crtc);
> > void intel_ddi_get_config(struct intel_encoder *encoder,
> > struct intel_crtc_state *pipe_config); struct
> intel_encoder *
>
> --
> Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list