[Intel-gfx] [PATCH v5 2/6] drm/i915: Stop frobbing with DDI encoder->type
Ville Syrjälä
ville.syrjala at linux.intel.com
Mon Oct 30 18:28:17 UTC 2017
On Mon, Oct 30, 2017 at 05:29:13PM +0100, Maarten Lankhorst wrote:
> Op 30-10-17 om 17:07 schreef Ville Syrjälä:
> > On Mon, Oct 30, 2017 at 09:59:29AM +0100, Maarten Lankhorst wrote:
> >> Op 27-10-17 om 21:31 schreef Ville Syrjala:
> >>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>>
> >>> Currently the DDI encoder->type will change at runtime depending on
> >>> what kind of hotplugs we've processed. That's quite bad since we can't
> >>> really trust that that current value of encoder->type actually matches
> >>> the type of signal we're trying to drive through it.
> >>>
> >>> Let's eliminate that problem by declaring that non-eDP DDI port will
> >>> always have the encoder type as INTEL_OUTPUT_DDI. This means the code
> >>> can no longer try to distinguish DP vs. HDMI based on encoder->type.
> >>> We'll leave eDP as INTEL_OUTPUT_EDP, since it'll never change and
> >>> there's a bunch of code that relies on that value to identify eDP
> >>> encoders.
> >>>
> >>> We'll introduce a new encoder .compute_output_type() hook. This allows
> >>> us to compute the full output_types before any encoder .compute_config()
> >>> hooks get called, thus those hooks can rely on output_types being
> >>> correct, which is useful for cloning on oldr platforms. For now we'll
> >>> just look at the connector type and pick the correct mode based on that.
> >>> In the future the new hook could be used to implement dynamic switching
> >>> between LS and PCON modes for LSPCON.
> >>>
> >>> v2: Fix BXT/GLK PPS explosion with DSI/MST encoders
> >>> v3: Avoid the PPS warn on pure HDMI/DVI DDI encoders by checking dp.output_reg
> >>> v4: Rebase
> >>> v5: Populate output_types in .get_config() rather than in the caller
> >>> v5: Split out populating output_types in .get_config() (Maarten)
> >>>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>> ---
> >>> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> >>> drivers/gpu/drm/i915/intel_ddi.c | 32 ++++++++++++++++++++++++--------
> >>> drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
> >>> drivers/gpu/drm/i915/intel_dp.c | 19 ++++++-------------
> >>> drivers/gpu/drm/i915/intel_drv.h | 7 +++++--
> >>> drivers/gpu/drm/i915/intel_hdmi.c | 10 ++--------
> >>> drivers/gpu/drm/i915/intel_opregion.c | 2 +-
> >>> 7 files changed, 47 insertions(+), 36 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >>> index c1e5bba91bff..39883cd915db 100644
> >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >>> @@ -3051,7 +3051,7 @@ static void intel_connector_info(struct seq_file *m,
> >>> break;
> >>> case DRM_MODE_CONNECTOR_HDMIA:
> >>> if (intel_encoder->type == INTEL_OUTPUT_HDMI ||
> >>> - intel_encoder->type == INTEL_OUTPUT_UNKNOWN)
> >>> + intel_encoder->type == INTEL_OUTPUT_DDI)
> >>> intel_hdmi_info(m, intel_connector);
> >>> break;
> >>> default:
> >>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>> index 7e0b1a02912a..e5dd281781fe 100644
> >>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>> @@ -497,10 +497,8 @@ 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:
> >>> + case INTEL_OUTPUT_DDI:
> >>> return enc_to_dig_port(&encoder->base)->port;
> >>> case INTEL_OUTPUT_ANALOG:
> >>> return PORT_E;
> >>> @@ -1534,6 +1532,7 @@ void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state *crtc_state,
> >>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >>> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >>> uint32_t temp;
> >>> +
> >>> temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> >>> if (state == true)
> >>> temp |= TRANS_DDI_DP_VC_PAYLOAD_ALLOC;
> >>> @@ -2652,21 +2651,36 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> >>> intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
> >>> }
> >>>
> >>> +static enum intel_output_type
> >>> +intel_ddi_compute_output_type(struct intel_encoder *encoder,
> >>> + struct intel_crtc_state *crtc_state,
> >>> + struct drm_connector_state *conn_state)
> >>> +{
> >>> + switch (conn_state->connector->connector_type) {
> >>> + case DRM_MODE_CONNECTOR_HDMIA:
> >>> + return INTEL_OUTPUT_HDMI;
> >>> + case DRM_MODE_CONNECTOR_eDP:
> >>> + return INTEL_OUTPUT_EDP;
> >>> + case DRM_MODE_CONNECTOR_DisplayPort:
> >>> + return INTEL_OUTPUT_DP;
> >>> + default:
> >>> + MISSING_CASE(conn_state->connector->connector_type);
> >>> + return INTEL_OUTPUT_UNUSED;
> >>> + }
> >>> +}
> >>> +
> >>> static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> >>> struct intel_crtc_state *pipe_config,
> >>> struct drm_connector_state *conn_state)
> >>> {
> >>> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >>> - int type = encoder->type;
> >>> int port = intel_ddi_get_encoder_port(encoder);
> >>> int ret;
> >>>
> >>> - WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
> >>> -
> >>> if (port == PORT_A)
> >>> pipe_config->cpu_transcoder = TRANSCODER_EDP;
> >>>
> >>> - if (type == INTEL_OUTPUT_HDMI)
> >>> + if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
> >>> ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
> >>> else
> >>> ret = intel_dp_compute_config(encoder, pipe_config, conn_state);
> >> Would it be an option to put it in compute_config for DDI only? There is no risk of cloning there, saves another hook. :)
> > That would mean the caller would set 'output_types|=DDI' which we'd have
> > to undo here, or we would need to special case DDI in the caller and not
> > set output_types there at all. Neither would look very pretty I think.
> Something like:
>
> /* There is no 1 to 1 mapping for DDI, it's set in compute_config()
> if (type != DDI)
> output_types |= BIT(type)
>
> would work for me. :)
Hmm. I guess that wouldn't be too bad. Though I don't particularly
enjoy encoder specific information leaking out.
But I've just gone and pushed the entire series as is, just to avoid
dragging this series along any longer. If someone feels strongly that
the hook shouldn't be there they can send a patch to kill it.
Thanks for the review.
>
> >> Either way is fine though.
> >>> @@ -2815,6 +2829,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> >>> drm_encoder_init(&dev_priv->drm, encoder, &intel_ddi_funcs,
> >>> DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
> >>>
> >>> + intel_encoder->compute_output_type = intel_ddi_compute_output_type;
> >>> intel_encoder->compute_config = intel_ddi_compute_config;
> >>> intel_encoder->enable = intel_enable_ddi;
> >>> if (IS_GEN9_LP(dev_priv))
> >>> @@ -2868,9 +2883,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> >>> max_lanes = 4;
> >>> }
> >>>
> >>> + intel_dig_port->dp.output_reg = INVALID_MMIO_REG;
> >>> intel_dig_port->max_lanes = max_lanes;
> >>>
> >>> - intel_encoder->type = INTEL_OUTPUT_UNKNOWN;
> >>> + intel_encoder->type = INTEL_OUTPUT_DDI;
> >>> intel_encoder->power_domain = intel_port_to_power_domain(port);
> >>> intel_encoder->port = port;
> >>> intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index 8f769e9b9342..737de251d0f8 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -10579,7 +10579,7 @@ static const char * const output_type_str[] = {
> >>> OUTPUT_TYPE(DP),
> >>> OUTPUT_TYPE(EDP),
> >>> OUTPUT_TYPE(DSI),
> >>> - OUTPUT_TYPE(UNKNOWN),
> >>> + OUTPUT_TYPE(DDI),
> >>> OUTPUT_TYPE(DP_MST),
> >>> };
> >>>
> >>> @@ -10750,7 +10750,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
> >>>
> >>> switch (encoder->type) {
> >>> unsigned int port_mask;
> >>> - case INTEL_OUTPUT_UNKNOWN:
> >>> + case INTEL_OUTPUT_DDI:
> >>> if (WARN_ON(!HAS_DDI(to_i915(dev))))
> >>> break;
> >>> case INTEL_OUTPUT_DP:
> >>> @@ -10883,7 +10883,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >>> * Determine output_types before calling the .compute_config()
> >>> * hooks so that the hooks can use this information safely.
> >>> */
> >>> - pipe_config->output_types |= 1 << encoder->type;
> >>> + if (encoder->compute_output_type)
> >>> + pipe_config->output_types |=
> >>> + BIT(encoder->compute_output_type(encoder, pipe_config,
> >>> + connector_state));
> >>> + else
> >>> + pipe_config->output_types |= BIT(encoder->type);
> >>> }
> >>>
> >>> encoder_retry:
> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>> index 30688a5d680d..f0c49962ffbe 100644
> >>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>> @@ -758,11 +758,16 @@ void intel_power_sequencer_reset(struct drm_i915_private *dev_priv)
> >>> struct intel_dp *intel_dp;
> >>>
> >>> if (encoder->type != INTEL_OUTPUT_DP &&
> >>> - encoder->type != INTEL_OUTPUT_EDP)
> >>> + encoder->type != INTEL_OUTPUT_EDP &&
> >>> + encoder->type != INTEL_OUTPUT_DDI)
> >>> continue;
> >>>
> >>> intel_dp = enc_to_intel_dp(&encoder->base);
> >>>
> >>> + /* Skip pure DVI/HDMI DDI encoders */
> >>> + if (!i915_mmio_reg_valid(intel_dp->output_reg))
> >>> + continue;
> >>> +
> >>> WARN_ON(intel_dp->active_pipe != INVALID_PIPE);
> >>>
> >>> if (encoder->type != INTEL_OUTPUT_EDP)
> >>> @@ -4733,8 +4738,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >>> {
> >>> struct drm_connector *connector = &intel_connector->base;
> >>> struct intel_dp *intel_dp = intel_attached_dp(connector);
> >>> - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >>> - struct intel_encoder *intel_encoder = &intel_dig_port->base;
> >>> struct drm_device *dev = connector->dev;
> >>> enum drm_connector_status status;
> >>> u8 sink_irq_vector = 0;
> >>> @@ -4767,9 +4770,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >>> goto out;
> >>> }
> >>>
> >>> - if (intel_encoder->type != INTEL_OUTPUT_EDP)
> >>> - intel_encoder->type = INTEL_OUTPUT_DP;
> >>> -
> >>> if (intel_dp->reset_link_params) {
> >>> /* Initial max link lane count */
> >>> intel_dp->max_link_lane_count = intel_dp_max_common_lane_count(intel_dp);
> >>> @@ -4886,9 +4886,6 @@ intel_dp_force(struct drm_connector *connector)
> >>> intel_dp_set_edid(intel_dp);
> >>>
> >>> intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> >>> -
> >>> - if (intel_encoder->type != INTEL_OUTPUT_EDP)
> >>> - intel_encoder->type = INTEL_OUTPUT_DP;
> >>> }
> >>>
> >>> static int intel_dp_get_modes(struct drm_connector *connector)
> >>> @@ -5107,10 +5104,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >>> struct drm_i915_private *dev_priv = to_i915(dev);
> >>> 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_DP;
> >>> -
> >>> if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> >>> /*
> >>> * vdd off can generate a long pulse on eDP which
> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>> index df966427232c..629ebc73bb09 100644
> >>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>> @@ -173,7 +173,7 @@ enum intel_output_type {
> >>> INTEL_OUTPUT_DP = 7,
> >>> INTEL_OUTPUT_EDP = 8,
> >>> INTEL_OUTPUT_DSI = 9,
> >>> - INTEL_OUTPUT_UNKNOWN = 10,
> >>> + INTEL_OUTPUT_DDI = 10,
> >>> INTEL_OUTPUT_DP_MST = 11,
> >>> };
> >>>
> >>> @@ -216,6 +216,9 @@ struct intel_encoder {
> >>> enum port port;
> >>> unsigned int cloneable;
> >>> void (*hot_plug)(struct intel_encoder *);
> >>> + enum intel_output_type (*compute_output_type)(struct intel_encoder *,
> >>> + struct intel_crtc_state *,
> >>> + struct drm_connector_state *);
> >>> bool (*compute_config)(struct intel_encoder *,
> >>> struct intel_crtc_state *,
> >>> struct drm_connector_state *);
> >>> @@ -1152,7 +1155,7 @@ enc_to_dig_port(struct drm_encoder *encoder)
> >>> struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
> >>>
> >>> switch (intel_encoder->type) {
> >>> - case INTEL_OUTPUT_UNKNOWN:
> >>> + case INTEL_OUTPUT_DDI:
> >>> WARN_ON(!HAS_DDI(to_i915(encoder->dev)));
> >>> case INTEL_OUTPUT_DP:
> >>> case INTEL_OUTPUT_EDP:
> >>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> index 97d08cda3f8e..dede2898cb8a 100644
> >>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> @@ -1617,12 +1617,9 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> >>>
> >>> intel_hdmi_unset_edid(connector);
> >>>
> >>> - if (intel_hdmi_set_edid(connector)) {
> >>> - struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> >>> -
> >>> - hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
> >>> + if (intel_hdmi_set_edid(connector))
> >>> status = connector_status_connected;
> >>> - } else
> >>> + else
> >>> status = connector_status_disconnected;
> >>>
> >>> intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> >>> @@ -1633,8 +1630,6 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> >>> static void
> >>> intel_hdmi_force(struct drm_connector *connector)
> >>> {
> >>> - struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> >>> -
> >>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> >>> connector->base.id, connector->name);
> >>>
> >>> @@ -1644,7 +1639,6 @@ intel_hdmi_force(struct drm_connector *connector)
> >>> return;
> >>>
> >>> intel_hdmi_set_edid(connector);
> >>> - hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
> >>> }
> >>>
> >>> static int intel_hdmi_get_modes(struct drm_connector *connector)
> >>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> >>> index 1d946240e55f..39714be3eb6b 100644
> >>> --- a/drivers/gpu/drm/i915/intel_opregion.c
> >>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> >>> @@ -383,7 +383,7 @@ 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_DDI:
> >>> case INTEL_OUTPUT_DP:
> >>> case INTEL_OUTPUT_HDMI:
> >>> case INTEL_OUTPUT_DP_MST:
>
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list