On Wed, 05 Jul 2023, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
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@xxxxxxxxx>
> ---
> 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
