diff mbox

[18/21] udlfb: allow reallocating the framebuffer

Message ID 20180603144225.463043082@twibright.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mikulas Patocka June 3, 2018, 2:41 p.m. UTC 
This patch changes udlfb so that it may reallocate the framebuffer when
setting higher-resolution mode. If we boot the system without monitor
attached, udlfb creates a framebuffer with the size 800x600. This patch
makes it possible to select higher videomode with the fbset command when
a monitor is attached.

Note that there is no reliable way to prevent the system from touching the
old framebuffer, so we must not free it. We add it to the list
dlfb->deferred_free and free it when the driver is unloaded.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/video/fbdev/udlfb.c |   70 +++++++++++++++++++++++++++++---------------
 include/video/udlfb.h       |    1 
 2 files changed, 48 insertions(+), 23 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

kernel test robot June 3, 2018, 7:24 p.m. UTC | #1 
Hi Mikulas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.17-rc7 next-20180601]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mikulas-Patocka/USB-DisplayLink-patches/20180603-233013
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)


vim +1198 drivers/video/fbdev/udlfb.c

  1171	
  1172	/*
  1173	 * Assumes &info->lock held by caller
  1174	 * Assumes no active clients have framebuffer open
  1175	 */
  1176	static int dlfb_realloc_framebuffer(struct dlfb_data *dlfb, struct fb_info *info, u32 new_len)
  1177	{
  1178		u32 old_len = info->fix.smem_len;
> 1179		unsigned char *old_fb = info->screen_base;
  1180		unsigned char *new_fb;
  1181		unsigned char *new_back = NULL;
  1182	
  1183		new_len = PAGE_ALIGN(new_len);
  1184	
  1185		if (new_len > old_len) {
  1186			/*
  1187			 * Alloc system memory for virtual framebuffer
  1188			 */
  1189			new_fb = vmalloc(new_len);
  1190			if (!new_fb) {
  1191				dev_err(info->dev, "Virtual framebuffer alloc failed\n");
  1192				return -ENOMEM;
  1193			}
  1194			memset(new_fb, 0xff, new_len);
  1195	
  1196			if (info->screen_base) {
  1197				memcpy(new_fb, old_fb, old_len);
> 1198				dlfb_deferred_vfree(dlfb, info->screen_base);
  1199			}
  1200	
  1201			info->screen_base = new_fb;
  1202			info->fix.smem_len = new_len;
  1203			info->fix.smem_start = (unsigned long) new_fb;
  1204			info->flags = udlfb_info_flags;
  1205	
  1206			/*
  1207			 * Second framebuffer copy to mirror the framebuffer state
  1208			 * on the physical USB device. We can function without this.
  1209			 * But with imperfect damage info we may send pixels over USB
  1210			 * that were, in fact, unchanged - wasting limited USB bandwidth
  1211			 */
  1212			if (shadow)
  1213				new_back = vzalloc(new_len);
  1214			if (!new_back)
  1215				dev_info(info->dev,
  1216					 "No shadow/backing buffer allocated\n");
  1217			else {
  1218				dlfb_deferred_vfree(dlfb, dlfb->backing_buffer);
  1219				dlfb->backing_buffer = new_back;
  1220			}
  1221		}
  1222		return 0;
  1223	}
  1224	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikulas Patocka June 12, 2018, 4:32 p.m. UTC | #2 
On Mon, 4 Jun 2018, kbuild test robot wrote:

> Hi Mikulas,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on drm/drm-next]
> [also build test WARNING on v4.17-rc7 next-20180601]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Mikulas-Patocka/USB-DisplayLink-patches/20180603-233013

What is it really complaining about? That URL shows 404 Not Found and this 
email has no warnings at all.

> base:   git://people.freedesktop.org/~airlied/linux.git drm-next
> reproduce:
>         # apt-get install sparse
>         make ARCH=x86_64 allmodconfig
>         make C=1 CF=-D__CHECK_ENDIAN__
> 
> 
> sparse warnings: (new ones prefixed by >>)
> 
> 
> vim +1198 drivers/video/fbdev/udlfb.c
> 
>   1171	
>   1172	/*
>   1173	 * Assumes &info->lock held by caller
>   1174	 * Assumes no active clients have framebuffer open
>   1175	 */
>   1176	static int dlfb_realloc_framebuffer(struct dlfb_data *dlfb, struct fb_info *info, u32 new_len)
>   1177	{
>   1178		u32 old_len = info->fix.smem_len;
> > 1179		unsigned char *old_fb = info->screen_base;
>   1180		unsigned char *new_fb;
>   1181		unsigned char *new_back = NULL;
>   1182	
>   1183		new_len = PAGE_ALIGN(new_len);
>   1184	
>   1185		if (new_len > old_len) {
>   1186			/*
>   1187			 * Alloc system memory for virtual framebuffer
>   1188			 */
>   1189			new_fb = vmalloc(new_len);
>   1190			if (!new_fb) {
>   1191				dev_err(info->dev, "Virtual framebuffer alloc failed\n");
>   1192				return -ENOMEM;
>   1193			}
>   1194			memset(new_fb, 0xff, new_len);
>   1195	
>   1196			if (info->screen_base) {
>   1197				memcpy(new_fb, old_fb, old_len);
> > 1198				dlfb_deferred_vfree(dlfb, info->screen_base);
>   1199			}
>   1200	
>   1201			info->screen_base = new_fb;
>   1202			info->fix.smem_len = new_len;
>   1203			info->fix.smem_start = (unsigned long) new_fb;
>   1204			info->flags = udlfb_info_flags;
>   1205	
>   1206			/*
>   1207			 * Second framebuffer copy to mirror the framebuffer state
>   1208			 * on the physical USB device. We can function without this.
>   1209			 * But with imperfect damage info we may send pixels over USB
>   1210			 * that were, in fact, unchanged - wasting limited USB bandwidth
>   1211			 */
>   1212			if (shadow)
>   1213				new_back = vzalloc(new_len);
>   1214			if (!new_back)
>   1215				dev_info(info->dev,
>   1216					 "No shadow/backing buffer allocated\n");
>   1217			else {
>   1218				dlfb_deferred_vfree(dlfb, dlfb->backing_buffer);
>   1219				dlfb->backing_buffer = new_back;
>   1220			}
>   1221		}
>   1222		return 0;
>   1223	}
>   1224	
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz July 3, 2018, 2:58 p.m. UTC | #3 
On Tuesday, June 12, 2018 12:32:34 PM Mikulas Patocka wrote:
> 
> On Mon, 4 Jun 2018, kbuild test robot wrote:
> 
> > Hi Mikulas,
> > 
> > I love your patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on drm/drm-next]
> > [also build test WARNING on v4.17-rc7 next-20180601]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Mikulas-Patocka/USB-DisplayLink-patches/20180603-233013
> 
> What is it really complaining about? That URL shows 404 Not Found and this 
> email has no warnings at all.

screen_base in struct fb_info is annotated with __iomem tag:

...
		char __iomem *screen_base;	/* Virtual address */
...

and this tag should be preserved (or explicitly casted).

> > base:   git://people.freedesktop.org/~airlied/linux.git drm-next
> > reproduce:
> >         # apt-get install sparse
> >         make ARCH=x86_64 allmodconfig
> >         make C=1 CF=-D__CHECK_ENDIAN__

You should be able to reproduce the issue with the above sequence.

> > sparse warnings: (new ones prefixed by >>)

[...]

> >   1178		u32 old_len = info->fix.smem_len;
> > > 1179		unsigned char *old_fb = info->screen_base;
> >   1180		unsigned char *new_fb;

[...]

> >   1196			if (info->screen_base) {
> >   1197				memcpy(new_fb, old_fb, old_len);
> > > 1198				dlfb_deferred_vfree(dlfb, info->screen_base);
> >   1199			}

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: linux-4.17-rc7/drivers/video/fbdev/udlfb.c
===================================================================
--- linux-4.17-rc7.orig/drivers/video/fbdev/udlfb.c	2018-06-03 13:17:41.000000000 +0200
+++ linux-4.17-rc7/drivers/video/fbdev/udlfb.c	2018-06-03 13:17:41.000000000 +0200
@@ -73,6 +73,13 @@  static bool fb_defio = 1;  /* Detect mma
 static bool shadow = 1; /* Optionally disable shadow framebuffer */
 static int pixel_limit; /* Optionally force a pixel resolution limit */
 
+struct dlfb_deferred_free {
+	struct list_head list;
+	void *mem;
+};
+
+static int dlfb_realloc_framebuffer(struct dlfb_data *dlfb, struct fb_info *info, u32 new_len);
+
 /* dlfb keeps a list of urbs for efficient bulk transfers */
 static void dlfb_urb_completion(struct urb *urb);
 static struct urb *dlfb_get_urb(struct dlfb_data *dlfb);
@@ -927,6 +934,12 @@  static void dlfb_free(struct kref *kref)
 {
 	struct dlfb_data *dlfb = container_of(kref, struct dlfb_data, kref);
 
+	while (!list_empty(&dlfb->deferred_free)) {
+		struct dlfb_deferred_free *d = list_entry(dlfb->deferred_free.next, struct dlfb_deferred_free, list);
+		list_del(&d->list);
+		vfree(d->mem);
+		kfree(d);
+	}
 	vfree(dlfb->backing_buffer);
 	kfree(dlfb->edid);
 	kfree(dlfb);
@@ -1020,10 +1033,6 @@  static int dlfb_ops_check_var(struct fb_
 	struct fb_videomode mode;
 	struct dlfb_data *dlfb = info->par;
 
-	/* TODO: support dynamically changing framebuffer size */
-	if ((var->xres * var->yres * 2) > info->fix.smem_len)
-		return -EINVAL;
-
 	/* set device-specific elements of var unrelated to mode */
 	dlfb_var_color_format(var);
 
@@ -1042,6 +1051,7 @@  static int dlfb_ops_set_par(struct fb_in
 	u16 *pix_framebuffer;
 	int i;
 	struct fb_var_screeninfo fvs;
+	u32 line_length = info->var.xres * (info->var.bits_per_pixel / 8);
 
 	/* clear the activate field because it causes spurious miscompares */
 	fvs = info->var;
@@ -1051,13 +1061,17 @@  static int dlfb_ops_set_par(struct fb_in
 	if (!memcmp(&dlfb->current_mode, &fvs, sizeof(struct fb_var_screeninfo)))
 		return 0;
 
+	result = dlfb_realloc_framebuffer(dlfb, info, info->var.yres * line_length);
+	if (result)
+		return result;
+
 	result = dlfb_set_video_mode(dlfb, &info->var);
 
 	if (result)
 		return result;
 
 	dlfb->current_mode = fvs;
-	info->fix.line_length = info->var.xres * (info->var.bits_per_pixel / 8);
+	info->fix.line_length = line_length;
 
 	if (dlfb->fb_count == 0) {
 
@@ -1066,11 +1080,11 @@  static int dlfb_ops_set_par(struct fb_in
 		pix_framebuffer = (u16 *) info->screen_base;
 		for (i = 0; i < info->fix.smem_len / 2; i++)
 			pix_framebuffer[i] = 0x37e6;
-
-		dlfb_handle_damage(dlfb, 0, 0, info->var.xres, info->var.yres,
-				   info->screen_base);
 	}
 
+	dlfb_handle_damage(dlfb, 0, 0, info->var.xres, info->var.yres,
+			   info->screen_base);
+
 	return 0;
 }
 
@@ -1146,21 +1160,29 @@  static struct fb_ops dlfb_ops = {
 };
 
 
+static void dlfb_deferred_vfree(struct dlfb_data *dlfb, void *mem)
+{
+	struct dlfb_deferred_free *d = kmalloc(sizeof(struct dlfb_deferred_free), GFP_KERNEL);
+	if (!d)
+		return;
+	d->mem = mem;
+	list_add(&d->list, &dlfb->deferred_free);
+}
+
 /*
  * Assumes &info->lock held by caller
  * Assumes no active clients have framebuffer open
  */
-static int dlfb_realloc_framebuffer(struct dlfb_data *dlfb, struct fb_info *info)
+static int dlfb_realloc_framebuffer(struct dlfb_data *dlfb, struct fb_info *info, u32 new_len)
 {
-	int old_len = info->fix.smem_len;
-	int new_len;
+	u32 old_len = info->fix.smem_len;
 	unsigned char *old_fb = info->screen_base;
 	unsigned char *new_fb;
 	unsigned char *new_back = NULL;
 
-	new_len = info->fix.line_length * info->var.yres;
+	new_len = PAGE_ALIGN(new_len);
 
-	if (PAGE_ALIGN(new_len) > old_len) {
+	if (new_len > old_len) {
 		/*
 		 * Alloc system memory for virtual framebuffer
 		 */
@@ -1169,14 +1191,15 @@  static int dlfb_realloc_framebuffer(stru
 			dev_err(info->dev, "Virtual framebuffer alloc failed\n");
 			return -ENOMEM;
 		}
+		memset(new_fb, 0xff, new_len);
 
 		if (info->screen_base) {
 			memcpy(new_fb, old_fb, old_len);
-			vfree(info->screen_base);
+			dlfb_deferred_vfree(dlfb, info->screen_base);
 		}
 
 		info->screen_base = new_fb;
-		info->fix.smem_len = PAGE_ALIGN(new_len);
+		info->fix.smem_len = new_len;
 		info->fix.smem_start = (unsigned long) new_fb;
 		info->flags = udlfb_info_flags;
 
@@ -1192,7 +1215,7 @@  static int dlfb_realloc_framebuffer(stru
 			dev_info(info->dev,
 				 "No shadow/backing buffer allocated\n");
 		else {
-			vfree(dlfb->backing_buffer);
+			dlfb_deferred_vfree(dlfb, dlfb->backing_buffer);
 			dlfb->backing_buffer = new_back;
 		}
 	}
@@ -1344,11 +1367,6 @@  static int dlfb_setup_modes(struct dlfb_
 		 * with mode size info, we can now alloc our framebuffer.
 		 */
 		memcpy(&info->fix, &dlfb_fix, sizeof(dlfb_fix));
-		info->fix.line_length = info->var.xres *
-			(info->var.bits_per_pixel / 8);
-
-		result = dlfb_realloc_framebuffer(dlfb, info);
-
 	} else
 		result = -EINVAL;
 
@@ -1436,7 +1454,10 @@  static ssize_t edid_store(
 	if (!dlfb->edid || memcmp(src, dlfb->edid, src_size))
 		return -EINVAL;
 
-	dlfb_ops_set_par(fb_info);
+	ret = dlfb_ops_set_par(fb_info);
+	if (ret)
+		return ret;
+
 	return src_size;
 }
 
@@ -1596,6 +1617,7 @@  static int dlfb_usb_probe(struct usb_int
 	}
 
 	kref_init(&dlfb->kref); /* matching kref_put in usb .disconnect fn */
+	INIT_LIST_HEAD(&dlfb->deferred_free);
 
 	dlfb->udev = usbdev;
 	usb_set_intfdata(intf, dlfb);
@@ -1693,7 +1715,9 @@  static void dlfb_init_framebuffer_work(s
 	dlfb_select_std_channel(dlfb);
 
 	dlfb_ops_check_var(&info->var, info);
-	dlfb_ops_set_par(info);
+	retval = dlfb_ops_set_par(info);
+	if (retval)
+		goto error;
 
 	retval = register_framebuffer(info);
 	if (retval < 0) {
Index: linux-4.17-rc7/include/video/udlfb.h
===================================================================
--- linux-4.17-rc7.orig/include/video/udlfb.h	2018-06-03 13:17:41.000000000 +0200
+++ linux-4.17-rc7/include/video/udlfb.h	2018-06-03 13:17:41.000000000 +0200
@@ -58,6 +58,7 @@  struct dlfb_data {
 	atomic_t bytes_sent; /* to usb, after compression including overhead */
 	atomic_t cpu_kcycles_used; /* transpired during pixel processing */
 	struct fb_var_screeninfo current_mode;
+	struct list_head deferred_free;
 };
 
 #define NR_USB_REQUEST_I2C_SUB_IO 0x02