[PATCH v3 0/4] drm/hibmc: Fix plane helpers and convert to gem-shmem

[PATCH v3 0/4] drm/hibmc: Fix plane helpers and convert to gem-shmem

[Thomas Zimmermann]

Convert hibmc's memory management from gem-vram ot gem-shmem. Fix a
number of bugs in preparation.

Patches 1 to 3 fix various bugs in the plane code.

Patch 4 replaces gem-vram with gem-shmem. The rational is that gem-vram
has corner cases on low-end devices and is limited in functionality. The
commit messages state a list of possible issues.

Switching to gem-shmem avoids these problems. It also enables buffer
sharing via dma_buf, which improves the hibmc driver's integration with
the overall graphics ecosystem.

We've done these changes on drivers for similar hardware, such as ast and
mgag200. Hibmc is the last of these drivers to run on gem-shmem. Gem-vram
instead is deprecated and can hopefully be removed in a later release.

Tested on an RH1288 v3 system.

v3:
- fix coding style
- rebase on display-detection fixes
v2:
- extend commit message of patch 2 (Yongbang)
- so not select TTM any longer

Thomas Zimmermann (4):
  drm/hibmc: Use drm_atomic_helper_check_plane_state()
  drm/hibmc: Fix list of formats on the primary plane
  drm/hibmc: Do not use cpp from struct drm_format_info
  drm/hibmc: Use gem-shmem with shadow-plane helpers for memory
    management

 drivers/gpu/drm/drm_gem_shmem_helper.c        |  22 +++-
 drivers/gpu/drm/hisilicon/hibmc/Kconfig       |   4 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 110 +++++++++---------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  62 +++++++---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   5 +
 include/drm/drm_gem_shmem_helper.h            |   4 +
 6 files changed, 129 insertions(+), 78 deletions(-)


base-commit: 4f554688dffcacf48630c14f9fb77a9f60394c1c
-- 
2.54.0

[PATCH v3 1/4] drm/hibmc: Use drm_atomic_helper_check_plane_state()

[Thomas Zimmermann]

Call drm_atomic_helper_check_plane_state() from the primary plane's
atomic-check helper and replace the custom implementation.

All plane's implementations of atomic_check should call the shared
_check_plane_state() helper first. It adjusts the plane state for
correct positioning, rotation and scaling of the plane. Do this
even if the plane's CRTC has been disabled by setting the parameter
can_update_disabled. The original code returned early in this case,
but it's safe to so and cleaner to have all plane state initialized.

As we don't set can_position, drm_atomic_helper_check_plane_state()'s
visibility check tests if the plane covers all of the CRTC. This is
a small change from the original code, which tested if the plane is
exactly the size of the CRTC. With the new test, the plane still has
to cover all of the CRTC, but can be larger than the CRTC's size. A
later patch can fully implement this feature in hibmc.

If the plane is disabled, the helper clears the visibility flag in the
plane state. On errors or if the plane is not visible, the atomic-check
helper can return early. Implement all this in hibmc and drop the custom
code that does some of it.

v2:
- extend the commit description (Yongbang)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
Reviewed-by: Yongbang Shi <shiyongbang@huawei.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Baihan Li <libaihan@huawei.com>
Cc: Yongbang Shi <shiyongbang@huawei.com>
Cc: <stable@vger.kernel.org> # v4.10+
---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 46 ++++++-------------
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index 3066dc9ebc64..7c0b88c774b5 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -72,46 +72,28 @@ static int hibmc_get_best_clock_idx(const struct drm_display_mode *mode)
 static int hibmc_plane_atomic_check(struct drm_plane *plane,
 				    struct drm_atomic_commit *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 *crtc = new_plane_state->crtc;
-	struct drm_crtc_state *crtc_state;
-	u32 src_w = new_plane_state->src_w >> 16;
-	u32 src_h = new_plane_state->src_h >> 16;
-
-	if (!crtc || !fb)
-		return 0;
+	struct drm_plane_state *new_plane_state =
+		drm_atomic_get_new_plane_state(state, plane);
+	struct drm_crtc_state *new_crtc_state = NULL;
+	int ret;
 
-	crtc_state = drm_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
+	if (new_plane_state->crtc)
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
 
-	if (src_w != new_plane_state->crtc_w || src_h != new_plane_state->crtc_h) {
-		drm_dbg_atomic(plane->dev, "scale not support\n");
-		return -EINVAL;
-	}
-
-	if (new_plane_state->crtc_x < 0 || new_plane_state->crtc_y < 0) {
-		drm_dbg_atomic(plane->dev, "crtc_x/y of drm_plane state is invalid\n");
-		return -EINVAL;
-	}
-
-	if (!crtc_state->enable)
+	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+						  DRM_PLANE_NO_SCALING,
+						  DRM_PLANE_NO_SCALING,
+						  false, true);
+	if (ret)
+		return ret;
+	else if (!new_plane_state->visible)
 		return 0;
 
-	if (new_plane_state->crtc_x + new_plane_state->crtc_w >
-	    crtc_state->adjusted_mode.hdisplay ||
-	    new_plane_state->crtc_y + new_plane_state->crtc_h >
-	    crtc_state->adjusted_mode.vdisplay) {
-		drm_dbg_atomic(plane->dev, "visible portion of plane is invalid\n");
-		return -EINVAL;
-	}
-
 	if (new_plane_state->fb->pitches[0] % 128 != 0) {
 		drm_dbg_atomic(plane->dev, "wrong stride with 128-byte aligned\n");
 		return -EINVAL;
 	}
+
 	return 0;
 }
 
-- 
2.54.0

[PATCH v3 2/4] drm/hibmc: Fix list of formats on the primary plane

[Thomas Zimmermann]

Remove all formats from the primary plane that are unsupported for
various reasons.

* Formats with alpha channel: planes should not announce alpha channels
unless they support transparency. There's no transparency support in
the primary plane's implementation.

* Formats with BGR order. The common format is in RGB channel order.
There's no BGR support in the primary plane's implementation.

* RGB888: atomic_update programs the format from cpp[0] * 8 / 16. For
RGB888's cpp value of 3 this returns 1.5; rounded to 1. Programming
the value of 1 to HIBMC_CRT_DISP_CTL_FORMAT sets up RGB565. Hence, the
output is distorted. This can be tested by booting with video=1024x768-24.

Removing all unsupported formats leaves XRGB8888 and RGB565. Both of
which are supported and work correctly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
Reviewed-by: Yongbang Shi <shiyongbang@huawei.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Yongbang Shi <shiyongbang@huawei.com>
Cc: Baihan Li <libaihan@huawei.com>
Cc: <stable@vger.kernel.org> # v4.10+
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index 7c0b88c774b5..2e6e189bec1a 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -135,10 +135,8 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
 }
 
 static const u32 channel_formats1[] = {
-	DRM_FORMAT_RGB565, DRM_FORMAT_BGR565, DRM_FORMAT_RGB888,
-	DRM_FORMAT_BGR888, DRM_FORMAT_XRGB8888, DRM_FORMAT_XBGR8888,
-	DRM_FORMAT_RGBA8888, DRM_FORMAT_BGRA8888, DRM_FORMAT_ARGB8888,
-	DRM_FORMAT_ABGR8888
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_RGB565,
 };
 
 static const struct drm_plane_funcs hibmc_plane_funcs = {
-- 
2.54.0

[PATCH v3 3/4] drm/hibmc: Do not use cpp from struct drm_format_info

[Thomas Zimmermann]

Replace uses of struct drm_format_info's cpp with appropriate interfaces.
The cpp field contains the characters per pixel. It is deprecated and
should be avoided.

Calculate the line width in bytes with drm_format_info_min_pitch(). This
is the preferred way of getting pixel and line sizes.

Program HIB_CRT_DISP_CTL_FORMAT from the format's 4CC code instead of
calculating the field's value from the cpp.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index 2e6e189bec1a..79c33c778d2c 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -100,8 +100,8 @@ static int hibmc_plane_atomic_check(struct drm_plane *plane,
 static void hibmc_plane_atomic_update(struct drm_plane *plane,
 				      struct drm_atomic_commit *state)
 {
-	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
-									   plane);
+	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_framebuffer *fb = new_state->fb;
 	u32 reg;
 	s64 gpu_addr = 0;
 	u32 line_l;
@@ -119,7 +119,7 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
 
 	writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
 
-	reg = new_state->fb->width * (new_state->fb->format->cpp[0]);
+	reg = drm_format_info_min_pitch(fb->format, 0, fb->width);
 
 	line_l = new_state->fb->pitches[0];
 	writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
@@ -129,8 +129,14 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
 	/* SET PIXEL FORMAT */
 	reg = readl(priv->mmio + HIBMC_CRT_DISP_CTL);
 	reg &= ~HIBMC_CRT_DISP_CTL_FORMAT_MASK;
-	reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT,
-			   new_state->fb->format->cpp[0] * 8 / 16);
+	switch (fb->format->format) {
+	case DRM_FORMAT_XRGB8888:
+		reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT, 2);
+		break;
+	case DRM_FORMAT_RGB565:
+		reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT, 1);
+		break;
+	}
 	writel(reg, priv->mmio + HIBMC_CRT_DISP_CTL);
 }
 
-- 
2.54.0

[PATCH v3 4/4] drm/hibmc: Use gem-shmem with shadow-plane helpers for memory management

[Thomas Zimmermann]

Replace the gem-vram memory manager with gem-shmem. Makes the driver more
robust and enables dma-buf sharing with other hardware.

Gem-vram was created from various drivers that used TTM for their memory
management. All these drivers have meanwhile been converted to gem-shmem.
Using gem-vram is deprecated because it has several problems.

* TTM requires significant overcommitment of video memory for reliable
page flips. There needs to be 3 times the size of the largest possible
framebuffer available or page flips can fail. This leaves the display
dark without further warning. Hibmc hardware with 32 MiB and a maximum
framebuffer size of 1920x2000 is at the limit.

* No dma-buf sharing without GTT support. Neither gem-vram nor hibmc
hardware support a GTT address space. This is required to share buffers
with other devices via dma-buf interfaces.

* TTM requires hardware-accelerated rendering into video memory for
optimal results. As hibmc hardware cannot do this, hibmc renders in
system memory and copies the result to video memory. This can be more
effectively implemented with gem-shmem and DRM's shadow-plane helpers.

Converting hibmc to gem-shmem and shadow-plane helpers.

* Replace gem-vram entry points in struct drm_driver with gem-shmem
equivalents. This makes the driver allocate struct drm_gem_shmem_object
for its buffers.

* Use DRM_GEM_SHADOW_*_PLANE for its plane funcs and plane-helper
funcs. The shadow-plane helpers map a plane's gem buffer objects into
kernel address space during a page flip, so that atomic_update can
copy them to video memory.

* Handle framebuffer damage in hibmc_plane_atomic_update(). This updates
video memory from the plane's framebuffer. It automatically synchronizes
shared buffers with other devices. Create the framebuffer with
drm_gem_fb_create_with_dirty() to trigger the update on each page flip.

* Initialize the plane with drm_plane_enable_fb_damage_clips() to limit
the damage updates to the framebuffer areas that changed. We don't want
to do a full-buffer memcpy if only a small area has changed.

* Test display modes against the available video memory in
hibmc_mode_config_mode_valid(). We only want to announce display modes
that fit into display memory.

* Map the display memory itself into kernel address space.

* Do not set drm_mode_config.prefer_shadow. This would advise user space
to install a shadow buffer. But with gem-shmem, the gem buffer object
already acts as a shadow buffer for video memory.

We use these patterns in many other drivers with similar limitation as
hibmc and its hardware. With these changes in place, hibmc is more robust
and better integrated into the overall DRM framework.

v3:
- fix coding style
v2:
- do not select TTM symbols

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c        | 22 +++++--
 drivers/gpu/drm/hisilicon/hibmc/Kconfig       |  4 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 42 ++++++++-----
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 62 ++++++++++++++-----
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  5 ++
 include/drm/drm_gem_shmem_helper.h            |  4 ++
 6 files changed, 102 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 545933c7f712..d4ace2a1c47d 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -453,10 +453,23 @@ void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_vunmap_locked);
 
-static int
-drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
-				 struct drm_device *dev, size_t size,
-				 uint32_t *handle)
+/**
+ * drm_gem_shmem_create_with_handle - Allocate an object with the given size and
+ *	returns a GEM handle
+ * @file_priv: DRM file structure to create the dumb buffer for
+ * @dev: DRM device
+ * @size: Size of the object to allocate
+ * @handle: Returns the GEM handle on success
+ *
+ * Allocates an shmem GEM buffer using drm_gem_shmem_create() and returns
+ * a GEM handle to it.
+ *
+ * Returns:
+ * Zero on success, or an error code otherwise.
+ */
+int drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
+				     struct drm_device *dev, size_t size,
+				     uint32_t *handle)
 {
 	struct drm_gem_shmem_object *shmem;
 	int ret;
@@ -475,6 +488,7 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(drm_gem_shmem_create_with_handle);
 
 /* Update madvise status, returns true if not purged, else
  * false or -errno.
diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
index d1f3f5793f34..adf4516bf8f6 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig
+++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
@@ -5,10 +5,8 @@ config DRM_HISI_HIBMC
 	select DRM_CLIENT_SELECTION
 	select DRM_DISPLAY_HELPER
 	select DRM_DISPLAY_DP_HELPER
+	select DRM_GEM_SHMEM_HELPER
 	select DRM_KMS_HELPER
-	select DRM_VRAM_HELPER
-	select DRM_TTM
-	select DRM_TTM_HELPER
 	select I2C
 	select I2C_ALGOBIT
 	help
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index 79c33c778d2c..b4ab53db1c08 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -15,8 +15,10 @@
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_fourcc.h>
-#include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "hibmc_drm_drv.h"
@@ -100,28 +102,41 @@ static int hibmc_plane_atomic_check(struct drm_plane *plane,
 static void hibmc_plane_atomic_update(struct drm_plane *plane,
 				      struct drm_atomic_commit *state)
 {
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
 	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 *old_state = drm_atomic_get_old_plane_state(state, plane);
+	u32 gpu_addr = 0;
 	u32 reg;
-	s64 gpu_addr = 0;
 	u32 line_l;
-	struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
-	struct drm_gem_vram_object *gbo;
 
-	if (!new_state->fb)
+	if (!fb)
 		return;
 
-	gbo = drm_gem_vram_of_gem(new_state->fb->obj[0]);
+	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE) == 0) {
+		struct drm_rect damage;
+		struct drm_atomic_helper_damage_iter iter;
+
+		drm_atomic_helper_damage_iter_init(&iter, old_state, new_state);
+		drm_atomic_for_each_plane_damage(&iter, &damage) {
+			struct iosys_map dst[DRM_FORMAT_MAX_PLANES] = {
+				IOSYS_MAP_INIT_VADDR_IOMEM(priv->vram + gpu_addr),
+			};
 
-	gpu_addr = drm_gem_vram_offset(gbo);
-	if (WARN_ON_ONCE(gpu_addr < 0))
-		return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
+			iosys_map_incr(&dst[0],
+				       drm_fb_clip_offset(fb->pitches[0], fb->format, &damage));
+			drm_fb_memcpy(dst, fb->pitches, shadow_plane_state->data, fb, &damage);
+		}
+
+		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+	}
 
 	writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
 
 	reg = drm_format_info_min_pitch(fb->format, 0, fb->width);
 
-	line_l = new_state->fb->pitches[0];
+	line_l = fb->pitches[0];
 	writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
 	       HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
 	       priv->mmio + HIBMC_CRT_FB_WIDTH);
@@ -149,13 +164,11 @@ static const struct drm_plane_funcs hibmc_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 const struct drm_plane_helper_funcs hibmc_plane_helper_funcs = {
-	DRM_GEM_VRAM_PLANE_HELPER_FUNCS,
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
 	.atomic_check = hibmc_plane_atomic_check,
 	.atomic_update = hibmc_plane_atomic_update,
 };
@@ -515,6 +528,7 @@ int hibmc_de_init(struct hibmc_drm_private *priv)
 	}
 
 	drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(plane);
 
 	ret = drm_crtc_init_with_planes(dev, crtc, plane,
 					NULL, &hibmc_crtc_funcs, NULL);
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 99b36de1fe13..19d3193e2f76 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -18,9 +18,10 @@
 #include <drm/clients/drm_client_setup.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
-#include <drm/drm_fbdev_ttm.h>
+#include <drm/drm_dumb_buffers.h>
+#include <drm/drm_fbdev_shmem.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_module.h>
 #include <drm/drm_vblank.h>
@@ -71,7 +72,13 @@ static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
 static int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
 			     struct drm_mode_create_dumb *args)
 {
-	return drm_gem_vram_fill_create_dumb(file, dev, 0, 128, args);
+	int ret;
+
+	ret = drm_mode_size_dumb(dev, args, SZ_128, 0);
+	if (ret)
+		return ret;
+
+	return drm_gem_shmem_create_with_handle(file, dev, args->size, &args->handle);
 }
 
 static const struct drm_driver hibmc_driver = {
@@ -81,10 +88,9 @@ static const struct drm_driver hibmc_driver = {
 	.desc			= "hibmc drm driver",
 	.major			= 1,
 	.minor			= 0,
-	.debugfs_init		= drm_vram_mm_debugfs_init,
-	.dumb_create            = hibmc_dumb_create,
-	.dumb_map_offset        = drm_gem_ttm_dumb_map_offset,
-	DRM_FBDEV_TTM_DRIVER_OPS,
+	.gem_prime_import	= drm_gem_shmem_prime_import_no_map,
+	.dumb_create		= hibmc_dumb_create,
+	DRM_FBDEV_SHMEM_DRIVER_OPS,
 };
 
 static int __maybe_unused hibmc_pm_suspend(struct device *dev)
@@ -106,11 +112,32 @@ static const struct dev_pm_ops hibmc_pm_ops = {
 				hibmc_pm_resume)
 };
 
+static enum drm_mode_status hibmc_mode_config_mode_valid(struct drm_device *dev,
+							 const struct drm_display_mode *mode)
+{
+	const struct drm_format_info *info =
+		drm_get_format_info(dev, DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR);
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
+	unsigned long max_fb_size = priv->vram_size;
+	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;
+	else if (pitch > max_fb_size / mode->vdisplay)
+		return MODE_MEM;
+
+	return MODE_OK;
+}
+
 static const struct drm_mode_config_funcs hibmc_mode_funcs = {
-	.mode_valid = drm_vram_helper_mode_valid,
+	.mode_valid = hibmc_mode_config_mode_valid,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
-	.fb_create = drm_gem_fb_create,
+	.fb_create = drm_gem_fb_create_with_dirty,
 };
 
 static int hibmc_kms_init(struct hibmc_drm_private *priv)
@@ -130,7 +157,6 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
 	dev->mode_config.max_height = 1200;
 
 	dev->mode_config.preferred_depth = 24;
-	dev->mode_config.prefer_shadow = 1;
 
 	dev->mode_config.funcs = (void *)&hibmc_mode_funcs;
 
@@ -335,18 +361,22 @@ static int hibmc_load(struct drm_device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
+	resource_size_t vram_base, vram_size;
 	int ret;
 
 	ret = hibmc_hw_init(priv);
 	if (ret)
 		return ret;
 
-	ret = drmm_vram_helper_init(dev, pci_resource_start(pdev, 0),
-				    pci_resource_len(pdev, 0));
-	if (ret) {
-		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
-		return ret;
-	}
+	vram_base = pci_resource_start(pdev, 0);
+	vram_size = pci_resource_len(pdev, 0);
+
+	priv->vram = devm_ioremap_wc(dev->dev, vram_base, vram_size);
+	if (!priv->vram)
+		return -ENOMEM;
+
+	priv->vram_base = vram_base;
+	priv->vram_size = vram_size;
 
 	ret = hibmc_kms_init(priv);
 	if (ret)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index cd3a3fca1fe6..dce8572bf63e 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -38,6 +38,11 @@ struct hibmc_drm_private {
 	/* hw */
 	void __iomem   *mmio;
 
+	/* vram */
+	void __iomem *vram;
+	resource_size_t	vram_base;
+	resource_size_t	vram_size;
+
 	/* drm */
 	struct drm_device dev;
 	struct drm_plane primary_plane;
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index 5ccdae21b94a..86b967174b60 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -141,6 +141,10 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
 void drm_gem_shmem_print_info(const struct drm_gem_shmem_object *shmem,
 			      struct drm_printer *p, unsigned int indent);
 
+int drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
+				     struct drm_device *dev, size_t size,
+				     uint32_t *handle);
+
 extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
 
 /*
-- 
2.54.0

Re: [PATCH v3 3/4] drm/hibmc: Do not use cpp from struct drm_format_info

[Yongbang Shi]

Hi, Thomas.

> Replace uses of struct drm_format_info's cpp with appropriate interfaces.
> The cpp field contains the characters per pixel. It is deprecated and
> should be avoided.
> 
> Calculate the line width in bytes with drm_format_info_min_pitch(). This
> is the preferred way of getting pixel and line sizes.
> 
> Program HIB_CRT_DISP_CTL_FORMAT from the format's 4CC code instead of
> calculating the field's value from the cpp.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> index 2e6e189bec1a..79c33c778d2c 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> @@ -100,8 +100,8 @@ static int hibmc_plane_atomic_check(struct drm_plane *plane,
>  static void hibmc_plane_atomic_update(struct drm_plane *plane,
>  				      struct drm_atomic_commit *state)
>  {
> -	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
> -									   plane);
> +	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_framebuffer *fb = new_state->fb;
>  	u32 reg;
>  	s64 gpu_addr = 0;
>  	u32 line_l;
> @@ -119,7 +119,7 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
>  
>  	writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
>  
> -	reg = new_state->fb->width * (new_state->fb->format->cpp[0]);
> +	reg = drm_format_info_min_pitch(fb->format, 0, fb->width);
>  
>  	line_l = new_state->fb->pitches[0];

Since the variable `fb` was defined earlier, it can be used directly here without needing `new_state->fb`.

This function uses `new_state->fb` in two other places:
https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-next/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c?ref_type=heads#L129
https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-next/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c?ref_type=heads#L132

Thanks,
Yongbang.

>  	writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
> @@ -129,8 +129,14 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
>  	/* SET PIXEL FORMAT */
>  	reg = readl(priv->mmio + HIBMC_CRT_DISP_CTL);
>  	reg &= ~HIBMC_CRT_DISP_CTL_FORMAT_MASK;
> -	reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT,
> -			   new_state->fb->format->cpp[0] * 8 / 16);
> +	switch (fb->format->format) {
> +	case DRM_FORMAT_XRGB8888:
> +		reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT, 2);
> +		break;
> +	case DRM_FORMAT_RGB565:
> +		reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT, 1);
> +		break;
> +	}
>  	writel(reg, priv->mmio + HIBMC_CRT_DISP_CTL);
>  }
>  

Re: [PATCH v3 4/4] drm/hibmc: Use gem-shmem with shadow-plane helpers for memory management

[Yongbang Shi]

Hi Thomas.

>  
>  #include "hibmc_drm_drv.h"
> @@ -100,28 +102,41 @@ static int hibmc_plane_atomic_check(struct drm_plane *plane,
>  static void hibmc_plane_atomic_update(struct drm_plane *plane,
>  				      struct drm_atomic_commit *state)
>  {
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
>  	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 *old_state = drm_atomic_get_old_plane_state(state, plane);
> +	u32 gpu_addr = 0;
>  	u32 reg;
> -	s64 gpu_addr = 0;
>  	u32 line_l;
> -	struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
> -	struct drm_gem_vram_object *gbo;
>  
> -	if (!new_state->fb)
> +	if (!fb)
>  		return;
>  
> -	gbo = drm_gem_vram_of_gem(new_state->fb->obj[0]);
> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE) == 0) {
> +		struct drm_rect damage;
> +		struct drm_atomic_helper_damage_iter iter;
> +
> +		drm_atomic_helper_damage_iter_init(&iter, old_state, new_state);
> +		drm_atomic_for_each_plane_damage(&iter, &damage) {
> +			struct iosys_map dst[DRM_FORMAT_MAX_PLANES] = {
> +				IOSYS_MAP_INIT_VADDR_IOMEM(priv->vram + gpu_addr),
> +			};
>  
> -	gpu_addr = drm_gem_vram_offset(gbo);
> -	if (WARN_ON_ONCE(gpu_addr < 0))
> -		return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
> +			iosys_map_incr(&dst[0],
> +				       drm_fb_clip_offset(fb->pitches[0], fb->format, &damage));
> +			drm_fb_memcpy(dst, fb->pitches, shadow_plane_state->data, fb, &damage);
> +		}
> +
> +		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +	}

This implementation uses shadow planes for damage rect updates. If the entire plane is updated frequently, could this
lead to a performance drop or frame drops?

Originally, the window system(X11/Wayland) drew directly to the hardware framebuffer, but now there is an extra copy in
`atomic_update`.

>  
>  	writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
>  
>  	reg = drm_format_info_min_pitch(fb->format, 0, fb->width);
>  
> -	line_l = new_state->fb->pitches[0];
> +	line_l = fb->pitches[0];
>  	writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
>  	       HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
>  	       priv->mmio + HIBMC_CRT_FB_WIDTH);
> @@ -149,13 +164,11 @@ static const struct drm_plane_funcs hibmc_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 const struct drm_plane_helper_funcs hibmc_plane_helper_funcs = {
> -	DRM_GEM_VRAM_PLANE_HELPER_FUNCS,
> +	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>  	.atomic_check = hibmc_plane_atomic_check,
>  	.atomic_update = hibmc_plane_atomic_update,
>  };
> @@ -515,6 +528,7 @@ int hibmc_de_init(struct hibmc_drm_private *priv)
>  	}
>  
>  	drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
> +	drm_plane_enable_fb_damage_clips(plane);
>  
>  	ret = drm_crtc_init_with_planes(dev, crtc, plane,
>  					NULL, &hibmc_crtc_funcs, NULL);
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 99b36de1fe13..19d3193e2f76 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -18,9 +18,10 @@
>  #include <drm/clients/drm_client_setup.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_drv.h>
> -#include <drm/drm_fbdev_ttm.h>
> +#include <drm/drm_dumb_buffers.h>
> +#include <drm/drm_fbdev_shmem.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_module.h>
>  #include <drm/drm_vblank.h>
> @@ -71,7 +72,13 @@ static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
>  static int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>  			     struct drm_mode_create_dumb *args)
>  {
> -	return drm_gem_vram_fill_create_dumb(file, dev, 0, 128, args);
> +	int ret;
> +
> +	ret = drm_mode_size_dumb(dev, args, SZ_128, 0);
> +	if (ret)
> +		return ret;
> +
> +	return drm_gem_shmem_create_with_handle(file, dev, args->size, &args->handle);
>  }
>  
>  static const struct drm_driver hibmc_driver = {
> @@ -81,10 +88,9 @@ static const struct drm_driver hibmc_driver = {
>  	.desc			= "hibmc drm driver",
>  	.major			= 1,
>  	.minor			= 0,
> -	.debugfs_init		= drm_vram_mm_debugfs_init,
> -	.dumb_create            = hibmc_dumb_create,
> -	.dumb_map_offset        = drm_gem_ttm_dumb_map_offset,
> -	DRM_FBDEV_TTM_DRIVER_OPS,
> +	.gem_prime_import	= drm_gem_shmem_prime_import_no_map,
> +	.dumb_create		= hibmc_dumb_create,
> +	DRM_FBDEV_SHMEM_DRIVER_OPS,
>  };
>  
>  static int __maybe_unused hibmc_pm_suspend(struct device *dev)
> @@ -106,11 +112,32 @@ static const struct dev_pm_ops hibmc_pm_ops = {
>  				hibmc_pm_resume)
>  };
>  
> +static enum drm_mode_status hibmc_mode_config_mode_valid(struct drm_device *dev,
> +							 const struct drm_display_mode *mode)
> +{
> +	const struct drm_format_info *info =
> +		drm_get_format_info(dev, DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR);
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> +	unsigned long max_fb_size = priv->vram_size;
> +	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;
> +	else if (pitch > max_fb_size / mode->vdisplay)
> +		return MODE_MEM;
> +

I reviewed the AST and Cirrus implementations, and they include checks for the maximum size supported by the hardware.

The hardware FB width register has only 14 bits, and the HIBMC_CRT_FB_WIDTH_WIDTH_MASK macro defines the maximum size as
0x3FFF. Would adding the following check make the code more rigorous?

Also, the pitch requires 128-byte alignment. Should we add a check here, or does the buffer created by
`hibmc_dumb_create` already ensure that the FB start address is aligned to 128 bytes via `SZ_128`? I’m not entirely sure
about this.

Of course, the original code used `drm_vram_helper_mode_valid` without strictly checking these hardware limitations.
Thank you for your modification.


Thanks,
Yongbang.

> +	return MODE_OK;
> +}
> +
>  static const struct drm_mode_config_funcs hibmc_mode_funcs = {
> -	.mode_valid = drm_vram_helper_mode_valid,
> +	.mode_valid = hibmc_mode_config_mode_valid,
>  	.atomic_check = drm_atomic_helper_check,
>  	.atomic_commit = drm_atomic_helper_commit,
> -	.fb_create = drm_gem_fb_create,
> +	.fb_create = drm_gem_fb_create_with_dirty,
>  };
>  
>  static int hibmc_kms_init(struct hibmc_drm_private *priv)
> @@ -130,7 +157,6 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
>  	dev->mode_config.max_height = 1200;
>  
>  	dev->mode_config.preferred_depth = 24;
> -	dev->mode_config.prefer_shadow = 1;
>  
>  	dev->mode_config.funcs = (void *)&hibmc_mode_funcs;
>  
> @@ -335,18 +361,22 @@ static int hibmc_load(struct drm_device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
>  	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> +	resource_size_t vram_base, vram_size;
>  	int ret;
>  
>  	ret = hibmc_hw_init(priv);
>  	if (ret)
>  		return ret;
>  
> -	ret = drmm_vram_helper_init(dev, pci_resource_start(pdev, 0),
> -				    pci_resource_len(pdev, 0));
> -	if (ret) {
> -		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
> -		return ret;
> -	}
> +	vram_base = pci_resource_start(pdev, 0);
> +	vram_size = pci_resource_len(pdev, 0);
> +
> +	priv->vram = devm_ioremap_wc(dev->dev, vram_base, vram_size);
> +	if (!priv->vram)
> +		return -ENOMEM;
> +
> +	priv->vram_base = vram_base;
> +	priv->vram_size = vram_size;
>  
>  	ret = hibmc_kms_init(priv);
>  	if (ret)
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index cd3a3fca1fe6..dce8572bf63e 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -38,6 +38,11 @@ struct hibmc_drm_private {
>  	/* hw */
>  	void __iomem   *mmio;
>  
> +	/* vram */
> +	void __iomem *vram;
> +	resource_size_t	vram_base;
> +	resource_size_t	vram_size;
> +
>  	/* drm */
>  	struct drm_device dev;
>  	struct drm_plane primary_plane;
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index 5ccdae21b94a..86b967174b60 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -141,6 +141,10 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
>  void drm_gem_shmem_print_info(const struct drm_gem_shmem_object *shmem,
>  			      struct drm_printer *p, unsigned int indent);
>  
> +int drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
> +				     struct drm_device *dev, size_t size,
> +				     uint32_t *handle);
> +
>  extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
>  
>  /*

Re: [PATCH v3 4/4] drm/hibmc: Use gem-shmem with shadow-plane helpers for memory management

[Thomas Zimmermann]

Hi

Am 08.06.26 um 15:42 schrieb Yongbang Shi:
> Hi Thomas.
>
>>   
>>   #include "hibmc_drm_drv.h"
>> @@ -100,28 +102,41 @@ static int hibmc_plane_atomic_check(struct drm_plane *plane,
>>   static void hibmc_plane_atomic_update(struct drm_plane *plane,
>>   				      struct drm_atomic_commit *state)
>>   {
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
>>   	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 *old_state = drm_atomic_get_old_plane_state(state, plane);
>> +	u32 gpu_addr = 0;
>>   	u32 reg;
>> -	s64 gpu_addr = 0;
>>   	u32 line_l;
>> -	struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
>> -	struct drm_gem_vram_object *gbo;
>>   
>> -	if (!new_state->fb)
>> +	if (!fb)
>>   		return;
>>   
>> -	gbo = drm_gem_vram_of_gem(new_state->fb->obj[0]);
>> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE) == 0) {
>> +		struct drm_rect damage;
>> +		struct drm_atomic_helper_damage_iter iter;
>> +
>> +		drm_atomic_helper_damage_iter_init(&iter, old_state, new_state);
>> +		drm_atomic_for_each_plane_damage(&iter, &damage) {
>> +			struct iosys_map dst[DRM_FORMAT_MAX_PLANES] = {
>> +				IOSYS_MAP_INIT_VADDR_IOMEM(priv->vram + gpu_addr),
>> +			};
>>   
>> -	gpu_addr = drm_gem_vram_offset(gbo);
>> -	if (WARN_ON_ONCE(gpu_addr < 0))
>> -		return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
>> +			iosys_map_incr(&dst[0],
>> +				       drm_fb_clip_offset(fb->pitches[0], fb->format, &damage));
>> +			drm_fb_memcpy(dst, fb->pitches, shadow_plane_state->data, fb, &damage);
>> +		}
>> +
>> +		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> +	}
> This implementation uses shadow planes for damage rect updates. If the entire plane is updated frequently, could this
> lead to a performance drop or frame drops?
>
> Originally, the window system(X11/Wayland) drew directly to the hardware framebuffer, but now there is an extra copy in
> `atomic_update`.

Generally speaking, shadow buffering is necessary for acceptable 
performance of the software renderer, because PCI access is rather slow 
(when compared to system memory).

The current hibmc driver sets drm_mode_config.prefer_shadow, [1] which 
tells compositors to maintain a shadow buffer.  IIRC Xorg announces the 
by mentioning shadowfb in its log files. It then writes the fully 
composed shadow buffer into video memory. Wayland compositors do the 
same AFAIK. Related code from Gnome's windows manager is a [2].

With the patch applied, prefer_shadow is no longer required. The kernel 
driver now maintains the shadow buffer. If there's a performance 
penalty, I'm not aware of it.

All this also applies to the kernel's framebuffer console. It currently 
uses an internal shadow buffer [3], but that is no longer needed with 
gem-shmem.

[1] 
https://elixir.bootlin.com/linux/v7.0.12/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c#L132
[2] 
https://gitlab.gnome.org/GNOME/mutter/-/blob/50.2/src/backends/native/meta-kms-impl-device.c?ref_type=tags#L587
[3] 
https://elixir.bootlin.com/linux/v7.0.12/source/drivers/gpu/drm/drm_fbdev_ttm.c#L200

>
>>   
>>   	writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
>>   
>>   	reg = drm_format_info_min_pitch(fb->format, 0, fb->width);
>>   
>> -	line_l = new_state->fb->pitches[0];
>> +	line_l = fb->pitches[0];
>>   	writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
>>   	       HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
>>   	       priv->mmio + HIBMC_CRT_FB_WIDTH);
>> @@ -149,13 +164,11 @@ static const struct drm_plane_funcs hibmc_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 const struct drm_plane_helper_funcs hibmc_plane_helper_funcs = {
>> -	DRM_GEM_VRAM_PLANE_HELPER_FUNCS,
>> +	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>>   	.atomic_check = hibmc_plane_atomic_check,
>>   	.atomic_update = hibmc_plane_atomic_update,
>>   };
>> @@ -515,6 +528,7 @@ int hibmc_de_init(struct hibmc_drm_private *priv)
>>   	}
>>   
>>   	drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
>> +	drm_plane_enable_fb_damage_clips(plane);
>>   
>>   	ret = drm_crtc_init_with_planes(dev, crtc, plane,
>>   					NULL, &hibmc_crtc_funcs, NULL);
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index 99b36de1fe13..19d3193e2f76 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -18,9 +18,10 @@
>>   #include <drm/clients/drm_client_setup.h>
>>   #include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_drv.h>
>> -#include <drm/drm_fbdev_ttm.h>
>> +#include <drm/drm_dumb_buffers.h>
>> +#include <drm/drm_fbdev_shmem.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_module.h>
>>   #include <drm/drm_vblank.h>
>> @@ -71,7 +72,13 @@ static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
>>   static int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>>   			     struct drm_mode_create_dumb *args)
>>   {
>> -	return drm_gem_vram_fill_create_dumb(file, dev, 0, 128, args);
>> +	int ret;
>> +
>> +	ret = drm_mode_size_dumb(dev, args, SZ_128, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return drm_gem_shmem_create_with_handle(file, dev, args->size, &args->handle);
>>   }
>>   
>>   static const struct drm_driver hibmc_driver = {
>> @@ -81,10 +88,9 @@ static const struct drm_driver hibmc_driver = {
>>   	.desc			= "hibmc drm driver",
>>   	.major			= 1,
>>   	.minor			= 0,
>> -	.debugfs_init		= drm_vram_mm_debugfs_init,
>> -	.dumb_create            = hibmc_dumb_create,
>> -	.dumb_map_offset        = drm_gem_ttm_dumb_map_offset,
>> -	DRM_FBDEV_TTM_DRIVER_OPS,
>> +	.gem_prime_import	= drm_gem_shmem_prime_import_no_map,
>> +	.dumb_create		= hibmc_dumb_create,
>> +	DRM_FBDEV_SHMEM_DRIVER_OPS,
>>   };
>>   
>>   static int __maybe_unused hibmc_pm_suspend(struct device *dev)
>> @@ -106,11 +112,32 @@ static const struct dev_pm_ops hibmc_pm_ops = {
>>   				hibmc_pm_resume)
>>   };
>>   
>> +static enum drm_mode_status hibmc_mode_config_mode_valid(struct drm_device *dev,
>> +							 const struct drm_display_mode *mode)
>> +{
>> +	const struct drm_format_info *info =
>> +		drm_get_format_info(dev, DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR);
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>> +	unsigned long max_fb_size = priv->vram_size;
>> +	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;
>> +	else if (pitch > max_fb_size / mode->vdisplay)
>> +		return MODE_MEM;
>> +
> I reviewed the AST and Cirrus implementations, and they include checks for the maximum size supported by the hardware.
>
> The hardware FB width register has only 14 bits, and the HIBMC_CRT_FB_WIDTH_WIDTH_MASK macro defines the maximum size as
> 0x3FFF. Would adding the following check make the code more rigorous?

The helper here is the general mode_valid callback from 
drm_mode_config_funcs. It should only test against devide-wide limits, 
such as the available video memory.

The framebuffer size is apparently a limit of the CRTC. There's already 
a check in the CRTC's mode_valid for the supported resolutions. [4] I 
think this should be fine.

[4] 
https://elixir.bootlin.com/linux/v7.1-rc7/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c#L223

>
> Also, the pitch requires 128-byte alignment. Should we add a check here, or does the buffer created by
> `hibmc_dumb_create` already ensure that the FB start address is aligned to 128 bytes via `SZ_128`? I’m not entirely sure
> about this.

Yes, using SZ_128 aligns each scanline in the framebuffer to a multiple 
of 128 bytes. The underlying GEM buffer is sized accordingly. And the 
GEM buffer itself always starts at a page boundary. Any GEM buffer 
allocated via the dumb-buffer interface should be correct and the ioctl 
also returns the correct pitch back into user space.

The driver later programs the hardware pitch to the value stored in the 
framebuffer (in pitches[0]). If it then does a memcpy from the GEM 
buffer to video memory, it all works out.

A corner case would be that user space allocates via the dumb-buffers 
ioctl, but ignores the returned pitch. If we want to do additional 
checks for the pitch, we could add a wrapper function around fb_create. 
[5]  This would be the right place to verify that user space does not 
allocate unaligned framebuffers.

[5] 
https://elixir.bootlin.com/linux/v7.1-rc7/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c#L112

Best regards
Thomas

>
> Of course, the original code used `drm_vram_helper_mode_valid` without strictly checking these hardware limitations.
> Thank you for your modification.
>
>
> Thanks,
> Yongbang.
>
>> +	return MODE_OK;
>> +}
>> +
>>   static const struct drm_mode_config_funcs hibmc_mode_funcs = {
>> -	.mode_valid = drm_vram_helper_mode_valid,
>> +	.mode_valid = hibmc_mode_config_mode_valid,
>>   	.atomic_check = drm_atomic_helper_check,
>>   	.atomic_commit = drm_atomic_helper_commit,
>> -	.fb_create = drm_gem_fb_create,
>> +	.fb_create = drm_gem_fb_create_with_dirty,
>>   };
>>   
>>   static int hibmc_kms_init(struct hibmc_drm_private *priv)
>> @@ -130,7 +157,6 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
>>   	dev->mode_config.max_height = 1200;
>>   
>>   	dev->mode_config.preferred_depth = 24;
>> -	dev->mode_config.prefer_shadow = 1;
>>   
>>   	dev->mode_config.funcs = (void *)&hibmc_mode_funcs;
>>   
>> @@ -335,18 +361,22 @@ static int hibmc_load(struct drm_device *dev)
>>   {
>>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
>>   	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>> +	resource_size_t vram_base, vram_size;
>>   	int ret;
>>   
>>   	ret = hibmc_hw_init(priv);
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = drmm_vram_helper_init(dev, pci_resource_start(pdev, 0),
>> -				    pci_resource_len(pdev, 0));
>> -	if (ret) {
>> -		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
>> -		return ret;
>> -	}
>> +	vram_base = pci_resource_start(pdev, 0);
>> +	vram_size = pci_resource_len(pdev, 0);
>> +
>> +	priv->vram = devm_ioremap_wc(dev->dev, vram_base, vram_size);
>> +	if (!priv->vram)
>> +		return -ENOMEM;
>> +
>> +	priv->vram_base = vram_base;
>> +	priv->vram_size = vram_size;
>>   
>>   	ret = hibmc_kms_init(priv);
>>   	if (ret)
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index cd3a3fca1fe6..dce8572bf63e 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -38,6 +38,11 @@ struct hibmc_drm_private {
>>   	/* hw */
>>   	void __iomem   *mmio;
>>   
>> +	/* vram */
>> +	void __iomem *vram;
>> +	resource_size_t	vram_base;
>> +	resource_size_t	vram_size;
>> +
>>   	/* drm */
>>   	struct drm_device dev;
>>   	struct drm_plane primary_plane;
>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>> index 5ccdae21b94a..86b967174b60 100644
>> --- a/include/drm/drm_gem_shmem_helper.h
>> +++ b/include/drm/drm_gem_shmem_helper.h
>> @@ -141,6 +141,10 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
>>   void drm_gem_shmem_print_info(const struct drm_gem_shmem_object *shmem,
>>   			      struct drm_printer *p, unsigned int indent);
>>   
>> +int drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
>> +				     struct drm_device *dev, size_t size,
>> +				     uint32_t *handle);
>> +
>>   extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
>>   
>>   /*

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH v3 4/4] drm/hibmc: Use gem-shmem with shadow-plane helpers for memory management

[Yongbang Shi]

> Hi
> 
> Am 08.06.26 um 15:42 schrieb Yongbang Shi:
>> Hi Thomas.
>>
>>>     #include "hibmc_drm_drv.h"
>>> @@ -100,28 +102,41 @@ static int hibmc_plane_atomic_check(struct drm_plane *plane,
>>>   static void hibmc_plane_atomic_update(struct drm_plane *plane,
>>>                         struct drm_atomic_commit *state)
>>>   {
>>> +    struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
>>>       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 *old_state = drm_atomic_get_old_plane_state(state, plane);
>>> +    u32 gpu_addr = 0;
>>>       u32 reg;
>>> -    s64 gpu_addr = 0;
>>>       u32 line_l;
>>> -    struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
>>> -    struct drm_gem_vram_object *gbo;
>>>   -    if (!new_state->fb)
>>> +    if (!fb)
>>>           return;
>>>   -    gbo = drm_gem_vram_of_gem(new_state->fb->obj[0]);
>>> +    if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE) == 0) {
>>> +        struct drm_rect damage;
>>> +        struct drm_atomic_helper_damage_iter iter;
>>> +
>>> +        drm_atomic_helper_damage_iter_init(&iter, old_state, new_state);
>>> +        drm_atomic_for_each_plane_damage(&iter, &damage) {
>>> +            struct iosys_map dst[DRM_FORMAT_MAX_PLANES] = {
>>> +                IOSYS_MAP_INIT_VADDR_IOMEM(priv->vram + gpu_addr),
>>> +            };
>>>   -    gpu_addr = drm_gem_vram_offset(gbo);
>>> -    if (WARN_ON_ONCE(gpu_addr < 0))
>>> -        return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
>>> +            iosys_map_incr(&dst[0],
>>> +                       drm_fb_clip_offset(fb->pitches[0], fb->format, &damage));
>>> +            drm_fb_memcpy(dst, fb->pitches, shadow_plane_state->data, fb, &damage);
>>> +        }
>>> +
>>> +        drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>> +    }
>> This implementation uses shadow planes for damage rect updates. If the entire plane is updated frequently, could this
>> lead to a performance drop or frame drops?
>>
>> Originally, the window system(X11/Wayland) drew directly to the hardware framebuffer, but now there is an extra copy in
>> `atomic_update`.
> 
> Generally speaking, shadow buffering is necessary for acceptable performance of the software renderer, because PCI
> access is rather slow (when compared to system memory).
> 
> The current hibmc driver sets drm_mode_config.prefer_shadow, [1] which tells compositors to maintain a shadow buffer. 
> IIRC Xorg announces the by mentioning shadowfb in its log files. It then writes the fully composed shadow buffer into
> video memory. Wayland compositors do the same AFAIK. Related code from Gnome's windows manager is a [2].
> 
> With the patch applied, prefer_shadow is no longer required. The kernel driver now maintains the shadow buffer. If
> there's a performance penalty, I'm not aware of it.
> 
> All this also applies to the kernel's framebuffer console. It currently uses an internal shadow buffer [3], but that is
> no longer needed with gem-shmem.
> 
> [1] https://elixir.bootlin.com/linux/v7.0.12/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c#L132
> [2] https://gitlab.gnome.org/GNOME/mutter/-/blob/50.2/src/backends/native/meta-kms-impl-device.c?ref_type=tags#L587
> [3] https://elixir.bootlin.com/linux/v7.0.12/source/drivers/gpu/drm/drm_fbdev_ttm.c#L200
> 

Okay, I get it. Thanks for explaining.

>>
>>>         writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
>>>         reg = drm_format_info_min_pitch(fb->format, 0, fb->width);
>>>   -    line_l = new_state->fb->pitches[0];
>>> +    line_l = fb->pitches[0];
>>>       writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
>>>              HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
>>>              priv->mmio + HIBMC_CRT_FB_WIDTH);
>>> @@ -149,13 +164,11 @@ static const struct drm_plane_funcs hibmc_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 const struct drm_plane_helper_funcs hibmc_plane_helper_funcs = {
>>> -    DRM_GEM_VRAM_PLANE_HELPER_FUNCS,
>>> +    DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>>>       .atomic_check = hibmc_plane_atomic_check,
>>>       .atomic_update = hibmc_plane_atomic_update,
>>>   };
>>> @@ -515,6 +528,7 @@ int hibmc_de_init(struct hibmc_drm_private *priv)
>>>       }
>>>         drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
>>> +    drm_plane_enable_fb_damage_clips(plane);
>>>         ret = drm_crtc_init_with_planes(dev, crtc, plane,
>>>                       NULL, &hibmc_crtc_funcs, NULL);
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> index 99b36de1fe13..19d3193e2f76 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> @@ -18,9 +18,10 @@
>>>   #include <drm/clients/drm_client_setup.h>
>>>   #include <drm/drm_atomic_helper.h>
>>>   #include <drm/drm_drv.h>
>>> -#include <drm/drm_fbdev_ttm.h>
>>> +#include <drm/drm_dumb_buffers.h>
>>> +#include <drm/drm_fbdev_shmem.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_module.h>
>>>   #include <drm/drm_vblank.h>
>>> @@ -71,7 +72,13 @@ static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
>>>   static int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>>>                    struct drm_mode_create_dumb *args)
>>>   {
>>> -    return drm_gem_vram_fill_create_dumb(file, dev, 0, 128, args);
>>> +    int ret;
>>> +
>>> +    ret = drm_mode_size_dumb(dev, args, SZ_128, 0);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return drm_gem_shmem_create_with_handle(file, dev, args->size, &args->handle);
>>>   }
>>>     static const struct drm_driver hibmc_driver = {
>>> @@ -81,10 +88,9 @@ static const struct drm_driver hibmc_driver = {
>>>       .desc            = "hibmc drm driver",
>>>       .major            = 1,
>>>       .minor            = 0,
>>> -    .debugfs_init        = drm_vram_mm_debugfs_init,
>>> -    .dumb_create            = hibmc_dumb_create,
>>> -    .dumb_map_offset        = drm_gem_ttm_dumb_map_offset,
>>> -    DRM_FBDEV_TTM_DRIVER_OPS,
>>> +    .gem_prime_import    = drm_gem_shmem_prime_import_no_map,
>>> +    .dumb_create        = hibmc_dumb_create,
>>> +    DRM_FBDEV_SHMEM_DRIVER_OPS,
>>>   };
>>>     static int __maybe_unused hibmc_pm_suspend(struct device *dev)
>>> @@ -106,11 +112,32 @@ static const struct dev_pm_ops hibmc_pm_ops = {
>>>                   hibmc_pm_resume)
>>>   };
>>>   +static enum drm_mode_status hibmc_mode_config_mode_valid(struct drm_device *dev,
>>> +                             const struct drm_display_mode *mode)
>>> +{
>>> +    const struct drm_format_info *info =
>>> +        drm_get_format_info(dev, DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR);
>>> +    struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>>> +    unsigned long max_fb_size = priv->vram_size;
>>> +    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;
>>> +    else if (pitch > max_fb_size / mode->vdisplay)
>>> +        return MODE_MEM;
>>> +
>> I reviewed the AST and Cirrus implementations, and they include checks for the maximum size supported by the hardware.
>>
>> The hardware FB width register has only 14 bits, and the HIBMC_CRT_FB_WIDTH_WIDTH_MASK macro defines the maximum size as
>> 0x3FFF. Would adding the following check make the code more rigorous?
> 
> The helper here is the general mode_valid callback from drm_mode_config_funcs. It should only test against devide-wide
> limits, such as the available video memory.
> 
> The framebuffer size is apparently a limit of the CRTC. There's already a check in the CRTC's mode_valid for the
> supported resolutions. [4] I think this should be fine.

Yes, the CRTC's `mode_valid` function ensures that the resolution falls within the valid fb width range; adding this
check wouldn't really provide any additional practical benefit.

> 
> [4] https://elixir.bootlin.com/linux/v7.1-rc7/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c#L223
> 
>>
>> Also, the pitch requires 128-byte alignment. Should we add a check here, or does the buffer created by
>> `hibmc_dumb_create` already ensure that the FB start address is aligned to 128 bytes via `SZ_128`? I’m not entirely sure
>> about this.
> 
> Yes, using SZ_128 aligns each scanline in the framebuffer to a multiple of 128 bytes. The underlying GEM buffer is sized
> accordingly. And the GEM buffer itself always starts at a page boundary. Any GEM buffer allocated via the dumb-buffer
> interface should be correct and the ioctl also returns the correct pitch back into user space.
> 
> The driver later programs the hardware pitch to the value stored in the framebuffer (in pitches[0]). If it then does a
> memcpy from the GEM buffer to video memory, it all works out.
> 
> A corner case would be that user space allocates via the dumb-buffers ioctl, but ignores the returned pitch. If we want
> to do additional checks for the pitch, we could add a wrapper function around fb_create. [5]  This would be the right
> place to verify that user space does not allocate unaligned framebuffers.
> 

Thanks for the suggestion. Yes, since the kernel does not trust user space, a check could be added in fb_create.

For windowing systems like Xorg or Wayland, I believe the pitch is typically handled correctly.
However, for some simple or buggy DRM clients, if the driver does not check the pitch, it may result in misaligned or
distorted screen display.

Thanks,
Yongbang.

> [5] https://elixir.bootlin.com/linux/v7.1-rc7/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c#L112
> 
> Best regards
> Thomas
> 
>>
>> Of course, the original code used `drm_vram_helper_mode_valid` without strictly checking these hardware limitations.
>> Thank you for your modification.
>>
>>
>> Thanks,
>> Yongbang.
>>
>>> +    return MODE_OK;
>>> +}
>>> +
>>>   static const struct drm_mode_config_funcs hibmc_mode_funcs = {
>>> -    .mode_valid = drm_vram_helper_mode_valid,
>>> +    .mode_valid = hibmc_mode_config_mode_valid,
>>>       .atomic_check = drm_atomic_helper_check,
>>>       .atomic_commit = drm_atomic_helper_commit,
>>> -    .fb_create = drm_gem_fb_create,
>>> +    .fb_create = drm_gem_fb_create_with_dirty,
>>>   };
>>>     static int hibmc_kms_init(struct hibmc_drm_private *priv)
>>> @@ -130,7 +157,6 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
>>>       dev->mode_config.max_height = 1200;
>>>         dev->mode_config.preferred_depth = 24;
>>> -    dev->mode_config.prefer_shadow = 1;
>>>         dev->mode_config.funcs = (void *)&hibmc_mode_funcs;
>>>   @@ -335,18 +361,22 @@ static int hibmc_load(struct drm_device *dev)
>>>   {
>>>       struct pci_dev *pdev = to_pci_dev(dev->dev);
>>>       struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>>> +    resource_size_t vram_base, vram_size;
>>>       int ret;
>>>         ret = hibmc_hw_init(priv);
>>>       if (ret)
>>>           return ret;
>>>   -    ret = drmm_vram_helper_init(dev, pci_resource_start(pdev, 0),
>>> -                    pci_resource_len(pdev, 0));
>>> -    if (ret) {
>>> -        drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
>>> -        return ret;
>>> -    }
>>> +    vram_base = pci_resource_start(pdev, 0);
>>> +    vram_size = pci_resource_len(pdev, 0);
>>> +
>>> +    priv->vram = devm_ioremap_wc(dev->dev, vram_base, vram_size);
>>> +    if (!priv->vram)
>>> +        return -ENOMEM;
>>> +
>>> +    priv->vram_base = vram_base;
>>> +    priv->vram_size = vram_size;
>>>         ret = hibmc_kms_init(priv);
>>>       if (ret)
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> index cd3a3fca1fe6..dce8572bf63e 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> @@ -38,6 +38,11 @@ struct hibmc_drm_private {
>>>       /* hw */
>>>       void __iomem   *mmio;
>>>   +    /* vram */
>>> +    void __iomem *vram;
>>> +    resource_size_t    vram_base;
>>> +    resource_size_t    vram_size;
>>> +
>>>       /* drm */
>>>       struct drm_device dev;
>>>       struct drm_plane primary_plane;
>>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>>> index 5ccdae21b94a..86b967174b60 100644
>>> --- a/include/drm/drm_gem_shmem_helper.h
>>> +++ b/include/drm/drm_gem_shmem_helper.h
>>> @@ -141,6 +141,10 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
>>>   void drm_gem_shmem_print_info(const struct drm_gem_shmem_object *shmem,
>>>                     struct drm_printer *p, unsigned int indent);
>>>   +int drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
>>> +                     struct drm_device *dev, size_t size,
>>> +                     uint32_t *handle);
>>> +
>>>   extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
>>>     /*
>