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

Message ID 1481105211-18020-3-git-send-email-benjamin.gaignard@linaro.org
State New
Headers show

Commit Message

Benjamin Gaignard Dec. 7, 2016, 10:06 a.m.
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.

version 4:
- introdude a configuration flag to be independent of architecture

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/video/fbdev/Kconfig      |  8 ++++++++
 drivers/video/fbdev/core/fbmem.c | 15 +++++++++++++++
 2 files changed, 23 insertions(+)

Comments

Laurent Pinchart Dec. 7, 2016, 2:35 p.m. | #1
Hi Benjamin,

Thank you for the patch.

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.

> version 4:
> - introdude a configuration flag to be independent of architecture
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@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,
Benjamin Gaignard Dec. 7, 2016, 2:57 p.m. | #2
2016-12-07 15:35 GMT+01:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Benjamin,
>
> Thank you for the patch.
>
> 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...

>
>> version 4:
>> - introdude a configuration flag to be independent of architecture
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@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
>
Laurent Pinchart Dec. 7, 2016, 3:19 p.m. | #3
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@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
Benjamin Gaignard Dec. 7, 2016, 3:31 p.m. | #4
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@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@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
>

Patch hide | download patch | download mbox

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,