[PATCH] fbdev: udl: Make CONFIG_FB_DEVICE optional
gonzalo.silvalde at gmail.com
gonzalo.silvalde at gmail.com
Sun Oct 27 11:13:33 UTC 2024
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 at 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 at 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);
> > }
> >
>
>
More information about the dri-devel
mailing list