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

The fbdev mmap function fb_mmap() unconditionally overrides the
driver's implementation if deferred I/O has been activated. This
makes it hard to implement mmap with anything but a vmalloc()'ed
software buffer. That is specifically a problem for DRM, where
video memory is maintained by a memory manager.

Leave the mmap handling to drivers and expect them to call the
helper for deferred I/O by thmeselves.

v2:
	* print a helpful error message if the defio setup is
	  incorrect (Javier)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c        |  4 +++-
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c     |  1 +
 drivers/hid/hid-picolcd_fb.c           |  1 +
 drivers/staging/fbtft/fbtft-core.c     |  1 +
 drivers/video/fbdev/broadsheetfb.c     |  1 +
 drivers/video/fbdev/core/fb_defio.c    |  1 +
 drivers/video/fbdev/core/fbmem.c       | 19 +++++++++----------
 drivers/video/fbdev/hecubafb.c         |  1 +
 drivers/video/fbdev/hyperv_fb.c        |  1 +
 drivers/video/fbdev/metronomefb.c      |  1 +
 drivers/video/fbdev/sh_mobile_lcdcfb.c |  6 ++++++
 drivers/video/fbdev/smscufx.c          |  3 +++
 drivers/video/fbdev/ssd1307fb.c        |  1 +
 drivers/video/fbdev/udlfb.c            |  3 +++
 drivers/video/fbdev/xen-fbfront.c      |  1 +
 15 files changed, 34 insertions(+), 11 deletions(-)

Hi Thomas,

I love your patch! Yet something to improve:

[auto build test ERROR on 0e7deff6446a4ba2c75f499a0bfa80cd6a15c129]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Decouple-deferred-I-O-from-struct-page/20220425-192955
base:   0e7deff6446a4ba2c75f499a0bfa80cd6a15c129
config: parisc-randconfig-r011-20220425 (https://download.01.org/0day-ci/archive/20220425/202204252303.Xu2N4l6Y-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/183267de16bfe1cd6ec119bcfdaf95e3706bf87e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/fbdev-Decouple-deferred-I-O-from-struct-page/20220425-192955
        git checkout 183267de16bfe1cd6ec119bcfdaf95e3706bf87e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash drivers/video/fbdev/core/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/printk.h:11,
                    from include/linux/kernel.h:29,
                    from include/linux/cpumask.h:10,
                    from include/linux/mm_types_task.h:14,
                    from include/linux/mm_types.h:5,
                    from include/linux/buildid.h:5,
                    from include/linux/module.h:14,
                    from drivers/video/fbdev/core/fbmem.c:14:
   drivers/video/fbdev/core/fbmem.c: In function 'fb_mmap':
>> drivers/video/fbdev/core/fbmem.c:1362:42: error: 'struct fb_info' has no member named 'fbdefio'
    1362 |         if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
         |                                          ^~
   include/linux/once_lite.h:15:41: note: in definition of macro 'DO_ONCE_LITE_IF'
      15 |                 bool __ret_do_once = !!(condition);                     \
         |                                         ^~~~~~~~~
   include/linux/dev_printk.h:274:9: note: in expansion of macro 'WARN_ONCE'
     274 |         WARN_ONCE(condition, "%s %s: " format, \
         |         ^~~~~~~~~
   drivers/video/fbdev/core/fbmem.c:1362:13: note: in expansion of macro 'dev_WARN_ONCE'
    1362 |         if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
         |             ^~~~~~~~~~~~~


vim +1362 drivers/video/fbdev/core/fbmem.c

  1332	
  1333	static int
  1334	fb_mmap(struct file *file, struct vm_area_struct * vma)
  1335	{
  1336		struct fb_info *info = file_fb_info(file);
  1337		unsigned long mmio_pgoff;
  1338		unsigned long start;
  1339		u32 len;
  1340	
  1341		if (!info)
  1342			return -ENODEV;
  1343		mutex_lock(&info->mm_lock);
  1344	
  1345		if (info->fbops->fb_mmap) {
  1346			int res;
  1347	
  1348			/*
  1349			 * The framebuffer needs to be accessed decrypted, be sure
  1350			 * SME protection is removed ahead of the call
  1351			 */
  1352			vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
  1353			res = info->fbops->fb_mmap(info, vma);
  1354			mutex_unlock(&info->mm_lock);
  1355			return res;
  1356		}
  1357	
  1358		/*
  1359		 * FB deferred I/O wants you to handle mmap in your drivers. At a
  1360		 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
  1361		 */
> 1362		if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
  1363			return -ENODEV;
  1364	
  1365		/*
  1366		 * Ugh. This can be either the frame buffer mapping, or
  1367		 * if pgoff points past it, the mmio mapping.
  1368		 */
  1369		start = info->fix.smem_start;
  1370		len = info->fix.smem_len;
  1371		mmio_pgoff = PAGE_ALIGN((start & ~PAGE_MASK) + len) >> PAGE_SHIFT;
  1372		if (vma->vm_pgoff >= mmio_pgoff) {
  1373			if (info->var.accel_flags) {
  1374				mutex_unlock(&info->mm_lock);
  1375				return -EINVAL;
  1376			}
  1377	
  1378			vma->vm_pgoff -= mmio_pgoff;
  1379			start = info->fix.mmio_start;
  1380			len = info->fix.mmio_len;
  1381		}
  1382		mutex_unlock(&info->mm_lock);
  1383	
  1384		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
  1385		fb_pgprotect(file, vma, start);
  1386	
  1387		return vm_iomap_memory(vma, start, len);
  1388	}
  1389

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

Hi Sam

Am 25.04.22 um 19:46 schrieb Sam Ravnborg:
> 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..

That's no stupid question. I didn't want to use a default implementation 
because there's no single one that severs all purposes. The current code 
all uses the same helper, but this will change with later patchsets. At 
some point, GEM is supposed to implement some of the logic for deferred 
I/O and will have to set it's own helpers. This can be seen in patches 8 
and 9 of [1]. I think it's better to clearly fail here than to provide a 
default implementation.

Best regards
Thomas

[1] 
https://lore.kernel.org/dri-devel/20220303205839.28484-1-tzimmermann@suse.de/


> 
> 	Sam