[PATCH 1/3] drm/rockchip: Use drm_gem_fb_create_with_dirty

[PATCH 1/3] drm/rockchip: Use drm_gem_fb_create_with_dirty

[Daniel Vetter]

If rockchip would switch over to the generic fbdev setup we could
grabage collect even more of all this code (all of the remaining fb
handling code really).

v2: Actually use _with_dirty like the patch subject promised (Andrzej)

Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sandy Huang <hjc@rock-chips.com>
Cc: "Heiko Stübner" <heiko@sntech.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-rockchip@lists.infradead.org
---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
 1 file changed, 1 insertion(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index ca01234c037c..221e72e71432 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
 	return fb;
 }
 
-static struct drm_framebuffer *
-rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
-			const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	const struct drm_format_info *info = drm_get_format_info(dev,
-								 mode_cmd);
-	struct drm_framebuffer *fb;
-	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
-	struct drm_gem_object *obj;
-	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
-	int ret;
-	int i;
-
-	for (i = 0; i < num_planes; i++) {
-		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
-		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
-		unsigned int min_size;
-
-		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
-		if (!obj) {
-			DRM_DEV_ERROR(dev->dev,
-				      "Failed to lookup GEM object\n");
-			ret = -ENXIO;
-			goto err_gem_object_unreference;
-		}
-
-		min_size = (height - 1) * mode_cmd->pitches[i] +
-			mode_cmd->offsets[i] +
-			width * info->cpp[i];
-
-		if (obj->size < min_size) {
-			drm_gem_object_put_unlocked(obj);
-			ret = -EINVAL;
-			goto err_gem_object_unreference;
-		}
-		objs[i] = obj;
-	}
-
-	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
-	if (IS_ERR(fb)) {
-		ret = PTR_ERR(fb);
-		goto err_gem_object_unreference;
-	}
-
-	return fb;
-
-err_gem_object_unreference:
-	for (i--; i >= 0; i--)
-		drm_gem_object_put_unlocked(objs[i]);
-	return ERR_PTR(ret);
-}
-
 static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
 };
 
 static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
-	.fb_create = rockchip_user_fb_create,
+	.fb_create = drm_gem_fb_create_with_dirty,
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
-- 
2.24.0

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

[PATCH 1/3] drm/rockchip: Use drm_gem_fb_create_with_dirty

[Daniel Vetter]

If rockchip would switch over to the generic fbdev setup we could
grabage collect even more of all this code (all of the remaining fb
handling code really).

v2: Actually use _with_dirty like the patch subject promised (Andrzej)

Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sandy Huang <hjc@rock-chips.com>
Cc: "Heiko Stübner" <heiko@sntech.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-rockchip@lists.infradead.org
---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
 1 file changed, 1 insertion(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index ca01234c037c..221e72e71432 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
 	return fb;
 }
 
-static struct drm_framebuffer *
-rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
-			const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	const struct drm_format_info *info = drm_get_format_info(dev,
-								 mode_cmd);
-	struct drm_framebuffer *fb;
-	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
-	struct drm_gem_object *obj;
-	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
-	int ret;
-	int i;
-
-	for (i = 0; i < num_planes; i++) {
-		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
-		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
-		unsigned int min_size;
-
-		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
-		if (!obj) {
-			DRM_DEV_ERROR(dev->dev,
-				      "Failed to lookup GEM object\n");
-			ret = -ENXIO;
-			goto err_gem_object_unreference;
-		}
-
-		min_size = (height - 1) * mode_cmd->pitches[i] +
-			mode_cmd->offsets[i] +
-			width * info->cpp[i];
-
-		if (obj->size < min_size) {
-			drm_gem_object_put_unlocked(obj);
-			ret = -EINVAL;
-			goto err_gem_object_unreference;
-		}
-		objs[i] = obj;
-	}
-
-	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
-	if (IS_ERR(fb)) {
-		ret = PTR_ERR(fb);
-		goto err_gem_object_unreference;
-	}
-
-	return fb;
-
-err_gem_object_unreference:
-	for (i--; i >= 0; i--)
-		drm_gem_object_put_unlocked(objs[i]);
-	return ERR_PTR(ret);
-}
-
 static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
 };
 
 static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
-	.fb_create = rockchip_user_fb_create,
+	.fb_create = drm_gem_fb_create_with_dirty,
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] drm/hibmc: Use drm_gem_fb_create

[Daniel Vetter]

Again we could delete a lot more if we'd switch over to the generic
fbdev stuff.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    |  4 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   | 11 +---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c |  5 +-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 62 +++++--------------
 4 files changed, 19 insertions(+), 63 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..2d0920ec4554 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(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 e58ecd7edcf8..ab5b4a4a2095 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -20,14 +20,9 @@
 struct drm_device;
 struct drm_gem_object;
 
-struct hibmc_framebuffer {
-	struct drm_framebuffer fb;
-	struct drm_gem_object *obj;
-};
-
 struct hibmc_fbdev {
 	struct drm_fb_helper helper; /* must be first */
-	struct hibmc_framebuffer *fb;
+	struct drm_framebuffer *fb;
 	int size;
 };
 
@@ -47,8 +42,6 @@ struct hibmc_drm_private {
 	struct hibmc_fbdev *fbdev;
 };
 
-#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,
@@ -61,7 +54,7 @@ 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);
-struct hibmc_framebuffer *
+struct drm_framebuffer *
 hibmc_framebuffer_init(struct drm_device *dev,
 		       const struct drm_mode_fb_cmd2 *mode_cmd,
 		       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
index b4c1cea051e8..446aeedc9e29 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
@@ -141,15 +141,14 @@ static int hibmc_drm_fb_create(struct drm_fb_helper *helper,
 
 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);
+	if (fbdev->fb)
+		drm_framebuffer_put(fbdev->fb);
 }
 
 static const struct drm_fb_helper_funcs hibmc_fbdev_helper_funcs = {
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
index 21b684eab5c9..386033b0d3a2 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
@@ -16,6 +16,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_print.h>
 
 #include "hibmc_drm_drv.h"
@@ -97,74 +98,39 @@ 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,
+	.destroy	= drm_gem_fb_destroy,
+	.create_handle	= drm_gem_fb_create_handle,
 };
 
-struct hibmc_framebuffer *
+struct drm_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;
+	struct drm_framebuffer *fb;
 	int ret;
 
-	hibmc_fb = kzalloc(sizeof(*hibmc_fb), GFP_KERNEL);
-	if (!hibmc_fb) {
-		DRM_ERROR("failed to allocate hibmc_fb\n");
+	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
+	if (!fb) {
+		DRM_ERROR("failed to allocate 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);
+	drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
+	fb->obj[0] = obj;
+	ret = drm_framebuffer_init(dev, fb, &hibmc_fb_funcs);
 	if (ret) {
 		DRM_ERROR("drm_framebuffer_init failed: %d\n", ret);
-		kfree(hibmc_fb);
+		kfree(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;
+	return 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.24.0

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

[PATCH 3/3] drm/todo: Add entry for fb funcs related cleanups

[Daniel Vetter]

We're doing a great job for really simple drivers right now, but still
a lot of boilerplate for the bigger ones.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/gpu/todo.rst | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 3ec509381fc5..2d85f37284a1 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -182,6 +182,32 @@ Contact: Maintainer of the driver you plan to convert
 
 Level: Intermediate
 
+drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
+-----------------------------------------------------------------
+
+A lot more drivers could be switched over to the drm_gem_framebuffer helpers.
+Various hold-ups:
+
+- Need to switch over to the generic dirty tracking code using
+  drm_atomic_helper_dirtyfb first (e.g. qxl).
+
+- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
+  setup code can't be deleted.
+
+- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
+  atomic drivers we could check for valid formats by calling
+  drm_plane_check_pixel_format() against all planes, and pass if any plane
+  supports the format. For non-atomic that's not possible since like the format
+  list for the primary plane is fake and we'd therefor reject valid formats.
+
+- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
+  version of the varios drm_gem_fb_create functions. Maybe called
+  drm_gem_fb_create/_with_dirty/_with_funcs as needed.
+
+Contact: Daniel Vetter
+
+Level: Intermediate
+
 Clean up mmap forwarding
 ------------------------
 
-- 
2.24.0

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

Re: [PATCH 2/3] drm/hibmc: Use drm_gem_fb_create

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]

Hi Daniel,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v5.4 next-20191127]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-rockchip-Use-drm_gem_fb_create_with_dirty/20191128-023917
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: arm64-randconfig-a001-20191128 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c: In function 'hibmc_plane_atomic_update':
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c:107:28: error: 'fb' undeclared (first use in this function); did you mean 'mb'?
     gbo = drm_gem_vram_of_gem(fb->obj[0]);
                               ^~
                               mb
   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c:107:28: note: each undeclared identifier is reported only once for each function it appears in
--
   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c: In function 'hibmc_drm_fb_create':
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c:119:37: error: 'struct drm_framebuffer' has no member named 'fb'
     hi_fbdev->helper.fb = &hi_fbdev->fb->fb;
                                        ^~

vim +107 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c

    93	
    94	static void hibmc_plane_atomic_update(struct drm_plane *plane,
    95					      struct drm_plane_state *old_state)
    96	{
    97		struct drm_plane_state	*state	= plane->state;
    98		u32 reg;
    99		s64 gpu_addr = 0;
   100		unsigned int line_l;
   101		struct hibmc_drm_private *priv = plane->dev->dev_private;
   102		struct drm_gem_vram_object *gbo;
   103	
   104		if (!state->fb)
   105			return;
   106	
 > 107		gbo = drm_gem_vram_of_gem(fb->obj[0]);
   108	
   109		gpu_addr = drm_gem_vram_offset(gbo);
   110		if (WARN_ON_ONCE(gpu_addr < 0))
   111			return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
   112	
   113		writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
   114	
   115		reg = state->fb->width * (state->fb->format->cpp[0]);
   116		/* now line_pad is 16 */
   117		reg = PADDING(16, reg);
   118	
   119		line_l = state->fb->width * state->fb->format->cpp[0];
   120		line_l = PADDING(16, line_l);
   121		writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
   122		       HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
   123		       priv->mmio + HIBMC_CRT_FB_WIDTH);
   124	
   125		/* SET PIXEL FORMAT */
   126		reg = readl(priv->mmio + HIBMC_CRT_DISP_CTL);
   127		reg &= ~HIBMC_CRT_DISP_CTL_FORMAT_MASK;
   128		reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT,
   129				   state->fb->format->cpp[0] * 8 / 16);
   130		writel(reg, priv->mmio + HIBMC_CRT_DISP_CTL);
   131	}
   132	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32916 bytes --]

[-- Attachment #3: 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 2/3] drm/hibmc: Use drm_gem_fb_create

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]

Hi Daniel,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v5.4 next-20191127]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-rockchip-Use-drm_gem_fb_create_with_dirty/20191128-023917
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: arm64-randconfig-a001-20191128 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c: In function 'hibmc_plane_atomic_update':
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c:107:28: error: 'fb' undeclared (first use in this function); did you mean 'mb'?
     gbo = drm_gem_vram_of_gem(fb->obj[0]);
                               ^~
                               mb
   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c:107:28: note: each undeclared identifier is reported only once for each function it appears in
--
   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c: In function 'hibmc_drm_fb_create':
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c:119:37: error: 'struct drm_framebuffer' has no member named 'fb'
     hi_fbdev->helper.fb = &hi_fbdev->fb->fb;
                                        ^~

vim +107 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c

    93	
    94	static void hibmc_plane_atomic_update(struct drm_plane *plane,
    95					      struct drm_plane_state *old_state)
    96	{
    97		struct drm_plane_state	*state	= plane->state;
    98		u32 reg;
    99		s64 gpu_addr = 0;
   100		unsigned int line_l;
   101		struct hibmc_drm_private *priv = plane->dev->dev_private;
   102		struct drm_gem_vram_object *gbo;
   103	
   104		if (!state->fb)
   105			return;
   106	
 > 107		gbo = drm_gem_vram_of_gem(fb->obj[0]);
   108	
   109		gpu_addr = drm_gem_vram_offset(gbo);
   110		if (WARN_ON_ONCE(gpu_addr < 0))
   111			return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
   112	
   113		writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
   114	
   115		reg = state->fb->width * (state->fb->format->cpp[0]);
   116		/* now line_pad is 16 */
   117		reg = PADDING(16, reg);
   118	
   119		line_l = state->fb->width * state->fb->format->cpp[0];
   120		line_l = PADDING(16, line_l);
   121		writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
   122		       HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
   123		       priv->mmio + HIBMC_CRT_FB_WIDTH);
   124	
   125		/* SET PIXEL FORMAT */
   126		reg = readl(priv->mmio + HIBMC_CRT_DISP_CTL);
   127		reg &= ~HIBMC_CRT_DISP_CTL_FORMAT_MASK;
   128		reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT,
   129				   state->fb->format->cpp[0] * 8 / 16);
   130		writel(reg, priv->mmio + HIBMC_CRT_DISP_CTL);
   131	}
   132	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32916 bytes --]

[-- Attachment #3: 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 2/3] drm/hibmc: Use drm_gem_fb_create

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3599 bytes --]

Hi Daniel,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v5.4 next-20191127]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-rockchip-Use-drm_gem_fb_create_with_dirty/20191128-023917
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: arm64-randconfig-a001-20191128 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c: In function 'hibmc_plane_atomic_update':
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c:107:28: error: 'fb' undeclared (first use in this function); did you mean 'mb'?
     gbo = drm_gem_vram_of_gem(fb->obj[0]);
                               ^~
                               mb
   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c:107:28: note: each undeclared identifier is reported only once for each function it appears in
--
   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c: In function 'hibmc_drm_fb_create':
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c:119:37: error: 'struct drm_framebuffer' has no member named 'fb'
     hi_fbdev->helper.fb = &hi_fbdev->fb->fb;
                                        ^~

vim +107 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c

    93	
    94	static void hibmc_plane_atomic_update(struct drm_plane *plane,
    95					      struct drm_plane_state *old_state)
    96	{
    97		struct drm_plane_state	*state	= plane->state;
    98		u32 reg;
    99		s64 gpu_addr = 0;
   100		unsigned int line_l;
   101		struct hibmc_drm_private *priv = plane->dev->dev_private;
   102		struct drm_gem_vram_object *gbo;
   103	
   104		if (!state->fb)
   105			return;
   106	
 > 107		gbo = drm_gem_vram_of_gem(fb->obj[0]);
   108	
   109		gpu_addr = drm_gem_vram_offset(gbo);
   110		if (WARN_ON_ONCE(gpu_addr < 0))
   111			return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
   112	
   113		writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
   114	
   115		reg = state->fb->width * (state->fb->format->cpp[0]);
   116		/* now line_pad is 16 */
   117		reg = PADDING(16, reg);
   118	
   119		line_l = state->fb->width * state->fb->format->cpp[0];
   120		line_l = PADDING(16, line_l);
   121		writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
   122		       HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
   123		       priv->mmio + HIBMC_CRT_FB_WIDTH);
   124	
   125		/* SET PIXEL FORMAT */
   126		reg = readl(priv->mmio + HIBMC_CRT_DISP_CTL);
   127		reg &= ~HIBMC_CRT_DISP_CTL_FORMAT_MASK;
   128		reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT,
   129				   state->fb->format->cpp[0] * 8 / 16);
   130		writel(reg, priv->mmio + HIBMC_CRT_DISP_CTL);
   131	}
   132	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32916 bytes --]

Re: [PATCH 2/3] drm/hibmc: Use drm_gem_fb_create

[Daniel Vetter]

On Thu, Nov 28, 2019 at 04:44:32PM +0800, kbuild test robot wrote:
> Hi Daniel,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on drm-intel/for-linux-next]
> [also build test ERROR on v5.4 next-20191127]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> 
> url:    https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-rockchip-Use-drm_gem_fb_create_with_dirty/20191128-023917
> base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> config: arm64-randconfig-a001-20191128 (attached as .config)
> compiler: aarch64-linux-gcc (GCC) 7.4.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.4.0 make.cross ARCH=arm64 
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):


Oops, I meant to drop this patch from this series, but forgot. It's
superseeded by the series Thomas has (which actually compiles).
-Daniel

> 
>    drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c: In function 'hibmc_plane_atomic_update':
> >> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c:107:28: error: 'fb' undeclared (first use in this function); did you mean 'mb'?
>      gbo = drm_gem_vram_of_gem(fb->obj[0]);
>                                ^~
>                                mb
>    drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c:107:28: note: each undeclared identifier is reported only once for each function it appears in
> --
>    drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c: In function 'hibmc_drm_fb_create':
> >> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c:119:37: error: 'struct drm_framebuffer' has no member named 'fb'
>      hi_fbdev->helper.fb = &hi_fbdev->fb->fb;
>                                         ^~
> 
> vim +107 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> 
>     93	
>     94	static void hibmc_plane_atomic_update(struct drm_plane *plane,
>     95					      struct drm_plane_state *old_state)
>     96	{
>     97		struct drm_plane_state	*state	= plane->state;
>     98		u32 reg;
>     99		s64 gpu_addr = 0;
>    100		unsigned int line_l;
>    101		struct hibmc_drm_private *priv = plane->dev->dev_private;
>    102		struct drm_gem_vram_object *gbo;
>    103	
>    104		if (!state->fb)
>    105			return;
>    106	
>  > 107		gbo = drm_gem_vram_of_gem(fb->obj[0]);
>    108	
>    109		gpu_addr = drm_gem_vram_offset(gbo);
>    110		if (WARN_ON_ONCE(gpu_addr < 0))
>    111			return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
>    112	
>    113		writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
>    114	
>    115		reg = state->fb->width * (state->fb->format->cpp[0]);
>    116		/* now line_pad is 16 */
>    117		reg = PADDING(16, reg);
>    118	
>    119		line_l = state->fb->width * state->fb->format->cpp[0];
>    120		line_l = PADDING(16, line_l);
>    121		writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
>    122		       HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
>    123		       priv->mmio + HIBMC_CRT_FB_WIDTH);
>    124	
>    125		/* SET PIXEL FORMAT */
>    126		reg = readl(priv->mmio + HIBMC_CRT_DISP_CTL);
>    127		reg &= ~HIBMC_CRT_DISP_CTL_FORMAT_MASK;
>    128		reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT,
>    129				   state->fb->format->cpp[0] * 8 / 16);
>    130		writel(reg, priv->mmio + HIBMC_CRT_DISP_CTL);
>    131	}
>    132	
> 
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation



-- 
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/3] drm/hibmc: Use drm_gem_fb_create

[Daniel Vetter]

[-- Attachment #1: Type: text/plain, Size: 4066 bytes --]

On Thu, Nov 28, 2019 at 04:44:32PM +0800, kbuild test robot wrote:
> Hi Daniel,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on drm-intel/for-linux-next]
> [also build test ERROR on v5.4 next-20191127]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> 
> url:    https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-rockchip-Use-drm_gem_fb_create_with_dirty/20191128-023917
> base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> config: arm64-randconfig-a001-20191128 (attached as .config)
> compiler: aarch64-linux-gcc (GCC) 7.4.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.4.0 make.cross ARCH=arm64 
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):


Oops, I meant to drop this patch from this series, but forgot. It's
superseeded by the series Thomas has (which actually compiles).
-Daniel

> 
>    drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c: In function 'hibmc_plane_atomic_update':
> >> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c:107:28: error: 'fb' undeclared (first use in this function); did you mean 'mb'?
>      gbo = drm_gem_vram_of_gem(fb->obj[0]);
>                                ^~
>                                mb
>    drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c:107:28: note: each undeclared identifier is reported only once for each function it appears in
> --
>    drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c: In function 'hibmc_drm_fb_create':
> >> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c:119:37: error: 'struct drm_framebuffer' has no member named 'fb'
>      hi_fbdev->helper.fb = &hi_fbdev->fb->fb;
>                                         ^~
> 
> vim +107 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> 
>     93	
>     94	static void hibmc_plane_atomic_update(struct drm_plane *plane,
>     95					      struct drm_plane_state *old_state)
>     96	{
>     97		struct drm_plane_state	*state	= plane->state;
>     98		u32 reg;
>     99		s64 gpu_addr = 0;
>    100		unsigned int line_l;
>    101		struct hibmc_drm_private *priv = plane->dev->dev_private;
>    102		struct drm_gem_vram_object *gbo;
>    103	
>    104		if (!state->fb)
>    105			return;
>    106	
>  > 107		gbo = drm_gem_vram_of_gem(fb->obj[0]);
>    108	
>    109		gpu_addr = drm_gem_vram_offset(gbo);
>    110		if (WARN_ON_ONCE(gpu_addr < 0))
>    111			return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
>    112	
>    113		writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
>    114	
>    115		reg = state->fb->width * (state->fb->format->cpp[0]);
>    116		/* now line_pad is 16 */
>    117		reg = PADDING(16, reg);
>    118	
>    119		line_l = state->fb->width * state->fb->format->cpp[0];
>    120		line_l = PADDING(16, line_l);
>    121		writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
>    122		       HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
>    123		       priv->mmio + HIBMC_CRT_FB_WIDTH);
>    124	
>    125		/* SET PIXEL FORMAT */
>    126		reg = readl(priv->mmio + HIBMC_CRT_DISP_CTL);
>    127		reg &= ~HIBMC_CRT_DISP_CTL_FORMAT_MASK;
>    128		reg |= HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT,
>    129				   state->fb->format->cpp[0] * 8 / 16);
>    130		writel(reg, priv->mmio + HIBMC_CRT_DISP_CTL);
>    131	}
>    132	
> 
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/rockchip: Use drm_gem_fb_create_with_dirty

[Andrzej Pietrasiewicz]

W dniu 27.11.2019 o 19:00, Daniel Vetter pisze:
> If rockchip would switch over to the generic fbdev setup we could
> grabage collect even more of all this code (all of the remaining fb
> handling code really).
> 
> v2: Actually use _with_dirty like the patch subject promised (Andrzej)
> 
> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-rockchip@lists.infradead.org

I understand that computing min_size is changing as per

042bf753842dd
drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info.

With other questions I had before answered in your previous reply the current
version of this patch is

Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
>   1 file changed, 1 insertion(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index ca01234c037c..221e72e71432 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
>   	return fb;
>   }
>   
> -static struct drm_framebuffer *
> -rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> -			const struct drm_mode_fb_cmd2 *mode_cmd)
> -{
> -	const struct drm_format_info *info = drm_get_format_info(dev,
> -								 mode_cmd);
> -	struct drm_framebuffer *fb;
> -	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
> -	struct drm_gem_object *obj;
> -	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
> -	int ret;
> -	int i;
> -
> -	for (i = 0; i < num_planes; i++) {
> -		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> -		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> -		unsigned int min_size;
> -
> -		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
> -		if (!obj) {
> -			DRM_DEV_ERROR(dev->dev,
> -				      "Failed to lookup GEM object\n");
> -			ret = -ENXIO;
> -			goto err_gem_object_unreference;
> -		}
> -
> -		min_size = (height - 1) * mode_cmd->pitches[i] +
> -			mode_cmd->offsets[i] +
> -			width * info->cpp[i];
> -
> -		if (obj->size < min_size) {
> -			drm_gem_object_put_unlocked(obj);
> -			ret = -EINVAL;
> -			goto err_gem_object_unreference;
> -		}
> -		objs[i] = obj;
> -	}
> -
> -	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
> -	if (IS_ERR(fb)) {
> -		ret = PTR_ERR(fb);
> -		goto err_gem_object_unreference;
> -	}
> -
> -	return fb;
> -
> -err_gem_object_unreference:
> -	for (i--; i >= 0; i--)
> -		drm_gem_object_put_unlocked(objs[i]);
> -	return ERR_PTR(ret);
> -}
> -
>   static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
>   	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>   };
>   
>   static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> -	.fb_create = rockchip_user_fb_create,
> +	.fb_create = drm_gem_fb_create_with_dirty,
>   	.output_poll_changed = drm_fb_helper_output_poll_changed,
>   	.atomic_check = drm_atomic_helper_check,
>   	.atomic_commit = drm_atomic_helper_commit,
> 

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

Re: [PATCH 1/3] drm/rockchip: Use drm_gem_fb_create_with_dirty

[Andrzej Pietrasiewicz]

W dniu 27.11.2019 o 19:00, Daniel Vetter pisze:
> If rockchip would switch over to the generic fbdev setup we could
> grabage collect even more of all this code (all of the remaining fb
> handling code really).
> 
> v2: Actually use _with_dirty like the patch subject promised (Andrzej)
> 
> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-rockchip@lists.infradead.org

I understand that computing min_size is changing as per

042bf753842dd
drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info.

With other questions I had before answered in your previous reply the current
version of this patch is

Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
>   1 file changed, 1 insertion(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index ca01234c037c..221e72e71432 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
>   	return fb;
>   }
>   
> -static struct drm_framebuffer *
> -rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> -			const struct drm_mode_fb_cmd2 *mode_cmd)
> -{
> -	const struct drm_format_info *info = drm_get_format_info(dev,
> -								 mode_cmd);
> -	struct drm_framebuffer *fb;
> -	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
> -	struct drm_gem_object *obj;
> -	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
> -	int ret;
> -	int i;
> -
> -	for (i = 0; i < num_planes; i++) {
> -		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> -		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> -		unsigned int min_size;
> -
> -		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
> -		if (!obj) {
> -			DRM_DEV_ERROR(dev->dev,
> -				      "Failed to lookup GEM object\n");
> -			ret = -ENXIO;
> -			goto err_gem_object_unreference;
> -		}
> -
> -		min_size = (height - 1) * mode_cmd->pitches[i] +
> -			mode_cmd->offsets[i] +
> -			width * info->cpp[i];
> -
> -		if (obj->size < min_size) {
> -			drm_gem_object_put_unlocked(obj);
> -			ret = -EINVAL;
> -			goto err_gem_object_unreference;
> -		}
> -		objs[i] = obj;
> -	}
> -
> -	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
> -	if (IS_ERR(fb)) {
> -		ret = PTR_ERR(fb);
> -		goto err_gem_object_unreference;
> -	}
> -
> -	return fb;
> -
> -err_gem_object_unreference:
> -	for (i--; i >= 0; i--)
> -		drm_gem_object_put_unlocked(objs[i]);
> -	return ERR_PTR(ret);
> -}
> -
>   static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
>   	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>   };
>   
>   static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> -	.fb_create = rockchip_user_fb_create,
> +	.fb_create = drm_gem_fb_create_with_dirty,
>   	.output_poll_changed = drm_fb_helper_output_poll_changed,
>   	.atomic_check = drm_atomic_helper_check,
>   	.atomic_commit = drm_atomic_helper_commit,
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] drm/rockchip: Use drm_gem_fb_create_with_dirty

[Daniel Vetter]

On Thu, Nov 28, 2019 at 04:58:26PM +0100, Andrzej Pietrasiewicz wrote:
> W dniu 27.11.2019 o 19:00, Daniel Vetter pisze:
> > If rockchip would switch over to the generic fbdev setup we could
> > grabage collect even more of all this code (all of the remaining fb
> > handling code really).
> > 
> > v2: Actually use _with_dirty like the patch subject promised (Andrzej)
> > 
> > Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Sandy Huang <hjc@rock-chips.com>
> > Cc: "Heiko Stübner" <heiko@sntech.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-rockchip@lists.infradead.org
> 
> I understand that computing min_size is changing as per
> 
> 042bf753842dd
> drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info.

Yeah it's the more flexible computation, but for everything that rockchip
actually supports it should be the same.

> With other questions I had before answered in your previous reply the current
> version of this patch is
> 
> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

Thanks for your review, patch applied.
-Daniel

> 
> > ---
> >   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
> >   1 file changed, 1 insertion(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index ca01234c037c..221e72e71432 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
> >   	return fb;
> >   }
> > -static struct drm_framebuffer *
> > -rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> > -			const struct drm_mode_fb_cmd2 *mode_cmd)
> > -{
> > -	const struct drm_format_info *info = drm_get_format_info(dev,
> > -								 mode_cmd);
> > -	struct drm_framebuffer *fb;
> > -	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
> > -	struct drm_gem_object *obj;
> > -	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
> > -	int ret;
> > -	int i;
> > -
> > -	for (i = 0; i < num_planes; i++) {
> > -		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> > -		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> > -		unsigned int min_size;
> > -
> > -		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
> > -		if (!obj) {
> > -			DRM_DEV_ERROR(dev->dev,
> > -				      "Failed to lookup GEM object\n");
> > -			ret = -ENXIO;
> > -			goto err_gem_object_unreference;
> > -		}
> > -
> > -		min_size = (height - 1) * mode_cmd->pitches[i] +
> > -			mode_cmd->offsets[i] +
> > -			width * info->cpp[i];
> > -
> > -		if (obj->size < min_size) {
> > -			drm_gem_object_put_unlocked(obj);
> > -			ret = -EINVAL;
> > -			goto err_gem_object_unreference;
> > -		}
> > -		objs[i] = obj;
> > -	}
> > -
> > -	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
> > -	if (IS_ERR(fb)) {
> > -		ret = PTR_ERR(fb);
> > -		goto err_gem_object_unreference;
> > -	}
> > -
> > -	return fb;
> > -
> > -err_gem_object_unreference:
> > -	for (i--; i >= 0; i--)
> > -		drm_gem_object_put_unlocked(objs[i]);
> > -	return ERR_PTR(ret);
> > -}
> > -
> >   static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
> >   	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> >   };
> >   static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> > -	.fb_create = rockchip_user_fb_create,
> > +	.fb_create = drm_gem_fb_create_with_dirty,
> >   	.output_poll_changed = drm_fb_helper_output_poll_changed,
> >   	.atomic_check = drm_atomic_helper_check,
> >   	.atomic_commit = drm_atomic_helper_commit,
> > 
> 

-- 
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 1/3] drm/rockchip: Use drm_gem_fb_create_with_dirty

[Daniel Vetter]

On Thu, Nov 28, 2019 at 04:58:26PM +0100, Andrzej Pietrasiewicz wrote:
> W dniu 27.11.2019 o 19:00, Daniel Vetter pisze:
> > If rockchip would switch over to the generic fbdev setup we could
> > grabage collect even more of all this code (all of the remaining fb
> > handling code really).
> > 
> > v2: Actually use _with_dirty like the patch subject promised (Andrzej)
> > 
> > Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Sandy Huang <hjc@rock-chips.com>
> > Cc: "Heiko Stübner" <heiko@sntech.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-rockchip@lists.infradead.org
> 
> I understand that computing min_size is changing as per
> 
> 042bf753842dd
> drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info.

Yeah it's the more flexible computation, but for everything that rockchip
actually supports it should be the same.

> With other questions I had before answered in your previous reply the current
> version of this patch is
> 
> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

Thanks for your review, patch applied.
-Daniel

> 
> > ---
> >   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
> >   1 file changed, 1 insertion(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index ca01234c037c..221e72e71432 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
> >   	return fb;
> >   }
> > -static struct drm_framebuffer *
> > -rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> > -			const struct drm_mode_fb_cmd2 *mode_cmd)
> > -{
> > -	const struct drm_format_info *info = drm_get_format_info(dev,
> > -								 mode_cmd);
> > -	struct drm_framebuffer *fb;
> > -	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
> > -	struct drm_gem_object *obj;
> > -	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
> > -	int ret;
> > -	int i;
> > -
> > -	for (i = 0; i < num_planes; i++) {
> > -		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> > -		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> > -		unsigned int min_size;
> > -
> > -		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
> > -		if (!obj) {
> > -			DRM_DEV_ERROR(dev->dev,
> > -				      "Failed to lookup GEM object\n");
> > -			ret = -ENXIO;
> > -			goto err_gem_object_unreference;
> > -		}
> > -
> > -		min_size = (height - 1) * mode_cmd->pitches[i] +
> > -			mode_cmd->offsets[i] +
> > -			width * info->cpp[i];
> > -
> > -		if (obj->size < min_size) {
> > -			drm_gem_object_put_unlocked(obj);
> > -			ret = -EINVAL;
> > -			goto err_gem_object_unreference;
> > -		}
> > -		objs[i] = obj;
> > -	}
> > -
> > -	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
> > -	if (IS_ERR(fb)) {
> > -		ret = PTR_ERR(fb);
> > -		goto err_gem_object_unreference;
> > -	}
> > -
> > -	return fb;
> > -
> > -err_gem_object_unreference:
> > -	for (i--; i >= 0; i--)
> > -		drm_gem_object_put_unlocked(objs[i]);
> > -	return ERR_PTR(ret);
> > -}
> > -
> >   static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
> >   	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> >   };
> >   static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> > -	.fb_create = rockchip_user_fb_create,
> > +	.fb_create = drm_gem_fb_create_with_dirty,
> >   	.output_poll_changed = drm_fb_helper_output_poll_changed,
> >   	.atomic_check = drm_atomic_helper_check,
> >   	.atomic_commit = drm_atomic_helper_commit,
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] drm/todo: Add entry for fb funcs related cleanups

[Thomas Zimmermann]

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

Hi

Am 27.11.19 um 19:00 schrieb Daniel Vetter:
> We're doing a great job for really simple drivers right now, but still
> a lot of boilerplate for the bigger ones.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Just a small remark below, otherwise

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>


> ---
>  Documentation/gpu/todo.rst | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 3ec509381fc5..2d85f37284a1 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -182,6 +182,32 @@ Contact: Maintainer of the driver you plan to convert
>  
>  Level: Intermediate
>  
> +drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> +-----------------------------------------------------------------
> +
> +A lot more drivers could be switched over to the drm_gem_framebuffer helpers.
> +Various hold-ups:
> +
> +- Need to switch over to the generic dirty tracking code using
> +  drm_atomic_helper_dirtyfb first (e.g. qxl).
> +
> +- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
> +  setup code can't be deleted.

From what I remember, almost all of the obvious, low-hanging fruits have
been picked here. The remaining fbdev users either have HW acceleration
(nouveau, gma500), or use the cfb drawing functions.

The TODO item should probably mention this, with some advise to do some
extra testing for compatibility or performance after moving to generic
fbdev.

Best regards
Thomas

> +
> +- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
> +  atomic drivers we could check for valid formats by calling
> +  drm_plane_check_pixel_format() against all planes, and pass if any plane
> +  supports the format. For non-atomic that's not possible since like the format
> +  list for the primary plane is fake and we'd therefor reject valid formats.
> +
> +- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
> +  version of the varios drm_gem_fb_create functions. Maybe called
> +  drm_gem_fb_create/_with_dirty/_with_funcs as needed.
> +
> +Contact: Daniel Vetter
> +
> +Level: Intermediate
> +
>  Clean up mmap forwarding
>  ------------------------
>  
> 

-- 
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/3] drm/todo: Add entry for fb funcs related cleanups

[Daniel Vetter]

On Fri, Nov 29, 2019 at 10:34:10AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 27.11.19 um 19:00 schrieb Daniel Vetter:
> > We're doing a great job for really simple drivers right now, but still
> > a lot of boilerplate for the bigger ones.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Just a small remark below, otherwise
> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> 
> > ---
> >  Documentation/gpu/todo.rst | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > index 3ec509381fc5..2d85f37284a1 100644
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -182,6 +182,32 @@ Contact: Maintainer of the driver you plan to convert
> >  
> >  Level: Intermediate
> >  
> > +drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> > +-----------------------------------------------------------------
> > +
> > +A lot more drivers could be switched over to the drm_gem_framebuffer helpers.
> > +Various hold-ups:
> > +
> > +- Need to switch over to the generic dirty tracking code using
> > +  drm_atomic_helper_dirtyfb first (e.g. qxl).
> > +
> > +- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
> > +  setup code can't be deleted.
> 
> From what I remember, almost all of the obvious, low-hanging fruits have
> been picked here. The remaining fbdev users either have HW acceleration
> (nouveau, gma500), or use the cfb drawing functions.

I think a bunch of these (from you) aren't merged yet. I'll add a note
about sys vs cfb. About gma500/nouveau, I'm kinda tempted to just ditch
the acceleration ... but maybe someone cares, dunno.

> The TODO item should probably mention this, with some advise to do some
> extra testing for compatibility or performance after moving to generic
> fbdev.

Testing (at least on x86) won't catch the cfb/sysfb stuff, since it's
exactly the same asm instructions :-/ tbh I still don't know where this
actually makes a difference.
-Daniel

> 
> Best regards
> Thomas
> 
> > +
> > +- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
> > +  atomic drivers we could check for valid formats by calling
> > +  drm_plane_check_pixel_format() against all planes, and pass if any plane
> > +  supports the format. For non-atomic that's not possible since like the format
> > +  list for the primary plane is fake and we'd therefor reject valid formats.
> > +
> > +- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
> > +  version of the varios drm_gem_fb_create functions. Maybe called
> > +  drm_gem_fb_create/_with_dirty/_with_funcs as needed.
> > +
> > +Contact: Daniel Vetter
> > +
> > +Level: Intermediate
> > +
> >  Clean up mmap forwarding
> >  ------------------------
> >  
> > 
> 
> -- 
> 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

Re: [PATCH 3/3] drm/todo: Add entry for fb funcs related cleanups

[Daniel Vetter]

On Fri, Nov 29, 2019 at 07:57:39PM +0100, Daniel Vetter wrote:
> On Fri, Nov 29, 2019 at 10:34:10AM +0100, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 27.11.19 um 19:00 schrieb Daniel Vetter:
> > > We're doing a great job for really simple drivers right now, but still
> > > a lot of boilerplate for the bigger ones.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > 
> > Just a small remark below, otherwise
> > 
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > 
> > > ---
> > >  Documentation/gpu/todo.rst | 26 ++++++++++++++++++++++++++
> > >  1 file changed, 26 insertions(+)
> > > 
> > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > > index 3ec509381fc5..2d85f37284a1 100644
> > > --- a/Documentation/gpu/todo.rst
> > > +++ b/Documentation/gpu/todo.rst
> > > @@ -182,6 +182,32 @@ Contact: Maintainer of the driver you plan to convert
> > >  
> > >  Level: Intermediate
> > >  
> > > +drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> > > +-----------------------------------------------------------------
> > > +
> > > +A lot more drivers could be switched over to the drm_gem_framebuffer helpers.
> > > +Various hold-ups:
> > > +
> > > +- Need to switch over to the generic dirty tracking code using
> > > +  drm_atomic_helper_dirtyfb first (e.g. qxl).
> > > +
> > > +- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
> > > +  setup code can't be deleted.
> > 
> > From what I remember, almost all of the obvious, low-hanging fruits have
> > been picked here. The remaining fbdev users either have HW acceleration
> > (nouveau, gma500), or use the cfb drawing functions.
> 
> I think a bunch of these (from you) aren't merged yet. I'll add a note
> about sys vs cfb. About gma500/nouveau, I'm kinda tempted to just ditch
> the acceleration ... but maybe someone cares, dunno.

Correction, we already have a task for drm_fbdev_generic_setup, and that
mentions the sys/cfb issue already. So I'll leave this as-is.
-Daniel

> 
> > The TODO item should probably mention this, with some advise to do some
> > extra testing for compatibility or performance after moving to generic
> > fbdev.
> 
> Testing (at least on x86) won't catch the cfb/sysfb stuff, since it's
> exactly the same asm instructions :-/ tbh I still don't know where this
> actually makes a difference.
> -Daniel
> 
> > 
> > Best regards
> > Thomas
> > 
> > > +
> > > +- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
> > > +  atomic drivers we could check for valid formats by calling
> > > +  drm_plane_check_pixel_format() against all planes, and pass if any plane
> > > +  supports the format. For non-atomic that's not possible since like the format
> > > +  list for the primary plane is fake and we'd therefor reject valid formats.
> > > +
> > > +- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
> > > +  version of the varios drm_gem_fb_create functions. Maybe called
> > > +  drm_gem_fb_create/_with_dirty/_with_funcs as needed.
> > > +
> > > +Contact: Daniel Vetter
> > > +
> > > +Level: Intermediate
> > > +
> > >  Clean up mmap forwarding
> > >  ------------------------
> > >  
> > > 
> > 
> > -- 
> > 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

-- 
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/3] drm/todo: Add entry for fb funcs related cleanups

[Thomas Zimmermann]

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

(cc: SPARC64 maintainer)
(cc: RISC-V maintainers)

Hi Daniel

Am 29.11.19 um 20:05 schrieb Daniel Vetter:
> On Fri, Nov 29, 2019 at 07:57:39PM +0100, Daniel Vetter wrote:
>> On Fri, Nov 29, 2019 at 10:34:10AM +0100, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 27.11.19 um 19:00 schrieb Daniel Vetter:
>>>> We're doing a great job for really simple drivers right now, but still
>>>> a lot of boilerplate for the bigger ones.
>>>>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>
>>> Just a small remark below, otherwise
>>>
>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>
>>>
>>>> ---
>>>>  Documentation/gpu/todo.rst | 26 ++++++++++++++++++++++++++
>>>>  1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
>>>> index 3ec509381fc5..2d85f37284a1 100644
>>>> --- a/Documentation/gpu/todo.rst
>>>> +++ b/Documentation/gpu/todo.rst
>>>> @@ -182,6 +182,32 @@ Contact: Maintainer of the driver you plan to convert
>>>>  
>>>>  Level: Intermediate
>>>>  
>>>> +drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
>>>> +-----------------------------------------------------------------
>>>> +
>>>> +A lot more drivers could be switched over to the drm_gem_framebuffer helpers.
>>>> +Various hold-ups:
>>>> +
>>>> +- Need to switch over to the generic dirty tracking code using
>>>> +  drm_atomic_helper_dirtyfb first (e.g. qxl).
>>>> +
>>>> +- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
>>>> +  setup code can't be deleted.
>>>
>>> From what I remember, almost all of the obvious, low-hanging fruits have
>>> been picked here. The remaining fbdev users either have HW acceleration
>>> (nouveau, gma500), or use the cfb drawing functions.
>>
>> I think a bunch of these (from you) aren't merged yet. I'll add a note
>> about sys vs cfb. About gma500/nouveau, I'm kinda tempted to just ditch
>> the acceleration ... but maybe someone cares, dunno.
> 
> Correction, we already have a task for drm_fbdev_generic_setup, and that
> mentions the sys/cfb issue already. So I'll leave this as-is.

Maybe refer to the related TODO item.

> -Daniel
> 
>>
>>> The TODO item should probably mention this, with some advise to do some
>>> extra testing for compatibility or performance after moving to generic
>>> fbdev.
>>
>> Testing (at least on x86) won't catch the cfb/sysfb stuff, since it's
>> exactly the same asm instructions :-/ tbh I still don't know where this
>> actually makes a difference.

I briefly looked through the code of the CFB helpers. They use the
helpers at [1] for accessing the framebuffer. Those are special for
several architectures. [2]

SPARC64 expands to assembly instructions. [3] The rest appears to have
regular memory instructions in their implementations of __raw_readb().
Although not listed here, I found that RISC V has assembly code in
__raw_readb(). [4]

In the CFB code, there's also bit-shifting code that cares about
endianess. [5] In the end it seems to depends on FBINFO_FOREIGN_ENDIAN,
but the only user I could find was mb862xxfb. [6]

I don't know what the hand-crafted assembly instructions do in detail,
but for the moment we seem to be good without CFB code.

Best regards
Thomas


[1] https://elixir.bootlin.com/linux/v5.4/source/include/linux/fb.h#L564
[2] https://elixir.bootlin.com/linux/v5.4/source/include/linux/fb.h#L527
[3]
https://elixir.bootlin.com/linux/v5.4/source/arch/sparc/include/asm/io_64.h#L20
[4]
https://elixir.bootlin.com/linux/v5.4/source/arch/riscv/include/asm/io.h#L59
[5] https://elixir.bootlin.com/linux/v5.4/source/include/linux/fb.h#L578
[6]
https://elixir.bootlin.com/linux/v5.4/source/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c#L501

>> -Daniel
>>
>>>
>>> Best regards
>>> Thomas
>>>
>>>> +
>>>> +- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
>>>> +  atomic drivers we could check for valid formats by calling
>>>> +  drm_plane_check_pixel_format() against all planes, and pass if any plane
>>>> +  supports the format. For non-atomic that's not possible since like the format
>>>> +  list for the primary plane is fake and we'd therefor reject valid formats.
>>>> +
>>>> +- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
>>>> +  version of the varios drm_gem_fb_create functions. Maybe called
>>>> +  drm_gem_fb_create/_with_dirty/_with_funcs as needed.
>>>> +
>>>> +Contact: Daniel Vetter
>>>> +
>>>> +Level: Intermediate
>>>> +
>>>>  Clean up mmap forwarding
>>>>  ------------------------
>>>>  
>>>>
>>>
>>> -- 
>>> 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
> 

-- 
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