Re: [PATCH 2/5] fbdev/core: Export framebuffer read and write code as cfb_ function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 29, 2020 at 06:36:03PM +0200, Sam Ravnborg wrote:
> Hi Daniel.
> 
> On Wed, Jul 29, 2020 at 03:53:28PM +0200, daniel@xxxxxxxx wrote:
> > On Wed, Jul 29, 2020 at 03:41:45PM +0200, Thomas Zimmermann wrote:
> > > DRM fb helpers require read and write functions for framebuffer
> > > memory. Export the existing code from fbdev.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > 
> > Hm I'm not super sure whether we want to actually reuse this stuff ... We
> > kinda don't care about the sparc special case, and just having an fbdev
> > implementation witch has the switch between memcpy and memcpy_to/from_io
> > in one single place sounds a lot simpler ...
> > 
> > This way we can have a clean split between the old horrors of real fbdev
> > drivers, and a much cleaner world in drm. It would mean a bit of
> > copypasting, but I think that's actually a good thing.
> > 
> > In general my idea for drm fbdev emulation is that for any area we have a
> > problem we just ignore the entire fbmem.c code and write our own: mmap,
> > backlight handling (still unsolved, and horrible), cfb vs sys here. This
> > entire fbmem.c stuff is pretty bad midlayer, trying to avoid code
> > duplication here doesn't seem worth it imo.
> > 
> > Thoughts?
> 
> 
> I can see that fbmem is a mix of ioctl support and other stuff.
> We could factor out all the ioctl parts of fbmem.c to a new file
> named fbioctl.c.
> 
> And then let the ioctl parts call down into drm stuff and avoid reusing
> the fbdev code when we first reach drm code.
> This would require local copies of:
> sys_read, sys_write, sys_fillrect, sys_copyarea, sys_imageblit
> and more I think which I missed.
> 
> With local copies we could avoid some of the special cases and trim the
> unctions to what is required by drm only.
> And then no more fbmem dependencies and no dependencies to several of
> the small helper functions. So less entanglement with fbdev core.
> 
> This all sounds simple so I am surely missing a lot a ugly details here.
> 
> And should we touch this anyway we need a test suite to verify not too
> much breaks. To the best of my knowledge there is not yet such a test
> suite :-( Maybe because people caring about fbdev are limited.

Well my idea was to not refactor anything, but just have drm copies of the
various fb_ops callbacks. Definitely not even more refactoring :-)
-Daniel

> 
> 	Sam
> 
> 
> 
> 
> 
> > -Daniel
> > 
> > > ---
> > >  drivers/video/fbdev/core/fbmem.c | 53 ++++++++++++++++++++++----------
> > >  include/linux/fb.h               |  5 +++
> > >  2 files changed, 41 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> > > index dd0ccf35f7b7..b496ff90db3e 100644
> > > --- a/drivers/video/fbdev/core/fbmem.c
> > > +++ b/drivers/video/fbdev/core/fbmem.c
> > > @@ -759,25 +759,18 @@ static struct fb_info *file_fb_info(struct file *file)
> > >  	return info;
> > >  }
> > >  
> > > -static ssize_t
> > > -fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> > > +ssize_t fb_cfb_read(struct fb_info *info, 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;
> > >  
> > > -	if (!info || ! info->screen_base)
> > > -		return -ENODEV;
> > > -
> > >  	if (info->state != FBINFO_STATE_RUNNING)
> > >  		return -EPERM;
> > >  
> > > -	if (info->fbops->fb_read)
> > > -		return info->fbops->fb_read(info, buf, count, ppos);
> > > -
> > >  	total_size = info->screen_size;
> > >  
> > >  	if (total_size == 0)
> > > @@ -823,16 +816,12 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> > >  
> > >  	return (err) ? err : cnt;
> > >  }
> > > +EXPORT_SYMBOL(fb_cfb_read);
> > >  
> > >  static ssize_t
> > > -fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> > > +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, *src;
> > > -	u8 __iomem *dst;
> > > -	int c, cnt = 0, err = 0;
> > > -	unsigned long total_size;
> > >  
> > >  	if (!info || !info->screen_base)
> > >  		return -ENODEV;
> > > @@ -840,8 +829,20 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> > >  	if (info->state != FBINFO_STATE_RUNNING)
> > >  		return -EPERM;
> > >  
> > > -	if (info->fbops->fb_write)
> > > -		return info->fbops->fb_write(info, buf, count, ppos);
> > > +	if (info->fbops->fb_read)
> > > +		return info->fbops->fb_read(info, buf, count, ppos);
> > > +	else
> > > +		return fb_cfb_read(info, buf, count, ppos);
> > > +}
> > > +
> > > +ssize_t fb_cfb_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;
> > >  
> > >  	total_size = info->screen_size;
> > >  
> > > @@ -895,6 +896,24 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> > >  
> > >  	return (cnt) ? cnt : err;
> > >  }
> > > +EXPORT_SYMBOL(fb_cfb_write);
> > > +
> > > +static ssize_t
> > > +fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> > > +{
> > > +	struct fb_info *info = file_fb_info(file);
> > > +
> > > +	if (!info || !info->screen_base)
> > > +		return -ENODEV;
> > > +
> > > +	if (info->state != FBINFO_STATE_RUNNING)
> > > +		return -EPERM;
> > > +
> > > +	if (info->fbops->fb_write)
> > > +		return info->fbops->fb_write(info, buf, count, ppos);
> > > +	else
> > > +		return fb_cfb_write(info, buf, count, ppos);
> > > +}
> > >  
> > >  int
> > >  fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
> > > diff --git a/include/linux/fb.h b/include/linux/fb.h
> > > index 714187bc13ac..12ad83963db5 100644
> > > --- a/include/linux/fb.h
> > > +++ b/include/linux/fb.h
> > > @@ -593,6 +593,11 @@ extern int fb_blank(struct fb_info *info, int blank);
> > >  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_cfb_read(struct fb_info *info, char __user *buf,
> > > +			   size_t count, loff_t *ppos);
> > > +extern ssize_t fb_cfb_write(struct fb_info *info, const char __user *buf,
> > > +			    size_t count, loff_t *ppos);
> > > +
> > >  /*
> > >   * Drawing operations where framebuffer is in system RAM
> > >   */
> > > -- 
> > > 2.27.0
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux