From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from metis.ext.pengutronix.de ([2001:6f8:1178:4:290:27ff:fe1d:cc33]) by canuck.infradead.org with esmtps (Exim 4.72 #1 (Red Hat Linux)) id 1PdP13-0003LO-Q5 for barebox@lists.infradead.org; Thu, 13 Jan 2011 15:23:10 +0000 From: Juergen Beisert Date: Thu, 13 Jan 2011 16:21:45 +0100 References: <4D233027.9030701@free-electrons.com> <4D234F8A.4070703@free-electrons.com> <4D2F15F4.3030300@free-electrons.com> In-Reply-To: <4D2F15F4.3030300@free-electrons.com> MIME-Version: 1.0 Content-Disposition: inline Message-Id: <201101131621.45581.jbe@pengutronix.de> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: barebox-bounces@lists.infradead.org Errors-To: barebox-bounces+u.kleine-koenig=pengutronix.de@lists.infradead.org Subject: Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe To: barebox@lists.infradead.org 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 > >>> > >>> [...] > >>> > >>>> 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