[PATCH] drm/rockchip: use generic fbdev setup

[PATCH] drm/rockchip: use generic fbdev setup

[John Keeping]

The Rockchip fbdev code does not add anything compared to
drm_fbdev_generic_setup(); the one custom function for .fb_mmap does the
same thing as gem_prime_mmap which is called by the helper.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/gpu/drm/rockchip/Makefile             |   1 -
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  10 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |   2 -
 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 164 ------------------
 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h |  24 ---
 5 files changed, 2 insertions(+), 199 deletions(-)
 delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
 delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h

diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
index 17a9e7eb2130..1a56f696558c 100644
--- a/drivers/gpu/drm/rockchip/Makefile
+++ b/drivers/gpu/drm/rockchip/Makefile
@@ -5,7 +5,6 @@
 
 rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
 		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
-rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
 
 rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
 rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 69c699459dce..20d81ae69828 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -26,7 +26,6 @@
 
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_fb.h"
-#include "rockchip_drm_fbdev.h"
 #include "rockchip_drm_gem.h"
 
 #define DRIVER_NAME	"rockchip"
@@ -159,10 +158,6 @@ static int rockchip_drm_bind(struct device *dev)
 
 	drm_mode_config_reset(drm_dev);
 
-	ret = rockchip_drm_fbdev_init(drm_dev);
-	if (ret)
-		goto err_unbind_all;
-
 	/* init kms poll for handling hpd */
 	drm_kms_helper_poll_init(drm_dev);
 
@@ -170,10 +165,11 @@ static int rockchip_drm_bind(struct device *dev)
 	if (ret)
 		goto err_kms_helper_poll_fini;
 
+	drm_fbdev_generic_setup(drm_dev, 32);
+
 	return 0;
 err_kms_helper_poll_fini:
 	drm_kms_helper_poll_fini(drm_dev);
-	rockchip_drm_fbdev_fini(drm_dev);
 err_unbind_all:
 	component_unbind_all(dev, drm_dev);
 err_iommu_cleanup:
@@ -189,7 +185,6 @@ static void rockchip_drm_unbind(struct device *dev)
 
 	drm_dev_unregister(drm_dev);
 
-	rockchip_drm_fbdev_fini(drm_dev);
 	drm_kms_helper_poll_fini(drm_dev);
 
 	drm_atomic_helper_shutdown(drm_dev);
@@ -203,7 +198,6 @@ DEFINE_DRM_GEM_FOPS(rockchip_drm_driver_fops);
 
 static const struct drm_driver rockchip_drm_driver = {
 	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
-	.lastclose		= drm_fb_helper_lastclose,
 	.dumb_create		= rockchip_gem_dumb_create,
 	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index aa0909e8edf9..143a48330f84 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -43,8 +43,6 @@ struct rockchip_crtc_state {
  * @mm_lock: protect drm_mm on multi-threads.
  */
 struct rockchip_drm_private {
-	struct drm_fb_helper fbdev_helper;
-	struct drm_gem_object *fbdev_bo;
 	struct iommu_domain *domain;
 	struct mutex mm_lock;
 	struct drm_mm mm;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
deleted file mode 100644
index d8418dd39d0e..000000000000
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
+++ /dev/null
@@ -1,164 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
- * Author:Mark Yao <mark.yao@rock-chips.com>
- */
-
-#include <drm/drm.h>
-#include <drm/drm_fb_helper.h>
-#include <drm/drm_fourcc.h>
-#include <drm/drm_prime.h>
-#include <drm/drm_probe_helper.h>
-
-#include "rockchip_drm_drv.h"
-#include "rockchip_drm_gem.h"
-#include "rockchip_drm_fb.h"
-#include "rockchip_drm_fbdev.h"
-
-#define PREFERRED_BPP		32
-#define to_drm_private(x) \
-		container_of(x, struct rockchip_drm_private, fbdev_helper)
-
-static int rockchip_fbdev_mmap(struct fb_info *info,
-			       struct vm_area_struct *vma)
-{
-	struct drm_fb_helper *helper = info->par;
-	struct rockchip_drm_private *private = to_drm_private(helper);
-
-	return drm_gem_prime_mmap(private->fbdev_bo, vma);
-}
-
-static const struct fb_ops rockchip_drm_fbdev_ops = {
-	.owner		= THIS_MODULE,
-	DRM_FB_HELPER_DEFAULT_OPS,
-	.fb_mmap	= rockchip_fbdev_mmap,
-	.fb_fillrect	= drm_fb_helper_cfb_fillrect,
-	.fb_copyarea	= drm_fb_helper_cfb_copyarea,
-	.fb_imageblit	= drm_fb_helper_cfb_imageblit,
-};
-
-static int rockchip_drm_fbdev_create(struct drm_fb_helper *helper,
-				     struct drm_fb_helper_surface_size *sizes)
-{
-	struct rockchip_drm_private *private = to_drm_private(helper);
-	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
-	struct drm_device *dev = helper->dev;
-	struct rockchip_gem_object *rk_obj;
-	struct drm_framebuffer *fb;
-	unsigned int bytes_per_pixel;
-	unsigned long offset;
-	struct fb_info *fbi;
-	size_t size;
-	int ret;
-
-	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] = sizes->surface_width * bytes_per_pixel;
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
-		sizes->surface_depth);
-
-	size = mode_cmd.pitches[0] * mode_cmd.height;
-
-	rk_obj = rockchip_gem_create_object(dev, size, true);
-	if (IS_ERR(rk_obj))
-		return -ENOMEM;
-
-	private->fbdev_bo = &rk_obj->base;
-
-	fbi = drm_fb_helper_alloc_fbi(helper);
-	if (IS_ERR(fbi)) {
-		DRM_DEV_ERROR(dev->dev, "Failed to create framebuffer info.\n");
-		ret = PTR_ERR(fbi);
-		goto out;
-	}
-
-	helper->fb = rockchip_drm_framebuffer_init(dev, &mode_cmd,
-						   private->fbdev_bo);
-	if (IS_ERR(helper->fb)) {
-		DRM_DEV_ERROR(dev->dev,
-			      "Failed to allocate DRM framebuffer.\n");
-		ret = PTR_ERR(helper->fb);
-		goto out;
-	}
-
-	fbi->fbops = &rockchip_drm_fbdev_ops;
-
-	fb = helper->fb;
-	drm_fb_helper_fill_info(fbi, helper, sizes);
-
-	offset = fbi->var.xoffset * bytes_per_pixel;
-	offset += fbi->var.yoffset * fb->pitches[0];
-
-	dev->mode_config.fb_base = 0;
-	fbi->screen_base = rk_obj->kvaddr + offset;
-	fbi->screen_size = rk_obj->base.size;
-	fbi->fix.smem_len = rk_obj->base.size;
-
-	DRM_DEBUG_KMS("FB [%dx%d]-%d kvaddr=%p offset=%ld size=%zu\n",
-		      fb->width, fb->height, fb->format->depth,
-		      rk_obj->kvaddr,
-		      offset, size);
-
-	return 0;
-
-out:
-	rockchip_gem_free_object(&rk_obj->base);
-	return ret;
-}
-
-static const struct drm_fb_helper_funcs rockchip_drm_fb_helper_funcs = {
-	.fb_probe = rockchip_drm_fbdev_create,
-};
-
-int rockchip_drm_fbdev_init(struct drm_device *dev)
-{
-	struct rockchip_drm_private *private = dev->dev_private;
-	struct drm_fb_helper *helper;
-	int ret;
-
-	if (!dev->mode_config.num_crtc || !dev->mode_config.num_connector)
-		return -EINVAL;
-
-	helper = &private->fbdev_helper;
-
-	drm_fb_helper_prepare(dev, helper, &rockchip_drm_fb_helper_funcs);
-
-	ret = drm_fb_helper_init(dev, helper);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev->dev,
-			      "Failed to initialize drm fb helper - %d.\n",
-			      ret);
-		return ret;
-	}
-
-	ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev->dev,
-			      "Failed to set initial hw config - %d.\n",
-			      ret);
-		goto err_drm_fb_helper_fini;
-	}
-
-	return 0;
-
-err_drm_fb_helper_fini:
-	drm_fb_helper_fini(helper);
-	return ret;
-}
-
-void rockchip_drm_fbdev_fini(struct drm_device *dev)
-{
-	struct rockchip_drm_private *private = dev->dev_private;
-	struct drm_fb_helper *helper;
-
-	helper = &private->fbdev_helper;
-
-	drm_fb_helper_unregister_fbi(helper);
-
-	if (helper->fb)
-		drm_framebuffer_put(helper->fb);
-
-	drm_fb_helper_fini(helper);
-}
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
deleted file mode 100644
index 5fb7ac2371a8..000000000000
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
+++ /dev/null
@@ -1,24 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
- * Author:Mark Yao <mark.yao@rock-chips.com>
- */
-
-#ifndef _ROCKCHIP_DRM_FBDEV_H
-#define _ROCKCHIP_DRM_FBDEV_H
-
-#ifdef CONFIG_DRM_FBDEV_EMULATION
-int rockchip_drm_fbdev_init(struct drm_device *dev);
-void rockchip_drm_fbdev_fini(struct drm_device *dev);
-#else
-static inline int rockchip_drm_fbdev_init(struct drm_device *dev)
-{
-	return 0;
-}
-
-static inline void rockchip_drm_fbdev_fini(struct drm_device *dev)
-{
-}
-#endif
-
-#endif /* _ROCKCHIP_DRM_FBDEV_H */
-- 
2.33.1

Re: [PATCH] drm/rockchip: use generic fbdev setup

[Thomas Zimmermann]

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

Hi

Am 29.10.21 um 13:50 schrieb John Keeping:
> The Rockchip fbdev code does not add anything compared to
> drm_fbdev_generic_setup(); the one custom function for .fb_mmap does the
> same thing as gem_prime_mmap which is called by the helper.
> 
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>   drivers/gpu/drm/rockchip/Makefile             |   1 -
>   drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  10 +-
>   drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |   2 -
>   drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 164 ------------------
>   drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h |  24 ---
>   5 files changed, 2 insertions(+), 199 deletions(-)
>   delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
>   delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> 
> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> index 17a9e7eb2130..1a56f696558c 100644
> --- a/drivers/gpu/drm/rockchip/Makefile
> +++ b/drivers/gpu/drm/rockchip/Makefile
> @@ -5,7 +5,6 @@
>   
>   rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
>   		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
> -rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>   
>   rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
>   rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 69c699459dce..20d81ae69828 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -26,7 +26,6 @@
>   
>   #include "rockchip_drm_drv.h"
>   #include "rockchip_drm_fb.h"
> -#include "rockchip_drm_fbdev.h"
>   #include "rockchip_drm_gem.h"
>   
>   #define DRIVER_NAME	"rockchip"
> @@ -159,10 +158,6 @@ static int rockchip_drm_bind(struct device *dev)
>   
>   	drm_mode_config_reset(drm_dev);
>   
> -	ret = rockchip_drm_fbdev_init(drm_dev);
> -	if (ret)
> -		goto err_unbind_all;
> -
>   	/* init kms poll for handling hpd */
>   	drm_kms_helper_poll_init(drm_dev);
>   
> @@ -170,10 +165,11 @@ static int rockchip_drm_bind(struct device *dev)
>   	if (ret)
>   		goto err_kms_helper_poll_fini;
>   
> +	drm_fbdev_generic_setup(drm_dev, 32);

Please pass 0 for the final argument. The fbdev helpers pick 32 by 
default. Maybe leave a comment if you require 32 here.

For RGB colors, you should rather set 
drm_device.mode_config.preferred_depth to 24 instead.

With these changes:

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

Best regards
Thomas


> +
>   	return 0;
>   err_kms_helper_poll_fini:
>   	drm_kms_helper_poll_fini(drm_dev);
> -	rockchip_drm_fbdev_fini(drm_dev);
>   err_unbind_all:
>   	component_unbind_all(dev, drm_dev);
>   err_iommu_cleanup:
> @@ -189,7 +185,6 @@ static void rockchip_drm_unbind(struct device *dev)
>   
>   	drm_dev_unregister(drm_dev);
>   
> -	rockchip_drm_fbdev_fini(drm_dev);
>   	drm_kms_helper_poll_fini(drm_dev);
>   
>   	drm_atomic_helper_shutdown(drm_dev);
> @@ -203,7 +198,6 @@ DEFINE_DRM_GEM_FOPS(rockchip_drm_driver_fops);
>   
>   static const struct drm_driver rockchip_drm_driver = {
>   	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
> -	.lastclose		= drm_fb_helper_lastclose,
>   	.dumb_create		= rockchip_gem_dumb_create,
>   	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
>   	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index aa0909e8edf9..143a48330f84 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -43,8 +43,6 @@ struct rockchip_crtc_state {
>    * @mm_lock: protect drm_mm on multi-threads.
>    */
>   struct rockchip_drm_private {
> -	struct drm_fb_helper fbdev_helper;
> -	struct drm_gem_object *fbdev_bo;
>   	struct iommu_domain *domain;
>   	struct mutex mm_lock;
>   	struct drm_mm mm;
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
> deleted file mode 100644
> index d8418dd39d0e..000000000000
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
> +++ /dev/null
> @@ -1,164 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> - * Author:Mark Yao <mark.yao@rock-chips.com>
> - */
> -
> -#include <drm/drm.h>
> -#include <drm/drm_fb_helper.h>
> -#include <drm/drm_fourcc.h>
> -#include <drm/drm_prime.h>
> -#include <drm/drm_probe_helper.h>
> -
> -#include "rockchip_drm_drv.h"
> -#include "rockchip_drm_gem.h"
> -#include "rockchip_drm_fb.h"
> -#include "rockchip_drm_fbdev.h"
> -
> -#define PREFERRED_BPP		32
> -#define to_drm_private(x) \
> -		container_of(x, struct rockchip_drm_private, fbdev_helper)
> -
> -static int rockchip_fbdev_mmap(struct fb_info *info,
> -			       struct vm_area_struct *vma)
> -{
> -	struct drm_fb_helper *helper = info->par;
> -	struct rockchip_drm_private *private = to_drm_private(helper);
> -
> -	return drm_gem_prime_mmap(private->fbdev_bo, vma);
> -}
> -
> -static const struct fb_ops rockchip_drm_fbdev_ops = {
> -	.owner		= THIS_MODULE,
> -	DRM_FB_HELPER_DEFAULT_OPS,
> -	.fb_mmap	= rockchip_fbdev_mmap,
> -	.fb_fillrect	= drm_fb_helper_cfb_fillrect,
> -	.fb_copyarea	= drm_fb_helper_cfb_copyarea,
> -	.fb_imageblit	= drm_fb_helper_cfb_imageblit,
> -};
> -
> -static int rockchip_drm_fbdev_create(struct drm_fb_helper *helper,
> -				     struct drm_fb_helper_surface_size *sizes)
> -{
> -	struct rockchip_drm_private *private = to_drm_private(helper);
> -	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> -	struct drm_device *dev = helper->dev;
> -	struct rockchip_gem_object *rk_obj;
> -	struct drm_framebuffer *fb;
> -	unsigned int bytes_per_pixel;
> -	unsigned long offset;
> -	struct fb_info *fbi;
> -	size_t size;
> -	int ret;
> -
> -	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] = sizes->surface_width * bytes_per_pixel;
> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> -		sizes->surface_depth);
> -
> -	size = mode_cmd.pitches[0] * mode_cmd.height;
> -
> -	rk_obj = rockchip_gem_create_object(dev, size, true);
> -	if (IS_ERR(rk_obj))
> -		return -ENOMEM;
> -
> -	private->fbdev_bo = &rk_obj->base;
> -
> -	fbi = drm_fb_helper_alloc_fbi(helper);
> -	if (IS_ERR(fbi)) {
> -		DRM_DEV_ERROR(dev->dev, "Failed to create framebuffer info.\n");
> -		ret = PTR_ERR(fbi);
> -		goto out;
> -	}
> -
> -	helper->fb = rockchip_drm_framebuffer_init(dev, &mode_cmd,
> -						   private->fbdev_bo);
> -	if (IS_ERR(helper->fb)) {
> -		DRM_DEV_ERROR(dev->dev,
> -			      "Failed to allocate DRM framebuffer.\n");
> -		ret = PTR_ERR(helper->fb);
> -		goto out;
> -	}
> -
> -	fbi->fbops = &rockchip_drm_fbdev_ops;
> -
> -	fb = helper->fb;
> -	drm_fb_helper_fill_info(fbi, helper, sizes);
> -
> -	offset = fbi->var.xoffset * bytes_per_pixel;
> -	offset += fbi->var.yoffset * fb->pitches[0];
> -
> -	dev->mode_config.fb_base = 0;
> -	fbi->screen_base = rk_obj->kvaddr + offset;
> -	fbi->screen_size = rk_obj->base.size;
> -	fbi->fix.smem_len = rk_obj->base.size;
> -
> -	DRM_DEBUG_KMS("FB [%dx%d]-%d kvaddr=%p offset=%ld size=%zu\n",
> -		      fb->width, fb->height, fb->format->depth,
> -		      rk_obj->kvaddr,
> -		      offset, size);
> -
> -	return 0;
> -
> -out:
> -	rockchip_gem_free_object(&rk_obj->base);
> -	return ret;
> -}
> -
> -static const struct drm_fb_helper_funcs rockchip_drm_fb_helper_funcs = {
> -	.fb_probe = rockchip_drm_fbdev_create,
> -};
> -
> -int rockchip_drm_fbdev_init(struct drm_device *dev)
> -{
> -	struct rockchip_drm_private *private = dev->dev_private;
> -	struct drm_fb_helper *helper;
> -	int ret;
> -
> -	if (!dev->mode_config.num_crtc || !dev->mode_config.num_connector)
> -		return -EINVAL;
> -
> -	helper = &private->fbdev_helper;
> -
> -	drm_fb_helper_prepare(dev, helper, &rockchip_drm_fb_helper_funcs);
> -
> -	ret = drm_fb_helper_init(dev, helper);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(dev->dev,
> -			      "Failed to initialize drm fb helper - %d.\n",
> -			      ret);
> -		return ret;
> -	}
> -
> -	ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(dev->dev,
> -			      "Failed to set initial hw config - %d.\n",
> -			      ret);
> -		goto err_drm_fb_helper_fini;
> -	}
> -
> -	return 0;
> -
> -err_drm_fb_helper_fini:
> -	drm_fb_helper_fini(helper);
> -	return ret;
> -}
> -
> -void rockchip_drm_fbdev_fini(struct drm_device *dev)
> -{
> -	struct rockchip_drm_private *private = dev->dev_private;
> -	struct drm_fb_helper *helper;
> -
> -	helper = &private->fbdev_helper;
> -
> -	drm_fb_helper_unregister_fbi(helper);
> -
> -	if (helper->fb)
> -		drm_framebuffer_put(helper->fb);
> -
> -	drm_fb_helper_fini(helper);
> -}
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> deleted file mode 100644
> index 5fb7ac2371a8..000000000000
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> - * Author:Mark Yao <mark.yao@rock-chips.com>
> - */
> -
> -#ifndef _ROCKCHIP_DRM_FBDEV_H
> -#define _ROCKCHIP_DRM_FBDEV_H
> -
> -#ifdef CONFIG_DRM_FBDEV_EMULATION
> -int rockchip_drm_fbdev_init(struct drm_device *dev);
> -void rockchip_drm_fbdev_fini(struct drm_device *dev);
> -#else
> -static inline int rockchip_drm_fbdev_init(struct drm_device *dev)
> -{
> -	return 0;
> -}
> -
> -static inline void rockchip_drm_fbdev_fini(struct drm_device *dev)
> -{
> -}
> -#endif
> -
> -#endif /* _ROCKCHIP_DRM_FBDEV_H */
> 

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/rockchip: use generic fbdev setup

[John Keeping]

Hi Thomas,

On Fri, Oct 29, 2021 at 09:00:08PM +0200, Thomas Zimmermann wrote:
> Am 29.10.21 um 13:50 schrieb John Keeping:
> > The Rockchip fbdev code does not add anything compared to
> > drm_fbdev_generic_setup(); the one custom function for .fb_mmap does the
> > same thing as gem_prime_mmap which is called by the helper.
> > 
> > Signed-off-by: John Keeping <john@metanate.com>
> > ---
> >   drivers/gpu/drm/rockchip/Makefile             |   1 -
> >   drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  10 +-
> >   drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |   2 -
> >   drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 164 ------------------
> >   drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h |  24 ---
> >   5 files changed, 2 insertions(+), 199 deletions(-)
> >   delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
> >   delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> > 
> > diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> > index 17a9e7eb2130..1a56f696558c 100644
> > --- a/drivers/gpu/drm/rockchip/Makefile
> > +++ b/drivers/gpu/drm/rockchip/Makefile
> > @@ -5,7 +5,6 @@
> >   rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> >   		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
> > -rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
> >   rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> >   rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > index 69c699459dce..20d81ae69828 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > @@ -26,7 +26,6 @@
> >   #include "rockchip_drm_drv.h"
> >   #include "rockchip_drm_fb.h"
> > -#include "rockchip_drm_fbdev.h"
> >   #include "rockchip_drm_gem.h"
> >   #define DRIVER_NAME	"rockchip"
> > @@ -159,10 +158,6 @@ static int rockchip_drm_bind(struct device *dev)
> >   	drm_mode_config_reset(drm_dev);
> > -	ret = rockchip_drm_fbdev_init(drm_dev);
> > -	if (ret)
> > -		goto err_unbind_all;
> > -
> >   	/* init kms poll for handling hpd */
> >   	drm_kms_helper_poll_init(drm_dev);
> > @@ -170,10 +165,11 @@ static int rockchip_drm_bind(struct device *dev)
> >   	if (ret)
> >   		goto err_kms_helper_poll_fini;
> > +	drm_fbdev_generic_setup(drm_dev, 32);
> 
> Please pass 0 for the final argument. The fbdev helpers pick 32 by default.
> Maybe leave a comment if you require 32 here.

I wanted to minimise the changes introduced here - passing 32 matches
the value passed to drm_fb_helper_initial_config() in the deleted code
from rockchip_drm_fbdev.c.

What do you think about changing this to 0 in a follow-up patch?

Re: [PATCH] drm/rockchip: use generic fbdev setup

[Thomas Zimmermann]

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

Hi

Am 30.10.21 um 14:05 schrieb John Keeping:
> Hi Thomas,
> 
> On Fri, Oct 29, 2021 at 09:00:08PM +0200, Thomas Zimmermann wrote:
>> Am 29.10.21 um 13:50 schrieb John Keeping:
>>> The Rockchip fbdev code does not add anything compared to
>>> drm_fbdev_generic_setup(); the one custom function for .fb_mmap does the
>>> same thing as gem_prime_mmap which is called by the helper.
>>>
>>> Signed-off-by: John Keeping <john@metanate.com>
>>> ---
>>>    drivers/gpu/drm/rockchip/Makefile             |   1 -
>>>    drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  10 +-
>>>    drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |   2 -
>>>    drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 164 ------------------
>>>    drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h |  24 ---
>>>    5 files changed, 2 insertions(+), 199 deletions(-)
>>>    delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
>>>    delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
>>> index 17a9e7eb2130..1a56f696558c 100644
>>> --- a/drivers/gpu/drm/rockchip/Makefile
>>> +++ b/drivers/gpu/drm/rockchip/Makefile
>>> @@ -5,7 +5,6 @@
>>>    rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
>>>    		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
>>> -rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>>>    rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
>>>    rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>> index 69c699459dce..20d81ae69828 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>> @@ -26,7 +26,6 @@
>>>    #include "rockchip_drm_drv.h"
>>>    #include "rockchip_drm_fb.h"
>>> -#include "rockchip_drm_fbdev.h"
>>>    #include "rockchip_drm_gem.h"
>>>    #define DRIVER_NAME	"rockchip"
>>> @@ -159,10 +158,6 @@ static int rockchip_drm_bind(struct device *dev)
>>>    	drm_mode_config_reset(drm_dev);
>>> -	ret = rockchip_drm_fbdev_init(drm_dev);
>>> -	if (ret)
>>> -		goto err_unbind_all;
>>> -
>>>    	/* init kms poll for handling hpd */
>>>    	drm_kms_helper_poll_init(drm_dev);
>>> @@ -170,10 +165,11 @@ static int rockchip_drm_bind(struct device *dev)
>>>    	if (ret)
>>>    		goto err_kms_helper_poll_fini;
>>> +	drm_fbdev_generic_setup(drm_dev, 32);
>>
>> Please pass 0 for the final argument. The fbdev helpers pick 32 by default.
>> Maybe leave a comment if you require 32 here.
> 
> I wanted to minimise the changes introduced here - passing 32 matches
> the value passed to drm_fb_helper_initial_config() in the deleted code
> from rockchip_drm_fbdev.c.

In that case

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

> 
> What do you think about changing this to 0 in a follow-up patch?
> 

Yes. If possible, please provide a follow-up patch for this and set 
modeconfig.prefered_depth to 24.

Best regards
Thomas


-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/rockchip: use generic fbdev setup

[John Keeping]

On Sun, 31 Oct 2021 19:09:39 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 30.10.21 um 14:05 schrieb John Keeping:
> > On Fri, Oct 29, 2021 at 09:00:08PM +0200, Thomas Zimmermann wrote:  
> >> Am 29.10.21 um 13:50 schrieb John Keeping:  
> >>> The Rockchip fbdev code does not add anything compared to
> >>> drm_fbdev_generic_setup(); the one custom function for .fb_mmap does the
> >>> same thing as gem_prime_mmap which is called by the helper.
> >>>
> >>> Signed-off-by: John Keeping <john@metanate.com>
> >>> ---
> >>>    drivers/gpu/drm/rockchip/Makefile             |   1 -
> >>>    drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  10 +-
> >>>    drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |   2 -
> >>>    drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 164 ------------------
> >>>    drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h |  24 ---
> >>>    5 files changed, 2 insertions(+), 199 deletions(-)
> >>>    delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
> >>>    delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> >>>
> >>> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> >>> index 17a9e7eb2130..1a56f696558c 100644
> >>> --- a/drivers/gpu/drm/rockchip/Makefile
> >>> +++ b/drivers/gpu/drm/rockchip/Makefile
> >>> @@ -5,7 +5,6 @@
> >>>    rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> >>>    		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
> >>> -rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
> >>>    rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> >>>    rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> >>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >>> index 69c699459dce..20d81ae69828 100644
> >>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >>> @@ -26,7 +26,6 @@
> >>>    #include "rockchip_drm_drv.h"
> >>>    #include "rockchip_drm_fb.h"
> >>> -#include "rockchip_drm_fbdev.h"
> >>>    #include "rockchip_drm_gem.h"
> >>>    #define DRIVER_NAME	"rockchip"
> >>> @@ -159,10 +158,6 @@ static int rockchip_drm_bind(struct device *dev)
> >>>    	drm_mode_config_reset(drm_dev);
> >>> -	ret = rockchip_drm_fbdev_init(drm_dev);
> >>> -	if (ret)
> >>> -		goto err_unbind_all;
> >>> -
> >>>    	/* init kms poll for handling hpd */
> >>>    	drm_kms_helper_poll_init(drm_dev);
> >>> @@ -170,10 +165,11 @@ static int rockchip_drm_bind(struct device *dev)
> >>>    	if (ret)
> >>>    		goto err_kms_helper_poll_fini;
> >>> +	drm_fbdev_generic_setup(drm_dev, 32);  
> >>
> >> Please pass 0 for the final argument. The fbdev helpers pick 32 by default.
> >> Maybe leave a comment if you require 32 here.  
> > 
> > I wanted to minimise the changes introduced here - passing 32 matches
> > the value passed to drm_fb_helper_initial_config() in the deleted code
> > from rockchip_drm_fbdev.c.  
> 
> In that case
> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks!

> > 
> > What do you think about changing this to 0 in a follow-up patch?
> >   
> 
> Yes. If possible, please provide a follow-up patch for this and set 
> modeconfig.prefered_depth to 24.

I'll follow up with a patch setting this to zero, but I'm not convinced
I understand mode_config.prefered_depth well enough to be confident
setting it to 24 is the right thing to do.

Looking at commits 23d4e55f7eeb ("drm/vkms: Unset preferred_depth") and
550f17441f53 ("drm/cirrus: flip default from 24bpp to 16bpp") it seems
that this will have a wider impact beyond fbdev.  32bpp has been used
since the Rockchip driver was added so I don't see any real need to
change to 24 now.


Regards,
John

Re: [PATCH] drm/rockchip: use generic fbdev setup

[John Keeping]

Hi Thomas,

On Mon, Nov 01, 2021 at 11:34:15AM +0000, John Keeping wrote:
> On Sun, 31 Oct 2021 19:09:39 +0100
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > Am 30.10.21 um 14:05 schrieb John Keeping:
> > > On Fri, Oct 29, 2021 at 09:00:08PM +0200, Thomas Zimmermann wrote:  
> > >> Am 29.10.21 um 13:50 schrieb John Keeping:  
> > >>> The Rockchip fbdev code does not add anything compared to
> > >>> drm_fbdev_generic_setup(); the one custom function for .fb_mmap does the
> > >>> same thing as gem_prime_mmap which is called by the helper.
> > >>>
> > >>> Signed-off-by: John Keeping <john@metanate.com>
> > >>> ---
> > >>>    drivers/gpu/drm/rockchip/Makefile             |   1 -
> > >>>    drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  10 +-
> > >>>    drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |   2 -
> > >>>    drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 164 ------------------
> > >>>    drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h |  24 ---
> > >>>    5 files changed, 2 insertions(+), 199 deletions(-)
> > >>>    delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
> > >>>    delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> > >>>
> > >>> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> > >>> index 17a9e7eb2130..1a56f696558c 100644
> > >>> --- a/drivers/gpu/drm/rockchip/Makefile
> > >>> +++ b/drivers/gpu/drm/rockchip/Makefile
> > >>> @@ -5,7 +5,6 @@
> > >>>    rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> > >>>    		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
> > >>> -rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
> > >>>    rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> > >>>    rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> > >>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > >>> index 69c699459dce..20d81ae69828 100644
> > >>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > >>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > >>> @@ -26,7 +26,6 @@
> > >>>    #include "rockchip_drm_drv.h"
> > >>>    #include "rockchip_drm_fb.h"
> > >>> -#include "rockchip_drm_fbdev.h"
> > >>>    #include "rockchip_drm_gem.h"
> > >>>    #define DRIVER_NAME	"rockchip"
> > >>> @@ -159,10 +158,6 @@ static int rockchip_drm_bind(struct device *dev)
> > >>>    	drm_mode_config_reset(drm_dev);
> > >>> -	ret = rockchip_drm_fbdev_init(drm_dev);
> > >>> -	if (ret)
> > >>> -		goto err_unbind_all;
> > >>> -
> > >>>    	/* init kms poll for handling hpd */
> > >>>    	drm_kms_helper_poll_init(drm_dev);
> > >>> @@ -170,10 +165,11 @@ static int rockchip_drm_bind(struct device *dev)
> > >>>    	if (ret)
> > >>>    		goto err_kms_helper_poll_fini;
> > >>> +	drm_fbdev_generic_setup(drm_dev, 32);  
> > >>
> > >> Please pass 0 for the final argument. The fbdev helpers pick 32 by default.
> > >> Maybe leave a comment if you require 32 here.  
> > > 
> > > I wanted to minimise the changes introduced here - passing 32 matches
> > > the value passed to drm_fb_helper_initial_config() in the deleted code
> > > from rockchip_drm_fbdev.c.  
> > 
> > In that case
> > 
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Thanks!

Are you able to pick this up (and [1])?  Otherwise what is needed here
and who should pick this up?

[1] https://lore.kernel.org/dri-devel/20211101114622.813536-1-john@metanate.com/

Re: [PATCH] drm/rockchip: use generic fbdev setup

[Thomas Zimmermann]

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

Hi

Am 07.12.21 um 12:54 schrieb John Keeping:
> 
> Are you able to pick this up (and [1])?  Otherwise what is needed here
> and who should pick this up?
> 
> [1] https://lore.kernel.org/dri-devel/20211101114622.813536-1-john@metanate.com/
> 

I added both patches to drm-misc-next.

Best regards
Thomas


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

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

[BUG] [PATCH] drm/rockchip: use generic fbdev setup

[Johan Jonker]

Hi John,

Your patch contribution causes a kernel panic on MK808 with Rockchip rk3066a SoC.
Would you like to contribute to fix this issue?
The assumtion that drm_fbdev_generic_setup() does what rockchip_drm_fbdev_init did is not true!
A revert makes it work again.

Johan

======

[    7.975906] ------------[ cut here ]------------
[    7.975929] WARNING: CPU: 0 PID: 35 at drivers/gpu/drm/drm_fb_helper.c:471 drm_fb_helper_damage_work+0x138/0x3b4
[    7.976044] rockchip-drm display-subsystem: Damage blitter failed: ret=-12
[    7.976064] Modules linked in:
[    7.976090] CPU: 0 PID: 35 Comm: kworker/0:4 Not tainted 6.0.0-next-20221013 #1
[    7.976126] Hardware name: Rockchip (Device Tree)
[    7.976145] Workqueue: events drm_fb_helper_damage_work
[    7.976196] Backtrace: 
[    7.976214]  dump_backtrace from show_stack+0x20/0x24
[    7.976276]  r7:000001d7 r6:00000009 r5:c0b2bc78 r4:60000013
[    7.976289]  show_stack from dump_stack_lvl+0x48/0x54
[    7.976357]  dump_stack_lvl from dump_stack+0x18/0x1c
[    7.976426]  r5:c0586054 r4:c0b63750
[    7.976436]  dump_stack from __warn+0xdc/0x154
[    7.976525]  __warn from warn_slowpath_fmt+0xa4/0xd8
[    7.976588]  r7:000001d7 r6:c0b63750 r5:c1004ec8 r4:c0b639ec
[    7.976598]  warn_slowpath_fmt from drm_fb_helper_damage_work+0x138/0x3b4
[    7.976670]  r9:ef7cf105 r8:c15dfc00 r7:fffffff4 r6:c200a590 r5:c1004ec8 r4:c200a594
[    7.976681]  drm_fb_helper_damage_work from process_one_work+0x230/0x518
[    7.976761]  r10:c110d140 r9:ef7cf105 r8:00000000 r7:ef7cf100 r6:ef7cbf00 r5:c200f300
[    7.976775]  r4:c200a594
[    7.976785]  process_one_work from worker_thread+0x54/0x554
[    7.976841]  r10:ef7cbf00 r9:00000008 r8:c1003d40 r7:ef7cbf1c r6:c200f318 r5:ef7cbf00
[    7.976855]  r4:c200f300
[    7.976864]  worker_thread from kthread+0xe8/0x104
[    7.976948]  r10:f0929e84 r9:c200ea40 r8:c169aa80 r7:c200f300 r6:c01419e4 r5:00000000
[    7.976962]  r4:c200e800
[    7.976971]  kthread from ret_from_fork+0x14/0x2c
[    7.977026] Exception stack(0xf092dfb0 to 0xf092dff8)
[    7.977052] dfa0:                                     00000000 00000000 00000000 00000000
[    7.977078] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    7.977100] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    7.977128]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c01491a8
[    7.977144]  r4:c200e800 r3:00000001
[    7.977155] ---[ end trace 0000000000000000 ]---

On 10/29/21 13:50, John Keeping wrote:
> The Rockchip fbdev code does not add anything compared to
> drm_fbdev_generic_setup(); the one custom function for .fb_mmap does the
> same thing as gem_prime_mmap which is called by the helper.
> 
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>  drivers/gpu/drm/rockchip/Makefile             |   1 -
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  10 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |   2 -
>  drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 164 ------------------
>  drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h |  24 ---
>  5 files changed, 2 insertions(+), 199 deletions(-)
>  delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
>  delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> 
> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> index 17a9e7eb2130..1a56f696558c 100644
> --- a/drivers/gpu/drm/rockchip/Makefile
> +++ b/drivers/gpu/drm/rockchip/Makefile
> @@ -5,7 +5,6 @@
>  
>  rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
>  		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
> -rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>  
>  rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
>  rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 69c699459dce..20d81ae69828 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -26,7 +26,6 @@
>  
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_fb.h"
> -#include "rockchip_drm_fbdev.h"
>  #include "rockchip_drm_gem.h"
>  
>  #define DRIVER_NAME	"rockchip"
> @@ -159,10 +158,6 @@ static int rockchip_drm_bind(struct device *dev)
>  
>  	drm_mode_config_reset(drm_dev);
>  
> -	ret = rockchip_drm_fbdev_init(drm_dev);
> -	if (ret)
> -		goto err_unbind_all;
> -
>  	/* init kms poll for handling hpd */
>  	drm_kms_helper_poll_init(drm_dev);
>  
> @@ -170,10 +165,11 @@ static int rockchip_drm_bind(struct device *dev)
>  	if (ret)
>  		goto err_kms_helper_poll_fini;
>  
> +	drm_fbdev_generic_setup(drm_dev, 32);
> +
>  	return 0;
>  err_kms_helper_poll_fini:
>  	drm_kms_helper_poll_fini(drm_dev);
> -	rockchip_drm_fbdev_fini(drm_dev);
>  err_unbind_all:
>  	component_unbind_all(dev, drm_dev);
>  err_iommu_cleanup:
> @@ -189,7 +185,6 @@ static void rockchip_drm_unbind(struct device *dev)
>  
>  	drm_dev_unregister(drm_dev);
>  
> -	rockchip_drm_fbdev_fini(drm_dev);
>  	drm_kms_helper_poll_fini(drm_dev);
>  
>  	drm_atomic_helper_shutdown(drm_dev);
> @@ -203,7 +198,6 @@ DEFINE_DRM_GEM_FOPS(rockchip_drm_driver_fops);
>  
>  static const struct drm_driver rockchip_drm_driver = {
>  	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
> -	.lastclose		= drm_fb_helper_lastclose,
>  	.dumb_create		= rockchip_gem_dumb_create,
>  	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index aa0909e8edf9..143a48330f84 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -43,8 +43,6 @@ struct rockchip_crtc_state {
>   * @mm_lock: protect drm_mm on multi-threads.
>   */
>  struct rockchip_drm_private {
> -	struct drm_fb_helper fbdev_helper;
> -	struct drm_gem_object *fbdev_bo;
>  	struct iommu_domain *domain;
>  	struct mutex mm_lock;
>  	struct drm_mm mm;
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
> deleted file mode 100644
> index d8418dd39d0e..000000000000
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
> +++ /dev/null
> @@ -1,164 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> - * Author:Mark Yao <mark.yao@rock-chips.com>
> - */
> -
> -#include <drm/drm.h>
> -#include <drm/drm_fb_helper.h>
> -#include <drm/drm_fourcc.h>
> -#include <drm/drm_prime.h>
> -#include <drm/drm_probe_helper.h>
> -
> -#include "rockchip_drm_drv.h"
> -#include "rockchip_drm_gem.h"
> -#include "rockchip_drm_fb.h"
> -#include "rockchip_drm_fbdev.h"
> -
> -#define PREFERRED_BPP		32
> -#define to_drm_private(x) \
> -		container_of(x, struct rockchip_drm_private, fbdev_helper)
> -
> -static int rockchip_fbdev_mmap(struct fb_info *info,
> -			       struct vm_area_struct *vma)
> -{
> -	struct drm_fb_helper *helper = info->par;
> -	struct rockchip_drm_private *private = to_drm_private(helper);
> -
> -	return drm_gem_prime_mmap(private->fbdev_bo, vma);
> -}
> -
> -static const struct fb_ops rockchip_drm_fbdev_ops = {
> -	.owner		= THIS_MODULE,
> -	DRM_FB_HELPER_DEFAULT_OPS,
> -	.fb_mmap	= rockchip_fbdev_mmap,
> -	.fb_fillrect	= drm_fb_helper_cfb_fillrect,
> -	.fb_copyarea	= drm_fb_helper_cfb_copyarea,
> -	.fb_imageblit	= drm_fb_helper_cfb_imageblit,
> -};
> -
> -static int rockchip_drm_fbdev_create(struct drm_fb_helper *helper,
> -				     struct drm_fb_helper_surface_size *sizes)
> -{
> -	struct rockchip_drm_private *private = to_drm_private(helper);
> -	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> -	struct drm_device *dev = helper->dev;
> -	struct rockchip_gem_object *rk_obj;
> -	struct drm_framebuffer *fb;
> -	unsigned int bytes_per_pixel;
> -	unsigned long offset;
> -	struct fb_info *fbi;
> -	size_t size;
> -	int ret;
> -
> -	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] = sizes->surface_width * bytes_per_pixel;
> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> -		sizes->surface_depth);
> -
> -	size = mode_cmd.pitches[0] * mode_cmd.height;
> -
> -	rk_obj = rockchip_gem_create_object(dev, size, true);
> -	if (IS_ERR(rk_obj))
> -		return -ENOMEM;
> -
> -	private->fbdev_bo = &rk_obj->base;
> -
> -	fbi = drm_fb_helper_alloc_fbi(helper);
> -	if (IS_ERR(fbi)) {
> -		DRM_DEV_ERROR(dev->dev, "Failed to create framebuffer info.\n");
> -		ret = PTR_ERR(fbi);
> -		goto out;
> -	}
> -
> -	helper->fb = rockchip_drm_framebuffer_init(dev, &mode_cmd,
> -						   private->fbdev_bo);
> -	if (IS_ERR(helper->fb)) {
> -		DRM_DEV_ERROR(dev->dev,
> -			      "Failed to allocate DRM framebuffer.\n");
> -		ret = PTR_ERR(helper->fb);
> -		goto out;
> -	}
> -
> -	fbi->fbops = &rockchip_drm_fbdev_ops;
> -
> -	fb = helper->fb;
> -	drm_fb_helper_fill_info(fbi, helper, sizes);
> -
> -	offset = fbi->var.xoffset * bytes_per_pixel;
> -	offset += fbi->var.yoffset * fb->pitches[0];
> -
> -	dev->mode_config.fb_base = 0;
> -	fbi->screen_base = rk_obj->kvaddr + offset;
> -	fbi->screen_size = rk_obj->base.size;
> -	fbi->fix.smem_len = rk_obj->base.size;
> -
> -	DRM_DEBUG_KMS("FB [%dx%d]-%d kvaddr=%p offset=%ld size=%zu\n",
> -		      fb->width, fb->height, fb->format->depth,
> -		      rk_obj->kvaddr,
> -		      offset, size);
> -
> -	return 0;
> -
> -out:
> -	rockchip_gem_free_object(&rk_obj->base);
> -	return ret;
> -}
> -
> -static const struct drm_fb_helper_funcs rockchip_drm_fb_helper_funcs = {
> -	.fb_probe = rockchip_drm_fbdev_create,
> -};
> -
> -int rockchip_drm_fbdev_init(struct drm_device *dev)
> -{
> -	struct rockchip_drm_private *private = dev->dev_private;
> -	struct drm_fb_helper *helper;
> -	int ret;
> -
> -	if (!dev->mode_config.num_crtc || !dev->mode_config.num_connector)
> -		return -EINVAL;
> -
> -	helper = &private->fbdev_helper;
> -
> -	drm_fb_helper_prepare(dev, helper, &rockchip_drm_fb_helper_funcs);
> -
> -	ret = drm_fb_helper_init(dev, helper);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(dev->dev,
> -			      "Failed to initialize drm fb helper - %d.\n",
> -			      ret);
> -		return ret;
> -	}
> -
> -	ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(dev->dev,
> -			      "Failed to set initial hw config - %d.\n",
> -			      ret);
> -		goto err_drm_fb_helper_fini;
> -	}
> -
> -	return 0;
> -
> -err_drm_fb_helper_fini:
> -	drm_fb_helper_fini(helper);
> -	return ret;
> -}
> -
> -void rockchip_drm_fbdev_fini(struct drm_device *dev)
> -{
> -	struct rockchip_drm_private *private = dev->dev_private;
> -	struct drm_fb_helper *helper;
> -
> -	helper = &private->fbdev_helper;
> -
> -	drm_fb_helper_unregister_fbi(helper);
> -
> -	if (helper->fb)
> -		drm_framebuffer_put(helper->fb);
> -
> -	drm_fb_helper_fini(helper);
> -}
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> deleted file mode 100644
> index 5fb7ac2371a8..000000000000
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> - * Author:Mark Yao <mark.yao@rock-chips.com>
> - */
> -
> -#ifndef _ROCKCHIP_DRM_FBDEV_H
> -#define _ROCKCHIP_DRM_FBDEV_H
> -
> -#ifdef CONFIG_DRM_FBDEV_EMULATION
> -int rockchip_drm_fbdev_init(struct drm_device *dev);
> -void rockchip_drm_fbdev_fini(struct drm_device *dev);
> -#else
> -static inline int rockchip_drm_fbdev_init(struct drm_device *dev)
> -{
> -	return 0;
> -}
> -
> -static inline void rockchip_drm_fbdev_fini(struct drm_device *dev)
> -{
> -}
> -#endif
> -
> -#endif /* _ROCKCHIP_DRM_FBDEV_H */

Re: [BUG] [PATCH] drm/rockchip: use generic fbdev setup

[John Keeping]

Hi Johan,

On Mon, Oct 17, 2022 at 10:11:32AM +0200, Johan Jonker wrote:
> Your patch contribution causes a kernel panic on MK808 with Rockchip rk3066a SoC.
> Would you like to contribute to fix this issue?
> The assumtion that drm_fbdev_generic_setup() does what rockchip_drm_fbdev_init did is not true!
> A revert makes it work again.

It looks like there are 3 different ways to end up with -ENOMEM here,
can you track down whether you're hitting one of the cases in
rockchip_gem_prime_vmap() or if it's the iosys_map_is_null case in
drm_gem_vmap()?

I guess the memory usage increases slightly using the generic code and
RK3066 has less memory available.

> ======
> 
> [    7.975906] ------------[ cut here ]------------
> [    7.975929] WARNING: CPU: 0 PID: 35 at drivers/gpu/drm/drm_fb_helper.c:471 drm_fb_helper_damage_work+0x138/0x3b4
> [    7.976044] rockchip-drm display-subsystem: Damage blitter failed: ret=-12
> [    7.976064] Modules linked in:
> [    7.976090] CPU: 0 PID: 35 Comm: kworker/0:4 Not tainted 6.0.0-next-20221013 #1
> [    7.976126] Hardware name: Rockchip (Device Tree)
> [    7.976145] Workqueue: events drm_fb_helper_damage_work
> [    7.976196] Backtrace: 
> [    7.976214]  dump_backtrace from show_stack+0x20/0x24
> [    7.976276]  r7:000001d7 r6:00000009 r5:c0b2bc78 r4:60000013
> [    7.976289]  show_stack from dump_stack_lvl+0x48/0x54
> [    7.976357]  dump_stack_lvl from dump_stack+0x18/0x1c
> [    7.976426]  r5:c0586054 r4:c0b63750
> [    7.976436]  dump_stack from __warn+0xdc/0x154
> [    7.976525]  __warn from warn_slowpath_fmt+0xa4/0xd8
> [    7.976588]  r7:000001d7 r6:c0b63750 r5:c1004ec8 r4:c0b639ec
> [    7.976598]  warn_slowpath_fmt from drm_fb_helper_damage_work+0x138/0x3b4
> [    7.976670]  r9:ef7cf105 r8:c15dfc00 r7:fffffff4 r6:c200a590 r5:c1004ec8 r4:c200a594
> [    7.976681]  drm_fb_helper_damage_work from process_one_work+0x230/0x518
> [    7.976761]  r10:c110d140 r9:ef7cf105 r8:00000000 r7:ef7cf100 r6:ef7cbf00 r5:c200f300
> [    7.976775]  r4:c200a594
> [    7.976785]  process_one_work from worker_thread+0x54/0x554
> [    7.976841]  r10:ef7cbf00 r9:00000008 r8:c1003d40 r7:ef7cbf1c r6:c200f318 r5:ef7cbf00
> [    7.976855]  r4:c200f300
> [    7.976864]  worker_thread from kthread+0xe8/0x104
> [    7.976948]  r10:f0929e84 r9:c200ea40 r8:c169aa80 r7:c200f300 r6:c01419e4 r5:00000000
> [    7.976962]  r4:c200e800
> [    7.976971]  kthread from ret_from_fork+0x14/0x2c
> [    7.977026] Exception stack(0xf092dfb0 to 0xf092dff8)
> [    7.977052] dfa0:                                     00000000 00000000 00000000 00000000
> [    7.977078] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    7.977100] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    7.977128]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c01491a8
> [    7.977144]  r4:c200e800 r3:00000001
> [    7.977155] ---[ end trace 0000000000000000 ]---
> 
> On 10/29/21 13:50, John Keeping wrote:
> > The Rockchip fbdev code does not add anything compared to
> > drm_fbdev_generic_setup(); the one custom function for .fb_mmap does the
> > same thing as gem_prime_mmap which is called by the helper.
> > 
> > Signed-off-by: John Keeping <john@metanate.com>
> > ---
> >  drivers/gpu/drm/rockchip/Makefile             |   1 -
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  10 +-
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |   2 -
> >  drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 164 ------------------
> >  drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h |  24 ---
> >  5 files changed, 2 insertions(+), 199 deletions(-)
> >  delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
> >  delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> > 
> > diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> > index 17a9e7eb2130..1a56f696558c 100644
> > --- a/drivers/gpu/drm/rockchip/Makefile
> > +++ b/drivers/gpu/drm/rockchip/Makefile
> > @@ -5,7 +5,6 @@
> >  
> >  rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> >  		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
> > -rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
> >  
> >  rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> >  rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > index 69c699459dce..20d81ae69828 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > @@ -26,7 +26,6 @@
> >  
> >  #include "rockchip_drm_drv.h"
> >  #include "rockchip_drm_fb.h"
> > -#include "rockchip_drm_fbdev.h"
> >  #include "rockchip_drm_gem.h"
> >  
> >  #define DRIVER_NAME	"rockchip"
> > @@ -159,10 +158,6 @@ static int rockchip_drm_bind(struct device *dev)
> >  
> >  	drm_mode_config_reset(drm_dev);
> >  
> > -	ret = rockchip_drm_fbdev_init(drm_dev);
> > -	if (ret)
> > -		goto err_unbind_all;
> > -
> >  	/* init kms poll for handling hpd */
> >  	drm_kms_helper_poll_init(drm_dev);
> >  
> > @@ -170,10 +165,11 @@ static int rockchip_drm_bind(struct device *dev)
> >  	if (ret)
> >  		goto err_kms_helper_poll_fini;
> >  
> > +	drm_fbdev_generic_setup(drm_dev, 32);
> > +
> >  	return 0;
> >  err_kms_helper_poll_fini:
> >  	drm_kms_helper_poll_fini(drm_dev);
> > -	rockchip_drm_fbdev_fini(drm_dev);
> >  err_unbind_all:
> >  	component_unbind_all(dev, drm_dev);
> >  err_iommu_cleanup:
> > @@ -189,7 +185,6 @@ static void rockchip_drm_unbind(struct device *dev)
> >  
> >  	drm_dev_unregister(drm_dev);
> >  
> > -	rockchip_drm_fbdev_fini(drm_dev);
> >  	drm_kms_helper_poll_fini(drm_dev);
> >  
> >  	drm_atomic_helper_shutdown(drm_dev);
> > @@ -203,7 +198,6 @@ DEFINE_DRM_GEM_FOPS(rockchip_drm_driver_fops);
> >  
> >  static const struct drm_driver rockchip_drm_driver = {
> >  	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
> > -	.lastclose		= drm_fb_helper_lastclose,
> >  	.dumb_create		= rockchip_gem_dumb_create,
> >  	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
> >  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> > index aa0909e8edf9..143a48330f84 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> > @@ -43,8 +43,6 @@ struct rockchip_crtc_state {
> >   * @mm_lock: protect drm_mm on multi-threads.
> >   */
> >  struct rockchip_drm_private {
> > -	struct drm_fb_helper fbdev_helper;
> > -	struct drm_gem_object *fbdev_bo;
> >  	struct iommu_domain *domain;
> >  	struct mutex mm_lock;
> >  	struct drm_mm mm;
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
> > deleted file mode 100644
> > index d8418dd39d0e..000000000000
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
> > +++ /dev/null
> > @@ -1,164 +0,0 @@
> > -// SPDX-License-Identifier: GPL-2.0-only
> > -/*
> > - * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> > - * Author:Mark Yao <mark.yao@rock-chips.com>
> > - */
> > -
> > -#include <drm/drm.h>
> > -#include <drm/drm_fb_helper.h>
> > -#include <drm/drm_fourcc.h>
> > -#include <drm/drm_prime.h>
> > -#include <drm/drm_probe_helper.h>
> > -
> > -#include "rockchip_drm_drv.h"
> > -#include "rockchip_drm_gem.h"
> > -#include "rockchip_drm_fb.h"
> > -#include "rockchip_drm_fbdev.h"
> > -
> > -#define PREFERRED_BPP		32
> > -#define to_drm_private(x) \
> > -		container_of(x, struct rockchip_drm_private, fbdev_helper)
> > -
> > -static int rockchip_fbdev_mmap(struct fb_info *info,
> > -			       struct vm_area_struct *vma)
> > -{
> > -	struct drm_fb_helper *helper = info->par;
> > -	struct rockchip_drm_private *private = to_drm_private(helper);
> > -
> > -	return drm_gem_prime_mmap(private->fbdev_bo, vma);
> > -}
> > -
> > -static const struct fb_ops rockchip_drm_fbdev_ops = {
> > -	.owner		= THIS_MODULE,
> > -	DRM_FB_HELPER_DEFAULT_OPS,
> > -	.fb_mmap	= rockchip_fbdev_mmap,
> > -	.fb_fillrect	= drm_fb_helper_cfb_fillrect,
> > -	.fb_copyarea	= drm_fb_helper_cfb_copyarea,
> > -	.fb_imageblit	= drm_fb_helper_cfb_imageblit,
> > -};
> > -
> > -static int rockchip_drm_fbdev_create(struct drm_fb_helper *helper,
> > -				     struct drm_fb_helper_surface_size *sizes)
> > -{
> > -	struct rockchip_drm_private *private = to_drm_private(helper);
> > -	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> > -	struct drm_device *dev = helper->dev;
> > -	struct rockchip_gem_object *rk_obj;
> > -	struct drm_framebuffer *fb;
> > -	unsigned int bytes_per_pixel;
> > -	unsigned long offset;
> > -	struct fb_info *fbi;
> > -	size_t size;
> > -	int ret;
> > -
> > -	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] = sizes->surface_width * bytes_per_pixel;
> > -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> > -		sizes->surface_depth);
> > -
> > -	size = mode_cmd.pitches[0] * mode_cmd.height;
> > -
> > -	rk_obj = rockchip_gem_create_object(dev, size, true);
> > -	if (IS_ERR(rk_obj))
> > -		return -ENOMEM;
> > -
> > -	private->fbdev_bo = &rk_obj->base;
> > -
> > -	fbi = drm_fb_helper_alloc_fbi(helper);
> > -	if (IS_ERR(fbi)) {
> > -		DRM_DEV_ERROR(dev->dev, "Failed to create framebuffer info.\n");
> > -		ret = PTR_ERR(fbi);
> > -		goto out;
> > -	}
> > -
> > -	helper->fb = rockchip_drm_framebuffer_init(dev, &mode_cmd,
> > -						   private->fbdev_bo);
> > -	if (IS_ERR(helper->fb)) {
> > -		DRM_DEV_ERROR(dev->dev,
> > -			      "Failed to allocate DRM framebuffer.\n");
> > -		ret = PTR_ERR(helper->fb);
> > -		goto out;
> > -	}
> > -
> > -	fbi->fbops = &rockchip_drm_fbdev_ops;
> > -
> > -	fb = helper->fb;
> > -	drm_fb_helper_fill_info(fbi, helper, sizes);
> > -
> > -	offset = fbi->var.xoffset * bytes_per_pixel;
> > -	offset += fbi->var.yoffset * fb->pitches[0];
> > -
> > -	dev->mode_config.fb_base = 0;
> > -	fbi->screen_base = rk_obj->kvaddr + offset;
> > -	fbi->screen_size = rk_obj->base.size;
> > -	fbi->fix.smem_len = rk_obj->base.size;
> > -
> > -	DRM_DEBUG_KMS("FB [%dx%d]-%d kvaddr=%p offset=%ld size=%zu\n",
> > -		      fb->width, fb->height, fb->format->depth,
> > -		      rk_obj->kvaddr,
> > -		      offset, size);
> > -
> > -	return 0;
> > -
> > -out:
> > -	rockchip_gem_free_object(&rk_obj->base);
> > -	return ret;
> > -}
> > -
> > -static const struct drm_fb_helper_funcs rockchip_drm_fb_helper_funcs = {
> > -	.fb_probe = rockchip_drm_fbdev_create,
> > -};
> > -
> > -int rockchip_drm_fbdev_init(struct drm_device *dev)
> > -{
> > -	struct rockchip_drm_private *private = dev->dev_private;
> > -	struct drm_fb_helper *helper;
> > -	int ret;
> > -
> > -	if (!dev->mode_config.num_crtc || !dev->mode_config.num_connector)
> > -		return -EINVAL;
> > -
> > -	helper = &private->fbdev_helper;
> > -
> > -	drm_fb_helper_prepare(dev, helper, &rockchip_drm_fb_helper_funcs);
> > -
> > -	ret = drm_fb_helper_init(dev, helper);
> > -	if (ret < 0) {
> > -		DRM_DEV_ERROR(dev->dev,
> > -			      "Failed to initialize drm fb helper - %d.\n",
> > -			      ret);
> > -		return ret;
> > -	}
> > -
> > -	ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
> > -	if (ret < 0) {
> > -		DRM_DEV_ERROR(dev->dev,
> > -			      "Failed to set initial hw config - %d.\n",
> > -			      ret);
> > -		goto err_drm_fb_helper_fini;
> > -	}
> > -
> > -	return 0;
> > -
> > -err_drm_fb_helper_fini:
> > -	drm_fb_helper_fini(helper);
> > -	return ret;
> > -}
> > -
> > -void rockchip_drm_fbdev_fini(struct drm_device *dev)
> > -{
> > -	struct rockchip_drm_private *private = dev->dev_private;
> > -	struct drm_fb_helper *helper;
> > -
> > -	helper = &private->fbdev_helper;
> > -
> > -	drm_fb_helper_unregister_fbi(helper);
> > -
> > -	if (helper->fb)
> > -		drm_framebuffer_put(helper->fb);
> > -
> > -	drm_fb_helper_fini(helper);
> > -}
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> > deleted file mode 100644
> > index 5fb7ac2371a8..000000000000
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
> > +++ /dev/null
> > @@ -1,24 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-only */
> > -/*
> > - * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> > - * Author:Mark Yao <mark.yao@rock-chips.com>
> > - */
> > -
> > -#ifndef _ROCKCHIP_DRM_FBDEV_H
> > -#define _ROCKCHIP_DRM_FBDEV_H
> > -
> > -#ifdef CONFIG_DRM_FBDEV_EMULATION
> > -int rockchip_drm_fbdev_init(struct drm_device *dev);
> > -void rockchip_drm_fbdev_fini(struct drm_device *dev);
> > -#else
> > -static inline int rockchip_drm_fbdev_init(struct drm_device *dev)
> > -{
> > -	return 0;
> > -}
> > -
> > -static inline void rockchip_drm_fbdev_fini(struct drm_device *dev)
> > -{
> > -}
> > -#endif
> > -
> > -#endif /* _ROCKCHIP_DRM_FBDEV_H */
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [BUG] [PATCH] drm/rockchip: use generic fbdev setup

[Heiko Stuebner]

Am Montag, 17. Oktober 2022, 12:05:16 CEST schrieb John Keeping:
> Hi Johan,
> 
> On Mon, Oct 17, 2022 at 10:11:32AM +0200, Johan Jonker wrote:
> > Your patch contribution causes a kernel panic on MK808 with Rockchip rk3066a SoC.
> > Would you like to contribute to fix this issue?
> > The assumtion that drm_fbdev_generic_setup() does what rockchip_drm_fbdev_init did is not true!
> > A revert makes it work again.
> 
> It looks like there are 3 different ways to end up with -ENOMEM here,
> can you track down whether you're hitting one of the cases in
> rockchip_gem_prime_vmap() or if it's the iosys_map_is_null case in
> drm_gem_vmap()?
> 
> I guess the memory usage increases slightly using the generic code and
> RK3066 has less memory available.

also rk3066 and rk3188 do not have an iommu, so rely
on cma allocations.


Heiko

Re: [BUG] [PATCH] drm/rockchip: use generic fbdev setup

[Johan Jonker]

On 10/17/22 13:29, Heiko Stuebner wrote:
> Am Montag, 17. Oktober 2022, 12:05:16 CEST schrieb John Keeping:
>> Hi Johan,
>>
>> On Mon, Oct 17, 2022 at 10:11:32AM +0200, Johan Jonker wrote:
>>> Your patch contribution causes a kernel panic on MK808 with Rockchip rk3066a SoC.
>>> Would you like to contribute to fix this issue?
>>> The assumtion that drm_fbdev_generic_setup() does what rockchip_drm_fbdev_init did is not true!
>>> A revert makes it work again.
>>

>> It looks like there are 3 different ways to end up with -ENOMEM here,
>> can you track down whether you're hitting one of the cases in
>> rockchip_gem_prime_vmap() or if it's the iosys_map_is_null case in
>> drm_gem_vmap()?

It looks like it comes from rockchip_gem_prime_vmap() second return (2).

====



int rockchip_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map)
{
	struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);

	if (rk_obj->pages) {
		void *vaddr = vmap(rk_obj->pages, rk_obj->num_pages, VM_MAP,
				  pgprot_writecombine(PAGE_KERNEL));
		if (!vaddr) {
			printk("FBDEV rockchip_gem_prime_vmap 1");
			return -ENOMEM;
		}
		iosys_map_set_vaddr(map, vaddr);
		return 0;
	}

	if (rk_obj->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING) {

////////////////

		printk("FBDEV rockchip_gem_prime_vmap 2");

////////////////
		return -ENOMEM;
	}
	iosys_map_set_vaddr(map, rk_obj->kvaddr);

	return 0;
}

====

[    7.678392] [drm:drm_client_modeset_probe] connector 39 enabled? yes
[    7.678435] [drm:drm_client_modeset_probe] Not using firmware configuration
[    7.678465] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 39
[    7.678494] [drm:drm_client_modeset_probe] looking for preferred mode on connector 39 0
[    7.678521] [drm:drm_client_modeset_probe] found mode 1920x1080
[    7.678545] [drm:drm_client_modeset_probe] picking CRTCs for 1920x1080 config
[    7.678585] [drm:drm_client_modeset_probe] desired mode 1920x1080 set on crtc 35 (0,0)
[    7.801673] Console: switching to colour frame buffer device 240x67


[    7.811047] FBDEV rockchip_gem_prime_vmap 2


[    7.811071] ------------[ cut here ]------------
[    7.811084] WARNING: CPU: 0 PID: 35 at drivers/gpu/drm/drm_fb_helper.c:471 drm_fb_helper_damage_work+0x138/0x3b4
[    7.811198] rockchip-drm display-subsystem: Damage blitter failed: ret=-12
[    7.811219] Modules linked in:
[    7.811244] CPU: 0 PID: 35 Comm: kworker/0:4 Not tainted 6.0.0-next-20221013+ #46
[    7.811281] Hardware name: Rockchip (Device Tree)
[    7.811300] Workqueue: events drm_fb_helper_damage_work
[    7.811352] Backtrace: 
[    7.811370]  dump_backtrace from show_stack+0x20/0x24
[    7.811431]  r7:000001d7 r6:00000009 r5:c0b2bc60 r4:60000013
[    7.811444]  show_stack from dump_stack_lvl+0x48/0x54
[    7.811512]  dump_stack_lvl from dump_stack+0x18/0x1c
[    7.811580]  r5:c0586064 r4:c0b6374c
[    7.811590]  dump_stack from __warn+0xdc/0x154
[    7.811677]  __warn from warn_slowpath_fmt+0xa4/0xd8
[    7.811740]  r7:000001d7 r6:c0b6374c r5:c1004ec8 r4:c0b639e8
[    7.811750]  warn_slowpath_fmt from drm_fb_helper_damage_work+0x138/0x3b4
[    7.811821]  r9:ef7cf105 r8:c15dfc00 r7:fffffff4 r6:c200b490 r5:c1004ec8 r4:c200b494
[    7.811833]  drm_fb_helper_damage_work from process_one_work+0x230/0x518
[    7.811912]  r10:c110d140 r9:ef7cf105 r8:00000000 r7:ef7cf100 r6:ef7cbf00 r5:c200e300
[    7.811927]  r4:c200b494
[    7.811936]  process_one_work from worker_thread+0x54/0x554
[    7.811991]  r10:ef7cbf00 r9:00000008 r8:c1003d40 r7:ef7cbf1c r6:c200e318 r5:ef7cbf00
[    7.812006]  r4:c200e300
[    7.812015]  worker_thread from kthread+0xe8/0x104
[    7.812100]  r10:f0929e84 r9:c200da00 r8:c169aa80 r7:c200e300 r6:c01419e4 r5:00000000
[    7.812114]  r4:c200d780
[    7.812124]  kthread from ret_from_fork+0x14/0x2c
[    7.812178] Exception stack(0xf092dfb0 to 0xf092dff8)
[    7.812205] dfa0:                                     00000000 00000000 00000000 00000000
[    7.812232] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    7.812255] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    7.812282]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c01491a8
[    7.812299]  r4:c200d780 r3:00000001
[    7.812309] ---[ end trace 0000000000000000 ]---
[    7.812336] FBDEV rockchip_gem_prime_vmap 2
[    7.889795] FBDEV rockchip_gem_prime_vmap 2
[    7.890418] FBDEV rockchip_gem_prime_vmap 2
[    7.899447] FBDEV rockchip_gem_prime_vmap 2
[    7.905252] FBDEV rockchip_gem_prime_vmap 2

>>
>> I guess the memory usage increases slightly using the generic code and
>> RK3066 has less memory available.
> 
> also rk3066 and rk3188 do not have an iommu, so rely
> on cma allocations.
> 
> 
> Heiko
> 
> 

Re: [BUG] [PATCH] drm/rockchip: use generic fbdev setup

[John Keeping]

On Mon, Oct 17, 2022 at 08:30:23PM +0200, Johan Jonker wrote:
> 
> 
> On 10/17/22 13:29, Heiko Stuebner wrote:
> > Am Montag, 17. Oktober 2022, 12:05:16 CEST schrieb John Keeping:
> >> Hi Johan,
> >>
> >> On Mon, Oct 17, 2022 at 10:11:32AM +0200, Johan Jonker wrote:
> >>> Your patch contribution causes a kernel panic on MK808 with Rockchip rk3066a SoC.
> >>> Would you like to contribute to fix this issue?
> >>> The assumtion that drm_fbdev_generic_setup() does what rockchip_drm_fbdev_init did is not true!
> >>> A revert makes it work again.
> >>
> 
> >> It looks like there are 3 different ways to end up with -ENOMEM here,
> >> can you track down whether you're hitting one of the cases in
> >> rockchip_gem_prime_vmap() or if it's the iosys_map_is_null case in
> >> drm_gem_vmap()?
> 
> It looks like it comes from rockchip_gem_prime_vmap() second return (2).
> 
> 
> 	if (rk_obj->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING) {
> 
> ////////////////
> 
> 		printk("FBDEV rockchip_gem_prime_vmap 2");
> 
> ////////////////
> 		return -ENOMEM;
> 	}

Ah-ha, Heiko was right that this is because the no-iommu path is broken
as a result of switching to the generic fbdev code.

This patch should fix it, but I wonder if Thomas has any ideas about a
better way to handle this since it feels a bit hacky to special-case the
fb_helper inside the GEM code:

-- >8 --
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index 614e97aaac80..da8a69953706 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -364,9 +364,12 @@ rockchip_gem_create_with_handle(struct drm_file *file_priv,
 {
 	struct rockchip_gem_object *rk_obj;
 	struct drm_gem_object *obj;
+	bool is_framebuffer;
 	int ret;
 
-	rk_obj = rockchip_gem_create_object(drm, size, false);
+	is_framebuffer = drm->fb_helper && file_priv == drm->fb_helper->client.file;
+
+	rk_obj = rockchip_gem_create_object(drm, size, is_framebuffer);
 	if (IS_ERR(rk_obj))
 		return ERR_CAST(rk_obj);
-- 8< -- 

Re: [BUG] [PATCH] drm/rockchip: use generic fbdev setup

[Johan Jonker]

On 10/17/22 21:00, John Keeping wrote:
> On Mon, Oct 17, 2022 at 08:30:23PM +0200, Johan Jonker wrote:
>>
>>
>> On 10/17/22 13:29, Heiko Stuebner wrote:
>>> Am Montag, 17. Oktober 2022, 12:05:16 CEST schrieb John Keeping:
>>>> Hi Johan,
>>>>
>>>> On Mon, Oct 17, 2022 at 10:11:32AM +0200, Johan Jonker wrote:
>>>>> Your patch contribution causes a kernel panic on MK808 with Rockchip rk3066a SoC.
>>>>> Would you like to contribute to fix this issue?
>>>>> The assumtion that drm_fbdev_generic_setup() does what rockchip_drm_fbdev_init did is not true!
>>>>> A revert makes it work again.
>>>>
>>
>>>> It looks like there are 3 different ways to end up with -ENOMEM here,
>>>> can you track down whether you're hitting one of the cases in
>>>> rockchip_gem_prime_vmap() or if it's the iosys_map_is_null case in
>>>> drm_gem_vmap()?
>>
>> It looks like it comes from rockchip_gem_prime_vmap() second return (2).
>>
>>
>> 	if (rk_obj->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING) {
>>
>> ////////////////
>>
>> 		printk("FBDEV rockchip_gem_prime_vmap 2");
>>
>> ////////////////
>> 		return -ENOMEM;
>> 	}
> 
> Ah-ha, Heiko was right that this is because the no-iommu path is broken
> as a result of switching to the generic fbdev code.
> 
> This patch should fix it, but I wonder if Thomas has any ideas about a
> better way to handle this since it feels a bit hacky to special-case the
> fb_helper inside the GEM code:

The penguin is back on screen. Thanks!

> 
> -- >8 --
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index 614e97aaac80..da8a69953706 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -364,9 +364,12 @@ rockchip_gem_create_with_handle(struct drm_file *file_priv,
>  {
>  	struct rockchip_gem_object *rk_obj;
>  	struct drm_gem_object *obj;
> +	bool is_framebuffer;
>  	int ret;
>  
> -	rk_obj = rockchip_gem_create_object(drm, size, false);
> +	is_framebuffer = drm->fb_helper && file_priv == drm->fb_helper->client.file;
> +
> +	rk_obj = rockchip_gem_create_object(drm, size, is_framebuffer);
>  	if (IS_ERR(rk_obj))
>  		return ERR_CAST(rk_obj);
> -- 8< --