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

Hi Helge

Am 25.10.24 um 17:37 schrieb Helge Deller:

On 10/25/24 11:25, Gonzalo Silvalde Blanco wrote:

quoted

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@suse.de/ (local)

Best regards
Thomas

Helge

quoted

Signed-off-by: Gonzalo Silvalde Blanco <redacted>> ---
  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)