[U-Boot] [PATCH 2/2] video: bcm2835: fix various output modes
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);
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?
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
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
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...
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; }
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?
}
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
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();
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
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; +}
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
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
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.
Acked-by: Stephen Warren swarren@wwwdotorg.org
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
participants (3)
-
Anatolij Gustschin -
Andre Heider -
Stephen Warren