| Message ID | 1461712873-6190-2-git-send-email-martin.pietryka@chello.at |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
On 04/27/2016 01:21 AM, Martin Pietryka wrote: > The DMA was outputting the palette on the screen because the base > for the DMA was not after the palette. In addition to that, the ceiling was > also too high, this led that the output on the screen was shifted. > > NOTE: According to the TRM, even in 16/24bit mode a palette is required > in the first 32 bytes of the framebuffer. > > See also: > https://e2e.ti.com/support/arm/sitara_arm/f/791/p/234967/834483#834483 > > "In this mode, the LCDC will assume all information is data and thus you > need to ensure that the DMA points to the first pixel of data and not the > first entry in the frame buffer which is the beginning of the 512 byte > palette." Hi Martin, many thanks for working on this. I'm just reviewing the stuff, you're right it shouldn't work right now. But it does ?! Perhaps it does because there is another issue. DMA today fetches palette also, but the Bit 21..20 in RASTER_CTRL aren't set correctly. We do: lcdhw->raster_ctrl = LCD_TFT_24BPP_MODE | LCD_TFT_24BPP_UNPACK | LCD_PALMODE_RAWDATA | LCD_TFT_MODE | LCD_RASTER_ENABLE; with #define LCD_PALMODE_RAWDATA (0x10 << 20) this doesn't set palmode as excepted to raw data, instead this sets 'stn565, bit24'. I will investigate a bit on this now, testing your patches and fixup my mistakes. best regards, Hannes
On 04/27/2016 08:43 AM, Hannes Schmelzer wrote: > On 04/27/2016 01:21 AM, Martin Pietryka wrote: >> The DMA was outputting the palette on the screen because the base >> for the DMA was not after the palette. In addition to that, the >> ceiling was >> also too high, this led that the output on the screen was shifted. >> >> NOTE: According to the TRM, even in 16/24bit mode a palette is required >> in the first 32 bytes of the framebuffer. >> >> See also: >> https://e2e.ti.com/support/arm/sitara_arm/f/791/p/234967/834483#834483 >> >> "In this mode, the LCDC will assume all information is data and thus you >> need to ensure that the DMA points to the first pixel of data and not >> the >> first entry in the frame buffer which is the beginning of the 512 byte >> palette." > Hi Martin, > many thanks for working on this. > > I'm just reviewing the stuff, you're right it shouldn't work right now. > But it does ?! Perhaps it does because there is another issue. > > DMA today fetches palette also, but the Bit 21..20 in RASTER_CTRL > aren't set correctly. > > We do: > lcdhw->raster_ctrl = LCD_TFT_24BPP_MODE | > LCD_TFT_24BPP_UNPACK | > LCD_PALMODE_RAWDATA | > LCD_TFT_MODE | > LCD_RASTER_ENABLE; > > with > > #define LCD_PALMODE_RAWDATA (0x10 << 20) sorry, i've missed recent changes applied to master, there this was already fixed. I also reviewed this ;-) > > this doesn't set palmode as excepted to raw data, instead this sets > 'stn565, bit24'. > > I will investigate a bit on this now, testing your patches and fixup > my mistakes. > > best regards, > Hannes >
Hi Hannes, On 04/27/2016 08:43 AM, Hannes Schmelzer wrote: > On 04/27/2016 01:21 AM, Martin Pietryka wrote: >> The DMA was outputting the palette on the screen because the base >> for the DMA was not after the palette. In addition to that, the >> ceiling was >> also too high, this led that the output on the screen was shifted. >> >> NOTE: According to the TRM, even in 16/24bit mode a palette is required >> in the first 32 bytes of the framebuffer. >> >> See also: >> https://e2e.ti.com/support/arm/sitara_arm/f/791/p/234967/834483#834483 >> >> "In this mode, the LCDC will assume all information is data and thus you >> need to ensure that the DMA points to the first pixel of data and not >> the >> first entry in the frame buffer which is the beginning of the 512 byte >> palette." > Hi Martin, > many thanks for working on this. > > I'm just reviewing the stuff, you're right it shouldn't work right now. > But it does ?! Perhaps it does because there is another issue. > > DMA today fetches palette also, but the Bit 21..20 in RASTER_CTRL > aren't set correctly. > > We do: > lcdhw->raster_ctrl = LCD_TFT_24BPP_MODE | > LCD_TFT_24BPP_UNPACK | > LCD_PALMODE_RAWDATA | > LCD_TFT_MODE | > LCD_RASTER_ENABLE; > > with > > #define LCD_PALMODE_RAWDATA (0x10 << 20) > > this doesn't set palmode as excepted to raw data, instead this sets > 'stn565, bit24'. > > I will investigate a bit on this now, testing your patches and fixup > my mistakes. > > best regards, > Hannes > > regarding the bits in the palmode in RASTER_CTRL, I already made a patch yesterday which fixes this: ac5c61b drivers/video/am335x-fb: Fix bits for LCD_PALMODE_RAWDATA definition That might explain why no one saw DMA issues before because setting it to stn565 mode might have not show any issues with the DMA (but that's just a guess). But I'm sure it was the wrong setting, I also crosschecked the RASTER_CTRL register value with a running kernel (4.4) and the stn565 bit is also not set for 16bpp mode. I tested it only on a 16bpp screen, it would be nice if someone could confirm that it works on a 24/32bpp screen. Martin
On 04/27/2016 01:21 AM, Martin Pietryka wrote: > The DMA was outputting the palette on the screen because the base > for the DMA was not after the palette. In addition to that, the ceiling was > also too high, this led that the output on the screen was shifted. This "fault" was masked in the past due to wrong handling of LCD_PALMODE_RAWDATA bit, which was fixed with commit ac5c61bfa6ad24a5384b9b89902e024a994f715f (drivers/video/am335x-fb: Fix bits for LCD_PALMODE_RAWDATA definition). So now the things are coming out and screen output is shifted. > NOTE: According to the TRM, even in 16/24bit mode a palette is required > in the first 32 bytes of the framebuffer. > > See also: > https://e2e.ti.com/support/arm/sitara_arm/f/791/p/234967/834483#834483 > > "In this mode, the LCDC will assume all information is data and thus you > need to ensure that the DMA points to the first pixel of data and not the > first entry in the frame buffer which is the beginning of the 512 byte > palette." Correct. > Signed-off-by: Martin Pietryka <martin.pietryka@chello.at> > --- > drivers/video/am335x-fb.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/am335x-fb.c b/drivers/video/am335x-fb.c > index f2b4c78..982db98 100644 > --- a/drivers/video/am335x-fb.c > +++ b/drivers/video/am335x-fb.c > @@ -128,6 +128,8 @@ int am335xfb_init(struct am335x_lcdpanel *panel) > /* palette default entry */ > memset((void *)gd->fb_base, 0, 0x20); > *(unsigned int *)gd->fb_base = 0x4000; > + /* point fb behind palette */ > + gd->fb_base += 0x20; In theory we don't need anymore this palette offset of 0x20 byte, because DMA is fetching startign behind this offset. But in praxis we have to use the offset also in future for compatibility reasons, because there may some OS which is writing to our bare-framebuffer starting at 32byte offset. For example, my vxWorks is doing so ;-( Also the wording within the TRM (spruh73l.pdf) about this topic isn't fully clear to me, on page 1842 they say: " 12-, 16-, and 24-BPP modes do not need a palette; i.e., the pixel data is the desired RGB value. However, the first 32 bytes are still considered a palette. The first entry should be 4000h (bit 14 is 1) while the remaining entries must be filled with 0. " Exactly as we do today. Few pages later, 1874, description from the RASTER_CTRL register they write for our case: "Data loading only For Raw Data (12/16/24 bpp) framebuffers, no palette lookup is employed." I've tested removing this offset and it works as expected: - fine display in u-boot - garbage within my vxWorks :-) Finally i think we have to leave it as it is, 0x20 bytes offset. > /* turn ON display through powercontrol function if accessible */ > if (0 != panel->panel_power_ctrl) > @@ -139,9 +141,9 @@ int am335xfb_init(struct am335x_lcdpanel *panel) > lcdhw->raster_ctrl = 0; > lcdhw->ctrl = LCD_CLK_DIVISOR(panel->pxl_clk_div) | LCD_RASTER_MODE; > lcdhw->lcddma_fb0_base = gd->fb_base; > - lcdhw->lcddma_fb0_ceiling = gd->fb_base + FBSIZE(panel) + 0x20; > + lcdhw->lcddma_fb0_ceiling = gd->fb_base + FBSIZE(panel); > lcdhw->lcddma_fb1_base = gd->fb_base; > - lcdhw->lcddma_fb1_ceiling = gd->fb_base + FBSIZE(panel) + 0x20; > + lcdhw->lcddma_fb1_ceiling = gd->fb_base + FBSIZE(panel); > lcdhw->lcddma_ctrl = LCD_DMA_BURST_SIZE(LCD_DMA_BURST_16); > > lcdhw->raster_timing0 = LCD_HORLSB(panel->hactive) | > @@ -180,8 +182,6 @@ int am335xfb_init(struct am335x_lcdpanel *panel) > > lcdhw->raster_ctrl = raster_ctrl; > > - gd->fb_base += 0x20; /* point fb behind palette */ > - > debug("am335x-fb: waiting picture to be stable.\n."); > mdelay(panel->pon_delay); > Reviewd-by: Hannes Schmelzer <oe5hpm@oevsv.at> Tested-by: Hannes Schmelzer <oe5hpm@oevsv.at> I've done my test with 32bit framebuffer. Martin, i think your'e testing with 16bit - right? many thanks for picking up this and best regards, Hannes - OE5HPM
On 04/27/2016 11:02 AM, Hannes Schmelzer wrote: > On 04/27/2016 01:21 AM, Martin Pietryka wrote: >> The DMA was outputting the palette on the screen because the base >> for the DMA was not after the palette. In addition to that, the ceiling was >> also too high, this led that the output on the screen was shifted. > This "fault" was masked in the past due to wrong handling of > LCD_PALMODE_RAWDATA bit, > which was fixed with commit ac5c61bfa6ad24a5384b9b89902e024a994f715f > (drivers/video/am335x-fb: Fix bits for LCD_PALMODE_RAWDATA definition). > > So now the things are coming out and screen output is shifted. >> NOTE: According to the TRM, even in 16/24bit mode a palette is required >> in the first 32 bytes of the framebuffer. >> >> See also: >> https://e2e.ti.com/support/arm/sitara_arm/f/791/p/234967/834483#834483 >> >> "In this mode, the LCDC will assume all information is data and thus you >> need to ensure that the DMA points to the first pixel of data and not the >> first entry in the frame buffer which is the beginning of the 512 byte >> palette." > Correct. >> Signed-off-by: Martin Pietryka<martin.pietryka@chello.at> >> --- >> drivers/video/am335x-fb.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/video/am335x-fb.c b/drivers/video/am335x-fb.c >> index f2b4c78..982db98 100644 >> --- a/drivers/video/am335x-fb.c >> +++ b/drivers/video/am335x-fb.c >> @@ -128,6 +128,8 @@ int am335xfb_init(struct am335x_lcdpanel *panel) >> /* palette default entry */ >> memset((void *)gd->fb_base, 0, 0x20); >> *(unsigned int *)gd->fb_base = 0x4000; >> + /* point fb behind palette */ >> + gd->fb_base += 0x20; > In theory we don't need anymore this palette offset of 0x20 byte, > because DMA is fetching startign behind this offset. > > But in praxis we have to use the offset also in future for > compatibility reasons, because there may some OS which is writing to > our bare-framebuffer starting at 32byte offset. > For example, my vxWorks is doing so ;-( > > Also the wording within the TRM (spruh73l.pdf) about this topic isn't > fully clear to me, on page 1842 they say: > " > 12-, 16-, and 24-BPP modes do not need a palette; i.e., the pixel data > is the desired RGB value. > However, the first 32 bytes are still considered a palette. The first > entry should be 4000h (bit 14 is 1) while the remaining entries must > be filled with 0. > " > > Exactly as we do today. > > Few pages later, 1874, description from the RASTER_CTRL register they > write for our case: > "Data loading only For Raw Data (12/16/24 bpp) framebuffers, no > palette lookup is employed." > > I've tested removing this offset and it works as expected: > - fine display in u-boot > - garbage within my vxWorks :-) > > Finally i think we have to leave it as it is, 0x20 bytes offset. >> /* turn ON display through powercontrol function if accessible */ >> if (0 != panel->panel_power_ctrl) >> @@ -139,9 +141,9 @@ int am335xfb_init(struct am335x_lcdpanel *panel) >> lcdhw->raster_ctrl = 0; >> lcdhw->ctrl = LCD_CLK_DIVISOR(panel->pxl_clk_div) | LCD_RASTER_MODE; >> lcdhw->lcddma_fb0_base = gd->fb_base; >> - lcdhw->lcddma_fb0_ceiling = gd->fb_base + FBSIZE(panel) + 0x20; >> + lcdhw->lcddma_fb0_ceiling = gd->fb_base + FBSIZE(panel); >> lcdhw->lcddma_fb1_base = gd->fb_base; >> - lcdhw->lcddma_fb1_ceiling = gd->fb_base + FBSIZE(panel) + 0x20; >> + lcdhw->lcddma_fb1_ceiling = gd->fb_base + FBSIZE(panel); >> lcdhw->lcddma_ctrl = LCD_DMA_BURST_SIZE(LCD_DMA_BURST_16); >> >> lcdhw->raster_timing0 = LCD_HORLSB(panel->hactive) | >> @@ -180,8 +182,6 @@ int am335xfb_init(struct am335x_lcdpanel *panel) >> >> lcdhw->raster_ctrl = raster_ctrl; >> >> - gd->fb_base += 0x20; /* point fb behind palette */ >> - >> debug("am335x-fb: waiting picture to be stable.\n."); >> mdelay(panel->pon_delay); >> > Reviewd-by: Hannes Schmelzer <oe5hpm@oevsv.at> > Tested-by: Hannes Schmelzer <oe5hpm@oevsv.at> > > I've done my test with 32bit framebuffer. > Martin, i think your'e testing with 16bit - right? Yes, I've done the testing and development with a 16bit display/framebuffer and it works as expected for me. Martin
diff --git a/drivers/video/am335x-fb.c b/drivers/video/am335x-fb.c index f2b4c78..982db98 100644 --- a/drivers/video/am335x-fb.c +++ b/drivers/video/am335x-fb.c @@ -128,6 +128,8 @@ int am335xfb_init(struct am335x_lcdpanel *panel) /* palette default entry */ memset((void *)gd->fb_base, 0, 0x20); *(unsigned int *)gd->fb_base = 0x4000; + /* point fb behind palette */ + gd->fb_base += 0x20; /* turn ON display through powercontrol function if accessible */ if (0 != panel->panel_power_ctrl) @@ -139,9 +141,9 @@ int am335xfb_init(struct am335x_lcdpanel *panel) lcdhw->raster_ctrl = 0; lcdhw->ctrl = LCD_CLK_DIVISOR(panel->pxl_clk_div) | LCD_RASTER_MODE; lcdhw->lcddma_fb0_base = gd->fb_base; - lcdhw->lcddma_fb0_ceiling = gd->fb_base + FBSIZE(panel) + 0x20; + lcdhw->lcddma_fb0_ceiling = gd->fb_base + FBSIZE(panel); lcdhw->lcddma_fb1_base = gd->fb_base; - lcdhw->lcddma_fb1_ceiling = gd->fb_base + FBSIZE(panel) + 0x20; + lcdhw->lcddma_fb1_ceiling = gd->fb_base + FBSIZE(panel); lcdhw->lcddma_ctrl = LCD_DMA_BURST_SIZE(LCD_DMA_BURST_16); lcdhw->raster_timing0 = LCD_HORLSB(panel->hactive) | @@ -180,8 +182,6 @@ int am335xfb_init(struct am335x_lcdpanel *panel) lcdhw->raster_ctrl = raster_ctrl; - gd->fb_base += 0x20; /* point fb behind palette */ - debug("am335x-fb: waiting picture to be stable.\n."); mdelay(panel->pon_delay);
The DMA was outputting the palette on the screen because the base for the DMA was not after the palette. In addition to that, the ceiling was also too high, this led that the output on the screen was shifted. NOTE: According to the TRM, even in 16/24bit mode a palette is required in the first 32 bytes of the framebuffer. See also: https://e2e.ti.com/support/arm/sitara_arm/f/791/p/234967/834483#834483 "In this mode, the LCDC will assume all information is data and thus you need to ensure that the DMA points to the first pixel of data and not the first entry in the frame buffer which is the beginning of the 512 byte palette." Signed-off-by: Martin Pietryka <martin.pietryka@chello.at> --- drivers/video/am335x-fb.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)