| Message ID | 1327415291-13260-6-git-send-email-pali.rohar@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
> * 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 */
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.
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
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
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
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
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?
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
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...
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
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
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...
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/
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!
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 */
* 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(-)