Re: [PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers

[Sam Ravnborg <sam@ravnborg.org>]

Hi Thomas,

On Mon, Apr 25, 2022 at 07:26:32PM +0200, Sam Ravnborg wrote:
> Hi Thomas,
> 
> > diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> > index 6aaf6d0abf39..6924d489a289 100644
> > --- a/drivers/video/fbdev/core/fb_defio.c
> > +++ b/drivers/video/fbdev/core/fb_defio.c
> > @@ -181,6 +181,7 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> >  	vma->vm_private_data = info;
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
> >  
> >  /* workqueue callback */
> >  static void fb_deferred_io_work(struct work_struct *work)
> > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> > index 84427470367b..52440e3f8f69 100644
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -1334,7 +1334,6 @@ static int
> >  fb_mmap(struct file *file, struct vm_area_struct * vma)
> >  {
> >  	struct fb_info *info = file_fb_info(file);
> > -	int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
> >  	unsigned long mmio_pgoff;
> >  	unsigned long start;
> >  	u32 len;
> > @@ -1343,14 +1342,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> >  		return -ENODEV;
> >  	mutex_lock(&info->mm_lock);
> >  
> > -	fb_mmap_fn = info->fbops->fb_mmap;
> > -
> > -#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
> > -	if (info->fbdefio)
> > -		fb_mmap_fn = fb_deferred_io_mmap;
> > -#endif
> > -
> > -	if (fb_mmap_fn) {
> > +	if (info->fbops->fb_mmap) {
> >  		int res;
> >  
> >  		/*
> > @@ -1358,11 +1350,18 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> >  		 * SME protection is removed ahead of the call
> >  		 */
> >  		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> > -		res = fb_mmap_fn(info, vma);
> > +		res = info->fbops->fb_mmap(info, vma);
> >  		mutex_unlock(&info->mm_lock);
> >  		return res;
> >  	}
> >  
> > +	/*
> > +	 * FB deferred I/O wants you to handle mmap in your drivers. At a
> > +	 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
> > +	 */
> > +	if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
> > +		return -ENODEV;
> > +
> 
> If not configured - then why not just call fb_deferred_io_mmap(), as
> this seems to be the default implementation anyway.
> Then drivers that needs it can override - and the rest fallback to the
> default.

Just to be clear - I already read:
"
Leave the mmap handling to drivers and expect them to call the
helper for deferred I/O by thmeselves.
"

But this does not help me understand why we need to explicit do what
could be a simple default implementation.
Chances are that I am stupid and it is obvious..

	Sam