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?
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
>>
>
--
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
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
