Re: [Linux-fbdev-devel] [PATCH] video: arch specific page protection support for deferred io

[Magnus D. <mag...@gm...>]

On Thu, Jun 25, 2009 at 11:56 AM, Andrew
Morton<ak...@li...> wrote:
> On Wed, 24 Jun 2009 19:54:13 +0900 Magnus Damm <mag...@gm...> wrote:
>
>> From: Magnus Damm <da...@ig...>
>>
>> This patch adds arch specific page protection support to deferred io.
>>
>> Instead of overwriting the info->fbops->mmap pointer with the
>> deferred io specific mmap callback, modify fb_mmap() to include
>> a #ifdef wrapped call to fb_deferred_io_mmap().  The function
>> fb_deferred_io_mmap() is extended to call fb_pgprotect() in the
>> case of non-vmalloc() frame buffers.
>>
>> With this patch uncached deferred io can be used together with
>> the sh_mobile_lcdcfb driver. Without this patch arch specific
>> page protection code in fb_pgprotect() never gets invoked with
>> deferred io.
>>
>> Signed-off-by: Magnus Damm <da...@ig...>
>> ---
>>
>>  For proper runtime operation with uncached vmas make sure
>>  "[PATCH][RFC] mm: uncached vma support with writenotify"
>>  is applied. There are no merge order dependencies.
>
> So this is dependent upon a patch which is in your tree, which is in
> linux-next?

I tried to say that there were _no_ dependencies merge wise. =)

There are 3 levels of dependencies:
1: pgprot_noncached() patches from Arnd
2: mm: uncached vma support with writenotify
3: video: arch specfic page protection support for deferred io

2 depends on 1 to compile, but 3 (this one) is disconnected from 2 and
1. So this patch can be merged independently.

>
>>  drivers/video/fb_defio.c |   10 +++++++---
>>  drivers/video/fbmem.c    |    6 ++++++
>>  include/linux/fb.h       |    2 ++
>>  3 files changed, 15 insertions(+), 3 deletions(-)
>>
>> --- 0001/drivers/video/fb_defio.c
>> +++ work/drivers/video/fb_defio.c     2009-06-24 19:07:11.000000000 +0900
>> @@ -19,6 +19,7 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/fb.h>
>>  #include <linux/list.h>
>> +#include <asm/fb.h>
>
> Microblaze doesn't have an asm/fb.h.

Right, but fbmem.c includes asm/fb.h as well (for fb_pgprotect()), so
framebuffer isn't supported on Microblaze. So I think this is a
separate issue.

>>  /* to support deferred IO */
>>  #include <linux/rmap.h>
>> @@ -141,11 +142,16 @@ static const struct address_space_operat
>>       .set_page_dirty = fb_deferred_io_set_page_dirty,
>>  };
>>
>> -static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
>> +int fb_deferred_io_mmap(struct file *file, struct fb_info *info,
>> +                     struct vm_area_struct *vma, unsigned long off)
>>  {
>>       vma->vm_ops = &fb_deferred_io_vm_ops;
>>       vma->vm_flags |= ( VM_IO | VM_RESERVED | VM_DONTEXPAND );
>>       vma->vm_private_data = info;
>> +
>> +     if (!is_vmalloc_addr(info->screen_base))
>> +             fb_pgprotect(file, vma, off);
>
> Add a comment explaining what's going on here?

Good idea!

>> @@ -1325,6 +1325,12 @@ __releases(&info->lock)
>>       off = vma->vm_pgoff << PAGE_SHIFT;
>>       if (!fb)
>>               return -ENODEV;
>> +
>> +#ifdef CONFIG_FB_DEFERRED_IO
>> +     if (info->fbdefio)
>> +             return fb_deferred_io_mmap(file, info, vma, off);
>> +#endif
>
> We can remove the ifdefs here...
>
>> +extern int fb_deferred_io_mmap(struct file *file, struct fb_info *info,
>> +                            struct vm_area_struct *vma, unsigned long off);
>
> if we do
>
> #else   /* CONFIG_FB_DEFERRED_IO */
> static inline int fb_deferred_io_mmap(struct file *file, struct fb_info *info,
>                                struct vm_area_struct *vma, unsigned long off)
> {
>        return 0;
> }
> #endif  /* CONFIG_FB_DEFERRED_IO */
>
> here.

The code is fbmem.c is currently filled with #ifdefs today, want me
create inline versions for fb_deferred_io_open() and
fb_deferred_io_fsync() as well?

Thanks for your comments!

/ magnus