drm/rockchip: Use drm_fbdev_generic_setup()

Other than using a rockchip_gem_object directly, the Rockchip fbdev setup has nothing special and can be converted to use the generic fbdev emulation instead.

This patch makes full use of the generic fbdev emulation by using its drm_client callbacks. This means that drm_mode_config_funcs->output_poll_changed and drm_driver->lastclose are now handled by the emulation code. Additionally fbdev unregister happens automatically on drm_dev_unregister().

The drm_fbdev_generic_setup() call is put after drm_dev_register() in the driver. This is done to highlight the fact that fbdev emulation is an internal client that makes use of the driver, it is not part of the driver as such. If fbdev setup fails, an error is printed, but the driver succeeds probing.

Cc Noralf Trønnes <[email protected]>

Cc Sandy Huang <[email protected]>

Cc "Heiko Stübner" <[email protected]>

Cc David Airlie <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Rob Herring <[email protected]>

---
 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_fb.c    |  14 --
 drivers/gpu/drm/rockchip/rockchip_drm_fb.h    |   6 -
 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 183 ------------------
 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h |  32 ---
 7 files changed, 2 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
 delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h

drivers/gpu/drm/rockchip/Makefile +0 -1

index f6fc9d5dd0ad..dff1d0962ff7 100644
@@ -6,7 +6,6 @@
 rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
 		rockchip_drm_gem.o rockchip_drm_psr.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

drivers/gpu/drm/rockchip/rockchip_drm_drv.c +2 -8

index be6c2573039a..7089f04970ab 100644
@@ -31,7 +31,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"

@@ -162,10 +161,6 @@ static int rockchip_drm_bind(struct device *dev)
 	 */
 	drm_dev->irq_enabled = true;
 
-	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);
 

@@ -173,10 +168,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_mode_config_cleanup:

@@ -195,7 +191,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);

@@ -222,7 +217,6 @@ static const struct file_operations rockchip_drm_driver_fops = {
 static struct drm_driver rockchip_drm_driver = {
 	.driver_features	= DRIVER_MODESET | DRIVER_GEM |
 				  DRIVER_PRIME | DRIVER_ATOMIC,
-	.lastclose		= drm_fb_helper_lastclose,
 	.gem_vm_ops		= &drm_gem_cma_vm_ops,
 	.gem_free_object_unlocked = rockchip_gem_free_object,
 	.dumb_create		= rockchip_gem_dumb_create,

drivers/gpu/drm/rockchip/rockchip_drm_drv.h +0 -2

index ce48568ec8a0..7f4ebd682ad2 100644
@@ -50,8 +50,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;

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +0 -14

index ea18cb2a76c0..5a008efa1e7d 100644
@@ -197,20 +197,6 @@ static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
-struct drm_framebuffer *
-rockchip_drm_framebuffer_init(struct drm_device *dev,
-			      const struct drm_mode_fb_cmd2 *mode_cmd,
-			      struct drm_gem_object *obj)
-{
-	struct drm_framebuffer *fb;
-
-	fb = rockchip_fb_alloc(dev, mode_cmd, &obj, 1);
-	if (IS_ERR(fb))
-		return ERR_CAST(fb);
-
-	return fb;
-}
-
 void rockchip_drm_mode_config_init(struct drm_device *dev)
 {
 	dev->mode_config.min_width = 0;

drivers/gpu/drm/rockchip/rockchip_drm_fb.h +0 -6

index f1265cb1aee8..69271df96fa6 100644
@@ -15,11 +15,5 @@
 #ifndef _ROCKCHIP_DRM_FB_H
 #define _ROCKCHIP_DRM_FB_H
 
-struct drm_framebuffer *
-rockchip_drm_framebuffer_init(struct drm_device *dev,
-			      const struct drm_mode_fb_cmd2 *mode_cmd,
-			      struct drm_gem_object *obj);
-void rockchip_drm_framebuffer_fini(struct drm_framebuffer *fb);
-
 void rockchip_drm_mode_config_init(struct drm_device *dev);
 #endif /* _ROCKCHIP_DRM_FB_H */

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c deleted +0 -61

deleted file mode 100644
index e6650553f5d6..000000000000
@@ -1,183 +0,0 @@
-/*
- * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
- * Author:Mark Yao <[email protected]>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <drm/drm.h>
-#include <drm/drmP.h>
-#include <drm/drm_fb_helper.h>
-#include <drm/drm_crtc_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 rockchip_gem_mmap_buf(private->fbdev_bo, vma);
-}
-
-static 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);

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c deleted +0 -112

-	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->par = helper;
-	fbi->flags = FBINFO_FLAG_DEFAULT;
-	fbi->fbops = &rockchip_drm_fbdev_ops;
-
-	fb = helper->fb;
-	drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
-	drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, sizes->fb_height);
-
-	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);
-
-	fbi->skip_vt_switch = true;
-
-	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, ROCKCHIP_MAX_CONNECTOR);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev->dev,
-			      "Failed to initialize drm fb helper - %d.\n",
-			      ret);
-		return ret;
-	}
-
-	ret = drm_fb_helper_single_add_all_connectors(helper);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev->dev,
-			      "Failed to add connectors - %d.\n", ret);
-		goto err_drm_fb_helper_fini;
-	}
-
-	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);
-}

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h deleted +0 -22

deleted file mode 100644
index 73718c5f5bbf..000000000000
@@ -1,32 +0,0 @@
-/*
- * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
- * Author:Mark Yao <[email protected]>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#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)

Den 02.02.2019 18.07, skrev Rob Herring:

Quoted 3 lines

Other than using a rockchip_gem_object directly, the Rockchip fbdev setup has nothing special and can be converted to use the generic fbdev emulation instead.

Quoted 21 lines

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c deleted +0 -21

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

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c line 71

Quoted 4 lines

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c deleted +0 -4

-
-     size = mode_cmd.pitches[0] * mode_cmd.height;
-
-     rk_obj = rockchip_gem_create_object(dev, size, true);

I don't think the generic emulation in it's current form will work for rockchip. rockchip treats fbdev buffers and dumb buffers differently. If it uses DMA buffers, then the dumb buffer can't get a virtual address.

From rockchip_gem_alloc_dma():

        if (!alloc_kmap)
                rk_obj->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;

From rockchip_gem_prime_vmap():

        if (rk_obj->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING)
                return NULL;

Maybe it's possible to vmap a buffer created using dma_alloc_attrs() after the allocation?

Quoted 4 lines

Den 02.02.2019 18.07, skrev Rob Herring:

Quoted 3 lines

Other than using a rockchip_gem_object directly, the Rockchip fbdev setup has nothing special and can be converted to use the generic fbdev emulation instead.

[...]

Quoted 21 lines

Quoted 21 lines

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c deleted +0 -21

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

Quoted 8 lines

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c line 71

Quoted 4 lines

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c deleted +0 -4

-
-     size = mode_cmd.pitches[0] * mode_cmd.height;
-
-     rk_obj = rockchip_gem_create_object(dev, size, true);

I don't think the generic emulation in it's current form will work for rockchip. rockchip treats fbdev buffers and dumb buffers differently. If it uses DMA buffers, then the dumb buffer can't get a virtual address.

Yes, you are right. I tested the iommu case.

Quoted 5 lines

From rockchip_gem_alloc_dma():

        if (!alloc_kmap)
                rk_obj->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;

Quoted 7 lines

From rockchip_gem_prime_vmap():

        if (rk_obj->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING)
                return NULL;

Maybe it's possible to vmap a buffer created using dma_alloc_attrs() after the allocation?

It should be possible I think as that is what drm_gem_cma_prime_import_sg_table_vmap() does. One problem though is you may already have a mapping because DMA_ATTR_NO_KERNEL_MAPPING is just a suggestion (only 32-bit ARM implements) and there's not a way to tell if you got a mapping or not. Maybe it's not all that important if there are 2 mappings because I think only fbcon needs a kernel mapping.

My follow-up to this was going to be converting Rockchip to use CMA and the shmem helpers. So I was already wondering about what to do with DMA_ATTR_NO_KERNEL_MAPPING. There's several drivers not using CMA just to set DMA_ATTR_NO_KERNEL_MAPPING. So it would be good to come up with a solution here.

Quoted 10 lines

The problem here is that this driver doesn't map a virtual address for its buffers:

	mtk_gem->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;

Probably because of limited virtual address space. rockchip is in the same situation.

I'm aware of this shortcoming of the generic emulation, but I don't see how it can be solved without using somekind of flag attached to the dumb buffer creation request.

The virtual address space is limited so we do not map it by default. I also see the shortcoming of the generic emulation, so I would refer to rockchip to implement this mapping.

Regards,
CK

Quoted 4 lines · Noralf Trønnes

Den 02.02.2019 18.07, skrev Rob Herring:

Quoted 3 lines

Other than using a rockchip_gem_object directly, the Rockchip fbdev setup has nothing special and can be converted to use the generic fbdev emulation instead.

[...]

Quoted 21 lines

Quoted 21 lines

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c deleted +0 -21

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

Quoted 8 lines

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c line 71

Quoted 4 lines

drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c deleted +0 -4

-
-     size = mode_cmd.pitches[0] * mode_cmd.height;
-
-     rk_obj = rockchip_gem_create_object(dev, size, true);

I don't think the generic emulation in it's current form will work for rockchip. rockchip treats fbdev buffers and dumb buffers differently. If it uses DMA buffers, then the dumb buffer can't get a virtual address.

Yes, you are right. I tested the iommu case.

Quoted 5 lines

From rockchip_gem_alloc_dma():

        if (!alloc_kmap)
                rk_obj->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;

Quoted 7 lines

From rockchip_gem_prime_vmap():

        if (rk_obj->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING)
                return NULL;

Maybe it's possible to vmap a buffer created using dma_alloc_attrs() after the allocation?

It should be possible I think as that is what drm_gem_cma_prime_import_sg_table_vmap() does. One problem though is you may already have a mapping because DMA_ATTR_NO_KERNEL_MAPPING is just a suggestion (only 32-bit ARM implements) and there's not a way to tell if you got a mapping or not. Maybe it's not all that important if there are 2 mappings because I think only fbcon needs a kernel mapping.