[PATCH V2] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[PATCH V2] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[Martin Hostettler]

Adds support for V4L2_MBUS_FMT_Y8_1X8 format and 8bit data width in
synchronous interface.

When in 8bit mode don't apply DC substraction of 64 per default as this would
remove 1/4 of the sensor range.

When using V4L2_MBUS_FMT_Y8_1X8 (or possibly another 8bit per pixel) mode
set the CDCC to output 8bit per pixel instead of 16bit.

Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
---
 drivers/media/video/isp/ispccdc.c  |   22 ++++++++++++++++++----
 drivers/media/video/isp/ispvideo.c |    2 ++
 2 files changed, 20 insertions(+), 4 deletions(-)

Changes since first version:
	- forward ported to current media.git

diff --git a/drivers/media/video/isp/ispccdc.c b/drivers/media/video/isp/ispccdc.c
index 578c8bf..c7397c9 100644
--- a/drivers/media/video/isp/ispccdc.c
+++ b/drivers/media/video/isp/ispccdc.c
@@ -43,6 +43,7 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 		  unsigned int pad, enum v4l2_subdev_format_whence which);
 
 static const unsigned int ccdc_fmts[] = {
+	V4L2_MBUS_FMT_Y8_1X8,
 	V4L2_MBUS_FMT_SGRBG10_1X10,
 	V4L2_MBUS_FMT_SRGGB10_1X10,
 	V4L2_MBUS_FMT_SBGGR10_1X10,
@@ -1127,6 +1128,9 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	ccdc->syncif.datsz = pdata ? pdata->width : 10;
 	ispccdc_config_sync_if(ccdc, &ccdc->syncif);
 
+	/* CCDC_PAD_SINK */
+	format = &ccdc->formats[CCDC_PAD_SINK];
+
 	syn_mode = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
 
 	/* Use the raw, unprocessed data when writing to memory. The H3A and
@@ -1144,10 +1148,15 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	else
 		syn_mode &= ~ISPCCDC_SYN_MODE_SDR2RSZ;
 
-	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
+	/* Use PACK8 mode for 1byte per pixel formats */
 
-	/* CCDC_PAD_SINK */
-	format = &ccdc->formats[CCDC_PAD_SINK];
+	if (isp_video_format_info(format->code)->bpp <= 8)
+		syn_mode |= ISPCCDC_SYN_MODE_PACK8;
+	else
+		syn_mode &= ~ISPCCDC_SYN_MODE_PACK8;
+
+
+	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
 
 	/* Mosaic filter */
 	switch (format->code) {
@@ -2244,7 +2253,12 @@ int isp_ccdc_init(struct isp_device *isp)
 	ccdc->syncif.vdpol = 0;
 
 	ccdc->clamp.oblen = 0;
-	ccdc->clamp.dcsubval = 64;
+
+	if (isp->pdata->subdevs->interface == ISP_INTERFACE_PARALLEL
+	    && isp->pdata->subdevs->bus.parallel.width <= 8)
+		ccdc->clamp.dcsubval = 0;
+	else
+		ccdc->clamp.dcsubval = 64;
 
 	ccdc->vpcfg.pixelclk = 0;
 
diff --git a/drivers/media/video/isp/ispvideo.c b/drivers/media/video/isp/ispvideo.c
index 5f984e4..cd3d331 100644
--- a/drivers/media/video/isp/ispvideo.c
+++ b/drivers/media/video/isp/ispvideo.c
@@ -221,6 +221,8 @@ isp_video_check_format(struct isp_video *video, struct isp_video_fh *vfh)
 }
 
 static struct isp_format_info formats[] = {
+	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
+	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
 	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
 	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
 	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,
-- 
1.7.1

Re: [PATCH V2] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[Laurent Pinchart]

Hi Martin,

Thanks for the patch. One comment below.

On Tuesday 18 January 2011 22:27:42 Martin Hostettler wrote:
> Adds support for V4L2_MBUS_FMT_Y8_1X8 format and 8bit data width in
> synchronous interface.
> 
> When in 8bit mode don't apply DC substraction of 64 per default as this
> would remove 1/4 of the sensor range.
> 
> When using V4L2_MBUS_FMT_Y8_1X8 (or possibly another 8bit per pixel) mode
> set the CDCC to output 8bit per pixel instead of 16bit.
> 
> Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
> ---
>  drivers/media/video/isp/ispccdc.c  |   22 ++++++++++++++++++----
>  drivers/media/video/isp/ispvideo.c |    2 ++
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> Changes since first version:
> 	- forward ported to current media.git
> 
> diff --git a/drivers/media/video/isp/ispccdc.c
> b/drivers/media/video/isp/ispccdc.c index 578c8bf..c7397c9 100644
> --- a/drivers/media/video/isp/ispccdc.c
> +++ b/drivers/media/video/isp/ispccdc.c
> @@ -43,6 +43,7 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct
> v4l2_subdev_fh *fh, unsigned int pad, enum v4l2_subdev_format_whence
> which);
> 
>  static const unsigned int ccdc_fmts[] = {
> +	V4L2_MBUS_FMT_Y8_1X8,
>  	V4L2_MBUS_FMT_SGRBG10_1X10,
>  	V4L2_MBUS_FMT_SRGGB10_1X10,
>  	V4L2_MBUS_FMT_SBGGR10_1X10,
> @@ -1127,6 +1128,9 @@ static void ccdc_configure(struct isp_ccdc_device
> *ccdc) ccdc->syncif.datsz = pdata ? pdata->width : 10;
>  	ispccdc_config_sync_if(ccdc, &ccdc->syncif);
> 
> +	/* CCDC_PAD_SINK */
> +	format = &ccdc->formats[CCDC_PAD_SINK];
> +
>  	syn_mode = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
> 
>  	/* Use the raw, unprocessed data when writing to memory. The H3A and
> @@ -1144,10 +1148,15 @@ static void ccdc_configure(struct isp_ccdc_device
> *ccdc) else
>  		syn_mode &= ~ISPCCDC_SYN_MODE_SDR2RSZ;
> 
> -	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
> +	/* Use PACK8 mode for 1byte per pixel formats */
> 
> -	/* CCDC_PAD_SINK */
> -	format = &ccdc->formats[CCDC_PAD_SINK];
> +	if (isp_video_format_info(format->code)->bpp <= 8)
> +		syn_mode |= ISPCCDC_SYN_MODE_PACK8;
> +	else
> +		syn_mode &= ~ISPCCDC_SYN_MODE_PACK8;
> +
> +
> +	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
> 
>  	/* Mosaic filter */
>  	switch (format->code) {
> @@ -2244,7 +2253,12 @@ int isp_ccdc_init(struct isp_device *isp)
>  	ccdc->syncif.vdpol = 0;
> 
>  	ccdc->clamp.oblen = 0;
> -	ccdc->clamp.dcsubval = 64;
> +
> +	if (isp->pdata->subdevs->interface == ISP_INTERFACE_PARALLEL
> +	    && isp->pdata->subdevs->bus.parallel.width <= 8)
> +		ccdc->clamp.dcsubval = 0;
> +	else
> +		ccdc->clamp.dcsubval = 64;

I don't like this too much. What happens if you have several sensors connected 
to the system with different bus width ?

>  	ccdc->vpcfg.pixelclk = 0;
> 
> diff --git a/drivers/media/video/isp/ispvideo.c
> b/drivers/media/video/isp/ispvideo.c index 5f984e4..cd3d331 100644
> --- a/drivers/media/video/isp/ispvideo.c
> +++ b/drivers/media/video/isp/ispvideo.c
> @@ -221,6 +221,8 @@ isp_video_check_format(struct isp_video *video, struct
> isp_video_fh *vfh) }
> 
>  static struct isp_format_info formats[] = {
> +	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
> +	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
>  	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
>  	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
>  	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,

-- 
Regards,

Laurent Pinchart

Re: [PATCH V2] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[Michael Jones]

Hi Martin,

a couple of comments inline below.

On 01/19/2011 12:27 AM, Laurent Pinchart wrote:
> Hi Martin,
> 
> Thanks for the patch. One comment below.
> 
> On Tuesday 18 January 2011 22:27:42 Martin Hostettler wrote:
>> Adds support for V4L2_MBUS_FMT_Y8_1X8 format and 8bit data width in
>> synchronous interface.
>>
>> When in 8bit mode don't apply DC substraction of 64 per default as this
>> would remove 1/4 of the sensor range.
>>
>> When using V4L2_MBUS_FMT_Y8_1X8 (or possibly another 8bit per pixel) mode
>> set the CDCC to output 8bit per pixel instead of 16bit.
>>
>> Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
>> ---
>>  drivers/media/video/isp/ispccdc.c  |   22 ++++++++++++++++++----
>>  drivers/media/video/isp/ispvideo.c |    2 ++
>>  2 files changed, 20 insertions(+), 4 deletions(-)
>>
>> Changes since first version:
>> 	- forward ported to current media.git
>>
>> diff --git a/drivers/media/video/isp/ispccdc.c
>> b/drivers/media/video/isp/ispccdc.c index 578c8bf..c7397c9 100644
>> --- a/drivers/media/video/isp/ispccdc.c
>> +++ b/drivers/media/video/isp/ispccdc.c
>> @@ -43,6 +43,7 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct
>> v4l2_subdev_fh *fh, unsigned int pad, enum v4l2_subdev_format_whence
>> which);
>>
>>  static const unsigned int ccdc_fmts[] = {
>> +	V4L2_MBUS_FMT_Y8_1X8,
>>  	V4L2_MBUS_FMT_SGRBG10_1X10,
>>  	V4L2_MBUS_FMT_SRGGB10_1X10,
>>  	V4L2_MBUS_FMT_SBGGR10_1X10,
>> @@ -1127,6 +1128,9 @@ static void ccdc_configure(struct isp_ccdc_device
>> *ccdc) ccdc->syncif.datsz = pdata ? pdata->width : 10;
>>  	ispccdc_config_sync_if(ccdc, &ccdc->syncif);
>>
>> +	/* CCDC_PAD_SINK */
>> +	format = &ccdc->formats[CCDC_PAD_SINK];
>> +
>>  	syn_mode = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
>>
>>  	/* Use the raw, unprocessed data when writing to memory. The H3A and
>> @@ -1144,10 +1148,15 @@ static void ccdc_configure(struct isp_ccdc_device
>> *ccdc) else
>>  		syn_mode &= ~ISPCCDC_SYN_MODE_SDR2RSZ;
>>
>> -	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
>> +	/* Use PACK8 mode for 1byte per pixel formats */
>>
>> -	/* CCDC_PAD_SINK */
>> -	format = &ccdc->formats[CCDC_PAD_SINK];
>> +	if (isp_video_format_info(format->code)->bpp <= 8)
>> +		syn_mode |= ISPCCDC_SYN_MODE_PACK8;
>> +	else
>> +		syn_mode &= ~ISPCCDC_SYN_MODE_PACK8;
>> +

It would make sense to me to move this bit into ispccdc_config_sync_if().

>> +
>> +	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
>>
>>  	/* Mosaic filter */
>>  	switch (format->code) {
>> @@ -2244,7 +2253,12 @@ int isp_ccdc_init(struct isp_device *isp)
>>  	ccdc->syncif.vdpol = 0;
>>
>>  	ccdc->clamp.oblen = 0;
>> -	ccdc->clamp.dcsubval = 64;
>> +
>> +	if (isp->pdata->subdevs->interface == ISP_INTERFACE_PARALLEL
>> +	    && isp->pdata->subdevs->bus.parallel.width <= 8)
>> +		ccdc->clamp.dcsubval = 0;
>> +	else
>> +		ccdc->clamp.dcsubval = 64;
> 
> I don't like this too much. What happens if you have several sensors connected 
> to the system with different bus width ?

I see Laurent's point here.  Maybe move the dcsubval assignment into
ccdc_configure().  Also, don't we also want to remove dcsubval for an
8-bit serially-attached sensor?  In ccdc_configure() you could make it
conditional on the mbus format's width on the CCDC sink pad.

> 
>>  	ccdc->vpcfg.pixelclk = 0;
>>
>> diff --git a/drivers/media/video/isp/ispvideo.c
>> b/drivers/media/video/isp/ispvideo.c index 5f984e4..cd3d331 100644
>> --- a/drivers/media/video/isp/ispvideo.c
>> +++ b/drivers/media/video/isp/ispvideo.c
>> @@ -221,6 +221,8 @@ isp_video_check_format(struct isp_video *video, struct
>> isp_video_fh *vfh) }
>>
>>  static struct isp_format_info formats[] = {
>> +	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
>> +	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
>>  	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
>>  	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
>>  	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,
> 


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH V2] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[Laurent Pinchart]

Hi Michael,

On Wednesday 19 January 2011 14:45:46 Michael Jones wrote:
> On 01/19/2011 12:27 AM, Laurent Pinchart wrote:
> > On Tuesday 18 January 2011 22:27:42 Martin Hostettler wrote:
> >> Adds support for V4L2_MBUS_FMT_Y8_1X8 format and 8bit data width in
> >> synchronous interface.
> >> 
> >> When in 8bit mode don't apply DC substraction of 64 per default as this
> >> would remove 1/4 of the sensor range.
> >> 
> >> When using V4L2_MBUS_FMT_Y8_1X8 (or possibly another 8bit per pixel)
> >> mode set the CDCC to output 8bit per pixel instead of 16bit.
> >> 
> >> Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
> >> ---
> >> 
> >>  drivers/media/video/isp/ispccdc.c  |   22 ++++++++++++++++++----
> >>  drivers/media/video/isp/ispvideo.c |    2 ++
> >>  2 files changed, 20 insertions(+), 4 deletions(-)
> >> 
> >> Changes since first version:
> >> 	- forward ported to current media.git
> >> 
> >> diff --git a/drivers/media/video/isp/ispccdc.c
> >> b/drivers/media/video/isp/ispccdc.c index 578c8bf..c7397c9 100644
> >> --- a/drivers/media/video/isp/ispccdc.c
> >> +++ b/drivers/media/video/isp/ispccdc.c
> >> @@ -43,6 +43,7 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct
> >> v4l2_subdev_fh *fh, unsigned int pad, enum v4l2_subdev_format_whence
> >> which);
> >> 
> >>  static const unsigned int ccdc_fmts[] = {
> >> 
> >> +	V4L2_MBUS_FMT_Y8_1X8,
> >> 
> >>  	V4L2_MBUS_FMT_SGRBG10_1X10,
> >>  	V4L2_MBUS_FMT_SRGGB10_1X10,
> >>  	V4L2_MBUS_FMT_SBGGR10_1X10,
> >> 
> >> @@ -1127,6 +1128,9 @@ static void ccdc_configure(struct isp_ccdc_device
> >> *ccdc) ccdc->syncif.datsz = pdata ? pdata->width : 10;
> >> 
> >>  	ispccdc_config_sync_if(ccdc, &ccdc->syncif);
> >> 
> >> +	/* CCDC_PAD_SINK */
> >> +	format = &ccdc->formats[CCDC_PAD_SINK];
> >> +
> >> 
> >>  	syn_mode = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
> >>  	
> >>  	/* Use the raw, unprocessed data when writing to memory. The H3A and
> >> 
> >> @@ -1144,10 +1148,15 @@ static void ccdc_configure(struct
> >> isp_ccdc_device *ccdc) else
> >> 
> >>  		syn_mode &= ~ISPCCDC_SYN_MODE_SDR2RSZ;
> >> 
> >> -	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
> >> +	/* Use PACK8 mode for 1byte per pixel formats */
> >> 
> >> -	/* CCDC_PAD_SINK */
> >> -	format = &ccdc->formats[CCDC_PAD_SINK];
> >> +	if (isp_video_format_info(format->code)->bpp <= 8)
> >> +		syn_mode |= ISPCCDC_SYN_MODE_PACK8;
> >> +	else
> >> +		syn_mode &= ~ISPCCDC_SYN_MODE_PACK8;
> >> +
> 
> It would make sense to me to move this bit into ispccdc_config_sync_if().

Why do you think so ? This configures how the data is written to memory, while 
ispccdc_config_sync_if() configures the CCDC input interface.

> >> +
> >> +	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
> >> 
> >>  	/* Mosaic filter */
> >>  	switch (format->code) {
> >> 
> >> @@ -2244,7 +2253,12 @@ int isp_ccdc_init(struct isp_device *isp)
> >> 
> >>  	ccdc->syncif.vdpol = 0;
> >>  	
> >>  	ccdc->clamp.oblen = 0;
> >> 
> >> -	ccdc->clamp.dcsubval = 64;
> >> +
> >> +	if (isp->pdata->subdevs->interface == ISP_INTERFACE_PARALLEL
> >> +	    && isp->pdata->subdevs->bus.parallel.width <= 8)
> >> +		ccdc->clamp.dcsubval = 0;
> >> +	else
> >> +		ccdc->clamp.dcsubval = 64;
> > 
> > I don't like this too much. What happens if you have several sensors
> > connected to the system with different bus width ?
> 
> I see Laurent's point here.  Maybe move the dcsubval assignment into
> ccdc_configure().  Also, don't we also want to remove dcsubval for an
> 8-bit serially-attached sensor?  In ccdc_configure() you could make it
> conditional on the mbus format's width on the CCDC sink pad.

This piece of code only sets the default value. If the user sets another 
value, the driver must not override it silently when the video stream is 
started. I'm not really sure how to properly fix this. The best solution is of 
course to set the value from userspace.

> >>  	ccdc->vpcfg.pixelclk = 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH V2] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[martin]

On Wed, Jan 19, 2011 at 12:27:19AM +0100, Laurent Pinchart wrote:
> Hi Martin,
> 
> Thanks for the patch. One comment below.
> 
> On Tuesday 18 January 2011 22:27:42 Martin Hostettler wrote:
> > Adds support for V4L2_MBUS_FMT_Y8_1X8 format and 8bit data width in
> > synchronous interface.
> > 
> > When in 8bit mode don't apply DC substraction of 64 per default as this
> > would remove 1/4 of the sensor range.
> > 
> > When using V4L2_MBUS_FMT_Y8_1X8 (or possibly another 8bit per pixel) mode
> > set the CDCC to output 8bit per pixel instead of 16bit.
> > 
> > Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
> > ---
> >  drivers/media/video/isp/ispccdc.c  |   22 ++++++++++++++++++----
> >  drivers/media/video/isp/ispvideo.c |    2 ++
> >  2 files changed, 20 insertions(+), 4 deletions(-)
> > 
> > Changes since first version:
> > 	- forward ported to current media.git
> > 
> > diff --git a/drivers/media/video/isp/ispccdc.c
> > b/drivers/media/video/isp/ispccdc.c index 578c8bf..c7397c9 100644
[...]
> > @@ -2244,7 +2253,12 @@ int isp_ccdc_init(struct isp_device *isp)
> >  	ccdc->syncif.vdpol = 0;
> > 
> >  	ccdc->clamp.oblen = 0;
> > -	ccdc->clamp.dcsubval = 64;
> > +
> > +	if (isp->pdata->subdevs->interface == ISP_INTERFACE_PARALLEL
> > +	    && isp->pdata->subdevs->bus.parallel.width <= 8)
> > +		ccdc->clamp.dcsubval = 0;
> > +	else
> > +		ccdc->clamp.dcsubval = 64;
> 
> I don't like this too much. What happens if you have several sensors connected 
> to the system with different bus width ?
> 

Yes, you're right of course, the current code is semantically broken.

But the only clean solution i can think of is setting it to 0
unconditionally.
I'm not sure what this default should acomplish, so maybe i'm missing
something here, but i think the right value if dc substraction is needed
would be highly sensor specific?
I think all other of these postprocessing features for the CCDC default to
off, so it would make sense to default this to off too.

The overenginered solution would be to maintain a different value for each
bus width and let the user change the setting for the buswidth of the
currently linked sensor. In a way this would make sense,
because the DC substraction is fundamentally dependent on the bus size i
think. But i don't think anyone would want such complexity.

But i think it wouldn't be nice if every user of an 8bit sensor needs to
set this manually just to get the sensor working in a sane way (for 8bit
substracting 64 is insane, for wider buses it's different)

So how to proceed with this?

 - Martin Hostettler

Re: [PATCH V2] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[Michael Jones]

Hi Laurent,

On 01/19/2011 05:38 PM, Laurent Pinchart wrote:
> Hi Michael,
> 
<snip>
>>>> @@ -1144,10 +1148,15 @@ static void ccdc_configure(struct
>>>> isp_ccdc_device *ccdc) else
>>>>
>>>>  		syn_mode &= ~ISPCCDC_SYN_MODE_SDR2RSZ;
>>>>
>>>> -	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
>>>> +	/* Use PACK8 mode for 1byte per pixel formats */
>>>>
>>>> -	/* CCDC_PAD_SINK */
>>>> -	format = &ccdc->formats[CCDC_PAD_SINK];
>>>> +	if (isp_video_format_info(format->code)->bpp <= 8)
>>>> +		syn_mode |= ISPCCDC_SYN_MODE_PACK8;
>>>> +	else
>>>> +		syn_mode &= ~ISPCCDC_SYN_MODE_PACK8;
>>>> +
>>
>> It would make sense to me to move this bit into ispccdc_config_sync_if().
> 
> Why do you think so ? This configures how the data is written to memory, while 
> ispccdc_config_sync_if() configures the CCDC input interface.

I see. I was only thinking of ispccdc_config_sync_if() as configuring
the CCDC_SYN_MODE register.  I was in fact wondering why the other
syn_mode assignments weren't made in there.  Now that I understand the
division, I agree that setting PACK8 makes sense wherever the other
memory-writing settings are.

> 
>>>> +
>>>> +	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
>>>>
>>>>  	/* Mosaic filter */
>>>>  	switch (format->code) {
>>>>
>>>> @@ -2244,7 +2253,12 @@ int isp_ccdc_init(struct isp_device *isp)
>>>>
>>>>  	ccdc->syncif.vdpol = 0;
>>>>  	
>>>>  	ccdc->clamp.oblen = 0;
>>>>
>>>> -	ccdc->clamp.dcsubval = 64;
>>>> +
>>>> +	if (isp->pdata->subdevs->interface == ISP_INTERFACE_PARALLEL
>>>> +	    && isp->pdata->subdevs->bus.parallel.width <= 8)
>>>> +		ccdc->clamp.dcsubval = 0;
>>>> +	else
>>>> +		ccdc->clamp.dcsubval = 64;
>>>
>>> I don't like this too much. What happens if you have several sensors
>>> connected to the system with different bus width ?
>>
>> I see Laurent's point here.  Maybe move the dcsubval assignment into
>> ccdc_configure().  Also, don't we also want to remove dcsubval for an
>> 8-bit serially-attached sensor?  In ccdc_configure() you could make it
>> conditional on the mbus format's width on the CCDC sink pad.
> 
> This piece of code only sets the default value. If the user sets another 
> value, the driver must not override it silently when the video stream is 
> started. I'm not really sure how to properly fix this. The best solution is of 
> course to set the value from userspace.

I see the predicament. 64 is a bad default value for 8-bit data, but we
can't at init time know whether we're going to have 8-bit data or
10(+)-bit data to set a different default.  And you can't make a 64-or-0
decision at runtime because the user may have set a custom value after
init (although this isn't currently possible).  But I don't think a user
should have to adjust dcsubval just because he changed to an 8-bit image
and wants a decent image.  Especially since at the moment the user can't
do that anyway.  What I keep coming back to, though it sounds ugly, is 2
different default values for dcsubval.

> 
>>>>  	ccdc->vpcfg.pixelclk = 0;
> 


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH V2] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[Laurent Pinchart]

Hi Martin,

On Wednesday 19 January 2011 18:47:59 martin@neutronstar.dyndns.org wrote:
> On Wed, Jan 19, 2011 at 12:27:19AM +0100, Laurent Pinchart wrote:
> > On Tuesday 18 January 2011 22:27:42 Martin Hostettler wrote:
> > > Adds support for V4L2_MBUS_FMT_Y8_1X8 format and 8bit data width in
> > > synchronous interface.
> > > 
> > > When in 8bit mode don't apply DC substraction of 64 per default as this
> > > would remove 1/4 of the sensor range.
> > > 
> > > When using V4L2_MBUS_FMT_Y8_1X8 (or possibly another 8bit per pixel)
> > > mode set the CDCC to output 8bit per pixel instead of 16bit.
> > > 
> > > Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
> > > ---
> > > 
> > >  drivers/media/video/isp/ispccdc.c  |   22 ++++++++++++++++++----
> > >  drivers/media/video/isp/ispvideo.c |    2 ++
> > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > 
> > > Changes since first version:
> > > 	- forward ported to current media.git
> > > 
> > > diff --git a/drivers/media/video/isp/ispccdc.c
> > > b/drivers/media/video/isp/ispccdc.c index 578c8bf..c7397c9 100644
> 
> [...]
> 
> > > @@ -2244,7 +2253,12 @@ int isp_ccdc_init(struct isp_device *isp)
> > > 
> > >  	ccdc->syncif.vdpol = 0;
> > >  	
> > >  	ccdc->clamp.oblen = 0;
> > > 
> > > -	ccdc->clamp.dcsubval = 64;
> > > +
> > > +	if (isp->pdata->subdevs->interface == ISP_INTERFACE_PARALLEL
> > > +	    && isp->pdata->subdevs->bus.parallel.width <= 8)
> > > +		ccdc->clamp.dcsubval = 0;
> > > +	else
> > > +		ccdc->clamp.dcsubval = 64;
> > 
> > I don't like this too much. What happens if you have several sensors
> > connected to the system with different bus width ?
> 
> Yes, you're right of course, the current code is semantically broken.
> 
> But the only clean solution i can think of is setting it to 0
> unconditionally.
> I'm not sure what this default should acomplish, so maybe i'm missing
> something here, but i think the right value if dc substraction is needed
> would be highly sensor specific?
> I think all other of these postprocessing features for the CCDC default to
> off, so it would make sense to default this to off too.
> 
> The overenginered solution would be to maintain a different value for each
> bus width and let the user change the setting for the buswidth of the
> currently linked sensor. In a way this would make sense,
> because the DC substraction is fundamentally dependent on the bus size i
> think. But i don't think anyone would want such complexity.
> 
> But i think it wouldn't be nice if every user of an 8bit sensor needs to
> set this manually just to get the sensor working in a sane way (for 8bit
> substracting 64 is insane, for wider buses it's different)
> 
> So how to proceed with this?

My personal opinion (at least for now) is that we should set the default value 
to 0. I'll see if I can convince people at Nokia that it would be the right 
way to go. If so I'll apply a patch for that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH V2] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[Martin Hostettler]

On Thu, Jan 20, 2011 at 03:37:50PM +0100, Laurent Pinchart wrote:
> Hi Martin,
> 
> On Wednesday 19 January 2011 18:47:59 martin@neutronstar.dyndns.org wrote:
> > But the only clean solution i can think of is setting it to 0
> > unconditionally.
> > I'm not sure what this default should acomplish, so maybe i'm missing
> > something here, but i think the right value if dc substraction is needed
> > would be highly sensor specific?
> > I think all other of these postprocessing features for the CCDC default to
> > off, so it would make sense to default this to off too.
> > 
> > The overenginered solution would be to maintain a different value for each
> > bus width and let the user change the setting for the buswidth of the
> > currently linked sensor. In a way this would make sense,
> > because the DC substraction is fundamentally dependent on the bus size i
> > think. But i don't think anyone would want such complexity.
> > 
> > But i think it wouldn't be nice if every user of an 8bit sensor needs to
> > set this manually just to get the sensor working in a sane way (for 8bit
> > substracting 64 is insane, for wider buses it's different)
> > 
> > So how to proceed with this?
> 
> My personal opinion (at least for now) is that we should set the default value 
> to 0. I'll see if I can convince people at Nokia that it would be the right 
> way to go. If so I'll apply a patch for that.
> 

Yes, that would be great, thanks.

I'll resend the patch with this part removed.

regards,
 - Martin Hostettler

[PATCH V3] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[Martin Hostettler]

Adds support for V4L2_MBUS_FMT_Y8_1X8 format and 8bit data width in
synchronous interface.

When using V4L2_MBUS_FMT_Y8_1X8 (or possibly another 8bit per pixel) mode
set the CDCC to output 8bit per pixel instead of 16bit.

Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
---
 drivers/media/video/isp/ispccdc.c  |   15 ++++++++++++---
 drivers/media/video/isp/ispvideo.c |    2 ++
 2 files changed, 14 insertions(+), 3 deletions(-)

Changes since first version:
	- forward ported to current media.git
Changes since second version:
	- remove ccdc->clamp.dcsubval related changes

diff --git a/drivers/media/video/isp/ispccdc.c b/drivers/media/video/isp/ispccdc.c
index 578c8bf..7632bc1 100644
--- a/drivers/media/video/isp/ispccdc.c
+++ b/drivers/media/video/isp/ispccdc.c
@@ -43,6 +43,7 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 		  unsigned int pad, enum v4l2_subdev_format_whence which);
 
 static const unsigned int ccdc_fmts[] = {
+	V4L2_MBUS_FMT_Y8_1X8,
 	V4L2_MBUS_FMT_SGRBG10_1X10,
 	V4L2_MBUS_FMT_SRGGB10_1X10,
 	V4L2_MBUS_FMT_SBGGR10_1X10,
@@ -1127,6 +1128,9 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	ccdc->syncif.datsz = pdata ? pdata->width : 10;
 	ispccdc_config_sync_if(ccdc, &ccdc->syncif);
 
+	/* CCDC_PAD_SINK */
+	format = &ccdc->formats[CCDC_PAD_SINK];
+
 	syn_mode = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
 
 	/* Use the raw, unprocessed data when writing to memory. The H3A and
@@ -1144,10 +1148,15 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	else
 		syn_mode &= ~ISPCCDC_SYN_MODE_SDR2RSZ;
 
-	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
+	/* Use PACK8 mode for 1byte per pixel formats */
 
-	/* CCDC_PAD_SINK */
-	format = &ccdc->formats[CCDC_PAD_SINK];
+	if (isp_video_format_info(format->code)->bpp <= 8)
+		syn_mode |= ISPCCDC_SYN_MODE_PACK8;
+	else
+		syn_mode &= ~ISPCCDC_SYN_MODE_PACK8;
+
+
+	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
 
 	/* Mosaic filter */
 	switch (format->code) {
diff --git a/drivers/media/video/isp/ispvideo.c b/drivers/media/video/isp/ispvideo.c
index 5f984e4..cd3d331 100644
--- a/drivers/media/video/isp/ispvideo.c
+++ b/drivers/media/video/isp/ispvideo.c
@@ -221,6 +221,8 @@ isp_video_check_format(struct isp_video *video, struct isp_video_fh *vfh)
 }
 
 static struct isp_format_info formats[] = {
+	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
+	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
 	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
 	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
 	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,
-- 
1.7.1

Re: [PATCH V3] v4l: OMAP3 ISP CCDC: Add support for 8bit greyscale sensors

[Laurent Pinchart]

Hi Martin,

Thanks for the patch.

On Friday 21 January 2011 00:43:31 Martin Hostettler wrote:
> Adds support for V4L2_MBUS_FMT_Y8_1X8 format and 8bit data width in
> synchronous interface.
> 
> When using V4L2_MBUS_FMT_Y8_1X8 (or possibly another 8bit per pixel) mode
> set the CDCC to output 8bit per pixel instead of 16bit.
> 
> Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I've submitted the patch internally, it should end up in the public tree in 
the (hopefully) near future.

-- 
Regards,

Laurent Pinchart