Patchwork [U-Boot,05/14] cfb_console: Fix function console_scrollup

login
register
mail settings
Submitter Pali Rohár
Date Jan. 24, 2012, 2:28 p.m.
Message ID <1327415291-13260-6-git-send-email-pali.rohar@gmail.com>
Download mbox | patch
Permalink /patch/137564/
State Rejected
Delegated to: Anatolij Gustschin
Headers show

Comments

Pali Rohár - Jan. 24, 2012, 2:28 p.m.
* Use correct buffer size, do not damage screen output

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
Changes since original version:
   - Fixed commit message

 drivers/video/cfb_console.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Marek Vasut - Jan. 25, 2012, 6:06 p.m.
>  * Use correct buffer size, do not damage screen output

What are the symptoms, how does this fix the issue?

Ccing Anatolij, I think he's the right one for this patch. Again, keep the 
correct CC!!!

> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> Changes since original version:
>    - Fixed commit message
> 
>  drivers/video/cfb_console.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
> index 904caf7..9092399 100644
> --- a/drivers/video/cfb_console.c
> +++ b/drivers/video/cfb_console.c
> @@ -701,7 +701,7 @@ static void console_scrollup(void)
>  		);
>  #else
>  	memcpyl(CONSOLE_ROW_FIRST, CONSOLE_ROW_SECOND,
> -		CONSOLE_SCROLL_SIZE >> 2);
> +		CONSOLE_SCROLL_SIZE);
>  #endif
> 
>  	/* clear the last one */
Pali Rohár - Jan. 25, 2012, 7:59 p.m.
On Wednesday 25 January 2012 19:06:51 Marek Vasut wrote:
> >  * Use correct buffer size, do not damage screen output
> 
> What are the symptoms, how does this fix the issue?
> 
> Ccing Anatolij, I think he's the right one for this patch. Again, keep the
> correct CC!!!
> 

Output on n900 screen is damged after function console_scrollup is called.
Anatolij Gustschin - March 21, 2012, 9:45 a.m.
Hi,

On Tue, 24 Jan 2012 15:28:02 +0100
Pali Rohár <pali.rohar@gmail.com> wrote:

>  * Use correct buffer size, do not damage screen output
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> Changes since original version:
>    - Fixed commit message
> 
>  drivers/video/cfb_console.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
> index 904caf7..9092399 100644
> --- a/drivers/video/cfb_console.c
> +++ b/drivers/video/cfb_console.c
> @@ -701,7 +701,7 @@ static void console_scrollup(void)
>  		);
>  #else
>  	memcpyl(CONSOLE_ROW_FIRST, CONSOLE_ROW_SECOND,
> -		CONSOLE_SCROLL_SIZE >> 2);
> +		CONSOLE_SCROLL_SIZE);

NAK. This change is wrong. CONSOLE_SCROLL_SIZE is the size of the
visible frame buffer - size of one row in bytes. We are using memcpyl()
here, so the division by 4 (size >> 2) is correct. With your change
we end up copying 4 times more data then needed.

Thanks,
Anatolij
Marek Vasut - March 21, 2012, 10:20 a.m.
Dear Anatolij Gustschin,

> Hi,
> 
> On Tue, 24 Jan 2012 15:28:02 +0100
> 
> Pali Rohár <pali.rohar@gmail.com> wrote:
> >  * Use correct buffer size, do not damage screen output
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > 
> > Changes since original version:
> >    - Fixed commit message
> >  
> >  drivers/video/cfb_console.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
> > index 904caf7..9092399 100644
> > --- a/drivers/video/cfb_console.c
> > +++ b/drivers/video/cfb_console.c
> > @@ -701,7 +701,7 @@ static void console_scrollup(void)
> > 
> >  		);
> >  
> >  #else
> >  
> >  	memcpyl(CONSOLE_ROW_FIRST, CONSOLE_ROW_SECOND,
> > 
> > -		CONSOLE_SCROLL_SIZE >> 2);
> > +		CONSOLE_SCROLL_SIZE);
> 
> NAK. This change is wrong. CONSOLE_SCROLL_SIZE is the size of the
> visible frame buffer - size of one row in bytes. We are using memcpyl()
> here, so the division by 4 (size >> 2) is correct. With your change
> we end up copying 4 times more data then needed.

What kind of a problem are we fixing here? And what are the symptoms of it?

> 
> Thanks,
> Anatolij


Best regards,
Marek Vasut
Anatolij Gustschin - March 21, 2012, 11:32 a.m.
Hi Marek,

On Wed, 21 Mar 2012 11:20:38 +0100
Marek Vasut <marex@denx.de> wrote:

> Dear Anatolij Gustschin,
> 
> > Hi,
> > 
> > On Tue, 24 Jan 2012 15:28:02 +0100
> > 
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > >  * Use correct buffer size, do not damage screen output
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > > 
> > > Changes since original version:
> > >    - Fixed commit message
> > >  
> > >  drivers/video/cfb_console.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
> > > index 904caf7..9092399 100644
> > > --- a/drivers/video/cfb_console.c
> > > +++ b/drivers/video/cfb_console.c
> > > @@ -701,7 +701,7 @@ static void console_scrollup(void)
> > > 
> > >  		);
> > >  
> > >  #else
> > >  
> > >  	memcpyl(CONSOLE_ROW_FIRST, CONSOLE_ROW_SECOND,
> > > 
> > > -		CONSOLE_SCROLL_SIZE >> 2);
> > > +		CONSOLE_SCROLL_SIZE);
> > 
> > NAK. This change is wrong. CONSOLE_SCROLL_SIZE is the size of the
> > visible frame buffer - size of one row in bytes. We are using memcpyl()
> > here, so the division by 4 (size >> 2) is correct. With your change
> > we end up copying 4 times more data then needed.
> 
> What kind of a problem are we fixing here? And what are the symptoms of it?

Actually I'm not aware of any problem in current console_scrollup().
At least on two test setups with different framebuffer drivers and
in one setup with HW accelerated scrolling I didn't see any problems
with current code.

The description in the commit log of this patch:
"Use correct buffer size, do not damage screen output"
doesn't say much about the problem. If the GraphicDevice structure
returned by video_hw_init() is setup correctly, the scrolling should be
working fine. From the other patch [1] I can see that the structure
is setup as follows:

        /* fill in Graphic Device */
        gdev.frameAdrs = 0x8f9c0000;
        gdev.winSizeX = 800;
        gdev.winSizeY = 480;
        gdev.gdfBytesPP = 2;
        gdev.gdfIndex = GDF_16BIT_565RGB;
        memset((void *)gdev.frameAdrs, 0, 0xbb800);
        return (void *) &gdev;

Most likely using 0x8f9c0000 as framebuffer address is the first _big_
problem. The framebuffer address range is not allocated properly and
could be used by malloc area. The board has 256 MB of RAM starting at
0x80000000, if U-Boot relocated itself to the upper RAM, the problems
should be expected.

AFAIK the N900 port doesn't use a framebuffer driver but probably uses
pre-initialized display controller configuration. This should be fixed
first.

Thanks,
Anatolij
Anatolij Gustschin - March 21, 2012, 11:39 a.m.
On Wed, 21 Mar 2012 12:32:16 +0100
Anatolij Gustschin <agust@denx.de> wrote:
...
> doesn't say much about the problem. If the GraphicDevice structure
> returned by video_hw_init() is setup correctly, the scrolling should be
> working fine. From the other patch [1] I can see that the structure

I forgot to include a link [1] to the mentioned patch, sorry.
Here it is:

[1] http://patchwork.ozlabs.org/patch/137567/

Thanks,
Anatolij
Pali Rohár - March 21, 2012, 6:50 p.m.
On Wednesday 21 March 2012 12:32:16 Anatolij Gustschin wrote:
> Hi Marek,
> 
> On Wed, 21 Mar 2012 11:20:38 +0100
> 
> Marek Vasut <marex@denx.de> wrote:
> > Dear Anatolij Gustschin,
> > 
> > > Hi,
> > > 
> > > On Tue, 24 Jan 2012 15:28:02 +0100
> > > 
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > >  * Use correct buffer size, do not damage screen output
> > > > 
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > ---
> > > > 
> > > > Changes since original version:
> > > >    - Fixed commit message
> > > >  
> > > >  drivers/video/cfb_console.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/video/cfb_console.c
> > > > b/drivers/video/cfb_console.c index 904caf7..9092399 100644
> > > > --- a/drivers/video/cfb_console.c
> > > > +++ b/drivers/video/cfb_console.c
> > > > @@ -701,7 +701,7 @@ static void console_scrollup(void)
> > > > 
> > > >  		);
> > > >  
> > > >  #else
> > > >  
> > > >  	memcpyl(CONSOLE_ROW_FIRST, CONSOLE_ROW_SECOND,
> > > > 
> > > > -		CONSOLE_SCROLL_SIZE >> 2);
> > > > +		CONSOLE_SCROLL_SIZE);
> > > 
> > > NAK. This change is wrong. CONSOLE_SCROLL_SIZE is the size of
> > > the
> > > visible frame buffer - size of one row in bytes. We are using
> > > memcpyl() here, so the division by 4 (size >> 2) is correct.
> > > With your change we end up copying 4 times more data then
> > > needed.
> > 
> > What kind of a problem are we fixing here? And what are the
> > symptoms of it?
> Actually I'm not aware of any problem in current
> console_scrollup(). At least on two test setups with different
> framebuffer drivers and in one setup with HW accelerated scrolling
> I didn't see any problems with current code.
> 
> The description in the commit log of this patch:
> "Use correct buffer size, do not damage screen output"
> doesn't say much about the problem. If the GraphicDevice structure
> returned by video_hw_init() is setup correctly, the scrolling
> should be working fine. From the other patch [1] I can see that
> the structure is setup as follows:
> 
>         /* fill in Graphic Device */
>         gdev.frameAdrs = 0x8f9c0000;
>         gdev.winSizeX = 800;
>         gdev.winSizeY = 480;
>         gdev.gdfBytesPP = 2;
>         gdev.gdfIndex = GDF_16BIT_565RGB;
>         memset((void *)gdev.frameAdrs, 0, 0xbb800);
>         return (void *) &gdev;
> 
> Most likely using 0x8f9c0000 as framebuffer address is the first
> _big_ problem. The framebuffer address range is not allocated
> properly and could be used by malloc area. The board has 256 MB of
> RAM starting at 0x80000000, if U-Boot relocated itself to the
> upper RAM, the problems should be expected.
> 
> AFAIK the N900 port doesn't use a framebuffer driver but probably
> uses pre-initialized display controller configuration. This should
> be fixed first.
> 
> Thanks,
> Anatolij

Hi, can you show me how to fix this? How to correctly use 
framebuffer?
Anatolij Gustschin - March 21, 2012, 10:58 p.m.
On Wed, 21 Mar 2012 19:50:34 +0100
Pali Rohár <pali.rohar@gmail.com> wrote:
...
> Hi, can you show me how to fix this? How to correctly use 
> framebuffer?

Hi,

Not really. This would be a new project to rewrite U-Boot driver for
OMAP3 DSS. There is a driver under drivers/video/omap3_dss.c,
but it doesn't do what we need here. You can try to set gdev.frameAdrs
to point to another address range in RAM which is not used by U-Boot.
Then get OMAP3430 TRM and read DSS documentation how to configure the
display controller so that is displays frame buffer data from this
address range. Currently I do not have time nor resources to rewrite
the existing omap3_dss driver.

Thanks,
Anatolij
Pali Rohár - March 22, 2012, 8:58 a.m.
On Wednesday 21 March 2012 23:58:10 Anatolij Gustschin wrote:
> On Wed, 21 Mar 2012 19:50:34 +0100
> Pali Rohár <pali.rohar@gmail.com> wrote:
> ...
> 
> > Hi, can you show me how to fix this? How to correctly use
> > framebuffer?
> 
> Hi,
> 
> Not really. This would be a new project to rewrite U-Boot driver
> for OMAP3 DSS. There is a driver under drivers/video/omap3_dss.c,
> but it doesn't do what we need here. You can try to set
> gdev.frameAdrs to point to another address range in RAM which is
> not used by U-Boot. Then get OMAP3430 TRM and read DSS
> documentation how to configure the display controller so that is
> displays frame buffer data from this address range. Currently I do
> not have time nor resources to rewrite the existing omap3_dss
> driver.
> 
> Thanks,
> Anatolij

Do you think that mapped address of framebuffer is only problem? Why 
then framebuffer output on n900 screen working without problem if I 
do NOT read framebuffer memory? Problem with garbaged display output 
seems happends only for read operations (console_scrollup, negation 
of pixels for cursor,...) but not for write-only operations (clear 
console, clear line, set black/white cursror, render fonsts...). I 
think if there is problem with address space, then garbaged screen 
should be also for framebuffer write operations...
Anatolij Gustschin - March 23, 2012, 8:47 a.m.
Hi,

On Thu, 22 Mar 2012 01:58:02 -0700 (PDT)
Pali Rohár <pali.rohar@gmail.com> wrote:
...
> Do you think that mapped address of framebuffer is only problem?

I wrote that it could be a problem. I do not know much about
your system and U-Boot port and do not know exactly what you
meant by "damaged screen output" and when exactly it happens,
so I can only speculate.

> Why 
> then framebuffer output on n900 screen working without problem if I 
> do NOT read framebuffer memory? Problem with garbaged display output 
> seems happends only for read operations (console_scrollup, negation 
> of pixels for cursor,...) but not for write-only operations (clear 
> console, clear line, set black/white cursror, render fonsts...). I 
> think if there is problem with address space, then garbaged screen 
> should be also for framebuffer write operations...

This is another issue then. Can you test read/write access to
the frame buffer area by available memory commands "cp", "md",
"mm", "cmp"? Can you also verify that reading from frame buffer
area by using these memory commands is not working?

Thanks,
Anatolij
Anatolij Gustschin - April 26, 2012, 9:45 p.m.
Hi,

On Tue, 24 Jan 2012 15:28:02 +0100
Pali Rohár <pali.rohar@gmail.com> wrote:

>  * Use correct buffer size, do not damage screen output

It seems that I can reproduce similar problem on the beagleboard now.
Will try to debug and find a solution for it.

Thanks,
Anatolij
Pali Rohár - April 26, 2012, 9:50 p.m.
On Thursday 26 April 2012 23:45:52 Anatolij Gustschin wrote:
> Hi,
> 
> On Tue, 24 Jan 2012 15:28:02 +0100
> 
> Pali Rohár <pali.rohar@gmail.com> wrote:
> >  * Use correct buffer size, do not damage screen output
> 
> It seems that I can reproduce similar problem on the
> beagleboard now. Will try to debug and find a solution for it.
> 
> Thanks,
> Anatolij

Ok, thanks! I played a lot of with u-boot on nokia n900, but I 
did not find reason...
Anatolij Gustschin - April 28, 2012, 3:11 p.m.
On Thu, 26 Apr 2012 23:50:57 +0200
Pali Rohár <pali.rohar@gmail.com> wrote:

> On Thursday 26 April 2012 23:45:52 Anatolij Gustschin wrote:
> > Hi,
> > 
> > On Tue, 24 Jan 2012 15:28:02 +0100
> > 
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > >  * Use correct buffer size, do not damage screen output
> > 
> > It seems that I can reproduce similar problem on the
> > beagleboard now. Will try to debug and find a solution for it.
> 
> Ok, thanks! I played a lot of with u-boot on nokia n900, but I 
> did not find reason...

Probably the issue is caused by cached frame buffer data. This [1]
patch is supposed to fix it. Could you please test it on N900?
Additional cache flushing also needs to be added to your extensions
for cfb_console to fix the menu output.

Thanks,
Anatolij

[1] http://patchwork.ozlabs.org/patch/155662/
Pali Rohár - April 28, 2012, 3:58 p.m.
On Saturday 28 April 2012 17:11:51 Anatolij Gustschin wrote:
> On Thu, 26 Apr 2012 23:50:57 +0200
> 
> Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Thursday 26 April 2012 23:45:52 Anatolij Gustschin wrote:
> > > Hi,
> > > 
> > > On Tue, 24 Jan 2012 15:28:02 +0100
> > > 
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > >  * Use correct buffer size, do not damage screen output
> > > 
> > > It seems that I can reproduce similar problem on the
> > > beagleboard now. Will try to debug and find a solution for
> > > it.
> > 
> > Ok, thanks! I played a lot of with u-boot on nokia n900, but
> > I
> > did not find reason...
> 
> Probably the issue is caused by cached frame buffer data. This
> [1] patch is supposed to fix it. Could you please test it on
> N900? Additional cache flushing also needs to be added to your
> extensions for cfb_console to fix the menu output.
> 
> Thanks,
> Anatolij
> 
> [1] http://patchwork.ozlabs.org/patch/155662/

Hi, your patch fixing this problem. Thanks!

Patch

diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
index 904caf7..9092399 100644
--- a/drivers/video/cfb_console.c
+++ b/drivers/video/cfb_console.c
@@ -701,7 +701,7 @@  static void console_scrollup(void)
 		);
 #else
 	memcpyl(CONSOLE_ROW_FIRST, CONSOLE_ROW_SECOND,
-		CONSOLE_SCROLL_SIZE >> 2);
+		CONSOLE_SCROLL_SIZE);
 #endif
 
 	/* clear the last one */