On Mon, Sep 15, 2025 at 11:11:39AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 15.09.25 um 10:42 schrieb Maxime Ripard:
> > Hi Tohmas,
> >
> > On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > > +/**
> > > > + * drm_atomic_build_readout_state - Creates an initial state from the
> > > > hardware
> > > > + * @dev: DRM device to build the state for
> > > > + *
> > > > + * This function allocates a &struct drm_atomic_state, calls the
> > > > + * atomic_readout_state callbacks, and fills the global state old
> > > > states
> > > > + * by what the callbacks returned.
> > > > + *
> > > > + * Returns:
> > > > + *
> > > > + * A partially initialized &struct drm_atomic_state on success, an
> > > > error
> > > > + * pointer otherwise.
> > > > + */
> > > > +static struct drm_atomic_state *
> > > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > > +{
> > > > + struct drm_connector_list_iter conn_iter;
> > > > + struct drm_atomic_state *state;
> > > > + struct drm_mode_config *config =
> > > > + &dev->mode_config;
> > > > + struct drm_connector *connector;
> > > > + struct drm_printer p =
> > > > + drm_info_printer(dev->dev);
> > > > + struct drm_encoder *encoder;
> > > > + struct drm_plane *plane;
> > > > + struct drm_crtc *crtc;
> > > > + int ret;
> > > > +
> > > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware
> > > > state.\n");
> > > > +
> > > > + state = drm_atomic_state_alloc(dev);
> > > > + if (WARN_ON(!state))
> > > > + return ERR_PTR(-ENOMEM);
> > > > +
> > > > + state->connectors = kcalloc(config->num_connector,
> > > > sizeof(*state->connectors), GFP_KERNEL);
> > > > + if (WARN_ON(!state->connectors)) {
> > > > + ret = -ENOMEM;
> > > > + goto err_state_put;
> > > > + }
> > > > +
> > > > + state->private_objs = kcalloc(count_private_obj(dev),
> > > > sizeof(*state->private_objs), GFP_KERNEL);
> > > > + if (WARN_ON(!state->private_objs)) {
> > > > + ret = -ENOMEM;
> > > > + goto err_state_put;
> > > > + }
> > > > +
> > > > + drm_for_each_crtc(crtc, dev) {
> > > > + const struct drm_crtc_funcs *crtc_funcs =
> > > > + crtc->funcs;
> > > > + struct drm_crtc_state *crtc_state;
> > > > +
> > > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n",
> > > > crtc->name);
> > > > +
> > > > + if (crtc_funcs->atomic_readout_state) {
> > > > + crtc_state =
> > > > crtc_funcs->atomic_readout_state(crtc);
> > > > + } else if (crtc_funcs->reset) {
> > > > + crtc_funcs->reset(crtc);
> > > > +
> > > > + /*
> > > > + * We don't want to set crtc->state field yet.
> > > > Let's save and clear it up.
> > > > + */
> > > > + crtc_state = crtc->state;
> > > > + crtc->state = NULL;
> > > Chancing the crtc->state pointer behind the back of the reset callback
> > > seems
> > > fragile. We never how if some other piece of the driver refers to it
> > > (although illegally).
> > I agree that it's clunky. I'm not sure who would use it at this point
> > though: we're in the middle of the drm_mode_config_reset(), so the
> > drivers' involvement is pretty minimal.
> >
> > I did wonder if changing reset to return the object instead of setting
> > $OBJECT->state would be a better interface?
>
> Probably not. The reset helper is supposed to initialize the object's
> software and hardware state. But in most drivers, we're currently mostly
> setting the minimal software state here and simply assume that hardware is
> off. Returning the state would water down semantics even further.
>
> Having said that, I could imaging building an atomic_clean_state callback
> that replaces the reset callback. It would work alongside the new
> atomic_readout_state callback. Current reset could be build upon that
> callback. The atomic_clean_state would intentionally only take care of the
> software state and leave hardware state undefined. This reflects the current
> realities of most DRM drivers. From that clean state, DRM could do an
> atomic commit that also initializes the hardware.That sounds like a good idea, but I wonder how we would deal with reset then? Should we remove it entirely? Still call it? What do you think? > > > For now, wouldn't it be better to require a read-out helper for all > > > elements > > > of the driver's mode-setting pipeline? The trivial implementation would > > > copy the existing reset function and keep crtc->state to NULL. > > I also considered that, but I'm not sure we can expect bridges to have > > readout hooks filled for every configuration in the wild. > > > > But maybe we can look during drm_mode_config_reset() at whether all the > > objects have their hook filled, and if not fall back on reset for > > everything. > > That's what I meant, I think. > > > It would make the implementation easier, but missing bridges > > implementations would trigger a mode change when it might actually work > > just fine since bridge state is pretty minimal. > > If there's an element in the pipeline that's missing the readout helper, it > might be safer to fallback to that modeset instead of ending up with > inconsistent state. Yeah, that sounds fair. Maxime
signature.asc
Description: PGP signature
