diff mbox

[v2,1/3] drm/i915: Add function to return port from an encoder

Message ID 1471305655-4695-2-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dhinakaran Pandiyan Aug. 16, 2016, midnight UTC
There are places in the driver where we just need the 'port' associated
with an encoder and not 'struct intel_digital_port' that contains it.
This basically is a generic implementation of intel_ddi_get_encoder_port()
handling both DDI and Non-DDI encoders. The handling of the analog encoder
is delegated to the DDI specific function.

The idea to have a generic implementation that returned the 'enum port'
from 'struct intel_encoder' came from Ville.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c      | 42 ++++++++++-------------------------
 drivers/gpu/drm/i915/intel_display.c  | 18 +++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h      |  8 ++++++-
 drivers/gpu/drm/i915/intel_opregion.c |  2 +-
 4 files changed, 38 insertions(+), 32 deletions(-)

Comments

Rodrigo Vivi Aug. 19, 2016, 4:39 a.m. UTC | #1
On Mon, Aug 15, 2016 at 05:00:53PM -0700, Dhinakaran Pandiyan wrote:
> There are places in the driver where we just need the 'port' associated
> with an encoder and not 'struct intel_digital_port' that contains it.
> This basically is a generic implementation of intel_ddi_get_encoder_port()
> handling both DDI and Non-DDI encoders. The handling of the analog encoder
> is delegated to the DDI specific function.
> 
> The idea to have a generic implementation that returned the 'enum port'
> from 'struct intel_encoder' came from Ville.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c      | 42 ++++++++++-------------------------
>  drivers/gpu/drm/i915/intel_display.c  | 18 +++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h      |  8 ++++++-
>  drivers/gpu/drm/i915/intel_opregion.c |  2 +-
>  4 files changed, 38 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index c2df4e4..13ceef4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -301,24 +301,6 @@ static const struct bxt_ddi_buf_trans bxt_ddi_translations_hdmi[] = {
>  	{ 154, 0x9A, 1, 128, true },	/* 9:	1200		0   */
>  };
>  
> -enum port intel_ddi_get_encoder_port(struct intel_encoder *encoder)
> -{
> -	switch (encoder->type) {
> -	case INTEL_OUTPUT_DP_MST:
> -		return enc_to_mst(&encoder->base)->primary->port;
> -	case INTEL_OUTPUT_DP:
> -	case INTEL_OUTPUT_EDP:
> -	case INTEL_OUTPUT_HDMI:
> -	case INTEL_OUTPUT_UNKNOWN:
> -		return enc_to_dig_port(&encoder->base)->port;
> -	case INTEL_OUTPUT_ANALOG:
> -		return PORT_E;
> -	default:
> -		MISSING_CASE(encoder->type);
> -		return PORT_A;
> -	}
> -}
> -
>  static const struct ddi_buf_trans *
>  bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
>  {
> @@ -421,7 +403,7 @@ void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	u32 iboost_bit = 0;
>  	int i, n_dp_entries, n_edp_entries, size;
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port = intel_get_encoder_port(encoder);
>  	const struct ddi_buf_trans *ddi_translations_fdi;
>  	const struct ddi_buf_trans *ddi_translations_dp;
>  	const struct ddi_buf_trans *ddi_translations_edp;
> @@ -499,7 +481,7 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	u32 iboost_bit = 0;
>  	int n_hdmi_entries, hdmi_level;
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port = intel_get_encoder_port(encoder);
>  	const struct ddi_buf_trans *ddi_translations_hdmi;
>  
>  	if (IS_BROXTON(dev_priv))
> @@ -987,7 +969,7 @@ static void bxt_ddi_clock_get(struct intel_encoder *encoder,
>  				struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port = intel_get_encoder_port(encoder);
>  	uint32_t dpll = port;
>  
>  	pipe_config->port_clock = bxt_calc_pll_link(dev_priv, dpll);
> @@ -1130,7 +1112,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum pipe pipe = intel_crtc->pipe;
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
>  	uint32_t temp;
>  
> @@ -1226,7 +1208,7 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_encoder *intel_encoder = intel_connector->encoder;
>  	int type = intel_connector->base.connector_type;
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	enum pipe pipe = 0;
>  	enum transcoder cpu_transcoder;
>  	enum intel_display_power_domain power_domain;
> @@ -1286,7 +1268,7 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
>  {
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port = intel_get_encoder_port(encoder);
>  	enum intel_display_power_domain power_domain;
>  	u32 tmp;
>  	int i;
> @@ -1361,7 +1343,7 @@ void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>  
>  	if (cpu_transcoder != TRANSCODER_EDP)
> @@ -1589,7 +1571,7 @@ void intel_ddi_clk_select(struct intel_encoder *encoder,
>  			  const struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port = intel_get_encoder_port(encoder);
>  
>  	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
>  		uint32_t dpll = pipe_config->ddi_pll_sel;
> @@ -1616,7 +1598,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>  	struct drm_encoder *encoder = &intel_encoder->base;
>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
>  	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
>  
>  	if (type == INTEL_OUTPUT_HDMI) {
> @@ -1668,7 +1650,7 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>  	struct drm_encoder *encoder = &intel_encoder->base;
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
>  	uint32_t val;
>  	bool wait = false;
> @@ -1715,7 +1697,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
>  
>  	if (type == INTEL_OUTPUT_HDMI) {
> @@ -2276,7 +2258,7 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	int type = encoder->type;
> -	int port = intel_ddi_get_encoder_port(encoder);
> +	int port = intel_get_encoder_port(encoder);
>  	int ret;
>  
>  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9cbf543..44d20bd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -16277,6 +16277,24 @@ err:
>  	return ret;
>  }
>  
> +enum port intel_get_encoder_port(struct intel_encoder *encoder)
> +{
> +        switch (encoder->type) {
> +        case INTEL_OUTPUT_DP_MST:
> +                return enc_to_mst(&encoder->base)->primary->port;
> +        case INTEL_OUTPUT_DP:
> +        case INTEL_OUTPUT_EDP:
> +        case INTEL_OUTPUT_HDMI:
> +        case INTEL_OUTPUT_UNKNOWN:
> +                return enc_to_dig_port(&encoder->base)->port;
> +        case INTEL_OUTPUT_ANALOG:
> +                return intel_ddi_get_analog_port();
> +        default:
> +                MISSING_CASE(encoder->type);
> +                return PORT_A;
> +        }
> +}

I don't see the difference here. For me this patch looks more a rename and
re-positioning than a change on the enum port x intel_digital_port as advertised.
What am I missing?


> +
>  void intel_connector_unregister(struct drm_connector *connector)
>  {
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b1fc67e..19aab7b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1111,7 +1111,12 @@ void intel_ddi_clk_select(struct intel_encoder *encoder,
>  void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder);
>  void hsw_fdi_link_train(struct drm_crtc *crtc);
>  void intel_ddi_init(struct drm_device *dev, enum port port);
> -enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);
> +
> +static inline enum port intel_ddi_get_analog_port(void)
> +{
> +	return PORT_A;

PORT_E?

Anyway unless this is used in more places I think I prefer the old way.

> +}
> +
>  bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
>  void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc);
>  void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
> @@ -1189,6 +1194,7 @@ intel_crtc_has_dp_encoder(const struct intel_crtc_state *crtc_state)
>  		 (1 << INTEL_OUTPUT_DP_MST) |
>  		 (1 << INTEL_OUTPUT_EDP));
>  }
> +enum port intel_get_encoder_port(struct intel_encoder *encoder);
>  static inline void
>  intel_wait_for_vblank(struct drm_device *dev, int pipe)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index adca262..9c0043e 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -366,7 +366,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>  	if (intel_encoder->type == INTEL_OUTPUT_DSI)
>  		port = 0;
>  	else
> -		port = intel_ddi_get_encoder_port(intel_encoder);
> +		port = intel_get_encoder_port(intel_encoder);
>  
>  	if (port == PORT_E)  {
>  		port = 0;
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Aug. 19, 2016, 8:02 a.m. UTC | #2
On Mon, Aug 15, 2016 at 05:00:53PM -0700, Dhinakaran Pandiyan wrote:
> There are places in the driver where we just need the 'port' associated
> with an encoder and not 'struct intel_digital_port' that contains it.
> This basically is a generic implementation of intel_ddi_get_encoder_port()
> handling both DDI and Non-DDI encoders. The handling of the analog encoder
> is delegated to the DDI specific function.
> 
> The idea to have a generic implementation that returned the 'enum port'
> from 'struct intel_encoder' came from Ville.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c      | 42 ++++++++++-------------------------
>  drivers/gpu/drm/i915/intel_display.c  | 18 +++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h      |  8 ++++++-
>  drivers/gpu/drm/i915/intel_opregion.c |  2 +-
>  4 files changed, 38 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index c2df4e4..13ceef4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -301,24 +301,6 @@ static const struct bxt_ddi_buf_trans bxt_ddi_translations_hdmi[] = {
>  	{ 154, 0x9A, 1, 128, true },	/* 9:	1200		0   */
>  };
>  
> -enum port intel_ddi_get_encoder_port(struct intel_encoder *encoder)
> -{
> -	switch (encoder->type) {
> -	case INTEL_OUTPUT_DP_MST:
> -		return enc_to_mst(&encoder->base)->primary->port;
> -	case INTEL_OUTPUT_DP:
> -	case INTEL_OUTPUT_EDP:
> -	case INTEL_OUTPUT_HDMI:
> -	case INTEL_OUTPUT_UNKNOWN:
> -		return enc_to_dig_port(&encoder->base)->port;
> -	case INTEL_OUTPUT_ANALOG:
> -		return PORT_E;
> -	default:
> -		MISSING_CASE(encoder->type);
> -		return PORT_A;
> -	}
> -}

tbh if we really need this in general, I'd just store the port enum in
intel_encoder and that's it. Maybe add a PORT_NONE = -1 for all the cases
where it's not a port. A bit more churn to roll it tou, but this maze of
indirections and casting here is really not pretty.

The give-away is that we have this massive type switch block, for
something which is designed after OO priniciples: The correct way to fix
that is by moving stuff up in the inheritence hirarchy.
-Daniel

> -
>  static const struct ddi_buf_trans *
>  bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
>  {
> @@ -421,7 +403,7 @@ void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	u32 iboost_bit = 0;
>  	int i, n_dp_entries, n_edp_entries, size;
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port = intel_get_encoder_port(encoder);
>  	const struct ddi_buf_trans *ddi_translations_fdi;
>  	const struct ddi_buf_trans *ddi_translations_dp;
>  	const struct ddi_buf_trans *ddi_translations_edp;
> @@ -499,7 +481,7 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	u32 iboost_bit = 0;
>  	int n_hdmi_entries, hdmi_level;
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port = intel_get_encoder_port(encoder);
>  	const struct ddi_buf_trans *ddi_translations_hdmi;
>  
>  	if (IS_BROXTON(dev_priv))
> @@ -987,7 +969,7 @@ static void bxt_ddi_clock_get(struct intel_encoder *encoder,
>  				struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port = intel_get_encoder_port(encoder);
>  	uint32_t dpll = port;
>  
>  	pipe_config->port_clock = bxt_calc_pll_link(dev_priv, dpll);
> @@ -1130,7 +1112,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum pipe pipe = intel_crtc->pipe;
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
>  	uint32_t temp;
>  
> @@ -1226,7 +1208,7 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_encoder *intel_encoder = intel_connector->encoder;
>  	int type = intel_connector->base.connector_type;
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	enum pipe pipe = 0;
>  	enum transcoder cpu_transcoder;
>  	enum intel_display_power_domain power_domain;
> @@ -1286,7 +1268,7 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
>  {
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port = intel_get_encoder_port(encoder);
>  	enum intel_display_power_domain power_domain;
>  	u32 tmp;
>  	int i;
> @@ -1361,7 +1343,7 @@ void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>  
>  	if (cpu_transcoder != TRANSCODER_EDP)
> @@ -1589,7 +1571,7 @@ void intel_ddi_clk_select(struct intel_encoder *encoder,
>  			  const struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port = intel_get_encoder_port(encoder);
>  
>  	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
>  		uint32_t dpll = pipe_config->ddi_pll_sel;
> @@ -1616,7 +1598,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>  	struct drm_encoder *encoder = &intel_encoder->base;
>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
>  	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
>  
>  	if (type == INTEL_OUTPUT_HDMI) {
> @@ -1668,7 +1650,7 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>  	struct drm_encoder *encoder = &intel_encoder->base;
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
>  	uint32_t val;
>  	bool wait = false;
> @@ -1715,7 +1697,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port = intel_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
>  
>  	if (type == INTEL_OUTPUT_HDMI) {
> @@ -2276,7 +2258,7 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	int type = encoder->type;
> -	int port = intel_ddi_get_encoder_port(encoder);
> +	int port = intel_get_encoder_port(encoder);
>  	int ret;
>  
>  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9cbf543..44d20bd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -16277,6 +16277,24 @@ err:
>  	return ret;
>  }
>  
> +enum port intel_get_encoder_port(struct intel_encoder *encoder)
> +{
> +        switch (encoder->type) {
> +        case INTEL_OUTPUT_DP_MST:
> +                return enc_to_mst(&encoder->base)->primary->port;
> +        case INTEL_OUTPUT_DP:
> +        case INTEL_OUTPUT_EDP:
> +        case INTEL_OUTPUT_HDMI:
> +        case INTEL_OUTPUT_UNKNOWN:
> +                return enc_to_dig_port(&encoder->base)->port;
> +        case INTEL_OUTPUT_ANALOG:
> +                return intel_ddi_get_analog_port();
> +        default:
> +                MISSING_CASE(encoder->type);
> +                return PORT_A;
> +        }
> +}
> +
>  void intel_connector_unregister(struct drm_connector *connector)
>  {
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b1fc67e..19aab7b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1111,7 +1111,12 @@ void intel_ddi_clk_select(struct intel_encoder *encoder,
>  void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder);
>  void hsw_fdi_link_train(struct drm_crtc *crtc);
>  void intel_ddi_init(struct drm_device *dev, enum port port);
> -enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);
> +
> +static inline enum port intel_ddi_get_analog_port(void)
> +{
> +	return PORT_A;
> +}
> +
>  bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
>  void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc);
>  void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
> @@ -1189,6 +1194,7 @@ intel_crtc_has_dp_encoder(const struct intel_crtc_state *crtc_state)
>  		 (1 << INTEL_OUTPUT_DP_MST) |
>  		 (1 << INTEL_OUTPUT_EDP));
>  }
> +enum port intel_get_encoder_port(struct intel_encoder *encoder);
>  static inline void
>  intel_wait_for_vblank(struct drm_device *dev, int pipe)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index adca262..9c0043e 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -366,7 +366,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>  	if (intel_encoder->type == INTEL_OUTPUT_DSI)
>  		port = 0;
>  	else
> -		port = intel_ddi_get_encoder_port(intel_encoder);
> +		port = intel_get_encoder_port(intel_encoder);
>  
>  	if (port == PORT_E)  {
>  		port = 0;
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dhinakaran Pandiyan Aug. 19, 2016, 4:57 p.m. UTC | #3
On Thu, 2016-08-18 at 21:39 -0700, Rodrigo Vivi wrote:
> On Mon, Aug 15, 2016 at 05:00:53PM -0700, Dhinakaran Pandiyan wrote:

> > There are places in the driver where we just need the 'port' associated

> > with an encoder and not 'struct intel_digital_port' that contains it.

> > This basically is a generic implementation of intel_ddi_get_encoder_port()

> > handling both DDI and Non-DDI encoders. The handling of the analog encoder

> > is delegated to the DDI specific function.

> > 

> > The idea to have a generic implementation that returned the 'enum port'

> > from 'struct intel_encoder' came from Ville.

> > 

> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> > ---

> >  drivers/gpu/drm/i915/intel_ddi.c      | 42 ++++++++++-------------------------

> >  drivers/gpu/drm/i915/intel_display.c  | 18 +++++++++++++++

> >  drivers/gpu/drm/i915/intel_drv.h      |  8 ++++++-

> >  drivers/gpu/drm/i915/intel_opregion.c |  2 +-

> >  4 files changed, 38 insertions(+), 32 deletions(-)

> > 

> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c

> > index c2df4e4..13ceef4 100644

> > --- a/drivers/gpu/drm/i915/intel_ddi.c

> > +++ b/drivers/gpu/drm/i915/intel_ddi.c

> > @@ -301,24 +301,6 @@ static const struct bxt_ddi_buf_trans bxt_ddi_translations_hdmi[] = {

> >  	{ 154, 0x9A, 1, 128, true },	/* 9:	1200		0   */

> >  };

> >  

> > -enum port intel_ddi_get_encoder_port(struct intel_encoder *encoder)

> > -{

> > -	switch (encoder->type) {

> > -	case INTEL_OUTPUT_DP_MST:

> > -		return enc_to_mst(&encoder->base)->primary->port;

> > -	case INTEL_OUTPUT_DP:

> > -	case INTEL_OUTPUT_EDP:

> > -	case INTEL_OUTPUT_HDMI:

> > -	case INTEL_OUTPUT_UNKNOWN:

> > -		return enc_to_dig_port(&encoder->base)->port;

> > -	case INTEL_OUTPUT_ANALOG:

> > -		return PORT_E;

> > -	default:

> > -		MISSING_CASE(encoder->type);

> > -		return PORT_A;

> > -	}

> > -}

> > -

> >  static const struct ddi_buf_trans *

> >  bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)

> >  {

> > @@ -421,7 +403,7 @@ void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder)

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> >  	u32 iboost_bit = 0;

> >  	int i, n_dp_entries, n_edp_entries, size;

> > -	enum port port = intel_ddi_get_encoder_port(encoder);

> > +	enum port port = intel_get_encoder_port(encoder);

> >  	const struct ddi_buf_trans *ddi_translations_fdi;

> >  	const struct ddi_buf_trans *ddi_translations_dp;

> >  	const struct ddi_buf_trans *ddi_translations_edp;

> > @@ -499,7 +481,7 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder)

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> >  	u32 iboost_bit = 0;

> >  	int n_hdmi_entries, hdmi_level;

> > -	enum port port = intel_ddi_get_encoder_port(encoder);

> > +	enum port port = intel_get_encoder_port(encoder);

> >  	const struct ddi_buf_trans *ddi_translations_hdmi;

> >  

> >  	if (IS_BROXTON(dev_priv))

> > @@ -987,7 +969,7 @@ static void bxt_ddi_clock_get(struct intel_encoder *encoder,

> >  				struct intel_crtc_state *pipe_config)

> >  {

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> > -	enum port port = intel_ddi_get_encoder_port(encoder);

> > +	enum port port = intel_get_encoder_port(encoder);

> >  	uint32_t dpll = port;

> >  

> >  	pipe_config->port_clock = bxt_calc_pll_link(dev_priv, dpll);

> > @@ -1130,7 +1112,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> >  	enum pipe pipe = intel_crtc->pipe;

> >  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	int type = intel_encoder->type;

> >  	uint32_t temp;

> >  

> > @@ -1226,7 +1208,7 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> >  	struct intel_encoder *intel_encoder = intel_connector->encoder;

> >  	int type = intel_connector->base.connector_type;

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	enum pipe pipe = 0;

> >  	enum transcoder cpu_transcoder;

> >  	enum intel_display_power_domain power_domain;

> > @@ -1286,7 +1268,7 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,

> >  {

> >  	struct drm_device *dev = encoder->base.dev;

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> > -	enum port port = intel_ddi_get_encoder_port(encoder);

> > +	enum port port = intel_get_encoder_port(encoder);

> >  	enum intel_display_power_domain power_domain;

> >  	u32 tmp;

> >  	int i;

> > @@ -1361,7 +1343,7 @@ void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)

> >  	struct drm_device *dev = crtc->dev;

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> >  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;

> >  

> >  	if (cpu_transcoder != TRANSCODER_EDP)

> > @@ -1589,7 +1571,7 @@ void intel_ddi_clk_select(struct intel_encoder *encoder,

> >  			  const struct intel_crtc_state *pipe_config)

> >  {

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> > -	enum port port = intel_ddi_get_encoder_port(encoder);

> > +	enum port port = intel_get_encoder_port(encoder);

> >  

> >  	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {

> >  		uint32_t dpll = pipe_config->ddi_pll_sel;

> > @@ -1616,7 +1598,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)

> >  	struct drm_encoder *encoder = &intel_encoder->base;

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);

> >  	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	int type = intel_encoder->type;

> >  

> >  	if (type == INTEL_OUTPUT_HDMI) {

> > @@ -1668,7 +1650,7 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)

> >  	struct drm_encoder *encoder = &intel_encoder->base;

> >  	struct drm_device *dev = encoder->dev;

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	int type = intel_encoder->type;

> >  	uint32_t val;

> >  	bool wait = false;

> > @@ -1715,7 +1697,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)

> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

> >  	struct drm_device *dev = encoder->dev;

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	int type = intel_encoder->type;

> >  

> >  	if (type == INTEL_OUTPUT_HDMI) {

> > @@ -2276,7 +2258,7 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,

> >  {

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> >  	int type = encoder->type;

> > -	int port = intel_ddi_get_encoder_port(encoder);

> > +	int port = intel_get_encoder_port(encoder);

> >  	int ret;

> >  

> >  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");

> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c

> > index 9cbf543..44d20bd 100644

> > --- a/drivers/gpu/drm/i915/intel_display.c

> > +++ b/drivers/gpu/drm/i915/intel_display.c

> > @@ -16277,6 +16277,24 @@ err:

> >  	return ret;

> >  }

> >  

> > +enum port intel_get_encoder_port(struct intel_encoder *encoder)

> > +{

> > +        switch (encoder->type) {

> > +        case INTEL_OUTPUT_DP_MST:

> > +                return enc_to_mst(&encoder->base)->primary->port;

> > +        case INTEL_OUTPUT_DP:

> > +        case INTEL_OUTPUT_EDP:

> > +        case INTEL_OUTPUT_HDMI:

> > +        case INTEL_OUTPUT_UNKNOWN:

> > +                return enc_to_dig_port(&encoder->base)->port;

> > +        case INTEL_OUTPUT_ANALOG:

> > +                return intel_ddi_get_analog_port();

> > +        default:

> > +                MISSING_CASE(encoder->type);

> > +                return PORT_A;

> > +        }

> > +}

> 

> I don't see the difference here. For me this patch looks more a rename and

> re-positioning than a change on the enum port x intel_digital_port as advertised.

> What am I missing?

> 

> 


The original implementation was to append MST handling to
enc_to_dig_port(). But, we decided not to mess with that function as we
only cared about the member intel_digital_port.port.This solution is to
re-use intel_ddi_get_encoder_port() functionality by making it generic.

> > +

> >  void intel_connector_unregister(struct drm_connector *connector)

> >  {

> >  	struct intel_connector *intel_connector = to_intel_connector(connector);

> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h

> > index b1fc67e..19aab7b 100644

> > --- a/drivers/gpu/drm/i915/intel_drv.h

> > +++ b/drivers/gpu/drm/i915/intel_drv.h

> > @@ -1111,7 +1111,12 @@ void intel_ddi_clk_select(struct intel_encoder *encoder,

> >  void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder);

> >  void hsw_fdi_link_train(struct drm_crtc *crtc);

> >  void intel_ddi_init(struct drm_device *dev, enum port port);

> > -enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);

> > +

> > +static inline enum port intel_ddi_get_analog_port(void)

> > +{

> > +	return PORT_A;

> 

> PORT_E?


Thanks for spotting, I have to fix this.

> 

> Anyway unless this is used in more places I think I prefer the old way.


Several functions in intel_audio.c, where we need the port from encoder,
will make use of this. We need a generic conversion function for all
encoder output types and not tied to DDI platforms.

> 

> > +}

> > +

> >  bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);

> >  void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc);

> >  void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,

> > @@ -1189,6 +1194,7 @@ intel_crtc_has_dp_encoder(const struct intel_crtc_state *crtc_state)

> >  		 (1 << INTEL_OUTPUT_DP_MST) |

> >  		 (1 << INTEL_OUTPUT_EDP));

> >  }

> > +enum port intel_get_encoder_port(struct intel_encoder *encoder);

> >  static inline void

> >  intel_wait_for_vblank(struct drm_device *dev, int pipe)

> >  {

> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c

> > index adca262..9c0043e 100644

> > --- a/drivers/gpu/drm/i915/intel_opregion.c

> > +++ b/drivers/gpu/drm/i915/intel_opregion.c

> > @@ -366,7 +366,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,

> >  	if (intel_encoder->type == INTEL_OUTPUT_DSI)

> >  		port = 0;

> >  	else

> > -		port = intel_ddi_get_encoder_port(intel_encoder);

> > +		port = intel_get_encoder_port(intel_encoder);

> >  

> >  	if (port == PORT_E)  {

> >  		port = 0;

> > -- 

> > 2.5.0

> > 

> > _______________________________________________

> > Intel-gfx mailing list

> > Intel-gfx@lists.freedesktop.org

> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dhinakaran Pandiyan Aug. 19, 2016, 6:03 p.m. UTC | #4
On Fri, 2016-08-19 at 10:02 +0200, Daniel Vetter wrote:
> On Mon, Aug 15, 2016 at 05:00:53PM -0700, Dhinakaran Pandiyan wrote:

> > There are places in the driver where we just need the 'port' associated

> > with an encoder and not 'struct intel_digital_port' that contains it.

> > This basically is a generic implementation of intel_ddi_get_encoder_port()

> > handling both DDI and Non-DDI encoders. The handling of the analog encoder

> > is delegated to the DDI specific function.

> > 

> > The idea to have a generic implementation that returned the 'enum port'

> > from 'struct intel_encoder' came from Ville.

> > 

> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> > ---

> >  drivers/gpu/drm/i915/intel_ddi.c      | 42 ++++++++++-------------------------

> >  drivers/gpu/drm/i915/intel_display.c  | 18 +++++++++++++++

> >  drivers/gpu/drm/i915/intel_drv.h      |  8 ++++++-

> >  drivers/gpu/drm/i915/intel_opregion.c |  2 +-

> >  4 files changed, 38 insertions(+), 32 deletions(-)

> > 

> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c

> > index c2df4e4..13ceef4 100644

> > --- a/drivers/gpu/drm/i915/intel_ddi.c

> > +++ b/drivers/gpu/drm/i915/intel_ddi.c

> > @@ -301,24 +301,6 @@ static const struct bxt_ddi_buf_trans bxt_ddi_translations_hdmi[] = {

> >  	{ 154, 0x9A, 1, 128, true },	/* 9:	1200		0   */

> >  };

> >  

> > -enum port intel_ddi_get_encoder_port(struct intel_encoder *encoder)

> > -{

> > -	switch (encoder->type) {

> > -	case INTEL_OUTPUT_DP_MST:

> > -		return enc_to_mst(&encoder->base)->primary->port;

> > -	case INTEL_OUTPUT_DP:

> > -	case INTEL_OUTPUT_EDP:

> > -	case INTEL_OUTPUT_HDMI:

> > -	case INTEL_OUTPUT_UNKNOWN:

> > -		return enc_to_dig_port(&encoder->base)->port;

> > -	case INTEL_OUTPUT_ANALOG:

> > -		return PORT_E;

> > -	default:

> > -		MISSING_CASE(encoder->type);

> > -		return PORT_A;

> > -	}

> > -}

> 

> tbh if we really need this in general, I'd just store the port enum in

> intel_encoder and that's it. Maybe add a PORT_NONE = -1 for all the cases

> where it's not a port. A bit more churn to roll it tou, but this maze of

> indirections and casting here is really not pretty.

> 

A cursory check shows around 12 places (combined) in intel_ddi.c,
intel_audio.c and intel_hdmi.c where this can be useful.


Out of curiosity, what are the cases where we don't have a port? Before
hdmi/dp initialization? Or wireless display? 

> The give-away is that we have this massive type switch block, for

> something which is designed after OO priniciples: The correct way to fix

> that is by moving stuff up in the inheritence hirarchy.

> -Daniel

> 

I get your point. Something like this?

struct intel_encoder{
	...
	enum port attached_port;

}



> > -

> >  static const struct ddi_buf_trans *

> >  bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)

> >  {

> > @@ -421,7 +403,7 @@ void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder)

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> >  	u32 iboost_bit = 0;

> >  	int i, n_dp_entries, n_edp_entries, size;

> > -	enum port port = intel_ddi_get_encoder_port(encoder);

> > +	enum port port = intel_get_encoder_port(encoder);

> >  	const struct ddi_buf_trans *ddi_translations_fdi;

> >  	const struct ddi_buf_trans *ddi_translations_dp;

> >  	const struct ddi_buf_trans *ddi_translations_edp;

> > @@ -499,7 +481,7 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder)

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> >  	u32 iboost_bit = 0;

> >  	int n_hdmi_entries, hdmi_level;

> > -	enum port port = intel_ddi_get_encoder_port(encoder);

> > +	enum port port = intel_get_encoder_port(encoder);

> >  	const struct ddi_buf_trans *ddi_translations_hdmi;

> >  

> >  	if (IS_BROXTON(dev_priv))

> > @@ -987,7 +969,7 @@ static void bxt_ddi_clock_get(struct intel_encoder *encoder,

> >  				struct intel_crtc_state *pipe_config)

> >  {

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> > -	enum port port = intel_ddi_get_encoder_port(encoder);

> > +	enum port port = intel_get_encoder_port(encoder);

> >  	uint32_t dpll = port;

> >  

> >  	pipe_config->port_clock = bxt_calc_pll_link(dev_priv, dpll);

> > @@ -1130,7 +1112,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> >  	enum pipe pipe = intel_crtc->pipe;

> >  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	int type = intel_encoder->type;

> >  	uint32_t temp;

> >  

> > @@ -1226,7 +1208,7 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> >  	struct intel_encoder *intel_encoder = intel_connector->encoder;

> >  	int type = intel_connector->base.connector_type;

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	enum pipe pipe = 0;

> >  	enum transcoder cpu_transcoder;

> >  	enum intel_display_power_domain power_domain;

> > @@ -1286,7 +1268,7 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,

> >  {

> >  	struct drm_device *dev = encoder->base.dev;

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> > -	enum port port = intel_ddi_get_encoder_port(encoder);

> > +	enum port port = intel_get_encoder_port(encoder);

> >  	enum intel_display_power_domain power_domain;

> >  	u32 tmp;

> >  	int i;

> > @@ -1361,7 +1343,7 @@ void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)

> >  	struct drm_device *dev = crtc->dev;

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> >  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;

> >  

> >  	if (cpu_transcoder != TRANSCODER_EDP)

> > @@ -1589,7 +1571,7 @@ void intel_ddi_clk_select(struct intel_encoder *encoder,

> >  			  const struct intel_crtc_state *pipe_config)

> >  {

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> > -	enum port port = intel_ddi_get_encoder_port(encoder);

> > +	enum port port = intel_get_encoder_port(encoder);

> >  

> >  	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {

> >  		uint32_t dpll = pipe_config->ddi_pll_sel;

> > @@ -1616,7 +1598,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)

> >  	struct drm_encoder *encoder = &intel_encoder->base;

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);

> >  	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	int type = intel_encoder->type;

> >  

> >  	if (type == INTEL_OUTPUT_HDMI) {

> > @@ -1668,7 +1650,7 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)

> >  	struct drm_encoder *encoder = &intel_encoder->base;

> >  	struct drm_device *dev = encoder->dev;

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	int type = intel_encoder->type;

> >  	uint32_t val;

> >  	bool wait = false;

> > @@ -1715,7 +1697,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)

> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

> >  	struct drm_device *dev = encoder->dev;

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);

> > +	enum port port = intel_get_encoder_port(intel_encoder);

> >  	int type = intel_encoder->type;

> >  

> >  	if (type == INTEL_OUTPUT_HDMI) {

> > @@ -2276,7 +2258,7 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,

> >  {

> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> >  	int type = encoder->type;

> > -	int port = intel_ddi_get_encoder_port(encoder);

> > +	int port = intel_get_encoder_port(encoder);

> >  	int ret;

> >  

> >  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");

> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c

> > index 9cbf543..44d20bd 100644

> > --- a/drivers/gpu/drm/i915/intel_display.c

> > +++ b/drivers/gpu/drm/i915/intel_display.c

> > @@ -16277,6 +16277,24 @@ err:

> >  	return ret;

> >  }

> >  

> > +enum port intel_get_encoder_port(struct intel_encoder *encoder)

> > +{

> > +        switch (encoder->type) {

> > +        case INTEL_OUTPUT_DP_MST:

> > +                return enc_to_mst(&encoder->base)->primary->port;

> > +        case INTEL_OUTPUT_DP:

> > +        case INTEL_OUTPUT_EDP:

> > +        case INTEL_OUTPUT_HDMI:

> > +        case INTEL_OUTPUT_UNKNOWN:

> > +                return enc_to_dig_port(&encoder->base)->port;

> > +        case INTEL_OUTPUT_ANALOG:

> > +                return intel_ddi_get_analog_port();

> > +        default:

> > +                MISSING_CASE(encoder->type);

> > +                return PORT_A;

> > +        }

> > +}

> > +

> >  void intel_connector_unregister(struct drm_connector *connector)

> >  {

> >  	struct intel_connector *intel_connector = to_intel_connector(connector);

> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h

> > index b1fc67e..19aab7b 100644

> > --- a/drivers/gpu/drm/i915/intel_drv.h

> > +++ b/drivers/gpu/drm/i915/intel_drv.h

> > @@ -1111,7 +1111,12 @@ void intel_ddi_clk_select(struct intel_encoder *encoder,

> >  void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder);

> >  void hsw_fdi_link_train(struct drm_crtc *crtc);

> >  void intel_ddi_init(struct drm_device *dev, enum port port);

> > -enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);

> > +

> > +static inline enum port intel_ddi_get_analog_port(void)

> > +{

> > +	return PORT_A;

> > +}

> > +

> >  bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);

> >  void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc);

> >  void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,

> > @@ -1189,6 +1194,7 @@ intel_crtc_has_dp_encoder(const struct intel_crtc_state *crtc_state)

> >  		 (1 << INTEL_OUTPUT_DP_MST) |

> >  		 (1 << INTEL_OUTPUT_EDP));

> >  }

> > +enum port intel_get_encoder_port(struct intel_encoder *encoder);

> >  static inline void

> >  intel_wait_for_vblank(struct drm_device *dev, int pipe)

> >  {

> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c

> > index adca262..9c0043e 100644

> > --- a/drivers/gpu/drm/i915/intel_opregion.c

> > +++ b/drivers/gpu/drm/i915/intel_opregion.c

> > @@ -366,7 +366,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,

> >  	if (intel_encoder->type == INTEL_OUTPUT_DSI)

> >  		port = 0;

> >  	else

> > -		port = intel_ddi_get_encoder_port(intel_encoder);

> > +		port = intel_get_encoder_port(intel_encoder);

> >  

> >  	if (port == PORT_E)  {

> >  		port = 0;

> > -- 

> > 2.5.0

> > 

> > _______________________________________________

> > Intel-gfx mailing list

> > Intel-gfx@lists.freedesktop.org

> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

>
Daniel Vetter Aug. 22, 2016, 7:51 a.m. UTC | #5
On Fri, Aug 19, 2016 at 06:03:06PM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2016-08-19 at 10:02 +0200, Daniel Vetter wrote:
> > On Mon, Aug 15, 2016 at 05:00:53PM -0700, Dhinakaran Pandiyan wrote:
> > > There are places in the driver where we just need the 'port' associated
> > > with an encoder and not 'struct intel_digital_port' that contains it.
> > > This basically is a generic implementation of intel_ddi_get_encoder_port()
> > > handling both DDI and Non-DDI encoders. The handling of the analog encoder
> > > is delegated to the DDI specific function.
> > > 
> > > The idea to have a generic implementation that returned the 'enum port'
> > > from 'struct intel_encoder' came from Ville.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c      | 42 ++++++++++-------------------------
> > >  drivers/gpu/drm/i915/intel_display.c  | 18 +++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h      |  8 ++++++-
> > >  drivers/gpu/drm/i915/intel_opregion.c |  2 +-
> > >  4 files changed, 38 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index c2df4e4..13ceef4 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -301,24 +301,6 @@ static const struct bxt_ddi_buf_trans bxt_ddi_translations_hdmi[] = {
> > >  	{ 154, 0x9A, 1, 128, true },	/* 9:	1200		0   */
> > >  };
> > >  
> > > -enum port intel_ddi_get_encoder_port(struct intel_encoder *encoder)
> > > -{
> > > -	switch (encoder->type) {
> > > -	case INTEL_OUTPUT_DP_MST:
> > > -		return enc_to_mst(&encoder->base)->primary->port;
> > > -	case INTEL_OUTPUT_DP:
> > > -	case INTEL_OUTPUT_EDP:
> > > -	case INTEL_OUTPUT_HDMI:
> > > -	case INTEL_OUTPUT_UNKNOWN:
> > > -		return enc_to_dig_port(&encoder->base)->port;
> > > -	case INTEL_OUTPUT_ANALOG:
> > > -		return PORT_E;
> > > -	default:
> > > -		MISSING_CASE(encoder->type);
> > > -		return PORT_A;
> > > -	}
> > > -}
> > 
> > tbh if we really need this in general, I'd just store the port enum in
> > intel_encoder and that's it. Maybe add a PORT_NONE = -1 for all the cases
> > where it's not a port. A bit more churn to roll it tou, but this maze of
> > indirections and casting here is really not pretty.
> > 
> A cursory check shows around 12 places (combined) in intel_ddi.c,
> intel_audio.c and intel_hdmi.c where this can be useful.
> 
> 
> Out of curiosity, what are the cases where we don't have a port? Before
> hdmi/dp initialization? Or wireless display? 

We shouldn't look at encoders before they're fully set up. And we don't
support wireless display. The PORT_NONE case is more for older
platforms/encoders like lvds, tv. (s)dvo does have the concept of
different ports.
> 
> > The give-away is that we have this massive type switch block, for
> > something which is designed after OO priniciples: The correct way to fix
> > that is by moving stuff up in the inheritence hirarchy.
> > -Daniel
> > 
> I get your point. Something like this?
> 
> struct intel_encoder{
> 	...
> 	enum port attached_port;
> 
> }

Yup.
-Daniel

> 
> 
> 
> > > -
> > >  static const struct ddi_buf_trans *
> > >  bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
> > >  {
> > > @@ -421,7 +403,7 @@ void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder)
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >  	u32 iboost_bit = 0;
> > >  	int i, n_dp_entries, n_edp_entries, size;
> > > -	enum port port = intel_ddi_get_encoder_port(encoder);
> > > +	enum port port = intel_get_encoder_port(encoder);
> > >  	const struct ddi_buf_trans *ddi_translations_fdi;
> > >  	const struct ddi_buf_trans *ddi_translations_dp;
> > >  	const struct ddi_buf_trans *ddi_translations_edp;
> > > @@ -499,7 +481,7 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder)
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >  	u32 iboost_bit = 0;
> > >  	int n_hdmi_entries, hdmi_level;
> > > -	enum port port = intel_ddi_get_encoder_port(encoder);
> > > +	enum port port = intel_get_encoder_port(encoder);
> > >  	const struct ddi_buf_trans *ddi_translations_hdmi;
> > >  
> > >  	if (IS_BROXTON(dev_priv))
> > > @@ -987,7 +969,7 @@ static void bxt_ddi_clock_get(struct intel_encoder *encoder,
> > >  				struct intel_crtc_state *pipe_config)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > -	enum port port = intel_ddi_get_encoder_port(encoder);
> > > +	enum port port = intel_get_encoder_port(encoder);
> > >  	uint32_t dpll = port;
> > >  
> > >  	pipe_config->port_clock = bxt_calc_pll_link(dev_priv, dpll);
> > > @@ -1130,7 +1112,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	enum pipe pipe = intel_crtc->pipe;
> > >  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> > > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> > > +	enum port port = intel_get_encoder_port(intel_encoder);
> > >  	int type = intel_encoder->type;
> > >  	uint32_t temp;
> > >  
> > > @@ -1226,7 +1208,7 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_encoder *intel_encoder = intel_connector->encoder;
> > >  	int type = intel_connector->base.connector_type;
> > > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> > > +	enum port port = intel_get_encoder_port(intel_encoder);
> > >  	enum pipe pipe = 0;
> > >  	enum transcoder cpu_transcoder;
> > >  	enum intel_display_power_domain power_domain;
> > > @@ -1286,7 +1268,7 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> > >  {
> > >  	struct drm_device *dev = encoder->base.dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > -	enum port port = intel_ddi_get_encoder_port(encoder);
> > > +	enum port port = intel_get_encoder_port(encoder);
> > >  	enum intel_display_power_domain power_domain;
> > >  	u32 tmp;
> > >  	int i;
> > > @@ -1361,7 +1343,7 @@ void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)
> > >  	struct drm_device *dev = crtc->dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
> > > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> > > +	enum port port = intel_get_encoder_port(intel_encoder);
> > >  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> > >  
> > >  	if (cpu_transcoder != TRANSCODER_EDP)
> > > @@ -1589,7 +1571,7 @@ void intel_ddi_clk_select(struct intel_encoder *encoder,
> > >  			  const struct intel_crtc_state *pipe_config)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > -	enum port port = intel_ddi_get_encoder_port(encoder);
> > > +	enum port port = intel_get_encoder_port(encoder);
> > >  
> > >  	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> > >  		uint32_t dpll = pipe_config->ddi_pll_sel;
> > > @@ -1616,7 +1598,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > >  	struct drm_encoder *encoder = &intel_encoder->base;
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> > >  	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> > > +	enum port port = intel_get_encoder_port(intel_encoder);
> > >  	int type = intel_encoder->type;
> > >  
> > >  	if (type == INTEL_OUTPUT_HDMI) {
> > > @@ -1668,7 +1650,7 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> > >  	struct drm_encoder *encoder = &intel_encoder->base;
> > >  	struct drm_device *dev = encoder->dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> > > +	enum port port = intel_get_encoder_port(intel_encoder);
> > >  	int type = intel_encoder->type;
> > >  	uint32_t val;
> > >  	bool wait = false;
> > > @@ -1715,7 +1697,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >  	struct drm_device *dev = encoder->dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> > > +	enum port port = intel_get_encoder_port(intel_encoder);
> > >  	int type = intel_encoder->type;
> > >  
> > >  	if (type == INTEL_OUTPUT_HDMI) {
> > > @@ -2276,7 +2258,7 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >  	int type = encoder->type;
> > > -	int port = intel_ddi_get_encoder_port(encoder);
> > > +	int port = intel_get_encoder_port(encoder);
> > >  	int ret;
> > >  
> > >  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 9cbf543..44d20bd 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -16277,6 +16277,24 @@ err:
> > >  	return ret;
> > >  }
> > >  
> > > +enum port intel_get_encoder_port(struct intel_encoder *encoder)
> > > +{
> > > +        switch (encoder->type) {
> > > +        case INTEL_OUTPUT_DP_MST:
> > > +                return enc_to_mst(&encoder->base)->primary->port;
> > > +        case INTEL_OUTPUT_DP:
> > > +        case INTEL_OUTPUT_EDP:
> > > +        case INTEL_OUTPUT_HDMI:
> > > +        case INTEL_OUTPUT_UNKNOWN:
> > > +                return enc_to_dig_port(&encoder->base)->port;
> > > +        case INTEL_OUTPUT_ANALOG:
> > > +                return intel_ddi_get_analog_port();
> > > +        default:
> > > +                MISSING_CASE(encoder->type);
> > > +                return PORT_A;
> > > +        }
> > > +}
> > > +
> > >  void intel_connector_unregister(struct drm_connector *connector)
> > >  {
> > >  	struct intel_connector *intel_connector = to_intel_connector(connector);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index b1fc67e..19aab7b 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1111,7 +1111,12 @@ void intel_ddi_clk_select(struct intel_encoder *encoder,
> > >  void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder);
> > >  void hsw_fdi_link_train(struct drm_crtc *crtc);
> > >  void intel_ddi_init(struct drm_device *dev, enum port port);
> > > -enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);
> > > +
> > > +static inline enum port intel_ddi_get_analog_port(void)
> > > +{
> > > +	return PORT_A;
> > > +}
> > > +
> > >  bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
> > >  void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc);
> > >  void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
> > > @@ -1189,6 +1194,7 @@ intel_crtc_has_dp_encoder(const struct intel_crtc_state *crtc_state)
> > >  		 (1 << INTEL_OUTPUT_DP_MST) |
> > >  		 (1 << INTEL_OUTPUT_EDP));
> > >  }
> > > +enum port intel_get_encoder_port(struct intel_encoder *encoder);
> > >  static inline void
> > >  intel_wait_for_vblank(struct drm_device *dev, int pipe)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> > > index adca262..9c0043e 100644
> > > --- a/drivers/gpu/drm/i915/intel_opregion.c
> > > +++ b/drivers/gpu/drm/i915/intel_opregion.c
> > > @@ -366,7 +366,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> > >  	if (intel_encoder->type == INTEL_OUTPUT_DSI)
> > >  		port = 0;
> > >  	else
> > > -		port = intel_ddi_get_encoder_port(intel_encoder);
> > > +		port = intel_get_encoder_port(intel_encoder);
> > >  
> > >  	if (port == PORT_E)  {
> > >  		port = 0;
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index c2df4e4..13ceef4 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -301,24 +301,6 @@  static const struct bxt_ddi_buf_trans bxt_ddi_translations_hdmi[] = {
 	{ 154, 0x9A, 1, 128, true },	/* 9:	1200		0   */
 };
 
-enum port intel_ddi_get_encoder_port(struct intel_encoder *encoder)
-{
-	switch (encoder->type) {
-	case INTEL_OUTPUT_DP_MST:
-		return enc_to_mst(&encoder->base)->primary->port;
-	case INTEL_OUTPUT_DP:
-	case INTEL_OUTPUT_EDP:
-	case INTEL_OUTPUT_HDMI:
-	case INTEL_OUTPUT_UNKNOWN:
-		return enc_to_dig_port(&encoder->base)->port;
-	case INTEL_OUTPUT_ANALOG:
-		return PORT_E;
-	default:
-		MISSING_CASE(encoder->type);
-		return PORT_A;
-	}
-}
-
 static const struct ddi_buf_trans *
 bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
 {
@@ -421,7 +403,7 @@  void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder)
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	u32 iboost_bit = 0;
 	int i, n_dp_entries, n_edp_entries, size;
-	enum port port = intel_ddi_get_encoder_port(encoder);
+	enum port port = intel_get_encoder_port(encoder);
 	const struct ddi_buf_trans *ddi_translations_fdi;
 	const struct ddi_buf_trans *ddi_translations_dp;
 	const struct ddi_buf_trans *ddi_translations_edp;
@@ -499,7 +481,7 @@  static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder)
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	u32 iboost_bit = 0;
 	int n_hdmi_entries, hdmi_level;
-	enum port port = intel_ddi_get_encoder_port(encoder);
+	enum port port = intel_get_encoder_port(encoder);
 	const struct ddi_buf_trans *ddi_translations_hdmi;
 
 	if (IS_BROXTON(dev_priv))
@@ -987,7 +969,7 @@  static void bxt_ddi_clock_get(struct intel_encoder *encoder,
 				struct intel_crtc_state *pipe_config)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum port port = intel_ddi_get_encoder_port(encoder);
+	enum port port = intel_get_encoder_port(encoder);
 	uint32_t dpll = port;
 
 	pipe_config->port_clock = bxt_calc_pll_link(dev_priv, dpll);
@@ -1130,7 +1112,7 @@  void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	enum pipe pipe = intel_crtc->pipe;
 	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
-	enum port port = intel_ddi_get_encoder_port(intel_encoder);
+	enum port port = intel_get_encoder_port(intel_encoder);
 	int type = intel_encoder->type;
 	uint32_t temp;
 
@@ -1226,7 +1208,7 @@  bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_encoder *intel_encoder = intel_connector->encoder;
 	int type = intel_connector->base.connector_type;
-	enum port port = intel_ddi_get_encoder_port(intel_encoder);
+	enum port port = intel_get_encoder_port(intel_encoder);
 	enum pipe pipe = 0;
 	enum transcoder cpu_transcoder;
 	enum intel_display_power_domain power_domain;
@@ -1286,7 +1268,7 @@  bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
 {
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	enum port port = intel_ddi_get_encoder_port(encoder);
+	enum port port = intel_get_encoder_port(encoder);
 	enum intel_display_power_domain power_domain;
 	u32 tmp;
 	int i;
@@ -1361,7 +1343,7 @@  void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
-	enum port port = intel_ddi_get_encoder_port(intel_encoder);
+	enum port port = intel_get_encoder_port(intel_encoder);
 	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
 
 	if (cpu_transcoder != TRANSCODER_EDP)
@@ -1589,7 +1571,7 @@  void intel_ddi_clk_select(struct intel_encoder *encoder,
 			  const struct intel_crtc_state *pipe_config)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum port port = intel_ddi_get_encoder_port(encoder);
+	enum port port = intel_get_encoder_port(encoder);
 
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
 		uint32_t dpll = pipe_config->ddi_pll_sel;
@@ -1616,7 +1598,7 @@  static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
 	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
-	enum port port = intel_ddi_get_encoder_port(intel_encoder);
+	enum port port = intel_get_encoder_port(intel_encoder);
 	int type = intel_encoder->type;
 
 	if (type == INTEL_OUTPUT_HDMI) {
@@ -1668,7 +1650,7 @@  static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	enum port port = intel_ddi_get_encoder_port(intel_encoder);
+	enum port port = intel_get_encoder_port(intel_encoder);
 	int type = intel_encoder->type;
 	uint32_t val;
 	bool wait = false;
@@ -1715,7 +1697,7 @@  static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	enum port port = intel_ddi_get_encoder_port(intel_encoder);
+	enum port port = intel_get_encoder_port(intel_encoder);
 	int type = intel_encoder->type;
 
 	if (type == INTEL_OUTPUT_HDMI) {
@@ -2276,7 +2258,7 @@  static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	int type = encoder->type;
-	int port = intel_ddi_get_encoder_port(encoder);
+	int port = intel_get_encoder_port(encoder);
 	int ret;
 
 	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9cbf543..44d20bd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -16277,6 +16277,24 @@  err:
 	return ret;
 }
 
+enum port intel_get_encoder_port(struct intel_encoder *encoder)
+{
+        switch (encoder->type) {
+        case INTEL_OUTPUT_DP_MST:
+                return enc_to_mst(&encoder->base)->primary->port;
+        case INTEL_OUTPUT_DP:
+        case INTEL_OUTPUT_EDP:
+        case INTEL_OUTPUT_HDMI:
+        case INTEL_OUTPUT_UNKNOWN:
+                return enc_to_dig_port(&encoder->base)->port;
+        case INTEL_OUTPUT_ANALOG:
+                return intel_ddi_get_analog_port();
+        default:
+                MISSING_CASE(encoder->type);
+                return PORT_A;
+        }
+}
+
 void intel_connector_unregister(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b1fc67e..19aab7b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1111,7 +1111,12 @@  void intel_ddi_clk_select(struct intel_encoder *encoder,
 void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder);
 void hsw_fdi_link_train(struct drm_crtc *crtc);
 void intel_ddi_init(struct drm_device *dev, enum port port);
-enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);
+
+static inline enum port intel_ddi_get_analog_port(void)
+{
+	return PORT_A;
+}
+
 bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
 void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc);
 void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
@@ -1189,6 +1194,7 @@  intel_crtc_has_dp_encoder(const struct intel_crtc_state *crtc_state)
 		 (1 << INTEL_OUTPUT_DP_MST) |
 		 (1 << INTEL_OUTPUT_EDP));
 }
+enum port intel_get_encoder_port(struct intel_encoder *encoder);
 static inline void
 intel_wait_for_vblank(struct drm_device *dev, int pipe)
 {
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index adca262..9c0043e 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -366,7 +366,7 @@  int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
 	if (intel_encoder->type == INTEL_OUTPUT_DSI)
 		port = 0;
 	else
-		port = intel_ddi_get_encoder_port(intel_encoder);
+		port = intel_get_encoder_port(intel_encoder);
 
 	if (port == PORT_E)  {
 		port = 0;