Re: [PATCH 50/70] drm/loongsoon: Move hardware reset to CRTC creation

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi Maxime

Am 21.08.26 um 16:39 schrieb Maxime Ripard:
> The loongsoon crtc reset hook performs both atomic state initialization
> using __drm_atomic_helper_crtc_reset() and hardware-level register
> writes to reset the CRTC.
>
> The hardware reset is not related to atomic state initialization, and
> the reset hook is being converted to atomic_create_state which only
> deals with state allocation. Move the hardware reset to the CRTC
> creation path instead.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> Cc: Huacai Chen <chenhuacai@kernel.org>
> Cc: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> Cc: Jianmin Lv <lvjianmin@loongson.cn>
> Cc: Mingcong Bai <jeffbai@aosc.io>
> Cc: Qianhai Wu <wuqianhai@loongson.cn>
> Cc: Xi Ruoyao <xry111@xry111.site>
> ---
>   drivers/gpu/drm/loongson/lsdc_crtc.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/loongson/lsdc_crtc.c b/drivers/gpu/drm/loongson/lsdc_crtc.c
> index 16b8f36a4071..97a3443739e1 100644
> --- a/drivers/gpu/drm/loongson/lsdc_crtc.c
> +++ b/drivers/gpu/drm/loongson/lsdc_crtc.c
> @@ -388,12 +388,10 @@ static const struct lsdc_crtc_hw_ops ls7a2000_crtc_hw_ops[2] = {
>   	},
>   };
>   
>   static void lsdc_crtc_reset(struct drm_crtc *crtc)
>   {
> -	struct lsdc_crtc *lcrtc = to_lsdc_crtc(crtc);
> -	const struct lsdc_crtc_hw_ops *ops = lcrtc->hw_ops;
>   	struct lsdc_crtc_state *priv_crtc_state;
>   
>   	if (crtc->state)
>   		crtc->funcs->atomic_destroy_state(crtc, crtc->state);
>   
> @@ -401,13 +399,10 @@ static void lsdc_crtc_reset(struct drm_crtc *crtc)
>   
>   	if (!priv_crtc_state)
>   		__drm_atomic_helper_crtc_reset(crtc, NULL);
>   	else
>   		__drm_atomic_helper_crtc_reset(crtc, &priv_crtc_state->base);
> -
> -	/* Reset the CRTC hardware, this is required for S3 support */
> -	ops->reset(lcrtc);

As the Sashiko bot noted, removing this here will likely break S3 
resume. IIUC you could instead add it before resuming DRM state at [1]

[1] 
https://elixir.bootlin.com/linux/v7.2/source/drivers/gpu/drm/loongson/lsdc_drv.c#L390

Best regards
Thomas

>   }
>   
>   static void lsdc_crtc_atomic_destroy_state(struct drm_crtc *crtc,
>   					   struct drm_crtc_state *state)
>   {
> @@ -981,10 +976,13 @@ int ls7a1000_crtc_init(struct drm_device *ddev,
>   	if (ret)
>   		return ret;
>   
>   	drm_crtc_enable_color_mgmt(crtc, 0, false, 256);
>   
> +	/* Reset the CRTC hardware, this is required for S3 support */
> +	lcrtc->hw_ops->reset(lcrtc);
> +
>   	return 0;
>   }
>   
>   int ls7a2000_crtc_init(struct drm_device *ddev,
>   		       struct drm_crtc *crtc,
> @@ -1020,7 +1018,10 @@ int ls7a2000_crtc_init(struct drm_device *ddev,
>   	if (ret)
>   		return ret;
>   
>   	drm_crtc_enable_color_mgmt(crtc, 0, false, 256);
>   
> +	/* Reset the CRTC hardware, this is required for S3 support */
> +	lcrtc->hw_ops->reset(lcrtc);
> +
>   	return 0;
>   }
>

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