[PATCH 0/8] drm/ast: Convert ast driver to SHMEM

[PATCH 0/8] drm/ast: Convert ast driver to SHMEM

[Thomas Zimmermann]

This patchset converts ast to GEM SHMEM helpers. Fixes problems with
memory allocation and BMC scanout updates.

Patches 1 to 3 are fixes for various minor problems in the ast driver.
We should merge them even without SHMEM support.

Patches 4 to 6 prepare the driver for the conversion. The cursor double
buffering is not required and prevents reuse among the plane code. Style
issues are being fixed separately from the conversion.

The conversion itself is in patch 7. Not only does it fix problems with
memory allocation, it also brings back several high-res display modes
that got lost during the ast driver's conversion to atomic modesetting.
There was an earlier RFC patch of this code that had issues with
rendering performnce. [1] We've meanwhile improved these areas and
performance was acceptable on the test systems.

With SHMEM in place, the scanout address for the primary plane does not
have to be changed often. Patch 8 fixes a performance problem where the
BMC output freezes for several seconds after reprogramming the scanout
address.

Tested on AST 2100 and 2300 with fbdev emulation, weston, and Gnome in
X11 and Wayland mode.

[1] https://lore.kernel.org/dri-devel/5a3537c3-2c81-b9de-e4c7-c00577cdd43d@suse.de/

Thomas Zimmermann (8):
  drm/ast: Acquire I/O-register lock in atomic_commit_tail function
  drm/ast: Call drm_atomic_helper_check_plane_state() unconditionally
  drm/ast: Do not call drm_atomic_add_affected_planes()
  drm/ast: Remove cursor double buffering
  drm/ast: Rename struct ast_cursor_plane to struct ast_plane
  drm/ast: Style cleanups in plane code
  drm/ast: Convert ast to SHMEM
  drm/ast: Avoid reprogramming primary-plane scanout address

 drivers/gpu/drm/ast/Kconfig    |   4 +-
 drivers/gpu/drm/ast/ast_drv.c  |   4 +-
 drivers/gpu/drm/ast/ast_drv.h  |  34 +--
 drivers/gpu/drm/ast/ast_main.c |   5 +-
 drivers/gpu/drm/ast/ast_mm.c   |  14 +-
 drivers/gpu/drm/ast/ast_mode.c | 399 ++++++++++++++++-----------------
 6 files changed, 219 insertions(+), 241 deletions(-)


base-commit: 74e2443e7681e4d442b45f551ddf12d09a6f00c3
-- 
2.37.3

[PATCH 1/8] drm/ast: Acquire I/O-register lock in atomic_commit_tail function

[Thomas Zimmermann]

Hold I/O-register lock in atomic_commit_tail to protect all pipeline
updates at once. Protects modesetting against concurrent EDID reads.

Complex modesetting operations involve mode changes and plane updates.
These steps used to be protected individually against concurrent I/O.
Make all this atomic wrt to reading display modes via EDID. The EDID
code in the conenctor's get_modes helper already acquires the necessary
lock.

A similar issue was fixed in commit 2d70b9a1482e ("drm/mgag200: Acquire
I/O-register lock in atomic_commit_tail function") for mgag200.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index d5ee3ad538a8..e1e07928906e 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1200,20 +1200,6 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
 	return drm_atomic_add_affected_planes(state, crtc);
 }
 
-static void ast_crtc_helper_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state)
-{
-	struct drm_device *dev = crtc->dev;
-	struct ast_private *ast = to_ast_private(dev);
-
-	/*
-	 * 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().
-	 */
-	mutex_lock(&ast->ioregs_lock);
-}
-
 static void
 ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 			     struct drm_atomic_state *state)
@@ -1241,8 +1227,6 @@ ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 	//Set Aspeed Display-Port
 	if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
 		ast_dp_set_mode(crtc, vbios_mode_info);
-
-	mutex_unlock(&ast->ioregs_lock);
 }
 
 static void
@@ -1301,7 +1285,6 @@ ast_crtc_helper_atomic_disable(struct drm_crtc *crtc,
 static const struct drm_crtc_helper_funcs ast_crtc_helper_funcs = {
 	.mode_valid = ast_crtc_helper_mode_valid,
 	.atomic_check = ast_crtc_helper_atomic_check,
-	.atomic_begin = ast_crtc_helper_atomic_begin,
 	.atomic_flush = ast_crtc_helper_atomic_flush,
 	.atomic_enable = ast_crtc_helper_atomic_enable,
 	.atomic_disable = ast_crtc_helper_atomic_disable,
@@ -1771,8 +1754,23 @@ static int ast_astdp_output_init(struct ast_private *ast)
  * Mode config
  */
 
+static void ast_mode_config_helper_atomic_commit_tail(struct drm_atomic_state *state)
+{
+	struct ast_private *ast = to_ast_private(state->dev);
+
+	/*
+	 * 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().
+	 */
+	mutex_lock(&ast->ioregs_lock);
+	drm_atomic_helper_commit_tail_rpm(state);
+	mutex_unlock(&ast->ioregs_lock);
+}
+
 static const struct drm_mode_config_helper_funcs ast_mode_config_helper_funcs = {
-	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+	.atomic_commit_tail = ast_mode_config_helper_atomic_commit_tail,
 };
 
 static const struct drm_mode_config_funcs ast_mode_config_funcs = {
-- 
2.37.3

Re: [PATCH 1/8] drm/ast: Acquire I/O-register lock in atomic_commit_tail function

[Jocelyn Falempe]

On 10/10/2022 12:36, Thomas Zimmermann wrote:
> Hold I/O-register lock in atomic_commit_tail to protect all pipeline
> updates at once. Protects modesetting against concurrent EDID reads.
> 
> Complex modesetting operations involve mode changes and plane updates.
> These steps used to be protected individually against concurrent I/O.
> Make all this atomic wrt to reading display modes via EDID. The EDID
> code in the conenctor's get_modes helper already acquires the necessary
small typo:  connector's

> lock.
> 
> A similar issue was fixed in commit 2d70b9a1482e ("drm/mgag200: Acquire
> I/O-register lock in atomic_commit_tail function") for mgag200.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_mode.c | 34 ++++++++++++++++------------------
>   1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index d5ee3ad538a8..e1e07928906e 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1200,20 +1200,6 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
>   	return drm_atomic_add_affected_planes(state, crtc);
>   }
>   
> -static void ast_crtc_helper_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct ast_private *ast = to_ast_private(dev);
> -
> -	/*
> -	 * 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().
> -	 */
> -	mutex_lock(&ast->ioregs_lock);
> -}
> -
>   static void
>   ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>   			     struct drm_atomic_state *state)
> @@ -1241,8 +1227,6 @@ ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>   	//Set Aspeed Display-Port
>   	if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
>   		ast_dp_set_mode(crtc, vbios_mode_info);
> -
> -	mutex_unlock(&ast->ioregs_lock);
>   }
>   
>   static void
> @@ -1301,7 +1285,6 @@ ast_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>   static const struct drm_crtc_helper_funcs ast_crtc_helper_funcs = {
>   	.mode_valid = ast_crtc_helper_mode_valid,
>   	.atomic_check = ast_crtc_helper_atomic_check,
> -	.atomic_begin = ast_crtc_helper_atomic_begin,
>   	.atomic_flush = ast_crtc_helper_atomic_flush,
>   	.atomic_enable = ast_crtc_helper_atomic_enable,
>   	.atomic_disable = ast_crtc_helper_atomic_disable,
> @@ -1771,8 +1754,23 @@ static int ast_astdp_output_init(struct ast_private *ast)
>    * Mode config
>    */
>   
> +static void ast_mode_config_helper_atomic_commit_tail(struct drm_atomic_state *state)
> +{
> +	struct ast_private *ast = to_ast_private(state->dev);
> +
> +	/*
> +	 * 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().
> +	 */
> +	mutex_lock(&ast->ioregs_lock);
> +	drm_atomic_helper_commit_tail_rpm(state);
> +	mutex_unlock(&ast->ioregs_lock);
> +}
> +
>   static const struct drm_mode_config_helper_funcs ast_mode_config_helper_funcs = {
> -	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +	.atomic_commit_tail = ast_mode_config_helper_atomic_commit_tail,
>   };
>   
>   static const struct drm_mode_config_funcs ast_mode_config_funcs = {

Best regards,

-- 

Jocelyn

[PATCH 2/8] drm/ast: Call drm_atomic_helper_check_plane_state() unconditionally

[Thomas Zimmermann]

Always call drm_atomic_helper_check_plane_state() in each plane's
atomic_check function. At the minimum, it needs to set or clear the
plane state's 'visible' field. Otherwise the plane-state handling
is bogus and would keep updating planes that have been disabled.

While at it, also warn if the primary plane has been enabled, but is
not visible. This cannot legally happen as the plane always covers
the entire screen.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index e1e07928906e..e26471ecffb1 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -578,27 +578,28 @@ static const uint32_t ast_primary_plane_formats[] = {
 static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						 struct drm_atomic_state *state)
 {
+	struct drm_device *dev = plane->dev;
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state = NULL;
 	struct ast_crtc_state *ast_crtc_state;
 	int ret;
 
-	if (!new_plane_state->crtc)
-		return 0;
-
-	crtc_state = drm_atomic_get_new_crtc_state(state,
-						   new_plane_state->crtc);
+	if (new_plane_state->crtc)
+		crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
 
 	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
 						  DRM_PLANE_NO_SCALING,
 						  DRM_PLANE_NO_SCALING,
 						  false, true);
-	if (ret)
+	if (ret) {
 		return ret;
-
-	if (!new_plane_state->visible)
-		return 0;
+	} else if (!new_plane_state->visible) {
+		if (drm_WARN_ON(dev, new_plane_state->crtc)) /* cannot legally happen */
+			return -EINVAL;
+		else
+			return 0;
+	}
 
 	ast_crtc_state = to_ast_crtc_state(crtc_state);
 
@@ -805,25 +806,19 @@ static int ast_cursor_plane_helper_atomic_check(struct drm_plane *plane,
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
 	struct drm_framebuffer *fb = new_plane_state->fb;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state = NULL;
 	int ret;
 
-	if (!new_plane_state->crtc)
-		return 0;
-
-	crtc_state = drm_atomic_get_new_crtc_state(state,
-						   new_plane_state->crtc);
+	if (new_plane_state->crtc)
+		crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
 
 	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
 						  DRM_PLANE_NO_SCALING,
 						  DRM_PLANE_NO_SCALING,
 						  true, true);
-	if (ret)
+	if (ret || !new_plane_state->visible)
 		return ret;
 
-	if (!new_plane_state->visible)
-		return 0;
-
 	if (fb->width > AST_MAX_HWC_WIDTH || fb->height > AST_MAX_HWC_HEIGHT)
 		return -EINVAL;
 
-- 
2.37.3

[PATCH 3/8] drm/ast: Do not call drm_atomic_add_affected_planes()

[Thomas Zimmermann]

There's no need to add planes to the atomic state. Remove the call
to drm_atomic_add_affected_planes() from ast.

On full modesets, the DRM helpers already add a CRTC's planes to the
atomic state; see drm_atomic_helper_check_modeset(). There's no reason
to call drm_atomic_add_affected_planes() unconditionally in the CRTC's
atomic_check() in ast. It's also too late, as the atomic_check() of
the added planes will not be called before the commit.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index e26471ecffb1..059e4906507d 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1157,7 +1157,7 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
 	int ret;
 
 	if (!crtc_state->enable)
-		goto out;
+		return 0;
 
 	ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
 	if (ret)
@@ -1191,8 +1191,7 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
 	if (!succ)
 		return -EINVAL;
 
-out:
-	return drm_atomic_add_affected_planes(state, crtc);
+	return 0;
 }
 
 static void
-- 
2.37.3

Re: [PATCH 3/8] drm/ast: Do not call drm_atomic_add_affected_planes()

[Javier Martinez Canillas]

Hello Thomas,

On 10/10/22 12:36, Thomas Zimmermann wrote:
> There's no need to add planes to the atomic state. Remove the call
> to drm_atomic_add_affected_planes() from ast.
> 
> On full modesets, the DRM helpers already add a CRTC's planes to the
> atomic state; see drm_atomic_helper_check_modeset(). There's no reason
> to call drm_atomic_add_affected_planes() unconditionally in the CRTC's
> atomic_check() in ast. It's also too late, as the atomic_check() of
> the added planes will not be called before the commit.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

The change makes sense to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 4/8] drm/ast: Remove cursor double buffering

[Thomas Zimmermann]

Update the cursor image via damage handling in-place. The cursor's
double buffering has no visible effect on the output, so remove it.

Done in preparation of switching ast to GEM SHMEM helpers. Removing
double buffering will allow us to use the same data structure for
primary and cursor plane.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  | 12 ++---
 drivers/gpu/drm/ast/ast_mode.c | 83 +++++++++++++---------------------
 2 files changed, 35 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 2e44b971c3a6..12294c74d0fc 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -96,8 +96,6 @@ enum ast_tx_chip {
 #define AST_HWC_SIZE		(AST_MAX_HWC_WIDTH * AST_MAX_HWC_HEIGHT * 2)
 #define AST_HWC_SIGNATURE_SIZE	32
 
-#define AST_DEFAULT_HWC_NUM	2
-
 /* define for signature structure */
 #define AST_HWC_SIGNATURE_CHECKSUM	0x00
 #define AST_HWC_SIGNATURE_SizeX		0x04
@@ -110,13 +108,9 @@ enum ast_tx_chip {
 struct ast_cursor_plane {
 	struct drm_plane base;
 
-	struct {
-		struct drm_gem_vram_object *gbo;
-		struct iosys_map map;
-		u64 off;
-	} hwc[AST_DEFAULT_HWC_NUM];
-
-	unsigned int next_hwc_index;
+	struct drm_gem_vram_object *gbo;
+	struct iosys_map map;
+	u64 off;
 };
 
 static inline struct ast_cursor_plane *
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 059e4906507d..06ee79ec86f1 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -837,10 +837,8 @@ ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(new_state);
 	struct drm_framebuffer *fb = new_state->fb;
 	struct ast_private *ast = to_ast_private(plane->dev);
-	struct iosys_map dst_map =
-		ast_cursor_plane->hwc[ast_cursor_plane->next_hwc_index].map;
-	u64 dst_off =
-		ast_cursor_plane->hwc[ast_cursor_plane->next_hwc_index].off;
+	struct iosys_map dst_map = ast_cursor_plane->map;
+	u64 dst_off = ast_cursor_plane->off;
 	struct iosys_map src_map = shadow_plane_state->data[0];
 	unsigned int offset_x, offset_y;
 	u16 x, y;
@@ -860,13 +858,9 @@ ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 
 	ast_update_cursor_image(dst, src, fb->width, fb->height);
 
-	if (new_state->fb != old_state->fb) {
+	if (new_state->fb != old_state->fb)
 		ast_set_cursor_base(ast, dst_off);
 
-		++ast_cursor_plane->next_hwc_index;
-		ast_cursor_plane->next_hwc_index %= ARRAY_SIZE(ast_cursor_plane->hwc);
-	}
-
 	/*
 	 * Update location in HWC signature and registers.
 	 */
@@ -917,17 +911,12 @@ static const struct drm_plane_helper_funcs ast_cursor_plane_helper_funcs = {
 static void ast_cursor_plane_destroy(struct drm_plane *plane)
 {
 	struct ast_cursor_plane *ast_cursor_plane = to_ast_cursor_plane(plane);
-	size_t i;
-	struct drm_gem_vram_object *gbo;
-	struct iosys_map map;
+	struct drm_gem_vram_object *gbo = ast_cursor_plane->gbo;
+	struct iosys_map map = ast_cursor_plane->map;
 
-	for (i = 0; i < ARRAY_SIZE(ast_cursor_plane->hwc); ++i) {
-		gbo = ast_cursor_plane->hwc[i].gbo;
-		map = ast_cursor_plane->hwc[i].map;
-		drm_gem_vram_vunmap(gbo, &map);
-		drm_gem_vram_unpin(gbo);
-		drm_gem_vram_put(gbo);
-	}
+	drm_gem_vram_vunmap(gbo, &map);
+	drm_gem_vram_unpin(gbo);
+	drm_gem_vram_put(gbo);
 
 	drm_plane_cleanup(plane);
 }
@@ -944,7 +933,7 @@ static int ast_cursor_plane_init(struct ast_private *ast)
 	struct drm_device *dev = &ast->base;
 	struct ast_cursor_plane *ast_cursor_plane = &ast->cursor_plane;
 	struct drm_plane *cursor_plane = &ast_cursor_plane->base;
-	size_t size, i;
+	size_t size;
 	struct drm_gem_vram_object *gbo;
 	struct iosys_map map;
 	int ret;
@@ -957,29 +946,27 @@ static int ast_cursor_plane_init(struct ast_private *ast)
 
 	size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
 
-	for (i = 0; i < ARRAY_SIZE(ast_cursor_plane->hwc); ++i) {
-		gbo = drm_gem_vram_create(dev, size, 0);
-		if (IS_ERR(gbo)) {
-			ret = PTR_ERR(gbo);
-			goto err_hwc;
-		}
-		ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM |
-					    DRM_GEM_VRAM_PL_FLAG_TOPDOWN);
-		if (ret)
-			goto err_drm_gem_vram_put;
-		ret = drm_gem_vram_vmap(gbo, &map);
-		if (ret)
-			goto err_drm_gem_vram_unpin;
-		off = drm_gem_vram_offset(gbo);
-		if (off < 0) {
-			ret = off;
-			goto err_drm_gem_vram_vunmap;
-		}
-		ast_cursor_plane->hwc[i].gbo = gbo;
-		ast_cursor_plane->hwc[i].map = map;
-		ast_cursor_plane->hwc[i].off = off;
+	gbo = drm_gem_vram_create(dev, size, 0);
+	if (IS_ERR(gbo))
+		return PTR_ERR(gbo);
+
+	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM |
+				    DRM_GEM_VRAM_PL_FLAG_TOPDOWN);
+	if (ret)
+		goto err_drm_gem_vram_put;
+	ret = drm_gem_vram_vmap(gbo, &map);
+	if (ret)
+		goto err_drm_gem_vram_unpin;
+	off = drm_gem_vram_offset(gbo);
+	if (off < 0) {
+		ret = off;
+		goto err_drm_gem_vram_vunmap;
 	}
 
+	ast_cursor_plane->gbo = gbo;
+	ast_cursor_plane->map = map;
+	ast_cursor_plane->off = off;
+
 	/*
 	 * Create the cursor plane. The plane's destroy callback will release
 	 * the backing storages' BO memory.
@@ -992,24 +979,18 @@ static int ast_cursor_plane_init(struct ast_private *ast)
 				       NULL, DRM_PLANE_TYPE_CURSOR, NULL);
 	if (ret) {
 		drm_err(dev, "drm_universal_plane failed(): %d\n", ret);
-		goto err_hwc;
+		goto err_drm_gem_vram_vunmap;
 	}
 	drm_plane_helper_add(cursor_plane, &ast_cursor_plane_helper_funcs);
 
 	return 0;
 
-err_hwc:
-	while (i) {
-		--i;
-		gbo = ast_cursor_plane->hwc[i].gbo;
-		map = ast_cursor_plane->hwc[i].map;
 err_drm_gem_vram_vunmap:
-		drm_gem_vram_vunmap(gbo, &map);
+	drm_gem_vram_vunmap(gbo, &map);
 err_drm_gem_vram_unpin:
-		drm_gem_vram_unpin(gbo);
+	drm_gem_vram_unpin(gbo);
 err_drm_gem_vram_put:
-		drm_gem_vram_put(gbo);
-	}
+	drm_gem_vram_put(gbo);
 	return ret;
 }
 
-- 
2.37.3

[PATCH 5/8] drm/ast: Rename struct ast_cursor_plane to struct ast_plane

[Thomas Zimmermann]

Rename the plane structure struct ast_cursor_plane to struct
ast_plane as it will be used for the primary plane as well. No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  | 15 +++++++++------
 drivers/gpu/drm/ast/ast_mode.c | 22 +++++++++++-----------
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 12294c74d0fc..02120025b7ac 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -87,7 +87,7 @@ enum ast_tx_chip {
 #define AST_DRAM_8Gx16   8
 
 /*
- * Cursor plane
+ * Hardware cursor
  */
 
 #define AST_MAX_HWC_WIDTH	64
@@ -105,7 +105,11 @@ enum ast_tx_chip {
 #define AST_HWC_SIGNATURE_HOTSPOTX	0x14
 #define AST_HWC_SIGNATURE_HOTSPOTY	0x18
 
-struct ast_cursor_plane {
+/*
+ * Planes
+ */
+
+struct ast_plane {
 	struct drm_plane base;
 
 	struct drm_gem_vram_object *gbo;
@@ -113,10 +117,9 @@ struct ast_cursor_plane {
 	u64 off;
 };
 
-static inline struct ast_cursor_plane *
-to_ast_cursor_plane(struct drm_plane *plane)
+static inline struct ast_plane *to_ast_plane(struct drm_plane *plane)
 {
-	return container_of(plane, struct ast_cursor_plane, base);
+	return container_of(plane, struct ast_plane, base);
 }
 
 /*
@@ -170,7 +173,7 @@ struct ast_private {
 	uint32_t mclk;
 
 	struct drm_plane primary_plane;
-	struct ast_cursor_plane cursor_plane;
+	struct ast_plane cursor_plane;
 	struct drm_crtc crtc;
 	struct {
 		struct {
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 06ee79ec86f1..00257db864ba 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -829,7 +829,7 @@ static void
 ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 				      struct drm_atomic_state *state)
 {
-	struct ast_cursor_plane *ast_cursor_plane = to_ast_cursor_plane(plane);
+	struct ast_plane *ast_plane = to_ast_plane(plane);
 	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
 									   plane);
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
@@ -837,8 +837,8 @@ ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(new_state);
 	struct drm_framebuffer *fb = new_state->fb;
 	struct ast_private *ast = to_ast_private(plane->dev);
-	struct iosys_map dst_map = ast_cursor_plane->map;
-	u64 dst_off = ast_cursor_plane->off;
+	struct iosys_map dst_map = ast_plane->map;
+	u64 dst_off = ast_plane->off;
 	struct iosys_map src_map = shadow_plane_state->data[0];
 	unsigned int offset_x, offset_y;
 	u16 x, y;
@@ -910,9 +910,9 @@ static const struct drm_plane_helper_funcs ast_cursor_plane_helper_funcs = {
 
 static void ast_cursor_plane_destroy(struct drm_plane *plane)
 {
-	struct ast_cursor_plane *ast_cursor_plane = to_ast_cursor_plane(plane);
-	struct drm_gem_vram_object *gbo = ast_cursor_plane->gbo;
-	struct iosys_map map = ast_cursor_plane->map;
+	struct ast_plane *ast_plane = to_ast_plane(plane);
+	struct drm_gem_vram_object *gbo = ast_plane->gbo;
+	struct iosys_map map = ast_plane->map;
 
 	drm_gem_vram_vunmap(gbo, &map);
 	drm_gem_vram_unpin(gbo);
@@ -931,8 +931,8 @@ static const struct drm_plane_funcs ast_cursor_plane_funcs = {
 static int ast_cursor_plane_init(struct ast_private *ast)
 {
 	struct drm_device *dev = &ast->base;
-	struct ast_cursor_plane *ast_cursor_plane = &ast->cursor_plane;
-	struct drm_plane *cursor_plane = &ast_cursor_plane->base;
+	struct ast_plane *ast_plane = &ast->cursor_plane;
+	struct drm_plane *cursor_plane = &ast_plane->base;
 	size_t size;
 	struct drm_gem_vram_object *gbo;
 	struct iosys_map map;
@@ -963,9 +963,9 @@ static int ast_cursor_plane_init(struct ast_private *ast)
 		goto err_drm_gem_vram_vunmap;
 	}
 
-	ast_cursor_plane->gbo = gbo;
-	ast_cursor_plane->map = map;
-	ast_cursor_plane->off = off;
+	ast_plane->gbo = gbo;
+	ast_plane->map = map;
+	ast_plane->off = off;
 
 	/*
 	 * Create the cursor plane. The plane's destroy callback will release
-- 
2.37.3

[PATCH 6/8] drm/ast: Style cleanups in plane code

[Thomas Zimmermann]

Rename some of the variables in the plane code to better reflect the
old and new state during checks and updates. Change some indention as
well. No functional changes.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 00257db864ba..59c70fd5b925 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -579,16 +579,15 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						 struct drm_atomic_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
-										 plane);
-	struct drm_crtc_state *crtc_state = NULL;
-	struct ast_crtc_state *ast_crtc_state;
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_crtc_state *new_crtc_state = NULL;
+	struct ast_crtc_state *new_ast_crtc_state;
 	int ret;
 
 	if (new_plane_state->crtc)
-		crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
 
-	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
+	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
 						  DRM_PLANE_NO_SCALING,
 						  DRM_PLANE_NO_SCALING,
 						  false, true);
@@ -601,30 +600,28 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
 			return 0;
 	}
 
-	ast_crtc_state = to_ast_crtc_state(crtc_state);
+	new_ast_crtc_state = to_ast_crtc_state(new_crtc_state);
 
-	ast_crtc_state->format = new_plane_state->fb->format;
+	new_ast_crtc_state->format = new_plane_state->fb->format;
 
 	return 0;
 }
 
-static void
-ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
-				       struct drm_atomic_state *state)
+static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
+						   struct drm_atomic_state *state)
 {
-	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
-									   plane);
 	struct drm_device *dev = plane->dev;
 	struct ast_private *ast = to_ast_private(dev);
-	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
-									   plane);
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+	struct drm_framebuffer *old_fb = old_plane_state->fb;
 	struct drm_gem_vram_object *gbo;
 	s64 gpu_addr;
-	struct drm_framebuffer *fb = new_state->fb;
-	struct drm_framebuffer *old_fb = old_state->fb;
 
 	if (!old_fb || (fb->format != old_fb->format)) {
-		struct drm_crtc_state *crtc_state = new_state->crtc->state;
+		struct drm_crtc *crtc = plane_state->crtc;
+		struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 		struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);
 		struct ast_vbios_mode_info *vbios_mode_info = &ast_crtc_state->vbios_mode_info;
 
@@ -643,9 +640,8 @@ ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
 }
 
-static void
-ast_primary_plane_helper_atomic_disable(struct drm_plane *plane,
-					struct drm_atomic_state *state)
+static void ast_primary_plane_helper_atomic_disable(struct drm_plane *plane,
+						    struct drm_atomic_state *state)
 {
 	struct ast_private *ast = to_ast_private(plane->dev);
 
@@ -803,39 +799,36 @@ static const uint32_t ast_cursor_plane_formats[] = {
 static int ast_cursor_plane_helper_atomic_check(struct drm_plane *plane,
 						struct drm_atomic_state *state)
 {
-	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
-										 plane);
-	struct drm_framebuffer *fb = new_plane_state->fb;
-	struct drm_crtc_state *crtc_state = NULL;
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_framebuffer *new_fb = new_plane_state->fb;
+	struct drm_crtc_state *new_crtc_state = NULL;
 	int ret;
 
 	if (new_plane_state->crtc)
-		crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
 
-	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
+	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
 						  DRM_PLANE_NO_SCALING,
 						  DRM_PLANE_NO_SCALING,
 						  true, true);
 	if (ret || !new_plane_state->visible)
 		return ret;
 
-	if (fb->width > AST_MAX_HWC_WIDTH || fb->height > AST_MAX_HWC_HEIGHT)
+	if (new_fb->width > AST_MAX_HWC_WIDTH || new_fb->height > AST_MAX_HWC_HEIGHT)
 		return -EINVAL;
 
 	return 0;
 }
 
-static void
-ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
-				      struct drm_atomic_state *state)
+static void ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
+						  struct drm_atomic_state *state)
 {
 	struct ast_plane *ast_plane = to_ast_plane(plane);
-	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
-									   plane);
-	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
-									   plane);
-	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(new_state);
-	struct drm_framebuffer *fb = new_state->fb;
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+	struct drm_framebuffer *old_fb = old_plane_state->fb;
 	struct ast_private *ast = to_ast_private(plane->dev);
 	struct iosys_map dst_map = ast_plane->map;
 	u64 dst_off = ast_plane->off;
@@ -858,32 +851,32 @@ ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 
 	ast_update_cursor_image(dst, src, fb->width, fb->height);
 
-	if (new_state->fb != old_state->fb)
+	if (fb != old_fb)
 		ast_set_cursor_base(ast, dst_off);
 
 	/*
 	 * Update location in HWC signature and registers.
 	 */
 
-	writel(new_state->crtc_x, sig + AST_HWC_SIGNATURE_X);
-	writel(new_state->crtc_y, sig + AST_HWC_SIGNATURE_Y);
+	writel(plane_state->crtc_x, sig + AST_HWC_SIGNATURE_X);
+	writel(plane_state->crtc_y, sig + AST_HWC_SIGNATURE_Y);
 
 	offset_x = AST_MAX_HWC_WIDTH - fb->width;
 	offset_y = AST_MAX_HWC_HEIGHT - fb->height;
 
-	if (new_state->crtc_x < 0) {
-		x_offset = (-new_state->crtc_x) + offset_x;
+	if (plane_state->crtc_x < 0) {
+		x_offset = (-plane_state->crtc_x) + offset_x;
 		x = 0;
 	} else {
 		x_offset = offset_x;
-		x = new_state->crtc_x;
+		x = plane_state->crtc_x;
 	}
-	if (new_state->crtc_y < 0) {
-		y_offset = (-new_state->crtc_y) + offset_y;
+	if (plane_state->crtc_y < 0) {
+		y_offset = (-plane_state->crtc_y) + offset_y;
 		y = 0;
 	} else {
 		y_offset = offset_y;
-		y = new_state->crtc_y;
+		y = plane_state->crtc_y;
 	}
 
 	ast_set_cursor_location(ast, x, y, x_offset, y_offset);
@@ -892,9 +885,8 @@ ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 	ast_set_cursor_enabled(ast, true);
 }
 
-static void
-ast_cursor_plane_helper_atomic_disable(struct drm_plane *plane,
-				       struct drm_atomic_state *state)
+static void ast_cursor_plane_helper_atomic_disable(struct drm_plane *plane,
+						   struct drm_atomic_state *state)
 {
 	struct ast_private *ast = to_ast_private(plane->dev);
 
@@ -1204,13 +1196,11 @@ ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 		ast_dp_set_mode(crtc, vbios_mode_info);
 }
 
-static void
-ast_crtc_helper_atomic_enable(struct drm_crtc *crtc,
-			      struct drm_atomic_state *state)
+static void ast_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
 {
 	struct drm_device *dev = crtc->dev;
 	struct ast_private *ast = to_ast_private(dev);
-	struct drm_crtc_state *crtc_state = crtc->state;
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);
 	struct ast_vbios_mode_info *vbios_mode_info =
 		&ast_crtc_state->vbios_mode_info;
@@ -1227,12 +1217,9 @@ ast_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 	ast_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
 }
 
-static void
-ast_crtc_helper_atomic_disable(struct drm_crtc *crtc,
-			       struct drm_atomic_state *state)
+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 drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
 	struct drm_device *dev = crtc->dev;
 	struct ast_private *ast = to_ast_private(dev);
 
@@ -1787,7 +1774,6 @@ int ast_mode_config_init(struct ast_private *ast)
 
 	dev->mode_config.helper_private = &ast_mode_config_helper_funcs;
 
-
 	ret = ast_primary_plane_init(ast);
 	if (ret)
 		return ret;
-- 
2.37.3

[PATCH 7/8] drm/ast: Convert ast to SHMEM

[Thomas Zimmermann]

Replace GEM VRAM helpers with GEM SHMEM helpers in ast. Avoids OOM
errors when allocating video memory. Also adds support for dma-buf
functionality.

Aspeed display hardware supports display resolutions of FullHD and
more at 32-bit pixel depth. But the amount of video memory is in
the range of 8 MiB to 32 MiB, which adds contrains to the actually
available resolutions. As atomic modesetting with VRAM helpers
requires double buffering in video memory, ast fails to pageflip
in some configurations. For example, FullHD with an active cursor
plane does not work on devices with 16 MiB of video memory.

Resolve this problem by converting the ast driver to GEM SHMEM helpers.
Keep the buffer objects in system memory and copy to video memory
on pageflips via shadow-plane helpers. Userspace used to require shadow
planes for decent performance, but that's now provided by the driver.
To replaces the memory management, the patch also implements damage
handling for the primary plane.

The GEM SHMEM helpers, dma-buf import and export is now supported
by ast. This allows easier screen mirroring across devices or with
an Aspeed-based BMC. A corresponding feature request is available
at [1].

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://lore.kernel.org/dri-devel/20220901124451.2523077-1-oushixiong@kylinos.cn/ # [1]
---
 drivers/gpu/drm/ast/Kconfig    |   4 +-
 drivers/gpu/drm/ast/ast_drv.c  |   4 +-
 drivers/gpu/drm/ast/ast_drv.h  |  13 ++-
 drivers/gpu/drm/ast/ast_main.c |   5 +-
 drivers/gpu/drm/ast/ast_mm.c   |  14 +--
 drivers/gpu/drm/ast/ast_mode.c | 204 +++++++++++++++++----------------
 6 files changed, 129 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/ast/Kconfig b/drivers/gpu/drm/ast/Kconfig
index fbcf2f45cef5..d367a90cd3de 100644
--- a/drivers/gpu/drm/ast/Kconfig
+++ b/drivers/gpu/drm/ast/Kconfig
@@ -2,10 +2,8 @@
 config DRM_AST
 	tristate "AST server chips"
 	depends on DRM && PCI && MMU
+	select DRM_GEM_SHMEM_HELPER
 	select DRM_KMS_HELPER
-	select DRM_VRAM_HELPER
-	select DRM_TTM
-	select DRM_TTM_HELPER
 	help
 	 Say yes for experimental AST GPU driver. Do not enable
 	 this driver without having a working -modesetting,
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index b9392f31e629..bbeb5defc8f5 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -33,7 +33,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
-#include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_module.h>
 #include <drm/drm_probe_helper.h>
 
@@ -63,7 +63,7 @@ static const struct drm_driver ast_driver = {
 	.minor = DRIVER_MINOR,
 	.patchlevel = DRIVER_PATCHLEVEL,
 
-	DRM_GEM_VRAM_DRIVER
+	DRM_GEM_SHMEM_DRIVER_OPS
 };
 
 /*
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 02120025b7ac..74f41282444f 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -112,9 +112,9 @@ enum ast_tx_chip {
 struct ast_plane {
 	struct drm_plane base;
 
-	struct drm_gem_vram_object *gbo;
-	struct iosys_map map;
-	u64 off;
+	void __iomem *vaddr;
+	u64 offset;
+	unsigned long size;
 };
 
 static inline struct ast_plane *to_ast_plane(struct drm_plane *plane)
@@ -172,7 +172,12 @@ struct ast_private {
 	uint32_t dram_type;
 	uint32_t mclk;
 
-	struct drm_plane primary_plane;
+	void __iomem	*vram;
+	unsigned long	vram_base;
+	unsigned long	vram_size;
+	unsigned long	vram_fb_available;
+
+	struct ast_plane primary_plane;
 	struct ast_plane cursor_plane;
 	struct drm_crtc crtc;
 	struct {
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 067453266897..bffa310a0431 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -32,7 +32,6 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_gem.h>
-#include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_managed.h>
 
 #include "ast_drv.h"
@@ -461,8 +460,8 @@ struct ast_private *ast_device_create(const struct drm_driver *drv,
 
 	/* map reserved buffer */
 	ast->dp501_fw_buf = NULL;
-	if (dev->vram_mm->vram_size < pci_resource_len(pdev, 0)) {
-		ast->dp501_fw_buf = pci_iomap_range(pdev, 0, dev->vram_mm->vram_size, 0);
+	if (ast->vram_size < pci_resource_len(pdev, 0)) {
+		ast->dp501_fw_buf = pci_iomap_range(pdev, 0, ast->vram_size, 0);
 		if (!ast->dp501_fw_buf)
 			drm_info(dev, "failed to map reserved buffer!\n");
 	}
diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index 6e999408dda9..248284a4b3ff 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -28,7 +28,6 @@
 
 #include <linux/pci.h>
 
-#include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_print.h>
 
@@ -80,7 +79,6 @@ int ast_mm_init(struct ast_private *ast)
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	resource_size_t base, size;
 	u32 vram_size;
-	int ret;
 
 	base = pci_resource_start(pdev, 0);
 	size = pci_resource_len(pdev, 0);
@@ -91,11 +89,13 @@ int ast_mm_init(struct ast_private *ast)
 
 	vram_size = ast_get_vram_size(ast);
 
-	ret = drmm_vram_helper_init(dev, base, vram_size);
-	if (ret) {
-		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
-		return ret;
-	}
+	ast->vram = devm_ioremap_wc(dev->dev, base, vram_size);
+	if (!ast->vram)
+		return -ENOMEM;
+
+	ast->vram_base = base;
+	ast->vram_size = vram_size;
+	ast->vram_fb_available = vram_size;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 59c70fd5b925..1b991658290b 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -36,11 +36,13 @@
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_format_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
-#include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -565,6 +567,29 @@ static void ast_wait_for_vretrace(struct ast_private *ast)
 	} while (!(vgair1 & AST_IO_VGAIR1_VREFRESH) && time_before(jiffies, timeout));
 }
 
+/*
+ * Planes
+ */
+
+static int ast_plane_init(struct drm_device *dev, struct ast_plane *ast_plane,
+			  void __iomem *vaddr, 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)
+{
+	struct drm_plane *plane = &ast_plane->base;
+
+	ast_plane->vaddr = vaddr;
+	ast_plane->offset = offset;
+	ast_plane->size = size;
+
+	return drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
+					formats, format_count, format_modifiers,
+					type, NULL);
+}
+
 /*
  * Primary plane
  */
@@ -607,17 +632,29 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
 	return 0;
 }
 
+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(ast_plane->vaddr);
+
+	iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], fb->format, clip));
+	drm_fb_memcpy(&dst, fb->pitches, src, fb, clip);
+}
+
 static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
 						   struct drm_atomic_state *state)
 {
 	struct drm_device *dev = plane->dev;
 	struct ast_private *ast = to_ast_private(dev);
 	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
 	struct drm_framebuffer *old_fb = old_plane_state->fb;
-	struct drm_gem_vram_object *gbo;
-	s64 gpu_addr;
+	struct ast_plane *ast_plane = to_ast_plane(plane);
+	struct drm_rect damage;
+	struct drm_atomic_helper_damage_iter iter;
 
 	if (!old_fb || (fb->format != old_fb->format)) {
 		struct drm_crtc *crtc = plane_state->crtc;
@@ -629,13 +666,13 @@ static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
 		ast_set_vbios_color_reg(ast, fb->format, vbios_mode_info);
 	}
 
-	gbo = drm_gem_vram_of_gem(fb->obj[0]);
-	gpu_addr = drm_gem_vram_offset(gbo);
-	if (drm_WARN_ON_ONCE(dev, gpu_addr < 0))
-		return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
+	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
+		ast_handle_damage(ast_plane, shadow_plane_state->data, fb, &damage);
+	}
 
 	ast_set_offset_reg(ast, fb);
-	ast_set_start_address_crt1(ast, (u32)gpu_addr);
+	ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
 
 	ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
 }
@@ -649,7 +686,7 @@ static void ast_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 }
 
 static const struct drm_plane_helper_funcs ast_primary_plane_helper_funcs = {
-	DRM_GEM_VRAM_PLANE_HELPER_FUNCS,
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
 	.atomic_check = ast_primary_plane_helper_atomic_check,
 	.atomic_update = ast_primary_plane_helper_atomic_update,
 	.atomic_disable = ast_primary_plane_helper_atomic_disable,
@@ -659,27 +696,30 @@ static const struct drm_plane_funcs ast_primary_plane_funcs = {
 	.update_plane = drm_atomic_helper_update_plane,
 	.disable_plane = drm_atomic_helper_disable_plane,
 	.destroy = drm_plane_cleanup,
-	.reset = drm_atomic_helper_plane_reset,
-	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+	DRM_GEM_SHADOW_PLANE_FUNCS,
 };
 
 static int ast_primary_plane_init(struct ast_private *ast)
 {
 	struct drm_device *dev = &ast->base;
-	struct drm_plane *primary_plane = &ast->primary_plane;
+	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->vram_base;
+	unsigned long cursor_size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
+	unsigned long size = ast->vram_fb_available - cursor_size;
 	int ret;
 
-	ret = drm_universal_plane_init(dev, primary_plane, 0x01,
-				       &ast_primary_plane_funcs,
-				       ast_primary_plane_formats,
-				       ARRAY_SIZE(ast_primary_plane_formats),
-				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	ret = ast_plane_init(dev, ast_primary_plane, vaddr, offset, size,
+			     0x01, &ast_primary_plane_funcs,
+			     ast_primary_plane_formats, ARRAY_SIZE(ast_primary_plane_formats),
+			     NULL, DRM_PLANE_TYPE_PRIMARY);
 	if (ret) {
-		drm_err(dev, "drm_universal_plane_init() failed: %d\n", ret);
+		drm_err(dev, "ast_plane_init() failed: %d\n", ret);
 		return ret;
 	}
 	drm_plane_helper_add(primary_plane, &ast_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(primary_plane);
 
 	return 0;
 }
@@ -828,31 +868,26 @@ static void ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
-	struct drm_framebuffer *old_fb = old_plane_state->fb;
 	struct ast_private *ast = to_ast_private(plane->dev);
-	struct iosys_map dst_map = ast_plane->map;
-	u64 dst_off = ast_plane->off;
 	struct iosys_map src_map = shadow_plane_state->data[0];
+	struct drm_rect damage;
+	const u8 *src = src_map.vaddr; /* TODO: Use mapping abstraction properly */
+	u64 dst_off = ast_plane->offset;
+	u8 __iomem *dst = ast_plane->vaddr; /* 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;
 	u8 x_offset, y_offset;
-	u8 __iomem *dst;
-	u8 __iomem *sig;
-	const u8 *src;
-
-	src = src_map.vaddr; /* TODO: Use mapping abstraction properly */
-	dst = dst_map.vaddr_iomem; /* TODO: Use mapping abstraction properly */
-	sig = dst + AST_HWC_SIZE; /* TODO: Use mapping abstraction properly */
 
 	/*
-	 * Do data transfer to HW cursor BO. If a new cursor image was installed,
-	 * point the scanout engine to dst_gbo's offset and page-flip the HWC buffers.
+	 * Do data transfer to hardware buffer and point the scanout
+	 * engine to the offset.
 	 */
 
-	ast_update_cursor_image(dst, src, fb->width, fb->height);
-
-	if (fb != old_fb)
+	if (drm_atomic_helper_damage_merged(old_plane_state, plane_state, &damage)) {
+		ast_update_cursor_image(dst, src, fb->width, fb->height);
 		ast_set_cursor_base(ast, dst_off);
+	}
 
 	/*
 	 * Update location in HWC signature and registers.
@@ -900,36 +935,22 @@ static const struct drm_plane_helper_funcs ast_cursor_plane_helper_funcs = {
 	.atomic_disable = ast_cursor_plane_helper_atomic_disable,
 };
 
-static void ast_cursor_plane_destroy(struct drm_plane *plane)
-{
-	struct ast_plane *ast_plane = to_ast_plane(plane);
-	struct drm_gem_vram_object *gbo = ast_plane->gbo;
-	struct iosys_map map = ast_plane->map;
-
-	drm_gem_vram_vunmap(gbo, &map);
-	drm_gem_vram_unpin(gbo);
-	drm_gem_vram_put(gbo);
-
-	drm_plane_cleanup(plane);
-}
-
 static const struct drm_plane_funcs ast_cursor_plane_funcs = {
 	.update_plane = drm_atomic_helper_update_plane,
 	.disable_plane = drm_atomic_helper_disable_plane,
-	.destroy = ast_cursor_plane_destroy,
+	.destroy = drm_plane_cleanup,
 	DRM_GEM_SHADOW_PLANE_FUNCS,
 };
 
 static int ast_cursor_plane_init(struct ast_private *ast)
 {
 	struct drm_device *dev = &ast->base;
-	struct ast_plane *ast_plane = &ast->cursor_plane;
-	struct drm_plane *cursor_plane = &ast_plane->base;
+	struct ast_plane *ast_cursor_plane = &ast->cursor_plane;
+	struct drm_plane *cursor_plane = &ast_cursor_plane->base;
 	size_t size;
-	struct drm_gem_vram_object *gbo;
-	struct iosys_map map;
+	void __iomem *vaddr;
+	u64 offset;
 	int ret;
-	s64 off;
 
 	/*
 	 * Allocate backing storage for cursors. The BOs are permanently
@@ -938,52 +959,26 @@ static int ast_cursor_plane_init(struct ast_private *ast)
 
 	size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
 
-	gbo = drm_gem_vram_create(dev, size, 0);
-	if (IS_ERR(gbo))
-		return PTR_ERR(gbo);
-
-	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM |
-				    DRM_GEM_VRAM_PL_FLAG_TOPDOWN);
-	if (ret)
-		goto err_drm_gem_vram_put;
-	ret = drm_gem_vram_vmap(gbo, &map);
-	if (ret)
-		goto err_drm_gem_vram_unpin;
-	off = drm_gem_vram_offset(gbo);
-	if (off < 0) {
-		ret = off;
-		goto err_drm_gem_vram_vunmap;
-	}
+	if (ast->vram_fb_available < size)
+		return -ENOMEM;
 
-	ast_plane->gbo = gbo;
-	ast_plane->map = map;
-	ast_plane->off = off;
-
-	/*
-	 * Create the cursor plane. The plane's destroy callback will release
-	 * the backing storages' BO memory.
-	 */
+	vaddr = ast->vram + ast->vram_fb_available - size;
+	offset = ast->vram_base + ast->vram_fb_available - size;
 
-	ret = drm_universal_plane_init(dev, cursor_plane, 0x01,
-				       &ast_cursor_plane_funcs,
-				       ast_cursor_plane_formats,
-				       ARRAY_SIZE(ast_cursor_plane_formats),
-				       NULL, DRM_PLANE_TYPE_CURSOR, NULL);
+	ret = ast_plane_init(dev, ast_cursor_plane, vaddr, offset, size,
+			     0x01, &ast_cursor_plane_funcs,
+			     ast_cursor_plane_formats, ARRAY_SIZE(ast_cursor_plane_formats),
+			     NULL, DRM_PLANE_TYPE_CURSOR);
 	if (ret) {
-		drm_err(dev, "drm_universal_plane failed(): %d\n", ret);
-		goto err_drm_gem_vram_vunmap;
+		drm_err(dev, "ast_plane_init() failed: %d\n", ret);
+		return ret;
 	}
 	drm_plane_helper_add(cursor_plane, &ast_cursor_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(cursor_plane);
 
-	return 0;
+	ast->vram_fb_available -= size;
 
-err_drm_gem_vram_vunmap:
-	drm_gem_vram_vunmap(gbo, &map);
-err_drm_gem_vram_unpin:
-	drm_gem_vram_unpin(gbo);
-err_drm_gem_vram_put:
-	drm_gem_vram_put(gbo);
-	return ret;
+	return 0;
 }
 
 /*
@@ -1313,7 +1308,7 @@ static int ast_crtc_init(struct drm_device *dev)
 	struct drm_crtc *crtc = &ast->crtc;
 	int ret;
 
-	ret = drm_crtc_init_with_planes(dev, crtc, &ast->primary_plane,
+	ret = drm_crtc_init_with_planes(dev, crtc, &ast->primary_plane.base,
 					&ast->cursor_plane.base, &ast_crtc_funcs,
 					NULL);
 	if (ret)
@@ -1735,9 +1730,27 @@ static const struct drm_mode_config_helper_funcs ast_mode_config_helper_funcs =
 	.atomic_commit_tail = ast_mode_config_helper_atomic_commit_tail,
 };
 
+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 */
+	struct ast_private *ast = to_ast_private(dev);
+	unsigned long fbsize, fbpages, max_fbpages;
+
+	max_fbpages = (ast->vram_fb_available) >> PAGE_SHIFT;
+
+	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
+	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
+
+	if (fbpages > max_fbpages)
+		return MODE_MEM;
+
+	return MODE_OK;
+}
+
 static const struct drm_mode_config_funcs ast_mode_config_funcs = {
-	.fb_create = drm_gem_fb_create,
-	.mode_valid = drm_vram_helper_mode_valid,
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.mode_valid = ast_mode_config_mode_valid,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
@@ -1756,7 +1769,6 @@ int ast_mode_config_init(struct ast_private *ast)
 	dev->mode_config.min_width = 0;
 	dev->mode_config.min_height = 0;
 	dev->mode_config.preferred_depth = 24;
-	dev->mode_config.prefer_shadow = 1;
 	dev->mode_config.fb_base = pci_resource_start(pdev, 0);
 
 	if (ast->chip == AST2100 ||
-- 
2.37.3

Re: [PATCH 7/8] drm/ast: Convert ast to SHMEM

[Jocelyn Falempe]

On 10/10/2022 12:36, Thomas Zimmermann wrote:
> Replace GEM VRAM helpers with GEM SHMEM helpers in ast. Avoids OOM
> errors when allocating video memory. Also adds support for dma-buf
> functionality.
> 
> Aspeed display hardware supports display resolutions of FullHD and
> more at 32-bit pixel depth. But the amount of video memory is in
> the range of 8 MiB to 32 MiB, which adds contrains to the actually
small typo: constraints
> available resolutions. As atomic modesetting with VRAM helpers
> requires double buffering in video memory, ast fails to pageflip
> in some configurations. For example, FullHD with an active cursor
> plane does not work on devices with 16 MiB of video memory.
> 
> Resolve this problem by converting the ast driver to GEM SHMEM helpers.
> Keep the buffer objects in system memory and copy to video memory
> on pageflips via shadow-plane helpers. Userspace used to require shadow
> planes for decent performance, but that's now provided by the driver.
> To replaces the memory management, the patch also implements damage
> handling for the primary plane.
> 
> The GEM SHMEM helpers, dma-buf import and export is now supported
> by ast. This allows easier screen mirroring across devices or with
> an Aspeed-based BMC. A corresponding feature request is available
> at [1].
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://lore.kernel.org/dri-devel/20220901124451.2523077-1-oushixiong@kylinos.cn/ # [1]
> ---
>   drivers/gpu/drm/ast/Kconfig    |   4 +-
>   drivers/gpu/drm/ast/ast_drv.c  |   4 +-
>   drivers/gpu/drm/ast/ast_drv.h  |  13 ++-
>   drivers/gpu/drm/ast/ast_main.c |   5 +-
>   drivers/gpu/drm/ast/ast_mm.c   |  14 +--
>   drivers/gpu/drm/ast/ast_mode.c | 204 +++++++++++++++++----------------
>   6 files changed, 129 insertions(+), 115 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/Kconfig b/drivers/gpu/drm/ast/Kconfig
> index fbcf2f45cef5..d367a90cd3de 100644
> --- a/drivers/gpu/drm/ast/Kconfig
> +++ b/drivers/gpu/drm/ast/Kconfig
> @@ -2,10 +2,8 @@
>   config DRM_AST
>   	tristate "AST server chips"
>   	depends on DRM && PCI && MMU
> +	select DRM_GEM_SHMEM_HELPER
>   	select DRM_KMS_HELPER
> -	select DRM_VRAM_HELPER
> -	select DRM_TTM
> -	select DRM_TTM_HELPER
>   	help
>   	 Say yes for experimental AST GPU driver. Do not enable
>   	 this driver without having a working -modesetting,
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index b9392f31e629..bbeb5defc8f5 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -33,7 +33,7 @@
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_crtc_helper.h>
>   #include <drm/drm_drv.h>
> -#include <drm/drm_gem_vram_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>   #include <drm/drm_module.h>
>   #include <drm/drm_probe_helper.h>
>   
> @@ -63,7 +63,7 @@ static const struct drm_driver ast_driver = {
>   	.minor = DRIVER_MINOR,
>   	.patchlevel = DRIVER_PATCHLEVEL,
>   
> -	DRM_GEM_VRAM_DRIVER
> +	DRM_GEM_SHMEM_DRIVER_OPS
>   };
>   
>   /*
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 02120025b7ac..74f41282444f 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -112,9 +112,9 @@ enum ast_tx_chip {
>   struct ast_plane {
>   	struct drm_plane base;
>   
> -	struct drm_gem_vram_object *gbo;
> -	struct iosys_map map;
> -	u64 off;
> +	void __iomem *vaddr;
> +	u64 offset;
> +	unsigned long size;
>   };
>   
>   static inline struct ast_plane *to_ast_plane(struct drm_plane *plane)
> @@ -172,7 +172,12 @@ struct ast_private {
>   	uint32_t dram_type;
>   	uint32_t mclk;
>   
> -	struct drm_plane primary_plane;
> +	void __iomem	*vram;
> +	unsigned long	vram_base;
> +	unsigned long	vram_size;
> +	unsigned long	vram_fb_available;
> +
> +	struct ast_plane primary_plane;
>   	struct ast_plane cursor_plane;
>   	struct drm_crtc crtc;
>   	struct {
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 067453266897..bffa310a0431 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -32,7 +32,6 @@
>   #include <drm/drm_crtc_helper.h>
>   #include <drm/drm_drv.h>
>   #include <drm/drm_gem.h>
> -#include <drm/drm_gem_vram_helper.h>
>   #include <drm/drm_managed.h>
>   
>   #include "ast_drv.h"
> @@ -461,8 +460,8 @@ struct ast_private *ast_device_create(const struct drm_driver *drv,
>   
>   	/* map reserved buffer */
>   	ast->dp501_fw_buf = NULL;
> -	if (dev->vram_mm->vram_size < pci_resource_len(pdev, 0)) {
> -		ast->dp501_fw_buf = pci_iomap_range(pdev, 0, dev->vram_mm->vram_size, 0);
> +	if (ast->vram_size < pci_resource_len(pdev, 0)) {
> +		ast->dp501_fw_buf = pci_iomap_range(pdev, 0, ast->vram_size, 0);
>   		if (!ast->dp501_fw_buf)
>   			drm_info(dev, "failed to map reserved buffer!\n");
>   	}
> diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
> index 6e999408dda9..248284a4b3ff 100644
> --- a/drivers/gpu/drm/ast/ast_mm.c
> +++ b/drivers/gpu/drm/ast/ast_mm.c
> @@ -28,7 +28,6 @@
>   
>   #include <linux/pci.h>
>   
> -#include <drm/drm_gem_vram_helper.h>
>   #include <drm/drm_managed.h>
>   #include <drm/drm_print.h>
>   
> @@ -80,7 +79,6 @@ int ast_mm_init(struct ast_private *ast)
>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
>   	resource_size_t base, size;
>   	u32 vram_size;
> -	int ret;
>   
>   	base = pci_resource_start(pdev, 0);
>   	size = pci_resource_len(pdev, 0);
> @@ -91,11 +89,13 @@ int ast_mm_init(struct ast_private *ast)
>   
>   	vram_size = ast_get_vram_size(ast);
>   
> -	ret = drmm_vram_helper_init(dev, base, vram_size);
> -	if (ret) {
> -		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
> -		return ret;
> -	}
> +	ast->vram = devm_ioremap_wc(dev->dev, base, vram_size);
> +	if (!ast->vram)
> +		return -ENOMEM;
> +
> +	ast->vram_base = base;
> +	ast->vram_size = vram_size;
> +	ast->vram_fb_available = vram_size;
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 59c70fd5b925..1b991658290b 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -36,11 +36,13 @@
>   #include <drm/drm_atomic_state_helper.h>
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_damage_helper.h>
>   #include <drm/drm_edid.h>
> +#include <drm/drm_format_helper.h>
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_gem_atomic_helper.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
> -#include <drm/drm_gem_vram_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>   #include <drm/drm_managed.h>
>   #include <drm/drm_probe_helper.h>
>   #include <drm/drm_simple_kms_helper.h>
> @@ -565,6 +567,29 @@ static void ast_wait_for_vretrace(struct ast_private *ast)
>   	} while (!(vgair1 & AST_IO_VGAIR1_VREFRESH) && time_before(jiffies, timeout));
>   }
>   
> +/*
> + * Planes
> + */
> +
> +static int ast_plane_init(struct drm_device *dev, struct ast_plane *ast_plane,
> +			  void __iomem *vaddr, 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)
> +{
> +	struct drm_plane *plane = &ast_plane->base;
> +
> +	ast_plane->vaddr = vaddr;
> +	ast_plane->offset = offset;
> +	ast_plane->size = size;
> +
> +	return drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
> +					formats, format_count, format_modifiers,
> +					type, NULL);
> +}
> +
>   /*
>    * Primary plane
>    */
> @@ -607,17 +632,29 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>   	return 0;
>   }
>   
> +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(ast_plane->vaddr);
> +
> +	iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], fb->format, clip));
> +	drm_fb_memcpy(&dst, fb->pitches, src, fb, clip);
> +}
> +
>   static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   						   struct drm_atomic_state *state)
>   {
>   	struct drm_device *dev = plane->dev;
>   	struct ast_private *ast = to_ast_private(dev);
>   	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>   	struct drm_framebuffer *fb = plane_state->fb;
>   	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>   	struct drm_framebuffer *old_fb = old_plane_state->fb;
> -	struct drm_gem_vram_object *gbo;
> -	s64 gpu_addr;
> +	struct ast_plane *ast_plane = to_ast_plane(plane);
> +	struct drm_rect damage;
> +	struct drm_atomic_helper_damage_iter iter;
>   
>   	if (!old_fb || (fb->format != old_fb->format)) {
>   		struct drm_crtc *crtc = plane_state->crtc;
> @@ -629,13 +666,13 @@ static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   		ast_set_vbios_color_reg(ast, fb->format, vbios_mode_info);
>   	}
>   
> -	gbo = drm_gem_vram_of_gem(fb->obj[0]);
> -	gpu_addr = drm_gem_vram_offset(gbo);
> -	if (drm_WARN_ON_ONCE(dev, gpu_addr < 0))
> -		return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
> +	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
> +		ast_handle_damage(ast_plane, shadow_plane_state->data, fb, &damage);
> +	}
>   
>   	ast_set_offset_reg(ast, fb);
> -	ast_set_start_address_crt1(ast, (u32)gpu_addr);
> +	ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
>   
>   	ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
>   }
> @@ -649,7 +686,7 @@ static void ast_primary_plane_helper_atomic_disable(struct drm_plane *plane,
>   }
>   
>   static const struct drm_plane_helper_funcs ast_primary_plane_helper_funcs = {
> -	DRM_GEM_VRAM_PLANE_HELPER_FUNCS,
> +	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>   	.atomic_check = ast_primary_plane_helper_atomic_check,
>   	.atomic_update = ast_primary_plane_helper_atomic_update,
>   	.atomic_disable = ast_primary_plane_helper_atomic_disable,
> @@ -659,27 +696,30 @@ static const struct drm_plane_funcs ast_primary_plane_funcs = {
>   	.update_plane = drm_atomic_helper_update_plane,
>   	.disable_plane = drm_atomic_helper_disable_plane,
>   	.destroy = drm_plane_cleanup,
> -	.reset = drm_atomic_helper_plane_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> +	DRM_GEM_SHADOW_PLANE_FUNCS,
>   };
>   
>   static int ast_primary_plane_init(struct ast_private *ast)
>   {
>   	struct drm_device *dev = &ast->base;
> -	struct drm_plane *primary_plane = &ast->primary_plane;
> +	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->vram_base;
> +	unsigned long cursor_size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
> +	unsigned long size = ast->vram_fb_available - cursor_size;
>   	int ret;
>   
> -	ret = drm_universal_plane_init(dev, primary_plane, 0x01,
> -				       &ast_primary_plane_funcs,
> -				       ast_primary_plane_formats,
> -				       ARRAY_SIZE(ast_primary_plane_formats),
> -				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> +	ret = ast_plane_init(dev, ast_primary_plane, vaddr, offset, size,
> +			     0x01, &ast_primary_plane_funcs,
> +			     ast_primary_plane_formats, ARRAY_SIZE(ast_primary_plane_formats),
> +			     NULL, DRM_PLANE_TYPE_PRIMARY);
>   	if (ret) {
> -		drm_err(dev, "drm_universal_plane_init() failed: %d\n", ret);
> +		drm_err(dev, "ast_plane_init() failed: %d\n", ret);
>   		return ret;
>   	}
>   	drm_plane_helper_add(primary_plane, &ast_primary_plane_helper_funcs);
> +	drm_plane_enable_fb_damage_clips(primary_plane);
>   
>   	return 0;
>   }
> @@ -828,31 +868,26 @@ static void ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
>   	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>   	struct drm_framebuffer *fb = plane_state->fb;
>   	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> -	struct drm_framebuffer *old_fb = old_plane_state->fb;
>   	struct ast_private *ast = to_ast_private(plane->dev);
> -	struct iosys_map dst_map = ast_plane->map;
> -	u64 dst_off = ast_plane->off;
>   	struct iosys_map src_map = shadow_plane_state->data[0];
> +	struct drm_rect damage;
> +	const u8 *src = src_map.vaddr; /* TODO: Use mapping abstraction properly */
> +	u64 dst_off = ast_plane->offset;
> +	u8 __iomem *dst = ast_plane->vaddr; /* 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;
>   	u8 x_offset, y_offset;
> -	u8 __iomem *dst;
> -	u8 __iomem *sig;
> -	const u8 *src;
> -
> -	src = src_map.vaddr; /* TODO: Use mapping abstraction properly */
> -	dst = dst_map.vaddr_iomem; /* TODO: Use mapping abstraction properly */
> -	sig = dst + AST_HWC_SIZE; /* TODO: Use mapping abstraction properly */
>   
>   	/*
> -	 * Do data transfer to HW cursor BO. If a new cursor image was installed,
> -	 * point the scanout engine to dst_gbo's offset and page-flip the HWC buffers.
> +	 * Do data transfer to hardware buffer and point the scanout
> +	 * engine to the offset.
>   	 */
>   
> -	ast_update_cursor_image(dst, src, fb->width, fb->height);
> -
> -	if (fb != old_fb)
> +	if (drm_atomic_helper_damage_merged(old_plane_state, plane_state, &damage)) {
> +		ast_update_cursor_image(dst, src, fb->width, fb->height);
>   		ast_set_cursor_base(ast, dst_off);
> +	}
>   
>   	/*
>   	 * Update location in HWC signature and registers.
> @@ -900,36 +935,22 @@ static const struct drm_plane_helper_funcs ast_cursor_plane_helper_funcs = {
>   	.atomic_disable = ast_cursor_plane_helper_atomic_disable,
>   };
>   
> -static void ast_cursor_plane_destroy(struct drm_plane *plane)
> -{
> -	struct ast_plane *ast_plane = to_ast_plane(plane);
> -	struct drm_gem_vram_object *gbo = ast_plane->gbo;
> -	struct iosys_map map = ast_plane->map;
> -
> -	drm_gem_vram_vunmap(gbo, &map);
> -	drm_gem_vram_unpin(gbo);
> -	drm_gem_vram_put(gbo);
> -
> -	drm_plane_cleanup(plane);
> -}
> -
>   static const struct drm_plane_funcs ast_cursor_plane_funcs = {
>   	.update_plane = drm_atomic_helper_update_plane,
>   	.disable_plane = drm_atomic_helper_disable_plane,
> -	.destroy = ast_cursor_plane_destroy,
> +	.destroy = drm_plane_cleanup,
>   	DRM_GEM_SHADOW_PLANE_FUNCS,
>   };
>   
>   static int ast_cursor_plane_init(struct ast_private *ast)
>   {
>   	struct drm_device *dev = &ast->base;
> -	struct ast_plane *ast_plane = &ast->cursor_plane;
> -	struct drm_plane *cursor_plane = &ast_plane->base;
> +	struct ast_plane *ast_cursor_plane = &ast->cursor_plane;
> +	struct drm_plane *cursor_plane = &ast_cursor_plane->base;
>   	size_t size;
> -	struct drm_gem_vram_object *gbo;
> -	struct iosys_map map;
> +	void __iomem *vaddr;
> +	u64 offset;
>   	int ret;
> -	s64 off;
>   
>   	/*
>   	 * Allocate backing storage for cursors. The BOs are permanently
> @@ -938,52 +959,26 @@ static int ast_cursor_plane_init(struct ast_private *ast)
>   
>   	size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
>   
> -	gbo = drm_gem_vram_create(dev, size, 0);
> -	if (IS_ERR(gbo))
> -		return PTR_ERR(gbo);
> -
> -	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM |
> -				    DRM_GEM_VRAM_PL_FLAG_TOPDOWN);
> -	if (ret)
> -		goto err_drm_gem_vram_put;
> -	ret = drm_gem_vram_vmap(gbo, &map);
> -	if (ret)
> -		goto err_drm_gem_vram_unpin;
> -	off = drm_gem_vram_offset(gbo);
> -	if (off < 0) {
> -		ret = off;
> -		goto err_drm_gem_vram_vunmap;
> -	}
> +	if (ast->vram_fb_available < size)
> +		return -ENOMEM;
>   
> -	ast_plane->gbo = gbo;
> -	ast_plane->map = map;
> -	ast_plane->off = off;
> -
> -	/*
> -	 * Create the cursor plane. The plane's destroy callback will release
> -	 * the backing storages' BO memory.
> -	 */
> +	vaddr = ast->vram + ast->vram_fb_available - size;
> +	offset = ast->vram_base + ast->vram_fb_available - size;
>   
> -	ret = drm_universal_plane_init(dev, cursor_plane, 0x01,
> -				       &ast_cursor_plane_funcs,
> -				       ast_cursor_plane_formats,
> -				       ARRAY_SIZE(ast_cursor_plane_formats),
> -				       NULL, DRM_PLANE_TYPE_CURSOR, NULL);
> +	ret = ast_plane_init(dev, ast_cursor_plane, vaddr, offset, size,
> +			     0x01, &ast_cursor_plane_funcs,
> +			     ast_cursor_plane_formats, ARRAY_SIZE(ast_cursor_plane_formats),
> +			     NULL, DRM_PLANE_TYPE_CURSOR);
>   	if (ret) {
> -		drm_err(dev, "drm_universal_plane failed(): %d\n", ret);
> -		goto err_drm_gem_vram_vunmap;
> +		drm_err(dev, "ast_plane_init() failed: %d\n", ret);
> +		return ret;
>   	}
>   	drm_plane_helper_add(cursor_plane, &ast_cursor_plane_helper_funcs);
> +	drm_plane_enable_fb_damage_clips(cursor_plane);
>   
> -	return 0;
> +	ast->vram_fb_available -= size;
>   
> -err_drm_gem_vram_vunmap:
> -	drm_gem_vram_vunmap(gbo, &map);
> -err_drm_gem_vram_unpin:
> -	drm_gem_vram_unpin(gbo);
> -err_drm_gem_vram_put:
> -	drm_gem_vram_put(gbo);
> -	return ret;
> +	return 0;
>   }
>   
>   /*
> @@ -1313,7 +1308,7 @@ static int ast_crtc_init(struct drm_device *dev)
>   	struct drm_crtc *crtc = &ast->crtc;
>   	int ret;
>   
> -	ret = drm_crtc_init_with_planes(dev, crtc, &ast->primary_plane,
> +	ret = drm_crtc_init_with_planes(dev, crtc, &ast->primary_plane.base,
>   					&ast->cursor_plane.base, &ast_crtc_funcs,
>   					NULL);
>   	if (ret)
> @@ -1735,9 +1730,27 @@ static const struct drm_mode_config_helper_funcs ast_mode_config_helper_funcs =
>   	.atomic_commit_tail = ast_mode_config_helper_atomic_commit_tail,
>   };
>   
> +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 */
> +	struct ast_private *ast = to_ast_private(dev);
> +	unsigned long fbsize, fbpages, max_fbpages;
> +
> +	max_fbpages = (ast->vram_fb_available) >> PAGE_SHIFT;
> +
> +	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
> +	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
> +
> +	if (fbpages > max_fbpages)
> +		return MODE_MEM;
> +
> +	return MODE_OK;
> +}
> +
>   static const struct drm_mode_config_funcs ast_mode_config_funcs = {
> -	.fb_create = drm_gem_fb_create,
> -	.mode_valid = drm_vram_helper_mode_valid,
> +	.fb_create = drm_gem_fb_create_with_dirty,
> +	.mode_valid = ast_mode_config_mode_valid,
>   	.atomic_check = drm_atomic_helper_check,
>   	.atomic_commit = drm_atomic_helper_commit,
>   };
> @@ -1756,7 +1769,6 @@ int ast_mode_config_init(struct ast_private *ast)
>   	dev->mode_config.min_width = 0;
>   	dev->mode_config.min_height = 0;
>   	dev->mode_config.preferred_depth = 24;
> -	dev->mode_config.prefer_shadow = 1;
>   	dev->mode_config.fb_base = pci_resource_start(pdev, 0);
>   
>   	if (ast->chip == AST2100 ||

Best regards,

-- 

Jocelyn

[PATCH 8/8] drm/ast: Avoid reprogramming primary-plane scanout address

[Thomas Zimmermann]

Some AST-based BMCs stop display output for up to 5 seconds after
reprogramming the scanout address. As the address is fixed, avoid
re-setting the address' value.

Reported-by: Jocelyn Falempe <jfalempe@redhat.com>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 1b991658290b..54a9643d86ce 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -672,9 +672,17 @@ static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	}
 
 	ast_set_offset_reg(ast, fb);
-	ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
 
-	ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
+	/*
+	 * Some BMCs stop scanning out the video signal after the driver
+	 * reprogrammed the scanout address. This stalls display output
+	 * for several seconds and makes the display unusable. Therefore
+	 * only reprogram the address after enabling the plane.
+	 */
+	if (!old_fb && fb) {
+		ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
+		ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
+	}
 }
 
 static void ast_primary_plane_helper_atomic_disable(struct drm_plane *plane,
-- 
2.37.3

Re: [PATCH 8/8] drm/ast: Avoid reprogramming primary-plane scanout address

[Jocelyn Falempe]

On 10/10/2022 12:36, Thomas Zimmermann wrote:
> Some AST-based BMCs stop display output for up to 5 seconds after
> reprogramming the scanout address. As the address is fixed, avoid
> re-setting the address' value.
> 
> Reported-by: Jocelyn Falempe <jfalempe@redhat.com>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_mode.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 1b991658290b..54a9643d86ce 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -672,9 +672,17 @@ static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   	}
>   
>   	ast_set_offset_reg(ast, fb);
> -	ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
>   
> -	ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
> +	/*
> +	 * Some BMCs stop scanning out the video signal after the driver
> +	 * reprogrammed the scanout address. This stalls display output
> +	 * for several seconds and makes the display unusable. Therefore
> +	 * only reprogram the address after enabling the plane.
> +	 */
> +	if (!old_fb && fb) {
> +		ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
> +		ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
> +	}
>   }

I've tested the series, and BMC is still very slow with Gnome/Wayland.

It's because ast_set_offset_reg() also trigger a 5s freeze of the BMC.

I added this, and it works well:

if (!old_fb || old_fb->pitches[0] != fb->pitches[0])
     ast_set_offset_reg(ast, fb);


>   
>   static void ast_primary_plane_helper_atomic_disable(struct drm_plane *plane,




-- 

Jocelyn

Re: [PATCH 8/8] drm/ast: Avoid reprogramming primary-plane scanout address

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2349 bytes --]

Hi

Am 11.10.22 um 16:21 schrieb Jocelyn Falempe:
> On 10/10/2022 12:36, Thomas Zimmermann wrote:
>> Some AST-based BMCs stop display output for up to 5 seconds after
>> reprogramming the scanout address. As the address is fixed, avoid
>> re-setting the address' value.
>>
>> Reported-by: Jocelyn Falempe <jfalempe@redhat.com>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/ast/ast_mode.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>> b/drivers/gpu/drm/ast/ast_mode.c
>> index 1b991658290b..54a9643d86ce 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -672,9 +672,17 @@ static void 
>> ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>       }
>>       ast_set_offset_reg(ast, fb);
>> -    ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
>> -    ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
>> +    /*
>> +     * Some BMCs stop scanning out the video signal after the driver
>> +     * reprogrammed the scanout address. This stalls display output
>> +     * for several seconds and makes the display unusable. Therefore
>> +     * only reprogram the address after enabling the plane.
>> +     */
>> +    if (!old_fb && fb) {
>> +        ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
>> +        ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
>> +    }
>>   }
> 
> I've tested the series, and BMC is still very slow with Gnome/Wayland.
> 
> It's because ast_set_offset_reg() also trigger a 5s freeze of the BMC.
> 
> I added this, and it works well:
> 
> if (!old_fb || old_fb->pitches[0] != fb->pitches[0])
>      ast_set_offset_reg(ast, fb);

Great thanks for testing. I'll add this to the next version.

I wonder if that problem is in all ast chips or just this one. :/

Best regards
Thomas

> 
> 
>>   static void ast_primary_plane_helper_atomic_disable(struct drm_plane 
>> *plane,
> 
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 8/8] drm/ast: Avoid reprogramming primary-plane scanout address

[Jocelyn Falempe]

On 11/10/2022 16:59, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.10.22 um 16:21 schrieb Jocelyn Falempe:
>> On 10/10/2022 12:36, Thomas Zimmermann wrote:
>>> Some AST-based BMCs stop display output for up to 5 seconds after
>>> reprogramming the scanout address. As the address is fixed, avoid
>>> re-setting the address' value.
>>>
>>> Reported-by: Jocelyn Falempe <jfalempe@redhat.com>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/gpu/drm/ast/ast_mode.c | 12 ++++++++++--
>>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>>> b/drivers/gpu/drm/ast/ast_mode.c
>>> index 1b991658290b..54a9643d86ce 100644
>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>> @@ -672,9 +672,17 @@ static void 
>>> ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>>       }
>>>       ast_set_offset_reg(ast, fb);
>>> -    ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
>>> -    ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
>>> +    /*
>>> +     * Some BMCs stop scanning out the video signal after the driver
>>> +     * reprogrammed the scanout address. This stalls display output
>>> +     * for several seconds and makes the display unusable. Therefore
>>> +     * only reprogram the address after enabling the plane.
>>> +     */
>>> +    if (!old_fb && fb) {
>>> +        ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
>>> +        ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
>>> +    }
>>>   }
>>
>> I've tested the series, and BMC is still very slow with Gnome/Wayland.
>>
>> It's because ast_set_offset_reg() also trigger a 5s freeze of the BMC.
>>
>> I added this, and it works well:
>>
>> if (!old_fb || old_fb->pitches[0] != fb->pitches[0])
>>      ast_set_offset_reg(ast, fb);
> 
> Great thanks for testing. I'll add this to the next version.
> 
> I wonder if that problem is in all ast chips or just this one. :/

My test machine has an AST2600, but I think it depends mostly on the BMC 
firmware.
Anyway it's not useful to reprogram the same value on the registers for 
each frame. And that's the good thing about atomic interface, because we 
have previous and next state, it's easy to check if the registers need 
to be updated.

> 
> Best regards
> Thomas
> 
>>
>>
>>>   static void ast_primary_plane_helper_atomic_disable(struct 
>>> drm_plane *plane,
>>
>>
>>
>>
> 

-- 

Jocelyn

Re: [PATCH 0/8] drm/ast: Convert ast driver to SHMEM

[Jocelyn Falempe]

Thanks a lot for your series. This solves a big performance impact when 
using Gnome/Wayland on some Aspeed chip, with BMC.
This also remove the need for "shadowFB" workaround in userspace.

With the small change in patch 8, and the two typo's,
the whole series is

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

Best regards,

-- 

Jocelyn

On 10/10/2022 12:36, Thomas Zimmermann wrote:
> This patchset converts ast to GEM SHMEM helpers. Fixes problems with
> memory allocation and BMC scanout updates.
> 
> Patches 1 to 3 are fixes for various minor problems in the ast driver.
> We should merge them even without SHMEM support.
> 
> Patches 4 to 6 prepare the driver for the conversion. The cursor double
> buffering is not required and prevents reuse among the plane code. Style
> issues are being fixed separately from the conversion.
> 
> The conversion itself is in patch 7. Not only does it fix problems with
> memory allocation, it also brings back several high-res display modes
> that got lost during the ast driver's conversion to atomic modesetting.
> There was an earlier RFC patch of this code that had issues with
> rendering performnce. [1] We've meanwhile improved these areas and
> performance was acceptable on the test systems.
> 
> With SHMEM in place, the scanout address for the primary plane does not
> have to be changed often. Patch 8 fixes a performance problem where the
> BMC output freezes for several seconds after reprogramming the scanout
> address.
> 
> Tested on AST 2100 and 2300 with fbdev emulation, weston, and Gnome in
> X11 and Wayland mode.
> 
> [1] https://lore.kernel.org/dri-devel/5a3537c3-2c81-b9de-e4c7-c00577cdd43d@suse.de/
> 
> Thomas Zimmermann (8):
>    drm/ast: Acquire I/O-register lock in atomic_commit_tail function
>    drm/ast: Call drm_atomic_helper_check_plane_state() unconditionally
>    drm/ast: Do not call drm_atomic_add_affected_planes()
>    drm/ast: Remove cursor double buffering
>    drm/ast: Rename struct ast_cursor_plane to struct ast_plane
>    drm/ast: Style cleanups in plane code
>    drm/ast: Convert ast to SHMEM
>    drm/ast: Avoid reprogramming primary-plane scanout address
> 
>   drivers/gpu/drm/ast/Kconfig    |   4 +-
>   drivers/gpu/drm/ast/ast_drv.c  |   4 +-
>   drivers/gpu/drm/ast/ast_drv.h  |  34 +--
>   drivers/gpu/drm/ast/ast_main.c |   5 +-
>   drivers/gpu/drm/ast/ast_mm.c   |  14 +-
>   drivers/gpu/drm/ast/ast_mode.c | 399 ++++++++++++++++-----------------
>   6 files changed, 219 insertions(+), 241 deletions(-)
> 
> 
> base-commit: 74e2443e7681e4d442b45f551ddf12d09a6f00c3