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

[Gregory CLEMENT <gregory.clement@free-electrons.com>]

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