[PATCH 0/2] fbdev: sh_mobile_lcdc: YUV framebuffer support

[PATCH 0/2] fbdev: sh_mobile_lcdc: YUV framebuffer support

[Damian Hobson-Garcia]

These patches add support for 12, 16, and 24 bit YUV framebuffers.

These patches are a reworking of an earlier submitted patch 
"Add NV12 input framebuffer support" to include the two other modes.

Additionally updated are:

* Y and C plane ordering.  When double-buffering both Y planes appear before
  the C planes (Y-Y-C-C), instead of Y-C-Y-C.  
* In YUV 420 mode, panning is only possible in 2 line increments
* Additionally in YUV 420 mode the value of yres must be set to an even number
* The value of .nonstd in struct sh_mobile_lcdc_chan_cfg from the platform data is
  exposed to applications via the .nonstd element of struct fb_var_screeninfo.
  Additionally this value is written to bits 16-31 of LDDFR in the LCDC.
* Chip dependent flags for the bits of LDDFR greater that bit 17 are defined
* Add a userspace include file <linux/sh_mobile_fb.h> as a place to hold defines
  and future ioctl definitions.

Damian Hobson-Garcia (2):
  fbdev: sh_mobile_lcdc: Add YUV input support
  fbdev: sh_mobile_lcdc: Define additional .nonstd flags for sh7372

 arch/arm/mach-shmobile/include/mach/sh7372.h |   11 ++
 drivers/video/sh_mobile_lcdcfb.c             |  142 +++++++++++++++++++++-----
 drivers/video/sh_mobile_lcdcfb.h             |    2 +-
 include/linux/sh_mobile_fb.h                 |   14 +++
 include/video/sh_mobile_lcdc.h               |    1 +
 5 files changed, 141 insertions(+), 29 deletions(-)
 create mode 100644 include/linux/sh_mobile_fb.h

[PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Damian Hobson-Garcia]

Supports YCbCr420sp, YCbCr422sp, and YCbCr44sp, formats (bpp = 12, 16, and 24)
respectively.
Set .nonstd = SH_FB_YUV to enable YUV mode, and use bpp to distiguish between
the 3 modes.
Due to the encoding of YUV data, the framebuffer will clear to green instead of black.

Signed-off-by: Damian Hobson-Garcia <dhobsong@igel.co.jp>
---
 drivers/video/sh_mobile_lcdcfb.c |  142 ++++++++++++++++++++++++++++++--------
 drivers/video/sh_mobile_lcdcfb.h |    2 +-
 include/linux/sh_mobile_fb.h     |   14 ++++
 include/video/sh_mobile_lcdc.h   |    1 +
 4 files changed, 130 insertions(+), 29 deletions(-)
 create mode 100644 include/linux/sh_mobile_fb.h

diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
index bf12e53..f2814ea 100644
--- a/drivers/video/sh_mobile_lcdcfb.c
+++ b/drivers/video/sh_mobile_lcdcfb.c
@@ -24,6 +24,7 @@
 #include <video/sh_mobile_lcdc.h>
 #include <asm/atomic.h>
 
+#include <linux/sh_mobile_fb.h>
 #include "sh_mobile_lcdcfb.h"
 
 #define SIDE_B_OFFSET 0x1000
@@ -67,6 +68,7 @@ static unsigned long lcdc_offs_mainlcd[NR_CH_REGS] = {
 	[LDSM1R] = 0x428,
 	[LDSM2R] = 0x42c,
 	[LDSA1R] = 0x430,
+	[LDSA2R] = 0x434,
 	[LDMLSR] = 0x438,
 	[LDHCNR] = 0x448,
 	[LDHSYNR] = 0x44c,
@@ -151,6 +153,7 @@ static bool banked(int reg_nr)
 	case LDDFR:
 	case LDSM1R:
 	case LDSA1R:
+	case LDSA2R:
 	case LDMLSR:
 	case LDHCNR:
 	case LDHSYNR:
@@ -463,6 +466,7 @@ static int sh_mobile_lcdc_start(struct sh_mobile_lcdc_priv *priv)
 	struct sh_mobile_lcdc_board_cfg	*board_cfg;
 	unsigned long tmp;
 	int bpp = 0;
+	unsigned long ldddsr;
 	int k, m;
 	int ret = 0;
 
@@ -541,16 +545,21 @@ static int sh_mobile_lcdc_start(struct sh_mobile_lcdc_priv *priv)
 	}
 
 	/* word and long word swap */
-	switch (bpp) {
-	case 16:
-		lcdc_write(priv, _LDDDSR, lcdc_read(priv, _LDDDSR) | 6);
-		break;
-	case 24:
-		lcdc_write(priv, _LDDDSR, lcdc_read(priv, _LDDDSR) | 7);
-		break;
-	case 32:
-		lcdc_write(priv, _LDDDSR, lcdc_read(priv, _LDDDSR) | 4);
-		break;
+	ldddsr = lcdc_read(priv, _LDDDSR);
+	if  (priv->ch[0].info->var.nonstd & SH_FB_YUV)
+		lcdc_write(priv, _LDDDSR, ldddsr | 7);
+	else {
+		switch (bpp) {
+		case 16:
+			lcdc_write(priv, _LDDDSR, ldddsr | 6);
+			break;
+		case 24:
+			lcdc_write(priv, _LDDDSR, ldddsr | 7);
+			break;
+		case 32:
+			lcdc_write(priv, _LDDDSR, ldddsr | 4);
+			break;
+		}
 	}
 
 	for (k = 0; k < ARRAY_SIZE(priv->ch); k++) {
@@ -561,21 +570,40 @@ static int sh_mobile_lcdc_start(struct sh_mobile_lcdc_priv *priv)
 
 		/* set bpp format in PKF[4:0] */
 		tmp = lcdc_read_chan(ch, LDDFR);
-		tmp &= ~0x0001001f;
-		switch (ch->info->var.bits_per_pixel) {
-		case 16:
-			tmp |= 0x03;
-			break;
-		case 24:
-			tmp |= 0x0b;
-			break;
-		case 32:
-			break;
+		tmp &= ~0x0003031f;
+		if (ch->info->var.nonstd & SH_FB_YUV) {
+			tmp |= (ch->info->var.nonstd << 16);
+			switch (ch->info->var.bits_per_pixel) {
+			case 12:
+				break;
+			case 16:
+				tmp |= (0x1 << 8);
+				break;
+			case 24:
+				tmp |= (0x2 << 8);
+				break;
+			}
+		} else {
+			switch (ch->info->var.bits_per_pixel) {
+			case 16:
+				tmp |= 0x03;
+				break;
+			case 24:
+				tmp |= 0x0b;
+				break;
+			case 32:
+				break;
+			}
 		}
 		lcdc_write_chan(ch, LDDFR, tmp);
 
 		/* point out our frame buffer */
 		lcdc_write_chan(ch, LDSA1R, ch->info->fix.smem_start);
+		if (ch->info->var.nonstd & SH_FB_YUV)
+			lcdc_write_chan(ch, LDSA2R,
+				ch->info->fix.smem_start +
+				ch->info->var.xres *
+				ch->info->var.yres_virtual);
 
 		/* set line size */
 		lcdc_write_chan(ch, LDMLSR, ch->info->fix.line_length);
@@ -804,9 +832,15 @@ static int sh_mobile_fb_pan_display(struct fb_var_screeninfo *var,
 	struct sh_mobile_lcdc_priv *priv = ch->lcdc;
 	unsigned long ldrcntr;
 	unsigned long new_pan_offset;
+	unsigned long base_addr_y, base_addr_c;
+	unsigned long c_offset;
 
-	new_pan_offset = (var->yoffset * info->fix.line_length) +
-		(var->xoffset * (info->var.bits_per_pixel / 8));
+	if (!(var->nonstd & SH_FB_YUV))
+		new_pan_offset = (var->yoffset * info->fix.line_length) +
+			(var->xoffset * (info->var.bits_per_pixel / 8));
+	else
+		new_pan_offset = (var->yoffset * info->fix.line_length) +
+			(var->xoffset);
 
 	if (new_pan_offset = ch->pan_offset)
 		return 0;	/* No change, do nothing */
@@ -814,7 +848,26 @@ static int sh_mobile_fb_pan_display(struct fb_var_screeninfo *var,
 	ldrcntr = lcdc_read(priv, _LDRCNTR);
 
 	/* Set the source address for the next refresh */
-	lcdc_write_chan_mirror(ch, LDSA1R, ch->dma_handle + new_pan_offset);
+	base_addr_y = ch->dma_handle + new_pan_offset;
+	if (var->nonstd & SH_FB_YUV) {
+		/* Set y offset */
+		c_offset = (var->yoffset *
+			info->fix.line_length *
+			(info->var.bits_per_pixel - 8)) / 8;
+		base_addr_c = ch->dma_handle + var->xres * var->yres_virtual +
+			c_offset;
+		/* Set x offset */
+		if (info->var.bits_per_pixel = 24)
+			base_addr_c += 2 * var->xoffset;
+		else
+			base_addr_c += var->xoffset;
+	} else
+		base_addr_c = 0;
+
+	lcdc_write_chan_mirror(ch, LDSA1R, base_addr_y);
+	if (base_addr_c)
+		lcdc_write_chan_mirror(ch, LDSA2R, base_addr_c);
+
 	if (lcdc_chan_is_sublcd(ch))
 		lcdc_write(ch->lcdc, _LDRCNTR, ldrcntr ^ LDRCNTR_SRS);
 	else
@@ -885,7 +938,10 @@ static void sh_mobile_fb_reconfig(struct fb_info *info)
 		/* Couldn't reconfigure, hopefully, can continue as before */
 		return;
 
-	info->fix.line_length = mode1.xres * (ch->cfg.bpp / 8);
+	if (info->var.nonstd & SH_FB_YUV)
+		info->fix.line_length = mode1.xres;
+	else
+		info->fix.line_length = mode1.xres * (ch->cfg.bpp / 8);
 
 	/*
 	 * fb_set_var() calls the notifier change internally, only if
@@ -980,8 +1036,22 @@ static struct fb_ops sh_mobile_lcdc_ops = {
 	.fb_check_var	= sh_mobile_check_var,
 };
 
-static int sh_mobile_lcdc_set_bpp(struct fb_var_screeninfo *var, int bpp)
+static int sh_mobile_lcdc_set_bpp(struct fb_var_screeninfo *var, int bpp,
+				   int nonstd)
 {
+	if (nonstd & SH_FB_YUV) {
+		switch (bpp) {
+		case 12:
+		case 16:
+		case 24:
+			var->bits_per_pixel = bpp;
+			var->nonstd = nonstd;
+			return 0;
+		default:
+			return -EINVAL;
+		}
+	}
+
 	switch (bpp) {
 	case 16: /* PKF[4:0] = 00011 - RGB 565 */
 		var->red.offset = 11;
@@ -1260,6 +1330,14 @@ static int __devinit sh_mobile_lcdc_probe(struct platform_device *pdev)
 		     k < cfg->num_cfg && lcd_cfg;
 		     k++, lcd_cfg++) {
 			unsigned long size = lcd_cfg->yres * lcd_cfg->xres;
+			/* NV12 buffers must have even number of lines */
+			if ((cfg->nonstd & SH_FB_YUV) && cfg->bpp = 12 &&
+					(lcd_cfg->yres & 0x1)) {
+				dev_err(&pdev->dev, "yres must be multiple of 2"
+						" for YCbCr420 mode.\n");
+				error = -EINVAL;
+				goto err1;
+			}
 
 			if (size > max_size) {
 				max_cfg = lcd_cfg;
@@ -1274,7 +1352,11 @@ static int __devinit sh_mobile_lcdc_probe(struct platform_device *pdev)
 				max_cfg->xres, max_cfg->yres);
 
 		info->fix = sh_mobile_lcdc_fix;
-		info->fix.smem_len = max_size * (cfg->bpp / 8) * 2;
+		info->fix.smem_len = max_size * 2 * cfg->bpp / 8;
+
+		 /* Only pan in 2 line steps for NV12 */
+		if ((cfg->nonstd & SH_FB_YUV) && cfg->bpp = 12)
+			info->fix.ypanstep = 2;
 
 		if (!mode) {
 			mode = &default_720p;
@@ -1292,7 +1374,7 @@ static int __devinit sh_mobile_lcdc_probe(struct platform_device *pdev)
 		var->yres_virtual = var->yres * 2;
 		var->activate = FB_ACTIVATE_NOW;
 
-		error = sh_mobile_lcdc_set_bpp(var, cfg->bpp);
+		error = sh_mobile_lcdc_set_bpp(var, cfg->bpp, cfg->nonstd);
 		if (error)
 			break;
 
@@ -1316,7 +1398,11 @@ static int __devinit sh_mobile_lcdc_probe(struct platform_device *pdev)
 		}
 
 		info->fix.smem_start = ch->dma_handle;
-		info->fix.line_length = var->xres * (cfg->bpp / 8);
+		if (var->nonstd & SH_FB_YUV)
+			info->fix.line_length = var->xres;
+		else
+			info->fix.line_length = var->xres * (cfg->bpp / 8);
+
 		info->screen_base = buf;
 		info->device = &pdev->dev;
 		ch->display_var = *var;
diff --git a/drivers/video/sh_mobile_lcdcfb.h b/drivers/video/sh_mobile_lcdcfb.h
index 9ecee2f..c953cb0 100644
--- a/drivers/video/sh_mobile_lcdcfb.h
+++ b/drivers/video/sh_mobile_lcdcfb.h
@@ -8,7 +8,7 @@
 
 /* per-channel registers */
 enum { LDDCKPAT1R, LDDCKPAT2R, LDMT1R, LDMT2R, LDMT3R, LDDFR, LDSM1R,
-       LDSM2R, LDSA1R, LDMLSR, LDHCNR, LDHSYNR, LDVLNR, LDVSYNR, LDPMR,
+       LDSM2R, LDSA1R, LDSA2R, LDMLSR, LDHCNR, LDHSYNR, LDVLNR, LDVSYNR, LDPMR,
        LDHAJR,
        NR_CH_REGS };
 
diff --git a/include/linux/sh_mobile_fb.h b/include/linux/sh_mobile_fb.h
new file mode 100644
index 0000000..ec448bc
--- /dev/null
+++ b/include/linux/sh_mobile_fb.h
@@ -0,0 +1,14 @@
+/*
+ * SH-Mobile High-Definition Multimedia Interface (HDMI)
+ *
+ * Copyright (C) 2011, Damian Hobson-Garciax <dhobsong@igel.co.jp>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef SH_MOBILE_FB_H
+#define SH_MOBILE_FB_H
+
+#define SH_FB_YUV	0x1
+#endif /* SH_MOBILE_FB_H */
diff --git a/include/video/sh_mobile_lcdc.h b/include/video/sh_mobile_lcdc.h
index daabae5..650ff17 100644
--- a/include/video/sh_mobile_lcdc.h
+++ b/include/video/sh_mobile_lcdc.h
@@ -77,6 +77,7 @@ struct sh_mobile_lcdc_chan_cfg {
 	struct sh_mobile_lcdc_lcd_size_cfg lcd_size_cfg;
 	struct sh_mobile_lcdc_board_cfg board_cfg;
 	struct sh_mobile_lcdc_sys_bus_cfg sys_bus_cfg; /* only for SYSn I/F */
+	int nonstd;
 };
 
 struct sh_mobile_lcdc_info {
-- 
1.7.1

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Guennadi Liakhovetski]

On Wed, 23 Feb 2011, Damian Hobson-Garcia wrote:

> Supports YCbCr420sp, YCbCr422sp, and YCbCr44sp, formats (bpp = 12, 16, and 24)
> respectively.
> Set .nonstd = SH_FB_YUV to enable YUV mode, and use bpp to distiguish between
> the 3 modes.
> Due to the encoding of YUV data, the framebuffer will clear to green instead of black.
> 
> Signed-off-by: Damian Hobson-Garcia <dhobsong@igel.co.jp>
> ---
>  drivers/video/sh_mobile_lcdcfb.c |  142 ++++++++++++++++++++++++++++++--------
>  drivers/video/sh_mobile_lcdcfb.h |    2 +-
>  include/linux/sh_mobile_fb.h     |   14 ++++
>  include/video/sh_mobile_lcdc.h   |    1 +
>  4 files changed, 130 insertions(+), 29 deletions(-)
>  create mode 100644 include/linux/sh_mobile_fb.h
> 

[snip]

> diff --git a/include/linux/sh_mobile_fb.h b/include/linux/sh_mobile_fb.h
> new file mode 100644
> index 0000000..ec448bc
> --- /dev/null
> +++ b/include/linux/sh_mobile_fb.h
> @@ -0,0 +1,14 @@
> +/*
> + * SH-Mobile High-Definition Multimedia Interface (HDMI)
> + *
> + * Copyright (C) 2011, Damian Hobson-Garciax <dhobsong@igel.co.jp>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef SH_MOBILE_FB_H
> +#define SH_MOBILE_FB_H
> +
> +#define SH_FB_YUV	0x1
> +#endif /* SH_MOBILE_FB_H */
> diff --git a/include/video/sh_mobile_lcdc.h b/include/video/sh_mobile_lcdc.h
> index daabae5..650ff17 100644
> --- a/include/video/sh_mobile_lcdc.h
> +++ b/include/video/sh_mobile_lcdc.h
> @@ -77,6 +77,7 @@ struct sh_mobile_lcdc_chan_cfg {
>  	struct sh_mobile_lcdc_lcd_size_cfg lcd_size_cfg;
>  	struct sh_mobile_lcdc_board_cfg board_cfg;
>  	struct sh_mobile_lcdc_sys_bus_cfg sys_bus_cfg; /* only for SYSn I/F */
> +	int nonstd;
>  };
>  
>  struct sh_mobile_lcdc_info {
> -- 
> 1.7.1

Can't the SH_FB_YUV macro definition go into 
include/video/sh_mobile_lcdc.h too?

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

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Damian]

>> diff --git a/include/linux/sh_mobile_fb.h b/include/linux/sh_mobile_fb.h
>> new file mode 100644
>> index 0000000..ec448bc
>> --- /dev/null
>> +++ b/include/linux/sh_mobile_fb.h
>> @@ -0,0 +1,14 @@
>> +/*
>> + * SH-Mobile High-Definition Multimedia Interface (HDMI)
>> + *
>> + * Copyright (C) 2011, Damian Hobson-Garciax<dhobsong@igel.co.jp>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#ifndef SH_MOBILE_FB_H
>> +#define SH_MOBILE_FB_H
>> +
>> +#define SH_FB_YUV	0x1
>> +#endif /* SH_MOBILE_FB_H */
>> diff --git a/include/video/sh_mobile_lcdc.h b/include/video/sh_mobile_lcdc.h
>> index daabae5..650ff17 100644
>> --- a/include/video/sh_mobile_lcdc.h
>> +++ b/include/video/sh_mobile_lcdc.h
>> @@ -77,6 +77,7 @@ struct sh_mobile_lcdc_chan_cfg {
>>   	struct sh_mobile_lcdc_lcd_size_cfg lcd_size_cfg;
>>   	struct sh_mobile_lcdc_board_cfg board_cfg;
>>   	struct sh_mobile_lcdc_sys_bus_cfg sys_bus_cfg; /* only for SYSn I/F */
>> +	int nonstd;
>>   };
>>
>>   struct sh_mobile_lcdc_info {
>> --
>> 1.7.1
>
> Can't the SH_FB_YUV macro definition go into
> include/video/sh_mobile_lcdc.h too?

My thinking behind separating this out was that I wanted this
define to be accessible from user space.  The reason is so that
an application can test the value of .nonstd against the flag to
know for sure if it is dealing with a YUV framebuffer or not.
I was under the impression that only headers under include/linux/ should 
be accessed from user space, but to be honest, I'm not sure about that.
If that is in fact not the case, then I totally agree that it can go
into include/video/sh_mobile_lcdc.h.
Do you know where the proper place for such a header would be?

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

Thanks,
Damian
-- 
Damian Hobson-Garcia
IGEL Co.,Ltd
http://www.igel.co.jp

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[James Simmons]

> > Can't the SH_FB_YUV macro definition go into
> > include/video/sh_mobile_lcdc.h too?
> 
> My thinking behind separating this out was that I wanted this
> define to be accessible from user space.  The reason is so that
> an application can test the value of .nonstd against the flag to
> know for sure if it is dealing with a YUV framebuffer or not.
> I was under the impression that only headers under include/linux/ should be
> accessed from user space, but to be honest, I'm not sure about that.
> If that is in fact not the case, then I totally agree that it can go
> into include/video/sh_mobile_lcdc.h.
> Do you know where the proper place for such a header would be?

Originally the idea was headers in include/video would be card 
specific material to be used by userspace. Also those headers could be 
used also by the kernel driver as well.

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Magnus Damm]

Hi Damian,

On Wed, Feb 23, 2011 at 8:22 PM, Damian <dhobsong@igel.co.jp> wrote:
>
>>> diff --git a/include/linux/sh_mobile_fb.h b/include/linux/sh_mobile_fb.h
>>> new file mode 100644
>>> index 0000000..ec448bc
>>> --- /dev/null
>>> +++ b/include/linux/sh_mobile_fb.h
>>> @@ -0,0 +1,14 @@
>>> +/*
>>> + * SH-Mobile High-Definition Multimedia Interface (HDMI)
>>> + *
>>> + * Copyright (C) 2011, Damian Hobson-Garciax<dhobsong@igel.co.jp>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +#ifndef SH_MOBILE_FB_H
>>> +#define SH_MOBILE_FB_H
>>> +
>>> +#define SH_FB_YUV      0x1
>>> +#endif /* SH_MOBILE_FB_H */
>>> diff --git a/include/video/sh_mobile_lcdc.h
>>> b/include/video/sh_mobile_lcdc.h
>>> index daabae5..650ff17 100644
>>> --- a/include/video/sh_mobile_lcdc.h
>>> +++ b/include/video/sh_mobile_lcdc.h
>>> @@ -77,6 +77,7 @@ struct sh_mobile_lcdc_chan_cfg {
>>>        struct sh_mobile_lcdc_lcd_size_cfg lcd_size_cfg;
>>>        struct sh_mobile_lcdc_board_cfg board_cfg;
>>>        struct sh_mobile_lcdc_sys_bus_cfg sys_bus_cfg; /* only for SYSn
>>> I/F */
>>> +       int nonstd;
>>>  };
>>>
>>>  struct sh_mobile_lcdc_info {
>>> --
>>> 1.7.1
>>
>> Can't the SH_FB_YUV macro definition go into
>> include/video/sh_mobile_lcdc.h too?
>
> My thinking behind separating this out was that I wanted this
> define to be accessible from user space.  The reason is so that
> an application can test the value of .nonstd against the flag to
> know for sure if it is dealing with a YUV framebuffer or not.

Hm, but ideally we want something standard. How do you know the nonstd
flag is working as you expect from user space? All of a sudden you
have code that depends on what type of fbdev driver you are using.
This is ugly, but abstracting the nonstd interface does not make it
better IMO.

The nonstd thing is a hack, but for now it is close enough. V4L2 has
standard NV12/NV16 support already, but I don't think there is any
such thing for fbdev. So i see your patches as a stop-gap, but I
really don't want to make it more complicated than it has to be. So
exporting nonstd values in a header file to user space seems too
complicated IMO.

Please just live with the fact that nonstd is special for now. We need
to rework the entire LCDC/HDMI/DSI area to support multiple planes and
better PM anyway. Perhaps KMS is the way forward, or perhaps Media
Controller? Maybe both?

> I was under the impression that only headers under include/linux/ should be
> accessed from user space, but to be honest, I'm not sure about that.
> If that is in fact not the case, then I totally agree that it can go
> into include/video/sh_mobile_lcdc.h.

Please ditch the SH_FB_YUV constant all together. No need to build
some abstraction on top of a hackish interface. Just check if nonstd
is non-zero in the driver and assume that means YUV for now. That's
good enough.

Thanks,

/ magnus

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Damian]

Hi Magnus,
On 2011/02/24 8:28, Magnus Damm wrote:
> Hi Damian,
>
> On Wed, Feb 23, 2011 at 8:22 PM, Damian<dhobsong@igel.co.jp>  wrote:
>>
>>>> diff --git a/include/linux/sh_mobile_fb.h b/include/linux/sh_mobile_fb.h
>>>> new file mode 100644
>>>> index 0000000..ec448bc
>>>> --- /dev/null
>>>> +++ b/include/linux/sh_mobile_fb.h
>>>> @@ -0,0 +1,14 @@
>>>> +/*
>>>> + * SH-Mobile High-Definition Multimedia Interface (HDMI)
>>>> + *
>>>> + * Copyright (C) 2011, Damian Hobson-Garciax<dhobsong@igel.co.jp>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + */
>>>> +#ifndef SH_MOBILE_FB_H
>>>> +#define SH_MOBILE_FB_H
>>>> +
>>>> +#define SH_FB_YUV      0x1
>>>> +#endif /* SH_MOBILE_FB_H */
>>>> diff --git a/include/video/sh_mobile_lcdc.h
>>>> b/include/video/sh_mobile_lcdc.h
>>>> index daabae5..650ff17 100644
>>>> --- a/include/video/sh_mobile_lcdc.h
>>>> +++ b/include/video/sh_mobile_lcdc.h
>>>> @@ -77,6 +77,7 @@ struct sh_mobile_lcdc_chan_cfg {
>>>>         struct sh_mobile_lcdc_lcd_size_cfg lcd_size_cfg;
>>>>         struct sh_mobile_lcdc_board_cfg board_cfg;
>>>>         struct sh_mobile_lcdc_sys_bus_cfg sys_bus_cfg; /* only for SYSn
>>>> I/F */
>>>> +       int nonstd;
>>>>   };
>>>>
>>>>   struct sh_mobile_lcdc_info {
>>>> --
>>>> 1.7.1
>>>
>>> Can't the SH_FB_YUV macro definition go into
>>> include/video/sh_mobile_lcdc.h too?
>>
>> My thinking behind separating this out was that I wanted this
>> define to be accessible from user space.  The reason is so that
>> an application can test the value of .nonstd against the flag to
>> know for sure if it is dealing with a YUV framebuffer or not.
>
> Hm, but ideally we want something standard. How do you know the nonstd
> flag is working as you expect from user space? All of a sudden you
> have code that depends on what type of fbdev driver you are using.
> This is ugly, but abstracting the nonstd interface does not make it
> better IMO.
>
> The nonstd thing is a hack, but for now it is close enough. V4L2 has
> standard NV12/NV16 support already, but I don't think there is any
> such thing for fbdev. So i see your patches as a stop-gap, but I
> really don't want to make it more complicated than it has to be. So
> exporting nonstd values in a header file to user space seems too
> complicated IMO.
>
> Please just live with the fact that nonstd is special for now. We need
> to rework the entire LCDC/HDMI/DSI area to support multiple planes and
> better PM anyway. Perhaps KMS is the way forward, or perhaps Media
> Controller? Maybe both?
>
>> I was under the impression that only headers under include/linux/ should be
>> accessed from user space, but to be honest, I'm not sure about that.
>> If that is in fact not the case, then I totally agree that it can go
>> into include/video/sh_mobile_lcdc.h.
>
> Please ditch the SH_FB_YUV constant all together. No need to build
> some abstraction on top of a hackish interface. Just check if nonstd
> is non-zero in the driver and assume that means YUV for now. That's
> good enough.
>

Ok, I see what you're saying. But if the SH_FB_YUV flag is disappearing, 
I guess it makes sense to ditch the second patch in this series as well, 
since that's just further abstraction (albeit locally).

> Thanks,
>
> / magnus

Thank you,
-- 
Damian Hobson-Garcia
IGEL Co.,Ltd
http://www.igel.co.jp

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Geert Uytterhoeven]

On Thu, Feb 24, 2011 at 00:28, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Wed, Feb 23, 2011 at 8:22 PM, Damian <dhobsong@igel.co.jp> wrote:
>>>> diff --git a/include/linux/sh_mobile_fb.h b/include/linux/sh_mobile_fb.h
>>>> new file mode 100644
>>>> index 0000000..ec448bc
>>>> --- /dev/null
>>>> +++ b/include/linux/sh_mobile_fb.h
>>>> @@ -0,0 +1,14 @@
>>>> +/*
>>>> + * SH-Mobile High-Definition Multimedia Interface (HDMI)
>>>> + *
>>>> + * Copyright (C) 2011, Damian Hobson-Garciax<dhobsong@igel.co.jp>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + */
>>>> +#ifndef SH_MOBILE_FB_H
>>>> +#define SH_MOBILE_FB_H
>>>> +
>>>> +#define SH_FB_YUV      0x1
>>>> +#endif /* SH_MOBILE_FB_H */
>>>> diff --git a/include/video/sh_mobile_lcdc.h
>>>> b/include/video/sh_mobile_lcdc.h
>>>> index daabae5..650ff17 100644
>>>> --- a/include/video/sh_mobile_lcdc.h
>>>> +++ b/include/video/sh_mobile_lcdc.h
>>>> @@ -77,6 +77,7 @@ struct sh_mobile_lcdc_chan_cfg {
>>>>        struct sh_mobile_lcdc_lcd_size_cfg lcd_size_cfg;
>>>>        struct sh_mobile_lcdc_board_cfg board_cfg;
>>>>        struct sh_mobile_lcdc_sys_bus_cfg sys_bus_cfg; /* only for SYSn
>>>> I/F */
>>>> +       int nonstd;
>>>>  };
>>>>
>>>>  struct sh_mobile_lcdc_info {
>>>> --
>>>> 1.7.1
>>>
>>> Can't the SH_FB_YUV macro definition go into
>>> include/video/sh_mobile_lcdc.h too?
>>
>> My thinking behind separating this out was that I wanted this
>> define to be accessible from user space.  The reason is so that
>> an application can test the value of .nonstd against the flag to
>> know for sure if it is dealing with a YUV framebuffer or not.
>
> Hm, but ideally we want something standard. How do you know the nonstd
> flag is working as you expect from user space? All of a sudden you
> have code that depends on what type of fbdev driver you are using.
> This is ugly, but abstracting the nonstd interface does not make it
> better IMO.
>
> The nonstd thing is a hack, but for now it is close enough. V4L2 has
> standard NV12/NV16 support already, but I don't think there is any
> such thing for fbdev. So i see your patches as a stop-gap, but I
> really don't want to make it more complicated than it has to be. So
> exporting nonstd values in a header file to user space seems too
> complicated IMO.
>
> Please just live with the fact that nonstd is special for now. We need
> to rework the entire LCDC/HDMI/DSI area to support multiple planes and
> better PM anyway. Perhaps KMS is the way forward, or perhaps Media
> Controller? Maybe both?
>
>> I was under the impression that only headers under include/linux/ should be
>> accessed from user space, but to be honest, I'm not sure about that.
>> If that is in fact not the case, then I totally agree that it can go
>> into include/video/sh_mobile_lcdc.h.
>
> Please ditch the SH_FB_YUV constant all together. No need to build
> some abstraction on top of a hackish interface. Just check if nonstd
> is non-zero in the driver and assume that means YUV for now. That's
> good enough.

For YUV (do you mean YCbCr?), I'm inclined to suggest adding a new FB_VISUAL_*
type instead, which indicates the fb_var_screeninfo.{red,green,blue} fields are
YCbCr instead of RGB.
Depending on the frame buffer organization, you also need new FB_TYPE_*/FB_AUX_*
types.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Damian]

Hi Geert,
On 2011/02/24 15:05, Geert Uytterhoeven wrote:
>>>
>>> My thinking behind separating this out was that I wanted this
>>> define to be accessible from user space.  The reason is so that
>>> an application can test the value of .nonstd against the flag to
>>> know for sure if it is dealing with a YUV framebuffer or not.
>>
>> Hm, but ideally we want something standard. How do you know the nonstd
>> flag is working as you expect from user space? All of a sudden you
>> have code that depends on what type of fbdev driver you are using.
>> This is ugly, but abstracting the nonstd interface does not make it
>> better IMO.
>>
>> The nonstd thing is a hack, but for now it is close enough. V4L2 has
>> standard NV12/NV16 support already, but I don't think there is any
>> such thing for fbdev. So i see your patches as a stop-gap, but I
>> really don't want to make it more complicated than it has to be. So
>> exporting nonstd values in a header file to user space seems too
>> complicated IMO.
>>
>> Please just live with the fact that nonstd is special for now. We need
>> to rework the entire LCDC/HDMI/DSI area to support multiple planes and
>> better PM anyway. Perhaps KMS is the way forward, or perhaps Media
>> Controller? Maybe both?
>>
>>> I was under the impression that only headers under include/linux/ should be
>>> accessed from user space, but to be honest, I'm not sure about that.
>>> If that is in fact not the case, then I totally agree that it can go
>>> into include/video/sh_mobile_lcdc.h.
>>
>> Please ditch the SH_FB_YUV constant all together. No need to build
>> some abstraction on top of a hackish interface. Just check if nonstd
>> is non-zero in the driver and assume that means YUV for now. That's
>> good enough.
>
> For YUV (do you mean YCbCr?), I'm inclined to suggest adding a new FB_VISUAL_*
> type instead, which indicates the fb_var_screeninfo.{red,green,blue} fields are
> YCbCr instead of RGB.
> Depending on the frame buffer organization, you also need new FB_TYPE_*/FB_AUX_*
> types.
I just wanted to clarify here.  Is your comment about these new flags in 
specific reference to this patch or to Magnus' "going forward" comment? 
  It seems like the beginnings of a method to standardize YCbCr support 
in fbdev across all platforms.
Also, do I understand correctly that FB_VISUAL_ would specify the 
colorspace (RGB, YCbCr), FB_TYPE_* would be a format specifier (i.e. 
planar, semiplanar, interleaved, etc)?  I'm not really sure what you are 
referring to with the FB_AUX_* however.
>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks very much,
Damian

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Geert Uytterhoeven]

On Tue, Mar 1, 2011 at 04:13, Damian <dhobsong@igel.co.jp> wrote:
> On 2011/02/24 15:05, Geert Uytterhoeven wrote:
>>>> My thinking behind separating this out was that I wanted this
>>>> define to be accessible from user space.  The reason is so that
>>>> an application can test the value of .nonstd against the flag to
>>>> know for sure if it is dealing with a YUV framebuffer or not.
>>>
>>> Hm, but ideally we want something standard. How do you know the nonstd
>>> flag is working as you expect from user space? All of a sudden you
>>> have code that depends on what type of fbdev driver you are using.
>>> This is ugly, but abstracting the nonstd interface does not make it
>>> better IMO.
>>>
>>> The nonstd thing is a hack, but for now it is close enough. V4L2 has
>>> standard NV12/NV16 support already, but I don't think there is any
>>> such thing for fbdev. So i see your patches as a stop-gap, but I
>>> really don't want to make it more complicated than it has to be. So
>>> exporting nonstd values in a header file to user space seems too
>>> complicated IMO.
>>>
>>> Please just live with the fact that nonstd is special for now. We need
>>> to rework the entire LCDC/HDMI/DSI area to support multiple planes and
>>> better PM anyway. Perhaps KMS is the way forward, or perhaps Media
>>> Controller? Maybe both?
>>>
>>>> I was under the impression that only headers under include/linux/ should
>>>> be
>>>> accessed from user space, but to be honest, I'm not sure about that.
>>>> If that is in fact not the case, then I totally agree that it can go
>>>> into include/video/sh_mobile_lcdc.h.
>>>
>>> Please ditch the SH_FB_YUV constant all together. No need to build
>>> some abstraction on top of a hackish interface. Just check if nonstd
>>> is non-zero in the driver and assume that means YUV for now. That's
>>> good enough.
>>
>> For YUV (do you mean YCbCr?), I'm inclined to suggest adding a new
>> FB_VISUAL_*
>> type instead, which indicates the fb_var_screeninfo.{red,green,blue}
>> fields are
>> YCbCr instead of RGB.
>> Depending on the frame buffer organization, you also need new
>> FB_TYPE_*/FB_AUX_*
>> types.
>
> I just wanted to clarify here.  Is your comment about these new flags in
> specific reference to this patch or to Magnus' "going forward" comment?  It

About new flags.

> seems like the beginnings of a method to standardize YCbCr support in fbdev
> across all platforms.
> Also, do I understand correctly that FB_VISUAL_ would specify the colorspace

FB_VISUAL_* specifies how pixel values (which may be tuples) are mapped to
colors on the screen, so to me it looks like the sensible way to set up YCbCr.

> (RGB, YCbCr), FB_TYPE_* would be a format specifier (i.e. planar,
> semiplanar, interleaved, etc)?  I'm not really sure what you are referring
> to with the FB_AUX_* however.

Yep, FB_TYPE_* specifies how pixel values/tuples are laid out in frame buffer
memory.

FB_AUX_* is only used if a specific value of FB_TYPE_* needs an additional
parameter (e.g. the interleave value for interleaved bitplanes).
Another possible use is the offset between the even and odd fields of
the screen,
for hardware that supports interlacing by storing the even and odd
fields separately.
I think I even posted patches for that several years ago... Indeed
http://lkml.indiana.edu/hypermail/linux/kernel/0304.0/0816.html

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Magnus Damm]

Hi Geert,

On Thu, Feb 24, 2011 at 3:05 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Thu, Feb 24, 2011 at 00:28, Magnus Damm <magnus.damm@gmail.com> wrote:
>> Please ditch the SH_FB_YUV constant all together. No need to build
>> some abstraction on top of a hackish interface. Just check if nonstd
>> is non-zero in the driver and assume that means YUV for now. That's
>> good enough.
>
> For YUV (do you mean YCbCr?), I'm inclined to suggest adding a new FB_VISUAL_*
> type instead, which indicates the fb_var_screeninfo.{red,green,blue} fields are
> YCbCr instead of RGB.
> Depending on the frame buffer organization, you also need new FB_TYPE_*/FB_AUX_*
> types.

I'm all for extending the common code instead of hiding code in
drivers. But I wonder how much overlap there is with V4L2 for
instance. I remember adding support for some NVxx formats for V4L2
some years ago. It's mainly used for Video input on Renesas SoCs
though:

http://kerneltrap.org/mailarchive/git-commits-head/2008/12/31/4560474

So I was hoping that something like the above could be added to fbdev
too, but it looks like more code is needed.

Do you have any idea on how to tie in the valid range of each color
channel? The LCDC hardware block can select between 0->255 range or
16->235/240 for the YUV channels. In V4L2 this is handled by
v4l2_colorspace, the 0->255 maps directly to V4L2_COLORSPACE_JPEG.

And how does all this relate to KMS? I'd prefer to keep this code in
one place if possible.

Thanks,

/ magnus

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Geert Uytterhoeven]

On Tue, Mar 1, 2011 at 09:25, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Thu, Feb 24, 2011 at 3:05 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Thu, Feb 24, 2011 at 00:28, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> Please ditch the SH_FB_YUV constant all together. No need to build
>>> some abstraction on top of a hackish interface. Just check if nonstd
>>> is non-zero in the driver and assume that means YUV for now. That's
>>> good enough.
>>
>> For YUV (do you mean YCbCr?), I'm inclined to suggest adding a new FB_VISUAL_*
>> type instead, which indicates the fb_var_screeninfo.{red,green,blue} fields are
>> YCbCr instead of RGB.
>> Depending on the frame buffer organization, you also need new FB_TYPE_*/FB_AUX_*
>> types.
>
> I'm all for extending the common code instead of hiding code in
> drivers. But I wonder how much overlap there is with V4L2 for
> instance. I remember adding support for some NVxx formats for V4L2
> some years ago. It's mainly used for Video input on Renesas SoCs
> though:
>
> http://kerneltrap.org/mailarchive/git-commits-head/2008/12/31/4560474
>
> So I was hoping that something like the above could be added to fbdev
> too, but it looks like more code is needed.
>
> Do you have any idea on how to tie in the valid range of each color
> channel? The LCDC hardware block can select between 0->255 range or
> 16->235/240 for the YUV channels. In V4L2 this is handled by
> v4l2_colorspace, the 0->255 maps directly to V4L2_COLORSPACE_JPEG.

Unfortunately not. Unlike for the YCbCr visual, I don't see a field we
can easily
(ab)use for that. Except if it's limited to standard 16-235/240 Y vs.
full 0-255 Y,
for which we could just have 2 different visual types.

> And how does all this relate to KMS? I'd prefer to keep this code in
> one place if possible.

Good question!

I'm still puzzled about this KMS thing. If the name "Kernel Mode Setting"
covers it, then how does it compare to plain fbdev? Just additional frame buffer
memory management?

Furthermore, everybody states that "future X/desktop" will require KMS drivers.
How do/will we handle this on dumb frame buffers? It's not like we can't do
"advanced" things like compositing using the CPU. Transparency may stretch
it a bit on lower end CPUs, but you don't always need that.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Alan Cox]

On Tue, 1 Mar 2011 13:31:19 -0800
Corbin Simpson <mostawesomedude@gmail.com> wrote:

> I am slightly curious about this as well; I have a device with only YUV
> scanout and was considering KMS, but don't know what the best approach is.

The problem with hiding behind a fake RGB frame buffer is you've then
completely stuffed any framebuffer based apps that could use YUV happily
(eg framebuffer video playback).

Far better to make the kernel tell the truth in these cases. You'll need
to add some tweaks to the fb code for it and a YUV boot penguin.[1]

If you want to run X on it then you can use shadowfb on the X side to do
your YUV/RGB adaptation.

Alan
--
[1] Am I the only one who thinks the fb code also needs an imagine of an
embarassed penguin that has wet itself to go with oops output ?

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Magnus Damm]

On Thu, Feb 24, 2011 at 12:38 PM, Damian <dhobsong@igel.co.jp> wrote:
> Ok, I see what you're saying. But if the SH_FB_YUV flag is disappearing, I
> guess it makes sense to ditch the second patch in this series as well, since
> that's just further abstraction (albeit locally).

Yep, I think so. Thanks for your help!

/ magnus

Re: [PATCH 1/2] fbdev: sh_mobile_lcdc: Add YUV input support

[Damian]

On 2011/03/01 17:59, Magnus Damm wrote:
> On Thu, Feb 24, 2011 at 12:38 PM, Damian<dhobsong@igel.co.jp>  wrote:
>> Ok, I see what you're saying. But if the SH_FB_YUV flag is disappearing, I
>> guess it makes sense to ditch the second patch in this series as well, since
>> that's just further abstraction (albeit locally).
>
> Yep, I think so. Thanks for your help!
Ok, then I will resubmit only the first patch of the series as
[PATCH v2] fbdev: sh_mobile_lcdc: Add YUV input support

Thanks,
Damian
>
> / magnus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Damian Hobson-Garcia
IGEL Co.,Ltd
http://www.igel.co.jp