[PATCH v2 18/19] fbdev: Move I/O read and write code into helper functions
Thomas Zimmermann
tzimmermann at suse.de
Tue May 2 16:42:14 UTC 2023
Hi Sam
Am 30.04.23 um 20:14 schrieb Sam Ravnborg:
> Hi Thomas,
>
> On Fri, Apr 28, 2023 at 02:24:51PM +0200, Thomas Zimmermann wrote:
>> Move the existing I/O read and write code for I/O memory into
>> the new helpers fb_cfb_read() and fb_cfb_write(). Make them the
> You may want to update the changelog to mention the new names.
>
> A few comments below, but reading it once more I realize this are
> all comments to the original code, so not something to fix in this
> patch.
Yes, it's probably worth a follow-up patch.
>
>
>> default fp_ops. No functional changes.
>>
>> In the near term, the new functions will be useful to the DRM
>> subsystem, which currently provides it's own implementation. It
>> can then use the shared code. In the longer term, it might make
>> sense to revise the I/O helper's default status and make them
>> opt-in by the driver. Systems that don't use them would not
>> contain the code any longer.
>>
>> v2:
>> * add detailed commit message (Javier)
>> * rename fb_cfb_() to fb_io_() (Geert)
>> * add fixes that got lost while moving the code (Geert)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Tested-by: Sui Jingfeng <suijingfeng at loongson.cn>
>> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
>> Acked-by: Helge Deller <deller at gmx.de>
> Reviewed-by: Sam Ravnborg <sam at ravnborg.org>
Thanks for reviewing. If nothing else comes in, I'll probably merge this
series in the next few days.
Best regards
Thomas
>> ---
>> drivers/video/fbdev/core/Makefile | 2 +-
>> drivers/video/fbdev/core/fb_io_fops.c | 133 ++++++++++++++++++++++++++
>> drivers/video/fbdev/core/fbmem.c | 118 +----------------------
>> include/linux/fb.h | 10 ++
>> 4 files changed, 146 insertions(+), 117 deletions(-)
>> create mode 100644 drivers/video/fbdev/core/fb_io_fops.c
>>
>> diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
>> index 08fabce76b74..8f0060160ffb 100644
>> --- a/drivers/video/fbdev/core/Makefile
>> +++ b/drivers/video/fbdev/core/Makefile
>> @@ -2,7 +2,7 @@
>> obj-$(CONFIG_FB_NOTIFY) += fb_notify.o
>> obj-$(CONFIG_FB) += fb.o
>> fb-y := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
>> - modedb.o fbcvt.o fb_cmdline.o
>> + modedb.o fbcvt.o fb_cmdline.o fb_io_fops.o
>> fb-$(CONFIG_FB_DEFERRED_IO) += fb_defio.o
>>
>> ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
>> diff --git a/drivers/video/fbdev/core/fb_io_fops.c b/drivers/video/fbdev/core/fb_io_fops.c
>> new file mode 100644
>> index 000000000000..f5299d50f33b
>> --- /dev/null
>> +++ b/drivers/video/fbdev/core/fb_io_fops.c
>> @@ -0,0 +1,133 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/fb.h>
>> +#include <linux/module.h>
>> +#include <linux/uaccess.h>
>> +
>> +ssize_t fb_io_read(struct fb_info *info, char __user *buf, size_t count, loff_t *ppos)
>> +{
>> + unsigned long p = *ppos;
>> + u8 *buffer, *dst;
>> + u8 __iomem *src;
>> + int c, cnt = 0, err = 0;
>> + unsigned long total_size, trailing;
>> +
>> + if (!info->screen_base)
>> + return -ENODEV;
>> +
>> + total_size = info->screen_size;
>> +
>> + if (total_size == 0)
>> + total_size = info->fix.smem_len;
>> +
>> + if (p >= total_size)
>> + return 0;
>> +
>> + if (count >= total_size)
>> + count = total_size;
>> +
>> + if (count + p > total_size)
>> + count = total_size - p;
>> +
>> + buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
>> + GFP_KERNEL);
>> + if (!buffer)
>> + return -ENOMEM;
>> +
>> + src = (u8 __iomem *) (info->screen_base + p);
> screen_base is char __iomem *, so I think the cast can go away.
>> +
>> + if (info->fbops->fb_sync)
>> + info->fbops->fb_sync(info);
>> +
>> + while (count) {
>> + c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>> + dst = buffer;
>> + fb_memcpy_fromfb(dst, src, c);
>> + dst += c;
> I see no reason to use dst here, as it is always equal to buffer when
> used.
> But this is copied from the original code, so it would be wrong to
> change this in the current patch. I just noticed.
>
>> + src += c;
>> +
>> + trailing = copy_to_user(buf, buffer, c);
>> + if (trailing == c) {
>> + err = -EFAULT;
>> + break;
>> + }
>> + c -= trailing;
>> +
>> + *ppos += c;
>> + buf += c;
>> + cnt += c;
>> + count -= c;
>> + }
>> +
>> + kfree(buffer);
>> +
>> + return cnt ? cnt : err;
>> +}
>> +EXPORT_SYMBOL(fb_io_read);
>> +
>> +ssize_t fb_io_write(struct fb_info *info, const char __user *buf, size_t count, loff_t *ppos)
>> +{
>> + unsigned long p = *ppos;
>> + u8 *buffer, *src;
>> + u8 __iomem *dst;
>> + int c, cnt = 0, err = 0;
>> + unsigned long total_size, trailing;
>> +
>> + if (!info->screen_base)
>> + return -ENODEV;
>> +
>> + total_size = info->screen_size;
>> +
>> + if (total_size == 0)
>> + total_size = info->fix.smem_len;
>> +
>> + if (p > total_size)
>> + return -EFBIG;
>> +
>> + if (count > total_size) {
>> + err = -EFBIG;
>> + count = total_size;
>> + }
>> +
>> + if (count + p > total_size) {
>> + if (!err)
>> + err = -ENOSPC;
>> +
>> + count = total_size - p;
>> + }
>> +
>> + buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
>> + GFP_KERNEL);
>> + if (!buffer)
>> + return -ENOMEM;
>> +
>> + dst = (u8 __iomem *) (info->screen_base + p);
> Cast can go away.
>
>> +
>> + if (info->fbops->fb_sync)
>> + info->fbops->fb_sync(info);
>> +
>> + while (count) {
>> + c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>> + src = buffer;
>> +
>> + trailing = copy_from_user(src, buf, c);
>> + if (trailing == c) {
>> + err = -EFAULT;
>> + break;
>> + }
>> + c -= trailing;
>> +
>> + fb_memcpy_tofb(dst, src, c);
>> + dst += c;
>> + src += c;
> src is incremented here, but the value are not used.
> Again, from the original code so not something to change here.
>
>> + *ppos += c;
>> + buf += c;
>> + cnt += c;
>> + count -= c;
>> + }
>> +
>> + kfree(buffer);
>> +
>> + return (cnt) ? cnt : err;
>> +}
>> +EXPORT_SYMBOL(fb_io_write);
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 3a80d13afd26..4035a57df116 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -761,12 +761,7 @@ static struct fb_info *file_fb_info(struct file *file)
>> static ssize_t
>> fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>> {
>> - unsigned long p = *ppos;
>> struct fb_info *info = file_fb_info(file);
>> - u8 *buffer, *dst;
>> - u8 __iomem *src;
>> - int c, cnt = 0, err = 0;
>> - unsigned long total_size, trailing;
>>
>> if (!info)
>> return -ENODEV;
>> @@ -777,67 +772,13 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>> if (info->fbops->fb_read)
>> return info->fbops->fb_read(info, buf, count, ppos);
>>
>> - if (!info->screen_base)
>> - return -ENODEV;
>> -
>> - total_size = info->screen_size;
>> -
>> - if (total_size == 0)
>> - total_size = info->fix.smem_len;
>> -
>> - if (p >= total_size)
>> - return 0;
>> -
>> - if (count >= total_size)
>> - count = total_size;
>> -
>> - if (count + p > total_size)
>> - count = total_size - p;
>> -
>> - buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
>> - GFP_KERNEL);
>> - if (!buffer)
>> - return -ENOMEM;
>> -
>> - src = (u8 __iomem *) (info->screen_base + p);
>> -
>> - if (info->fbops->fb_sync)
>> - info->fbops->fb_sync(info);
>> -
>> - while (count) {
>> - c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>> - dst = buffer;
>> - fb_memcpy_fromfb(dst, src, c);
>> - dst += c;
>> - src += c;
>> -
>> - trailing = copy_to_user(buf, buffer, c);
>> - if (trailing == c) {
>> - err = -EFAULT;
>> - break;
>> - }
>> - c -= trailing;
>> -
>> - *ppos += c;
>> - buf += c;
>> - cnt += c;
>> - count -= c;
>> - }
>> -
>> - kfree(buffer);
>> -
>> - return cnt ? cnt : err;
>> + return fb_io_read(info, buf, count, ppos);
>> }
>>
>> static ssize_t
>> fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>> {
>> - unsigned long p = *ppos;
>> struct fb_info *info = file_fb_info(file);
>> - u8 *buffer, *src;
>> - u8 __iomem *dst;
>> - int c, cnt = 0, err = 0;
>> - unsigned long total_size, trailing;
>>
>> if (!info)
>> return -ENODEV;
>> @@ -848,62 +789,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>> if (info->fbops->fb_write)
>> return info->fbops->fb_write(info, buf, count, ppos);
>>
>> - if (!info->screen_base)
>> - return -ENODEV;
>> -
>> - total_size = info->screen_size;
>> -
>> - if (total_size == 0)
>> - total_size = info->fix.smem_len;
>> -
>> - if (p > total_size)
>> - return -EFBIG;
>> -
>> - if (count > total_size) {
>> - err = -EFBIG;
>> - count = total_size;
>> - }
>> -
>> - if (count + p > total_size) {
>> - if (!err)
>> - err = -ENOSPC;
>> -
>> - count = total_size - p;
>> - }
>> -
>> - buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
>> - GFP_KERNEL);
>> - if (!buffer)
>> - return -ENOMEM;
>> -
>> - dst = (u8 __iomem *) (info->screen_base + p);
>> -
>> - if (info->fbops->fb_sync)
>> - info->fbops->fb_sync(info);
>> -
>> - while (count) {
>> - c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>> - src = buffer;
>> -
>> - trailing = copy_from_user(src, buf, c);
>> - if (trailing == c) {
>> - err = -EFAULT;
>> - break;
>> - }
>> - c -= trailing;
>> -
>> - fb_memcpy_tofb(dst, src, c);
>> - dst += c;
>> - src += c;
>> - *ppos += c;
>> - buf += c;
>> - cnt += c;
>> - count -= c;
>> - }
>> -
>> - kfree(buffer);
>> -
>> - return (cnt) ? cnt : err;
>> + return fb_io_write(info, buf, count, ppos);
>> }
>>
>> int
>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>> index 08cb47da71f8..ec978a4969a9 100644
>> --- a/include/linux/fb.h
>> +++ b/include/linux/fb.h
>> @@ -576,9 +576,19 @@ struct fb_info {
>> extern int fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var);
>> extern int fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var);
>> extern int fb_blank(struct fb_info *info, int blank);
>> +
>> +/*
>> + * Drawing operations where framebuffer is in I/O memory
>> + */
>> +
>> extern void cfb_fillrect(struct fb_info *info, const struct fb_fillrect *rect);
>> extern void cfb_copyarea(struct fb_info *info, const struct fb_copyarea *area);
>> extern void cfb_imageblit(struct fb_info *info, const struct fb_image *image);
>> +extern ssize_t fb_io_read(struct fb_info *info, char __user *buf,
>> + size_t count, loff_t *ppos);
>> +extern ssize_t fb_io_write(struct fb_info *info, const char __user *buf,
>> + size_t count, loff_t *ppos);
>> +
>> /*
>> * Drawing operations where framebuffer is in system RAM
>> */
>> --
>> 2.40.0
--
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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230502/e927f734/attachment.sig>
More information about the dri-devel
mailing list