[PATCH 0/3] drm/ast: Various fixes

[PATCH 0/3] drm/ast: Various fixes

[Thomas Zimmermann]

Three small fixes for ast to resolve a few minor issues. Tested on
AST2600 hardware.

Thomas Zimmermann (3):
  drm/ast: Fix comment on modeset lock
  drm/ast: Remove vaddr field from struct ast_plane
  drm/ast: Validate display modes against framebuffer and format limits

 drivers/gpu/drm/ast/ast_cursor.c |  8 ++----
 drivers/gpu/drm/ast/ast_drv.h    |  4 +--
 drivers/gpu/drm/ast/ast_mode.c   | 49 +++++++++++++++++++-------------
 3 files changed, 35 insertions(+), 26 deletions(-)

-- 
2.48.1

[PATCH 1/3] drm/ast: Fix comment on modeset lock

[Thomas Zimmermann]

The ast driver protects the commit tail against concurrent reads
of the display modes by acquiring a lock. The comment is misleading
as the lock is not released in atomic_flush, but at the end of the
commit-tail helper. Rewrite the comment.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 1fe182154984 ("drm/ast: Acquire I/O-register lock in atomic_commit_tail function")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Jocelyn Falempe <jfalempe@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v6.2+
---
 drivers/gpu/drm/ast/ast_mode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 4cac5c7f4547..20fbea11b710 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -939,9 +939,9 @@ static void ast_mode_config_helper_atomic_commit_tail(struct drm_atomic_state *s
 
 	/*
 	 * Concurrent operations could possibly trigger a call to
-	 * drm_connector_helper_funcs.get_modes by trying to read the
-	 * display modes. Protect access to I/O registers by acquiring
-	 * the I/O-register lock. Released in atomic_flush().
+	 * drm_connector_helper_funcs.get_modes by reading the display
+	 * modes. Protect access to registers by acquiring the modeset
+	 * lock.
 	 */
 	mutex_lock(&ast->modeset_lock);
 	drm_atomic_helper_commit_tail(state);
-- 
2.48.1

Re: [PATCH 1/3] drm/ast: Fix comment on modeset lock

[Jocelyn Falempe]

On 24/03/2025 10:44, Thomas Zimmermann wrote:
> The ast driver protects the commit tail against concurrent reads
> of the display modes by acquiring a lock. The comment is misleading
> as the lock is not released in atomic_flush, but at the end of the
> commit-tail helper. Rewrite the comment.

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 1fe182154984 ("drm/ast: Acquire I/O-register lock in atomic_commit_tail function")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Jocelyn Falempe <jfalempe@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v6.2+
> ---
>   drivers/gpu/drm/ast/ast_mode.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 4cac5c7f4547..20fbea11b710 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -939,9 +939,9 @@ static void ast_mode_config_helper_atomic_commit_tail(struct drm_atomic_state *s
>   
>   	/*
>   	 * Concurrent operations could possibly trigger a call to
> -	 * drm_connector_helper_funcs.get_modes by trying to read the
> -	 * display modes. Protect access to I/O registers by acquiring
> -	 * the I/O-register lock. Released in atomic_flush().
> +	 * drm_connector_helper_funcs.get_modes by reading the display
> +	 * modes. Protect access to registers by acquiring the modeset
> +	 * lock.
>   	 */
>   	mutex_lock(&ast->modeset_lock);
>   	drm_atomic_helper_commit_tail(state);

[PATCH 2/3] drm/ast: Remove vaddr field from struct ast_plane

[Thomas Zimmermann]

The vaddr field in struct ast_plane serves no purpose. Its value
can be calculated easily from the VRAM base plus the plane offset.
Do so and remove the field.

In ast_primary_plane_helper_get_scanout_buffer(), remove the test
for vaddr being NULL. This cannot legally happen.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_cursor.c |  8 +++-----
 drivers/gpu/drm/ast/ast_drv.h    |  4 ++--
 drivers/gpu/drm/ast/ast_mode.c   | 19 ++++++++++++-------
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
index 5ee724bfd682..2d3ad7610c2e 100644
--- a/drivers/gpu/drm/ast/ast_cursor.c
+++ b/drivers/gpu/drm/ast/ast_cursor.c
@@ -91,7 +91,7 @@ static u32 ast_cursor_calculate_checksum(const void *src, unsigned int width, un
 static void ast_set_cursor_image(struct ast_device *ast, const u8 *src,
 				 unsigned int width, unsigned int height)
 {
-	u8 __iomem *dst = ast->cursor_plane.base.vaddr;
+	u8 __iomem *dst = ast_plane_vaddr(&ast->cursor_plane.base);
 	u32 csum;
 
 	csum = ast_cursor_calculate_checksum(src, width, height);
@@ -193,7 +193,7 @@ static void ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 	struct ast_device *ast = to_ast_device(plane->dev);
 	struct drm_rect damage;
 	u64 dst_off = ast_plane->offset;
-	u8 __iomem *dst = ast_plane->vaddr; /* TODO: Use mapping abstraction properly */
+	u8 __iomem *dst = ast_plane_vaddr(ast_plane); /* TODO: Use mapping abstraction properly */
 	u8 __iomem *sig = dst + AST_HWC_SIZE; /* TODO: Use mapping abstraction properly */
 	unsigned int offset_x, offset_y;
 	u16 x, y;
@@ -291,7 +291,6 @@ int ast_cursor_plane_init(struct ast_device *ast)
 	struct ast_plane *ast_plane = &ast_cursor_plane->base;
 	struct drm_plane *cursor_plane = &ast_plane->base;
 	unsigned long size;
-	void __iomem *vaddr;
 	long offset;
 	int ret;
 
@@ -299,9 +298,8 @@ int ast_cursor_plane_init(struct ast_device *ast)
 	offset = ast_cursor_vram_offset(ast);
 	if (offset < 0)
 		return offset;
-	vaddr = ast->vram + offset;
 
-	ret = ast_plane_init(dev, ast_plane, vaddr, offset, size,
+	ret = ast_plane_init(dev, ast_plane, offset, size,
 			     0x01, &ast_cursor_plane_funcs,
 			     ast_cursor_plane_formats, ARRAY_SIZE(ast_cursor_plane_formats),
 			     NULL, DRM_PLANE_TYPE_CURSOR);
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index d9da2328d46b..2ee402096cd9 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -122,7 +122,6 @@ enum ast_config_mode {
 struct ast_plane {
 	struct drm_plane base;
 
-	void __iomem *vaddr;
 	u64 offset;
 	unsigned long size;
 };
@@ -443,11 +442,12 @@ int ast_astdp_output_init(struct ast_device *ast);
 /* ast_mode.c */
 int ast_mode_config_init(struct ast_device *ast);
 int ast_plane_init(struct drm_device *dev, struct ast_plane *ast_plane,
-		   void __iomem *vaddr, u64 offset, unsigned long size,
+		   u64 offset, unsigned long size,
 		   uint32_t possible_crtcs,
 		   const struct drm_plane_funcs *funcs,
 		   const uint32_t *formats, unsigned int format_count,
 		   const uint64_t *format_modifiers,
 		   enum drm_plane_type type);
+void __iomem *ast_plane_vaddr(struct ast_plane *ast);
 
 #endif
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 20fbea11b710..d3ed27faefd1 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -457,7 +457,7 @@ static void ast_wait_for_vretrace(struct ast_device *ast)
  */
 
 int ast_plane_init(struct drm_device *dev, struct ast_plane *ast_plane,
-		   void __iomem *vaddr, u64 offset, unsigned long size,
+		   u64 offset, unsigned long size,
 		   uint32_t possible_crtcs,
 		   const struct drm_plane_funcs *funcs,
 		   const uint32_t *formats, unsigned int format_count,
@@ -466,7 +466,6 @@ int ast_plane_init(struct drm_device *dev, struct ast_plane *ast_plane,
 {
 	struct drm_plane *plane = &ast_plane->base;
 
-	ast_plane->vaddr = vaddr;
 	ast_plane->offset = offset;
 	ast_plane->size = size;
 
@@ -475,6 +474,13 @@ int ast_plane_init(struct drm_device *dev, struct ast_plane *ast_plane,
 					type, NULL);
 }
 
+void __iomem *ast_plane_vaddr(struct ast_plane *ast_plane)
+{
+	struct ast_device *ast = to_ast_device(ast_plane->base.dev);
+
+	return ast->vram + ast_plane->offset;
+}
+
 /*
  * Primary plane
  */
@@ -521,7 +527,7 @@ static void ast_handle_damage(struct ast_plane *ast_plane, struct iosys_map *src
 			      struct drm_framebuffer *fb,
 			      const struct drm_rect *clip)
 {
-	struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(ast_plane->vaddr);
+	struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(ast_plane_vaddr(ast_plane));
 
 	iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], fb->format, clip));
 	drm_fb_memcpy(&dst, fb->pitches, src, fb, clip);
@@ -594,12 +600,12 @@ static int ast_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane,
 {
 	struct ast_plane *ast_plane = to_ast_plane(plane);
 
-	if (plane->state && plane->state->fb && ast_plane->vaddr) {
+	if (plane->state && plane->state->fb) {
 		sb->format = plane->state->fb->format;
 		sb->width = plane->state->fb->width;
 		sb->height = plane->state->fb->height;
 		sb->pitch[0] = plane->state->fb->pitches[0];
-		iosys_map_set_vaddr_iomem(&sb->map[0], ast_plane->vaddr);
+		iosys_map_set_vaddr_iomem(&sb->map[0], ast_plane_vaddr(ast_plane));
 		return 0;
 	}
 	return -ENODEV;
@@ -626,12 +632,11 @@ static int ast_primary_plane_init(struct ast_device *ast)
 	struct drm_device *dev = &ast->base;
 	struct ast_plane *ast_primary_plane = &ast->primary_plane;
 	struct drm_plane *primary_plane = &ast_primary_plane->base;
-	void __iomem *vaddr = ast->vram;
 	u64 offset = ast_fb_vram_offset();
 	unsigned long size = ast_fb_vram_size(ast);
 	int ret;
 
-	ret = ast_plane_init(dev, ast_primary_plane, vaddr, offset, size,
+	ret = ast_plane_init(dev, ast_primary_plane, offset, size,
 			     0x01, &ast_primary_plane_funcs,
 			     ast_primary_plane_formats, ARRAY_SIZE(ast_primary_plane_formats),
 			     NULL, DRM_PLANE_TYPE_PRIMARY);
-- 
2.48.1

Re: [PATCH 2/3] drm/ast: Remove vaddr field from struct ast_plane

[Jocelyn Falempe]

On 24/03/2025 10:44, Thomas Zimmermann wrote:
> The vaddr field in struct ast_plane serves no purpose. Its value
> can be calculated easily from the VRAM base plus the plane offset.
> Do so and remove the field.
> 
> In ast_primary_plane_helper_get_scanout_buffer(), remove the test
> for vaddr being NULL. This cannot legally happen.

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_cursor.c |  8 +++-----
>   drivers/gpu/drm/ast/ast_drv.h    |  4 ++--
>   drivers/gpu/drm/ast/ast_mode.c   | 19 ++++++++++++-------
>   3 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
> index 5ee724bfd682..2d3ad7610c2e 100644
> --- a/drivers/gpu/drm/ast/ast_cursor.c
> +++ b/drivers/gpu/drm/ast/ast_cursor.c
> @@ -91,7 +91,7 @@ static u32 ast_cursor_calculate_checksum(const void *src, unsigned int width, un
>   static void ast_set_cursor_image(struct ast_device *ast, const u8 *src,
>   				 unsigned int width, unsigned int height)
>   {
> -	u8 __iomem *dst = ast->cursor_plane.base.vaddr;
> +	u8 __iomem *dst = ast_plane_vaddr(&ast->cursor_plane.base);
>   	u32 csum;
>   
>   	csum = ast_cursor_calculate_checksum(src, width, height);
> @@ -193,7 +193,7 @@ static void ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
>   	struct ast_device *ast = to_ast_device(plane->dev);
>   	struct drm_rect damage;
>   	u64 dst_off = ast_plane->offset;
> -	u8 __iomem *dst = ast_plane->vaddr; /* TODO: Use mapping abstraction properly */
> +	u8 __iomem *dst = ast_plane_vaddr(ast_plane); /* TODO: Use mapping abstraction properly */
>   	u8 __iomem *sig = dst + AST_HWC_SIZE; /* TODO: Use mapping abstraction properly */
>   	unsigned int offset_x, offset_y;
>   	u16 x, y;
> @@ -291,7 +291,6 @@ int ast_cursor_plane_init(struct ast_device *ast)
>   	struct ast_plane *ast_plane = &ast_cursor_plane->base;
>   	struct drm_plane *cursor_plane = &ast_plane->base;
>   	unsigned long size;
> -	void __iomem *vaddr;
>   	long offset;
>   	int ret;
>   
> @@ -299,9 +298,8 @@ int ast_cursor_plane_init(struct ast_device *ast)
>   	offset = ast_cursor_vram_offset(ast);
>   	if (offset < 0)
>   		return offset;
> -	vaddr = ast->vram + offset;
>   
> -	ret = ast_plane_init(dev, ast_plane, vaddr, offset, size,
> +	ret = ast_plane_init(dev, ast_plane, offset, size,
>   			     0x01, &ast_cursor_plane_funcs,
>   			     ast_cursor_plane_formats, ARRAY_SIZE(ast_cursor_plane_formats),
>   			     NULL, DRM_PLANE_TYPE_CURSOR);
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index d9da2328d46b..2ee402096cd9 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -122,7 +122,6 @@ enum ast_config_mode {
>   struct ast_plane {
>   	struct drm_plane base;
>   
> -	void __iomem *vaddr;
>   	u64 offset;
>   	unsigned long size;
>   };
> @@ -443,11 +442,12 @@ int ast_astdp_output_init(struct ast_device *ast);
>   /* ast_mode.c */
>   int ast_mode_config_init(struct ast_device *ast);
>   int ast_plane_init(struct drm_device *dev, struct ast_plane *ast_plane,
> -		   void __iomem *vaddr, u64 offset, unsigned long size,
> +		   u64 offset, unsigned long size,
>   		   uint32_t possible_crtcs,
>   		   const struct drm_plane_funcs *funcs,
>   		   const uint32_t *formats, unsigned int format_count,
>   		   const uint64_t *format_modifiers,
>   		   enum drm_plane_type type);
> +void __iomem *ast_plane_vaddr(struct ast_plane *ast);
>   
>   #endif
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 20fbea11b710..d3ed27faefd1 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -457,7 +457,7 @@ static void ast_wait_for_vretrace(struct ast_device *ast)
>    */
>   
>   int ast_plane_init(struct drm_device *dev, struct ast_plane *ast_plane,
> -		   void __iomem *vaddr, u64 offset, unsigned long size,
> +		   u64 offset, unsigned long size,
>   		   uint32_t possible_crtcs,
>   		   const struct drm_plane_funcs *funcs,
>   		   const uint32_t *formats, unsigned int format_count,
> @@ -466,7 +466,6 @@ int ast_plane_init(struct drm_device *dev, struct ast_plane *ast_plane,
>   {
>   	struct drm_plane *plane = &ast_plane->base;
>   
> -	ast_plane->vaddr = vaddr;
>   	ast_plane->offset = offset;
>   	ast_plane->size = size;
>   
> @@ -475,6 +474,13 @@ int ast_plane_init(struct drm_device *dev, struct ast_plane *ast_plane,
>   					type, NULL);
>   }
>   
> +void __iomem *ast_plane_vaddr(struct ast_plane *ast_plane)
> +{
> +	struct ast_device *ast = to_ast_device(ast_plane->base.dev);
> +
> +	return ast->vram + ast_plane->offset;
> +}
> +
>   /*
>    * Primary plane
>    */
> @@ -521,7 +527,7 @@ static void ast_handle_damage(struct ast_plane *ast_plane, struct iosys_map *src
>   			      struct drm_framebuffer *fb,
>   			      const struct drm_rect *clip)
>   {
> -	struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(ast_plane->vaddr);
> +	struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(ast_plane_vaddr(ast_plane));
>   
>   	iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], fb->format, clip));
>   	drm_fb_memcpy(&dst, fb->pitches, src, fb, clip);
> @@ -594,12 +600,12 @@ static int ast_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane,
>   {
>   	struct ast_plane *ast_plane = to_ast_plane(plane);
>   
> -	if (plane->state && plane->state->fb && ast_plane->vaddr) {
> +	if (plane->state && plane->state->fb) {
>   		sb->format = plane->state->fb->format;
>   		sb->width = plane->state->fb->width;
>   		sb->height = plane->state->fb->height;
>   		sb->pitch[0] = plane->state->fb->pitches[0];
> -		iosys_map_set_vaddr_iomem(&sb->map[0], ast_plane->vaddr);
> +		iosys_map_set_vaddr_iomem(&sb->map[0], ast_plane_vaddr(ast_plane));
>   		return 0;
>   	}
>   	return -ENODEV;
> @@ -626,12 +632,11 @@ static int ast_primary_plane_init(struct ast_device *ast)
>   	struct drm_device *dev = &ast->base;
>   	struct ast_plane *ast_primary_plane = &ast->primary_plane;
>   	struct drm_plane *primary_plane = &ast_primary_plane->base;
> -	void __iomem *vaddr = ast->vram;
>   	u64 offset = ast_fb_vram_offset();
>   	unsigned long size = ast_fb_vram_size(ast);
>   	int ret;
>   
> -	ret = ast_plane_init(dev, ast_primary_plane, vaddr, offset, size,
> +	ret = ast_plane_init(dev, ast_primary_plane, offset, size,
>   			     0x01, &ast_primary_plane_funcs,
>   			     ast_primary_plane_formats, ARRAY_SIZE(ast_primary_plane_formats),
>   			     NULL, DRM_PLANE_TYPE_PRIMARY);

[PATCH 3/3] drm/ast: Validate display modes against framebuffer and format limits

[Thomas Zimmermann]

Reimplement ast_mode_config_mode_valid() with DRM format helpers and
ast's helpers for framebuffer size calculation. Replaces ast's open-
coded assumptions on bpp and page-alignments.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index d3ed27faefd1..a9a3252ff3c2 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -51,6 +51,8 @@
 
 #define AST_LUT_SIZE 256
 
+#define AST_PRIMARY_PLANE_MAX_OFFSET	(BIT(16) - 1)
+
 static unsigned long ast_fb_vram_offset(void)
 {
 	return 0; // with shmem, the primary plane is always at offset 0
@@ -960,16 +962,20 @@ static const struct drm_mode_config_helper_funcs ast_mode_config_helper_funcs =
 static enum drm_mode_status ast_mode_config_mode_valid(struct drm_device *dev,
 						       const struct drm_display_mode *mode)
 {
-	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGB8888 */
+	const struct drm_format_info *info = drm_format_info(DRM_FORMAT_XRGB8888);
 	struct ast_device *ast = to_ast_device(dev);
-	unsigned long fbsize, fbpages, max_fbpages;
-
-	max_fbpages = ast_fb_vram_size(ast) >> PAGE_SHIFT;
-
-	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
-	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
-
-	if (fbpages > max_fbpages)
+	unsigned long max_fb_size = ast_fb_vram_size(ast);
+	u64 pitch;
+
+	if (drm_WARN_ON_ONCE(dev, !info))
+		return MODE_ERROR; /* driver bug */
+
+	pitch = drm_format_info_min_pitch(info, 0, mode->hdisplay);
+	if (!pitch)
+		return MODE_BAD_WIDTH;
+	if (pitch > AST_PRIMARY_PLANE_MAX_OFFSET)
+		return MODE_BAD_WIDTH; /* maximum programmable pitch */
+	if (pitch > max_fb_size / mode->vdisplay)
 		return MODE_MEM;
 
 	return MODE_OK;
-- 
2.48.1

Re: [PATCH 3/3] drm/ast: Validate display modes against framebuffer and format limits

[Jocelyn Falempe]

On 24/03/2025 10:44, Thomas Zimmermann wrote:
> Reimplement ast_mode_config_mode_valid() with DRM format helpers and
> ast's helpers for framebuffer size calculation. Replaces ast's open-
> coded assumptions on bpp and page-alignments.
> 

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_mode.c | 24 +++++++++++++++---------
>   1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index d3ed27faefd1..a9a3252ff3c2 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -51,6 +51,8 @@
>   
>   #define AST_LUT_SIZE 256
>   
> +#define AST_PRIMARY_PLANE_MAX_OFFSET	(BIT(16) - 1)
> +
>   static unsigned long ast_fb_vram_offset(void)
>   {
>   	return 0; // with shmem, the primary plane is always at offset 0
> @@ -960,16 +962,20 @@ static const struct drm_mode_config_helper_funcs ast_mode_config_helper_funcs =
>   static enum drm_mode_status ast_mode_config_mode_valid(struct drm_device *dev,
>   						       const struct drm_display_mode *mode)
>   {
> -	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGB8888 */
> +	const struct drm_format_info *info = drm_format_info(DRM_FORMAT_XRGB8888);
>   	struct ast_device *ast = to_ast_device(dev);
> -	unsigned long fbsize, fbpages, max_fbpages;
> -
> -	max_fbpages = ast_fb_vram_size(ast) >> PAGE_SHIFT;
> -
> -	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
> -	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
> -
> -	if (fbpages > max_fbpages)
> +	unsigned long max_fb_size = ast_fb_vram_size(ast);
> +	u64 pitch;
> +
> +	if (drm_WARN_ON_ONCE(dev, !info))
> +		return MODE_ERROR; /* driver bug */
> +
> +	pitch = drm_format_info_min_pitch(info, 0, mode->hdisplay);
> +	if (!pitch)
> +		return MODE_BAD_WIDTH;
> +	if (pitch > AST_PRIMARY_PLANE_MAX_OFFSET)
> +		return MODE_BAD_WIDTH; /* maximum programmable pitch */
> +	if (pitch > max_fb_size / mode->vdisplay)
>   		return MODE_MEM;
>   
>   	return MODE_OK;