[U-Boot] [PATCH 2/2] video: bcm2835: fix various output modes

[U-Boot] [PATCH 2/2] video: bcm2835: fix various output modes

[Andre Heider]

Depending on the firmware's video options [1] the active SDTV or
HDTV mode can yield a framebuffer with noncontiguous horizontal lines,
giving a messed up display, for both, u-boot and the loaded kernel.

To always archive the required contiguousness for the used 16bpp, round
the framebuffer width down so its aligned to a width of 16.

[1] http://elinux.org/RPiconfig#Video_mode_options

Signed-off-by: Andre Heider <a.heider@gmail.com>
---
 drivers/video/bcm2835.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/bcm2835.c b/drivers/video/bcm2835.c
index 58a6163..1404340 100644
--- a/drivers/video/bcm2835.c
+++ b/drivers/video/bcm2835.c
@@ -52,7 +52,7 @@ void lcd_ctrl_init(void *lcdbase)
 		return;
 	}
 
-	w = msg_query->physical_w_h.body.resp.width;
+	w = ROUND(msg_query->physical_w_h.body.resp.width, 16);
 	h = msg_query->physical_w_h.body.resp.height;
 
 	debug("bcm2835: Setting up display for %d x %d\n", w, h);
-- 
1.8.3.2

[U-Boot] [PATCH 2/2] video: bcm2835: fix various output modes

[Stephen Warren]

On 10/22/2013 09:27 PM, Andre Heider wrote:
> Depending on the firmware's video options [1] the active SDTV or
> HDTV mode can yield a framebuffer with noncontiguous horizontal lines,
> giving a messed up display, for both, u-boot and the loaded kernel.
> 
> To always archive the required contiguousness for the used 16bpp, round
> the framebuffer width down so its aligned to a width of 16.

This doesn't sound like the correct approach. By doing this, either the
SET_PHYSICAL_W_H request will fail since the requested mode doesn't
match the connected display device, or perhaps it'll work, but end up
with a frame-buffer that's a different resolution than the video signal,
so the HW will scale the image slightly, which will reduce quality.

Instead, can't you obtain the buffer width and stride separately, and
then configure the LCD core based on both those values, rather than
assuming they're the same?

[U-Boot] [PATCH 2/2] video: bcm2835: fix various output modes

[Andre Heider]

On Wed, Oct 23, 2013 at 05:57:53PM +0100, Stephen Warren wrote:
> On 10/22/2013 09:27 PM, Andre Heider wrote:
> > Depending on the firmware's video options [1] the active SDTV or
> > HDTV mode can yield a framebuffer with noncontiguous horizontal lines,
> > giving a messed up display, for both, u-boot and the loaded kernel.
> > 
> > To always archive the required contiguousness for the used 16bpp, round
> > the framebuffer width down so its aligned to a width of 16.
> 
> This doesn't sound like the correct approach. By doing this, either the
> SET_PHYSICAL_W_H request will fail since the requested mode doesn't
> match the connected display device, or perhaps it'll work, but end up
> with a frame-buffer that's a different resolution than the video signal,
> so the HW will scale the image slightly, which will reduce quality.

SET_PHYSICAL_W_H works in any case, but yeah, it does get "funky"
resolutions in some cases.

The thing is, the firmware is stupid to begin with (duh). In my case, I do
have both outputs connected. For the analog one, I have to set sdtv_*
and overscan_* config settings.
When the HDMI connection is active, the analog output is inactive, but
yet the firmware applies the sdtv_* and overscan_* settings to the HDMI
output too, so the clean 1:1 resolution is already gone...

> Instead, can't you obtain the buffer width and stride separately, and
> then configure the LCD core based on both those values, rather than
> assuming they're the same?

What I did try is to get the overscan values and add those to the
requested resolution (that's where patch 1 comes from). That gives saner
resolutions, but some are still broken for me.

I agree that getting the pitch value would be the right thing to do, I
do some digging to find a spot where to shove that into.

Thanks,
Andre

[U-Boot] [PATCH 2/2] video: bcm2835: fix various output modes

[Andre Heider]

Hi Stephen,

On Wed, Oct 23, 2013 at 05:57:53PM +0100, Stephen Warren wrote:
> Instead, can't you obtain the buffer width and stride separately, and
> then configure the LCD core based on both those values, rather than
> assuming they're the same?

I just sent a v2 patch to do exactly that.

Regarding overscan, I wonder why you zero out the initial overscan
settings.

If it was just to gain a contiguous frame buffer, can we keep the
overscan settings now (assuming my v2 patch is accepted)?
That way the framebuffer doesn't get cut off on the sides for old
analog SDTV display devices.
For HDTV, that means there's a border - but only if the user set the
overscan_* values.

Thanks,
Andre

[U-Boot] [PATCH 2/2] video: bcm2835: fix various output modes

[Stephen Warren]

On 10/24/2013 07:14 PM, Andre Heider wrote:
> Hi Stephen,
> 
> On Wed, Oct 23, 2013 at 05:57:53PM +0100, Stephen Warren wrote:
>> Instead, can't you obtain the buffer width and stride separately, and
>> then configure the LCD core based on both those values, rather than
>> assuming they're the same?
> 
> I just sent a v2 patch to do exactly that.
> 
> Regarding overscan, I wonder why you zero out the initial overscan
> settings.

I think it was because the firmware incorrectly decided by default that
my HDMI device needed overscan, when in fact it can display the entire
signal it's sent. If there's a way to control that in the config file,
then perhaps that might be possible instead, or perhaps I should file a
bug against the firmware, assuming I'm remembering correctly...

[U-Boot] [PATCH v2 2/2] video: bcm2835: respect the pitch value

[Andre Heider]

Depending on the firmware's video options [1] the active SDTV or
HDTV mode can yield a framebuffer with noncontiguous horizontal lines,
giving a messed up display, for both, u-boot and the loaded kernel.

Fix this by setting lcd_line_length to the pitch value of the configured
framebuffer.

[1] http://elinux.org/RPiconfig#Video_mode_options

Signed-off-by: Andre Heider <a.heider@gmail.com>
---
 drivers/video/bcm2835.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/video/bcm2835.c b/drivers/video/bcm2835.c
index 58a6163..431b8a8 100644
--- a/drivers/video/bcm2835.c
+++ b/drivers/video/bcm2835.c
@@ -14,6 +14,8 @@ DECLARE_GLOBAL_DATA_PTR;
 /* Global variables that lcd.c expects to exist */
 vidinfo_t panel_info;
 
+static u32 bcm2835_pitch;
+
 struct msg_query {
 	struct bcm2835_mbox_hdr hdr;
 	struct bcm2835_mbox_tag_physical_w_h physical_w_h;
@@ -30,6 +32,7 @@ struct msg_setup {
 	struct bcm2835_mbox_tag_virtual_offset virtual_offset;
 	struct bcm2835_mbox_tag_overscan overscan;
 	struct bcm2835_mbox_tag_allocate_buffer allocate_buffer;
+	struct bcm2835_mbox_tag_pitch pitch;
 	u32 end_tag;
 };
 
@@ -80,6 +83,7 @@ void lcd_ctrl_init(void *lcdbase)
 	msg_setup->overscan.body.req.right = 0;
 	BCM2835_MBOX_INIT_TAG(&msg_setup->allocate_buffer, ALLOCATE_BUFFER);
 	msg_setup->allocate_buffer.body.req.alignment = 0x100;
+	BCM2835_MBOX_INIT_TAG_NO_REQ(&msg_setup->pitch, GET_PITCH);
 
 	ret = bcm2835_mbox_call_prop(BCM2835_MBOX_PROP_CHAN, &msg_setup->hdr);
 	if (ret) {
@@ -90,6 +94,7 @@ void lcd_ctrl_init(void *lcdbase)
 
 	w = msg_setup->physical_w_h.body.resp.width;
 	h = msg_setup->physical_w_h.body.resp.height;
+	bcm2835_pitch = msg_setup->pitch.body.resp.pitch;
 
 	debug("bcm2835: Final resolution is %d x %d\n", w, h);
 
@@ -102,4 +107,6 @@ void lcd_ctrl_init(void *lcdbase)
 
 void lcd_enable(void)
 {
+	if (bcm2835_pitch)
+		lcd_line_length = bcm2835_pitch;
 }
-- 
1.8.3.2

[U-Boot] [PATCH v2 2/2] video: bcm2835: respect the pitch value

[Stephen Warren]

On 10/24/2013 07:00 PM, Andre Heider wrote:
> Depending on the firmware's video options [1] the active SDTV or
> HDTV mode can yield a framebuffer with noncontiguous horizontal lines,
> giving a messed up display, for both, u-boot and the loaded kernel.
> 
> Fix this by setting lcd_line_length to the pitch value of the configured
> framebuffer.

This sounds like the right concept.

> diff --git a/drivers/video/bcm2835.c b/drivers/video/bcm2835.c

>  void lcd_enable(void)
>  {
> +	if (bcm2835_pitch)

Why make this conditional? Does the firmware sometimes not return the
correct pitch and/or does lcd_enable() get called before lcd_ctrl_init()
so the global hasn't been set up? Either of those seem like nasty bugs
that should be fixed...

> +		lcd_line_length = bcm2835_pitch;

Why set lcd_line_length at a different time than the mailbox message is
executed? Can't lcd_line_length be set at the end of lcd_ctrl_init(),
thus avoiding the global variable bcm2835_pitch?

>  }

[U-Boot] [PATCH v2 2/2] video: bcm2835: respect the pitch value

[Anatolij Gustschin]

On Thu, 24 Oct 2013 20:00:41 +0200
Andre Heider <a.heider@gmail.com> wrote:
...
> @@ -90,6 +94,7 @@ void lcd_ctrl_init(void *lcdbase)
>  
>  	w = msg_setup->physical_w_h.body.resp.width;
>  	h = msg_setup->physical_w_h.body.resp.height;
> +	bcm2835_pitch = msg_setup->pitch.body.resp.pitch;
>  
>  	debug("bcm2835: Final resolution is %d x %d\n", w, h);
>  
> @@ -102,4 +107,6 @@ void lcd_ctrl_init(void *lcdbase)
>  
>  void lcd_enable(void)
>  {
> +	if (bcm2835_pitch)
> +		lcd_line_length = bcm2835_pitch;
>  }

setting lcd_line_length in lcd_enable() is wrong, it should
happen earlier, before lcd_clear() is called in the lcd.c
driver. I suggest making the lcd_get_size() in lcd.c a weak
function and then provide bcm2835 specific lcd_get_size()
which sets the pitch as needed:

diff --git a/common/lcd.c b/common/lcd.c
index 5dd7948..60faa62 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -386,8 +386,13 @@ static void test_pattern(void)
 /************************************************************************/
 /* ** GENERIC Initialization Routines                                  */
 /************************************************************************/
-
-int lcd_get_size(int *line_length)
+/*
+ * Implement a weak default function for getting the length/size
+ * from panel_info parameters. With some boards/drivers the
+ * length might need adjustments, so allow defining the driver
+ * specific lcd_get_size() function.
+ */
+__weak int lcd_get_size(int *line_length)
 {
        *line_length = (panel_info.vl_col * NBITS(panel_info.vl_bpix)) / 8;
        return *line_length * panel_info.vl_row;

and

diff --git a/drivers/video/bcm2835.c b/drivers/video/bcm2835.c
index 58a6163..45b470a 100644
--- a/drivers/video/bcm2835.c
+++ b/drivers/video/bcm2835.c
@@ -103,3 +103,9 @@ void lcd_ctrl_init(void *lcdbase)
 void lcd_enable(void)
 {
 }
+
+void lcd_get_size(int *line_length)
+{
+       *line_length = bcm2835_pitch;
+       return *line_length * panel_info.vl_row;
+}

Anatolij

[U-Boot] [PATCH] lcd: allow overriding lcd_get_size()

[Anatolij Gustschin]

Remove the redundant lcd_line_length initialisation which
sneaked in when an earlier version of the patch of commit
6d330719 has been rebased.

Some lcd drivers need to setup lcd_line_length not from the
panel_info parameters but by different means. Make the
lcd_get_size() weak to allow setting lcd_line_length in
a driver specific way.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
---
 common/lcd.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/common/lcd.c b/common/lcd.c
index 5dd7948..56bf067 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -386,8 +386,13 @@ static void test_pattern(void)
 /************************************************************************/
 /* ** GENERIC Initialization Routines					*/
 /************************************************************************/
-
-int lcd_get_size(int *line_length)
+/*
+ * With most lcd drivers the line length is set up
+ * by calculating it from panel_info parameters. Some
+ * drivers need to calculate the line length differently,
+ * so make the function weak to allow overriding it.
+ */
+__weak int lcd_get_size(int *line_length)
 {
 	*line_length = (panel_info.vl_col * NBITS(panel_info.vl_bpix)) / 8;
 	return *line_length * panel_info.vl_row;
@@ -495,7 +500,6 @@ static int lcd_init(void *lcdbase)
 	debug("[LCD] Using LCD frambuffer at %p\n", lcd_base);
 
 	lcd_get_size(&lcd_line_length);
-	lcd_line_length = (panel_info.vl_col * NBITS(panel_info.vl_bpix)) / 8;
 	lcd_is_enabled = 1;
 	lcd_clear();
 	lcd_enable();
-- 
1.7.9.5

[U-Boot] [PATCH] lcd: allow overriding lcd_get_size()

[Anatolij Gustschin]

On Sat,  9 Nov 2013 11:00:09 +0100
Anatolij Gustschin <agust@denx.de> wrote:

> Remove the redundant lcd_line_length initialisation which
> sneaked in when an earlier version of the patch of commit
> 6d330719 has been rebased.
> 
> Some lcd drivers need to setup lcd_line_length not from the
> panel_info parameters but by different means. Make the
> lcd_get_size() weak to allow setting lcd_line_length in
> a driver specific way.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> ---
>  common/lcd.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

applied to u-boot-video/master, thanks!

Anatolij

[U-Boot] [PATCH v3 2/2] video: bcm2835: respect the pitch value

[Anatolij Gustschin]

From: Andre Heider <a.heider@gmail.com>

Depending on the firmware's video options [1] the active SDTV or
HDTV mode can yield a framebuffer with noncontiguous horizontal lines,
giving a messed up display, for both, u-boot and the loaded kernel.

Fix this by setting lcd_line_length to the pitch value of the configured
framebuffer.

[1] http://elinux.org/RPiconfig#Video_mode_options

Signed-off-by: Andre Heider <a.heider@gmail.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
Changes in v3:
 - set lcd_line_length by driver specific lcd_get_size() since
   setting it in lcd_enable() is not correct. This patch version
   depends on patch http://patchwork.ozlabs.org/patch/289957
   Please test these both patches on rpi_b, thanks!

 drivers/video/bcm2835.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/video/bcm2835.c b/drivers/video/bcm2835.c
index 58a6163..1f18231 100644
--- a/drivers/video/bcm2835.c
+++ b/drivers/video/bcm2835.c
@@ -14,6 +14,8 @@ DECLARE_GLOBAL_DATA_PTR;
 /* Global variables that lcd.c expects to exist */
 vidinfo_t panel_info;
 
+static u32 bcm2835_pitch;
+
 struct msg_query {
 	struct bcm2835_mbox_hdr hdr;
 	struct bcm2835_mbox_tag_physical_w_h physical_w_h;
@@ -30,6 +32,7 @@ struct msg_setup {
 	struct bcm2835_mbox_tag_virtual_offset virtual_offset;
 	struct bcm2835_mbox_tag_overscan overscan;
 	struct bcm2835_mbox_tag_allocate_buffer allocate_buffer;
+	struct bcm2835_mbox_tag_pitch pitch;
 	u32 end_tag;
 };
 
@@ -80,6 +83,7 @@ void lcd_ctrl_init(void *lcdbase)
 	msg_setup->overscan.body.req.right = 0;
 	BCM2835_MBOX_INIT_TAG(&msg_setup->allocate_buffer, ALLOCATE_BUFFER);
 	msg_setup->allocate_buffer.body.req.alignment = 0x100;
+	BCM2835_MBOX_INIT_TAG_NO_REQ(&msg_setup->pitch, GET_PITCH);
 
 	ret = bcm2835_mbox_call_prop(BCM2835_MBOX_PROP_CHAN, &msg_setup->hdr);
 	if (ret) {
@@ -90,6 +94,7 @@ void lcd_ctrl_init(void *lcdbase)
 
 	w = msg_setup->physical_w_h.body.resp.width;
 	h = msg_setup->physical_w_h.body.resp.height;
+	bcm2835_pitch = msg_setup->pitch.body.resp.pitch;
 
 	debug("bcm2835: Final resolution is %d x %d\n", w, h);
 
@@ -103,3 +108,9 @@ void lcd_ctrl_init(void *lcdbase)
 void lcd_enable(void)
 {
 }
+
+int lcd_get_size(int *line_length)
+{
+	*line_length = bcm2835_pitch;
+	return *line_length * panel_info.vl_row;
+}
-- 
1.7.9.5

[U-Boot] [PATCH v3 2/2] video: bcm2835: respect the pitch value

[Andre Heider]

Hi Anatolij,

On Sat, Nov 09, 2013 at 11:07:53AM +0100, Anatolij Gustschin wrote:
> From: Andre Heider <a.heider@gmail.com>
> 
> Depending on the firmware's video options [1] the active SDTV or
> HDTV mode can yield a framebuffer with noncontiguous horizontal lines,
> giving a messed up display, for both, u-boot and the loaded kernel.
> 
> Fix this by setting lcd_line_length to the pitch value of the configured
> framebuffer.
> 
> [1] http://elinux.org/RPiconfig#Video_mode_options
> 
> Signed-off-by: Andre Heider <a.heider@gmail.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
> Changes in v3:
>  - set lcd_line_length by driver specific lcd_get_size() since
>    setting it in lcd_enable() is not correct. This patch version
>    depends on patch http://patchwork.ozlabs.org/patch/289957
>    Please test these both patches on rpi_b, thanks!

I just tested these on my rpi, and confirm that it still works.

Thanks,
Andre

[U-Boot] [PATCH v3 2/2] video: bcm2835: respect the pitch value

[Anatolij Gustschin]

Hi Andre,

On Sat, 9 Nov 2013 13:25:58 +0100
Andre Heider <a.heider@gmail.com> wrote:
...
> I just tested these on my rpi, and confirm that it still works.

Thanks for testing!

Anatolij

[U-Boot] [PATCH v3 2/2] video: bcm2835: respect the pitch value

[Stephen Warren]

On 11/09/2013 03:07 AM, Anatolij Gustschin wrote:
> From: Andre Heider <a.heider@gmail.com>
> 
> Depending on the firmware's video options [1] the active SDTV or
> HDTV mode can yield a framebuffer with noncontiguous horizontal lines,
> giving a messed up display, for both, u-boot and the loaded kernel.
> 
> Fix this by setting lcd_line_length to the pitch value of the configured
> framebuffer.
> 
> [1] http://elinux.org/RPiconfig#Video_mode_options

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

[U-Boot] [PATCH v3 2/2] video: bcm2835: respect the pitch value

[Anatolij Gustschin]

On Sat,  9 Nov 2013 11:07:53 +0100
Anatolij Gustschin <agust@denx.de> wrote:

> From: Andre Heider <a.heider@gmail.com>
> 
> Depending on the firmware's video options [1] the active SDTV or
> HDTV mode can yield a framebuffer with noncontiguous horizontal lines,
> giving a messed up display, for both, u-boot and the loaded kernel.
> 
> Fix this by setting lcd_line_length to the pitch value of the configured
> framebuffer.
> 
> [1] http://elinux.org/RPiconfig#Video_mode_options
> 
> Signed-off-by: Andre Heider <a.heider@gmail.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
> Changes in v3:
>  - set lcd_line_length by driver specific lcd_get_size() since
>    setting it in lcd_enable() is not correct. This patch version
>    depends on patch http://patchwork.ozlabs.org/patch/289957
>    Please test these both patches on rpi_b, thanks!
> 
>  drivers/video/bcm2835.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)

applied to u-boot-video/master, thanks!

Anatolij