Re: [PATCH] fbdev: udl: Make CONFIG_FB_DEVICE optional

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi Helge

Am 25.10.24 um 17:37 schrieb Helge Deller:
> On 10/25/24 11:25, Gonzalo Silvalde Blanco wrote:
> > The fb_udl driver currently depends on CONFIG_FB_DEVICE to create sysfs
> > entries and access framebuffer device information. This patch wraps the
> > relevant code blocks with #ifdef CONFIG_FB_DEVICE, allowing the 
> > driver to
> > be built and used even if CONFIG_FB_DEVICE is not selected.
> >
> > The sysfs setting only controls access to certain framebuffer attributes
> > and is not required for the basic display functionality to work 
> > correctly.
> > (For information on DisplayLink devices and their Linux support, see:
> > https://wiki.archlinux.org/title/DisplayLink).
> >
> > Tested by building with and without CONFIG_FB_DEVICE, both of which
> > compiled and ran without issues.
>
> Gonzalo, I don't like this patch very much.
>
> It adds lots of #ifdefs around functions like dev_dbg().
> Instead of ifdefs, aren't there other possibilities, e.g.
> using fb_dbg() if appropriate?
> Or using any other generic dbg() info or simply dropping the line?

I talked Gonzalo into sending this patch. I think dev_dbg() calls should 
be replaced with fb_dbg(), same for _info() and _err(). That's probably 
worth doing anyway.

> But more important:
> This is a fbdev driver and currently depends on CONFIG_FB_DEVICE.
> If I'm right, the only reason to disable CONFIG_FB_DEVICE is if
> you want fbdev output at bootup, but otherwise just want to use DRM.

It's unrelated to booting. CONFIG_FB_DEVICE enables/disables userspace 
interfaces (/dev/fb*, /sys/graphics/fb*). Even without, there's still 
fbcon that runs on top of the fbdev driver.

> But then, doesn't there exist a native DRM driver for this graphics
> card which can be used instead?
> If so, I suggest to not change this fbdev driver at all.

Or can we talk about removing udlfb entirely? I tried before, but there 
was one person still using it. [1] He had concerns about udl's (the DRM 
driver) stability. I think DRM's udl has matured enough and is in better 
shape than udlfb. Maybe we can try again.

[1] 
https://lore.kernel.org/dri-devel/20201130125200.10416-1-tzimmermann@xxxxxxx/

Best regards
Thomas

> Helge
>
> > Signed-off-by: Gonzalo Silvalde Blanco <gonzalo.silvalde@xxxxxxxxx>> ---
> >   drivers/video/fbdev/Kconfig |  1 -
> >   drivers/video/fbdev/udlfb.c | 41 ++++++++++++++++++++++---------------
> >   2 files changed, 24 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> > index ea36c6956bf3..9bf6cf74b9cb 100644
> > --- a/drivers/video/fbdev/Kconfig
> > +++ b/drivers/video/fbdev/Kconfig
> > @@ -1588,7 +1588,6 @@ config FB_SMSCUFX
> >   config FB_UDL
> >       tristate "Displaylink USB Framebuffer support"
> >       depends on FB && USB
> > -    depends on FB_DEVICE
> >       select FB_MODE_HELPERS
> >       select FB_SYSMEM_HELPERS_DEFERRED
> >       help
> > diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
> > index 71ac9e36f67c..de4800f09dc7 100644
> > --- a/drivers/video/fbdev/udlfb.c
> > +++ b/drivers/video/fbdev/udlfb.c
> > @@ -341,10 +341,10 @@ static int dlfb_ops_mmap(struct fb_info *info, 
> > struct vm_area_struct *vma)
> >           return -EINVAL;
> >
> >       pos = (unsigned long)info->fix.smem_start + offset;
> > -
> > +#ifdef CONFIG_FB_DEVICE
> >       dev_dbg(info->dev, "mmap() framebuffer addr:%lu size:%lu\n",
> >           pos, size);
> > -
> > +#endif
> >       while (size > 0) {
> >           page = vmalloc_to_pfn((void *)pos);
> >           if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
> > @@ -929,10 +929,10 @@ static int dlfb_ops_open(struct fb_info *info, 
> > int user)
> >           info->fbdefio = fbdefio;
> >           fb_deferred_io_init(info);
> >       }
> > -
> > +#ifdef CONFIG_FB_DEVICE
> >       dev_dbg(info->dev, "open, user=%d fb_info=%p count=%d\n",
> >           user, info, dlfb->fb_count);
> > -
> > +#endif
> >       return 0;
> >   }
> >
> > @@ -982,9 +982,9 @@ static int dlfb_ops_release(struct fb_info *info, 
> > int user)
> >           kfree(info->fbdefio);
> >           info->fbdefio = NULL;
> >       }
> > -
> > +#ifdef CONFIG_FB_DEVICE
> >       dev_dbg(info->dev, "release, user=%d count=%d\n", user, 
> > dlfb->fb_count);
> > -
> > +#endif
> >       return 0;
> >   }
> >
> > @@ -1095,10 +1095,10 @@ static int dlfb_ops_blank(int blank_mode, 
> > struct fb_info *info)
> >       struct dlfb_data *dlfb = info->par;
> >       char *bufptr;
> >       struct urb *urb;
> > -
> > +#ifdef CONFIG_FB_DEVICE
> >       dev_dbg(info->dev, "blank, mode %d --> %d\n",
> >           dlfb->blank_mode, blank_mode);
> > -
> > +#endif
> >       if ((dlfb->blank_mode == FB_BLANK_POWERDOWN) &&
> >           (blank_mode != FB_BLANK_POWERDOWN)) {
> >
> > @@ -1190,7 +1190,9 @@ static int dlfb_realloc_framebuffer(struct 
> > dlfb_data *dlfb, struct fb_info *info
> >            */
> >           new_fb = vmalloc(new_len);
> >           if (!new_fb) {
> > +#ifdef CONFIG_FB_DEVICE
> >               dev_err(info->dev, "Virtual framebuffer alloc failed\n");
> > +#endif
> >               return -ENOMEM;
> >           }
> >           memset(new_fb, 0xff, new_len);
> > @@ -1213,9 +1215,11 @@ static int dlfb_realloc_framebuffer(struct 
> > dlfb_data *dlfb, struct fb_info *info
> >            */
> >           if (shadow)
> >               new_back = vzalloc(new_len);
> > +#ifdef CONFIG_FB_DEVICE
> >           if (!new_back)
> >               dev_info(info->dev,
> >                    "No shadow/backing buffer allocated\n");
> > +#endif
> >           else {
> >               dlfb_deferred_vfree(dlfb, dlfb->backing_buffer);
> >               dlfb->backing_buffer = new_back;
> > @@ -1247,14 +1251,14 @@ static int dlfb_setup_modes(struct dlfb_data 
> > *dlfb,
> >       struct device *dev = info->device;
> >       struct fb_videomode *mode;
> >       const struct fb_videomode *default_vmode = NULL;
> > -
> > +#ifdef CONFIG_FB_DEVICE
> >       if (info->dev) {
> >           /* only use mutex if info has been registered */
> >           mutex_lock(&info->lock);
> >           /* parent device is used otherwise */
> >           dev = info->dev;
> >       }
> > -
> > +#endif
> >       edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
> >       if (!edid) {
> >           result = -ENOMEM;
> > @@ -1375,10 +1379,10 @@ static int dlfb_setup_modes(struct dlfb_data 
> > *dlfb,
> >   error:
> >       if (edid && (dlfb->edid != edid))
> >           kfree(edid);
> > -
> > +#ifdef CONFIG_FB_DEVICE
> >       if (info->dev)
> >           mutex_unlock(&info->lock);
> > -
> > +#endif
> >       return result;
> >   }
> >
> > @@ -1597,8 +1601,10 @@ static int dlfb_parse_vendor_descriptor(struct 
> > dlfb_data *dlfb,
> >   static int dlfb_usb_probe(struct usb_interface *intf,
> >                 const struct usb_device_id *id)
> >   {
> > +#ifdef CONFIG_FB_DEVICE
> >       int i;
> >       const struct device_attribute *attr;
> > +#endif
> >       struct dlfb_data *dlfb;
> >       struct fb_info *info;
> >       int retval;
> > @@ -1701,7 +1707,7 @@ static int dlfb_usb_probe(struct usb_interface 
> > *intf,
> >               retval);
> >           goto error;
> >       }
> > -
> > +#ifdef CONFIG_FB_DEVICE
> >       for (i = 0; i < ARRAY_SIZE(fb_device_attrs); i++) {
> >           attr = &fb_device_attrs[i];
> >           retval = device_create_file(info->dev, attr);
> > @@ -1710,17 +1716,16 @@ static int dlfb_usb_probe(struct 
> > usb_interface *intf,
> >                    "failed to create '%s' attribute: %d\n",
> >                    attr->attr.name, retval);
> >       }
> > -
> >       retval = device_create_bin_file(info->dev, &edid_attr);
> >       if (retval)
> >           dev_warn(info->device, "failed to create '%s' attribute: 
> > %d\n",
> >                edid_attr.attr.name, retval);
> > -
> >       dev_info(info->device,
> >            "%s is DisplayLink USB device (%dx%d, %dK framebuffer 
> > memory)\n",
> >            dev_name(info->dev), info->var.xres, info->var.yres,
> >            ((dlfb->backing_buffer) ?
> >            info->fix.smem_len * 2 : info->fix.smem_len) >> 10);
> > +#endif
> >       return 0;
> >
> >   error:
> > @@ -1737,8 +1742,9 @@ static void dlfb_usb_disconnect(struct 
> > usb_interface *intf)
> >   {
> >       struct dlfb_data *dlfb;
> >       struct fb_info *info;
> > +#ifdef CONFIG_FB_DEVICE
> >       int i;
> > -
> > +#endif
> >       dlfb = usb_get_intfdata(intf);
> >       info = dlfb->info;
> >
> > @@ -1754,10 +1760,11 @@ static void dlfb_usb_disconnect(struct 
> > usb_interface *intf)
> >       dlfb_free_urb_list(dlfb);
> >
> >       /* remove udlfb's sysfs interfaces */
> > +#ifdef CONFIG_FB_DEVICE
> >       for (i = 0; i < ARRAY_SIZE(fb_device_attrs); i++)
> >           device_remove_file(info->dev, &fb_device_attrs[i]);
> >       device_remove_bin_file(info->dev, &edid_attr);
> > -
> > +#endif
> >       unregister_framebuffer(info);
> >   }

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)