[PATCH 0/4] Replace hibmc code with generic implmentation

[PATCH 0/4] Replace hibmc code with generic implmentation

[Thomas Zimmermann]

The patch set replaces code in hibmc with generic implementation.

Patches 1 to 3 replace fbdev emuation, framebuffer and creation of
dumb buffers with respective code from DRM helpers. Patch 4 adds an
additional interface to debugfs that displays the allocated and free
areas in video memory.

The patches have only been compile-tested. Further testing is
appreciated.

Thomas Zimmermann (4):
  drm/hisilicon/hibmc: Switch to generic fbdev emulation
  drm/hisilicon/hibmc: Replace struct hibmc_framebuffer with generic
    code
  drm/hisilicon/hibmc: Implement hibmc_dumb_create() with generic
    helpers
  drm/hisilicon/hibmc: Export VRAM MM information to debugfs

 drivers/gpu/drm/drm_gem_vram_helper.c         |  10 +-
 drivers/gpu/drm/hisilicon/hibmc/Makefile      |   2 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    |   4 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |   6 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  26 --
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 240 ------------------
 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 117 +--------
 include/drm/drm_gem_vram_helper.h             |   1 +
 8 files changed, 17 insertions(+), 389 deletions(-)
 delete mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c

--
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/hisilicon/hibmc: Switch to generic fbdev emulation

[Thomas Zimmermann]

There's nothing special about hibmc's fbdev emulation that is not
provided by the generic implementation. Switch over and remove the
driver's code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/hisilicon/hibmc/Makefile      |   2 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |   5 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  11 -
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 240 ------------------
 4 files changed, 3 insertions(+), 255 deletions(-)
 delete mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c

diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
index 0c2d4296bccd..f99132715597 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
+++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
@@ -1,4 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
-hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_fbdev.o hibmc_ttm.o
+hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_ttm.o
 
 obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 2fd4ca91a62d..113d27b8a8f1 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -17,6 +17,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_irq.h>
 #include <drm/drm_print.h>
@@ -247,8 +248,6 @@ static int hibmc_unload(struct drm_device *dev)
 {
 	struct hibmc_drm_private *priv = dev->dev_private;
 
-	hibmc_fbdev_fini(priv);
-
 	drm_atomic_helper_shutdown(dev);
 
 	if (dev->irq_enabled)
@@ -307,7 +306,7 @@ static int hibmc_load(struct drm_device *dev)
 	/* reset all the states of crtc/plane/encoder/connector */
 	drm_mode_config_reset(dev);
 
-	ret = hibmc_fbdev_init(priv);
+	ret = drm_fbdev_generic_setup(dev, 16);
 	if (ret) {
 		DRM_ERROR("failed to initialize fbdev: %d\n", ret);
 		goto err;
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index e58ecd7edcf8..b34493ead30b 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -25,12 +25,6 @@ struct hibmc_framebuffer {
 	struct drm_gem_object *obj;
 };
 
-struct hibmc_fbdev {
-	struct drm_fb_helper helper; /* must be first */
-	struct hibmc_framebuffer *fb;
-	int size;
-};
-
 struct hibmc_drm_private {
 	/* hw */
 	void __iomem   *mmio;
@@ -42,9 +36,6 @@ struct hibmc_drm_private {
 	/* drm */
 	struct drm_device  *dev;
 	bool mode_config_initialized;
-
-	/* fbdev */
-	struct hibmc_fbdev *fbdev;
 };
 
 #define to_hibmc_framebuffer(x) container_of(x, struct hibmc_framebuffer, fb)
@@ -56,8 +47,6 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
 
 int hibmc_de_init(struct hibmc_drm_private *priv);
 int hibmc_vdac_init(struct hibmc_drm_private *priv);
-int hibmc_fbdev_init(struct hibmc_drm_private *priv);
-void hibmc_fbdev_fini(struct hibmc_drm_private *priv);
 
 int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
 		     struct drm_gem_object **obj);
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
deleted file mode 100644
index b4c1cea051e8..000000000000
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
+++ /dev/null
@@ -1,240 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/* Hisilicon Hibmc SoC drm driver
- *
- * Based on the bochs drm driver.
- *
- * Copyright (c) 2016 Huawei Limited.
- *
- * Author:
- *	Rongrong Zou <zourongrong@huawei.com>
- *	Rongrong Zou <zourongrong@gmail.com>
- *	Jianhua Li <lijianhua@huawei.com>
- */
-
-#include <drm/drm_crtc.h>
-#include <drm/drm_fb_helper.h>
-#include <drm/drm_fourcc.h>
-#include <drm/drm_gem_vram_helper.h>
-#include <drm/drm_probe_helper.h>
-
-#include "hibmc_drm_drv.h"
-
-static int hibmcfb_create_object(
-				struct hibmc_drm_private *priv,
-				const struct drm_mode_fb_cmd2 *mode_cmd,
-				struct drm_gem_object **gobj_p)
-{
-	struct drm_gem_object *gobj;
-	struct drm_device *dev = priv->dev;
-	u32 size;
-	int ret = 0;
-
-	size = mode_cmd->pitches[0] * mode_cmd->height;
-	ret = hibmc_gem_create(dev, size, true, &gobj);
-	if (ret)
-		return ret;
-
-	*gobj_p = gobj;
-	return ret;
-}
-
-static struct fb_ops hibmc_drm_fb_ops = {
-	.owner = THIS_MODULE,
-	.fb_check_var = drm_fb_helper_check_var,
-	.fb_set_par = drm_fb_helper_set_par,
-	.fb_fillrect = drm_fb_helper_sys_fillrect,
-	.fb_copyarea = drm_fb_helper_sys_copyarea,
-	.fb_imageblit = drm_fb_helper_sys_imageblit,
-	.fb_pan_display = drm_fb_helper_pan_display,
-	.fb_blank = drm_fb_helper_blank,
-	.fb_setcmap = drm_fb_helper_setcmap,
-};
-
-static int hibmc_drm_fb_create(struct drm_fb_helper *helper,
-			       struct drm_fb_helper_surface_size *sizes)
-{
-	struct hibmc_fbdev *hi_fbdev =
-		container_of(helper, struct hibmc_fbdev, helper);
-	struct hibmc_drm_private *priv = helper->dev->dev_private;
-	struct fb_info *info;
-	struct drm_mode_fb_cmd2 mode_cmd;
-	struct drm_gem_object *gobj = NULL;
-	int ret = 0;
-	size_t size;
-	unsigned int bytes_per_pixel;
-	struct drm_gem_vram_object *gbo = NULL;
-	void *base;
-
-	DRM_DEBUG_DRIVER("surface width(%d), height(%d) and bpp(%d)\n",
-			 sizes->surface_width, sizes->surface_height,
-			 sizes->surface_bpp);
-
-	bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
-
-	mode_cmd.width = sizes->surface_width;
-	mode_cmd.height = sizes->surface_height;
-	mode_cmd.pitches[0] = mode_cmd.width * bytes_per_pixel;
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
-							  sizes->surface_depth);
-
-	size = PAGE_ALIGN(mode_cmd.pitches[0] * mode_cmd.height);
-
-	ret = hibmcfb_create_object(priv, &mode_cmd, &gobj);
-	if (ret) {
-		DRM_ERROR("failed to create fbcon backing object: %d\n", ret);
-		return -ENOMEM;
-	}
-
-	gbo = drm_gem_vram_of_gem(gobj);
-
-	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
-	if (ret) {
-		DRM_ERROR("failed to pin fbcon: %d\n", ret);
-		goto out_unref_gem;
-	}
-
-	base = drm_gem_vram_kmap(gbo, true, NULL);
-	if (IS_ERR(base)) {
-		ret = PTR_ERR(base);
-		DRM_ERROR("failed to kmap fbcon: %d\n", ret);
-		goto out_unpin_bo;
-	}
-
-	info = drm_fb_helper_alloc_fbi(helper);
-	if (IS_ERR(info)) {
-		ret = PTR_ERR(info);
-		DRM_ERROR("failed to allocate fbi: %d\n", ret);
-		goto out_release_fbi;
-	}
-
-	hi_fbdev->fb = hibmc_framebuffer_init(priv->dev, &mode_cmd, gobj);
-	if (IS_ERR(hi_fbdev->fb)) {
-		ret = PTR_ERR(hi_fbdev->fb);
-		hi_fbdev->fb = NULL;
-		DRM_ERROR("failed to initialize framebuffer: %d\n", ret);
-		goto out_release_fbi;
-	}
-
-	priv->fbdev->size = size;
-	hi_fbdev->helper.fb = &hi_fbdev->fb->fb;
-
-	info->fbops = &hibmc_drm_fb_ops;
-
-	drm_fb_helper_fill_info(info, &priv->fbdev->helper, sizes);
-
-	info->screen_base = base;
-	info->screen_size = size;
-
-	info->fix.smem_start = gbo->bo.mem.bus.offset + gbo->bo.mem.bus.base;
-	info->fix.smem_len = size;
-	return 0;
-
-out_release_fbi:
-	drm_gem_vram_kunmap(gbo);
-out_unpin_bo:
-	drm_gem_vram_unpin(gbo);
-out_unref_gem:
-	drm_gem_object_put_unlocked(gobj);
-
-	return ret;
-}
-
-static void hibmc_fbdev_destroy(struct hibmc_fbdev *fbdev)
-{
-	struct hibmc_framebuffer *gfb = fbdev->fb;
-	struct drm_fb_helper *fbh = &fbdev->helper;
-
-	drm_fb_helper_unregister_fbi(fbh);
-
-	drm_fb_helper_fini(fbh);
-
-	if (gfb)
-		drm_framebuffer_put(&gfb->fb);
-}
-
-static const struct drm_fb_helper_funcs hibmc_fbdev_helper_funcs = {
-	.fb_probe = hibmc_drm_fb_create,
-};
-
-int hibmc_fbdev_init(struct hibmc_drm_private *priv)
-{
-	int ret;
-	struct fb_var_screeninfo *var;
-	struct fb_fix_screeninfo *fix;
-	struct hibmc_fbdev *hifbdev;
-
-	hifbdev = devm_kzalloc(priv->dev->dev, sizeof(*hifbdev), GFP_KERNEL);
-	if (!hifbdev) {
-		DRM_ERROR("failed to allocate hibmc_fbdev\n");
-		return -ENOMEM;
-	}
-
-	priv->fbdev = hifbdev;
-	drm_fb_helper_prepare(priv->dev, &hifbdev->helper,
-			      &hibmc_fbdev_helper_funcs);
-
-	/* Now just one crtc and one channel */
-	ret = drm_fb_helper_init(priv->dev, &hifbdev->helper, 1);
-	if (ret) {
-		DRM_ERROR("failed to initialize fb helper: %d\n", ret);
-		return ret;
-	}
-
-	ret = drm_fb_helper_single_add_all_connectors(&hifbdev->helper);
-	if (ret) {
-		DRM_ERROR("failed to add all connectors: %d\n", ret);
-		goto fini;
-	}
-
-	ret = drm_fb_helper_initial_config(&hifbdev->helper, 16);
-	if (ret) {
-		DRM_ERROR("failed to setup initial conn config: %d\n", ret);
-		goto fini;
-	}
-
-	var = &hifbdev->helper.fbdev->var;
-	fix = &hifbdev->helper.fbdev->fix;
-
-	DRM_DEBUG_DRIVER("Member of info->var is :\n"
-			 "xres=%d\n"
-			 "yres=%d\n"
-			 "xres_virtual=%d\n"
-			 "yres_virtual=%d\n"
-			 "xoffset=%d\n"
-			 "yoffset=%d\n"
-			 "bits_per_pixel=%d\n"
-			 "...\n", var->xres, var->yres, var->xres_virtual,
-			 var->yres_virtual, var->xoffset, var->yoffset,
-			 var->bits_per_pixel);
-	DRM_DEBUG_DRIVER("Member of info->fix is :\n"
-			 "smem_start=%lx\n"
-			 "smem_len=%d\n"
-			 "type=%d\n"
-			 "type_aux=%d\n"
-			 "visual=%d\n"
-			 "xpanstep=%d\n"
-			 "ypanstep=%d\n"
-			 "ywrapstep=%d\n"
-			 "line_length=%d\n"
-			 "accel=%d\n"
-			 "capabilities=%d\n"
-			 "...\n", fix->smem_start, fix->smem_len, fix->type,
-			 fix->type_aux, fix->visual, fix->xpanstep,
-			 fix->ypanstep, fix->ywrapstep, fix->line_length,
-			 fix->accel, fix->capabilities);
-
-	return 0;
-
-fini:
-	drm_fb_helper_fini(&hifbdev->helper);
-	return ret;
-}
-
-void hibmc_fbdev_fini(struct hibmc_drm_private *priv)
-{
-	if (!priv->fbdev)
-		return;
-
-	hibmc_fbdev_destroy(priv->fbdev);
-	priv->fbdev = NULL;
-}
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/hisilicon/hibmc: Replace struct hibmc_framebuffer with generic code

[Thomas Zimmermann]

The hibmc driver's struct hibmc_framebuffer stores a DRM framebuffer
with an associated GEM object. This functionality is also provided by
generic code. Switch hibmc over.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    |  4 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   | 11 ---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 69 +------------------
 3 files changed, 3 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index 6527a97f68a3..7fa7d4933f60 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -99,14 +99,12 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
 	s64 gpu_addr = 0;
 	unsigned int line_l;
 	struct hibmc_drm_private *priv = plane->dev->dev_private;
-	struct hibmc_framebuffer *hibmc_fb;
 	struct drm_gem_vram_object *gbo;
 
 	if (!state->fb)
 		return;
 
-	hibmc_fb = to_hibmc_framebuffer(state->fb);
-	gbo = drm_gem_vram_of_gem(hibmc_fb->obj);
+	gbo = drm_gem_vram_of_gem(state->fb->obj[0]);
 
 	gpu_addr = drm_gem_vram_offset(gbo);
 	if (WARN_ON_ONCE(gpu_addr < 0))
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index b34493ead30b..8eb7258b236a 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -20,11 +20,6 @@
 struct drm_device;
 struct drm_gem_object;
 
-struct hibmc_framebuffer {
-	struct drm_framebuffer fb;
-	struct drm_gem_object *obj;
-};
-
 struct hibmc_drm_private {
 	/* hw */
 	void __iomem   *mmio;
@@ -38,8 +33,6 @@ struct hibmc_drm_private {
 	bool mode_config_initialized;
 };
 
-#define to_hibmc_framebuffer(x) container_of(x, struct hibmc_framebuffer, fb)
-
 void hibmc_set_power_mode(struct hibmc_drm_private *priv,
 			  unsigned int power_mode);
 void hibmc_set_current_gate(struct hibmc_drm_private *priv,
@@ -50,10 +43,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv);
 
 int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
 		     struct drm_gem_object **obj);
-struct hibmc_framebuffer *
-hibmc_framebuffer_init(struct drm_device *dev,
-		       const struct drm_mode_fb_cmd2 *mode_cmd,
-		       struct drm_gem_object *obj);
 
 int hibmc_mm_init(struct hibmc_drm_private *hibmc);
 void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
index 21b684eab5c9..f6d25b85c209 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
@@ -15,6 +15,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_print.h>
 
@@ -97,74 +98,8 @@ int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
 	return 0;
 }
 
-static void hibmc_user_framebuffer_destroy(struct drm_framebuffer *fb)
-{
-	struct hibmc_framebuffer *hibmc_fb = to_hibmc_framebuffer(fb);
-
-	drm_gem_object_put_unlocked(hibmc_fb->obj);
-	drm_framebuffer_cleanup(fb);
-	kfree(hibmc_fb);
-}
-
-static const struct drm_framebuffer_funcs hibmc_fb_funcs = {
-	.destroy = hibmc_user_framebuffer_destroy,
-};
-
-struct hibmc_framebuffer *
-hibmc_framebuffer_init(struct drm_device *dev,
-		       const struct drm_mode_fb_cmd2 *mode_cmd,
-		       struct drm_gem_object *obj)
-{
-	struct hibmc_framebuffer *hibmc_fb;
-	int ret;
-
-	hibmc_fb = kzalloc(sizeof(*hibmc_fb), GFP_KERNEL);
-	if (!hibmc_fb) {
-		DRM_ERROR("failed to allocate hibmc_fb\n");
-		return ERR_PTR(-ENOMEM);
-	}
-
-	drm_helper_mode_fill_fb_struct(dev, &hibmc_fb->fb, mode_cmd);
-	hibmc_fb->obj = obj;
-	ret = drm_framebuffer_init(dev, &hibmc_fb->fb, &hibmc_fb_funcs);
-	if (ret) {
-		DRM_ERROR("drm_framebuffer_init failed: %d\n", ret);
-		kfree(hibmc_fb);
-		return ERR_PTR(ret);
-	}
-
-	return hibmc_fb;
-}
-
-static struct drm_framebuffer *
-hibmc_user_framebuffer_create(struct drm_device *dev,
-			      struct drm_file *filp,
-			      const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	struct drm_gem_object *obj;
-	struct hibmc_framebuffer *hibmc_fb;
-
-	DRM_DEBUG_DRIVER("%dx%d, format %c%c%c%c\n",
-			 mode_cmd->width, mode_cmd->height,
-			 (mode_cmd->pixel_format) & 0xff,
-			 (mode_cmd->pixel_format >> 8)  & 0xff,
-			 (mode_cmd->pixel_format >> 16) & 0xff,
-			 (mode_cmd->pixel_format >> 24) & 0xff);
-
-	obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
-	if (!obj)
-		return ERR_PTR(-ENOENT);
-
-	hibmc_fb = hibmc_framebuffer_init(dev, mode_cmd, obj);
-	if (IS_ERR(hibmc_fb)) {
-		drm_gem_object_put_unlocked(obj);
-		return ERR_PTR((long)hibmc_fb);
-	}
-	return &hibmc_fb->fb;
-}
-
 const struct drm_mode_config_funcs hibmc_mode_funcs = {
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
-	.fb_create = hibmc_user_framebuffer_create,
+	.fb_create = drm_gem_fb_create,
 };
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/hisilicon/hibmc: Implement hibmc_dumb_create() with generic helpers

[Thomas Zimmermann]

The hibmc driver aligns scanlines to 16 bytes. Adding the pitch alignment
as an argument to drm_gem_vram_fill_create_dumb() allows to use the generic
implementation with hibmc. A value of 0 disables scanline pitches.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c         | 10 ++--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  4 --
 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 48 +------------------
 include/drm/drm_gem_vram_helper.h             |  1 +
 4 files changed, 10 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 666cb4c22bb9..f098784e7dfd 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -485,6 +485,7 @@ EXPORT_SYMBOL(drm_gem_vram_vunmap);
  * @dev:		the DRM device
  * @bdev:		the TTM BO device managing the buffer object
  * @pg_align:		the buffer's alignment in multiples of the page size
+ * @pitch_align:	the scanline's alignment in powers of 2
  * @interruptible:	sleep interruptible if waiting for memory
  * @args:		the arguments as provided to \
 				&struct drm_driver.dumb_create
@@ -502,6 +503,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
 				  struct drm_device *dev,
 				  struct ttm_bo_device *bdev,
 				  unsigned long pg_align,
+				  unsigned long pitch_align,
 				  bool interruptible,
 				  struct drm_mode_create_dumb *args)
 {
@@ -510,7 +512,9 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
 	int ret;
 	u32 handle;
 
-	pitch = args->width * ((args->bpp + 7) / 8);
+	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
+	if (pitch_align)
+		pitch = ALIGN(pitch, pitch_align);
 	size = pitch * args->height;
 
 	size = roundup(size, PAGE_SIZE);
@@ -612,8 +616,8 @@ int drm_gem_vram_driver_dumb_create(struct drm_file *file,
 	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
 		return -EINVAL;
 
-	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev, 0,
-					     false, args);
+	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
+					     0, 0, false, args);
 }
 EXPORT_SYMBOL(drm_gem_vram_driver_dumb_create);
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index 8eb7258b236a..50a0c1f9d211 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -18,7 +18,6 @@
 #include <drm/drm_framebuffer.h>
 
 struct drm_device;
-struct drm_gem_object;
 
 struct hibmc_drm_private {
 	/* hw */
@@ -41,9 +40,6 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
 int hibmc_de_init(struct hibmc_drm_private *priv);
 int hibmc_vdac_init(struct hibmc_drm_private *priv);
 
-int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
-		     struct drm_gem_object **obj);
-
 int hibmc_mm_init(struct hibmc_drm_private *hibmc);
 void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
 int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
index f6d25b85c209..0af5d966a480 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
@@ -47,55 +47,11 @@ void hibmc_mm_fini(struct hibmc_drm_private *hibmc)
 	drm_vram_helper_release_mm(hibmc->dev);
 }
 
-int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
-		     struct drm_gem_object **obj)
-{
-	struct drm_gem_vram_object *gbo;
-	int ret;
-
-	*obj = NULL;
-
-	size = roundup(size, PAGE_SIZE);
-	if (size == 0)
-		return -EINVAL;
-
-	gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, size, 0, false);
-	if (IS_ERR(gbo)) {
-		ret = PTR_ERR(gbo);
-		if (ret != -ERESTARTSYS)
-			DRM_ERROR("failed to allocate GEM object: %d\n", ret);
-		return ret;
-	}
-	*obj = &gbo->bo.base;
-	return 0;
-}
-
 int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
 		      struct drm_mode_create_dumb *args)
 {
-	struct drm_gem_object *gobj;
-	u32 handle;
-	int ret;
-
-	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 16);
-	args->size = args->pitch * args->height;
-
-	ret = hibmc_gem_create(dev, args->size, false,
-			       &gobj);
-	if (ret) {
-		DRM_ERROR("failed to create GEM object: %d\n", ret);
-		return ret;
-	}
-
-	ret = drm_gem_handle_create(file, gobj, &handle);
-	drm_gem_object_put_unlocked(gobj);
-	if (ret) {
-		DRM_ERROR("failed to unreference GEM object: %d\n", ret);
-		return ret;
-	}
-
-	args->handle = handle;
-	return 0;
+	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
+					     0, 16, false, args);
 }
 
 const struct drm_mode_config_funcs hibmc_mode_funcs = {
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index e040541a105f..c642b4cb6600 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -113,6 +113,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
 				  struct drm_device *dev,
 				  struct ttm_bo_device *bdev,
 				  unsigned long pg_align,
+				  unsigned long pitch_align,
 				  bool interruptible,
 				  struct drm_mode_create_dumb *args);
 
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/hisilicon/hibmc: Export VRAM MM information to debugfs

[Thomas Zimmermann]

This change makes information about VRAM consumption available on
debugfs. See

  /sys/kernel/debug/dri/0/vram-mm

for an overview of how VRAM is being used.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 113d27b8a8f1..11d1b0761c9a 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -55,6 +55,7 @@ static 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_vram_driver_dumb_mmap_offset,
 	.gem_prime_mmap		= drm_gem_prime_mmap,
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/hisilicon/hibmc: Implement hibmc_dumb_create() with generic helpers

[Sam Ravnborg]

Hi Thomas.

Change looks good. If you spin this could you move the
changes to generic drm code to a separate patch?
As it is now it is hidden for most.
But then there are also no users of drm_gem_vram_fill_create_dumb()

One small nit below that you can safely ignore :-)


	Sam


On Fri, Nov 22, 2019 at 09:30:43AM +0100, Thomas Zimmermann wrote:
> The hibmc driver aligns scanlines to 16 bytes. Adding the pitch alignment
> as an argument to drm_gem_vram_fill_create_dumb() allows to use the generic
> implementation with hibmc. A value of 0 disables scanline pitches.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_gem_vram_helper.c         | 10 ++--
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  4 --
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 48 +------------------
>  include/drm/drm_gem_vram_helper.h             |  1 +
>  4 files changed, 10 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 666cb4c22bb9..f098784e7dfd 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -485,6 +485,7 @@ EXPORT_SYMBOL(drm_gem_vram_vunmap);
>   * @dev:		the DRM device
>   * @bdev:		the TTM BO device managing the buffer object
>   * @pg_align:		the buffer's alignment in multiples of the page size
> + * @pitch_align:	the scanline's alignment in powers of 2
>   * @interruptible:	sleep interruptible if waiting for memory
>   * @args:		the arguments as provided to \
>  				&struct drm_driver.dumb_create
> @@ -502,6 +503,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>  				  struct drm_device *dev,
>  				  struct ttm_bo_device *bdev,
>  				  unsigned long pg_align,
> +				  unsigned long pitch_align,
>  				  bool interruptible,
>  				  struct drm_mode_create_dumb *args)
>  {
> @@ -510,7 +512,9 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>  	int ret;
>  	u32 handle;
>  
> -	pitch = args->width * ((args->bpp + 7) / 8);
> +	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
Semi related change...

> +	if (pitch_align)
> +		pitch = ALIGN(pitch, pitch_align);
>  	size = pitch * args->height;
>  
>  	size = roundup(size, PAGE_SIZE);
> @@ -612,8 +616,8 @@ int drm_gem_vram_driver_dumb_create(struct drm_file *file,
>  	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
>  		return -EINVAL;
>  
> -	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev, 0,
> -					     false, args);
> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
> +					     0, 0, false, args);
>  }
>  EXPORT_SYMBOL(drm_gem_vram_driver_dumb_create);
>  
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index 8eb7258b236a..50a0c1f9d211 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -18,7 +18,6 @@
>  #include <drm/drm_framebuffer.h>
>  
>  struct drm_device;
> -struct drm_gem_object;
>  
>  struct hibmc_drm_private {
>  	/* hw */
> @@ -41,9 +40,6 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
>  int hibmc_de_init(struct hibmc_drm_private *priv);
>  int hibmc_vdac_init(struct hibmc_drm_private *priv);
>  
> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
> -		     struct drm_gem_object **obj);
> -
>  int hibmc_mm_init(struct hibmc_drm_private *hibmc);
>  void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> index f6d25b85c209..0af5d966a480 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> @@ -47,55 +47,11 @@ void hibmc_mm_fini(struct hibmc_drm_private *hibmc)
>  	drm_vram_helper_release_mm(hibmc->dev);
>  }
>  
> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
> -		     struct drm_gem_object **obj)
> -{
> -	struct drm_gem_vram_object *gbo;
> -	int ret;
> -
> -	*obj = NULL;
> -
> -	size = roundup(size, PAGE_SIZE);
> -	if (size == 0)
> -		return -EINVAL;
> -
> -	gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, size, 0, false);
> -	if (IS_ERR(gbo)) {
> -		ret = PTR_ERR(gbo);
> -		if (ret != -ERESTARTSYS)
> -			DRM_ERROR("failed to allocate GEM object: %d\n", ret);
> -		return ret;
> -	}
> -	*obj = &gbo->bo.base;
> -	return 0;
> -}
> -
>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>  		      struct drm_mode_create_dumb *args)
>  {
> -	struct drm_gem_object *gobj;
> -	u32 handle;
> -	int ret;
> -
> -	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 16);
> -	args->size = args->pitch * args->height;
> -
> -	ret = hibmc_gem_create(dev, args->size, false,
> -			       &gobj);
> -	if (ret) {
> -		DRM_ERROR("failed to create GEM object: %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = drm_gem_handle_create(file, gobj, &handle);
> -	drm_gem_object_put_unlocked(gobj);
> -	if (ret) {
> -		DRM_ERROR("failed to unreference GEM object: %d\n", ret);
> -		return ret;
> -	}
> -
> -	args->handle = handle;
> -	return 0;
> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
> +					     0, 16, false, args);
>  }
>  
>  const struct drm_mode_config_funcs hibmc_mode_funcs = {
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index e040541a105f..c642b4cb6600 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -113,6 +113,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>  				  struct drm_device *dev,
>  				  struct ttm_bo_device *bdev,
>  				  unsigned long pg_align,
> +				  unsigned long pitch_align,
>  				  bool interruptible,
>  				  struct drm_mode_create_dumb *args);
>  
> -- 
> 2.23.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/4] Replace hibmc code with generic implmentation

[Sam Ravnborg]

Hi Thomas.


> The patch set replaces code in hibmc with generic implementation.
> 
> Patches 1 to 3 replace fbdev emuation, framebuffer and creation of
> dumb buffers with respective code from DRM helpers. Patch 4 adds an
> additional interface to debugfs that displays the allocated and free
> areas in video memory.
> 
> The patches have only been compile-tested. Further testing is
> appreciated.

Changes looks good and diffstat is very nice.
From my quick browsing everything was fine, a small comment on
one of the patches.

You can add my:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

to the whole series.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/hisilicon/hibmc: Switch to generic fbdev emulation

[Daniel Vetter]

On Fri, Nov 22, 2019 at 09:30:41AM +0100, Thomas Zimmermann wrote:
> There's nothing special about hibmc's fbdev emulation that is not
> provided by the generic implementation. Switch over and remove the
> driver's code.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/Makefile      |   2 +-
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |   5 +-
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  11 -
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 240 ------------------
>  4 files changed, 3 insertions(+), 255 deletions(-)
>  delete mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index 0c2d4296bccd..f99132715597 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,4 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_fbdev.o hibmc_ttm.o
> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_ttm.o
>  
>  obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 2fd4ca91a62d..113d27b8a8f1 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -17,6 +17,7 @@
>  
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_drv.h>
> +#include <drm/drm_fb_helper.h>
>  #include <drm/drm_gem_vram_helper.h>
>  #include <drm/drm_irq.h>
>  #include <drm/drm_print.h>
> @@ -247,8 +248,6 @@ static int hibmc_unload(struct drm_device *dev)
>  {
>  	struct hibmc_drm_private *priv = dev->dev_private;
>  
> -	hibmc_fbdev_fini(priv);
> -
>  	drm_atomic_helper_shutdown(dev);
>  
>  	if (dev->irq_enabled)
> @@ -307,7 +306,7 @@ static int hibmc_load(struct drm_device *dev)
>  	/* reset all the states of crtc/plane/encoder/connector */
>  	drm_mode_config_reset(dev);
>  
> -	ret = hibmc_fbdev_init(priv);
> +	ret = drm_fbdev_generic_setup(dev, 16);

Nice, +1 line, -lots of lines!

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	if (ret) {
>  		DRM_ERROR("failed to initialize fbdev: %d\n", ret);
>  		goto err;
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index e58ecd7edcf8..b34493ead30b 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -25,12 +25,6 @@ struct hibmc_framebuffer {
>  	struct drm_gem_object *obj;
>  };
>  
> -struct hibmc_fbdev {
> -	struct drm_fb_helper helper; /* must be first */
> -	struct hibmc_framebuffer *fb;
> -	int size;
> -};
> -
>  struct hibmc_drm_private {
>  	/* hw */
>  	void __iomem   *mmio;
> @@ -42,9 +36,6 @@ struct hibmc_drm_private {
>  	/* drm */
>  	struct drm_device  *dev;
>  	bool mode_config_initialized;
> -
> -	/* fbdev */
> -	struct hibmc_fbdev *fbdev;
>  };
>  
>  #define to_hibmc_framebuffer(x) container_of(x, struct hibmc_framebuffer, fb)
> @@ -56,8 +47,6 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
>  
>  int hibmc_de_init(struct hibmc_drm_private *priv);
>  int hibmc_vdac_init(struct hibmc_drm_private *priv);
> -int hibmc_fbdev_init(struct hibmc_drm_private *priv);
> -void hibmc_fbdev_fini(struct hibmc_drm_private *priv);
>  
>  int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>  		     struct drm_gem_object **obj);
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
> deleted file mode 100644
> index b4c1cea051e8..000000000000
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
> +++ /dev/null
> @@ -1,240 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-or-later
> -/* Hisilicon Hibmc SoC drm driver
> - *
> - * Based on the bochs drm driver.
> - *
> - * Copyright (c) 2016 Huawei Limited.
> - *
> - * Author:
> - *	Rongrong Zou <zourongrong@huawei.com>
> - *	Rongrong Zou <zourongrong@gmail.com>
> - *	Jianhua Li <lijianhua@huawei.com>
> - */
> -
> -#include <drm/drm_crtc.h>
> -#include <drm/drm_fb_helper.h>
> -#include <drm/drm_fourcc.h>
> -#include <drm/drm_gem_vram_helper.h>
> -#include <drm/drm_probe_helper.h>
> -
> -#include "hibmc_drm_drv.h"
> -
> -static int hibmcfb_create_object(
> -				struct hibmc_drm_private *priv,
> -				const struct drm_mode_fb_cmd2 *mode_cmd,
> -				struct drm_gem_object **gobj_p)
> -{
> -	struct drm_gem_object *gobj;
> -	struct drm_device *dev = priv->dev;
> -	u32 size;
> -	int ret = 0;
> -
> -	size = mode_cmd->pitches[0] * mode_cmd->height;
> -	ret = hibmc_gem_create(dev, size, true, &gobj);
> -	if (ret)
> -		return ret;
> -
> -	*gobj_p = gobj;
> -	return ret;
> -}
> -
> -static struct fb_ops hibmc_drm_fb_ops = {
> -	.owner = THIS_MODULE,
> -	.fb_check_var = drm_fb_helper_check_var,
> -	.fb_set_par = drm_fb_helper_set_par,
> -	.fb_fillrect = drm_fb_helper_sys_fillrect,
> -	.fb_copyarea = drm_fb_helper_sys_copyarea,
> -	.fb_imageblit = drm_fb_helper_sys_imageblit,
> -	.fb_pan_display = drm_fb_helper_pan_display,
> -	.fb_blank = drm_fb_helper_blank,
> -	.fb_setcmap = drm_fb_helper_setcmap,
> -};
> -
> -static int hibmc_drm_fb_create(struct drm_fb_helper *helper,
> -			       struct drm_fb_helper_surface_size *sizes)
> -{
> -	struct hibmc_fbdev *hi_fbdev =
> -		container_of(helper, struct hibmc_fbdev, helper);
> -	struct hibmc_drm_private *priv = helper->dev->dev_private;
> -	struct fb_info *info;
> -	struct drm_mode_fb_cmd2 mode_cmd;
> -	struct drm_gem_object *gobj = NULL;
> -	int ret = 0;
> -	size_t size;
> -	unsigned int bytes_per_pixel;
> -	struct drm_gem_vram_object *gbo = NULL;
> -	void *base;
> -
> -	DRM_DEBUG_DRIVER("surface width(%d), height(%d) and bpp(%d)\n",
> -			 sizes->surface_width, sizes->surface_height,
> -			 sizes->surface_bpp);
> -
> -	bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
> -
> -	mode_cmd.width = sizes->surface_width;
> -	mode_cmd.height = sizes->surface_height;
> -	mode_cmd.pitches[0] = mode_cmd.width * bytes_per_pixel;
> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> -							  sizes->surface_depth);
> -
> -	size = PAGE_ALIGN(mode_cmd.pitches[0] * mode_cmd.height);
> -
> -	ret = hibmcfb_create_object(priv, &mode_cmd, &gobj);
> -	if (ret) {
> -		DRM_ERROR("failed to create fbcon backing object: %d\n", ret);
> -		return -ENOMEM;
> -	}
> -
> -	gbo = drm_gem_vram_of_gem(gobj);
> -
> -	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
> -	if (ret) {
> -		DRM_ERROR("failed to pin fbcon: %d\n", ret);
> -		goto out_unref_gem;
> -	}
> -
> -	base = drm_gem_vram_kmap(gbo, true, NULL);
> -	if (IS_ERR(base)) {
> -		ret = PTR_ERR(base);
> -		DRM_ERROR("failed to kmap fbcon: %d\n", ret);
> -		goto out_unpin_bo;
> -	}
> -
> -	info = drm_fb_helper_alloc_fbi(helper);
> -	if (IS_ERR(info)) {
> -		ret = PTR_ERR(info);
> -		DRM_ERROR("failed to allocate fbi: %d\n", ret);
> -		goto out_release_fbi;
> -	}
> -
> -	hi_fbdev->fb = hibmc_framebuffer_init(priv->dev, &mode_cmd, gobj);
> -	if (IS_ERR(hi_fbdev->fb)) {
> -		ret = PTR_ERR(hi_fbdev->fb);
> -		hi_fbdev->fb = NULL;
> -		DRM_ERROR("failed to initialize framebuffer: %d\n", ret);
> -		goto out_release_fbi;
> -	}
> -
> -	priv->fbdev->size = size;
> -	hi_fbdev->helper.fb = &hi_fbdev->fb->fb;
> -
> -	info->fbops = &hibmc_drm_fb_ops;
> -
> -	drm_fb_helper_fill_info(info, &priv->fbdev->helper, sizes);
> -
> -	info->screen_base = base;
> -	info->screen_size = size;
> -
> -	info->fix.smem_start = gbo->bo.mem.bus.offset + gbo->bo.mem.bus.base;
> -	info->fix.smem_len = size;
> -	return 0;
> -
> -out_release_fbi:
> -	drm_gem_vram_kunmap(gbo);
> -out_unpin_bo:
> -	drm_gem_vram_unpin(gbo);
> -out_unref_gem:
> -	drm_gem_object_put_unlocked(gobj);
> -
> -	return ret;
> -}
> -
> -static void hibmc_fbdev_destroy(struct hibmc_fbdev *fbdev)
> -{
> -	struct hibmc_framebuffer *gfb = fbdev->fb;
> -	struct drm_fb_helper *fbh = &fbdev->helper;
> -
> -	drm_fb_helper_unregister_fbi(fbh);
> -
> -	drm_fb_helper_fini(fbh);
> -
> -	if (gfb)
> -		drm_framebuffer_put(&gfb->fb);
> -}
> -
> -static const struct drm_fb_helper_funcs hibmc_fbdev_helper_funcs = {
> -	.fb_probe = hibmc_drm_fb_create,
> -};
> -
> -int hibmc_fbdev_init(struct hibmc_drm_private *priv)
> -{
> -	int ret;
> -	struct fb_var_screeninfo *var;
> -	struct fb_fix_screeninfo *fix;
> -	struct hibmc_fbdev *hifbdev;
> -
> -	hifbdev = devm_kzalloc(priv->dev->dev, sizeof(*hifbdev), GFP_KERNEL);
> -	if (!hifbdev) {
> -		DRM_ERROR("failed to allocate hibmc_fbdev\n");
> -		return -ENOMEM;
> -	}
> -
> -	priv->fbdev = hifbdev;
> -	drm_fb_helper_prepare(priv->dev, &hifbdev->helper,
> -			      &hibmc_fbdev_helper_funcs);
> -
> -	/* Now just one crtc and one channel */
> -	ret = drm_fb_helper_init(priv->dev, &hifbdev->helper, 1);
> -	if (ret) {
> -		DRM_ERROR("failed to initialize fb helper: %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = drm_fb_helper_single_add_all_connectors(&hifbdev->helper);
> -	if (ret) {
> -		DRM_ERROR("failed to add all connectors: %d\n", ret);
> -		goto fini;
> -	}
> -
> -	ret = drm_fb_helper_initial_config(&hifbdev->helper, 16);
> -	if (ret) {
> -		DRM_ERROR("failed to setup initial conn config: %d\n", ret);
> -		goto fini;
> -	}
> -
> -	var = &hifbdev->helper.fbdev->var;
> -	fix = &hifbdev->helper.fbdev->fix;
> -
> -	DRM_DEBUG_DRIVER("Member of info->var is :\n"
> -			 "xres=%d\n"
> -			 "yres=%d\n"
> -			 "xres_virtual=%d\n"
> -			 "yres_virtual=%d\n"
> -			 "xoffset=%d\n"
> -			 "yoffset=%d\n"
> -			 "bits_per_pixel=%d\n"
> -			 "...\n", var->xres, var->yres, var->xres_virtual,
> -			 var->yres_virtual, var->xoffset, var->yoffset,
> -			 var->bits_per_pixel);
> -	DRM_DEBUG_DRIVER("Member of info->fix is :\n"
> -			 "smem_start=%lx\n"
> -			 "smem_len=%d\n"
> -			 "type=%d\n"
> -			 "type_aux=%d\n"
> -			 "visual=%d\n"
> -			 "xpanstep=%d\n"
> -			 "ypanstep=%d\n"
> -			 "ywrapstep=%d\n"
> -			 "line_length=%d\n"
> -			 "accel=%d\n"
> -			 "capabilities=%d\n"
> -			 "...\n", fix->smem_start, fix->smem_len, fix->type,
> -			 fix->type_aux, fix->visual, fix->xpanstep,
> -			 fix->ypanstep, fix->ywrapstep, fix->line_length,
> -			 fix->accel, fix->capabilities);
> -
> -	return 0;
> -
> -fini:
> -	drm_fb_helper_fini(&hifbdev->helper);
> -	return ret;
> -}
> -
> -void hibmc_fbdev_fini(struct hibmc_drm_private *priv)
> -{
> -	if (!priv->fbdev)
> -		return;
> -
> -	hibmc_fbdev_destroy(priv->fbdev);
> -	priv->fbdev = NULL;
> -}
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/hisilicon/hibmc: Replace struct hibmc_framebuffer with generic code

[Daniel Vetter]

On Fri, Nov 22, 2019 at 09:30:42AM +0100, Thomas Zimmermann wrote:
> The hibmc driver's struct hibmc_framebuffer stores a DRM framebuffer
> with an associated GEM object. This functionality is also provided by
> generic code. Switch hibmc over.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Aside from the one silly compile fail I managed to do and not spot this
matches what I've done.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    |  4 +-
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   | 11 ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 69 +------------------
>  3 files changed, 3 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> index 6527a97f68a3..7fa7d4933f60 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> @@ -99,14 +99,12 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
>  	s64 gpu_addr = 0;
>  	unsigned int line_l;
>  	struct hibmc_drm_private *priv = plane->dev->dev_private;
> -	struct hibmc_framebuffer *hibmc_fb;
>  	struct drm_gem_vram_object *gbo;
>  
>  	if (!state->fb)
>  		return;
>  
> -	hibmc_fb = to_hibmc_framebuffer(state->fb);
> -	gbo = drm_gem_vram_of_gem(hibmc_fb->obj);
> +	gbo = drm_gem_vram_of_gem(state->fb->obj[0]);
>  
>  	gpu_addr = drm_gem_vram_offset(gbo);
>  	if (WARN_ON_ONCE(gpu_addr < 0))
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index b34493ead30b..8eb7258b236a 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -20,11 +20,6 @@
>  struct drm_device;
>  struct drm_gem_object;
>  
> -struct hibmc_framebuffer {
> -	struct drm_framebuffer fb;
> -	struct drm_gem_object *obj;
> -};
> -
>  struct hibmc_drm_private {
>  	/* hw */
>  	void __iomem   *mmio;
> @@ -38,8 +33,6 @@ struct hibmc_drm_private {
>  	bool mode_config_initialized;
>  };
>  
> -#define to_hibmc_framebuffer(x) container_of(x, struct hibmc_framebuffer, fb)
> -
>  void hibmc_set_power_mode(struct hibmc_drm_private *priv,
>  			  unsigned int power_mode);
>  void hibmc_set_current_gate(struct hibmc_drm_private *priv,
> @@ -50,10 +43,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv);
>  
>  int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>  		     struct drm_gem_object **obj);
> -struct hibmc_framebuffer *
> -hibmc_framebuffer_init(struct drm_device *dev,
> -		       const struct drm_mode_fb_cmd2 *mode_cmd,
> -		       struct drm_gem_object *obj);
>  
>  int hibmc_mm_init(struct hibmc_drm_private *hibmc);
>  void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> index 21b684eab5c9..f6d25b85c209 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> @@ -15,6 +15,7 @@
>  
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_gem_vram_helper.h>
>  #include <drm/drm_print.h>
>  
> @@ -97,74 +98,8 @@ int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>  	return 0;
>  }
>  
> -static void hibmc_user_framebuffer_destroy(struct drm_framebuffer *fb)
> -{
> -	struct hibmc_framebuffer *hibmc_fb = to_hibmc_framebuffer(fb);
> -
> -	drm_gem_object_put_unlocked(hibmc_fb->obj);
> -	drm_framebuffer_cleanup(fb);
> -	kfree(hibmc_fb);
> -}
> -
> -static const struct drm_framebuffer_funcs hibmc_fb_funcs = {
> -	.destroy = hibmc_user_framebuffer_destroy,
> -};
> -
> -struct hibmc_framebuffer *
> -hibmc_framebuffer_init(struct drm_device *dev,
> -		       const struct drm_mode_fb_cmd2 *mode_cmd,
> -		       struct drm_gem_object *obj)
> -{
> -	struct hibmc_framebuffer *hibmc_fb;
> -	int ret;
> -
> -	hibmc_fb = kzalloc(sizeof(*hibmc_fb), GFP_KERNEL);
> -	if (!hibmc_fb) {
> -		DRM_ERROR("failed to allocate hibmc_fb\n");
> -		return ERR_PTR(-ENOMEM);
> -	}
> -
> -	drm_helper_mode_fill_fb_struct(dev, &hibmc_fb->fb, mode_cmd);
> -	hibmc_fb->obj = obj;
> -	ret = drm_framebuffer_init(dev, &hibmc_fb->fb, &hibmc_fb_funcs);
> -	if (ret) {
> -		DRM_ERROR("drm_framebuffer_init failed: %d\n", ret);
> -		kfree(hibmc_fb);
> -		return ERR_PTR(ret);
> -	}
> -
> -	return hibmc_fb;
> -}
> -
> -static struct drm_framebuffer *
> -hibmc_user_framebuffer_create(struct drm_device *dev,
> -			      struct drm_file *filp,
> -			      const struct drm_mode_fb_cmd2 *mode_cmd)
> -{
> -	struct drm_gem_object *obj;
> -	struct hibmc_framebuffer *hibmc_fb;
> -
> -	DRM_DEBUG_DRIVER("%dx%d, format %c%c%c%c\n",
> -			 mode_cmd->width, mode_cmd->height,
> -			 (mode_cmd->pixel_format) & 0xff,
> -			 (mode_cmd->pixel_format >> 8)  & 0xff,
> -			 (mode_cmd->pixel_format >> 16) & 0xff,
> -			 (mode_cmd->pixel_format >> 24) & 0xff);
> -
> -	obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
> -	if (!obj)
> -		return ERR_PTR(-ENOENT);
> -
> -	hibmc_fb = hibmc_framebuffer_init(dev, mode_cmd, obj);
> -	if (IS_ERR(hibmc_fb)) {
> -		drm_gem_object_put_unlocked(obj);
> -		return ERR_PTR((long)hibmc_fb);
> -	}
> -	return &hibmc_fb->fb;
> -}
> -
>  const struct drm_mode_config_funcs hibmc_mode_funcs = {
>  	.atomic_check = drm_atomic_helper_check,
>  	.atomic_commit = drm_atomic_helper_commit,
> -	.fb_create = hibmc_user_framebuffer_create,
> +	.fb_create = drm_gem_fb_create,
>  };
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/hisilicon/hibmc: Implement hibmc_dumb_create() with generic helpers

[Daniel Vetter]

On Fri, Nov 22, 2019 at 09:30:43AM +0100, Thomas Zimmermann wrote:
> The hibmc driver aligns scanlines to 16 bytes. Adding the pitch alignment
> as an argument to drm_gem_vram_fill_create_dumb() allows to use the generic
> implementation with hibmc. A value of 0 disables scanline pitches.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

I concur with Sam, the vram change should be split out.

> ---
>  drivers/gpu/drm/drm_gem_vram_helper.c         | 10 ++--
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  4 --
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 48 +------------------
>  include/drm/drm_gem_vram_helper.h             |  1 +
>  4 files changed, 10 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 666cb4c22bb9..f098784e7dfd 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -485,6 +485,7 @@ EXPORT_SYMBOL(drm_gem_vram_vunmap);
>   * @dev:		the DRM device
>   * @bdev:		the TTM BO device managing the buffer object
>   * @pg_align:		the buffer's alignment in multiples of the page size
> + * @pitch_align:	the scanline's alignment in powers of 2
>   * @interruptible:	sleep interruptible if waiting for memory

I also noticed that no one sets this to true, neither here nor in
drm_gem_vram_create(). Maybe remove that too? Otherwise the argument list
becomes very unwielding. And you're already touching the (few) callers.

>   * @args:		the arguments as provided to \
>  				&struct drm_driver.dumb_create
> @@ -502,6 +503,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>  				  struct drm_device *dev,
>  				  struct ttm_bo_device *bdev,
>  				  unsigned long pg_align,
> +				  unsigned long pitch_align,
>  				  bool interruptible,
>  				  struct drm_mode_create_dumb *args)
>  {
> @@ -510,7 +512,9 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>  	int ret;
>  	u32 handle;
>  
> -	pitch = args->width * ((args->bpp + 7) / 8);
> +	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> +	if (pitch_align)
> +		pitch = ALIGN(pitch, pitch_align);

Maybe throw a WARN_IS(is_pot(align)) in here?

Cheers, Daniel

>  	size = pitch * args->height;
>  
>  	size = roundup(size, PAGE_SIZE);
> @@ -612,8 +616,8 @@ int drm_gem_vram_driver_dumb_create(struct drm_file *file,
>  	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
>  		return -EINVAL;
>  
> -	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev, 0,
> -					     false, args);
> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
> +					     0, 0, false, args);
>  }
>  EXPORT_SYMBOL(drm_gem_vram_driver_dumb_create);
>  
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index 8eb7258b236a..50a0c1f9d211 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -18,7 +18,6 @@
>  #include <drm/drm_framebuffer.h>
>  
>  struct drm_device;
> -struct drm_gem_object;
>  
>  struct hibmc_drm_private {
>  	/* hw */
> @@ -41,9 +40,6 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
>  int hibmc_de_init(struct hibmc_drm_private *priv);
>  int hibmc_vdac_init(struct hibmc_drm_private *priv);
>  
> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
> -		     struct drm_gem_object **obj);
> -
>  int hibmc_mm_init(struct hibmc_drm_private *hibmc);
>  void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> index f6d25b85c209..0af5d966a480 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> @@ -47,55 +47,11 @@ void hibmc_mm_fini(struct hibmc_drm_private *hibmc)
>  	drm_vram_helper_release_mm(hibmc->dev);
>  }
>  
> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
> -		     struct drm_gem_object **obj)
> -{
> -	struct drm_gem_vram_object *gbo;
> -	int ret;
> -
> -	*obj = NULL;
> -
> -	size = roundup(size, PAGE_SIZE);
> -	if (size == 0)
> -		return -EINVAL;
> -
> -	gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, size, 0, false);
> -	if (IS_ERR(gbo)) {
> -		ret = PTR_ERR(gbo);
> -		if (ret != -ERESTARTSYS)
> -			DRM_ERROR("failed to allocate GEM object: %d\n", ret);
> -		return ret;
> -	}
> -	*obj = &gbo->bo.base;
> -	return 0;
> -}
> -
>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>  		      struct drm_mode_create_dumb *args)
>  {
> -	struct drm_gem_object *gobj;
> -	u32 handle;
> -	int ret;
> -
> -	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 16);
> -	args->size = args->pitch * args->height;
> -
> -	ret = hibmc_gem_create(dev, args->size, false,
> -			       &gobj);
> -	if (ret) {
> -		DRM_ERROR("failed to create GEM object: %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = drm_gem_handle_create(file, gobj, &handle);
> -	drm_gem_object_put_unlocked(gobj);
> -	if (ret) {
> -		DRM_ERROR("failed to unreference GEM object: %d\n", ret);
> -		return ret;
> -	}
> -
> -	args->handle = handle;
> -	return 0;
> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
> +					     0, 16, false, args);
>  }
>  
>  const struct drm_mode_config_funcs hibmc_mode_funcs = {
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index e040541a105f..c642b4cb6600 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -113,6 +113,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>  				  struct drm_device *dev,
>  				  struct ttm_bo_device *bdev,
>  				  unsigned long pg_align,
> +				  unsigned long pitch_align,
>  				  bool interruptible,
>  				  struct drm_mode_create_dumb *args);
>  
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/hisilicon/hibmc: Export VRAM MM information to debugfs

[Daniel Vetter]

On Fri, Nov 22, 2019 at 09:30:44AM +0100, Thomas Zimmermann wrote:
> This change makes information about VRAM consumption available on
> debugfs. See
> 
>   /sys/kernel/debug/dri/0/vram-mm
> 
> for an overview of how VRAM is being used.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Wrt merging: I'd wait 2-3 weeks for testing from maintainers, then merge
either way. Either it works, or we've found someone who cares enough to
test patches if something broke in here :-)
-Daniel

> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 113d27b8a8f1..11d1b0761c9a 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -55,6 +55,7 @@ static 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_vram_driver_dumb_mmap_offset,
>  	.gem_prime_mmap		= drm_gem_prime_mmap,
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/hisilicon/hibmc: Implement hibmc_dumb_create() with generic helpers

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 6802 bytes --]

Hi

Am 25.11.19 um 10:14 schrieb Daniel Vetter:
> On Fri, Nov 22, 2019 at 09:30:43AM +0100, Thomas Zimmermann wrote:
>> The hibmc driver aligns scanlines to 16 bytes. Adding the pitch alignment
>> as an argument to drm_gem_vram_fill_create_dumb() allows to use the generic
>> implementation with hibmc. A value of 0 disables scanline pitches.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> I concur with Sam, the vram change should be split out.
> 
>> ---
>>  drivers/gpu/drm/drm_gem_vram_helper.c         | 10 ++--
>>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  4 --
>>  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 48 +------------------
>>  include/drm/drm_gem_vram_helper.h             |  1 +
>>  4 files changed, 10 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
>> index 666cb4c22bb9..f098784e7dfd 100644
>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>> @@ -485,6 +485,7 @@ EXPORT_SYMBOL(drm_gem_vram_vunmap);
>>   * @dev:		the DRM device
>>   * @bdev:		the TTM BO device managing the buffer object
>>   * @pg_align:		the buffer's alignment in multiples of the page size
>> + * @pitch_align:	the scanline's alignment in powers of 2
>>   * @interruptible:	sleep interruptible if waiting for memory
> 
> I also noticed that no one sets this to true, neither here nor in
> drm_gem_vram_create(). Maybe remove that too? Otherwise the argument list
> becomes very unwielding. And you're already touching the (few) callers.

OK, I'll add this as a separate patch.

BTW What's the DRM interface's behavior wrt interruption? For example,
can a ioctl call like CREATE_DUMB return EINTR to userspace?

Best regards
Thomas

> 
>>   * @args:		the arguments as provided to \
>>  				&struct drm_driver.dumb_create
>> @@ -502,6 +503,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>>  				  struct drm_device *dev,
>>  				  struct ttm_bo_device *bdev,
>>  				  unsigned long pg_align,
>> +				  unsigned long pitch_align,
>>  				  bool interruptible,
>>  				  struct drm_mode_create_dumb *args)
>>  {
>> @@ -510,7 +512,9 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>>  	int ret;
>>  	u32 handle;
>>  
>> -	pitch = args->width * ((args->bpp + 7) / 8);
>> +	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
>> +	if (pitch_align)
>> +		pitch = ALIGN(pitch, pitch_align);
> 
> Maybe throw a WARN_IS(is_pot(align)) in here?
> 
> Cheers, Daniel
> 
>>  	size = pitch * args->height;
>>  
>>  	size = roundup(size, PAGE_SIZE);
>> @@ -612,8 +616,8 @@ int drm_gem_vram_driver_dumb_create(struct drm_file *file,
>>  	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
>>  		return -EINVAL;
>>  
>> -	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev, 0,
>> -					     false, args);
>> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
>> +					     0, 0, false, args);
>>  }
>>  EXPORT_SYMBOL(drm_gem_vram_driver_dumb_create);
>>  
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index 8eb7258b236a..50a0c1f9d211 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -18,7 +18,6 @@
>>  #include <drm/drm_framebuffer.h>
>>  
>>  struct drm_device;
>> -struct drm_gem_object;
>>  
>>  struct hibmc_drm_private {
>>  	/* hw */
>> @@ -41,9 +40,6 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
>>  int hibmc_de_init(struct hibmc_drm_private *priv);
>>  int hibmc_vdac_init(struct hibmc_drm_private *priv);
>>  
>> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>> -		     struct drm_gem_object **obj);
>> -
>>  int hibmc_mm_init(struct hibmc_drm_private *hibmc);
>>  void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
>>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> index f6d25b85c209..0af5d966a480 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> @@ -47,55 +47,11 @@ void hibmc_mm_fini(struct hibmc_drm_private *hibmc)
>>  	drm_vram_helper_release_mm(hibmc->dev);
>>  }
>>  
>> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>> -		     struct drm_gem_object **obj)
>> -{
>> -	struct drm_gem_vram_object *gbo;
>> -	int ret;
>> -
>> -	*obj = NULL;
>> -
>> -	size = roundup(size, PAGE_SIZE);
>> -	if (size == 0)
>> -		return -EINVAL;
>> -
>> -	gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, size, 0, false);
>> -	if (IS_ERR(gbo)) {
>> -		ret = PTR_ERR(gbo);
>> -		if (ret != -ERESTARTSYS)
>> -			DRM_ERROR("failed to allocate GEM object: %d\n", ret);
>> -		return ret;
>> -	}
>> -	*obj = &gbo->bo.base;
>> -	return 0;
>> -}
>> -
>>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>>  		      struct drm_mode_create_dumb *args)
>>  {
>> -	struct drm_gem_object *gobj;
>> -	u32 handle;
>> -	int ret;
>> -
>> -	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 16);
>> -	args->size = args->pitch * args->height;
>> -
>> -	ret = hibmc_gem_create(dev, args->size, false,
>> -			       &gobj);
>> -	if (ret) {
>> -		DRM_ERROR("failed to create GEM object: %d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	ret = drm_gem_handle_create(file, gobj, &handle);
>> -	drm_gem_object_put_unlocked(gobj);
>> -	if (ret) {
>> -		DRM_ERROR("failed to unreference GEM object: %d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	args->handle = handle;
>> -	return 0;
>> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
>> +					     0, 16, false, args);
>>  }
>>  
>>  const struct drm_mode_config_funcs hibmc_mode_funcs = {
>> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
>> index e040541a105f..c642b4cb6600 100644
>> --- a/include/drm/drm_gem_vram_helper.h
>> +++ b/include/drm/drm_gem_vram_helper.h
>> @@ -113,6 +113,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>>  				  struct drm_device *dev,
>>  				  struct ttm_bo_device *bdev,
>>  				  unsigned long pg_align,
>> +				  unsigned long pitch_align,
>>  				  bool interruptible,
>>  				  struct drm_mode_create_dumb *args);
>>  
>> -- 
>> 2.23.0
>>
> 

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


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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/hisilicon/hibmc: Implement hibmc_dumb_create() with generic helpers

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 6884 bytes --]

Hi

thanks for the review.

Am 23.11.19 um 09:56 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> Change looks good. If you spin this could you move the
> changes to generic drm code to a separate patch?
> As it is now it is hidden for most.
> But then there are also no users of drm_gem_vram_fill_create_dumb()

I just posted a patchset for mgag200 that uses this function. Maybe
it'll have to stay.

Best regards
Thomas

> 
> One small nit below that you can safely ignore :-)
> 
> 
> 	Sam
> 
> 
> On Fri, Nov 22, 2019 at 09:30:43AM +0100, Thomas Zimmermann wrote:
>> The hibmc driver aligns scanlines to 16 bytes. Adding the pitch alignment
>> as an argument to drm_gem_vram_fill_create_dumb() allows to use the generic
>> implementation with hibmc. A value of 0 disables scanline pitches.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/drm_gem_vram_helper.c         | 10 ++--
>>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  4 --
>>  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 48 +------------------
>>  include/drm/drm_gem_vram_helper.h             |  1 +
>>  4 files changed, 10 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
>> index 666cb4c22bb9..f098784e7dfd 100644
>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>> @@ -485,6 +485,7 @@ EXPORT_SYMBOL(drm_gem_vram_vunmap);
>>   * @dev:		the DRM device
>>   * @bdev:		the TTM BO device managing the buffer object
>>   * @pg_align:		the buffer's alignment in multiples of the page size
>> + * @pitch_align:	the scanline's alignment in powers of 2
>>   * @interruptible:	sleep interruptible if waiting for memory
>>   * @args:		the arguments as provided to \
>>  				&struct drm_driver.dumb_create
>> @@ -502,6 +503,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>>  				  struct drm_device *dev,
>>  				  struct ttm_bo_device *bdev,
>>  				  unsigned long pg_align,
>> +				  unsigned long pitch_align,
>>  				  bool interruptible,
>>  				  struct drm_mode_create_dumb *args)
>>  {
>> @@ -510,7 +512,9 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>>  	int ret;
>>  	u32 handle;
>>  
>> -	pitch = args->width * ((args->bpp + 7) / 8);
>> +	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> Semi related change...
> 
>> +	if (pitch_align)
>> +		pitch = ALIGN(pitch, pitch_align);
>>  	size = pitch * args->height;
>>  
>>  	size = roundup(size, PAGE_SIZE);
>> @@ -612,8 +616,8 @@ int drm_gem_vram_driver_dumb_create(struct drm_file *file,
>>  	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
>>  		return -EINVAL;
>>  
>> -	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev, 0,
>> -					     false, args);
>> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
>> +					     0, 0, false, args);
>>  }
>>  EXPORT_SYMBOL(drm_gem_vram_driver_dumb_create);
>>  
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index 8eb7258b236a..50a0c1f9d211 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -18,7 +18,6 @@
>>  #include <drm/drm_framebuffer.h>
>>  
>>  struct drm_device;
>> -struct drm_gem_object;
>>  
>>  struct hibmc_drm_private {
>>  	/* hw */
>> @@ -41,9 +40,6 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
>>  int hibmc_de_init(struct hibmc_drm_private *priv);
>>  int hibmc_vdac_init(struct hibmc_drm_private *priv);
>>  
>> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>> -		     struct drm_gem_object **obj);
>> -
>>  int hibmc_mm_init(struct hibmc_drm_private *hibmc);
>>  void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
>>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> index f6d25b85c209..0af5d966a480 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> @@ -47,55 +47,11 @@ void hibmc_mm_fini(struct hibmc_drm_private *hibmc)
>>  	drm_vram_helper_release_mm(hibmc->dev);
>>  }
>>  
>> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>> -		     struct drm_gem_object **obj)
>> -{
>> -	struct drm_gem_vram_object *gbo;
>> -	int ret;
>> -
>> -	*obj = NULL;
>> -
>> -	size = roundup(size, PAGE_SIZE);
>> -	if (size == 0)
>> -		return -EINVAL;
>> -
>> -	gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, size, 0, false);
>> -	if (IS_ERR(gbo)) {
>> -		ret = PTR_ERR(gbo);
>> -		if (ret != -ERESTARTSYS)
>> -			DRM_ERROR("failed to allocate GEM object: %d\n", ret);
>> -		return ret;
>> -	}
>> -	*obj = &gbo->bo.base;
>> -	return 0;
>> -}
>> -
>>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>>  		      struct drm_mode_create_dumb *args)
>>  {
>> -	struct drm_gem_object *gobj;
>> -	u32 handle;
>> -	int ret;
>> -
>> -	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 16);
>> -	args->size = args->pitch * args->height;
>> -
>> -	ret = hibmc_gem_create(dev, args->size, false,
>> -			       &gobj);
>> -	if (ret) {
>> -		DRM_ERROR("failed to create GEM object: %d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	ret = drm_gem_handle_create(file, gobj, &handle);
>> -	drm_gem_object_put_unlocked(gobj);
>> -	if (ret) {
>> -		DRM_ERROR("failed to unreference GEM object: %d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	args->handle = handle;
>> -	return 0;
>> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
>> +					     0, 16, false, args);
>>  }
>>  
>>  const struct drm_mode_config_funcs hibmc_mode_funcs = {
>> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
>> index e040541a105f..c642b4cb6600 100644
>> --- a/include/drm/drm_gem_vram_helper.h
>> +++ b/include/drm/drm_gem_vram_helper.h
>> @@ -113,6 +113,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>>  				  struct drm_device *dev,
>>  				  struct ttm_bo_device *bdev,
>>  				  unsigned long pg_align,
>> +				  unsigned long pitch_align,
>>  				  bool interruptible,
>>  				  struct drm_mode_create_dumb *args);
>>  
>> -- 
>> 2.23.0
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

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


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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/hisilicon/hibmc: Implement hibmc_dumb_create() with generic helpers

[Daniel Vetter]

On Tue, Nov 26, 2019 at 08:40:27AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 25.11.19 um 10:14 schrieb Daniel Vetter:
> > On Fri, Nov 22, 2019 at 09:30:43AM +0100, Thomas Zimmermann wrote:
> >> The hibmc driver aligns scanlines to 16 bytes. Adding the pitch alignment
> >> as an argument to drm_gem_vram_fill_create_dumb() allows to use the generic
> >> implementation with hibmc. A value of 0 disables scanline pitches.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > I concur with Sam, the vram change should be split out.
> > 
> >> ---
> >>  drivers/gpu/drm/drm_gem_vram_helper.c         | 10 ++--
> >>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  4 --
> >>  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 48 +------------------
> >>  include/drm/drm_gem_vram_helper.h             |  1 +
> >>  4 files changed, 10 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> >> index 666cb4c22bb9..f098784e7dfd 100644
> >> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> >> @@ -485,6 +485,7 @@ EXPORT_SYMBOL(drm_gem_vram_vunmap);
> >>   * @dev:		the DRM device
> >>   * @bdev:		the TTM BO device managing the buffer object
> >>   * @pg_align:		the buffer's alignment in multiples of the page size
> >> + * @pitch_align:	the scanline's alignment in powers of 2
> >>   * @interruptible:	sleep interruptible if waiting for memory
> > 
> > I also noticed that no one sets this to true, neither here nor in
> > drm_gem_vram_create(). Maybe remove that too? Otherwise the argument list
> > becomes very unwielding. And you're already touching the (few) callers.
> 
> OK, I'll add this as a separate patch.

Yeah makes sense.

> BTW What's the DRM interface's behavior wrt interruption? For example,
> can a ioctl call like CREATE_DUMB return EINTR to userspace?

Yup. Everyone is required to use drmIoctl() for all drm ioctls, which
auto-restarts all syscalls when userspace sees a EINTR. We also generally
test that in igt (but maybe not for all the kms ioctls, at least not for
the dumb ones).

interruptible + igts using igt_while_interruptible is a fairly effective
way to exercise error paths in all kinds of places. Only trouble is that
if we introduce a new interruptible point somewhere we might run into
userspace that gets it wrong (e.g. dumb ioctls I think aren't
interruptible on most x86 drivers right now, so there might be a surprise
and we need to audit userspaces, including plymouth and all those).
-Daniel

> 
> Best regards
> Thomas
> 
> > 
> >>   * @args:		the arguments as provided to \
> >>  				&struct drm_driver.dumb_create
> >> @@ -502,6 +503,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> >>  				  struct drm_device *dev,
> >>  				  struct ttm_bo_device *bdev,
> >>  				  unsigned long pg_align,
> >> +				  unsigned long pitch_align,
> >>  				  bool interruptible,
> >>  				  struct drm_mode_create_dumb *args)
> >>  {
> >> @@ -510,7 +512,9 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> >>  	int ret;
> >>  	u32 handle;
> >>  
> >> -	pitch = args->width * ((args->bpp + 7) / 8);
> >> +	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> >> +	if (pitch_align)
> >> +		pitch = ALIGN(pitch, pitch_align);
> > 
> > Maybe throw a WARN_IS(is_pot(align)) in here?
> > 
> > Cheers, Daniel
> > 
> >>  	size = pitch * args->height;
> >>  
> >>  	size = roundup(size, PAGE_SIZE);
> >> @@ -612,8 +616,8 @@ int drm_gem_vram_driver_dumb_create(struct drm_file *file,
> >>  	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
> >>  		return -EINVAL;
> >>  
> >> -	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev, 0,
> >> -					     false, args);
> >> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
> >> +					     0, 0, false, args);
> >>  }
> >>  EXPORT_SYMBOL(drm_gem_vram_driver_dumb_create);
> >>  
> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> >> index 8eb7258b236a..50a0c1f9d211 100644
> >> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> >> @@ -18,7 +18,6 @@
> >>  #include <drm/drm_framebuffer.h>
> >>  
> >>  struct drm_device;
> >> -struct drm_gem_object;
> >>  
> >>  struct hibmc_drm_private {
> >>  	/* hw */
> >> @@ -41,9 +40,6 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
> >>  int hibmc_de_init(struct hibmc_drm_private *priv);
> >>  int hibmc_vdac_init(struct hibmc_drm_private *priv);
> >>  
> >> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
> >> -		     struct drm_gem_object **obj);
> >> -
> >>  int hibmc_mm_init(struct hibmc_drm_private *hibmc);
> >>  void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
> >>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> >> index f6d25b85c209..0af5d966a480 100644
> >> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> >> @@ -47,55 +47,11 @@ void hibmc_mm_fini(struct hibmc_drm_private *hibmc)
> >>  	drm_vram_helper_release_mm(hibmc->dev);
> >>  }
> >>  
> >> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
> >> -		     struct drm_gem_object **obj)
> >> -{
> >> -	struct drm_gem_vram_object *gbo;
> >> -	int ret;
> >> -
> >> -	*obj = NULL;
> >> -
> >> -	size = roundup(size, PAGE_SIZE);
> >> -	if (size == 0)
> >> -		return -EINVAL;
> >> -
> >> -	gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, size, 0, false);
> >> -	if (IS_ERR(gbo)) {
> >> -		ret = PTR_ERR(gbo);
> >> -		if (ret != -ERESTARTSYS)
> >> -			DRM_ERROR("failed to allocate GEM object: %d\n", ret);
> >> -		return ret;
> >> -	}
> >> -	*obj = &gbo->bo.base;
> >> -	return 0;
> >> -}
> >> -
> >>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
> >>  		      struct drm_mode_create_dumb *args)
> >>  {
> >> -	struct drm_gem_object *gobj;
> >> -	u32 handle;
> >> -	int ret;
> >> -
> >> -	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 16);
> >> -	args->size = args->pitch * args->height;
> >> -
> >> -	ret = hibmc_gem_create(dev, args->size, false,
> >> -			       &gobj);
> >> -	if (ret) {
> >> -		DRM_ERROR("failed to create GEM object: %d\n", ret);
> >> -		return ret;
> >> -	}
> >> -
> >> -	ret = drm_gem_handle_create(file, gobj, &handle);
> >> -	drm_gem_object_put_unlocked(gobj);
> >> -	if (ret) {
> >> -		DRM_ERROR("failed to unreference GEM object: %d\n", ret);
> >> -		return ret;
> >> -	}
> >> -
> >> -	args->handle = handle;
> >> -	return 0;
> >> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
> >> +					     0, 16, false, args);
> >>  }
> >>  
> >>  const struct drm_mode_config_funcs hibmc_mode_funcs = {
> >> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> >> index e040541a105f..c642b4cb6600 100644
> >> --- a/include/drm/drm_gem_vram_helper.h
> >> +++ b/include/drm/drm_gem_vram_helper.h
> >> @@ -113,6 +113,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> >>  				  struct drm_device *dev,
> >>  				  struct ttm_bo_device *bdev,
> >>  				  unsigned long pg_align,
> >> +				  unsigned long pitch_align,
> >>  				  bool interruptible,
> >>  				  struct drm_mode_create_dumb *args);
> >>  
> >> -- 
> >> 2.23.0
> >>
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel