Re: [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 17, 2018 at 08:34:36PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 17, 2018 at 10:16:05AM -0700, Rodrigo Vivi wrote:
> > On Mon, Sep 17, 2018 at 06:15:03PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > SDVO encoders can have multiple different types of outputs hanging off
> > > them. Currently the code tries to muck around with various is_foo
> > > flags in the encoder to figure out which type its driving. That doesn't
> > > work with atomic and other stuff, so let's nuke those flags and just
> > > look at which type of connector we're actually dealing with.
> > > 
> > > The is_hdmi we'll need as that's not discoverable via the output flags,
> > > but we'll just move it under the connector.
> > > 
> > > We'll also move the sdvo fixed mode handling out from the .get_modes()
> > > hook into the sdvo lvds init function so that we can bail out properly
> > > if there is no fixed mode to be found.
> > 
> > I wondered if this last part didn't deserved a separated patch. But not sure
> > how chicken-egg issue this could cause so maybe the same patch is better
> > indeed...
> 
> There are both chickens and eggs in there for sure. I suppose it might
> have been possible to handle it by clearing the LVDS bit from
> output_flag if we fail to find the mode. But then we'd be left with
> an LVDS connector without the flag and I'm not quite sure where the
> code would lead if we ever tried to use it. Although one can argue
> that the current code already has that same issue, maybe.

Decided to keep this patch as is, and pushed it. Thanks for the review.

> 
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_sdvo.c | 101 +++++++++++++++++---------------------
> > >  1 file changed, 44 insertions(+), 57 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > > index 812fe7b06f87..701372e512a8 100644
> > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > > @@ -99,31 +99,12 @@ struct intel_sdvo {
> > >  	 */
> > >  	uint16_t hotplug_active;
> > >  
> > > -	/**
> > > -	 * This is set if we're going to treat the device as TV-out.
> > > -	 *
> > > -	 * While we have these nice friendly flags for output types that ought
> > > -	 * to decide this for us, the S-Video output on our HDMI+S-Video card
> > > -	 * shows up as RGB1 (VGA).
> > > -	 */
> > > -	bool is_tv;
> > > -
> > >  	enum port port;
> > >  
> > > -	/**
> > > -	 * This is set if we treat the device as HDMI, instead of DVI.
> > > -	 */
> > > -	bool is_hdmi;
> > >  	bool has_hdmi_monitor;
> > >  	bool has_hdmi_audio;
> > >  	bool rgb_quant_range_selectable;
> > >  
> > > -	/**
> > > -	 * This is set if we detect output of sdvo device as LVDS and
> > > -	 * have a valid fixed mode to use with the panel.
> > > -	 */
> > > -	bool is_lvds;
> > > -
> > >  	/**
> > >  	 * This is sdvo fixed pannel mode pointer
> > >  	 */
> > > @@ -172,6 +153,11 @@ struct intel_sdvo_connector {
> > >  
> > >  	/* this is to get the range of margin.*/
> > >  	u32 max_hscan, max_vscan;
> > > +
> > > +	/**
> > > +	 * This is set if we treat the device as HDMI, instead of DVI.
> > > +	 */
> > > +	bool is_hdmi;
> > >  };
> > >  
> > >  struct intel_sdvo_connector_state {
> > > @@ -766,6 +752,7 @@ static bool intel_sdvo_get_input_timing(struct intel_sdvo *intel_sdvo,
> > >  
> > >  static bool
> > >  intel_sdvo_create_preferred_input_timing(struct intel_sdvo *intel_sdvo,
> > > +					 struct intel_sdvo_connector *intel_sdvo_connector,
> > >  					 uint16_t clock,
> > >  					 uint16_t width,
> > >  					 uint16_t height)
> > > @@ -778,7 +765,7 @@ intel_sdvo_create_preferred_input_timing(struct intel_sdvo *intel_sdvo,
> > >  	args.height = height;
> > >  	args.interlace = 0;
> > >  
> > > -	if (intel_sdvo->is_lvds &&
> > > +	if (IS_LVDS(intel_sdvo_connector) &&
> > >  	   (intel_sdvo->sdvo_lvds_fixed_mode->hdisplay != width ||
> > >  	    intel_sdvo->sdvo_lvds_fixed_mode->vdisplay != height))
> > >  		args.scaled = 1;
> > > @@ -1067,6 +1054,7 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
> > >   */
> > >  static bool
> > >  intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > > +				    struct intel_sdvo_connector *intel_sdvo_connector,
> > >  				    const struct drm_display_mode *mode,
> > >  				    struct drm_display_mode *adjusted_mode)
> > >  {
> > > @@ -1077,6 +1065,7 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > >  		return false;
> > >  
> > >  	if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
> > > +						      intel_sdvo_connector,
> > >  						      mode->clock / 10,
> > >  						      mode->hdisplay,
> > >  						      mode->vdisplay))
> > > @@ -1127,6 +1116,8 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
> > >  	struct intel_sdvo *intel_sdvo = to_sdvo(encoder);
> > >  	struct intel_sdvo_connector_state *intel_sdvo_state =
> > >  		to_intel_sdvo_connector_state(conn_state);
> > > +	struct intel_sdvo_connector *intel_sdvo_connector =
> > > +		to_intel_sdvo_connector(conn_state->connector);
> > >  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > >  	struct drm_display_mode *mode = &pipe_config->base.mode;
> > >  
> > > @@ -1142,20 +1133,22 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
> > >  	 * timings, even though this isn't really the right place in
> > >  	 * the sequence to do it. Oh well.
> > >  	 */
> > > -	if (intel_sdvo->is_tv) {
> > > +	if (IS_TV(intel_sdvo_connector)) {
> > >  		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> > >  			return false;
> > >  
> > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > +							   intel_sdvo_connector,
> > >  							   mode,
> > >  							   adjusted_mode);
> > >  		pipe_config->sdvo_tv_clock = true;
> > > -	} else if (intel_sdvo->is_lvds) {
> > > +	} else if (IS_LVDS(intel_sdvo_connector)) {
> > >  		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> > >  							     intel_sdvo->sdvo_lvds_fixed_mode))
> > >  			return false;
> > >  
> > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > +							   intel_sdvo_connector,
> > >  							   mode,
> > >  							   adjusted_mode);
> > >  	}
> > > @@ -1194,11 +1187,11 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
> > >  	}
> > >  
> > >  	/* Clock computation needs to happen after pixel multiplier. */
> > > -	if (intel_sdvo->is_tv)
> > > +	if (IS_TV(intel_sdvo_connector))
> > >  		i9xx_adjust_sdvo_tv_clock(pipe_config);
> > >  
> > >  	/* Set user selected PAR to incoming mode's member */
> > > -	if (intel_sdvo->is_hdmi)
> > > +	if (intel_sdvo_connector->is_hdmi)
> > >  		adjusted_mode->picture_aspect_ratio = conn_state->picture_aspect_ratio;
> > >  
> > >  	return true;
> > > @@ -1275,6 +1268,8 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
> > >  	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
> > >  	const struct intel_sdvo_connector_state *sdvo_state =
> > >  		to_intel_sdvo_connector_state(conn_state);
> > > +	const struct intel_sdvo_connector *intel_sdvo_connector =
> > > +		to_intel_sdvo_connector(conn_state->connector);
> > >  	const struct drm_display_mode *mode = &crtc_state->base.mode;
> > >  	struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
> > >  	u32 sdvox;
> > > @@ -1304,7 +1299,7 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
> > >  		return;
> > >  
> > >  	/* lvds has a special fixed output timing. */
> > > -	if (intel_sdvo->is_lvds)
> > > +	if (IS_LVDS(intel_sdvo_connector))
> > >  		intel_sdvo_get_dtd_from_mode(&output_dtd,
> > >  					     intel_sdvo->sdvo_lvds_fixed_mode);
> > >  	else
> > > @@ -1325,13 +1320,13 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
> > >  	} else
> > >  		intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_DVI);
> > >  
> > > -	if (intel_sdvo->is_tv &&
> > > +	if (IS_TV(intel_sdvo_connector) &&
> > >  	    !intel_sdvo_set_tv_format(intel_sdvo, conn_state))
> > >  		return;
> > >  
> > >  	intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> > >  
> > > -	if (intel_sdvo->is_tv || intel_sdvo->is_lvds)
> > > +	if (IS_TV(intel_sdvo_connector) || IS_LVDS(intel_sdvo_connector))
> > >  		input_dtd.part2.sdvo_flags = intel_sdvo->dtd_sdvo_flags;
> > >  	if (!intel_sdvo_set_input_timing(intel_sdvo, &input_dtd))
> > >  		DRM_INFO("Setting input timings on %s failed\n",
> > > @@ -1630,6 +1625,8 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
> > >  		      struct drm_display_mode *mode)
> > >  {
> > >  	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
> > > +	struct intel_sdvo_connector *intel_sdvo_connector =
> > > +		to_intel_sdvo_connector(connector);
> > >  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
> > >  
> > >  	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > > @@ -1644,7 +1641,7 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
> > >  	if (mode->clock > max_dotclk)
> > >  		return MODE_CLOCK_HIGH;
> > >  
> > > -	if (intel_sdvo->is_lvds) {
> > > +	if (IS_LVDS(intel_sdvo_connector)) {
> > >  		if (mode->hdisplay > intel_sdvo->sdvo_lvds_fixed_mode->hdisplay)
> > >  			return MODE_PANEL;
> > >  
> > > @@ -1759,6 +1756,8 @@ static enum drm_connector_status
> > >  intel_sdvo_tmds_sink_detect(struct drm_connector *connector)
> > >  {
> > >  	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
> > > +	struct intel_sdvo_connector *intel_sdvo_connector =
> > > +		to_intel_sdvo_connector(connector);
> > >  	enum drm_connector_status status;
> > >  	struct edid *edid;
> > >  
> > > @@ -1797,7 +1796,7 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector)
> > >  		/* DDC bus is shared, match EDID to connector type */
> > >  		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> > >  			status = connector_status_connected;
> > > -			if (intel_sdvo->is_hdmi) {
> > > +			if (intel_sdvo_connector->is_hdmi) {
> > >  				intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid);
> > >  				intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid);
> > >  				intel_sdvo->rgb_quant_range_selectable =
> > > @@ -1875,17 +1874,6 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
> > >  			ret = connector_status_connected;
> > >  	}
> > >  
> > > -	/* May update encoder flag for like clock for SDVO TV, etc.*/
> > > -	if (ret == connector_status_connected) {
> > > -		intel_sdvo->is_tv = false;
> > > -		intel_sdvo->is_lvds = false;
> > > -
> > > -		if (response & SDVO_TV_MASK)
> > > -			intel_sdvo->is_tv = true;
> > > -		if (response & SDVO_LVDS_MASK)
> > > -			intel_sdvo->is_lvds = intel_sdvo->sdvo_lvds_fixed_mode != NULL;
> > > -	}
> > > -
> > >  	return ret;
> > >  }
> > >  
> > > @@ -2054,16 +2042,6 @@ static void intel_sdvo_get_lvds_modes(struct drm_connector *connector)
> > >  	 * arranged in priority order.
> > >  	 */
> > >  	intel_ddc_get_modes(connector, &intel_sdvo->ddc);
> > > -
> > > -	list_for_each_entry(newmode, &connector->probed_modes, head) {
> > > -		if (newmode->type & DRM_MODE_TYPE_PREFERRED) {
> > > -			intel_sdvo->sdvo_lvds_fixed_mode =
> > > -				drm_mode_duplicate(connector->dev, newmode);
> > > -
> > > -			intel_sdvo->is_lvds = true;
> > > -			break;
> > > -		}
> > > -	}
> > >  }
> > >  
> > >  static int intel_sdvo_get_modes(struct drm_connector *connector)
> > > @@ -2555,7 +2533,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
> > >  	if (INTEL_GEN(dev_priv) >= 4 &&
> > >  	    intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
> > >  		connector->connector_type = DRM_MODE_CONNECTOR_HDMIA;
> > > -		intel_sdvo->is_hdmi = true;
> > > +		intel_sdvo_connector->is_hdmi = true;
> > >  	}
> > >  
> > >  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > @@ -2563,7 +2541,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
> > >  		return false;
> > >  	}
> > >  
> > > -	if (intel_sdvo->is_hdmi)
> > > +	if (intel_sdvo_connector->is_hdmi)
> > >  		intel_sdvo_add_hdmi_properties(intel_sdvo, intel_sdvo_connector);
> > >  
> > >  	return true;
> > > @@ -2591,8 +2569,6 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
> > >  	intel_sdvo->controlled_output |= type;
> > >  	intel_sdvo_connector->output_flag = type;
> > >  
> > > -	intel_sdvo->is_tv = true;
> > > -
> > >  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > >  		kfree(intel_sdvo_connector);
> > >  		return false;
> > > @@ -2654,6 +2630,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> > >  	struct drm_connector *connector;
> > >  	struct intel_connector *intel_connector;
> > >  	struct intel_sdvo_connector *intel_sdvo_connector;
> > > +	struct drm_display_mode *mode;
> > >  
> > >  	DRM_DEBUG_KMS("initialising LVDS device %d\n", device);
> > >  
> > > @@ -2682,6 +2659,19 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> > >  	if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector))
> > >  		goto err;
> > >  
> > > +	intel_sdvo_get_lvds_modes(connector);
> > > +
> > > +	list_for_each_entry(mode, &connector->probed_modes, head) {
> > > +		if (mode->type & DRM_MODE_TYPE_PREFERRED) {
> > > +			intel_sdvo->sdvo_lvds_fixed_mode =
> > > +				drm_mode_duplicate(connector->dev, mode);
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (!intel_sdvo->sdvo_lvds_fixed_mode)
> > > +		goto err;
> > > +
> > >  	return true;
> > >  
> > >  err:
> > > @@ -2692,9 +2682,6 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> > >  static bool
> > >  intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, uint16_t flags)
> > >  {
> > > -	intel_sdvo->is_tv = false;
> > > -	intel_sdvo->is_lvds = false;
> > > -
> > >  	/* SDVO requires XXX1 function may not exist unless it has XXX0 function.*/
> > >  
> > >  	if (flags & SDVO_OUTPUT_TMDS0)
> > > -- 
> > > 2.16.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx