Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Medium] drm/atomic_sro: unexported drm_atomic_helper_colorop_create_state() call - [Medium] drm/atomic_sro: missing EXPORT_SYMBOL() on per-object readout helpers - [Medium] drm/atomic_sro: colorop state leak in drm_atomic_helper_sro_readout_state() - [Medium] drm/atomic_sro: missing fallback state when object readout hook is absent - [Medium] drm/atomic_sro: bridge readout WARN_ON() on partial commit connector state --
commit dadf54d14092f977607e2034fd03f065196831b7 Author: Maxime Ripard <[email protected]> drm/atomic_sro: Provide helpers to implement hardware state readout This commit introduces helper functions in drm_kms_helper to iterate over KMS objects, allocate pristine states, and invoke their hardware state readout callbacks, along with top-level helpers to build and install the initial readout state. > diff --git a/drivers/gpu/drm/drm_atomic_state_readout_helper.c > b/drivers/gpu/drm/drm_atomic_state_readout_helper.c > new file mode 100644 > index 0000000000000..a0b0337df2a00 > --- /dev/null > +++ b/drivers/gpu/drm/drm_atomic_state_readout_helper.c > @@ -0,0 +1,681 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +#include <drm/drm_atomic.h> > +#include <drm/drm_atomic_state_readout.h> > +#include <drm/drm_atomic_state_readout_helper.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_colorop.h> > +#include <drm/drm_connector.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_encoder.h> > +#include <drm/drm_modeset_helper_vtables.h> > +#include <drm/drm_plane.h> > +#include <drm/drm_print.h> > + > +#include <linux/gfp.h> > +#include <linux/sched/mm.h> > + > +static int > +drm_atomic_helper_sro_readout_plane_state(struct drm_device *dev, > + struct drm_atomic_sro_state *state, > + struct drm_plane *plane) > +{ > + const struct drm_plane_funcs *plane_funcs = plane->funcs; > + struct drm_plane_state *plane_state; > + int ret; > + > + if (!plane_funcs->atomic_sro_readout_state) { > + drm_warn(dev, "No plane readout implementation."); > + return 0; > + } [Severity: Medium] When atomic_readout is set to DRM_ATOMIC_READOUT_SKIP_MISSING_READOUT, drm_atomic_sro_device_can_readout() allows readout to proceed even if some objects do not implement atomic_sro_readout_state. If drm_atomic_helper_sro_readout_plane_state() (and the corresponding CRTC, connector, bridge, and private object helpers below) returns 0 here without allocating a default state via atomic_create_state() and storing it in state, drm_atomic_helper_sro_readout_state() will succeed and cause drm_mode_config_create_initial_state() to skip drm_mode_config_create_state(): drivers/gpu/drm/drm_mode_config.c:drm_mode_config_create_initial_state() { if (dev->mode_config.funcs->atomic_sro_readout_state) { ret = dev->mode_config.funcs->atomic_sro_readout_state(dev); if (!ret) goto out; } ret = drm_mode_config_create_state(dev); } Won't that leave any object lacking atomic_sro_readout_state with a NULL state pointer after initialization (and also trigger WARN_ON(!conn_state) in find_connector_state_for_encoder() during bridge readout if a connector was skipped)? > + > + drm_dbg_kms(dev, "Initializing Plane %s state.\n", plane->name); > + > + plane_state = plane_funcs->atomic_create_state(plane); > + if (drm_WARN_ON(dev, IS_ERR(plane_state))) > + return PTR_ERR(plane_state); > + > + ret = plane_funcs->atomic_sro_readout_state(plane, state, > + plane_state); > + if (drm_WARN_ON(dev, ret)) { > + plane_funcs->atomic_destroy_state(plane, plane_state); > + return ret; > + } > + > + drm_atomic_sro_set_plane_state(state, plane, plane_state); > + > + return 0; > +} [ ... ] > +int > +drm_atomic_helper_sro_readout_planes_state(struct drm_device *dev, > + struct drm_atomic_sro_state *state, > + struct drm_atomic_commit *commit) > +{ > + struct drm_plane *plane; > + int ret; > + > + might_alloc(GFP_KERNEL); > + > + if (commit) > + return drm_atomic_helper_sro_readout_commit_planes_state(dev, > state, commit); > + > + drm_for_each_plane(plane, dev) { > + ret = drm_atomic_helper_sro_readout_plane_state(dev, state, > plane); > + if (ret) > + return ret; > + } > + > + return 0; > +} [Severity: Medium] Should drm_atomic_helper_sro_readout_planes_state() and the other four per-object helpers (drm_atomic_helper_sro_readout_crtcs_state(), drm_atomic_helper_sro_readout_connectors_state(), drm_atomic_helper_sro_readout_bridges_state(), and drm_atomic_helper_sro_readout_private_objs_state()) have EXPORT_SYMBOL()? They are declared in include/drm/drm_atomic_state_readout_helper.h and pulled into Documentation/gpu/drm-kms-helpers.rst via :export:, but without EXPORT_SYMBOL() modular drivers implementing a custom atomic_sro_build_state callback will fail to link at modpost. [ ... ] > +static int > +drm_atomic_helper_sro_readout_commit_connectors_state(struct drm_device *dev, > + struct > drm_atomic_sro_state *state, > + struct drm_atomic_commit > *commit) > +{ > + struct drm_connector_state *new_connector_state; > + struct drm_connector *connector; > + unsigned int i; > + int ret; > + > + if (!commit) > + return -EINVAL; > + > + for_each_new_connector_in_state(commit, connector, new_connector_state, > i) { > + ret = drm_atomic_helper_sro_readout_connector_state(dev, state, > connector); > + if (ret) > + return ret; > + } > + > + return 0; > +} [Severity: Medium] When drm_atomic_helper_sro_build_state() is called with a non-NULL commit that only includes a subset of the device's connectors, drm_atomic_helper_sro_readout_commit_connectors_state() only populates connector states in state for the connectors in that commit. Later in drm_atomic_helper_sro_build_state(), drm_atomic_helper_sro_readout_bridges_state() reads out the bridges for the committed connectors' encoders. For bridges using drm_bridge_atomic_readout_priv_state(), this calls find_connector_state_for_encoder() in drivers/gpu/drm/drm_bridge.c: drm_for_each_connector_iter(connector, &conn_iter) { struct drm_connector_state *conn_state = drm_atomic_sro_get_connector_state(state, connector); if (WARN_ON(!conn_state)) continue; Doesn't this trigger WARN_ON(!conn_state) for every connector on the device that was not part of the commit? [ ... ] > +int drm_atomic_helper_sro_readout_state(struct drm_device *dev) > +{ > + const struct drm_mode_config_helper_funcs *funcs = > + dev->mode_config.helper_private; > + struct drm_atomic_sro_state *state; > + struct drm_colorop *colorop; > + > + if (!drm_atomic_sro_device_can_readout(dev)) { > + drm_info(dev, "Device doesn't support hardware state readout."); > + return -ENOTSUPP; > + } > + > + state = funcs->atomic_sro_build_state(dev, NULL); > + if (IS_ERR(state)) > + return PTR_ERR(state); > + > + /* TODO: Add colorop state readout support */ > + drm_for_each_colorop(colorop, dev) { > + struct drm_colorop_state *colorop_state; > + > + colorop_state = drm_atomic_helper_colorop_create_state(colorop); [Severity: Medium] Does this build when CONFIG_DRM_KMS_HELPER=m? drm_atomic_state_readout_helper.o is linked into drm_kms_helper.ko in drivers/gpu/drm/Makefile, while drm_atomic_helper_colorop_create_state() is defined in drivers/gpu/drm/drm_colorop.c (part of drm.ko) without EXPORT_SYMBOL(), which causes a modpost undefined symbol error. > + if (IS_ERR(colorop_state)) { > + drm_atomic_sro_state_free(state); > + return PTR_ERR(colorop_state); > + } > + > + colorop->state = colorop_state; > + } [Severity: Medium] Can this loop leak colorop->state? The drm_plane_colorop_*_init() helpers in drivers/gpu/drm/drm_colorop.c call drm_colorop_reset(), which allocates and populates colorop->state during initialization. In drivers/gpu/drm/drm_mode_config.c, drm_mode_config_create_state() skips colorops that already have a state: drm_for_each_colorop(colorop, dev) { struct drm_colorop_state *colorop_state; if (colorop->state) continue; colorop_state = drm_atomic_helper_colorop_create_state(colorop); Assigning colorop->state unconditionally here without checking or destroying an existing colorop->state (and without cleaning up earlier iterations if a later drm_atomic_helper_colorop_create_state() call fails) appears to leak the previously allocated struct drm_colorop_state. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=17
