Re: [PATCH v5 2/6] drm/i915: Stop frobbing with DDI encoder->type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> >  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.

> 
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux