Re: [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 25, 2019 at 10:08 AM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@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 */
> >>  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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux