* [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe @ 2011-01-04 14:35 Gregory CLEMENT 2011-01-04 14:46 ` Juergen Beisert 0 siblings, 1 reply; 7+ messages in thread From: Gregory CLEMENT @ 2011-01-04 14:35 UTC (permalink / raw) Cc: barebox 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe 2011-01-04 14:35 [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe Gregory CLEMENT @ 2011-01-04 14:46 ` Juergen Beisert 2011-01-04 15:04 ` Gregory CLEMENT 0 siblings, 1 reply; 7+ messages in thread From: Juergen Beisert @ 2011-01-04 14:46 UTC (permalink / raw) To: barebox 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe 2011-01-04 14:46 ` Juergen Beisert @ 2011-01-04 15:04 ` Gregory CLEMENT 2011-01-04 16:16 ` Juergen Beisert 0 siblings, 1 reply; 7+ messages in thread From: Gregory CLEMENT @ 2011-01-04 15:04 UTC (permalink / raw) To: Juergen Beisert; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe 2011-01-04 15:04 ` Gregory CLEMENT @ 2011-01-04 16:16 ` Juergen Beisert 2011-01-04 16:49 ` Gregory CLEMENT 0 siblings, 1 reply; 7+ messages in thread From: Juergen Beisert @ 2011-01-04 16:16 UTC (permalink / raw) To: Gregory CLEMENT; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe 2011-01-04 16:16 ` Juergen Beisert @ 2011-01-04 16:49 ` Gregory CLEMENT 2011-01-13 15:10 ` Gregory CLEMENT 0 siblings, 1 reply; 7+ messages in thread From: Gregory CLEMENT @ 2011-01-04 16:49 UTC (permalink / raw) To: Juergen Beisert; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe 2011-01-04 16:49 ` Gregory CLEMENT @ 2011-01-13 15:10 ` Gregory CLEMENT 2011-01-13 15:21 ` Juergen Beisert 0 siblings, 1 reply; 7+ messages in thread From: Gregory CLEMENT @ 2011-01-13 15:10 UTC (permalink / raw) To: Juergen Beisert; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe 2011-01-13 15:10 ` Gregory CLEMENT @ 2011-01-13 15:21 ` Juergen Beisert 0 siblings, 0 replies; 7+ messages in thread From: Juergen Beisert @ 2011-01-13 15:21 UTC (permalink / raw) To: barebox 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-01-13 15:23 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-01-04 14:35 [PATCH 1/4] ARM STM/i.MX: Allocate memory for framebuffer during probe Gregory CLEMENT 2011-01-04 14:46 ` Juergen Beisert 2011-01-04 15:04 ` Gregory CLEMENT 2011-01-04 16:16 ` Juergen Beisert 2011-01-04 16:49 ` Gregory CLEMENT 2011-01-13 15:10 ` Gregory CLEMENT 2011-01-13 15:21 ` Juergen Beisert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox