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

[Juergen Beisert <jbe@pengutronix.de>]

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