diff mbox

[RFC,v3,1/3] fbmem: add default get_fb_unmapped_area function

Message ID 1480534459-7239-2-git-send-email-benjamin.gaignard@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Benjamin Gaignard Nov. 30, 2016, 7:34 p.m. UTC
If HAVE_ARCH_FB_UNMAPPED_AREA is set and get_fb_unmapped_area
function not defined in platform architecture let use a default function.

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

Comments

Laurent Pinchart Nov. 30, 2016, 7:39 p.m. UTC | #1
On Wednesday 30 Nov 2016 20:34:17 Benjamin Gaignard wrote:
> If HAVE_ARCH_FB_UNMAPPED_AREA is set and get_fb_unmapped_area
> function not defined in platform architecture let use a default function.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
>  drivers/video/fbdev/core/fbmem.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..54488ee 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;
>  }
> 
> +#if defined(HAVE_ARCH_FB_UNMAPPED_AREA) && !defined(get_fb_unmapped_area)

I think I've asked this twice already, how is that possible ?

> +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 Nov. 30, 2016, 7:46 p.m. UTC | #2
2016-11-30 20:39 GMT+01:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> On Wednesday 30 Nov 2016 20:34:17 Benjamin Gaignard wrote:
>> If HAVE_ARCH_FB_UNMAPPED_AREA is set and get_fb_unmapped_area
>> function not defined in platform architecture let use a default function.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> ---
>>  drivers/video/fbdev/core/fbmem.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/core/fbmem.c
>> b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..54488ee 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;
>>  }
>>
>> +#if defined(HAVE_ARCH_FB_UNMAPPED_AREA) && !defined(get_fb_unmapped_area)
>
> I think I've asked this twice already, how is that possible ?

As you said before only sparc and blackfin architectures have define
HAVE_ARCH_FB_UNMAPPED_AREA
and get_fb_unmapped_area().
For stm32f4 (ARMv7m) I plan to only add HAVE_ARCH_FB_UNMAPPED_AREA and
use this default function

By testing those two flags I avoid to define twince
get_fb_unmapped_area() if the architecture already
provide it.

>
>
>> +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. 1, 2016, 1:13 a.m. UTC | #3
Hi Benjamin,

On Wednesday 30 Nov 2016 20:46:23 Benjamin Gaignard wrote:
> 2016-11-30 20:39 GMT+01:00 Laurent Pinchart:
> > On Wednesday 30 Nov 2016 20:34:17 Benjamin Gaignard wrote:
> >> If HAVE_ARCH_FB_UNMAPPED_AREA is set and get_fb_unmapped_area
> >> function not defined in platform architecture let use a default function.
> >> 
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> >> ---
> >> 
> >>  drivers/video/fbdev/core/fbmem.c | 15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >> 
> >> diff --git a/drivers/video/fbdev/core/fbmem.c
> >> b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..54488ee 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;
> >>  }
> >> 
> >> +#if defined(HAVE_ARCH_FB_UNMAPPED_AREA) &&
> >> !defined(get_fb_unmapped_area)
> > 
> > I think I've asked this twice already, how is that possible ?
> 
> As you said before only sparc and blackfin architectures have define
> HAVE_ARCH_FB_UNMAPPED_AREA and get_fb_unmapped_area().
> For stm32f4 (ARMv7m) I plan to only add HAVE_ARCH_FB_UNMAPPED_AREA and
> use this default function

Ah, so it's for support of a future patch. You should explain that in the 
commit message.

The other question I asked was whether it would be possible to come up with a 
generic implementation of this function that would work on all architectures. 
At first sight the blackfin implementation is similar, but the sparc one is 
quite different. Why does it have to be arch-specific ?

> By testing those two flags I avoid to define twince
> get_fb_unmapped_area() if the architecture already
> provide it.
> 
> >> +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,
Michel Dänzer Dec. 1, 2016, 2:31 a.m. UTC | #4
On 01/12/16 04:46 AM, Benjamin Gaignard wrote:
> 2016-11-30 20:39 GMT+01:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>> On Wednesday 30 Nov 2016 20:34:17 Benjamin Gaignard wrote:
>>> If HAVE_ARCH_FB_UNMAPPED_AREA is set and get_fb_unmapped_area
>>> function not defined in platform architecture let use a default function.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>> ---
>>>  drivers/video/fbdev/core/fbmem.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/video/fbdev/core/fbmem.c
>>> b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..54488ee 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;
>>>  }
>>>
>>> +#if defined(HAVE_ARCH_FB_UNMAPPED_AREA) && !defined(get_fb_unmapped_area)
>>
>> I think I've asked this twice already, how is that possible ?
> 
> As you said before only sparc and blackfin architectures have define
> HAVE_ARCH_FB_UNMAPPED_AREA
> and get_fb_unmapped_area().
> For stm32f4 (ARMv7m) I plan to only add HAVE_ARCH_FB_UNMAPPED_AREA and
> use this default function

Note that the preprocessor only considers defined(get_fb_unmapped_area)
true if get_fb_unmapped_area is defined as a macro, not if it's a normal
function. Since AFAICT get_fb_unmapped_area is a normal function on
sparc and blackfin, this default function will be compiled on those
architectures as well.
Benjamin Gaignard Dec. 1, 2016, 7:24 a.m. UTC | #5
2016-12-01 2:13 GMT+01:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Benjamin,
>
> On Wednesday 30 Nov 2016 20:46:23 Benjamin Gaignard wrote:
>> 2016-11-30 20:39 GMT+01:00 Laurent Pinchart:
>> > On Wednesday 30 Nov 2016 20:34:17 Benjamin Gaignard wrote:
>> >> If HAVE_ARCH_FB_UNMAPPED_AREA is set and get_fb_unmapped_area
>> >> function not defined in platform architecture let use a default function.
>> >>
>> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> >> ---
>> >>
>> >>  drivers/video/fbdev/core/fbmem.c | 15 +++++++++++++++
>> >>  1 file changed, 15 insertions(+)
>> >>
>> >> diff --git a/drivers/video/fbdev/core/fbmem.c
>> >> b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..54488ee 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;
>> >>  }
>> >>
>> >> +#if defined(HAVE_ARCH_FB_UNMAPPED_AREA) &&
>> >> !defined(get_fb_unmapped_area)
>> >
>> > I think I've asked this twice already, how is that possible ?
>>
>> As you said before only sparc and blackfin architectures have define
>> HAVE_ARCH_FB_UNMAPPED_AREA and get_fb_unmapped_area().
>> For stm32f4 (ARMv7m) I plan to only add HAVE_ARCH_FB_UNMAPPED_AREA and
>> use this default function
>
> Ah, so it's for support of a future patch. You should explain that in the
> commit message.

OK

> The other question I asked was whether it would be possible to come up with a
> generic implementation of this function that would work on all architectures.
> At first sight the blackfin implementation is similar, but the sparc one is
> quite different. Why does it have to be arch-specific ?

I don't know but I guess it is link to how memory is mmapped by the
architecture

>> By testing those two flags I avoid to define twince
>> get_fb_unmapped_area() if the architecture already
>> provide it.
>>
>> >> +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. 1, 2016, 7:27 a.m. UTC | #6
2016-12-01 3:31 GMT+01:00 Michel Dänzer <michel@daenzer.net>:
> On 01/12/16 04:46 AM, Benjamin Gaignard wrote:
>> 2016-11-30 20:39 GMT+01:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>>> On Wednesday 30 Nov 2016 20:34:17 Benjamin Gaignard wrote:
>>>> If HAVE_ARCH_FB_UNMAPPED_AREA is set and get_fb_unmapped_area
>>>> function not defined in platform architecture let use a default function.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>>> ---
>>>>  drivers/video/fbdev/core/fbmem.c | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/video/fbdev/core/fbmem.c
>>>> b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..54488ee 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;
>>>>  }
>>>>
>>>> +#if defined(HAVE_ARCH_FB_UNMAPPED_AREA) && !defined(get_fb_unmapped_area)
>>>
>>> I think I've asked this twice already, how is that possible ?
>>
>> As you said before only sparc and blackfin architectures have define
>> HAVE_ARCH_FB_UNMAPPED_AREA
>> and get_fb_unmapped_area().
>> For stm32f4 (ARMv7m) I plan to only add HAVE_ARCH_FB_UNMAPPED_AREA and
>> use this default function
>
> Note that the preprocessor only considers defined(get_fb_unmapped_area)
> true if get_fb_unmapped_area is defined as a macro, not if it's a normal
> function. Since AFAICT get_fb_unmapped_area is a normal function on
> sparc and blackfin, this default function will be compiled on those
> architectures as well.

Since only sparc and blackfin have this function already, I will
change the condition to:
#ifdef HAVE_ARCH_FB_UNMAPPED_AREA
#ifndef CONFIG_SPARC
#ifndef CONFIG_BLACKFIN

so this function won't be compiled for those architectures

>
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
Laurent Pinchart Dec. 1, 2016, 8:38 a.m. UTC | #7
Hi Benjamin,

On Thursday 01 Dec 2016 08:24:50 Benjamin Gaignard wrote:
> 2016-12-01 2:13 GMT+01:00 Laurent Pinchart:
> > On Wednesday 30 Nov 2016 20:46:23 Benjamin Gaignard wrote:
> >> 2016-11-30 20:39 GMT+01:00 Laurent Pinchart:
> >>> On Wednesday 30 Nov 2016 20:34:17 Benjamin Gaignard wrote:
> >>>> If HAVE_ARCH_FB_UNMAPPED_AREA is set and get_fb_unmapped_area
> >>>> function not defined in platform architecture let use a default
> >>>> function.
> >>>> 
> >>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> >>>> ---
> >>>> 
> >>>>  drivers/video/fbdev/core/fbmem.c | 15 +++++++++++++++
> >>>>  1 file changed, 15 insertions(+)
> >>>> 
> >>>> diff --git a/drivers/video/fbdev/core/fbmem.c
> >>>> b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..54488ee 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;
> >>>>  }
> >>>> 
> >>>> +#if defined(HAVE_ARCH_FB_UNMAPPED_AREA) &&
> >>>> !defined(get_fb_unmapped_area)
> >>> 
> >>> I think I've asked this twice already, how is that possible ?
> >> 
> >> As you said before only sparc and blackfin architectures have define
> >> HAVE_ARCH_FB_UNMAPPED_AREA and get_fb_unmapped_area().
> >> For stm32f4 (ARMv7m) I plan to only add HAVE_ARCH_FB_UNMAPPED_AREA and
> >> use this default function
> > 
> > Ah, so it's for support of a future patch. You should explain that in the
> > commit message.
> 
> OK
> 
> > The other question I asked was whether it would be possible to come up
> > with a generic implementation of this function that would work on all
> > architectures. At first sight the blackfin implementation is similar, but
> > the sparc one is quite different. Why does it have to be arch-specific ?
> 
> I don't know but I guess it is link to how memory is mmapped by the
> architecture

Well, how about trying to find out then ? :-)

> >> By testing those two flags I avoid to define twince
> >> get_fb_unmapped_area() if the architecture already
> >> provide it.
> >> 
> >>>> +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,
Laurent Pinchart Dec. 1, 2016, 8:42 a.m. UTC | #8
On Thursday 01 Dec 2016 08:27:58 Benjamin Gaignard wrote:
> 2016-12-01 3:31 GMT+01:00 Michel Dänzer <michel@daenzer.net>:
> > On 01/12/16 04:46 AM, Benjamin Gaignard wrote:
> >> 2016-11-30 20:39 GMT+01:00 Laurent Pinchart:
> >>> On Wednesday 30 Nov 2016 20:34:17 Benjamin Gaignard wrote:
> >>>> If HAVE_ARCH_FB_UNMAPPED_AREA is set and get_fb_unmapped_area
> >>>> function not defined in platform architecture let use a default
> >>>> function.
> >>>> 
> >>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> >>>> ---
> >>>> 
> >>>>  drivers/video/fbdev/core/fbmem.c | 15 +++++++++++++++
> >>>>  1 file changed, 15 insertions(+)
> >>>> 
> >>>> diff --git a/drivers/video/fbdev/core/fbmem.c
> >>>> b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..54488ee 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;
> >>>> 
> >>>>  }
> >>>> 
> >>>> +#if defined(HAVE_ARCH_FB_UNMAPPED_AREA) &&
> >>>> !defined(get_fb_unmapped_area)
> >>> 
> >>> I think I've asked this twice already, how is that possible ?
> >> 
> >> As you said before only sparc and blackfin architectures have define
> >> HAVE_ARCH_FB_UNMAPPED_AREA
> >> and get_fb_unmapped_area().
> >> For stm32f4 (ARMv7m) I plan to only add HAVE_ARCH_FB_UNMAPPED_AREA and
> >> use this default function
> > 
> > Note that the preprocessor only considers defined(get_fb_unmapped_area)
> > true if get_fb_unmapped_area is defined as a macro, not if it's a normal
> > function. Since AFAICT get_fb_unmapped_area is a normal function on
> > sparc and blackfin, this default function will be compiled on those
> > architectures as well.
> 
> Since only sparc and blackfin have this function already, I will
> change the condition to:
> #ifdef HAVE_ARCH_FB_UNMAPPED_AREA
> #ifndef CONFIG_SPARC
> #ifndef CONFIG_BLACKFIN

No, that's really hackish. HAVE_ARCH_FB_UNMAPPED_AREA means that the 
architecture provides the function. The generic version should not be provided 
when that macro is defined.

Please try to see if you can't share the implementation with at least 
blackfin, and possibly sparc. If sparc requires its own version, you need a 
different mechanism to decide whether to provide the generic one.

> so this function won't be compiled for those architectures
diff mbox

Patch

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 76c1ad9..54488ee 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;
 }
 
+#if defined(HAVE_ARCH_FB_UNMAPPED_AREA) && !defined(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,