[PATCH 3/5] fbdev: sh_mobile_lcdc: increase maximum framebuffer size

[PATCH 3/5] fbdev: sh_mobile_lcdc: increase maximum framebuffer size

[Guennadi Liakhovetski]

LCDC hardware can support 1920x1080 formats, adjust the driver to cover them.
Besides, instead of guessing some "reasonable" validity checks, only verify
values in .fb_check_var(), that we are sure, we cannot support.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/video/sh_mobile_lcdcfb.c |   26 ++++++++------------------
 1 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
index a87dace..8e8d698 100644
--- a/drivers/video/sh_mobile_lcdcfb.c
+++ b/drivers/video/sh_mobile_lcdcfb.c
@@ -54,8 +54,8 @@ static int lcdc_shared_regs[] = {
 };
 #define NR_SHARED_REGS ARRAY_SIZE(lcdc_shared_regs)
 
-#define DEFAULT_XRES 1280
-#define DEFAULT_YRES 1024
+#define MAX_XRES 1920
+#define MAX_YRES 1080
 
 static unsigned long lcdc_offs_mainlcd[NR_CH_REGS] = {
 	[LDDCKPAT1R] = 0x400,
@@ -914,22 +914,12 @@ static int sh_mobile_check_var(struct fb_var_screeninfo *var, struct fb_info *in
 {
 	struct sh_mobile_lcdc_chan *ch = info->par;
 
-	if (var->xres < 160 || var->xres > 1920 ||
-	    var->yres < 120 || var->yres > 1080 ||
-	    var->left_margin < 32 || var->left_margin > 320 ||
-	    var->right_margin < 12 || var->right_margin > 240 ||
-	    var->upper_margin < 12 || var->upper_margin > 120 ||
-	    var->lower_margin < 1 || var->lower_margin > 64 ||
-	    var->hsync_len < 32 || var->hsync_len > 240 ||
-	    var->vsync_len < 2 || var->vsync_len > 64 ||
-	    var->pixclock < 6000 || var->pixclock > 40000 ||
+	if (var->xres > MAX_XRES || var->yres > MAX_YRES ||
 	    var->xres * var->yres * (ch->cfg.bpp / 8) * 2 > info->fix.smem_len) {
-		dev_warn(info->dev, "Invalid info: %u %u %u %u %u %u %u %u %u!\n",
-			 var->xres, var->yres,
-			 var->left_margin, var->right_margin,
-			 var->upper_margin, var->lower_margin,
-			 var->hsync_len, var->vsync_len,
-			 var->pixclock);
+		dev_warn(info->dev, "Invalid info: %u-%u-%u-%u x %u-%u-%u-%u @ %ukHz!\n",
+			 var->left_margin, var->xres, var->right_margin, var->hsync_len,
+			 var->upper_margin, var->yres, var->lower_margin, var->vsync_len,
+			 PICOS2KHZ(var->pixclock));
 		return -EINVAL;
 	}
 	return 0;
@@ -1225,7 +1215,7 @@ static int __devinit sh_mobile_lcdc_probe(struct platform_device *pdev)
 		}
 
 		if (!mode)
-			max_size = DEFAULT_XRES * DEFAULT_YRES;
+			max_size = MAX_XRES * MAX_YRES;
 		else if (max_cfg)
 			dev_dbg(&pdev->dev, "Found largest videomode %ux%u\n",
 				max_cfg->xres, max_cfg->yres);
-- 
1.7.2.3

Re: [PATCH 3/5] fbdev: sh_mobile_lcdc: increase maximum framebuffer

[Magnus Damm]

Hi Guennadi,

On Sun, Dec 26, 2010 at 6:44 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Paul, apart from increasing the fb size to 1080p, this patch also fixes a
> regression, which leads to unusable by applications framebuffer on some
> platforms, including migor. This happens, when one of parameters lies
> outside of limits, being checked in sh_mobile_check_var(). On migor it is
>
>                .left_margin = 0,
>
> Then the fb-console is working, but, e.g., mplayer is not. So, we need
> this patch in 2.6.37 too, please. Unfortunately, it also introduces a
> wrong format printk format, which a later patch of yours fixes, so, you
> might want to push that patch too, although, that's not that important.

It's great that you work on fixing this issue, thank you.

In the long run I wonder if it's best to try to get rid of the
MAX_XRES/MAX_YRES stuff from the LCDC driver. I understand that you
need to allocate the frame buffer memory early on, but it would be
much better if the maximum values could be kept in platform data. The
maximum resolution varies with CPU type, so a one-for-all modification
like this may break older platforms.

Thanks,

/ magnus

Re: [PATCH 3/5] fbdev: sh_mobile_lcdc: increase maximum framebuffer

[Guennadi Liakhovetski]

On Mon, 27 Dec 2010, Magnus Damm wrote:

> Hi Guennadi,
> 
> On Sun, Dec 26, 2010 at 6:44 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Paul, apart from increasing the fb size to 1080p, this patch also fixes a
> > regression, which leads to unusable by applications framebuffer on some
> > platforms, including migor. This happens, when one of parameters lies
> > outside of limits, being checked in sh_mobile_check_var(). On migor it is
> >
> >                .left_margin = 0,
> >
> > Then the fb-console is working, but, e.g., mplayer is not. So, we need
> > this patch in 2.6.37 too, please. Unfortunately, it also introduces a
> > wrong format printk format, which a later patch of yours fixes, so, you
> > might want to push that patch too, although, that's not that important.
> 
> It's great that you work on fixing this issue, thank you.
> 
> In the long run I wonder if it's best to try to get rid of the
> MAX_XRES/MAX_YRES stuff from the LCDC driver. I understand that you
> need to allocate the frame buffer memory early on, but it would be
> much better if the maximum values could be kept in platform data. The
> maximum resolution varies with CPU type, so a one-for-all modification
> like this may break older platforms.

Hm, I'l not sure how it can break other platforms. You mean they just will 
fail to allocate that much RAM? Otherwise it shouldn't cause any problems. 
But yes, I'll make an incremental patch to let platform override those 
values, and use defaults otherwise, would that be ok?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 3/5] fbdev: sh_mobile_lcdc: increase maximum framebuffer

[Magnus Damm]

On Mon, Dec 27, 2010 at 10:41 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Mon, 27 Dec 2010, Magnus Damm wrote:
>> On Sun, Dec 26, 2010 at 6:44 AM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > Paul, apart from increasing the fb size to 1080p, this patch also fixes a
>> > regression, which leads to unusable by applications framebuffer on some
>> > platforms, including migor. This happens, when one of parameters lies
>> > outside of limits, being checked in sh_mobile_check_var(). On migor it is
>> >
>> >                .left_margin = 0,
>> >
>> > Then the fb-console is working, but, e.g., mplayer is not. So, we need
>> > this patch in 2.6.37 too, please. Unfortunately, it also introduces a
>> > wrong format printk format, which a later patch of yours fixes, so, you
>> > might want to push that patch too, although, that's not that important.
>>
>> It's great that you work on fixing this issue, thank you.
>>
>> In the long run I wonder if it's best to try to get rid of the
>> MAX_XRES/MAX_YRES stuff from the LCDC driver. I understand that you
>> need to allocate the frame buffer memory early on, but it would be
>> much better if the maximum values could be kept in platform data. The
>> maximum resolution varies with CPU type, so a one-for-all modification
>> like this may break older platforms.
>
> Hm, I'l not sure how it can break other platforms. You mean they just will
> fail to allocate that much RAM? Otherwise it shouldn't cause any problems.
> But yes, I'll make an incremental patch to let platform override those
> values, and use defaults otherwise, would that be ok?

There seem to be two ways to specify the maximum LCDC resolution today:

1) The highest resolution specified by the lcd_cfg array.
2) The default MAX_XRES / MAX_YRES.

Your patch is good in the sense that it starts using the limits from
above in check_var(), but only for case 2). Perhaps it would be good
to walk the lcd_cfg arrray to handle case 1) too?

Also, about case 2), I don't really see why we need it. I would much
prefer to treat missing platform data information as an error case.

With the current hard coded MAX_XRES / MAX_YRES the driver needs to be
updated now and then. I'm sure there are many good reasons to update
the driver, but having to do so just to increase the max resolution
seems a bit silly to me. Also, with the MAX_XRES / MAX_YRES there is a
silent assumption about the maximum resolution that has nothing to do
with the LCDC hardware capabilities at all.

I understand you may need to allocate memory early for the hotplug
HDMI case, but it must be possible to specify the maximum resolution
with platform data in that case too, no?

As for breaking other platforms, if there are any users of the LCDC
driver in mode 2) above that assume that MAX_XRES / MAX_YRES allocate
720p memory then they may break with this patch. The pain may be
limited to allocating more memory than actually needed though, but
that may cause a run-time error anyway depending on the MAX_ZONEORDER
kernel configuration.

So if possible I'd like to get rid of 2) above. And use the lcd_cfg
array in check_var(). Does it make sense?

Thanks,

/ magnus