| Message ID | 20230828132131.29295-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | fbdev: Use helpers for deferred I/O | expand |
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas, > Generate callback functions for struct fb_ops with the fbdev macro > FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(). Initialize struct fb_ops to > the generated functions with fbdev initializer macros. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Steve Glendinning <steve.glendinning@shawell.net> > --- The patch looks good to me, but I've a question below. Acked-by: Javier Martinez Canillas <javierm@redhat.com> > drivers/video/fbdev/smscufx.c | 85 +++++++++-------------------------- > 1 file changed, 22 insertions(+), 63 deletions(-) > > diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c [...] > static const struct fb_ops ufx_ops = { > .owner = THIS_MODULE, > - .fb_read = fb_sys_read, > - .fb_write = ufx_ops_write, > + __FB_DEFAULT_DEFERRED_OPS_RDWR(ufx_ops), > .fb_setcolreg = ufx_ops_setcolreg, > - .fb_fillrect = ufx_ops_fillrect, > - .fb_copyarea = ufx_ops_copyarea, > - .fb_imageblit = ufx_ops_imageblit, > + __FB_DEFAULT_DEFERRED_OPS_DRAW(ufx_ops), > .fb_mmap = ufx_ops_mmap, There are no generated functions for .fb_mmap, I wonder what's the value of __FB_DEFAULT_DEFERRED_OPS_MMAP() ? Maybe just removing that macro and setting .fb_mmap = fb_deferred_io_mmap instead if there's no custom mmap handler would be easier to read ? Alternatively, __FB_DEFAULT_DEFERRED_OPS_MMAP() could still be left but not taking a __prefix argument since that is not used anyways ?
Hi Javier Am 04.09.23 um 14:59 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > > Hello Thomas, > >> Generate callback functions for struct fb_ops with the fbdev macro >> FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(). Initialize struct fb_ops to >> the generated functions with fbdev initializer macros. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: Steve Glendinning <steve.glendinning@shawell.net> >> --- > > The patch looks good to me, but I've a question below. > > Acked-by: Javier Martinez Canillas <javierm@redhat.com> > >> drivers/video/fbdev/smscufx.c | 85 +++++++++-------------------------- >> 1 file changed, 22 insertions(+), 63 deletions(-) >> >> diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c > > [...] > >> static const struct fb_ops ufx_ops = { >> .owner = THIS_MODULE, >> - .fb_read = fb_sys_read, >> - .fb_write = ufx_ops_write, >> + __FB_DEFAULT_DEFERRED_OPS_RDWR(ufx_ops), >> .fb_setcolreg = ufx_ops_setcolreg, >> - .fb_fillrect = ufx_ops_fillrect, >> - .fb_copyarea = ufx_ops_copyarea, >> - .fb_imageblit = ufx_ops_imageblit, >> + __FB_DEFAULT_DEFERRED_OPS_DRAW(ufx_ops), >> .fb_mmap = ufx_ops_mmap, > > There are no generated functions for .fb_mmap, I wonder what's the value > of __FB_DEFAULT_DEFERRED_OPS_MMAP() ? Maybe just removing that macro and > setting .fb_mmap = fb_deferred_io_mmap instead if there's no custom mmap > handler would be easier to read ? At least two drivers could use __FB_DEFAULT_DEFERRED_OPS_MMAP: picolcd-fb and hyperv_fb. At some point, we might want to set/clear fb_mmap depending on some Kconfig value. Having __FB_DEFAULT_DEFERRED_OPS_MMAP might be helpful then. > > Alternatively, __FB_DEFAULT_DEFERRED_OPS_MMAP() could still be left but > not taking a __prefix argument since that is not used anyways ? The driver optionally provides mmap without deferred I/O, hence the mmap function. That makes no sense, as these writes to the buffer would never make it to the device memory. But I didn't want to remove the code either. So I just left the existing function as-is. Usually, the deferred-I/O mmap is called immediately. [1] Best regards Thomas [1] https://elixir.bootlin.com/linux/v6.5.1/source/drivers/video/fbdev/smscufx.c#L784 >
Am 04.09.23 um 16:39 schrieb Thomas Zimmermann: [...] > At least two drivers could use __FB_DEFAULT_DEFERRED_OPS_MMAP: > picolcd-fb and hyperv_fb. At some point, we might want to set/clear Both drivers are already in this patchset. > fb_mmap depending on some Kconfig value. Having > __FB_DEFAULT_DEFERRED_OPS_MMAP might be helpful then. > >> >> Alternatively, __FB_DEFAULT_DEFERRED_OPS_MMAP() could still be left but >> not taking a __prefix argument since that is not used anyways ? > > The driver optionally provides mmap without deferred I/O, hence the mmap > function. That makes no sense, as these writes to the buffer would never > make it to the device memory. But I didn't want to remove the code > either. So I just left the existing function as-is. Usually, the > deferred-I/O mmap is called immediately. [1] > > Best regards > Thomas > > [1] > https://elixir.bootlin.com/linux/v6.5.1/source/drivers/video/fbdev/smscufx.c#L784 > >> >
Thomas Zimmermann <tzimmermann@suse.de> writes: > Hi Javier > > Am 04.09.23 um 14:59 schrieb Javier Martinez Canillas: >> Thomas Zimmermann <tzimmermann@suse.de> writes: >> >> Hello Thomas, >> >>> Generate callback functions for struct fb_ops with the fbdev macro >>> FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(). Initialize struct fb_ops to >>> the generated functions with fbdev initializer macros. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Cc: Steve Glendinning <steve.glendinning@shawell.net> >>> --- >> >> The patch looks good to me, but I've a question below. >> >> Acked-by: Javier Martinez Canillas <javierm@redhat.com> >> >>> drivers/video/fbdev/smscufx.c | 85 +++++++++-------------------------- >>> 1 file changed, 22 insertions(+), 63 deletions(-) >>> >>> diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c >> >> [...] >> >>> static const struct fb_ops ufx_ops = { >>> .owner = THIS_MODULE, >>> - .fb_read = fb_sys_read, >>> - .fb_write = ufx_ops_write, >>> + __FB_DEFAULT_DEFERRED_OPS_RDWR(ufx_ops), >>> .fb_setcolreg = ufx_ops_setcolreg, >>> - .fb_fillrect = ufx_ops_fillrect, >>> - .fb_copyarea = ufx_ops_copyarea, >>> - .fb_imageblit = ufx_ops_imageblit, >>> + __FB_DEFAULT_DEFERRED_OPS_DRAW(ufx_ops), >>> .fb_mmap = ufx_ops_mmap, >> >> There are no generated functions for .fb_mmap, I wonder what's the value >> of __FB_DEFAULT_DEFERRED_OPS_MMAP() ? Maybe just removing that macro and >> setting .fb_mmap = fb_deferred_io_mmap instead if there's no custom mmap >> handler would be easier to read ? > > At least two drivers could use __FB_DEFAULT_DEFERRED_OPS_MMAP: > picolcd-fb and hyperv_fb. At some point, we might want to set/clear > fb_mmap depending on some Kconfig value. Having > __FB_DEFAULT_DEFERRED_OPS_MMAP might be helpful then. > Got it, thanks for the explanation. >> >> Alternatively, __FB_DEFAULT_DEFERRED_OPS_MMAP() could still be left but >> not taking a __prefix argument since that is not used anyways ? > > The driver optionally provides mmap without deferred I/O, hence the mmap > function. That makes no sense, as these writes to the buffer would never > make it to the device memory. But I didn't want to remove the code > either. So I just left the existing function as-is. Usually, the > deferred-I/O mmap is called immediately. [1] > Makes sense.
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c index 387d18706fec..90a77d19b236 100644 --- a/drivers/video/fbdev/smscufx.c +++ b/drivers/video/fbdev/smscufx.c @@ -894,64 +894,6 @@ static int ufx_handle_damage(struct ufx_data *dev, int x, int y, return 0; } -/* Path triggered by usermode clients who write to filesystem - * e.g. cat filename > /dev/fb1 - * Not used by X Windows or text-mode console. But useful for testing. - * Slow because of extra copy and we must assume all pixels dirty. */ -static ssize_t ufx_ops_write(struct fb_info *info, const char __user *buf, - size_t count, loff_t *ppos) -{ - ssize_t result; - struct ufx_data *dev = info->par; - u32 offset = (u32) *ppos; - - result = fb_sys_write(info, buf, count, ppos); - - if (result > 0) { - int start = max((int)(offset / info->fix.line_length), 0); - int lines = min((u32)((result / info->fix.line_length) + 1), - (u32)info->var.yres); - - ufx_handle_damage(dev, 0, start, info->var.xres, lines); - } - - return result; -} - -static void ufx_ops_copyarea(struct fb_info *info, - const struct fb_copyarea *area) -{ - - struct ufx_data *dev = info->par; - - sys_copyarea(info, area); - - ufx_handle_damage(dev, area->dx, area->dy, - area->width, area->height); -} - -static void ufx_ops_imageblit(struct fb_info *info, - const struct fb_image *image) -{ - struct ufx_data *dev = info->par; - - sys_imageblit(info, image); - - ufx_handle_damage(dev, image->dx, image->dy, - image->width, image->height); -} - -static void ufx_ops_fillrect(struct fb_info *info, - const struct fb_fillrect *rect) -{ - struct ufx_data *dev = info->par; - - sys_fillrect(info, rect); - - ufx_handle_damage(dev, rect->dx, rect->dy, rect->width, - rect->height); -} - /* NOTE: fb_defio.c is holding info->fbdefio.mutex * Touching ANY framebuffer memory that triggers a page fault * in fb_defio will cause a deadlock, when it also tries to @@ -1279,14 +1221,31 @@ static int ufx_ops_blank(int blank_mode, struct fb_info *info) return 0; } +static void ufx_ops_damage_range(struct fb_info *info, off_t off, size_t len) +{ + struct ufx_data *dev = info->par; + int start = max((int)(off / info->fix.line_length), 0); + int lines = min((u32)((len / info->fix.line_length) + 1), (u32)info->var.yres); + + ufx_handle_damage(dev, 0, start, info->var.xres, lines); +} + +static void ufx_ops_damage_area(struct fb_info *info, u32 x, u32 y, u32 width, u32 height) +{ + struct ufx_data *dev = info->par; + + ufx_handle_damage(dev, x, y, width, height); +} + +FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(ufx_ops, + ufx_ops_damage_range, + ufx_ops_damage_area) + static const struct fb_ops ufx_ops = { .owner = THIS_MODULE, - .fb_read = fb_sys_read, - .fb_write = ufx_ops_write, + __FB_DEFAULT_DEFERRED_OPS_RDWR(ufx_ops), .fb_setcolreg = ufx_ops_setcolreg, - .fb_fillrect = ufx_ops_fillrect, - .fb_copyarea = ufx_ops_copyarea, - .fb_imageblit = ufx_ops_imageblit, + __FB_DEFAULT_DEFERRED_OPS_DRAW(ufx_ops), .fb_mmap = ufx_ops_mmap, .fb_ioctl = ufx_ops_ioctl, .fb_open = ufx_ops_open,
Generate callback functions for struct fb_ops with the fbdev macro FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(). Initialize struct fb_ops to the generated functions with fbdev initializer macros. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Cc: Steve Glendinning <steve.glendinning@shawell.net> --- drivers/video/fbdev/smscufx.c | 85 +++++++++-------------------------- 1 file changed, 22 insertions(+), 63 deletions(-)