diff mbox series

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

Message ID 20240627153638.8765-7-tzimmermann@suse.de (mailing list archive)
State New, archived
Headers show
Series drm/ast: Untangle the chaos in mode setting | expand

Commit Message

Thomas Zimmermann June 27, 2024, 3:27 p.m. UTC 
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.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 14 +++++++-------
 drivers/gpu/drm/ast/ast_reg.h  |  1 +
 2 files changed, 8 insertions(+), 7 deletions(-)

Comments

Jocelyn Falempe June 28, 2024, 9:56 a.m. UTC | #1 
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@redhat.com>

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   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)
diff mbox series

Patch

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)