[PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe

[PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe

[Gregory CLEMENT]

With current code when the framebuffer is register, screen_base is not
set yet. So when we want to access framebuffer from mmap we get a
pointer to 0x0 instead of getting the pointer to the framebuffer
address. This patch fix this bug by allocating memory for framebuffer
during probe just before registering framebuffer driver.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/video/stm.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/video/stm.c b/drivers/video/stm.c
index f0abe4c..8a9acff 100644
--- a/drivers/video/stm.c
+++ b/drivers/video/stm.c
@@ -480,6 +480,7 @@ static int stmfb_probe(struct device_d *hw_dev)
 {
 	struct imx_fb_videomode *pdata = hw_dev->platform_data;
 	int ret;
+	unsigned size;
 
 	/* just init */
 	fbi.info.priv = &fbi;
@@ -497,6 +498,15 @@ static int stmfb_probe(struct device_d *hw_dev)
 	fbi.info.yres = fbi.info.mode->yres;
 	fbi.info.bits_per_pixel = 16;
 
+	size = calc_line_length(fbi.info.mode->xres, fbi.info.bits_per_pixel) *
+		fbi.info.mode->yres;
+
+	ret = stmfb_memory_mmgt(&fbi.info, size);
+	if (ret != 0) {
+		dev_err(hw_dev, "Cannot allocate framebuffer memory\n");
+		return ret;
+	}
+
 	ret = register_framebuffer(&fbi.info);
 	if (ret != 0) {
 		dev_err(hw_dev, "Failed to register framebuffer\n");
-- 
1.7.0.4




_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe

[Juergen Beisert]

Gregory CLEMENT wrote:
> With current code when the framebuffer is register, screen_base is not
> set yet. So when we want to access framebuffer from mmap we get a
> pointer to 0x0 instead of getting the pointer to the framebuffer
> address. This patch fix this bug by allocating memory for framebuffer
> during probe just before registering framebuffer driver.

Are you sure? See below

> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/video/stm.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/stm.c b/drivers/video/stm.c
> index f0abe4c..8a9acff 100644
> --- a/drivers/video/stm.c
> +++ b/drivers/video/stm.c
> @@ -480,6 +480,7 @@ static int stmfb_probe(struct device_d *hw_dev)
>  {
>  	struct imx_fb_videomode *pdata = hw_dev->platform_data;
>  	int ret;
> +	unsigned size;
>
>  	/* just init */
>  	fbi.info.priv = &fbi;
> @@ -497,6 +498,15 @@ static int stmfb_probe(struct device_d *hw_dev)
>  	fbi.info.yres = fbi.info.mode->yres;
>  	fbi.info.bits_per_pixel = 16;
>
> +	size = calc_line_length(fbi.info.mode->xres, fbi.info.bits_per_pixel) *
> +		fbi.info.mode->yres;
> +
> +	ret = stmfb_memory_mmgt(&fbi.info, size);
> +	if (ret != 0) {
> +		dev_err(hw_dev, "Cannot allocate framebuffer memory\n");
> +		return ret;
> +	}
> +
>  	ret = register_framebuffer(&fbi.info);
>  	if (ret != 0) {
>  		dev_err(hw_dev, "Failed to register framebuffer\n");

This probe function sets up the default mode. And register_framebuffer() 
itself calls stmfb_activate_var() which calls stmfb_memory_mmgt(). So, when 
the call to register_framebuffer() returns the 'screen_base' should no longer 
be NULL.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe

[Gregory CLEMENT]

On 01/04/2011 03:46 PM, Juergen Beisert wrote:
> Gregory CLEMENT wrote:
>> With current code when the framebuffer is register, screen_base is not
>> set yet. So when we want to access framebuffer from mmap we get a
>> pointer to 0x0 instead of getting the pointer to the framebuffer
>> address. This patch fix this bug by allocating memory for framebuffer
>> during probe just before registering framebuffer driver.
> 
> Are you sure? See below

It was the value I get at runtime at least.

> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
[...]
> 
> This probe function sets up the default mode. And register_framebuffer() 
> itself calls stmfb_activate_var() which calls stmfb_memory_mmgt(). So, when 
> the call to register_framebuffer() returns the 'screen_base' should no longer 
> be NULL.

But in register_framebuffer first map_base get its value from screen_base and
it is only after that fb_activate_var is called. And the problem is that mmap
will not get screen_base but map_base and this one is set too early.
Extract from  register_framebuffer() in fb.c:

	info->cdev.dev->map_base = (unsigned long)info->screen_base;
map_base get its value here ------^
	info->cdev.dev->size = info->cdev.size;

	dev = &info->dev;
	dev->priv = info;
	dev->id = id;

	sprintf(dev->name, "fb");

	register_device(&info->dev);
	dev_add_param(dev, "enable", fb_enable_set, NULL, 0);
	dev_set_param(dev, "enable", "0");

	if (info->num_modes && (info->mode_list != NULL) &&
			(info->fbops->fb_activate_var != NULL)) {
		dev_add_param(dev, "mode_name", fb_setup_mode, NULL, 0);
		dev_set_param(dev, "mode_name", info->mode_list[0].name);
fb_activate_var is called here ------------------------^


This code is new for me, maybe I missed something.

> 
> jbe
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe

[Juergen Beisert]

Gregory CLEMENT wrote:
> On 01/04/2011 03:46 PM, Juergen Beisert wrote:
> > Gregory CLEMENT wrote:
> >> With current code when the framebuffer is register, screen_base is not
> >> set yet. So when we want to access framebuffer from mmap we get a
> >> pointer to 0x0 instead of getting the pointer to the framebuffer
> >> address. This patch fix this bug by allocating memory for framebuffer
> >> during probe just before registering framebuffer driver.
> >
> > Are you sure? See below
>
> It was the value I get at runtime at least.
>
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> [...]
>
> > This probe function sets up the default mode. And register_framebuffer()
> > itself calls stmfb_activate_var() which calls stmfb_memory_mmgt(). So,
> > when the call to register_framebuffer() returns the 'screen_base' should
> > no longer be NULL.
>
> But in register_framebuffer first map_base get its value from screen_base
> and it is only after that fb_activate_var is called. And the problem is
> that mmap will not get screen_base but map_base and this one is set too
> early. Extract from  register_framebuffer() in fb.c:
>
> 	info->cdev.dev->map_base = (unsigned long)info->screen_base;
> map_base get its value here ------^
> 	info->cdev.dev->size = info->cdev.size;
>
> 	dev = &info->dev;
> 	dev->priv = info;
> 	dev->id = id;
>
> 	sprintf(dev->name, "fb");
>
> 	register_device(&info->dev);
> 	dev_add_param(dev, "enable", fb_enable_set, NULL, 0);
> 	dev_set_param(dev, "enable", "0");
>
> 	if (info->num_modes && (info->mode_list != NULL) &&
> 			(info->fbops->fb_activate_var != NULL)) {
> 		dev_add_param(dev, "mode_name", fb_setup_mode, NULL, 0);
> 		dev_set_param(dev, "mode_name", info->mode_list[0].name);
> fb_activate_var is called here ------------------------^

And this call will call driver's stmfb_activate_var() (and(!) 
stmfb_memory_mmgt()).
But I will check it again.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe

[Gregory CLEMENT]

On 01/04/2011 05:16 PM, Juergen Beisert wrote:
> Gregory CLEMENT wrote:
>> On 01/04/2011 03:46 PM, Juergen Beisert wrote:
>>> Gregory CLEMENT wrote:
>>>> With current code when the framebuffer is register, screen_base is not
>>>> set yet. So when we want to access framebuffer from mmap we get a
>>>> pointer to 0x0 instead of getting the pointer to the framebuffer
>>>> address. This patch fix this bug by allocating memory for framebuffer
>>>> during probe just before registering framebuffer driver.
>>>
>>> Are you sure? See below
>>
>> It was the value I get at runtime at least.
>>
>>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>
>> [...]
>>
>>> This probe function sets up the default mode. And register_framebuffer()
>>> itself calls stmfb_activate_var() which calls stmfb_memory_mmgt(). So,
>>> when the call to register_framebuffer() returns the 'screen_base' should
>>> no longer be NULL.
>>
>> But in register_framebuffer first map_base get its value from screen_base
>> and it is only after that fb_activate_var is called. And the problem is
>> that mmap will not get screen_base but map_base and this one is set too
>> early. Extract from  register_framebuffer() in fb.c:
>>
>> 	info->cdev.dev->map_base = (unsigned long)info->screen_base;
>> map_base get its value here ------^
>> 	info->cdev.dev->size = info->cdev.size;
>>
>> 	dev = &info->dev;
>> 	dev->priv = info;
>> 	dev->id = id;
>>
>> 	sprintf(dev->name, "fb");
>>
>> 	register_device(&info->dev);
>> 	dev_add_param(dev, "enable", fb_enable_set, NULL, 0);
>> 	dev_set_param(dev, "enable", "0");
>>
>> 	if (info->num_modes && (info->mode_list != NULL) &&
>> 			(info->fbops->fb_activate_var != NULL)) {
>> 		dev_add_param(dev, "mode_name", fb_setup_mode, NULL, 0);
>> 		dev_set_param(dev, "mode_name", info->mode_list[0].name);
>> fb_activate_var is called here ------------------------^
> 
> And this call will call driver's stmfb_activate_var() (and(!) 
> stmfb_memory_mmgt()).

Yes I totally agree with this. But at this time it's too late because changing
screen_base won't change map_base. And a call to memmap is a call to
generic_memmap_rw which will return map_base.
I didn't find anywhere in video driver code a place where map_base is updated, that's
why I think that once it get its value it won't change.

> But I will check it again.
> 
> jbe
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe

[Gregory CLEMENT]

On 01/04/2011 05:49 PM, Gregory CLEMENT wrote:
> On 01/04/2011 05:16 PM, Juergen Beisert wrote:
>> Gregory CLEMENT wrote:
>>> On 01/04/2011 03:46 PM, Juergen Beisert wrote:
>>>> Gregory CLEMENT wrote:
>>>>> With current code when the framebuffer is register, screen_base is not
>>>>> set yet. So when we want to access framebuffer from mmap we get a
>>>>> pointer to 0x0 instead of getting the pointer to the framebuffer
>>>>> address. This patch fix this bug by allocating memory for framebuffer
>>>>> during probe just before registering framebuffer driver.
>>>>
>>>> Are you sure? See below
>>>
>>> It was the value I get at runtime at least.
>>>
>>>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>>
>>> [...]
>>>
>>>> This probe function sets up the default mode. And register_framebuffer()
>>>> itself calls stmfb_activate_var() which calls stmfb_memory_mmgt(). So,
>>>> when the call to register_framebuffer() returns the 'screen_base' should
>>>> no longer be NULL.
>>>
>>> But in register_framebuffer first map_base get its value from screen_base
>>> and it is only after that fb_activate_var is called. And the problem is
>>> that mmap will not get screen_base but map_base and this one is set too
>>> early. Extract from  register_framebuffer() in fb.c:
>>>
>>> 	info->cdev.dev->map_base = (unsigned long)info->screen_base;
>>> map_base get its value here ------^
>>> 	info->cdev.dev->size = info->cdev.size;
>>>
>>> 	dev = &info->dev;
>>> 	dev->priv = info;
>>> 	dev->id = id;
>>>
>>> 	sprintf(dev->name, "fb");
>>>
>>> 	register_device(&info->dev);
>>> 	dev_add_param(dev, "enable", fb_enable_set, NULL, 0);
>>> 	dev_set_param(dev, "enable", "0");
>>>
>>> 	if (info->num_modes && (info->mode_list != NULL) &&
>>> 			(info->fbops->fb_activate_var != NULL)) {
>>> 		dev_add_param(dev, "mode_name", fb_setup_mode, NULL, 0);
>>> 		dev_set_param(dev, "mode_name", info->mode_list[0].name);
>>> fb_activate_var is called here ------------------------^
>>
>> And this call will call driver's stmfb_activate_var() (and(!) 
>> stmfb_memory_mmgt()).
> 
> Yes I totally agree with this. But at this time it's too late because changing
> screen_base won't change map_base. And a call to memmap is a call to
> generic_memmap_rw which will return map_base.
> I didn't find anywhere in video driver code a place where map_base is updated, that's
> why I think that once it get its value it won't change.
> 
>> But I will check it again.
>>

Had you had some time to check it?
Will you take this patch ?
If you have any comments I am willing to take them in account to make a new
version of this patch if it necessary.

Regards,

>> jbe
>>
> 
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe

[Juergen Beisert]

Gregory CLEMENT wrote:
> On 01/04/2011 05:49 PM, Gregory CLEMENT wrote:
> > On 01/04/2011 05:16 PM, Juergen Beisert wrote:
> >> Gregory CLEMENT wrote:
> >>> On 01/04/2011 03:46 PM, Juergen Beisert wrote:
> >>>> Gregory CLEMENT wrote:
> >>>>> With current code when the framebuffer is register, screen_base is
> >>>>> not set yet. So when we want to access framebuffer from mmap we get a
> >>>>> pointer to 0x0 instead of getting the pointer to the framebuffer
> >>>>> address. This patch fix this bug by allocating memory for framebuffer
> >>>>> during probe just before registering framebuffer driver.
> >>>>
> >>>> Are you sure? See below
> >>>
> >>> It was the value I get at runtime at least.
> >>>
> >>>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> >>>
> >>> [...]
> >>>
> >>>> This probe function sets up the default mode. And
> >>>> register_framebuffer() itself calls stmfb_activate_var() which calls
> >>>> stmfb_memory_mmgt(). So, when the call to register_framebuffer()
> >>>> returns the 'screen_base' should no longer be NULL.
> >>>
> >>> But in register_framebuffer first map_base get its value from
> >>> screen_base and it is only after that fb_activate_var is called. And
> >>> the problem is that mmap will not get screen_base but map_base and this
> >>> one is set too early. Extract from  register_framebuffer() in fb.c:
> >>>
> >>> 	info->cdev.dev->map_base = (unsigned long)info->screen_base;
> >>> map_base get its value here ------^
> >>> 	info->cdev.dev->size = info->cdev.size;
> >>>
> >>> 	dev = &info->dev;
> >>> 	dev->priv = info;
> >>> 	dev->id = id;
> >>>
> >>> 	sprintf(dev->name, "fb");
> >>>
> >>> 	register_device(&info->dev);
> >>> 	dev_add_param(dev, "enable", fb_enable_set, NULL, 0);
> >>> 	dev_set_param(dev, "enable", "0");
> >>>
> >>> 	if (info->num_modes && (info->mode_list != NULL) &&
> >>> 			(info->fbops->fb_activate_var != NULL)) {
> >>> 		dev_add_param(dev, "mode_name", fb_setup_mode, NULL, 0);
> >>> 		dev_set_param(dev, "mode_name", info->mode_list[0].name);
> >>> fb_activate_var is called here ------------------------^
> >>
> >> And this call will call driver's stmfb_activate_var() (and(!)
> >> stmfb_memory_mmgt()).
> >
> > Yes I totally agree with this. But at this time it's too late because
> > changing screen_base won't change map_base. And a call to memmap is a
> > call to generic_memmap_rw which will return map_base.
> > I didn't find anywhere in video driver code a place where map_base is
> > updated, that's why I think that once it get its value it won't change.
> >
> >> But I will check it again.
>
> Had you had some time to check it?

Not yet, sorry.

> Will you take this patch ?
> If you have any comments I am willing to take them in account to make a new
> version of this patch if it necessary.

Maybe I can check it tomorrow.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox