| Message ID | 20241025092538.38339-1-gonzalo.silvalde@gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | fbdev: udl: Make CONFIG_FB_DEVICE optional | expand |
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? 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. 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. Helge > Signed-off-by: Gonzalo Silvalde Blanco <gonzalo.silvalde@gmail.com>> --- > 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); > } >
Hi Helge, Thank you for your feedback. I understand the concern about the amount of #ifdefs. I’ll review the code to see if there are other ways to handle the conditional dependencies, perhaps by using fb_dbg() or similar, as you suggested. I agree that keeping readability is important, and I’ll explore options to simplify this. Regarding the reason for removing CONFIG_FB_DEVICE, my understanding is that this would allow the driver to be more flexible without that configuration in certain cases, as mentioned in the GPU TODO [1]. Thomas mentioned I could approach this driver with that in mind. However, if there’s a native DRM driver available that manages this device better, I could focus on other drivers instead. Would you then recommend that I work on drivers without native DRM support for this kind of task? I’d appreciate any specific suggestions on the approach here. Best regards, Gonzalo Silvalde Blanco [1] https://elixir.bootlin.com/linux/v6.11/source/Documentation/gpu/todo.rst#L459 On 25/10/24 17:37, Helge Deller <deller@gmx.de> wrote: > 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? > > 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. > 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. > > Helge > > > Signed-off-by: Gonzalo Silvalde Blanco <gonzalo.silvalde@gmail.com>> --- > > 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); > > } > > > >
Hi Gonzalo, kernel test robot noticed the following build warnings: [auto build test WARNING on drm-misc/drm-misc-next] [also build test WARNING on drm-tip/drm-tip linus/master v6.12-rc4 next-20241025] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Gonzalo-Silvalde-Blanco/fbdev-udl-Make-CONFIG_FB_DEVICE-optional/20241025-172653 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20241025092538.38339-1-gonzalo.silvalde%40gmail.com patch subject: [PATCH] fbdev: udl: Make CONFIG_FB_DEVICE optional config: i386-randconfig-r121-20241027 (https://download.01.org/0day-ci/archive/20241028/202410280002.AXteAJwc-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241028/202410280002.AXteAJwc-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202410280002.AXteAJwc-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/video/fbdev/udlfb.c:1493:38: warning: 'fb_device_attrs' defined but not used [-Wunused-const-variable=] 1493 | static const struct device_attribute fb_device_attrs[] = { | ^~~~~~~~~~~~~~~ >> drivers/video/fbdev/udlfb.c:1485:35: warning: 'edid_attr' defined but not used [-Wunused-const-variable=] 1485 | static const struct bin_attribute edid_attr = { | ^~~~~~~~~ vim +/fb_device_attrs +1493 drivers/video/fbdev/udlfb.c 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1484 598b2eedfc3fbe drivers/video/fbdev/udlfb.c Bhumika Goyal 2017-08-18 @1485 static const struct bin_attribute edid_attr = { 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1486 .attr.name = "edid", 8ef8cc4fca4a92 drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-09-05 1487 .attr.mode = 0666, b9f03a3cd06c6f drivers/video/udlfb.c Paul Mundt 2011-01-06 1488 .size = EDID_LENGTH, 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1489 .read = edid_show, 8ef8cc4fca4a92 drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-09-05 1490 .write = edid_store 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1491 }; 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1492 fa738a5c4b2a6b drivers/video/fbdev/udlfb.c Ladislav Michl 2018-01-16 @1493 static const struct device_attribute fb_device_attrs[] = { 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1494 __ATTR_RO(metrics_bytes_rendered), 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1495 __ATTR_RO(metrics_bytes_identical), 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1496 __ATTR_RO(metrics_bytes_sent), 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1497 __ATTR_RO(metrics_cpu_kcycles_used), 926c11151e3b82 drivers/staging/udlfb/udlfb.c Greg Kroah-Hartman 2010-11-18 1498 __ATTR(metrics_reset, S_IWUSR, NULL, metrics_reset_store), 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1499 }; 7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1500
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@suse.de/ Best regards Thomas > > Helge > >> Signed-off-by: Gonzalo Silvalde Blanco <gonzalo.silvalde@gmail.com>> --- >> 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); >> } >> > >
Hi Gonzalo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm-tip/drm-tip linus/master v6.12-rc5 next-20241028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Gonzalo-Silvalde-Blanco/fbdev-udl-Make-CONFIG_FB_DEVICE-optional/20241025-172653
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20241025092538.38339-1-gonzalo.silvalde%40gmail.com
patch subject: [PATCH] fbdev: udl: Make CONFIG_FB_DEVICE optional
config: riscv-randconfig-r073-20241029 (https://download.01.org/0day-ci/archive/20241029/202410290452.XKXQkwp1-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 5886454669c3c9026f7f27eab13509dd0241f2d6)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241029/202410290452.XKXQkwp1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410290452.XKXQkwp1-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/video/fbdev/udlfb.c:19:
In file included from include/linux/usb.h:16:
In file included from include/linux/interrupt.h:22:
In file included from arch/riscv/include/asm/sections.h:9:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/video/fbdev/udlfb.c:1485:35: warning: unused variable 'edid_attr' [-Wunused-const-variable]
1485 | static const struct bin_attribute edid_attr = {
| ^~~~~~~~~
>> drivers/video/fbdev/udlfb.c:1493:38: warning: unused variable 'fb_device_attrs' [-Wunused-const-variable]
1493 | static const struct device_attribute fb_device_attrs[] = {
| ^~~~~~~~~~~~~~~
3 warnings generated.
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for GET_FREE_REGION
Depends on [n]: SPARSEMEM [=n]
Selected by [m]:
- RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]
vim +/edid_attr +1485 drivers/video/fbdev/udlfb.c
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1484
598b2eedfc3fbe drivers/video/fbdev/udlfb.c Bhumika Goyal 2017-08-18 @1485 static const struct bin_attribute edid_attr = {
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1486 .attr.name = "edid",
8ef8cc4fca4a92 drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-09-05 1487 .attr.mode = 0666,
b9f03a3cd06c6f drivers/video/udlfb.c Paul Mundt 2011-01-06 1488 .size = EDID_LENGTH,
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1489 .read = edid_show,
8ef8cc4fca4a92 drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-09-05 1490 .write = edid_store
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1491 };
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1492
fa738a5c4b2a6b drivers/video/fbdev/udlfb.c Ladislav Michl 2018-01-16 @1493 static const struct device_attribute fb_device_attrs[] = {
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1494 __ATTR_RO(metrics_bytes_rendered),
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1495 __ATTR_RO(metrics_bytes_identical),
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1496 __ATTR_RO(metrics_bytes_sent),
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1497 __ATTR_RO(metrics_cpu_kcycles_used),
926c11151e3b82 drivers/staging/udlfb/udlfb.c Greg Kroah-Hartman 2010-11-18 1498 __ATTR(metrics_reset, S_IWUSR, NULL, metrics_reset_store),
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1499 };
7d9485e2c53caa drivers/staging/udlfb/udlfb.c Bernie Thompson 2010-02-15 1500
Hi Thomas, On 10/28/24 09:41, Thomas Zimmermann wrote: > 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. Yes, but I doubt every of those calls can be replaced... >> 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. Sure, I meant that if people enable a fdev driver, they most likely want /dev/fb as well ..... unless they want to use mostly DRM drivers. >> 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/ The stability was one of the issues, but IMHO the *main* issue he mentions is this: The framebuffer driver is faster, it keeps back buffer and updates only data that differ between the front and back buffer. The DRM driver doesn't have such optimization, it will update everything in a given rectangle - this increases USB traffic and makes video playback more jerky. That's exactly the main concern I'm regularily bringing up and which IMHO is the main reason we still have many fbdev drivers. You added support for some of those graphics cards with native DRM drivers, but all of them are unaccelerated. This hurts a lot on old machines and as such specific cards are ugly slowly with DRM. A good example for this is the kvm drm graphics driver which is sluggish and slow when using KVM. I'm happy to get rid of the fbdev drivers, but for that DRM really needs to allow some sort of native fillrect, copyarea and imageblt operations so that we can get performance back on the old cards when implementing them as DRM driver. Helge
Hi Am 29.10.24 um 21:42 schrieb Helge Deller: > Hi Thomas, > > On 10/28/24 09:41, Thomas Zimmermann wrote: >> 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. > > Yes, but I doubt every of those calls can be replaced... > >>> 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. > > Sure, I meant that if people enable a fdev driver, they most likely > want /dev/fb as well ..... unless they want to use mostly DRM drivers. > >>> 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/ > > The stability was one of the issues, but IMHO the *main* issue he > mentions is this: > > The framebuffer driver is faster, it keeps back buffer and updates only > data that differ between the front and back buffer. The DRM driver > doesn't > have such optimization, it will update everything in a given rectangle - > this increases USB traffic and makes video playback more jerky. If that was a problem, it has long been solved. [1][2] The DRM udl driver keeps a backbuffer in system memory. The DRM API provides built-in damage handling, so that clients can mark the framebuffer regions that have been written. Udl will only update the regions that have been modified. For fbdev support specifically, the fbdev code mmaps the drivers internal backbuffer to userspace and does deferred I/O and damage handling on these pages. Hence, there's only one transfer over USB with no internal copying. There used to be more internal copying, but that is gone. [3] [1] https://patchwork.freedesktop.org/patch/501943/ [2] https://patchwork.freedesktop.org/patch/506133/ [3] https://patchwork.freedesktop.org/patch/590306/?series=131037&rev=4 > > That's exactly the main concern I'm regularily bringing up and which > IMHO is the main reason we still have many fbdev drivers. > You added support for some of those graphics cards with native DRM > drivers, but all of them are unaccelerated. This hurts a lot on old > machines and as such specific cards are ugly slowly with DRM. > A good example for this is the kvm drm graphics driver which is sluggish > and slow when using KVM. > > I'm happy to get rid of the fbdev drivers, but for that DRM really needs > to allow some sort of native fillrect, copyarea and imageblt > operations so > that we can get performance back on the old cards when implementing them > as DRM driver. This is unrelated to udl. Best regards Thomas > > Helge
On 10/30/24 09:33, Thomas Zimmermann wrote: > Hi > > Am 29.10.24 um 21:42 schrieb Helge Deller: >> Hi Thomas, >> >> On 10/28/24 09:41, Thomas Zimmermann wrote: >>> 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. >> >> Yes, but I doubt every of those calls can be replaced... >> >>>> 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. >> >> Sure, I meant that if people enable a fdev driver, they most likely >> want /dev/fb as well ..... unless they want to use mostly DRM drivers. >> >>>> 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/ >> >> The stability was one of the issues, but IMHO the *main* issue he mentions is this: >> >> The framebuffer driver is faster, it keeps back buffer and updates only >> data that differ between the front and back buffer. The DRM driver doesn't >> have such optimization, it will update everything in a given rectangle - >> this increases USB traffic and makes video playback more jerky. > > If that was a problem, it has long been solved. [1][2] The DRM udl driver keeps a backbuffer in system memory. The DRM API provides built-in damage handling, so that clients can mark the framebuffer regions that have been written. Udl will only update the regions that have been modified. > > For fbdev support specifically, the fbdev code mmaps the drivers internal backbuffer to userspace and does deferred I/O and damage handling on these pages. Hence, there's only one transfer over USB with no internal copying. There used to be more internal copying, but that is gone. [3] Sounds good. Maybe you should ask Mikulas if it helps him? > [1] https://patchwork.freedesktop.org/patch/501943/ > [2] https://patchwork.freedesktop.org/patch/506133/ > [3] https://patchwork.freedesktop.org/patch/590306/?series=131037&rev=4 > >> >> That's exactly the main concern I'm regularily bringing up and which >> IMHO is the main reason we still have many fbdev drivers. >> You added support for some of those graphics cards with native DRM >> drivers, but all of them are unaccelerated. This hurts a lot on old >> machines and as such specific cards are ugly slowly with DRM. >> A good example for this is the kvm drm graphics driver which is sluggish >> and slow when using KVM. >> >> I'm happy to get rid of the fbdev drivers, but for that DRM really needs >> to allow some sort of native fillrect, copyarea and imageblt operations so >> that we can get performance back on the old cards when implementing them >> as DRM driver. > > This is unrelated to udl. No, it's not. The udl fbdev driver implements those functions (like the other fbdev drivers) and as such fbcon on top of udl is accelerated, while fbcon on drm drivers is unaccelerated. Helge
Hi Am 30.10.24 um 10:30 schrieb Helge Deller: >>> >>> I'm happy to get rid of the fbdev drivers, but for that DRM really >>> needs >>> to allow some sort of native fillrect, copyarea and imageblt >>> operations so >>> that we can get performance back on the old cards when implementing >>> them >>> as DRM driver. >> >> This is unrelated to udl. > > No, it's not. > The udl fbdev driver implements those functions (like the other fbdev > drivers) > and as such fbcon on top of udl is accelerated, while fbcon on drm > drivers > is unaccelerated. Udlfb uses the regular software implementations to draw into its shadow buffer. It then schedules an update to copy the update over USB to the adapter's internal framebuffer memory. [1] Udl uses exactly the same code pattern and most of the involved helpers. [2] [1] https://elixir.bootlin.com/linux/v6.11.5/source/drivers/video/fbdev/udlfb.c#L1145 [2] https://elixir.bootlin.com/linux/v6.11.5/source/drivers/gpu/drm/drm_fbdev_shmem.c#L39 Best regards Thomas > > Helge
On 11/1/24 09:19, Thomas Zimmermann wrote: > Am 30.10.24 um 10:30 schrieb Helge Deller: >>>> I'm happy to get rid of the fbdev drivers, but for that DRM really needs >>>> to allow some sort of native fillrect, copyarea and imageblt operations so >>>> that we can get performance back on the old cards when implementing them >>>> as DRM driver. >>> >>> This is unrelated to udl. >> >> No, it's not. >> The udl fbdev driver implements those functions (like the other fbdev drivers) >> and as such fbcon on top of udl is accelerated, while fbcon on drm drivers >> is unaccelerated. > > Udlfb uses the regular software implementations to draw into its > shadow buffer. It then schedules an update to copy the update over > USB to the adapter's internal framebuffer memory. [1] Udl uses > exactly the same code pattern and most of the involved helpers. [2]> [1] https://elixir.bootlin.com/linux/v6.11.5/source/drivers/video/fbdev/udlfb.c#L1145 > [2] https://elixir.bootlin.com/linux/v6.11.5/source/drivers/gpu/drm/drm_fbdev_shmem.c#L39 Yes, you are correct with this summary for those drivers which use the damage helpers. Maybe the previous udlfb driver had one additional optimization where it might bitblt the screen when scrolling, but this is just an assumption I did not check now. I don't have that card, but if Mikulas can test and verify that the drm driver is now ok for him, I'm fine that we drop udlfb. Please note that my statement above that DRM should support native fillrect, copyarea and imageblt operations is still true. Without those fbcon is too slow and flickering on old machines and fbdev cards. Helge
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); }
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. Signed-off-by: Gonzalo Silvalde Blanco <gonzalo.silvalde@gmail.com> --- drivers/video/fbdev/Kconfig | 1 - drivers/video/fbdev/udlfb.c | 41 ++++++++++++++++++++++--------------- 2 files changed, 24 insertions(+), 18 deletions(-)