Re: [PATCH 6/8] drm/ast: Only set VGA SCREEN_DISABLE bit in CRTC code

[Jocelyn Falempe <jfalempe@xxxxxxxxxx>]

On 27/06/2024 17:27, Thomas Zimmermann wrote:
> The SCREEN_DISABLE bit controls scanout from display memory. The bit
> affects all planes, so set it only in the CRTC's atomic enable and
> disable functions.
>
> A number of bugs affect this fix. First of all, ast_set_std_regs()
> tries to set VGASR1 except for the SD bit. Bit the read bitmask is
> invert, so it preserves anything exceptt the SD bit. Fix this by
> inverting the read mask.
>
> The second issue is that primary-plane and CRTC helpers modify the
> SD bit. The bit controls scanout for all planes, primary and HW
> cursor, so set it only in the CRTC code.
>
> Further add a constant to represent the SD bit in VGASR1. Keep the
> plane's atomic_disable around to make the DRM framework happy.

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 | 14 +++++++-------
>   drivers/gpu/drm/ast/ast_reg.h  |  1 +
>   2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index e90179bc0842..77358b587098 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -303,7 +303,7 @@ static void ast_set_std_reg(struct ast_device *ast,
>   
>   	/* Set SEQ; except Screen Disable field */
>   	ast_set_index_reg(ast, AST_IO_VGASRI, 0x00, 0x03);
> -	ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x01, 0xdf, stdtable->seq[0]);
> +	ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x01, 0x20, stdtable->seq[0]);
>   	for (i = 1; i < 4; i++) {
>   		jreg = stdtable->seq[i];
>   		ast_set_index_reg(ast, AST_IO_VGASRI, (i + 1), jreg);
> @@ -690,15 +690,15 @@ static void ast_primary_plane_helper_atomic_enable(struct drm_plane *plane,
>   	 * Therefore only reprogram the address after enabling the plane.
>   	 */
>   	ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
> -	ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x1, 0xdf, 0x00);
>   }
>   
>   static void ast_primary_plane_helper_atomic_disable(struct drm_plane *plane,
>   						    struct drm_atomic_state *state)
>   {
> -	struct ast_device *ast = to_ast_device(plane->dev);
> -
> -	ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x1, 0xdf, 0x20);
> +	/*
> +	 * Keep this empty function to avoid calling
> +	 * atomic_update when disabling the plane.
> +	 */
>   }
>   
>   static int ast_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane,
> @@ -1029,14 +1029,14 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
>   	 */
>   	switch (mode) {
>   	case DRM_MODE_DPMS_ON:
> -		ast_set_index_reg_mask(ast, AST_IO_VGASRI,  0x01, 0xdf, 0);
>   		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, 0x20);
> +		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;
>   	}
> diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
> index 62dddbf3fe56..6326cbdadc82 100644
> --- a/drivers/gpu/drm/ast/ast_reg.h
> +++ b/drivers/gpu/drm/ast/ast_reg.h
> @@ -22,6 +22,7 @@
>   #define AST_IO_VGAER_VGA_ENABLE		BIT(0)
>   
>   #define AST_IO_VGASRI			(0x44)
> +#define AST_IO_VGASR1_SD		BIT(5)
>   #define AST_IO_VGADRR			(0x47)
>   #define AST_IO_VGADWR			(0x48)
>   #define AST_IO_VGAPDR		        (0x49)