Re: [PATCH v2 11/28] drm/atomic_sro: Create kernel parameter to force or disable readout

[Simona Vetter <simona.vetter@ffwll.ch>]

On Thu, May 21, 2026 at 12:51:06PM +0200, Simona Vetter wrote:
> On Thu, Apr 23, 2026 at 12:18:24PM +0200, Maxime Ripard wrote:
> > The hardware state readout is useful, but might need to be disabled
> > in case of bugs, or its checks relaxed during development when not
> > all hooks are implemented yet.
> > 
> > Add a module parameter to control the readout behavior: it can be
> > disabled entirely, or the checks for missing compare or readout hooks
> > can be skipped independently.
> > 
> > Suggested-by: Simona Vetter <simona@ffwll.ch>
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/gpu/drm/drm_atomic_sro.c | 36 ++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_atomic_sro.h     |  2 ++
> >  2 files changed, 38 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_sro.c b/drivers/gpu/drm/drm_atomic_sro.c
> > index 177b97d451f5..a46f06e75c4e 100644
> > --- a/drivers/gpu/drm/drm_atomic_sro.c
> > +++ b/drivers/gpu/drm/drm_atomic_sro.c
> > @@ -11,10 +11,46 @@
> >  #include <linux/module.h>
> >  
> >  #include "drm_internal.h"
> >  #include "drm_crtc_internal.h"
> >  
> > +enum drm_atomic_readout_status {
> > +	DRM_ATOMIC_READOUT_DISABLED = 0,
> > +	DRM_ATOMIC_READOUT_ENABLED,
> > +	DRM_ATOMIC_READOUT_SKIP_MISSING_COMPARE,
> > +	DRM_ATOMIC_READOUT_SKIP_MISSING_READOUT,
> > +};
> > +
> > +static unsigned int atomic_readout = DRM_ATOMIC_READOUT_ENABLED;
> > +module_param_unsafe(atomic_readout, uint, 0);
> 
> Default is actually 0 here. I agree with the docs that it should be 1,
> since drivers have an explicit opt-in through setting the main entry point
> in drm_mode_config_funcs.
> 
> I was also pondering whether we should have a compare-only mode, but the
> issue is that once you build a driver on readout being a thing, that could
> blow up. So I think that should be left as a per-driver tunable.
> 
> That's also why this must be a unsafe debug option, it might actually
> break the driver.

Maxime pointed out that this makes non sense because the default is
already enabled, and yes I got confused. So strike that, but maybe think
whether 0600 as permissions makes more sense, because that's the bit that
confused me.

Either way, this hunk here also looks good as-is.
-Sima

> 
> > +MODULE_PARM_DESC(atomic_readout,
> > +		 "Enable Hardware State Readout (0 = disabled, 1 = enabled, 2 = ignore missing compares, 3 = ignore missing readouts and compares, default = 1)");
> > +
> > +/**
> > + * drm_atomic_sro_device_can_readout - check if a device supports hardware state readout
> > + * @dev: DRM device to check
> > + *
> > + * Verifies that the device is an atomic driver, that readout is
> > + * enabled, and that all KMS objects implement the relevant hooks.
> > + *
> > + * RETURNS:
> > + *
> > + * True if the device supports full hardware state readout, false
> > + * otherwise.
> > + */
> > +bool drm_atomic_sro_device_can_readout(struct drm_device *dev)
> > +{
> > +	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> 
> I think this should be drm_drv_uses_atomic_modeset() since it's an
> internal check, not an uapi check.
> 
> With the two issues addressed:
> 
> Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>
> 
> > +		return false;
> > +
> > +	if (atomic_readout == DRM_ATOMIC_READOUT_DISABLED)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_sro_device_can_readout);
> > +
> >  struct __drm_atomic_sro_plane {
> >  	struct drm_plane *ptr;
> >  	struct drm_plane_state *state;
> >  };
> >  
> > diff --git a/include/drm/drm_atomic_sro.h b/include/drm/drm_atomic_sro.h
> > index 5a9333a05796..6e5262384c71 100644
> > --- a/include/drm/drm_atomic_sro.h
> > +++ b/include/drm/drm_atomic_sro.h
> > @@ -13,10 +13,12 @@ struct drm_plane;
> >  struct drm_plane_state;
> >  struct drm_printer;
> >  struct drm_private_obj;
> >  struct drm_private_state;
> >  
> > +bool drm_atomic_sro_device_can_readout(struct drm_device *dev);
> > +
> >  struct drm_atomic_sro_state *drm_atomic_sro_state_alloc(struct drm_device *dev);
> >  void drm_atomic_sro_state_free(struct drm_atomic_sro_state *state);
> >  void drm_atomic_sro_state_print(const struct drm_atomic_sro_state *state,
> >  				struct drm_printer *p);
> >  
> > 
> > -- 
> > 2.53.0
> > 
> 
> -- 
> Simona Vetter
> Software Engineer
> http://blog.ffwll.ch

-- 
Simona Vetter
Software Engineer
http://blog.ffwll.ch