Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/bridge: display-connector SRO readout aborts on missing upstream state --
commit 3d42e46c975cd0148add9f035d4abfcb174d9299 Author: Maxime Ripard <[email protected]> drm/bridge: display-connector: Implement readout support The display-connector bridge is the last bridge in the chain on platforms using drm_bridge_connector. This commit implements the atomic_sro_readout_state hook, reconstructing the bridge bus configuration from the previous bridge's readout state and the connector's display_info. > diff --git a/drivers/gpu/drm/bridge/display-connector.c > b/drivers/gpu/drm/bridge/display-connector.c > index eb3412ec30a07..7f285b7c1e84b 100644 > --- a/drivers/gpu/drm/bridge/display-connector.c > +++ b/drivers/gpu/drm/bridge/display-connector.c [ ... ] > @@ -213,6 +216,65 @@ static u32 *display_connector_get_input_bus_fmts(struct > drm_bridge *bridge, [ ... ] > +static int > +display_connector_readout_state(struct drm_bridge *bridge, > + struct drm_atomic_sro_state *state, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + struct drm_display_info *di = &conn_state->connector->display_info; > + struct drm_bridge *prev_bridge __free(drm_bridge_put) = > + drm_bridge_get_prev_bridge(bridge); > + > + if (prev_bridge) { > + struct drm_private_state *prev_priv_state = > + drm_atomic_sro_get_private_obj_state(state, > + > &prev_bridge->base); > + struct drm_bridge_state *prev_bridge_state; > + > + if (!prev_priv_state) > + return -ENODEV; [Severity: Medium] Will this hard error abort the entire SRO readout if an upstream bridge is missing readout support? If the DRM core is configured to attempt readout despite missing implementations, drm_atomic_helper_sro_readout_bridges_state() will skip bridges lacking an atomic_sro_readout_state hook: if (!bridge_obj_funcs->atomic_sro_readout_state) { drm_warn(dev, "No bridge %s readout implementation.", bridge->name); continue; } If that happens, prev_bridge will be non-NULL, but prev_priv_state will be NULL since the core continued without allocating it. Returning -ENODEV here causes the core to fail with a warning, rather than gracefully falling back to default values like it does when there is no prev_bridge at all. Should this fall back to MEDIA_BUS_FMT_FIXED instead of returning an error? > + > + prev_bridge_state = > + drm_priv_to_bridge_state(prev_priv_state); > + > + bridge_state->input_bus_cfg.format = > + prev_bridge_state->output_bus_cfg.format; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=21
