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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3690 bytes --]

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]