Re: [PATCH 7/8] drm/ast: Inline ast_crtc_dpms() into callers

[Jocelyn Falempe <jfalempe@xxxxxxxxxx>]

On 27/06/2024 17:27, Thomas Zimmermann wrote:
> The function ast_crtc_dpms() is left over from when the ast driver
> did not implement atomic modesetting. But DPMS is not supported by
> atomic modesetting and the helper is only called to enable or
> disable the CRTC sync pulses. Inline the function into its callers.
>
> To disable the CRTC, ast sets (AST_DPMS_VSYNC_OFF | AST_DPMS_HSYNC_OFF)
> in VGACRB6. Replace the constants with the correct register constants
> for VGACRB6.

Thanks, it looks good te me.

Reviewed-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
>   drivers/gpu/drm/ast/ast_mode.c | 36 +++++++++++-----------------------
>   drivers/gpu/drm/ast/ast_reg.h  |  9 ++-------
>   2 files changed, 13 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 77358b587098..d130c96edf35 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1019,29 +1019,6 @@ static int ast_cursor_plane_init(struct ast_device *ast)
>    * CRTC
>    */
>   
> -static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
> -{
> -	struct ast_device *ast = to_ast_device(crtc->dev);
> -	u8 ch = AST_DPMS_VSYNC_OFF | AST_DPMS_HSYNC_OFF;
> -
> -	/* TODO: Maybe control display signal generation with
> -	 *       Sync Enable (bit CR17.7).
> -	 */
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xb6, 0xfc, 0);
> -		ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x01, 0xdf, 0);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		ch = mode;
> -		ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x01, 0xdf, AST_IO_VGASR1_SD);
> -		ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xb6, 0xfc, ch);
> -		break;
> -	}
> -}
> -
>   static enum drm_mode_status
>   ast_crtc_helper_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode)
>   {
> @@ -1217,14 +1194,23 @@ ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>   
>   static void ast_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
>   {
> -	ast_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
> +	struct ast_device *ast = to_ast_device(crtc->dev);
> +
> +	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xb6, 0xfc, 0x00);
> +	ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x01, 0xdf, 0x00);
>   }
>   
>   static void ast_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
>   {
>   	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> +	struct ast_device *ast = to_ast_device(crtc->dev);
> +	u8 vgacrb6;
> +
> +	ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x01, 0xdf, AST_IO_VGASR1_SD);
>   
> -	ast_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
> +	vgacrb6 = AST_IO_VGACRB6_VSYNC_OFF |
> +		  AST_IO_VGACRB6_HSYNC_OFF;
> +	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xb6, 0xfc, vgacrb6);
>   
>   	/*
>   	 * HW cursors require the underlying primary plane and CRTC to
> diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
> index 6326cbdadc82..75671d345057 100644
> --- a/drivers/gpu/drm/ast/ast_reg.h
> +++ b/drivers/gpu/drm/ast/ast_reg.h
> @@ -32,6 +32,8 @@
>   #define AST_IO_VGACR80_PASSWORD		(0xa8)
>   #define AST_IO_VGACRA1_VGAIO_DISABLED	BIT(1)
>   #define AST_IO_VGACRA1_MMIO_ENABLED	BIT(2)
> +#define AST_IO_VGACRB6_HSYNC_OFF	BIT(0)
> +#define AST_IO_VGACRB6_VSYNC_OFF	BIT(1)
>   #define AST_IO_VGACRCB_HWC_16BPP	BIT(0) /* set: ARGB4444, cleared: 2bpp palette */
>   #define AST_IO_VGACRCB_HWC_ENABLED	BIT(1)
>   
> @@ -76,13 +78,6 @@
>   #define ASTDP_HOST_EDID_READ_DONE	BIT(0)
>   #define ASTDP_HOST_EDID_READ_DONE_MASK	GENMASK(0, 0)
>   
> -/*
> - * CRB8[b1]: Enable VSYNC off
> - * CRB8[b0]: Enable HSYNC off
> - */
> -#define AST_DPMS_VSYNC_OFF		BIT(1)
> -#define AST_DPMS_HSYNC_OFF		BIT(0)
> -
>   /*
>    * CRDF[b4]: Mirror of AST_DP_VIDEO_ENABLE
>    * Precondition:	A. ~AST_DP_PHY_SLEEP  &&