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,
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, };
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_... 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
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_... 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]); ^~ mbdrivers/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
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 ------------------------
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
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
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
(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#... [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/mb8...
-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
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,
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,
dri-devel@lists.freedesktop.org
-
Andrzej Pietrasiewicz -
Daniel Vetter -
Daniel Vetter -
kbuild test robot -
Thomas Zimmermann