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

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

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.

> +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