On Tue, Dec 08, 2015 at 07:59:45PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Eliminate the troublesome role switching DDI encoder, and just register
> a separate encoder for each role (DP and HDMI).
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Caveat about max_lanes and pre-DDI platforms still apply.
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 232 +++++++++++++++++++++++++---------
> drivers/gpu/drm/i915/intel_display.c | 18 ---
> drivers/gpu/drm/i915/intel_dp.c | 23 +---
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_hdmi.c | 3 +
> drivers/gpu/drm/i915/intel_opregion.c | 1 -
> 6 files changed, 180 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 074121efb265..5f008f0fdc13 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -318,7 +318,6 @@ static void ddi_get_encoder_port(struct intel_encoder *intel_encoder,
> case INTEL_OUTPUT_DISPLAYPORT:
> case INTEL_OUTPUT_EDP:
> case INTEL_OUTPUT_HDMI:
> - case INTEL_OUTPUT_UNKNOWN:
> *dig_port = enc_to_dig_port(encoder);
> *port = (*dig_port)->port;
> break;
> @@ -1942,19 +1941,19 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
> switch (tmp & TRANS_DDI_MODE_SELECT_MASK) {
> case TRANS_DDI_MODE_SELECT_HDMI:
> case TRANS_DDI_MODE_SELECT_DVI:
> - return (type == DRM_MODE_CONNECTOR_HDMIA);
> + return type == DRM_MODE_CONNECTOR_HDMIA;
>
> case TRANS_DDI_MODE_SELECT_DP_SST:
> - if (type == DRM_MODE_CONNECTOR_eDP)
> - return true;
> - return (type == DRM_MODE_CONNECTOR_DisplayPort);
> + return type == DRM_MODE_CONNECTOR_DisplayPort ||
> + type == DRM_MODE_CONNECTOR_eDP;
> +
> case TRANS_DDI_MODE_SELECT_DP_MST:
> /* if the transcoder is in MST state then
> * connector isn't connected */
> return false;
>
> case TRANS_DDI_MODE_SELECT_FDI:
> - return (type == DRM_MODE_CONNECTOR_VGA);
> + return type == DRM_MODE_CONNECTOR_VGA;
>
> default:
> return false;
> @@ -1981,8 +1980,23 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> return false;
>
> if (port == PORT_A) {
> + WARN_ON(encoder->type != INTEL_OUTPUT_EDP);
> +
> tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
>
> + if ((tmp & TRANS_DDI_FUNC_ENABLE) == 0)
> + goto out;
> +
> + switch (tmp & TRANS_DDI_MODE_SELECT_MASK) {
> + case TRANS_DDI_MODE_SELECT_DP_SST:
> + break;
> + default:
> + WARN(1,
> + "Bad transcoder EDP DDI mode 0x%08x for port %c\n",
> + tmp, port_name(port));
> + return false;
> + }
> +
> switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
> case TRANS_DDI_EDP_INPUT_A_ON:
> case TRANS_DDI_EDP_INPUT_A_ONOFF:
> @@ -1994,25 +2008,98 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> case TRANS_DDI_EDP_INPUT_C_ONOFF:
> *pipe = PIPE_C;
> break;
> + default:
> + WARN(1,
> + "Bad transcoder EDP input select 0x%08x for port %c\n",
> + tmp, port_name(port));
> + return false;
> }
>
> return true;
> } else {
> + int num_mst_transcoders = 0;
> + int num_sst_transcoders = 0;
> + int num_fdi_transcoders = 0;
> + int num_hdmi_transcoders = 0;
> + int num_transcoders = 0;
> + bool enabled = false;
> +
> for (i = TRANSCODER_A; i <= TRANSCODER_C; i++) {
> tmp = I915_READ(TRANS_DDI_FUNC_CTL(i));
>
> - if ((tmp & TRANS_DDI_PORT_MASK)
> - == TRANS_DDI_SELECT_PORT(port)) {
> - if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST)
> - return false;
> + if ((tmp & TRANS_DDI_FUNC_ENABLE) == 0)
> + continue;
> +
> + if ((tmp & TRANS_DDI_PORT_MASK) != TRANS_DDI_SELECT_PORT(port))
> + continue;
> +
> + if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST) {
> + num_mst_transcoders++;
> + WARN_ON(port == PORT_E);
> + continue;
> + }
> +
> +
> + switch (tmp & TRANS_DDI_MODE_SELECT_MASK) {
> + case TRANS_DDI_MODE_SELECT_DP_SST:
> + WARN_ON(port == PORT_E && INTEL_INFO(dev_priv)->gen < 9);
> +
> + num_sst_transcoders++;
> + if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> + encoder->type == INTEL_OUTPUT_EDP) {
> + enabled = true;
> + *pipe = i;
> + }
> + break;
> + case TRANS_DDI_MODE_SELECT_HDMI:
> + case TRANS_DDI_MODE_SELECT_DVI:
> + WARN_ON(port == PORT_E);
Hm, previous patches made it look like hdmi on port E is possible - at
least you added piles of checks to make sure we have at least 4 lanes
everywhere. Am I mistaken?
> +
> + num_hdmi_transcoders++;
> + if (encoder->type == INTEL_OUTPUT_HDMI) {
> + enabled = true;
> + *pipe = i;
> + }
> + break;
> +
> + case TRANS_DDI_MODE_SELECT_FDI:
> + WARN_ON(port != PORT_E || INTEL_INFO(dev_priv)->gen >= 9);
> +
> + num_fdi_transcoders++;
> + if (encoder->type == INTEL_OUTPUT_ANALOG) {
> + enabled = true;
> + *pipe = i;
> + }
> + break;
>
> - *pipe = i;
> - return true;
> + default:
> + WARN(1, "Bad transcoder %c DDI mode 0x%08x for port %c\n",
> + transcoder_name(i), tmp, port_name(port));
> + return false;
> }
> }
> +
> + num_transcoders = num_sst_transcoders +
> + num_fdi_transcoders + num_hdmi_transcoders;
> +
> + if (WARN(num_transcoders && num_mst_transcoders,
> + "MST and non-MST transcoders enabled for port %c (%d sst, %d mst, %d fdi, %d hdmi)\n",
> + port_name(port), num_sst_transcoders, num_mst_transcoders,
> + num_fdi_transcoders, num_hdmi_transcoders))
> + return false;
> +
> + if (WARN(num_transcoders > 1,
> + "Multiple transcoders enabled for port %c (%d sst, %d mst, %d fdi, %d hdmi)\n",
> + port_name(port), num_sst_transcoders, num_mst_transcoders,
> + num_fdi_transcoders, num_hdmi_transcoders))
> + return false;
> +
> + if (enabled)
> + return true;
This is too big and needs to be extracted into a static function.
Otherwise I didn't spot anything, and getting rid of OUTPUT_UNKNOWN is
really nice. I also wonder whether we should long term rework mst to use
the fake encoder as the real one. The fake encoder was just done to not
confuse the ddi encoder, but now that we have multiples of those we might
as well embrace that everywhere.
Cheers, Daniel
> }
>
> - DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port));
> +out:
> + DRM_DEBUG_KMS("No pipe for DDI port %c found\n", port_name(port));
>
> return false;
> }
> @@ -3174,8 +3261,6 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> int type = encoder->type;
> int port = intel_ddi_get_encoder_port(encoder);
>
> - WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
> -
> if (port == PORT_A)
> pipe_config->cpu_transcoder = TRANSCODER_EDP;
>
> @@ -3224,53 +3309,18 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
> return connector;
> }
>
> -void intel_ddi_init(struct drm_device *dev, enum port port)
> +static int intel_ddi_init_role(struct drm_device *dev, enum port port,
> + int encoder_type, uint32_t saved_port_bits,
> + int max_lanes)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_digital_port *intel_dig_port;
> struct intel_encoder *intel_encoder;
> struct drm_encoder *encoder;
> - bool init_hdmi, init_dp;
> - int max_lanes;
> -
> - if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) {
> - switch (port) {
> - case PORT_A:
> - max_lanes = 4;
> - break;
> - case PORT_E:
> - max_lanes = 0;
> - break;
> - default:
> - max_lanes = 4;
> - break;
> - }
> - } else {
> - switch (port) {
> - case PORT_A:
> - max_lanes = 2;
> - break;
> - case PORT_E:
> - max_lanes = 2;
> - break;
> - default:
> - max_lanes = 4;
> - break;
> - }
> - }
> -
> - init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi ||
> - dev_priv->vbt.ddi_port_info[port].supports_hdmi);
> - init_dp = dev_priv->vbt.ddi_port_info[port].supports_dp;
> - if (!init_dp && !init_hdmi) {
> - DRM_DEBUG_KMS("VBT says port %c is not DVI/HDMI/DP compatible, respect it\n",
> - port_name(port));
> - return;
> - }
>
> intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
> if (!intel_dig_port)
> - return;
> + return -ENOMEM;
>
> intel_encoder = &intel_dig_port->base;
> encoder = &intel_encoder->base;
> @@ -3287,9 +3337,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
> intel_encoder->get_config = intel_ddi_get_config;
>
> intel_dig_port->port = port;
> - intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
> - (DDI_BUF_PORT_REVERSAL |
> - DDI_A_4_LANES);
> + intel_dig_port->saved_port_bits = saved_port_bits;
> intel_dig_port->max_lanes = max_lanes;
>
> /*
> @@ -3306,11 +3354,11 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
> }
> }
>
> - intel_encoder->type = INTEL_OUTPUT_UNKNOWN;
> + intel_encoder->type = encoder_type;
> intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> intel_encoder->cloneable = 0;
>
> - if (init_dp) {
> + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
> if (!intel_ddi_init_dp_connector(intel_dig_port))
> goto err;
>
> @@ -3327,14 +3375,74 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>
> /* In theory we don't need the encoder->type check, but leave it just in
> * case we have some really bad VBTs... */
> - if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
> + if (intel_encoder->type == INTEL_OUTPUT_HDMI) {
> if (!intel_ddi_init_hdmi_connector(intel_dig_port))
> goto err;
> }
>
> - return;
> + return intel_encoder->type;
>
> err:
> drm_encoder_cleanup(encoder);
> kfree(intel_dig_port);
> +
> + return -ENODEV;
> +}
> +
> +void intel_ddi_init(struct drm_device *dev, enum port port)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + uint32_t saved_port_bits;
> + bool init_hdmi, init_dp;
> + int max_lanes;
> +
> + if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) {
> + switch (port) {
> + case PORT_A:
> + max_lanes = 4;
> + break;
> + case PORT_E:
> + max_lanes = 0;
> + break;
> + default:
> + max_lanes = 4;
> + break;
> + }
> + } else {
> + switch (port) {
> + case PORT_A:
> + max_lanes = 2;
> + break;
> + case PORT_E:
> + max_lanes = 2;
> + break;
> + default:
> + max_lanes = 4;
> + break;
> + }
> + }
> +
> + init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi ||
> + dev_priv->vbt.ddi_port_info[port].supports_hdmi);
> + init_dp = dev_priv->vbt.ddi_port_info[port].supports_dp;
> + if (!init_dp && !init_hdmi) {
> + DRM_DEBUG_KMS("VBT says port %c is not DVI/HDMI/DP compatible, respect it\n",
> + port_name(port));
> + return;
> + }
> +
> + saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
> + (DDI_BUF_PORT_REVERSAL | DDI_A_4_LANES);
> +
> + if (init_dp) {
> + int ret = intel_ddi_init_role(dev, port, INTEL_OUTPUT_DISPLAYPORT,
> + saved_port_bits, max_lanes);
> + /* Don't register the HDMI connector/encoder when we have eDP */
> + if (ret == INTEL_OUTPUT_EDP)
> + init_hdmi = false;
> + }
> +
> + if (init_hdmi)
> + intel_ddi_init_role(dev, port, INTEL_OUTPUT_HDMI,
> + saved_port_bits, max_lanes);
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bc7aaa3c431e..fc1d7387eb12 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5252,13 +5252,9 @@ static enum intel_display_power_domain port_to_aux_power_domain(enum port port)
> enum intel_display_power_domain
> intel_display_port_power_domain(struct intel_encoder *intel_encoder)
> {
> - struct drm_device *dev = intel_encoder->base.dev;
> struct intel_digital_port *intel_dig_port;
>
> switch (intel_encoder->type) {
> - case INTEL_OUTPUT_UNKNOWN:
> - /* Only DDI platforms should ever use this output type */
> - WARN_ON_ONCE(!HAS_DDI(dev));
> case INTEL_OUTPUT_DISPLAYPORT:
> case INTEL_OUTPUT_HDMI:
> case INTEL_OUTPUT_EDP:
> @@ -5279,20 +5275,9 @@ intel_display_port_power_domain(struct intel_encoder *intel_encoder)
> enum intel_display_power_domain
> intel_display_port_aux_power_domain(struct intel_encoder *intel_encoder)
> {
> - struct drm_device *dev = intel_encoder->base.dev;
> struct intel_digital_port *intel_dig_port;
>
> switch (intel_encoder->type) {
> - case INTEL_OUTPUT_UNKNOWN:
> - case INTEL_OUTPUT_HDMI:
> - /*
> - * Only DDI platforms should ever use these output types.
> - * We can get here after the HDMI detect code has already set
> - * the type of the shared encoder. Since we can't be sure
> - * what's the status of the given connectors, play safe and
> - * run the DP detection too.
> - */
> - WARN_ON_ONCE(!HAS_DDI(dev));
> case INTEL_OUTPUT_DISPLAYPORT:
> case INTEL_OUTPUT_EDP:
> intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> @@ -12283,9 +12268,6 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
>
> switch (encoder->type) {
> unsigned int port_mask;
> - case INTEL_OUTPUT_UNKNOWN:
> - if (WARN_ON(!HAS_DDI(dev)))
> - break;
> case INTEL_OUTPUT_DISPLAYPORT:
> case INTEL_OUTPUT_HDMI:
> case INTEL_OUTPUT_EDP:
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7d354b1e5e5f..1d31aa296aaa 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4601,8 +4601,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>
> if (intel_dp->is_mst) {
> /* MST devices are disconnected from a monitor POV */
> - if (intel_encoder->type != INTEL_OUTPUT_EDP)
> - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> return connector_status_disconnected;
> }
>
> @@ -4632,8 +4630,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> if (ret) {
> /* if we are in MST mode then this connector
> won't appear connected or have anything with EDID on it */
> - if (intel_encoder->type != INTEL_OUTPUT_EDP)
> - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> status = connector_status_disconnected;
> goto out;
> }
> @@ -4648,8 +4644,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>
> intel_dp_set_edid(intel_dp);
>
> - if (intel_encoder->type != INTEL_OUTPUT_EDP)
> - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> status = connector_status_connected;
>
> /* Try to read the source of the interrupt */
> @@ -4692,9 +4686,6 @@ intel_dp_force(struct drm_connector *connector)
> intel_dp_set_edid(intel_dp);
>
> intel_display_power_put(dev_priv, power_domain);
> -
> - if (intel_encoder->type != INTEL_OUTPUT_EDP)
> - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> }
>
> static int intel_dp_get_modes(struct drm_connector *connector)
> @@ -4969,9 +4960,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> enum intel_display_power_domain power_domain;
> enum irqreturn ret = IRQ_NONE;
>
> - if (intel_dig_port->base.type != INTEL_OUTPUT_EDP &&
> - intel_dig_port->base.type != INTEL_OUTPUT_HDMI)
> - intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
> + if (WARN_ON_ONCE(intel_dig_port->base.type != INTEL_OUTPUT_EDP &&
> + intel_dig_port->base.type != INTEL_OUTPUT_DISPLAYPORT))
> + return IRQ_HANDLED;
>
> if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> /*
> @@ -5815,6 +5806,9 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> enum port port = intel_dig_port->port;
> int type, ret;
>
> + if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT))
> + return false;
> +
> if (WARN(intel_dig_port->max_lanes < 1,
> "Not enough lanes (%d) for DP on port %c\n",
> intel_dig_port->max_lanes, port_name(port)))
> @@ -5851,11 +5845,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> else
> type = DRM_MODE_CONNECTOR_DisplayPort;
>
> - /*
> - * For eDP we always set the encoder type to INTEL_OUTPUT_EDP, but
> - * for DP the encoder type can be set by the caller to
> - * INTEL_OUTPUT_UNKNOWN for DDI, so don't rewrite it.
> - */
> if (type == DRM_MODE_CONNECTOR_eDP)
> intel_encoder->type = INTEL_OUTPUT_EDP;
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a8a84b8c2bac..9e5db3d71e12 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -103,8 +103,7 @@ enum intel_output_type {
> INTEL_OUTPUT_DISPLAYPORT = 7,
> INTEL_OUTPUT_EDP = 8,
> INTEL_OUTPUT_DSI = 9,
> - INTEL_OUTPUT_UNKNOWN = 10,
> - INTEL_OUTPUT_DP_MST = 11,
> + INTEL_OUTPUT_DP_MST = 10,
> };
>
> #define INTEL_DVO_CHIP_NONE 0
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 895189abfd56..75ea9515a9ce 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -2034,6 +2034,9 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> enum port port = intel_dig_port->port;
> uint8_t alternate_ddc_pin;
>
> + if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI))
> + return;
> +
> if (WARN(intel_dig_port->max_lanes < 4,
> "Not enough lanes (%d) for HDMI on port %c\n",
> intel_dig_port->max_lanes, port_name(port)))
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index e362a30776fa..a15459a451c2 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -360,7 +360,6 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> case INTEL_OUTPUT_ANALOG:
> type = DISPLAY_TYPE_CRT;
> break;
> - case INTEL_OUTPUT_UNKNOWN:
> case INTEL_OUTPUT_DISPLAYPORT:
> case INTEL_OUTPUT_HDMI:
> case INTEL_OUTPUT_DP_MST:
> --
> 2.4.10
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
