From: Jani Nikula <jani.nikula@linux.intel.com>
To: libin.yang@intel.com, intel-gfx@lists.freedesktop.org,
ville.syrjala@linux.intel.com, daniel.vetter@intel.com,
tiwai@suse.de
Cc: Libin Yang <libin.yang@linux.intel.com>,
Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH v3] drm/i915: start adding dp mst audio
Date: Fri, 25 Nov 2016 16:22:41 +0200 [thread overview]
Message-ID: <871sxzvdu6.fsf@intel.com> (raw)
In-Reply-To: <1479101531-89624-1-git-send-email-libin.yang@intel.com>
On Mon, 14 Nov 2016, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@linux.intel.com>
>
> (This patch is developed by Dave Airlie <airlied@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@linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Lyude <cpaul@redhat.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Link: http://patchwork.freedesktop.org/patch/msgid/1474334681-22690-6-git-send-email-dhinakaran.pandiyan@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-11-25 14:22 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-14 5:32 [PATCH v3] drm/i915: start adding dp mst audio libin.yang
2016-11-14 5:40 ` Yang, Libin
2016-11-17 5:10 ` Yang, Libin
2016-11-14 6:15 ` ✓ Fi.CI.BAT: success for " Patchwork
2016-11-25 14:22 ` Jani Nikula [this message]
2016-11-27 1:33 ` [PATCH v3] " Yang, Libin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=871sxzvdu6.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=daniel.vetter@intel.com \
--cc=dhinakaran.pandiyan@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=libin.yang@intel.com \
--cc=libin.yang@linux.intel.com \
--cc=rodrigo.vivi@intel.com \
--cc=tiwai@suse.de \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.