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@xxxxxxx>
> > ---
> > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
