| Message ID | 20191024144237.8898-6-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/udl: Convert to generic fbdev emulation | expand |
On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote: > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/udl/udl_drv.c | 3 + > drivers/gpu/drm/udl/udl_drv.h | 4 - > drivers/gpu/drm/udl/udl_fb.c | 263 +----------------------------- > drivers/gpu/drm/udl/udl_main.c | 2 - > drivers/gpu/drm/udl/udl_modeset.c | 3 +- > 5 files changed, 8 insertions(+), 267 deletions(-) > > diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c > index 15ad7a338f9d..6beaa1109c2c 100644 > --- a/drivers/gpu/drm/udl/udl_drv.c > +++ b/drivers/gpu/drm/udl/udl_drv.c > @@ -7,6 +7,7 @@ > > #include <drm/drm_crtc_helper.h> > #include <drm/drm_drv.h> > +#include <drm/drm_fb_helper.h> > #include <drm/drm_file.h> > #include <drm/drm_ioctl.h> > #include <drm/drm_probe_helper.h> > @@ -62,6 +63,8 @@ static struct drm_driver driver = { > .driver_features = DRIVER_MODESET | DRIVER_GEM, > .release = udl_driver_release, > > + .lastclose = drm_fb_helper_lastclose, > + > /* gem hooks */ > .gem_free_object_unlocked = udl_gem_free_object, > .gem_vm_ops = &udl_gem_vm_ops, > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h > index 12a970fd9a87..5f8a7ac084f6 100644 > --- a/drivers/gpu/drm/udl/udl_drv.h > +++ b/drivers/gpu/drm/udl/udl_drv.h > @@ -50,8 +50,6 @@ struct urb_list { > size_t size; > }; > > -struct udl_fbdev; > - > struct udl_device { > struct drm_device drm; > struct device *dev; > @@ -65,7 +63,6 @@ struct udl_device { > struct urb_list urbs; > atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */ > > - struct udl_fbdev *fbdev; > char mode_buf[1024]; > uint32_t mode_buf_len; > atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */ > @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl); > void udl_fini(struct drm_device *dev); > > int udl_fbdev_init(struct drm_device *dev); > -void udl_fbdev_cleanup(struct drm_device *dev); > void udl_fbdev_unplug(struct drm_device *dev); > struct drm_framebuffer * > udl_fb_user_fb_create(struct drm_device *dev, > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c > index ef3504d06343..43a1da3a56c3 100644 > --- a/drivers/gpu/drm/udl/udl_fb.c > +++ b/drivers/gpu/drm/udl/udl_fb.c > @@ -19,19 +19,9 @@ > > #include "udl_drv.h" > > -#define DL_DEFIO_WRITE_DELAY (HZ/20) /* fb_deferred_io.delay in jiffies */ > - > -static int fb_defio = 0; /* Optionally enable experimental fb_defio mmap support */ > static int fb_bpp = 16; > > module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP); > -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP); > - > -struct udl_fbdev { > - struct drm_fb_helper helper; /* must be first */ > - struct udl_framebuffer ufb; > - int fb_count; > -}; > > #define DL_ALIGN_UP(x, a) ALIGN(x, a) > #define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a) > @@ -157,123 +147,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, > return 0; > } > > -static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > -{ > - unsigned long start = vma->vm_start; > - unsigned long size = vma->vm_end - vma->vm_start; > - unsigned long offset; > - unsigned long page, pos; > - > - if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT)) > - return -EINVAL; > - > - offset = vma->vm_pgoff << PAGE_SHIFT; > - > - if (offset > info->fix.smem_len || size > info->fix.smem_len - offset) > - return -EINVAL; > - > - pos = (unsigned long)info->fix.smem_start + offset; > - > - pr_debug("mmap() framebuffer addr:%lu size:%lu\n", > - pos, size); > - > - /* We don't want the framebuffer to be mapped encrypted */ > - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); > - > - while (size > 0) { > - page = vmalloc_to_pfn((void *)pos); > - if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) > - return -EAGAIN; > - > - start += PAGE_SIZE; > - pos += PAGE_SIZE; > - if (size > PAGE_SIZE) > - size -= PAGE_SIZE; > - else > - size = 0; > - } > - > - /* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */ > - return 0; > -} > - > -/* > - * It's common for several clients to have framebuffer open simultaneously. > - * e.g. both fbcon and X. Makes things interesting. > - * Assumes caller is holding info->lock (for open and release at least) > - */ > -static int udl_fb_open(struct fb_info *info, int user) > -{ > - struct udl_fbdev *ufbdev = info->par; > - struct drm_device *dev = ufbdev->ufb.base.dev; > - struct udl_device *udl = to_udl(dev); > - > - /* If the USB device is gone, we don't accept new opens */ > - if (drm_dev_is_unplugged(&udl->drm)) > - return -ENODEV; > - > - ufbdev->fb_count++; > - > -#ifdef CONFIG_DRM_FBDEV_EMULATION > - if (fb_defio && (info->fbdefio == NULL)) { > - /* enable defio at last moment if not disabled by client */ > - > - struct fb_deferred_io *fbdefio; > - > - fbdefio = kzalloc(sizeof(struct fb_deferred_io), GFP_KERNEL); > - > - if (fbdefio) { > - fbdefio->delay = DL_DEFIO_WRITE_DELAY; > - fbdefio->deferred_io = drm_fb_helper_deferred_io; > - } > - > - info->fbdefio = fbdefio; > - fb_deferred_io_init(info); > - } > -#endif > - > - pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d\n", > - info->node, user, info, ufbdev->fb_count); > - > - return 0; > -} > - > - > -/* > - * Assumes caller is holding info->lock mutex (for open and release at least) > - */ > -static int udl_fb_release(struct fb_info *info, int user) > -{ > - struct udl_fbdev *ufbdev = info->par; > - > - ufbdev->fb_count--; > - > -#ifdef CONFIG_DRM_FBDEV_EMULATION > - if ((ufbdev->fb_count == 0) && (info->fbdefio)) { > - fb_deferred_io_cleanup(info); > - kfree(info->fbdefio); > - info->fbdefio = NULL; > - info->fbops->fb_mmap = udl_fb_mmap; > - } > -#endif > - > - pr_debug("released /dev/fb%d user=%d count=%d\n", > - info->node, user, ufbdev->fb_count); > - > - return 0; > -} > - > -static struct fb_ops udlfb_ops = { > - .owner = THIS_MODULE, > - DRM_FB_HELPER_DEFAULT_OPS, > - .fb_fillrect = drm_fb_helper_sys_fillrect, > - .fb_copyarea = drm_fb_helper_sys_copyarea, > - .fb_imageblit = drm_fb_helper_sys_imageblit, > - .fb_mmap = udl_fb_mmap, > - .fb_open = udl_fb_open, > - .fb_release = udl_fb_release, > -}; > - > static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, > struct drm_file *file, > unsigned flags, unsigned color, > @@ -346,150 +219,20 @@ udl_framebuffer_init(struct drm_device *dev, > return ret; > } > > - > -static int udlfb_create(struct drm_fb_helper *helper, > - struct drm_fb_helper_surface_size *sizes) > -{ > - struct udl_fbdev *ufbdev = > - container_of(helper, struct udl_fbdev, helper); > - struct drm_device *dev = ufbdev->helper.dev; > - struct fb_info *info; > - struct drm_framebuffer *fb; > - struct drm_mode_fb_cmd2 mode_cmd; > - struct udl_gem_object *obj; > - uint32_t size; > - int ret = 0; > - > - if (sizes->surface_bpp == 24) > - sizes->surface_bpp = 32; > - > - mode_cmd.width = sizes->surface_width; > - mode_cmd.height = sizes->surface_height; > - mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8); > - > - mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, > - sizes->surface_depth); > - > - size = mode_cmd.pitches[0] * mode_cmd.height; > - size = ALIGN(size, PAGE_SIZE); > - > - obj = udl_gem_alloc_object(dev, size); > - if (!obj) > - goto out; > - > - ret = udl_gem_vmap(obj); > - if (ret) { > - DRM_ERROR("failed to vmap fb\n"); > - goto out_gfree; > - } > - > - info = drm_fb_helper_alloc_fbi(helper); > - if (IS_ERR(info)) { > - ret = PTR_ERR(info); > - goto out_gfree; > - } > - > - ret = udl_framebuffer_init(dev, &ufbdev->ufb, &mode_cmd, obj); > - if (ret) > - goto out_gfree; > - > - fb = &ufbdev->ufb.base; > - > - ufbdev->helper.fb = fb; > - > - info->screen_base = ufbdev->ufb.obj->vmapping; > - info->fix.smem_len = size; > - info->fix.smem_start = (unsigned long)ufbdev->ufb.obj->vmapping; > - > - info->fbops = &udlfb_ops; > - drm_fb_helper_fill_info(info, &ufbdev->helper, sizes); > - > - DRM_DEBUG_KMS("allocated %dx%d vmal %p\n", > - fb->width, fb->height, > - ufbdev->ufb.obj->vmapping); > - > - return ret; > -out_gfree: > - drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base); > -out: > - return ret; > -} > - > -static const struct drm_fb_helper_funcs udl_fb_helper_funcs = { > - .fb_probe = udlfb_create, > -}; > - > -static void udl_fbdev_destroy(struct drm_device *dev, > - struct udl_fbdev *ufbdev) > -{ > - drm_fb_helper_unregister_fbi(&ufbdev->helper); > - drm_fb_helper_fini(&ufbdev->helper); > - if (ufbdev->ufb.obj) { > - drm_framebuffer_unregister_private(&ufbdev->ufb.base); > - drm_framebuffer_cleanup(&ufbdev->ufb.base); > - drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base); > - } > -} > - > int udl_fbdev_init(struct drm_device *dev) > { > - struct udl_device *udl = to_udl(dev); > int bpp_sel = fb_bpp; > - struct udl_fbdev *ufbdev; > int ret; > > - ufbdev = kzalloc(sizeof(struct udl_fbdev), GFP_KERNEL); > - if (!ufbdev) > - return -ENOMEM; > - > - udl->fbdev = ufbdev; > - > - drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs); > - > - ret = drm_fb_helper_init(dev, &ufbdev->helper, 1); > - if (ret) > - goto free; > - > - ret = drm_fb_helper_single_add_all_connectors(&ufbdev->helper); > + ret = drm_fbdev_generic_setup(dev, bpp_sel); > if (ret) > - goto fini; > - > - /* disable all the possible outputs/crtcs before entering KMS mode */ > - drm_helper_disable_unused_functions(dev); > - > - ret = drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel); > - if (ret) > - goto fini; > - > + return ret; > return 0; > - > -fini: > - drm_fb_helper_fini(&ufbdev->helper); > -free: > - kfree(ufbdev); > - return ret; > -} > - > -void udl_fbdev_cleanup(struct drm_device *dev) > -{ > - struct udl_device *udl = to_udl(dev); > - if (!udl->fbdev) > - return; > - > - udl_fbdev_destroy(dev, udl->fbdev); > - kfree(udl->fbdev); > - udl->fbdev = NULL; > } > > void udl_fbdev_unplug(struct drm_device *dev) > { > - struct udl_device *udl = to_udl(dev); > - struct udl_fbdev *ufbdev; > - if (!udl->fbdev) > - return; > - > - ufbdev = udl->fbdev; > - drm_fb_helper_unlink_fbi(&ufbdev->helper); > + drm_fb_helper_unlink_fbi(dev->fb_helper); Uh I think this here can be all garbage-collected. The generic fbdev already calls drm_fb_helper_unregister_fbi, which calls unregister_framebuffer, which is a strict superset of unlink_framebuffer. The later isn't really enough for hotunplug. So for generic fbdev you really shouldn't need any cleanup nor unplug functions anymore. Also udl is the only caller of drm_fb_helper_unlink_fbi, so we could drop that. Which removes the only external caller of unlink_framebuffer, so we can drop that too. Care to follow up with those 2 patches? Aside from this this patch here looks great! -Daniel > } > > struct drm_framebuffer * > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c > index 4e854e017390..2a6399290f09 100644 > --- a/drivers/gpu/drm/udl/udl_main.c > +++ b/drivers/gpu/drm/udl/udl_main.c > @@ -367,6 +367,4 @@ void udl_fini(struct drm_device *dev) > > if (udl->urbs.count) > udl_free_urb_list(dev); > - > - udl_fbdev_cleanup(dev); > } > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c > index bc1ab6060dc6..1517d5e881b8 100644 > --- a/drivers/gpu/drm/udl/udl_modeset.c > +++ b/drivers/gpu/drm/udl/udl_modeset.c > @@ -10,6 +10,7 @@ > */ > > #include <drm/drm_crtc_helper.h> > +#include <drm/drm_fb_helper.h> > #include <drm/drm_modeset_helper_vtables.h> > #include <drm/drm_vblank.h> > > @@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev) > > static const struct drm_mode_config_funcs udl_mode_funcs = { > .fb_create = udl_fb_user_fb_create, > - .output_poll_changed = NULL, > + .output_poll_changed = drm_fb_helper_output_poll_changed, > }; > > int udl_modeset_init(struct drm_device *dev) > -- > 2.23.0 >
On Fri, Oct 25, 2019 at 09:47:46AM +0200, Daniel Vetter wrote: > On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote: > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > --- > > drivers/gpu/drm/udl/udl_drv.c | 3 + > > drivers/gpu/drm/udl/udl_drv.h | 4 - > > drivers/gpu/drm/udl/udl_fb.c | 263 +----------------------------- > > drivers/gpu/drm/udl/udl_main.c | 2 - > > drivers/gpu/drm/udl/udl_modeset.c | 3 +- > > 5 files changed, 8 insertions(+), 267 deletions(-) > > > > diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c > > index 15ad7a338f9d..6beaa1109c2c 100644 > > --- a/drivers/gpu/drm/udl/udl_drv.c > > +++ b/drivers/gpu/drm/udl/udl_drv.c > > @@ -7,6 +7,7 @@ > > > > #include <drm/drm_crtc_helper.h> > > #include <drm/drm_drv.h> > > +#include <drm/drm_fb_helper.h> > > #include <drm/drm_file.h> > > #include <drm/drm_ioctl.h> > > #include <drm/drm_probe_helper.h> > > @@ -62,6 +63,8 @@ static struct drm_driver driver = { > > .driver_features = DRIVER_MODESET | DRIVER_GEM, > > .release = udl_driver_release, > > > > + .lastclose = drm_fb_helper_lastclose, > > + > > /* gem hooks */ > > .gem_free_object_unlocked = udl_gem_free_object, > > .gem_vm_ops = &udl_gem_vm_ops, > > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h > > index 12a970fd9a87..5f8a7ac084f6 100644 > > --- a/drivers/gpu/drm/udl/udl_drv.h > > +++ b/drivers/gpu/drm/udl/udl_drv.h > > @@ -50,8 +50,6 @@ struct urb_list { > > size_t size; > > }; > > > > -struct udl_fbdev; > > - > > struct udl_device { > > struct drm_device drm; > > struct device *dev; > > @@ -65,7 +63,6 @@ struct udl_device { > > struct urb_list urbs; > > atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */ > > > > - struct udl_fbdev *fbdev; > > char mode_buf[1024]; > > uint32_t mode_buf_len; > > atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */ > > @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl); > > void udl_fini(struct drm_device *dev); > > > > int udl_fbdev_init(struct drm_device *dev); > > -void udl_fbdev_cleanup(struct drm_device *dev); > > void udl_fbdev_unplug(struct drm_device *dev); > > struct drm_framebuffer * > > udl_fb_user_fb_create(struct drm_device *dev, > > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c > > index ef3504d06343..43a1da3a56c3 100644 > > --- a/drivers/gpu/drm/udl/udl_fb.c > > +++ b/drivers/gpu/drm/udl/udl_fb.c > > @@ -19,19 +19,9 @@ > > > > #include "udl_drv.h" > > > > -#define DL_DEFIO_WRITE_DELAY (HZ/20) /* fb_deferred_io.delay in jiffies */ > > - > > -static int fb_defio = 0; /* Optionally enable experimental fb_defio mmap support */ Correction on my enthusiasm, this here is a problem: The udl defio support as-is is broken, fbdev defio and shmem are fight over the page flags. Not a problem with the old code, since disabled by default. But will be a problem with the new code. I guess you didn't test fbdev mmap? We unfortunately also lack an easy igt testcase for this ... The problem is fairly tricky to solve, here's an untested idea that might work: https://dri.freedesktop.org/docs/drm/gpu/todo.html#generic-fbdev-defio-support Cheers, Daniel > > static int fb_bpp = 16; > > > > module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP); > > -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP); > > - > > -struct udl_fbdev { > > - struct drm_fb_helper helper; /* must be first */ > > - struct udl_framebuffer ufb; > > - int fb_count; > > -}; > > > > #define DL_ALIGN_UP(x, a) ALIGN(x, a) > > #define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a) > > @@ -157,123 +147,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, > > return 0; > > } > > > > -static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > > -{ > > - unsigned long start = vma->vm_start; > > - unsigned long size = vma->vm_end - vma->vm_start; > > - unsigned long offset; > > - unsigned long page, pos; > > - > > - if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT)) > > - return -EINVAL; > > - > > - offset = vma->vm_pgoff << PAGE_SHIFT; > > - > > - if (offset > info->fix.smem_len || size > info->fix.smem_len - offset) > > - return -EINVAL; > > - > > - pos = (unsigned long)info->fix.smem_start + offset; > > - > > - pr_debug("mmap() framebuffer addr:%lu size:%lu\n", > > - pos, size); > > - > > - /* We don't want the framebuffer to be mapped encrypted */ > > - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); > > - > > - while (size > 0) { > > - page = vmalloc_to_pfn((void *)pos); > > - if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) > > - return -EAGAIN; > > - > > - start += PAGE_SIZE; > > - pos += PAGE_SIZE; > > - if (size > PAGE_SIZE) > > - size -= PAGE_SIZE; > > - else > > - size = 0; > > - } > > - > > - /* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */ > > - return 0; > > -} > > - > > -/* > > - * It's common for several clients to have framebuffer open simultaneously. > > - * e.g. both fbcon and X. Makes things interesting. > > - * Assumes caller is holding info->lock (for open and release at least) > > - */ > > -static int udl_fb_open(struct fb_info *info, int user) > > -{ > > - struct udl_fbdev *ufbdev = info->par; > > - struct drm_device *dev = ufbdev->ufb.base.dev; > > - struct udl_device *udl = to_udl(dev); > > - > > - /* If the USB device is gone, we don't accept new opens */ > > - if (drm_dev_is_unplugged(&udl->drm)) > > - return -ENODEV; > > - > > - ufbdev->fb_count++; > > - > > -#ifdef CONFIG_DRM_FBDEV_EMULATION > > - if (fb_defio && (info->fbdefio == NULL)) { > > - /* enable defio at last moment if not disabled by client */ > > - > > - struct fb_deferred_io *fbdefio; > > - > > - fbdefio = kzalloc(sizeof(struct fb_deferred_io), GFP_KERNEL); > > - > > - if (fbdefio) { > > - fbdefio->delay = DL_DEFIO_WRITE_DELAY; > > - fbdefio->deferred_io = drm_fb_helper_deferred_io; > > - } > > - > > - info->fbdefio = fbdefio; > > - fb_deferred_io_init(info); > > - } > > -#endif > > - > > - pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d\n", > > - info->node, user, info, ufbdev->fb_count); > > - > > - return 0; > > -} > > - > > - > > -/* > > - * Assumes caller is holding info->lock mutex (for open and release at least) > > - */ > > -static int udl_fb_release(struct fb_info *info, int user) > > -{ > > - struct udl_fbdev *ufbdev = info->par; > > - > > - ufbdev->fb_count--; > > - > > -#ifdef CONFIG_DRM_FBDEV_EMULATION > > - if ((ufbdev->fb_count == 0) && (info->fbdefio)) { > > - fb_deferred_io_cleanup(info); > > - kfree(info->fbdefio); > > - info->fbdefio = NULL; > > - info->fbops->fb_mmap = udl_fb_mmap; > > - } > > -#endif > > - > > - pr_debug("released /dev/fb%d user=%d count=%d\n", > > - info->node, user, ufbdev->fb_count); > > - > > - return 0; > > -} > > - > > -static struct fb_ops udlfb_ops = { > > - .owner = THIS_MODULE, > > - DRM_FB_HELPER_DEFAULT_OPS, > > - .fb_fillrect = drm_fb_helper_sys_fillrect, > > - .fb_copyarea = drm_fb_helper_sys_copyarea, > > - .fb_imageblit = drm_fb_helper_sys_imageblit, > > - .fb_mmap = udl_fb_mmap, > > - .fb_open = udl_fb_open, > > - .fb_release = udl_fb_release, > > -}; > > - > > static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, > > struct drm_file *file, > > unsigned flags, unsigned color, > > @@ -346,150 +219,20 @@ udl_framebuffer_init(struct drm_device *dev, > > return ret; > > } > > > > - > > -static int udlfb_create(struct drm_fb_helper *helper, > > - struct drm_fb_helper_surface_size *sizes) > > -{ > > - struct udl_fbdev *ufbdev = > > - container_of(helper, struct udl_fbdev, helper); > > - struct drm_device *dev = ufbdev->helper.dev; > > - struct fb_info *info; > > - struct drm_framebuffer *fb; > > - struct drm_mode_fb_cmd2 mode_cmd; > > - struct udl_gem_object *obj; > > - uint32_t size; > > - int ret = 0; > > - > > - if (sizes->surface_bpp == 24) > > - sizes->surface_bpp = 32; > > - > > - mode_cmd.width = sizes->surface_width; > > - mode_cmd.height = sizes->surface_height; > > - mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8); > > - > > - mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, > > - sizes->surface_depth); > > - > > - size = mode_cmd.pitches[0] * mode_cmd.height; > > - size = ALIGN(size, PAGE_SIZE); > > - > > - obj = udl_gem_alloc_object(dev, size); > > - if (!obj) > > - goto out; > > - > > - ret = udl_gem_vmap(obj); > > - if (ret) { > > - DRM_ERROR("failed to vmap fb\n"); > > - goto out_gfree; > > - } > > - > > - info = drm_fb_helper_alloc_fbi(helper); > > - if (IS_ERR(info)) { > > - ret = PTR_ERR(info); > > - goto out_gfree; > > - } > > - > > - ret = udl_framebuffer_init(dev, &ufbdev->ufb, &mode_cmd, obj); > > - if (ret) > > - goto out_gfree; > > - > > - fb = &ufbdev->ufb.base; > > - > > - ufbdev->helper.fb = fb; > > - > > - info->screen_base = ufbdev->ufb.obj->vmapping; > > - info->fix.smem_len = size; > > - info->fix.smem_start = (unsigned long)ufbdev->ufb.obj->vmapping; > > - > > - info->fbops = &udlfb_ops; > > - drm_fb_helper_fill_info(info, &ufbdev->helper, sizes); > > - > > - DRM_DEBUG_KMS("allocated %dx%d vmal %p\n", > > - fb->width, fb->height, > > - ufbdev->ufb.obj->vmapping); > > - > > - return ret; > > -out_gfree: > > - drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base); > > -out: > > - return ret; > > -} > > - > > -static const struct drm_fb_helper_funcs udl_fb_helper_funcs = { > > - .fb_probe = udlfb_create, > > -}; > > - > > -static void udl_fbdev_destroy(struct drm_device *dev, > > - struct udl_fbdev *ufbdev) > > -{ > > - drm_fb_helper_unregister_fbi(&ufbdev->helper); > > - drm_fb_helper_fini(&ufbdev->helper); > > - if (ufbdev->ufb.obj) { > > - drm_framebuffer_unregister_private(&ufbdev->ufb.base); > > - drm_framebuffer_cleanup(&ufbdev->ufb.base); > > - drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base); > > - } > > -} > > - > > int udl_fbdev_init(struct drm_device *dev) > > { > > - struct udl_device *udl = to_udl(dev); > > int bpp_sel = fb_bpp; > > - struct udl_fbdev *ufbdev; > > int ret; > > > > - ufbdev = kzalloc(sizeof(struct udl_fbdev), GFP_KERNEL); > > - if (!ufbdev) > > - return -ENOMEM; > > - > > - udl->fbdev = ufbdev; > > - > > - drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs); > > - > > - ret = drm_fb_helper_init(dev, &ufbdev->helper, 1); > > - if (ret) > > - goto free; > > - > > - ret = drm_fb_helper_single_add_all_connectors(&ufbdev->helper); > > + ret = drm_fbdev_generic_setup(dev, bpp_sel); > > if (ret) > > - goto fini; > > - > > - /* disable all the possible outputs/crtcs before entering KMS mode */ > > - drm_helper_disable_unused_functions(dev); > > - > > - ret = drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel); > > - if (ret) > > - goto fini; > > - > > + return ret; > > return 0; > > - > > -fini: > > - drm_fb_helper_fini(&ufbdev->helper); > > -free: > > - kfree(ufbdev); > > - return ret; > > -} > > - > > -void udl_fbdev_cleanup(struct drm_device *dev) > > -{ > > - struct udl_device *udl = to_udl(dev); > > - if (!udl->fbdev) > > - return; > > - > > - udl_fbdev_destroy(dev, udl->fbdev); > > - kfree(udl->fbdev); > > - udl->fbdev = NULL; > > } > > > > void udl_fbdev_unplug(struct drm_device *dev) > > { > > - struct udl_device *udl = to_udl(dev); > > - struct udl_fbdev *ufbdev; > > - if (!udl->fbdev) > > - return; > > - > > - ufbdev = udl->fbdev; > > - drm_fb_helper_unlink_fbi(&ufbdev->helper); > > + drm_fb_helper_unlink_fbi(dev->fb_helper); > > Uh I think this here can be all garbage-collected. The generic fbdev > already calls drm_fb_helper_unregister_fbi, which calls > unregister_framebuffer, which is a strict superset of unlink_framebuffer. > The later isn't really enough for hotunplug. > > So for generic fbdev you really shouldn't need any cleanup nor unplug > functions anymore. > > Also udl is the only caller of drm_fb_helper_unlink_fbi, so we could drop > that. Which removes the only external caller of unlink_framebuffer, so we > can drop that too. Care to follow up with those 2 patches? > > Aside from this this patch here looks great! > -Daniel > > > } > > > > struct drm_framebuffer * > > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c > > index 4e854e017390..2a6399290f09 100644 > > --- a/drivers/gpu/drm/udl/udl_main.c > > +++ b/drivers/gpu/drm/udl/udl_main.c > > @@ -367,6 +367,4 @@ void udl_fini(struct drm_device *dev) > > > > if (udl->urbs.count) > > udl_free_urb_list(dev); > > - > > - udl_fbdev_cleanup(dev); > > } > > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c > > index bc1ab6060dc6..1517d5e881b8 100644 > > --- a/drivers/gpu/drm/udl/udl_modeset.c > > +++ b/drivers/gpu/drm/udl/udl_modeset.c > > @@ -10,6 +10,7 @@ > > */ > > > > #include <drm/drm_crtc_helper.h> > > +#include <drm/drm_fb_helper.h> > > #include <drm/drm_modeset_helper_vtables.h> > > #include <drm/drm_vblank.h> > > > > @@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev) > > > > static const struct drm_mode_config_funcs udl_mode_funcs = { > > .fb_create = udl_fb_user_fb_create, > > - .output_poll_changed = NULL, > > + .output_poll_changed = drm_fb_helper_output_poll_changed, > > }; > > > > int udl_modeset_init(struct drm_device *dev) > > -- > > 2.23.0 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Hi Am 25.10.19 um 09:47 schrieb Daniel Vetter: > On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote: >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/udl/udl_drv.c | 3 + >> drivers/gpu/drm/udl/udl_drv.h | 4 - >> drivers/gpu/drm/udl/udl_fb.c | 263 +----------------------------- >> drivers/gpu/drm/udl/udl_main.c | 2 - >> drivers/gpu/drm/udl/udl_modeset.c | 3 +- >> 5 files changed, 8 insertions(+), 267 deletions(-) >> >> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c >> index 15ad7a338f9d..6beaa1109c2c 100644 >> --- a/drivers/gpu/drm/udl/udl_drv.c >> +++ b/drivers/gpu/drm/udl/udl_drv.c >> @@ -7,6 +7,7 @@ >> >> #include <drm/drm_crtc_helper.h> >> #include <drm/drm_drv.h> >> +#include <drm/drm_fb_helper.h> >> #include <drm/drm_file.h> >> #include <drm/drm_ioctl.h> >> #include <drm/drm_probe_helper.h> >> @@ -62,6 +63,8 @@ static struct drm_driver driver = { >> .driver_features = DRIVER_MODESET | DRIVER_GEM, >> .release = udl_driver_release, >> >> + .lastclose = drm_fb_helper_lastclose, >> + >> /* gem hooks */ >> .gem_free_object_unlocked = udl_gem_free_object, >> .gem_vm_ops = &udl_gem_vm_ops, >> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h >> index 12a970fd9a87..5f8a7ac084f6 100644 >> --- a/drivers/gpu/drm/udl/udl_drv.h >> +++ b/drivers/gpu/drm/udl/udl_drv.h >> @@ -50,8 +50,6 @@ struct urb_list { >> size_t size; >> }; >> >> -struct udl_fbdev; >> - >> struct udl_device { >> struct drm_device drm; >> struct device *dev; >> @@ -65,7 +63,6 @@ struct udl_device { >> struct urb_list urbs; >> atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */ >> >> - struct udl_fbdev *fbdev; >> char mode_buf[1024]; >> uint32_t mode_buf_len; >> atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */ >> @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl); >> void udl_fini(struct drm_device *dev); >> >> int udl_fbdev_init(struct drm_device *dev); >> -void udl_fbdev_cleanup(struct drm_device *dev); >> void udl_fbdev_unplug(struct drm_device *dev); >> struct drm_framebuffer * >> udl_fb_user_fb_create(struct drm_device *dev, >> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c >> index ef3504d06343..43a1da3a56c3 100644 >> --- a/drivers/gpu/drm/udl/udl_fb.c >> +++ b/drivers/gpu/drm/udl/udl_fb.c >> @@ -19,19 +19,9 @@ >> >> #include "udl_drv.h" >> >> -#define DL_DEFIO_WRITE_DELAY (HZ/20) /* fb_deferred_io.delay in jiffies */ >> - >> -static int fb_defio = 0; /* Optionally enable experimental fb_defio mmap support */ >> static int fb_bpp = 16; >> >> module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP); >> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP); >> - >> -struct udl_fbdev { >> - struct drm_fb_helper helper; /* must be first */ >> - struct udl_framebuffer ufb; >> - int fb_count; >> -}; >> >> #define DL_ALIGN_UP(x, a) ALIGN(x, a) >> #define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a) >> @@ -157,123 +147,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, >> return 0; >> } >> >> -static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) >> -{ >> - unsigned long start = vma->vm_start; >> - unsigned long size = vma->vm_end - vma->vm_start; >> - unsigned long offset; >> - unsigned long page, pos; >> - >> - if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT)) >> - return -EINVAL; >> - >> - offset = vma->vm_pgoff << PAGE_SHIFT; >> - >> - if (offset > info->fix.smem_len || size > info->fix.smem_len - offset) >> - return -EINVAL; >> - >> - pos = (unsigned long)info->fix.smem_start + offset; >> - >> - pr_debug("mmap() framebuffer addr:%lu size:%lu\n", >> - pos, size); >> - >> - /* We don't want the framebuffer to be mapped encrypted */ >> - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); >> - >> - while (size > 0) { >> - page = vmalloc_to_pfn((void *)pos); >> - if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) >> - return -EAGAIN; >> - >> - start += PAGE_SIZE; >> - pos += PAGE_SIZE; >> - if (size > PAGE_SIZE) >> - size -= PAGE_SIZE; >> - else >> - size = 0; >> - } >> - >> - /* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */ >> - return 0; >> -} >> - >> -/* >> - * It's common for several clients to have framebuffer open simultaneously. >> - * e.g. both fbcon and X. Makes things interesting. >> - * Assumes caller is holding info->lock (for open and release at least) >> - */ >> -static int udl_fb_open(struct fb_info *info, int user) >> -{ >> - struct udl_fbdev *ufbdev = info->par; >> - struct drm_device *dev = ufbdev->ufb.base.dev; >> - struct udl_device *udl = to_udl(dev); >> - >> - /* If the USB device is gone, we don't accept new opens */ >> - if (drm_dev_is_unplugged(&udl->drm)) >> - return -ENODEV; >> - >> - ufbdev->fb_count++; >> - >> -#ifdef CONFIG_DRM_FBDEV_EMULATION >> - if (fb_defio && (info->fbdefio == NULL)) { >> - /* enable defio at last moment if not disabled by client */ >> - >> - struct fb_deferred_io *fbdefio; >> - >> - fbdefio = kzalloc(sizeof(struct fb_deferred_io), GFP_KERNEL); >> - >> - if (fbdefio) { >> - fbdefio->delay = DL_DEFIO_WRITE_DELAY; >> - fbdefio->deferred_io = drm_fb_helper_deferred_io; >> - } >> - >> - info->fbdefio = fbdefio; >> - fb_deferred_io_init(info); >> - } >> -#endif >> - >> - pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d\n", >> - info->node, user, info, ufbdev->fb_count); >> - >> - return 0; >> -} >> - >> - >> -/* >> - * Assumes caller is holding info->lock mutex (for open and release at least) >> - */ >> -static int udl_fb_release(struct fb_info *info, int user) >> -{ >> - struct udl_fbdev *ufbdev = info->par; >> - >> - ufbdev->fb_count--; >> - >> -#ifdef CONFIG_DRM_FBDEV_EMULATION >> - if ((ufbdev->fb_count == 0) && (info->fbdefio)) { >> - fb_deferred_io_cleanup(info); >> - kfree(info->fbdefio); >> - info->fbdefio = NULL; >> - info->fbops->fb_mmap = udl_fb_mmap; >> - } >> -#endif >> - >> - pr_debug("released /dev/fb%d user=%d count=%d\n", >> - info->node, user, ufbdev->fb_count); >> - >> - return 0; >> -} >> - >> -static struct fb_ops udlfb_ops = { >> - .owner = THIS_MODULE, >> - DRM_FB_HELPER_DEFAULT_OPS, >> - .fb_fillrect = drm_fb_helper_sys_fillrect, >> - .fb_copyarea = drm_fb_helper_sys_copyarea, >> - .fb_imageblit = drm_fb_helper_sys_imageblit, >> - .fb_mmap = udl_fb_mmap, >> - .fb_open = udl_fb_open, >> - .fb_release = udl_fb_release, >> -}; >> - >> static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, >> struct drm_file *file, >> unsigned flags, unsigned color, >> @@ -346,150 +219,20 @@ udl_framebuffer_init(struct drm_device *dev, >> return ret; >> } >> >> - >> -static int udlfb_create(struct drm_fb_helper *helper, >> - struct drm_fb_helper_surface_size *sizes) >> -{ >> - struct udl_fbdev *ufbdev = >> - container_of(helper, struct udl_fbdev, helper); >> - struct drm_device *dev = ufbdev->helper.dev; >> - struct fb_info *info; >> - struct drm_framebuffer *fb; >> - struct drm_mode_fb_cmd2 mode_cmd; >> - struct udl_gem_object *obj; >> - uint32_t size; >> - int ret = 0; >> - >> - if (sizes->surface_bpp == 24) >> - sizes->surface_bpp = 32; >> - >> - mode_cmd.width = sizes->surface_width; >> - mode_cmd.height = sizes->surface_height; >> - mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8); >> - >> - mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, >> - sizes->surface_depth); >> - >> - size = mode_cmd.pitches[0] * mode_cmd.height; >> - size = ALIGN(size, PAGE_SIZE); >> - >> - obj = udl_gem_alloc_object(dev, size); >> - if (!obj) >> - goto out; >> - >> - ret = udl_gem_vmap(obj); >> - if (ret) { >> - DRM_ERROR("failed to vmap fb\n"); >> - goto out_gfree; >> - } >> - >> - info = drm_fb_helper_alloc_fbi(helper); >> - if (IS_ERR(info)) { >> - ret = PTR_ERR(info); >> - goto out_gfree; >> - } >> - >> - ret = udl_framebuffer_init(dev, &ufbdev->ufb, &mode_cmd, obj); >> - if (ret) >> - goto out_gfree; >> - >> - fb = &ufbdev->ufb.base; >> - >> - ufbdev->helper.fb = fb; >> - >> - info->screen_base = ufbdev->ufb.obj->vmapping; >> - info->fix.smem_len = size; >> - info->fix.smem_start = (unsigned long)ufbdev->ufb.obj->vmapping; >> - >> - info->fbops = &udlfb_ops; >> - drm_fb_helper_fill_info(info, &ufbdev->helper, sizes); >> - >> - DRM_DEBUG_KMS("allocated %dx%d vmal %p\n", >> - fb->width, fb->height, >> - ufbdev->ufb.obj->vmapping); >> - >> - return ret; >> -out_gfree: >> - drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base); >> -out: >> - return ret; >> -} >> - >> -static const struct drm_fb_helper_funcs udl_fb_helper_funcs = { >> - .fb_probe = udlfb_create, >> -}; >> - >> -static void udl_fbdev_destroy(struct drm_device *dev, >> - struct udl_fbdev *ufbdev) >> -{ >> - drm_fb_helper_unregister_fbi(&ufbdev->helper); >> - drm_fb_helper_fini(&ufbdev->helper); >> - if (ufbdev->ufb.obj) { >> - drm_framebuffer_unregister_private(&ufbdev->ufb.base); >> - drm_framebuffer_cleanup(&ufbdev->ufb.base); >> - drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base); >> - } >> -} >> - >> int udl_fbdev_init(struct drm_device *dev) >> { >> - struct udl_device *udl = to_udl(dev); >> int bpp_sel = fb_bpp; >> - struct udl_fbdev *ufbdev; >> int ret; >> >> - ufbdev = kzalloc(sizeof(struct udl_fbdev), GFP_KERNEL); >> - if (!ufbdev) >> - return -ENOMEM; >> - >> - udl->fbdev = ufbdev; >> - >> - drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs); >> - >> - ret = drm_fb_helper_init(dev, &ufbdev->helper, 1); >> - if (ret) >> - goto free; >> - >> - ret = drm_fb_helper_single_add_all_connectors(&ufbdev->helper); >> + ret = drm_fbdev_generic_setup(dev, bpp_sel); >> if (ret) >> - goto fini; >> - >> - /* disable all the possible outputs/crtcs before entering KMS mode */ >> - drm_helper_disable_unused_functions(dev); >> - >> - ret = drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel); >> - if (ret) >> - goto fini; >> - >> + return ret; >> return 0; >> - >> -fini: >> - drm_fb_helper_fini(&ufbdev->helper); >> -free: >> - kfree(ufbdev); >> - return ret; >> -} >> - >> -void udl_fbdev_cleanup(struct drm_device *dev) >> -{ >> - struct udl_device *udl = to_udl(dev); >> - if (!udl->fbdev) >> - return; >> - >> - udl_fbdev_destroy(dev, udl->fbdev); >> - kfree(udl->fbdev); >> - udl->fbdev = NULL; >> } >> >> void udl_fbdev_unplug(struct drm_device *dev) >> { >> - struct udl_device *udl = to_udl(dev); >> - struct udl_fbdev *ufbdev; >> - if (!udl->fbdev) >> - return; >> - >> - ufbdev = udl->fbdev; >> - drm_fb_helper_unlink_fbi(&ufbdev->helper); >> + drm_fb_helper_unlink_fbi(dev->fb_helper); > > Uh I think this here can be all garbage-collected. The generic fbdev > already calls drm_fb_helper_unregister_fbi, which calls > unregister_framebuffer, which is a strict superset of unlink_framebuffer. > The later isn't really enough for hotunplug. > > So for generic fbdev you really shouldn't need any cleanup nor unplug > functions anymore. > > Also udl is the only caller of drm_fb_helper_unlink_fbi, so we could drop > that. Which removes the only external caller of unlink_framebuffer, so we > can drop that too. Care to follow up with those 2 patches? Sure. BTW is there a policy for keeping potentially useful functions? I recently converted vboxvideo to generic fbdev and notices that there are no more users of drm_fb_helper_fbdev_{setup,teardown}(). But there's still a TODO item for converting drivers over to them. Shall they remain in the code base? I have quite a few patches for udl in the making. After converting to shmem and generic fbdev, udl_framebuffer can be replaced with the GEM framebuffer code. The udl modesetting code is an epicenter of simple display pipelines. Converting this over should give atomic mode setting. Best regards Thomas > Aside from this this patch here looks great! > -Daniel > >> } >> >> struct drm_framebuffer * >> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c >> index 4e854e017390..2a6399290f09 100644 >> --- a/drivers/gpu/drm/udl/udl_main.c >> +++ b/drivers/gpu/drm/udl/udl_main.c >> @@ -367,6 +367,4 @@ void udl_fini(struct drm_device *dev) >> >> if (udl->urbs.count) >> udl_free_urb_list(dev); >> - >> - udl_fbdev_cleanup(dev); >> } >> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c >> index bc1ab6060dc6..1517d5e881b8 100644 >> --- a/drivers/gpu/drm/udl/udl_modeset.c >> +++ b/drivers/gpu/drm/udl/udl_modeset.c >> @@ -10,6 +10,7 @@ >> */ >> >> #include <drm/drm_crtc_helper.h> >> +#include <drm/drm_fb_helper.h> >> #include <drm/drm_modeset_helper_vtables.h> >> #include <drm/drm_vblank.h> >> >> @@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev) >> >> static const struct drm_mode_config_funcs udl_mode_funcs = { >> .fb_create = udl_fb_user_fb_create, >> - .output_poll_changed = NULL, >> + .output_poll_changed = drm_fb_helper_output_poll_changed, >> }; >> >> int udl_modeset_init(struct drm_device *dev) >> -- >> 2.23.0 >> >
Den 25.10.2019 10.00, skrev Daniel Vetter: > On Fri, Oct 25, 2019 at 09:47:46AM +0200, Daniel Vetter wrote: >> On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote: >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> --- >>> drivers/gpu/drm/udl/udl_drv.c | 3 + >>> drivers/gpu/drm/udl/udl_drv.h | 4 - >>> drivers/gpu/drm/udl/udl_fb.c | 263 +----------------------------- >>> drivers/gpu/drm/udl/udl_main.c | 2 - >>> drivers/gpu/drm/udl/udl_modeset.c | 3 +- >>> 5 files changed, 8 insertions(+), 267 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c >>> index 15ad7a338f9d..6beaa1109c2c 100644 >>> --- a/drivers/gpu/drm/udl/udl_drv.c >>> +++ b/drivers/gpu/drm/udl/udl_drv.c >>> @@ -7,6 +7,7 @@ >>> >>> #include <drm/drm_crtc_helper.h> >>> #include <drm/drm_drv.h> >>> +#include <drm/drm_fb_helper.h> >>> #include <drm/drm_file.h> >>> #include <drm/drm_ioctl.h> >>> #include <drm/drm_probe_helper.h> >>> @@ -62,6 +63,8 @@ static struct drm_driver driver = { >>> .driver_features = DRIVER_MODESET | DRIVER_GEM, >>> .release = udl_driver_release, >>> >>> + .lastclose = drm_fb_helper_lastclose, >>> + >>> /* gem hooks */ >>> .gem_free_object_unlocked = udl_gem_free_object, >>> .gem_vm_ops = &udl_gem_vm_ops, >>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h >>> index 12a970fd9a87..5f8a7ac084f6 100644 >>> --- a/drivers/gpu/drm/udl/udl_drv.h >>> +++ b/drivers/gpu/drm/udl/udl_drv.h >>> @@ -50,8 +50,6 @@ struct urb_list { >>> size_t size; >>> }; >>> >>> -struct udl_fbdev; >>> - >>> struct udl_device { >>> struct drm_device drm; >>> struct device *dev; >>> @@ -65,7 +63,6 @@ struct udl_device { >>> struct urb_list urbs; >>> atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */ >>> >>> - struct udl_fbdev *fbdev; >>> char mode_buf[1024]; >>> uint32_t mode_buf_len; >>> atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */ >>> @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl); >>> void udl_fini(struct drm_device *dev); >>> >>> int udl_fbdev_init(struct drm_device *dev); >>> -void udl_fbdev_cleanup(struct drm_device *dev); >>> void udl_fbdev_unplug(struct drm_device *dev); >>> struct drm_framebuffer * >>> udl_fb_user_fb_create(struct drm_device *dev, >>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c >>> index ef3504d06343..43a1da3a56c3 100644 >>> --- a/drivers/gpu/drm/udl/udl_fb.c >>> +++ b/drivers/gpu/drm/udl/udl_fb.c >>> @@ -19,19 +19,9 @@ >>> >>> #include "udl_drv.h" >>> >>> -#define DL_DEFIO_WRITE_DELAY (HZ/20) /* fb_deferred_io.delay in jiffies */ >>> - >>> -static int fb_defio = 0; /* Optionally enable experimental fb_defio mmap support */ > > Correction on my enthusiasm, this here is a problem: > > The udl defio support as-is is broken, fbdev defio and shmem are fight > over the page flags. Not a problem with the old code, since disabled by > default. But will be a problem with the new code. I guess you didn't test > fbdev mmap? We unfortunately also lack an easy igt testcase for this ... This is where the shadow buffer comes to the rescue. fbdev gets a vmalloc buffer and this is blitted on the shmem buffer in the defio callback before calling the framebuffer .dirty callback. So the defio internals never sees the shmem buffer. At least this worked when I was writing the shmem helper. Noralf. > > The problem is fairly tricky to solve, here's an untested idea that might > work: > > https://dri.freedesktop.org/docs/drm/gpu/todo.html#generic-fbdev-defio-support > > Cheers, Daniel >
Hi Noralf Am 25.10.19 um 13:22 schrieb Noralf Trønnes: > > > Den 25.10.2019 10.00, skrev Daniel Vetter: >> On Fri, Oct 25, 2019 at 09:47:46AM +0200, Daniel Vetter wrote: >>> On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote: >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> --- >>>> drivers/gpu/drm/udl/udl_drv.c | 3 + >>>> drivers/gpu/drm/udl/udl_drv.h | 4 - >>>> drivers/gpu/drm/udl/udl_fb.c | 263 +----------------------------- >>>> drivers/gpu/drm/udl/udl_main.c | 2 - >>>> drivers/gpu/drm/udl/udl_modeset.c | 3 +- >>>> 5 files changed, 8 insertions(+), 267 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c >>>> index 15ad7a338f9d..6beaa1109c2c 100644 >>>> --- a/drivers/gpu/drm/udl/udl_drv.c >>>> +++ b/drivers/gpu/drm/udl/udl_drv.c >>>> @@ -7,6 +7,7 @@ >>>> >>>> #include <drm/drm_crtc_helper.h> >>>> #include <drm/drm_drv.h> >>>> +#include <drm/drm_fb_helper.h> >>>> #include <drm/drm_file.h> >>>> #include <drm/drm_ioctl.h> >>>> #include <drm/drm_probe_helper.h> >>>> @@ -62,6 +63,8 @@ static struct drm_driver driver = { >>>> .driver_features = DRIVER_MODESET | DRIVER_GEM, >>>> .release = udl_driver_release, >>>> >>>> + .lastclose = drm_fb_helper_lastclose, >>>> + >>>> /* gem hooks */ >>>> .gem_free_object_unlocked = udl_gem_free_object, >>>> .gem_vm_ops = &udl_gem_vm_ops, >>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h >>>> index 12a970fd9a87..5f8a7ac084f6 100644 >>>> --- a/drivers/gpu/drm/udl/udl_drv.h >>>> +++ b/drivers/gpu/drm/udl/udl_drv.h >>>> @@ -50,8 +50,6 @@ struct urb_list { >>>> size_t size; >>>> }; >>>> >>>> -struct udl_fbdev; >>>> - >>>> struct udl_device { >>>> struct drm_device drm; >>>> struct device *dev; >>>> @@ -65,7 +63,6 @@ struct udl_device { >>>> struct urb_list urbs; >>>> atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */ >>>> >>>> - struct udl_fbdev *fbdev; >>>> char mode_buf[1024]; >>>> uint32_t mode_buf_len; >>>> atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */ >>>> @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl); >>>> void udl_fini(struct drm_device *dev); >>>> >>>> int udl_fbdev_init(struct drm_device *dev); >>>> -void udl_fbdev_cleanup(struct drm_device *dev); >>>> void udl_fbdev_unplug(struct drm_device *dev); >>>> struct drm_framebuffer * >>>> udl_fb_user_fb_create(struct drm_device *dev, >>>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c >>>> index ef3504d06343..43a1da3a56c3 100644 >>>> --- a/drivers/gpu/drm/udl/udl_fb.c >>>> +++ b/drivers/gpu/drm/udl/udl_fb.c >>>> @@ -19,19 +19,9 @@ >>>> >>>> #include "udl_drv.h" >>>> >>>> -#define DL_DEFIO_WRITE_DELAY (HZ/20) /* fb_deferred_io.delay in jiffies */ >>>> - >>>> -static int fb_defio = 0; /* Optionally enable experimental fb_defio mmap support */ >> >> Correction on my enthusiasm, this here is a problem: >> >> The udl defio support as-is is broken, fbdev defio and shmem are fight >> over the page flags. Not a problem with the old code, since disabled by >> default. But will be a problem with the new code. I guess you didn't test >> fbdev mmap? We unfortunately also lack an easy igt testcase for this ... > > This is where the shadow buffer comes to the rescue. fbdev gets a > vmalloc buffer and this is blitted on the shmem buffer in the defio > callback before calling the framebuffer .dirty callback. So the defio > internals never sees the shmem buffer. At least this worked when I was > writing the shmem helper. There's already a patchset on the ML, cleaning up the docs around fb defio. Please take a look. Best regards Thomas > > Noralf. > >> >> The problem is fairly tricky to solve, here's an untested idea that might >> work: >> >> https://dri.freedesktop.org/docs/drm/gpu/todo.html#generic-fbdev-defio-support >> >> Cheers, Daniel >>
On Fri, Oct 25, 2019 at 10:08 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 25.10.19 um 09:47 schrieb Daniel Vetter: > > On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote: > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> --- > >> drivers/gpu/drm/udl/udl_drv.c | 3 + > >> drivers/gpu/drm/udl/udl_drv.h | 4 - > >> drivers/gpu/drm/udl/udl_fb.c | 263 +----------------------------- > >> drivers/gpu/drm/udl/udl_main.c | 2 - > >> drivers/gpu/drm/udl/udl_modeset.c | 3 +- > >> 5 files changed, 8 insertions(+), 267 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c > >> index 15ad7a338f9d..6beaa1109c2c 100644 > >> --- a/drivers/gpu/drm/udl/udl_drv.c > >> +++ b/drivers/gpu/drm/udl/udl_drv.c > >> @@ -7,6 +7,7 @@ > >> > >> #include <drm/drm_crtc_helper.h> > >> #include <drm/drm_drv.h> > >> +#include <drm/drm_fb_helper.h> > >> #include <drm/drm_file.h> > >> #include <drm/drm_ioctl.h> > >> #include <drm/drm_probe_helper.h> > >> @@ -62,6 +63,8 @@ static struct drm_driver driver = { > >> .driver_features = DRIVER_MODESET | DRIVER_GEM, > >> .release = udl_driver_release, > >> > >> + .lastclose = drm_fb_helper_lastclose, > >> + > >> /* gem hooks */ > >> .gem_free_object_unlocked = udl_gem_free_object, > >> .gem_vm_ops = &udl_gem_vm_ops, > >> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h > >> index 12a970fd9a87..5f8a7ac084f6 100644 > >> --- a/drivers/gpu/drm/udl/udl_drv.h > >> +++ b/drivers/gpu/drm/udl/udl_drv.h > >> @@ -50,8 +50,6 @@ struct urb_list { > >> size_t size; > >> }; > >> > >> -struct udl_fbdev; > >> - > >> struct udl_device { > >> struct drm_device drm; > >> struct device *dev; > >> @@ -65,7 +63,6 @@ struct udl_device { > >> struct urb_list urbs; > >> atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */ > >> > >> - struct udl_fbdev *fbdev; > >> char mode_buf[1024]; > >> uint32_t mode_buf_len; > >> atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */ > >> @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl); > >> void udl_fini(struct drm_device *dev); > >> > >> int udl_fbdev_init(struct drm_device *dev); > >> -void udl_fbdev_cleanup(struct drm_device *dev); > >> void udl_fbdev_unplug(struct drm_device *dev); > >> struct drm_framebuffer * > >> udl_fb_user_fb_create(struct drm_device *dev, > >> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c > >> index ef3504d06343..43a1da3a56c3 100644 > >> --- a/drivers/gpu/drm/udl/udl_fb.c > >> +++ b/drivers/gpu/drm/udl/udl_fb.c > >> @@ -19,19 +19,9 @@ > >> > >> #include "udl_drv.h" > >> > >> -#define DL_DEFIO_WRITE_DELAY (HZ/20) /* fb_deferred_io.delay in jiffies */ > >> - > >> -static int fb_defio = 0; /* Optionally enable experimental fb_defio mmap support */ > >> static int fb_bpp = 16; > >> > >> module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP); > >> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP); > >> - > >> -struct udl_fbdev { > >> - struct drm_fb_helper helper; /* must be first */ > >> - struct udl_framebuffer ufb; > >> - int fb_count; > >> -}; > >> > >> #define DL_ALIGN_UP(x, a) ALIGN(x, a) > >> #define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a) > >> @@ -157,123 +147,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, > >> return 0; > >> } > >> > >> -static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > >> -{ > >> - unsigned long start = vma->vm_start; > >> - unsigned long size = vma->vm_end - vma->vm_start; > >> - unsigned long offset; > >> - unsigned long page, pos; > >> - > >> - if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT)) > >> - return -EINVAL; > >> - > >> - offset = vma->vm_pgoff << PAGE_SHIFT; > >> - > >> - if (offset > info->fix.smem_len || size > info->fix.smem_len - offset) > >> - return -EINVAL; > >> - > >> - pos = (unsigned long)info->fix.smem_start + offset; > >> - > >> - pr_debug("mmap() framebuffer addr:%lu size:%lu\n", > >> - pos, size); > >> - > >> - /* We don't want the framebuffer to be mapped encrypted */ > >> - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); > >> - > >> - while (size > 0) { > >> - page = vmalloc_to_pfn((void *)pos); > >> - if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) > >> - return -EAGAIN; > >> - > >> - start += PAGE_SIZE; > >> - pos += PAGE_SIZE; > >> - if (size > PAGE_SIZE) > >> - size -= PAGE_SIZE; > >> - else > >> - size = 0; > >> - } > >> - > >> - /* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */ > >> - return 0; > >> -} > >> - > >> -/* > >> - * It's common for several clients to have framebuffer open simultaneously. > >> - * e.g. both fbcon and X. Makes things interesting. > >> - * Assumes caller is holding info->lock (for open and release at least) > >> - */ > >> -static int udl_fb_open(struct fb_info *info, int user) > >> -{ > >> - struct udl_fbdev *ufbdev = info->par; > >> - struct drm_device *dev = ufbdev->ufb.base.dev; > >> - struct udl_device *udl = to_udl(dev); > >> - > >> - /* If the USB device is gone, we don't accept new opens */ > >> - if (drm_dev_is_unplugged(&udl->drm)) > >> - return -ENODEV; > >> - > >> - ufbdev->fb_count++; > >> - > >> -#ifdef CONFIG_DRM_FBDEV_EMULATION > >> - if (fb_defio && (info->fbdefio == NULL)) { > >> - /* enable defio at last moment if not disabled by client */ > >> - > >> - struct fb_deferred_io *fbdefio; > >> - > >> - fbdefio = kzalloc(sizeof(struct fb_deferred_io), GFP_KERNEL); > >> - > >> - if (fbdefio) { > >> - fbdefio->delay = DL_DEFIO_WRITE_DELAY; > >> - fbdefio->deferred_io = drm_fb_helper_deferred_io; > >> - } > >> - > >> - info->fbdefio = fbdefio; > >> - fb_deferred_io_init(info); > >> - } > >> -#endif > >> - > >> - pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d\n", > >> - info->node, user, info, ufbdev->fb_count); > >> - > >> - return 0; > >> -} > >> - > >> - > >> -/* > >> - * Assumes caller is holding info->lock mutex (for open and release at least) > >> - */ > >> -static int udl_fb_release(struct fb_info *info, int user) > >> -{ > >> - struct udl_fbdev *ufbdev = info->par; > >> - > >> - ufbdev->fb_count--; > >> - > >> -#ifdef CONFIG_DRM_FBDEV_EMULATION > >> - if ((ufbdev->fb_count == 0) && (info->fbdefio)) { > >> - fb_deferred_io_cleanup(info); > >> - kfree(info->fbdefio); > >> - info->fbdefio = NULL; > >> - info->fbops->fb_mmap = udl_fb_mmap; > >> - } > >> -#endif > >> - > >> - pr_debug("released /dev/fb%d user=%d count=%d\n", > >> - info->node, user, ufbdev->fb_count); > >> - > >> - return 0; > >> -} > >> - > >> -static struct fb_ops udlfb_ops = { > >> - .owner = THIS_MODULE, > >> - DRM_FB_HELPER_DEFAULT_OPS, > >> - .fb_fillrect = drm_fb_helper_sys_fillrect, > >> - .fb_copyarea = drm_fb_helper_sys_copyarea, > >> - .fb_imageblit = drm_fb_helper_sys_imageblit, > >> - .fb_mmap = udl_fb_mmap, > >> - .fb_open = udl_fb_open, > >> - .fb_release = udl_fb_release, > >> -}; > >> - > >> static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, > >> struct drm_file *file, > >> unsigned flags, unsigned color, > >> @@ -346,150 +219,20 @@ udl_framebuffer_init(struct drm_device *dev, > >> return ret; > >> } > >> > >> - > >> -static int udlfb_create(struct drm_fb_helper *helper, > >> - struct drm_fb_helper_surface_size *sizes) > >> -{ > >> - struct udl_fbdev *ufbdev = > >> - container_of(helper, struct udl_fbdev, helper); > >> - struct drm_device *dev = ufbdev->helper.dev; > >> - struct fb_info *info; > >> - struct drm_framebuffer *fb; > >> - struct drm_mode_fb_cmd2 mode_cmd; > >> - struct udl_gem_object *obj; > >> - uint32_t size; > >> - int ret = 0; > >> - > >> - if (sizes->surface_bpp == 24) > >> - sizes->surface_bpp = 32; > >> - > >> - mode_cmd.width = sizes->surface_width; > >> - mode_cmd.height = sizes->surface_height; > >> - mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8); > >> - > >> - mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, > >> - sizes->surface_depth); > >> - > >> - size = mode_cmd.pitches[0] * mode_cmd.height; > >> - size = ALIGN(size, PAGE_SIZE); > >> - > >> - obj = udl_gem_alloc_object(dev, size); > >> - if (!obj) > >> - goto out; > >> - > >> - ret = udl_gem_vmap(obj); > >> - if (ret) { > >> - DRM_ERROR("failed to vmap fb\n"); > >> - goto out_gfree; > >> - } > >> - > >> - info = drm_fb_helper_alloc_fbi(helper); > >> - if (IS_ERR(info)) { > >> - ret = PTR_ERR(info); > >> - goto out_gfree; > >> - } > >> - > >> - ret = udl_framebuffer_init(dev, &ufbdev->ufb, &mode_cmd, obj); > >> - if (ret) > >> - goto out_gfree; > >> - > >> - fb = &ufbdev->ufb.base; > >> - > >> - ufbdev->helper.fb = fb; > >> - > >> - info->screen_base = ufbdev->ufb.obj->vmapping; > >> - info->fix.smem_len = size; > >> - info->fix.smem_start = (unsigned long)ufbdev->ufb.obj->vmapping; > >> - > >> - info->fbops = &udlfb_ops; > >> - drm_fb_helper_fill_info(info, &ufbdev->helper, sizes); > >> - > >> - DRM_DEBUG_KMS("allocated %dx%d vmal %p\n", > >> - fb->width, fb->height, > >> - ufbdev->ufb.obj->vmapping); > >> - > >> - return ret; > >> -out_gfree: > >> - drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base); > >> -out: > >> - return ret; > >> -} > >> - > >> -static const struct drm_fb_helper_funcs udl_fb_helper_funcs = { > >> - .fb_probe = udlfb_create, > >> -}; > >> - > >> -static void udl_fbdev_destroy(struct drm_device *dev, > >> - struct udl_fbdev *ufbdev) > >> -{ > >> - drm_fb_helper_unregister_fbi(&ufbdev->helper); > >> - drm_fb_helper_fini(&ufbdev->helper); > >> - if (ufbdev->ufb.obj) { > >> - drm_framebuffer_unregister_private(&ufbdev->ufb.base); > >> - drm_framebuffer_cleanup(&ufbdev->ufb.base); > >> - drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base); > >> - } > >> -} > >> - > >> int udl_fbdev_init(struct drm_device *dev) > >> { > >> - struct udl_device *udl = to_udl(dev); > >> int bpp_sel = fb_bpp; > >> - struct udl_fbdev *ufbdev; > >> int ret; > >> > >> - ufbdev = kzalloc(sizeof(struct udl_fbdev), GFP_KERNEL); > >> - if (!ufbdev) > >> - return -ENOMEM; > >> - > >> - udl->fbdev = ufbdev; > >> - > >> - drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs); > >> - > >> - ret = drm_fb_helper_init(dev, &ufbdev->helper, 1); > >> - if (ret) > >> - goto free; > >> - > >> - ret = drm_fb_helper_single_add_all_connectors(&ufbdev->helper); > >> + ret = drm_fbdev_generic_setup(dev, bpp_sel); > >> if (ret) > >> - goto fini; > >> - > >> - /* disable all the possible outputs/crtcs before entering KMS mode */ > >> - drm_helper_disable_unused_functions(dev); > >> - > >> - ret = drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel); > >> - if (ret) > >> - goto fini; > >> - > >> + return ret; > >> return 0; > >> - > >> -fini: > >> - drm_fb_helper_fini(&ufbdev->helper); > >> -free: > >> - kfree(ufbdev); > >> - return ret; > >> -} > >> - > >> -void udl_fbdev_cleanup(struct drm_device *dev) > >> -{ > >> - struct udl_device *udl = to_udl(dev); > >> - if (!udl->fbdev) > >> - return; > >> - > >> - udl_fbdev_destroy(dev, udl->fbdev); > >> - kfree(udl->fbdev); > >> - udl->fbdev = NULL; > >> } > >> > >> void udl_fbdev_unplug(struct drm_device *dev) > >> { > >> - struct udl_device *udl = to_udl(dev); > >> - struct udl_fbdev *ufbdev; > >> - if (!udl->fbdev) > >> - return; > >> - > >> - ufbdev = udl->fbdev; > >> - drm_fb_helper_unlink_fbi(&ufbdev->helper); > >> + drm_fb_helper_unlink_fbi(dev->fb_helper); > > > > Uh I think this here can be all garbage-collected. The generic fbdev > > already calls drm_fb_helper_unregister_fbi, which calls > > unregister_framebuffer, which is a strict superset of unlink_framebuffer. > > The later isn't really enough for hotunplug. > > > > So for generic fbdev you really shouldn't need any cleanup nor unplug > > functions anymore. > > > > Also udl is the only caller of drm_fb_helper_unlink_fbi, so we could drop > > that. Which removes the only external caller of unlink_framebuffer, so we > > can drop that too. Care to follow up with those 2 patches? > > Sure. BTW is there a policy for keeping potentially useful functions? I > recently converted vboxvideo to generic fbdev and notices that there are > no more users of drm_fb_helper_fbdev_{setup,teardown}(). But there's > still a TODO item for converting drivers over to them. Shall they remain > in the code base? As Noralf mentioned, maybe better to ditch them, and update the TODO entry to instead encourage the generic fbdev code. That one is even less code. > I have quite a few patches for udl in the making. After converting to > shmem and generic fbdev, udl_framebuffer can be replaced with the GEM > framebuffer code. The udl modesetting code is an epicenter of simple > display pipelines. Converting this over should give atomic mode setting. Awesome! -Daniel > > Best regards > Thomas > > > Aside from this this patch here looks great! > > -Daniel > > > >> } > >> > >> struct drm_framebuffer * > >> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c > >> index 4e854e017390..2a6399290f09 100644 > >> --- a/drivers/gpu/drm/udl/udl_main.c > >> +++ b/drivers/gpu/drm/udl/udl_main.c > >> @@ -367,6 +367,4 @@ void udl_fini(struct drm_device *dev) > >> > >> if (udl->urbs.count) > >> udl_free_urb_list(dev); > >> - > >> - udl_fbdev_cleanup(dev); > >> } > >> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c > >> index bc1ab6060dc6..1517d5e881b8 100644 > >> --- a/drivers/gpu/drm/udl/udl_modeset.c > >> +++ b/drivers/gpu/drm/udl/udl_modeset.c > >> @@ -10,6 +10,7 @@ > >> */ > >> > >> #include <drm/drm_crtc_helper.h> > >> +#include <drm/drm_fb_helper.h> > >> #include <drm/drm_modeset_helper_vtables.h> > >> #include <drm/drm_vblank.h> > >> > >> @@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev) > >> > >> static const struct drm_mode_config_funcs udl_mode_funcs = { > >> .fb_create = udl_fb_user_fb_create, > >> - .output_poll_changed = NULL, > >> + .output_poll_changed = drm_fb_helper_output_poll_changed, > >> }; > >> > >> int udl_modeset_init(struct drm_device *dev) > >> -- > >> 2.23.0 > >> > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 15ad7a338f9d..6beaa1109c2c 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -7,6 +7,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_drv.h> +#include <drm/drm_fb_helper.h> #include <drm/drm_file.h> #include <drm/drm_ioctl.h> #include <drm/drm_probe_helper.h> @@ -62,6 +63,8 @@ static struct drm_driver driver = { .driver_features = DRIVER_MODESET | DRIVER_GEM, .release = udl_driver_release, + .lastclose = drm_fb_helper_lastclose, + /* gem hooks */ .gem_free_object_unlocked = udl_gem_free_object, .gem_vm_ops = &udl_gem_vm_ops, diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 12a970fd9a87..5f8a7ac084f6 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -50,8 +50,6 @@ struct urb_list { size_t size; }; -struct udl_fbdev; - struct udl_device { struct drm_device drm; struct device *dev; @@ -65,7 +63,6 @@ struct udl_device { struct urb_list urbs; atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */ - struct udl_fbdev *fbdev; char mode_buf[1024]; uint32_t mode_buf_len; atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */ @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl); void udl_fini(struct drm_device *dev); int udl_fbdev_init(struct drm_device *dev); -void udl_fbdev_cleanup(struct drm_device *dev); void udl_fbdev_unplug(struct drm_device *dev); struct drm_framebuffer * udl_fb_user_fb_create(struct drm_device *dev, diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index ef3504d06343..43a1da3a56c3 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -19,19 +19,9 @@ #include "udl_drv.h" -#define DL_DEFIO_WRITE_DELAY (HZ/20) /* fb_deferred_io.delay in jiffies */ - -static int fb_defio = 0; /* Optionally enable experimental fb_defio mmap support */ static int fb_bpp = 16; module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP); -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP); - -struct udl_fbdev { - struct drm_fb_helper helper; /* must be first */ - struct udl_framebuffer ufb; - int fb_count; -}; #define DL_ALIGN_UP(x, a) ALIGN(x, a) #define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a) @@ -157,123 +147,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, return 0; } -static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) -{ - unsigned long start = vma->vm_start; - unsigned long size = vma->vm_end - vma->vm_start; - unsigned long offset; - unsigned long page, pos; - - if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT)) - return -EINVAL; - - offset = vma->vm_pgoff << PAGE_SHIFT; - - if (offset > info->fix.smem_len || size > info->fix.smem_len - offset) - return -EINVAL; - - pos = (unsigned long)info->fix.smem_start + offset; - - pr_debug("mmap() framebuffer addr:%lu size:%lu\n", - pos, size); - - /* We don't want the framebuffer to be mapped encrypted */ - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); - - while (size > 0) { - page = vmalloc_to_pfn((void *)pos); - if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) - return -EAGAIN; - - start += PAGE_SIZE; - pos += PAGE_SIZE; - if (size > PAGE_SIZE) - size -= PAGE_SIZE; - else - size = 0; - } - - /* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */ - return 0; -} - -/* - * It's common for several clients to have framebuffer open simultaneously. - * e.g. both fbcon and X. Makes things interesting. - * Assumes caller is holding info->lock (for open and release at least) - */ -static int udl_fb_open(struct fb_info *info, int user) -{ - struct udl_fbdev *ufbdev = info->par; - struct drm_device *dev = ufbdev->ufb.base.dev; - struct udl_device *udl = to_udl(dev); - - /* If the USB device is gone, we don't accept new opens */ - if (drm_dev_is_unplugged(&udl->drm)) - return -ENODEV; - - ufbdev->fb_count++; - -#ifdef CONFIG_DRM_FBDEV_EMULATION - if (fb_defio && (info->fbdefio == NULL)) { - /* enable defio at last moment if not disabled by client */ - - struct fb_deferred_io *fbdefio; - - fbdefio = kzalloc(sizeof(struct fb_deferred_io), GFP_KERNEL); - - if (fbdefio) { - fbdefio->delay = DL_DEFIO_WRITE_DELAY; - fbdefio->deferred_io = drm_fb_helper_deferred_io; - } - - info->fbdefio = fbdefio; - fb_deferred_io_init(info); - } -#endif - - pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d\n", - info->node, user, info, ufbdev->fb_count); - - return 0; -} - - -/* - * Assumes caller is holding info->lock mutex (for open and release at least) - */ -static int udl_fb_release(struct fb_info *info, int user) -{ - struct udl_fbdev *ufbdev = info->par; - - ufbdev->fb_count--; - -#ifdef CONFIG_DRM_FBDEV_EMULATION - if ((ufbdev->fb_count == 0) && (info->fbdefio)) { - fb_deferred_io_cleanup(info); - kfree(info->fbdefio); - info->fbdefio = NULL; - info->fbops->fb_mmap = udl_fb_mmap; - } -#endif - - pr_debug("released /dev/fb%d user=%d count=%d\n", - info->node, user, ufbdev->fb_count); - - return 0; -} - -static struct fb_ops udlfb_ops = { - .owner = THIS_MODULE, - DRM_FB_HELPER_DEFAULT_OPS, - .fb_fillrect = drm_fb_helper_sys_fillrect, - .fb_copyarea = drm_fb_helper_sys_copyarea, - .fb_imageblit = drm_fb_helper_sys_imageblit, - .fb_mmap = udl_fb_mmap, - .fb_open = udl_fb_open, - .fb_release = udl_fb_release, -}; - static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, struct drm_file *file, unsigned flags, unsigned color, @@ -346,150 +219,20 @@ udl_framebuffer_init(struct drm_device *dev, return ret; } - -static int udlfb_create(struct drm_fb_helper *helper, - struct drm_fb_helper_surface_size *sizes) -{ - struct udl_fbdev *ufbdev = - container_of(helper, struct udl_fbdev, helper); - struct drm_device *dev = ufbdev->helper.dev; - struct fb_info *info; - struct drm_framebuffer *fb; - struct drm_mode_fb_cmd2 mode_cmd; - struct udl_gem_object *obj; - uint32_t size; - int ret = 0; - - if (sizes->surface_bpp == 24) - sizes->surface_bpp = 32; - - mode_cmd.width = sizes->surface_width; - mode_cmd.height = sizes->surface_height; - mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8); - - mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, - sizes->surface_depth); - - size = mode_cmd.pitches[0] * mode_cmd.height; - size = ALIGN(size, PAGE_SIZE); - - obj = udl_gem_alloc_object(dev, size); - if (!obj) - goto out; - - ret = udl_gem_vmap(obj); - if (ret) { - DRM_ERROR("failed to vmap fb\n"); - goto out_gfree; - } - - info = drm_fb_helper_alloc_fbi(helper); - if (IS_ERR(info)) { - ret = PTR_ERR(info); - goto out_gfree; - } - - ret = udl_framebuffer_init(dev, &ufbdev->ufb, &mode_cmd, obj); - if (ret) - goto out_gfree; - - fb = &ufbdev->ufb.base; - - ufbdev->helper.fb = fb; - - info->screen_base = ufbdev->ufb.obj->vmapping; - info->fix.smem_len = size; - info->fix.smem_start = (unsigned long)ufbdev->ufb.obj->vmapping; - - info->fbops = &udlfb_ops; - drm_fb_helper_fill_info(info, &ufbdev->helper, sizes); - - DRM_DEBUG_KMS("allocated %dx%d vmal %p\n", - fb->width, fb->height, - ufbdev->ufb.obj->vmapping); - - return ret; -out_gfree: - drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base); -out: - return ret; -} - -static const struct drm_fb_helper_funcs udl_fb_helper_funcs = { - .fb_probe = udlfb_create, -}; - -static void udl_fbdev_destroy(struct drm_device *dev, - struct udl_fbdev *ufbdev) -{ - drm_fb_helper_unregister_fbi(&ufbdev->helper); - drm_fb_helper_fini(&ufbdev->helper); - if (ufbdev->ufb.obj) { - drm_framebuffer_unregister_private(&ufbdev->ufb.base); - drm_framebuffer_cleanup(&ufbdev->ufb.base); - drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base); - } -} - int udl_fbdev_init(struct drm_device *dev) { - struct udl_device *udl = to_udl(dev); int bpp_sel = fb_bpp; - struct udl_fbdev *ufbdev; int ret; - ufbdev = kzalloc(sizeof(struct udl_fbdev), GFP_KERNEL); - if (!ufbdev) - return -ENOMEM; - - udl->fbdev = ufbdev; - - drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs); - - ret = drm_fb_helper_init(dev, &ufbdev->helper, 1); - if (ret) - goto free; - - ret = drm_fb_helper_single_add_all_connectors(&ufbdev->helper); + ret = drm_fbdev_generic_setup(dev, bpp_sel); if (ret) - goto fini; - - /* disable all the possible outputs/crtcs before entering KMS mode */ - drm_helper_disable_unused_functions(dev); - - ret = drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel); - if (ret) - goto fini; - + return ret; return 0; - -fini: - drm_fb_helper_fini(&ufbdev->helper); -free: - kfree(ufbdev); - return ret; -} - -void udl_fbdev_cleanup(struct drm_device *dev) -{ - struct udl_device *udl = to_udl(dev); - if (!udl->fbdev) - return; - - udl_fbdev_destroy(dev, udl->fbdev); - kfree(udl->fbdev); - udl->fbdev = NULL; } void udl_fbdev_unplug(struct drm_device *dev) { - struct udl_device *udl = to_udl(dev); - struct udl_fbdev *ufbdev; - if (!udl->fbdev) - return; - - ufbdev = udl->fbdev; - drm_fb_helper_unlink_fbi(&ufbdev->helper); + drm_fb_helper_unlink_fbi(dev->fb_helper); } struct drm_framebuffer * diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 4e854e017390..2a6399290f09 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -367,6 +367,4 @@ void udl_fini(struct drm_device *dev) if (udl->urbs.count) udl_free_urb_list(dev); - - udl_fbdev_cleanup(dev); } diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c index bc1ab6060dc6..1517d5e881b8 100644 --- a/drivers/gpu/drm/udl/udl_modeset.c +++ b/drivers/gpu/drm/udl/udl_modeset.c @@ -10,6 +10,7 @@ */ #include <drm/drm_crtc_helper.h> +#include <drm/drm_fb_helper.h> #include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_vblank.h> @@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev) static const struct drm_mode_config_funcs udl_mode_funcs = { .fb_create = udl_fb_user_fb_create, - .output_poll_changed = NULL, + .output_poll_changed = drm_fb_helper_output_poll_changed, }; int udl_modeset_init(struct drm_device *dev)
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/udl/udl_drv.c | 3 + drivers/gpu/drm/udl/udl_drv.h | 4 - drivers/gpu/drm/udl/udl_fb.c | 263 +----------------------------- drivers/gpu/drm/udl/udl_main.c | 2 - drivers/gpu/drm/udl/udl_modeset.c | 3 +- 5 files changed, 8 insertions(+), 267 deletions(-)