Re: [PATCH v2 01/13] drm/crtc: Introduce hw_reset helper hook

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi Maxime

Am 31.08.26 um 17:34 schrieb Maxime Ripard:
> The CRTC reset hook conflates initial software state allocation with
> hardware reset. The atomic_create_state hook addresses the software
> state side, but drivers may still need to reset hardware to a known good
> state during drm_mode_config_reset(), for example during suspend/resume.
>
> Separating hardware reset from state allocation is also useful for the
> pending atomic state readout and userspace atomic reset flag series,
> which need to create pristine software state without affecting the
> hardware.
>
> Introduce a hw_reset hook in struct drm_crtc_helper_funcs that only
> resets the hardware, without touching the software state at all. Call it
> from drm_mode_config_crtc_reset_with_create_state() after the state has
> been successfully created.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

with comments below.

> ---
>   drivers/gpu/drm/drm_mode_config.c        | 13 ++++++++++++-
>   include/drm/drm_modeset_helper_vtables.h | 14 ++++++++++++++
>   2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 366f6d821242..bb2efc274323 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -28,10 +28,11 @@
>   #include <drm/drm_encoder.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_managed.h>
>   #include <drm/drm_mode_config.h>
> +#include <drm/drm_modeset_helper_vtables.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_colorop.h>
>   #include <linux/dma-resv.h>
>   
>   #include "drm_crtc_internal.h"
> @@ -228,16 +229,26 @@ static int drm_mode_config_crtc_create_state(struct drm_crtc *crtc)
>   	return 0;
>   }
>   
>   static int drm_mode_config_crtc_reset_with_create_state(struct drm_crtc *crtc)
>   {
> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;

Please see the Sahsiko comment about this being NULL. And I suggest to 
call the variable 'helpers' because 'funcs' sounds like regular 
drm_crtc_funcs. But that's really just nitpicking.

Best regards
Thomas

> +	int ret;
> +
>   	if (crtc->state) {
>   		crtc->funcs->atomic_destroy_state(crtc, crtc->state);
>   		crtc->state = NULL;
>   	}
>   
> -	return drm_mode_config_crtc_create_state(crtc);
> +	ret = drm_mode_config_crtc_create_state(crtc);
> +	if (ret)
> +		return ret;
> +
> +	if (crtc_funcs->hw_reset)
> +		crtc_funcs->hw_reset(crtc);
> +
> +	return 0;
>   }
>   
>   static int drm_mode_config_connector_create_state(struct drm_connector *connector)
>   {
>   	struct drm_connector_state *conn_state;
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index ca6268945c28..4965356a6314 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -279,10 +279,24 @@ struct drm_crtc_helper_funcs {
>   	 * drivers still using legacy CRTC helpers, which is different from the
>   	 * rules under atomic.
>   	 */
>   	void (*disable)(struct drm_crtc *crtc);
>   
> +	/**
> +	 * @hw_reset:
> +	 *
> +	 * Optional hook for CRTC hardware reset.
> +	 *
> +	 * Unlike @drm_crtc_funcs.reset, which both resets hardware and
> +	 * creates new software state, this hook only resets the
> +	 * hardware to a known good state without touching the software
> +	 * state at all.
> +	 *
> +	 * This hook is called by drm_mode_config_reset().
> +	 */
> +	void (*hw_reset)(struct drm_crtc *crtc);
> +
>   	/**
>   	 * @atomic_check:
>   	 *
>   	 * Drivers should check plane-update related CRTC constraints in this
>   	 * hook. They can also check mode related limitations but need to be
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)