Re: [PATCH v6 22/28] drm/bridge_connector: Implement hw readout for connector

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

For the series:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Would really love it if atomic_sro.[ch] is renamed to atomic_state_readout.[ch], so
we have a better idea what it's about. :-)

2 things to note:
- Hardware readout may be succesful, but the resulting state may not be valid.
  Think for example a crtc with a DP-MST connector, but the DP-MST connector
  is not yet detected at time of readout. crtc_state is invalid because connector_mask = 0.

It would be awesome if DP-MST would be detected before this point. I'm not sure this is
actually possible but it would make fastboot work on DP-MST as well.

- Alternatively some hardware may be connected to the wrong plane. Older i915 cards had a
  flexible mapping of plane 1 being connected to crtc 1 or 2, but for optimal performance you'd
  want it to be plane 1 to crtc 2. BIOS chose plane 1 to crtc 1.

Kind regards,
~Maarten Lankhorst

On 10/15/26 17:09, Maxime Ripard wrote:
> drm_bridge_connector allows to create a generic connector from a list of
> bridges.
> 
> However, it's a somewhat virtual connector, and relies on the bridges to
> implement its various capabilities.
> 
> What we actually want though is for the last bridge implementing
> hardware readout to fill the connector state from its own state.
> 
> Thus, implement a new op for bridge_connector to allow just
> that.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/display/drm_bridge_connector.c | 35 ++++++++++++++++++++++++++
>  include/drm/drm_bridge.h                       | 27 ++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 81f3e26f6fdf..dfc633bd8823 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -8,10 +8,11 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
>  
> +#include <drm/drm_atomic_sro_helper.h>
>  #include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_bridge_connector.h>
>  #include <drm/drm_connector.h>
>  #include <drm/drm_device.h>
> @@ -65,10 +66,18 @@ struct drm_bridge_connector {
>  	 * @encoder:
>  	 *
>  	 * The encoder at the start of the bridges chain.
>  	 */
>  	struct drm_encoder *encoder;
> +	/**
> +	 * @bridge_connector_hw_readout:
> +	 *
> +	 * The last bridge in the chain (closest to the connector) that
> +	 * provides hardware state readout support, if any (see
> +	 * &DRM_BRIDGE_OP_CONNECTOR_HW_READOUT).
> +	 */
> +	struct drm_bridge *bridge_connector_hw_readout;
>  	/**
>  	 * @bridge_edid:
>  	 *
>  	 * The last bridge in the chain (closest to the connector) that provides
>  	 * EDID read support, if any (see &DRM_BRIDGE_OP_EDID).
> @@ -242,10 +251,30 @@ drm_bridge_connector_create_state(struct drm_connector *connector)
>  							      conn_state);
>  
>  	return conn_state;
>  }
>  
> +static int
> +drm_bridge_connector_readout_state(struct drm_connector *connector,
> +				   struct drm_atomic_sro_state *state,
> +				   struct drm_connector_state *conn_state)
> +{
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *readout =
> +		bridge_connector->bridge_connector_hw_readout;
> +	int ret;
> +
> +	if (readout) {
> +		ret = readout->funcs->atomic_sro_connector_readout(readout, state, conn_state);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static enum drm_connector_color_format
>  drm_bridge_connector_color_format(const struct drm_connector_state *conn_state)
>  {
>  	struct drm_bridge_connector *bridge_connector =
>  		to_drm_bridge_connector(conn_state->connector);
> @@ -268,10 +297,12 @@ drm_bridge_connector_color_format(const struct drm_connector_state *conn_state)
>  }
>  
>  static const struct drm_connector_funcs drm_bridge_connector_funcs = {
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.atomic_create_state = drm_bridge_connector_create_state,
> +	.atomic_sro_readout_state = drm_bridge_connector_readout_state,
> +	.atomic_sro_compare_state = drm_atomic_helper_connector_compare_state,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  	.debugfs_init = drm_bridge_connector_debugfs_init,
>  	.oob_hotplug_event = drm_bridge_connector_oob_hotplug_event,
>  	.color_format = drm_bridge_connector_color_format,
> @@ -894,10 +925,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  		if (!bridge->interlace_allowed)
>  			connector->interlace_allowed = false;
>  		if (!bridge->ycbcr_420_allowed)
>  			connector->ycbcr_420_allowed = false;
>  
> +		if (bridge->ops & DRM_BRIDGE_OP_CONNECTOR_HW_READOUT) {
> +			drm_bridge_put(bridge_connector->bridge_connector_hw_readout);
> +			bridge_connector->bridge_connector_hw_readout = drm_bridge_get(bridge);
> +		}
>  		/*
>  		 * Ensure the last bridge declares OP_EDID or OP_MODES or both.
>  		 */
>  		if (bridge->ops & DRM_BRIDGE_OP_EDID || bridge->ops & DRM_BRIDGE_OP_MODES) {
>  			drm_bridge_put(bridge_connector->bridge_edid);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index fcccd8f6403d..7d6aaba0b82c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -935,10 +935,30 @@ struct drm_bridge_funcs {
>  	 */
>  	int (*dp_audio_mute_stream)(struct drm_bridge *bridge,
>  				    struct drm_connector *connector,
>  				    bool enable, int direction);
>  
> +	/**
> +	 * @atomic_sro_connector_readout:
> +	 *
> +	 * This optional hook initializes the &struct drm_connector_state
> +	 * based on hardware state.
> +	 *
> +	 * It is implemented by bridges that set the
> +	 * %DRM_BRIDGE_OP_CONNECTOR_HW_READOUT flag in their
> +	 * &drm_bridge.ops. When using drm_bridge_connector, the last
> +	 * bridge in the chain with this flag set will have its hook
> +	 * called to fill the connector state.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success, a negative error code otherwise.
> +	 */
> +	int (*atomic_sro_connector_readout)(struct drm_bridge *bridge,
> +					    struct drm_atomic_sro_state *state,
> +					    struct drm_connector_state *conn_state);
> +
>  	/**
>  	 * @debugfs_init:
>  	 *
>  	 * Allows bridges to create bridge-specific debugfs files.
>  	 */
> @@ -1075,10 +1095,17 @@ enum drm_bridge_ops {
>  	 * @DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME: The bridge supports
>  	 * &drm_bridge_funcs->hdmi_write_spd_infoframe and
>  	 * &drm_bridge_funcs->hdmi_clear_spd_infoframe callbacks.
>  	 */
>  	DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME = BIT(10),
> +	/**
> +	 * @DRM_BRIDGE_OP_CONNECTOR_HW_READOUT: The bridge supports the
> +	 * &drm_bridge_funcs.atomic_sro_connector_readout callback to
> +	 * fill the connector state from the bridge's own hardware state
> +	 * during state readout.
> +	 */
> +	DRM_BRIDGE_OP_CONNECTOR_HW_READOUT = BIT(11),
>  };
>  
>  /**
>   * struct drm_bridge - central DRM bridge control structure
>   */
>