Re: [Intel-gfx] [PATCH 12/13] drm/i915/sdvo: Rework DDC bus handling

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 05 Jul 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Each SDVO device can have up to three sets of DDC pins.
> Currently we just register a single i2c_adapter for the
> entire SDVO device and semi-randomly pick the "correct"
> set of DDC pins during intel_sdvo_tmds_sink_detect().
> This doesn't make any real sense especially if we have
> multiple outputs each with their own dedicated DDC bus.
>
> Let's clean up this mess and register a dedicated
> i2c_adapter for each of the possible pin pairs. Each
> output (ie. connector) can then pick the correct i2c_adapter
> to use for its DDC bus. And we can just switch over to
> drm_connector_init_with_ddc() to take care of the
> connector->ddc association, which also populates the
> "ddc" sysfs symlink as a bonus.
>
> And now that things are based on the actual connector we can
> also nuke the sketchy sdvo->controller_output thing.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Okay, so I'm not going to claim I considered all the possible aspects of
the changes here, but I did not spot anything wrong with it
either. Maybe there's a bit much shoved in one patch, but not sure if it
would be feasible or productive to chop it up. The direction looks good.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_sdvo.c | 205 ++++++++++++----------
>  1 file changed, 114 insertions(+), 91 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 5c25417d256a..d7edb5714c4c 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -65,6 +65,8 @@
>  #define IS_TV_OR_LVDS(c)	((c)->output_flag & (SDVO_TV_MASK | SDVO_LVDS_MASK))
>  #define IS_DIGITAL(c)		((c)->output_flag & (SDVO_TMDS_MASK | SDVO_LVDS_MASK))
>  
> +#define HAS_DDC(c)		((c)->output_flag & (SDVO_RGB_MASK | SDVO_TMDS_MASK | \
> +						     SDVO_LVDS_MASK))
>  
>  static const char * const tv_format_names[] = {
>  	"NTSC_M"   , "NTSC_J"  , "NTSC_443",
> @@ -78,20 +80,25 @@ static const char * const tv_format_names[] = {
>  
>  #define TV_FORMAT_NUM  ARRAY_SIZE(tv_format_names)
>  
> +struct intel_sdvo;
> +
> +struct intel_sdvo_ddc {
> +	struct i2c_adapter ddc;
> +	struct intel_sdvo *sdvo;
> +	u8 ddc_bus;
> +};
> +
>  struct intel_sdvo {
>  	struct intel_encoder base;
>  
>  	struct i2c_adapter *i2c;
>  	u8 slave_addr;
>  
> -	struct i2c_adapter ddc;
> +	struct intel_sdvo_ddc ddc[3];
>  
>  	/* Register for the SDVO device: SDVOB or SDVOC */
>  	i915_reg_t sdvo_reg;
>  
> -	/* Active outputs controlled by this SDVO output */
> -	u16 controlled_output;
> -
>  	/*
>  	 * Capabilities of the SDVO device returned by
>  	 * intel_sdvo_get_capabilities()
> @@ -108,9 +115,6 @@ struct intel_sdvo {
>  	 */
>  	u16 hotplug_active;
>  
> -	/* DDC bus used by this SDVO encoder */
> -	u8 ddc_bus;
> -
>  	/*
>  	 * the sdvo flag gets lost in round trip: dtd->adjusted_mode->dtd
>  	 */
> @@ -2035,18 +2039,15 @@ intel_sdvo_hotplug(struct intel_encoder *encoder,
>  	return intel_encoder_hotplug(encoder, connector);
>  }
>  
> -static bool
> -intel_sdvo_multifunc_encoder(struct intel_sdvo *intel_sdvo)
> -{
> -	/* Is there more than one type of output? */
> -	return hweight16(intel_sdvo->caps.output_flags) > 1;
> -}
> -
>  static const struct drm_edid *
>  intel_sdvo_get_edid(struct drm_connector *connector)
>  {
> -	struct intel_sdvo *sdvo = intel_attached_sdvo(to_intel_connector(connector));
> -	return drm_edid_read_ddc(connector, &sdvo->ddc);
> +	struct i2c_adapter *ddc = connector->ddc;
> +
> +	if (!ddc)
> +		return NULL;
> +
> +	return drm_edid_read_ddc(connector, ddc);
>  }
>  
>  /* Mac mini hack -- use the same DDC as the analog connector */
> @@ -2054,43 +2055,23 @@ static const struct drm_edid *
>  intel_sdvo_get_analog_edid(struct drm_connector *connector)
>  {
>  	struct drm_i915_private *i915 = to_i915(connector->dev);
> -	struct i2c_adapter *i2c;
> +	struct i2c_adapter *ddc;
>  
> -	i2c = intel_gmbus_get_adapter(i915, i915->display.vbt.crt_ddc_pin);
> +	ddc = intel_gmbus_get_adapter(i915, i915->display.vbt.crt_ddc_pin);
> +	if (!ddc)
> +		return NULL;
>  
> -	return drm_edid_read_ddc(connector, i2c);
> +	return drm_edid_read_ddc(connector, ddc);
>  }
>  
>  static enum drm_connector_status
>  intel_sdvo_tmds_sink_detect(struct drm_connector *connector)
>  {
> -	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(to_intel_connector(connector));
>  	enum drm_connector_status status;
>  	const struct drm_edid *drm_edid;
>  
>  	drm_edid = intel_sdvo_get_edid(connector);
>  
> -	if (!drm_edid && intel_sdvo_multifunc_encoder(intel_sdvo)) {
> -		u8 ddc, saved_ddc = intel_sdvo->ddc_bus;
> -
> -		/*
> -		 * Don't use the 1 as the argument of DDC bus switch to get
> -		 * the EDID. It is used for SDVO SPD ROM.
> -		 */
> -		for (ddc = intel_sdvo->ddc_bus >> 1; ddc > 1; ddc >>= 1) {
> -			intel_sdvo->ddc_bus = ddc;
> -			drm_edid = intel_sdvo_get_edid(connector);
> -			if (drm_edid)
> -				break;
> -		}
> -		/*
> -		 * If we found the EDID on the other bus,
> -		 * assume that is the correct DDC bus.
> -		 */
> -		if (!drm_edid)
> -			intel_sdvo->ddc_bus = saved_ddc;
> -	}
> -
>  	/*
>  	 * When there is no edid and no monitor is connected with VGA
>  	 * port, try to use the CRT ddc to read the EDID for DVI-connector.
> @@ -2524,29 +2505,37 @@ static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs
>  	.atomic_check = intel_sdvo_atomic_check,
>  };
>  
> -static void intel_sdvo_enc_destroy(struct drm_encoder *encoder)
> +static void intel_sdvo_encoder_destroy(struct drm_encoder *_encoder)
>  {
> -	struct intel_sdvo *intel_sdvo = to_sdvo(to_intel_encoder(encoder));
> +	struct intel_encoder *encoder = to_intel_encoder(_encoder);
> +	struct intel_sdvo *sdvo = to_sdvo(encoder);
> +	int i;
>  
> -	i2c_del_adapter(&intel_sdvo->ddc);
> -	intel_encoder_destroy(encoder);
> -}
> +	for (i = 0; i < ARRAY_SIZE(sdvo->ddc); i++) {
> +		if (sdvo->ddc[i].ddc_bus)
> +			i2c_del_adapter(&sdvo->ddc[i].ddc);
> +	}
> +
> +	drm_encoder_cleanup(&encoder->base);
> +	kfree(sdvo);
> +};
>  
>  static const struct drm_encoder_funcs intel_sdvo_enc_funcs = {
> -	.destroy = intel_sdvo_enc_destroy,
> +	.destroy = intel_sdvo_encoder_destroy,
>  };
>  
> -static void
> -intel_sdvo_guess_ddc_bus(struct intel_sdvo *sdvo)
> +static int
> +intel_sdvo_guess_ddc_bus(struct intel_sdvo *sdvo,
> +			 struct intel_sdvo_connector *connector)
>  {
>  	u16 mask = 0;
> -	unsigned int num_bits;
> +	int num_bits;
>  
>  	/*
>  	 * Make a mask of outputs less than or equal to our own priority in the
>  	 * list.
>  	 */
> -	switch (sdvo->controlled_output) {
> +	switch (connector->output_flag) {
>  	case SDVO_OUTPUT_LVDS1:
>  		mask |= SDVO_OUTPUT_LVDS1;
>  		fallthrough;
> @@ -2575,7 +2564,7 @@ intel_sdvo_guess_ddc_bus(struct intel_sdvo *sdvo)
>  		num_bits = 3;
>  
>  	/* Corresponds to SDVO_CONTROL_BUS_DDCx */
> -	sdvo->ddc_bus = 1 << num_bits;
> +	return num_bits;
>  }
>  
>  /*
> @@ -2585,11 +2574,13 @@ intel_sdvo_guess_ddc_bus(struct intel_sdvo *sdvo)
>   * DDC bus number assignment is in a priority order of RGB outputs, then TMDS
>   * outputs, then LVDS outputs.
>   */
> -static void
> -intel_sdvo_select_ddc_bus(struct intel_sdvo *sdvo)
> +static struct intel_sdvo_ddc *
> +intel_sdvo_select_ddc_bus(struct intel_sdvo *sdvo,
> +			  struct intel_sdvo_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(sdvo->base.base.dev);
>  	struct sdvo_device_mapping *mapping;
> +	int ddc_bus;
>  
>  	if (sdvo->base.port == PORT_B)
>  		mapping = &dev_priv->display.vbt.sdvo_mappings[0];
> @@ -2597,9 +2588,14 @@ intel_sdvo_select_ddc_bus(struct intel_sdvo *sdvo)
>  		mapping = &dev_priv->display.vbt.sdvo_mappings[1];
>  
>  	if (mapping->initialized)
> -		sdvo->ddc_bus = 1 << ((mapping->ddc_pin & 0xf0) >> 4);
> +		ddc_bus = (mapping->ddc_pin & 0xf0) >> 4;
>  	else
> -		intel_sdvo_guess_ddc_bus(sdvo);
> +		ddc_bus = intel_sdvo_guess_ddc_bus(sdvo, connector);
> +
> +	if (ddc_bus < 1 || ddc_bus > 3)
> +		return NULL;
> +
> +	return &sdvo->ddc[ddc_bus - 1];
>  }
>  
>  static void
> @@ -2682,22 +2678,30 @@ intel_sdvo_get_slave_addr(struct intel_sdvo *sdvo)
>  		return 0x72;
>  }
>  
> +static int
> +intel_sdvo_init_ddc_proxy(struct intel_sdvo_ddc *ddc,
> +			  struct intel_sdvo *sdvo, int bit);
> +
>  static int
>  intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
>  			  struct intel_sdvo *encoder)
>  {
> -	struct drm_connector *drm_connector;
> +	struct drm_i915_private *i915 = to_i915(encoder->base.base.dev);
> +	struct intel_sdvo_ddc *ddc = NULL;
>  	int ret;
>  
> -	drm_connector = &connector->base.base;
> -	ret = drm_connector_init(encoder->base.base.dev,
> -			   drm_connector,
> -			   &intel_sdvo_connector_funcs,
> -			   connector->base.base.connector_type);
> +	if (HAS_DDC(connector))
> +		ddc = intel_sdvo_select_ddc_bus(encoder, connector);
> +
> +	ret = drm_connector_init_with_ddc(encoder->base.base.dev,
> +					  &connector->base.base,
> +					  &intel_sdvo_connector_funcs,
> +					  connector->base.base.connector_type,
> +					  ddc ? &ddc->ddc : NULL);
>  	if (ret < 0)
>  		return ret;
>  
> -	drm_connector_helper_add(drm_connector,
> +	drm_connector_helper_add(&connector->base.base,
>  				 &intel_sdvo_connector_helper_funcs);
>  
>  	connector->base.base.display_info.subpixel_order = SubPixelHorizontalRGB;
> @@ -2706,6 +2710,11 @@ intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
>  
>  	intel_connector_attach_encoder(&connector->base, &encoder->base);
>  
> +	if (ddc)
> +		drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] using %s\n",
> +			    connector->base.base.base.id, connector->base.base.name,
> +			    ddc->ddc.name);
> +
>  	return 0;
>  }
>  
> @@ -2903,7 +2912,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, u16 type)
>  	if (!intel_panel_preferred_fixed_mode(intel_connector)) {
>  		mutex_lock(&i915->drm.mode_config.mutex);
>  
> -		intel_ddc_get_modes(connector, &intel_sdvo->ddc);
> +		intel_ddc_get_modes(connector, connector->ddc);
>  		intel_panel_add_edid_fixed_modes(intel_connector, false);
>  
>  		mutex_unlock(&i915->drm.mode_config.mutex);
> @@ -2978,10 +2987,6 @@ intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo)
>  		return false;
>  	}
>  
> -	intel_sdvo->controlled_output = flags;
> -
> -	intel_sdvo_select_ddc_bus(intel_sdvo);
> -
>  	for (i = 0; i < ARRAY_SIZE(probe_order); i++) {
>  		u16 type = flags & probe_order[i];
>  
> @@ -3234,9 +3239,10 @@ static int intel_sdvo_ddc_proxy_xfer(struct i2c_adapter *adapter,
>  				     struct i2c_msg *msgs,
>  				     int num)
>  {
> -	struct intel_sdvo *sdvo = adapter->algo_data;
> +	struct intel_sdvo_ddc *ddc = adapter->algo_data;
> +	struct intel_sdvo *sdvo = ddc->sdvo;
>  
> -	if (!__intel_sdvo_set_control_bus_switch(sdvo, sdvo->ddc_bus))
> +	if (!__intel_sdvo_set_control_bus_switch(sdvo, 1 << ddc->ddc_bus))
>  		return -EIO;
>  
>  	return sdvo->i2c->algo->master_xfer(sdvo->i2c, msgs, num);
> @@ -3244,7 +3250,9 @@ static int intel_sdvo_ddc_proxy_xfer(struct i2c_adapter *adapter,
>  
>  static u32 intel_sdvo_ddc_proxy_func(struct i2c_adapter *adapter)
>  {
> -	struct intel_sdvo *sdvo = adapter->algo_data;
> +	struct intel_sdvo_ddc *ddc = adapter->algo_data;
> +	struct intel_sdvo *sdvo = ddc->sdvo;
> +
>  	return sdvo->i2c->algo->functionality(sdvo->i2c);
>  }
>  
> @@ -3256,21 +3264,27 @@ static const struct i2c_algorithm intel_sdvo_ddc_proxy = {
>  static void proxy_lock_bus(struct i2c_adapter *adapter,
>  			   unsigned int flags)
>  {
> -	struct intel_sdvo *sdvo = adapter->algo_data;
> +	struct intel_sdvo_ddc *ddc = adapter->algo_data;
> +	struct intel_sdvo *sdvo = ddc->sdvo;
> +
>  	sdvo->i2c->lock_ops->lock_bus(sdvo->i2c, flags);
>  }
>  
>  static int proxy_trylock_bus(struct i2c_adapter *adapter,
>  			     unsigned int flags)
>  {
> -	struct intel_sdvo *sdvo = adapter->algo_data;
> +	struct intel_sdvo_ddc *ddc = adapter->algo_data;
> +	struct intel_sdvo *sdvo = ddc->sdvo;
> +
>  	return sdvo->i2c->lock_ops->trylock_bus(sdvo->i2c, flags);
>  }
>  
>  static void proxy_unlock_bus(struct i2c_adapter *adapter,
>  			     unsigned int flags)
>  {
> -	struct intel_sdvo *sdvo = adapter->algo_data;
> +	struct intel_sdvo_ddc *ddc = adapter->algo_data;
> +	struct intel_sdvo *sdvo = ddc->sdvo;
> +
>  	sdvo->i2c->lock_ops->unlock_bus(sdvo->i2c, flags);
>  }
>  
> @@ -3280,21 +3294,26 @@ static const struct i2c_lock_operations proxy_lock_ops = {
>  	.unlock_bus =  proxy_unlock_bus,
>  };
>  
> -static bool
> -intel_sdvo_init_ddc_proxy(struct intel_sdvo *sdvo)
> +static int
> +intel_sdvo_init_ddc_proxy(struct intel_sdvo_ddc *ddc,
> +			  struct intel_sdvo *sdvo, int ddc_bus)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(sdvo->base.base.dev);
>  	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  
> -	sdvo->ddc.owner = THIS_MODULE;
> -	sdvo->ddc.class = I2C_CLASS_DDC;
> -	snprintf(sdvo->ddc.name, I2C_NAME_SIZE, "SDVO DDC proxy");
> -	sdvo->ddc.dev.parent = &pdev->dev;
> -	sdvo->ddc.algo_data = sdvo;
> -	sdvo->ddc.algo = &intel_sdvo_ddc_proxy;
> -	sdvo->ddc.lock_ops = &proxy_lock_ops;
> +	ddc->sdvo = sdvo;
> +	ddc->ddc_bus = ddc_bus;
>  
> -	return i2c_add_adapter(&sdvo->ddc) == 0;
> +	ddc->ddc.owner = THIS_MODULE;
> +	ddc->ddc.class = I2C_CLASS_DDC;
> +	snprintf(ddc->ddc.name, I2C_NAME_SIZE, "SDVO %c DDC%d",
> +		 port_name(sdvo->base.port), ddc_bus);
> +	ddc->ddc.dev.parent = &pdev->dev;
> +	ddc->ddc.algo_data = ddc;
> +	ddc->ddc.algo = &intel_sdvo_ddc_proxy;
> +	ddc->ddc.lock_ops = &proxy_lock_ops;
> +
> +	return i2c_add_adapter(&ddc->ddc);
>  }
>  
>  static bool is_sdvo_port_valid(struct drm_i915_private *dev_priv, enum port port)
> @@ -3341,9 +3360,8 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
>  
>  	intel_sdvo->sdvo_reg = sdvo_reg;
>  	intel_sdvo->slave_addr = intel_sdvo_get_slave_addr(intel_sdvo) >> 1;
> +
>  	intel_sdvo_select_i2c_bus(intel_sdvo);
> -	if (!intel_sdvo_init_ddc_proxy(intel_sdvo))
> -		goto err_i2c_bus;
>  
>  	/* Read the regs to test if we can talk to the device */
>  	for (i = 0; i < 0x40; i++) {
> @@ -3376,6 +3394,15 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
>  	intel_sdvo->colorimetry_cap =
>  		intel_sdvo_get_colorimetry_cap(intel_sdvo);
>  
> +	for (i = 0; i < ARRAY_SIZE(intel_sdvo->ddc); i++) {
> +		int ret;
> +
> +		ret = intel_sdvo_init_ddc_proxy(&intel_sdvo->ddc[i],
> +						intel_sdvo, i + 1);
> +		if (ret)
> +			goto err;
> +	}
> +
>  	if (!intel_sdvo_output_setup(intel_sdvo)) {
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "SDVO output failed to setup on %s\n",
> @@ -3436,13 +3463,9 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
>  
>  err_output:
>  	intel_sdvo_output_cleanup(intel_sdvo);
> -
>  err:
> -	drm_encoder_cleanup(&intel_encoder->base);
> -	i2c_del_adapter(&intel_sdvo->ddc);
> -err_i2c_bus:
>  	intel_sdvo_unselect_i2c_bus(intel_sdvo);
> -	kfree(intel_sdvo);
> +	intel_sdvo_encoder_destroy(&intel_encoder->base);
>  
>  	return false;
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center