[PATCH v4 2/4] fbmem: add a default get_fb_unmapped_area function

[Benjamin Gaignard]

Hi Thomas,

in commit 59bd00c8 (Blackfin: fix framebuffer mmap bug for nommu) you
have introduce
get_fb_unmapped_area() for blackfin architecture.

I'm proposing a patch to have a default function in fbmem which
slightly does the same.

Do you think is new function could also fit with blackfin architecture needs ?
or does this architecture have specific requirements ?

Regards,
Benjamin

2016-12-07 16:19 GMT+01:00 Laurent Pinchart <laurent.pinchart at ideasonboard.com>:
> Hi Benjamin,
>
> On Wednesday 07 Dec 2016 15:57:49 Benjamin Gaignard wrote:
>> 2016-12-07 15:35 GMT+01:00 Laurent Pinchart:
>> > On Wednesday 07 Dec 2016 11:06:49 Benjamin Gaignard wrote:
>> >> Allow generic frame-buffer to provide a default
>> >> get_fb_unmapped_area function if specific devices need it.
>> >>
>> >> Usually this function is defined in architecture directories but
>> >> define it here may limit code duplication especially for all ARM
>> >> platforms without MMU.
>> >
>> > I still would like to see an explanation why an architecture-specific
>> > version is sometimes needed, and why this implementation is a reasonable
>> > default. Furthermore, it looks very similar to the blackfin
>> > implementation, so if you can't unify the two I'd like to know why.
>>
>> Obviously I don't know all the legacy behind this function but it is
>> definitively link to architectures memory mapping strategies in no MMU
>> cases.
>>
>> I think this function is a reasonable default when the architecture is
>> doing a direct mapping (no translation) of the memory like it is done in
>> ARM.
>>
>> Unlike what I propose blackfin implementation doesn't do any check on pgoff
>> and length and always return fb base address even is an offset has been
>> requested. I don't know if it is on purpose or just because nobody has
>> never try to mmap blackfin framebuffer with an offset...
>
> And that's exactly what I'd like you to try and find out :-) git blame and
> contacting the original authors of that code could be a first step.
>
>> >> version 4:
>> >> - introdude a configuration flag to be independent of architecture
>> >>
>> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard at linaro.org>
>> >> ---
>> >>
>> >>  drivers/video/fbdev/Kconfig      |  8 ++++++++
>> >>  drivers/video/fbdev/core/fbmem.c | 15 +++++++++++++++
>> >>  2 files changed, 23 insertions(+)
>> >>
>> >> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>> >> index 5d3b0db..922e4ea 100644
>> >> --- a/drivers/video/fbdev/Kconfig
>> >> +++ b/drivers/video/fbdev/Kconfig
>> >> @@ -138,6 +138,14 @@ config FB_SYS_IMAGEBLIT
>> >>
>> >>         blitting. This is used by drivers that don't provide their own
>> >>         (accelerated) version and the framebuffer is in system RAM.
>> >>
>> >> +config FB_PROVIDE_GET_FB_UNMAPPED_AREA
>> >> +     bool
>> >> +     depends on FB
>> >> +     default n
>> >> +     ---help---
>> >> +       Allow generic frame-buffer to provide get_fb_unmapped_area
>> >> +       function.
>> >> +
>> >>
>> >>  menuconfig FB_FOREIGN_ENDIAN
>> >>
>> >>       bool "Framebuffer foreign endianness support"
>> >>       depends on FB
>> >>
>> >> diff --git a/drivers/video/fbdev/core/fbmem.c
>> >> b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..22321a2 100644
>> >> --- a/drivers/video/fbdev/core/fbmem.c
>> >> +++ b/drivers/video/fbdev/core/fbmem.c
>> >> @@ -1492,6 +1492,21 @@ static long fb_compat_ioctl(struct file *file,
>> >> unsigned int cmd, return 0;
>> >>
>> >>  }
>> >>
>> >> +#ifdef CONFIG_FB_PROVIDE_GET_FB_UNMAPPED_AREA
>> >> +unsigned long get_fb_unmapped_area(struct file *filp,
>> >> +                                unsigned long addr, unsigned long len,
>> >> +                                unsigned long pgoff, unsigned long
>> >> flags)
>> >> +{
>> >> +     struct fb_info * const info = filp->private_data;
>> >> +     unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len);
>> >> +
>> >> +     if (pgoff > fb_size || len > fb_size - pgoff)
>> >> +             return -EINVAL;
>> >> +
>> >> +     return (unsigned long)info->screen_base + pgoff;
>> >> +}
>> >> +#endif
>> >> +
>> >>
>> >>  static const struct file_operations fb_fops = {
>> >>
>> >>       .owner =        THIS_MODULE,
>> >>       .read =         fb_read,
>> >
>> > --
>> > Regards,
>> >
>> > Laurent Pinchart
>
> --
> Regards,
>
> Laurent Pinchart
>