* [PATCH v3 0/4] OMAP3-ISP lane shifter support
@ 2011-03-11 8:05 Michael Jones
2011-03-11 8:05 ` [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format Michael Jones
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Michael Jones @ 2011-03-11 8:05 UTC (permalink / raw)
To: Laurent Pinchart, linux-media; +Cc: Sakari Ailus, Hans Verkuil
Add support for the ISP's lane shifter. To use the shifter, set different
pixel formats at each end of the link at the CCDC input.
This has only been tested shifting Y12 and SBGGR12 from a parallel sensor to Y8
and SBGGR8 (respectively) at the CCDC input. Support has also been added for
other formats and other shifting values, but is untested. Shifting data coming
from one of the serial sensor interfaces (CSI2a, etc) is also untested.
As before, ccdc_try_format() does not check that the format at its input is
compatible with the format coming from the sensor interface. This consistency
check is first done when activating the pipeline.
These patches apply to Laurent's media-0005-omap3isp branch, based on 2.6.38-rc5
Changes since v2:
-new formats are also added to Documentation/DocBook/v4l/
-reintroduce .data_lane_shift for sensors whose LSB is not aligned with
sensor interfaces's LSB.
-style changes according to feedback
Changes since v1:
-added support for remaining 8-bit Bayer formats (SGBRG8_1X8 & SRGGB8_1X8)
-moved omap3isp_is_shiftable() from isp.c to ispvideo.c and return bool
-moved 'shift' calculation from omap3isp_configure_bridge() to ccdc_configure()
-added 'shift' arg to omap3isp_configure_bridge()
-misc minor changes according to feedback (removed unnecessary tests, etc.)
Michael Jones (4):
v4l: add V4L2_PIX_FMT_Y12 format
media: add 8-bit bayer formats and Y12
omap3isp: ccdc: support Y10/12, 8-bit bayer fmts
omap3isp: lane shifter support
Documentation/DocBook/v4l/pixfmt-y12.xml | 79 +++++++++++++++++++
Documentation/DocBook/v4l/subdev-formats.xml | 59 ++++++++++++++
drivers/media/video/omap3-isp/isp.c | 7 +-
drivers/media/video/omap3-isp/isp.h | 5 +-
drivers/media/video/omap3-isp/ispccdc.c | 33 +++++++-
drivers/media/video/omap3-isp/ispvideo.c | 108 ++++++++++++++++++++++----
drivers/media/video/omap3-isp/ispvideo.h | 3 +
include/linux/v4l2-mediabus.h | 7 +-
include/linux/videodev2.h | 1 +
9 files changed, 276 insertions(+), 26 deletions(-)
create mode 100644 Documentation/DocBook/v4l/pixfmt-y12.xml
--
1.7.4.1
MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format 2011-03-11 8:05 [PATCH v3 0/4] OMAP3-ISP lane shifter support Michael Jones @ 2011-03-11 8:05 ` Michael Jones 2011-03-11 8:56 ` Laurent Pinchart 2011-03-11 9:21 ` Antonio Ospite 2011-03-11 8:05 ` [PATCH v3 2/4] media: add 8-bit bayer formats and Y12 Michael Jones ` (2 subsequent siblings) 3 siblings, 2 replies; 17+ messages in thread From: Michael Jones @ 2011-03-11 8:05 UTC (permalink / raw) To: Laurent Pinchart, linux-media; +Cc: Sakari Ailus, Hans Verkuil Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> --- Documentation/DocBook/v4l/pixfmt-y12.xml | 79 ++++++++++++++++++++++++++++++ include/linux/videodev2.h | 1 + 2 files changed, 80 insertions(+), 0 deletions(-) create mode 100644 Documentation/DocBook/v4l/pixfmt-y12.xml diff --git a/Documentation/DocBook/v4l/pixfmt-y12.xml b/Documentation/DocBook/v4l/pixfmt-y12.xml new file mode 100644 index 0000000..ff417b8 --- /dev/null +++ b/Documentation/DocBook/v4l/pixfmt-y12.xml @@ -0,0 +1,79 @@ +<refentry id="V4L2-PIX-FMT-Y12"> + <refmeta> + <refentrytitle>V4L2_PIX_FMT_Y12 ('Y12 ')</refentrytitle> + &manvol; + </refmeta> + <refnamediv> + <refname><constant>V4L2_PIX_FMT_Y12</constant></refname> + <refpurpose>Grey-scale image</refpurpose> + </refnamediv> + <refsect1> + <title>Description</title> + + <para>This is a grey-scale image with a depth of 12 bits per pixel. Pixels +are stored in 16-bit words with unused high bits padded with 0. The least +significant byte is stored at lower memory addresses (little-endian).</para> + + <example> + <title><constant>V4L2_PIX_FMT_Y12</constant> 4 × 4 +pixel image</title> + + <formalpara> + <title>Byte Order.</title> + <para>Each cell is one byte. + <informaltable frame="none"> + <tgroup cols="9" align="center"> + <colspec align="left" colwidth="2*" /> + <tbody valign="top"> + <row> + <entry>start + 0:</entry> + <entry>Y'<subscript>00low</subscript></entry> + <entry>Y'<subscript>00high</subscript></entry> + <entry>Y'<subscript>01low</subscript></entry> + <entry>Y'<subscript>01high</subscript></entry> + <entry>Y'<subscript>02low</subscript></entry> + <entry>Y'<subscript>02high</subscript></entry> + <entry>Y'<subscript>03low</subscript></entry> + <entry>Y'<subscript>03high</subscript></entry> + </row> + <row> + <entry>start + 8:</entry> + <entry>Y'<subscript>10low</subscript></entry> + <entry>Y'<subscript>10high</subscript></entry> + <entry>Y'<subscript>11low</subscript></entry> + <entry>Y'<subscript>11high</subscript></entry> + <entry>Y'<subscript>12low</subscript></entry> + <entry>Y'<subscript>12high</subscript></entry> + <entry>Y'<subscript>13low</subscript></entry> + <entry>Y'<subscript>13high</subscript></entry> + </row> + <row> + <entry>start + 16:</entry> + <entry>Y'<subscript>20low</subscript></entry> + <entry>Y'<subscript>20high</subscript></entry> + <entry>Y'<subscript>21low</subscript></entry> + <entry>Y'<subscript>21high</subscript></entry> + <entry>Y'<subscript>22low</subscript></entry> + <entry>Y'<subscript>22high</subscript></entry> + <entry>Y'<subscript>23low</subscript></entry> + <entry>Y'<subscript>23high</subscript></entry> + </row> + <row> + <entry>start + 24:</entry> + <entry>Y'<subscript>30low</subscript></entry> + <entry>Y'<subscript>30high</subscript></entry> + <entry>Y'<subscript>31low</subscript></entry> + <entry>Y'<subscript>31high</subscript></entry> + <entry>Y'<subscript>32low</subscript></entry> + <entry>Y'<subscript>32high</subscript></entry> + <entry>Y'<subscript>33low</subscript></entry> + <entry>Y'<subscript>33high</subscript></entry> + </row> + </tbody> + </tgroup> + </informaltable> + </para> + </formalpara> + </example> + </refsect1> +</refentry> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 02da9e7..6fac463 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -288,6 +288,7 @@ struct v4l2_pix_format { #define V4L2_PIX_FMT_Y4 v4l2_fourcc('Y', '0', '4', ' ') /* 4 Greyscale */ #define V4L2_PIX_FMT_Y6 v4l2_fourcc('Y', '0', '6', ' ') /* 6 Greyscale */ #define V4L2_PIX_FMT_Y10 v4l2_fourcc('Y', '1', '0', ' ') /* 10 Greyscale */ +#define V4L2_PIX_FMT_Y12 v4l2_fourcc('Y', '1', '2', ' ') /* 12 Greyscale */ #define V4L2_PIX_FMT_Y16 v4l2_fourcc('Y', '1', '6', ' ') /* 16 Greyscale */ /* Palette formats */ -- 1.7.4.1 MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format 2011-03-11 8:05 ` [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format Michael Jones @ 2011-03-11 8:56 ` Laurent Pinchart 2011-03-11 9:21 ` Antonio Ospite 1 sibling, 0 replies; 17+ messages in thread From: Laurent Pinchart @ 2011-03-11 8:56 UTC (permalink / raw) To: Michael Jones; +Cc: linux-media, Sakari Ailus, Hans Verkuil Hi Michael, Thanks for the patch. On Friday 11 March 2011 09:05:46 Michael Jones wrote: > Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> > --- > Documentation/DocBook/v4l/pixfmt-y12.xml | 79 +++++++++++++++++++++++++++ > include/linux/videodev2.h | 1 + > 2 files changed, 80 insertions(+), 0 deletions(-) > create mode 100644 Documentation/DocBook/v4l/pixfmt-y12.xml You also need to modify Documentation/DocBook/v4l/pixfmt.xml (and Documentation/DocBook/media-entities.tmpl) to include pixfmt-y12.xml, otherwise the new documentation file won't be used. Search for 'sub-y16' for an example. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format 2011-03-11 8:05 ` [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format Michael Jones 2011-03-11 8:56 ` Laurent Pinchart @ 2011-03-11 9:21 ` Antonio Ospite 2011-03-11 9:38 ` Michael Jones 1 sibling, 1 reply; 17+ messages in thread From: Antonio Ospite @ 2011-03-11 9:21 UTC (permalink / raw) To: Michael Jones; +Cc: Laurent Pinchart, linux-media, Sakari Ailus, Hans Verkuil [-- Attachment #1: Type: text/plain, Size: 774 bytes --] On Fri, 11 Mar 2011 09:05:46 +0100 Michael Jones <michael.jones@matrix-vision.de> wrote: > Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> > --- > Documentation/DocBook/v4l/pixfmt-y12.xml | 79 ++++++++++++++++++++++++++++++ > include/linux/videodev2.h | 1 + > 2 files changed, 80 insertions(+), 0 deletions(-) > create mode 100644 Documentation/DocBook/v4l/pixfmt-y12.xml > Hi Michael, are you going to release also Y12 conversion routines for libv4lconvert? Regards, Antonio -- Antonio Ospite http://ao2.it PGP public key ID: 0x4553B001 A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing? [-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format 2011-03-11 9:21 ` Antonio Ospite @ 2011-03-11 9:38 ` Michael Jones 2011-03-11 11:15 ` Antonio Ospite 0 siblings, 1 reply; 17+ messages in thread From: Michael Jones @ 2011-03-11 9:38 UTC (permalink / raw) To: Antonio Ospite; +Cc: Laurent Pinchart, linux-media, Sakari Ailus, Hans Verkuil On 03/11/2011 10:21 AM, Antonio Ospite wrote: > Hi Michael, > > are you going to release also Y12 conversion routines for libv4lconvert? > > Regards, > Antonio > Hi Antonio, As I am neither a user nor developer of libv4lconvert, I am not planning on adding Y12 conversion routines there. Hopefully somebody else will step up. Maybe you? -Michael MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format 2011-03-11 9:38 ` Michael Jones @ 2011-03-11 11:15 ` Antonio Ospite 0 siblings, 0 replies; 17+ messages in thread From: Antonio Ospite @ 2011-03-11 11:15 UTC (permalink / raw) To: Michael Jones; +Cc: Laurent Pinchart, linux-media, Sakari Ailus, Hans Verkuil [-- Attachment #1: Type: text/plain, Size: 1041 bytes --] On Fri, 11 Mar 2011 10:38:08 +0100 Michael Jones <michael.jones@matrix-vision.de> wrote: > On 03/11/2011 10:21 AM, Antonio Ospite wrote: > > Hi Michael, > > > > are you going to release also Y12 conversion routines for libv4lconvert? > > > > Regards, > > Antonio > > > > Hi Antonio, > > As I am neither a user nor developer of libv4lconvert, I am not planning > on adding Y12 conversion routines there. Hopefully somebody else will > step up. Maybe you? > I asked just for curiosity as I don't have any device producing this Y12 format, however I _might_ play with it if you can provide some Y12 (or Y10) raw frames. I am playing with some compressed variant of Y10 and I am exploring different ways to add support for those formats to libv4l. Regards, Antonio -- Antonio Ospite http://ao2.it PGP public key ID: 0x4553B001 A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing? [-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 2/4] media: add 8-bit bayer formats and Y12 2011-03-11 8:05 [PATCH v3 0/4] OMAP3-ISP lane shifter support Michael Jones 2011-03-11 8:05 ` [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format Michael Jones @ 2011-03-11 8:05 ` Michael Jones 2011-03-11 8:05 ` [PATCH v3 3/4] omap3isp: ccdc: support Y10/12, 8-bit bayer fmts Michael Jones 2011-03-11 8:05 ` [PATCH v3 4/4] omap3isp: lane shifter support Michael Jones 3 siblings, 0 replies; 17+ messages in thread From: Michael Jones @ 2011-03-11 8:05 UTC (permalink / raw) To: Laurent Pinchart, linux-media; +Cc: Sakari Ailus, Hans Verkuil Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> --- Documentation/DocBook/v4l/subdev-formats.xml | 59 ++++++++++++++++++++++++++ include/linux/v4l2-mediabus.h | 7 ++- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/Documentation/DocBook/v4l/subdev-formats.xml b/Documentation/DocBook/v4l/subdev-formats.xml index 2fed9be..37e5bc6 100644 --- a/Documentation/DocBook/v4l/subdev-formats.xml +++ b/Documentation/DocBook/v4l/subdev-formats.xml @@ -456,6 +456,23 @@ <entry>b<subscript>1</subscript></entry> <entry>b<subscript>0</subscript></entry> </row> + <row id="V4L2-MBUS-FMT-SGBRG8-1X8"> + <entry>V4L2_MBUS_FMT_SGBRG8_1X8</entry> + <entry>0x3013</entry> + <entry></entry> + <entry>-</entry> + <entry>-</entry> + <entry>-</entry> + <entry>-</entry> + <entry>g<subscript>7</subscript></entry> + <entry>g<subscript>6</subscript></entry> + <entry>g<subscript>5</subscript></entry> + <entry>g<subscript>4</subscript></entry> + <entry>g<subscript>3</subscript></entry> + <entry>g<subscript>2</subscript></entry> + <entry>g<subscript>1</subscript></entry> + <entry>g<subscript>0</subscript></entry> + </row> <row id="V4L2-MBUS-FMT-SGRBG8-1X8"> <entry>V4L2_MBUS_FMT_SGRBG8_1X8</entry> <entry>0x3002</entry> @@ -473,6 +490,23 @@ <entry>g<subscript>1</subscript></entry> <entry>g<subscript>0</subscript></entry> </row> + <row id="V4L2-MBUS-FMT-SRGGB8-1X8"> + <entry>V4L2_MBUS_FMT_SRGGB8_1X8</entry> + <entry>0x3014</entry> + <entry></entry> + <entry>-</entry> + <entry>-</entry> + <entry>-</entry> + <entry>-</entry> + <entry>r<subscript>7</subscript></entry> + <entry>r<subscript>6</subscript></entry> + <entry>r<subscript>5</subscript></entry> + <entry>r<subscript>4</subscript></entry> + <entry>r<subscript>3</subscript></entry> + <entry>r<subscript>2</subscript></entry> + <entry>r<subscript>1</subscript></entry> + <entry>r<subscript>0</subscript></entry> + </row> <row id="V4L2-MBUS-FMT-SBGGR10-DPCM8-1X8"> <entry>V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8</entry> <entry>0x300b</entry> @@ -2159,6 +2193,31 @@ <entry>u<subscript>1</subscript></entry> <entry>u<subscript>0</subscript></entry> </row> + <row id="V4L2-MBUS-FMT-Y12-1X12"> + <entry>V4L2_MBUS_FMT_Y12_1X12</entry> + <entry>0x2013</entry> + <entry></entry> + <entry>-</entry> + <entry>-</entry> + <entry>-</entry> + <entry>-</entry> + <entry>-</entry> + <entry>-</entry> + <entry>-</entry> + <entry>-</entry> + <entry>y<subscript>11</subscript></entry> + <entry>y<subscript>10</subscript></entry> + <entry>y<subscript>9</subscript></entry> + <entry>y<subscript>8</subscript></entry> + <entry>y<subscript>7</subscript></entry> + <entry>y<subscript>6</subscript></entry> + <entry>y<subscript>5</subscript></entry> + <entry>y<subscript>4</subscript></entry> + <entry>y<subscript>3</subscript></entry> + <entry>y<subscript>2</subscript></entry> + <entry>y<subscript>1</subscript></entry> + <entry>y<subscript>0</subscript></entry> + </row> <row id="V4L2-MBUS-FMT-UYVY8-1X16"> <entry>V4L2_MBUS_FMT_UYVY8_1X16</entry> <entry>0x200f</entry> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h index 7054a7a..de5c159 100644 --- a/include/linux/v4l2-mediabus.h +++ b/include/linux/v4l2-mediabus.h @@ -47,7 +47,7 @@ enum v4l2_mbus_pixelcode { V4L2_MBUS_FMT_RGB565_2X8_BE = 0x1007, V4L2_MBUS_FMT_RGB565_2X8_LE = 0x1008, - /* YUV (including grey) - next is 0x2013 */ + /* YUV (including grey) - next is 0x2014 */ V4L2_MBUS_FMT_Y8_1X8 = 0x2001, V4L2_MBUS_FMT_UYVY8_1_5X8 = 0x2002, V4L2_MBUS_FMT_VYUY8_1_5X8 = 0x2003, @@ -60,6 +60,7 @@ enum v4l2_mbus_pixelcode { V4L2_MBUS_FMT_Y10_1X10 = 0x200a, V4L2_MBUS_FMT_YUYV10_2X10 = 0x200b, V4L2_MBUS_FMT_YVYU10_2X10 = 0x200c, + V4L2_MBUS_FMT_Y12_1X12 = 0x2013, V4L2_MBUS_FMT_UYVY8_1X16 = 0x200f, V4L2_MBUS_FMT_VYUY8_1X16 = 0x2010, V4L2_MBUS_FMT_YUYV8_1X16 = 0x2011, @@ -67,9 +68,11 @@ enum v4l2_mbus_pixelcode { V4L2_MBUS_FMT_YUYV10_1X20 = 0x200d, V4L2_MBUS_FMT_YVYU10_1X20 = 0x200e, - /* Bayer - next is 0x3013 */ + /* Bayer - next is 0x3015 */ V4L2_MBUS_FMT_SBGGR8_1X8 = 0x3001, + V4L2_MBUS_FMT_SGBRG8_1X8 = 0x3013, V4L2_MBUS_FMT_SGRBG8_1X8 = 0x3002, + V4L2_MBUS_FMT_SRGGB8_1X8 = 0x3014, V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8 = 0x300b, V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8 = 0x300c, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8 = 0x3009, -- 1.7.4.1 MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 3/4] omap3isp: ccdc: support Y10/12, 8-bit bayer fmts 2011-03-11 8:05 [PATCH v3 0/4] OMAP3-ISP lane shifter support Michael Jones 2011-03-11 8:05 ` [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format Michael Jones 2011-03-11 8:05 ` [PATCH v3 2/4] media: add 8-bit bayer formats and Y12 Michael Jones @ 2011-03-11 8:05 ` Michael Jones 2011-03-11 8:05 ` [PATCH v3 4/4] omap3isp: lane shifter support Michael Jones 3 siblings, 0 replies; 17+ messages in thread From: Michael Jones @ 2011-03-11 8:05 UTC (permalink / raw) To: Laurent Pinchart, linux-media; +Cc: Sakari Ailus, Hans Verkuil Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/video/omap3-isp/ispccdc.c | 6 ++++++ drivers/media/video/omap3-isp/ispvideo.c | 12 ++++++++++++ 2 files changed, 18 insertions(+), 0 deletions(-) diff --git a/drivers/media/video/omap3-isp/ispccdc.c b/drivers/media/video/omap3-isp/ispccdc.c index e4d04ce..23000b6 100644 --- a/drivers/media/video/omap3-isp/ispccdc.c +++ b/drivers/media/video/omap3-isp/ispccdc.c @@ -43,6 +43,12 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh, static const unsigned int ccdc_fmts[] = { V4L2_MBUS_FMT_Y8_1X8, + V4L2_MBUS_FMT_Y10_1X10, + V4L2_MBUS_FMT_Y12_1X12, + V4L2_MBUS_FMT_SGRBG8_1X8, + V4L2_MBUS_FMT_SRGGB8_1X8, + V4L2_MBUS_FMT_SBGGR8_1X8, + V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10, diff --git a/drivers/media/video/omap3-isp/ispvideo.c b/drivers/media/video/omap3-isp/ispvideo.c index f16d787..3c3b3c4 100644 --- a/drivers/media/video/omap3-isp/ispvideo.c +++ b/drivers/media/video/omap3-isp/ispvideo.c @@ -48,6 +48,18 @@ 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_Y10_1X10, V4L2_MBUS_FMT_Y10_1X10, + V4L2_MBUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10, 10, }, + { V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y10_1X10, + V4L2_MBUS_FMT_Y12_1X12, V4L2_PIX_FMT_Y12, 12, }, + { V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8, + V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8, }, + { V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8, + V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8, }, + { V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8, + V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8, }, + { V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8, + V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 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.4.1 MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 4/4] omap3isp: lane shifter support 2011-03-11 8:05 [PATCH v3 0/4] OMAP3-ISP lane shifter support Michael Jones ` (2 preceding siblings ...) 2011-03-11 8:05 ` [PATCH v3 3/4] omap3isp: ccdc: support Y10/12, 8-bit bayer fmts Michael Jones @ 2011-03-11 8:05 ` Michael Jones 2011-03-16 15:44 ` Sakari Ailus 3 siblings, 1 reply; 17+ messages in thread From: Michael Jones @ 2011-03-11 8:05 UTC (permalink / raw) To: Laurent Pinchart, linux-media; +Cc: Sakari Ailus, Hans Verkuil To use the lane shifter, set different pixel formats at each end of the link at the CCDC input. Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> --- drivers/media/video/omap3-isp/isp.c | 7 ++- drivers/media/video/omap3-isp/isp.h | 5 +- drivers/media/video/omap3-isp/ispccdc.c | 27 ++++++-- drivers/media/video/omap3-isp/ispvideo.c | 108 ++++++++++++++++++++++++------ drivers/media/video/omap3-isp/ispvideo.h | 3 + 5 files changed, 120 insertions(+), 30 deletions(-) diff --git a/drivers/media/video/omap3-isp/isp.c b/drivers/media/video/omap3-isp/isp.c index 08d90fe..866ce09 100644 --- a/drivers/media/video/omap3-isp/isp.c +++ b/drivers/media/video/omap3-isp/isp.c @@ -285,7 +285,8 @@ static void isp_power_settings(struct isp_device *isp, int idle) */ void omap3isp_configure_bridge(struct isp_device *isp, enum ccdc_input_entity input, - const struct isp_parallel_platform_data *pdata) + const struct isp_parallel_platform_data *pdata, + int shift) { u32 ispctrl_val; @@ -298,9 +299,9 @@ void omap3isp_configure_bridge(struct isp_device *isp, switch (input) { case CCDC_INPUT_PARALLEL: ispctrl_val |= ISPCTRL_PAR_SER_CLK_SEL_PARALLEL; - ispctrl_val |= pdata->data_lane_shift << ISPCTRL_SHIFT_SHIFT; ispctrl_val |= pdata->clk_pol << ISPCTRL_PAR_CLK_POL_SHIFT; ispctrl_val |= pdata->bridge << ISPCTRL_PAR_BRIDGE_SHIFT; + shift += pdata->data_lane_shift*2; break; case CCDC_INPUT_CSI2A: @@ -319,6 +320,8 @@ void omap3isp_configure_bridge(struct isp_device *isp, return; } + ispctrl_val |= ((shift/2) << ISPCTRL_SHIFT_SHIFT) & ISPCTRL_SHIFT_MASK; + ispctrl_val &= ~ISPCTRL_SYNC_DETECT_MASK; ispctrl_val |= ISPCTRL_SYNC_DETECT_VSRISE; diff --git a/drivers/media/video/omap3-isp/isp.h b/drivers/media/video/omap3-isp/isp.h index 21fa88b..6f0ff1a 100644 --- a/drivers/media/video/omap3-isp/isp.h +++ b/drivers/media/video/omap3-isp/isp.h @@ -130,7 +130,6 @@ struct isp_reg { /** * struct isp_parallel_platform_data - Parallel interface platform data - * @width: Parallel bus width in bits (8, 10, 11 or 12) * @data_lane_shift: Data lane shifter * 0 - CAMEXT[13:0] -> CAM[13:0] * 1 - CAMEXT[13:2] -> CAM[11:0] @@ -144,7 +143,6 @@ struct isp_reg { * ISPCTRL_PAR_BRIDGE_BENDIAN - Big endian */ struct isp_parallel_platform_data { - unsigned int width; unsigned int data_lane_shift:2; unsigned int clk_pol:1; unsigned int bridge:4; @@ -322,7 +320,8 @@ int omap3isp_pipeline_set_stream(struct isp_pipeline *pipe, enum isp_pipeline_stream_state state); void omap3isp_configure_bridge(struct isp_device *isp, enum ccdc_input_entity input, - const struct isp_parallel_platform_data *pdata); + const struct isp_parallel_platform_data *pdata, + int shift); #define ISP_XCLK_NONE -1 #define ISP_XCLK_A 0 diff --git a/drivers/media/video/omap3-isp/ispccdc.c b/drivers/media/video/omap3-isp/ispccdc.c index 23000b6..bbcf619 100644 --- a/drivers/media/video/omap3-isp/ispccdc.c +++ b/drivers/media/video/omap3-isp/ispccdc.c @@ -1120,21 +1120,38 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc) struct isp_parallel_platform_data *pdata = NULL; struct v4l2_subdev *sensor; struct v4l2_mbus_framefmt *format; + const struct isp_format_info *fmt_info; + struct v4l2_subdev_format fmt_src; + unsigned int depth_out = 0; + unsigned int depth_in = 0; struct media_pad *pad; unsigned long flags; + unsigned int shift; u32 syn_mode; u32 ccdc_pattern; - if (ccdc->input == CCDC_INPUT_PARALLEL) { - pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]); - sensor = media_entity_to_v4l2_subdev(pad->entity); + pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]); + sensor = media_entity_to_v4l2_subdev(pad->entity); + if (ccdc->input == CCDC_INPUT_PARALLEL) pdata = &((struct isp_v4l2_subdevs_group *)sensor->host_priv) ->bus.parallel; + + /* Compute shift value for lane shifter to configure the bridge. */ + fmt_src.pad = pad->index; + fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE; + if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) { + fmt_info = omap3isp_video_format_info(fmt_src.format.code); + depth_in = fmt_info->bpp; } - omap3isp_configure_bridge(isp, ccdc->input, pdata); + fmt_info = omap3isp_video_format_info + (isp->isp_ccdc.formats[CCDC_PAD_SINK].code); + depth_out = fmt_info->bpp; + + shift = depth_in - depth_out; + omap3isp_configure_bridge(isp, ccdc->input, pdata, shift); - ccdc->syncif.datsz = pdata ? pdata->width : 10; + ccdc->syncif.datsz = depth_out; ccdc_config_sync_if(ccdc, &ccdc->syncif); /* CCDC_PAD_SINK */ diff --git a/drivers/media/video/omap3-isp/ispvideo.c b/drivers/media/video/omap3-isp/ispvideo.c index 3c3b3c4..c4b9fe1 100644 --- a/drivers/media/video/omap3-isp/ispvideo.c +++ b/drivers/media/video/omap3-isp/ispvideo.c @@ -47,41 +47,59 @@ 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_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8, + V4L2_PIX_FMT_GREY, 8, }, { V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y10_1X10, - V4L2_MBUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10, 10, }, + V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y8_1X8, + V4L2_PIX_FMT_Y10, 10, }, { V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y10_1X10, - V4L2_MBUS_FMT_Y12_1X12, V4L2_PIX_FMT_Y12, 12, }, + V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y8_1X8, + V4L2_PIX_FMT_Y12, 12, }, { V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8, - V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8, }, + V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8, + V4L2_PIX_FMT_SBGGR8, 8, }, { V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8, - V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8, }, + V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8, + V4L2_PIX_FMT_SGBRG8, 8, }, { V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8, - V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8, }, + V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8, + V4L2_PIX_FMT_SGRBG8, 8, }, { V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8, - V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8, }, + V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8, + V4L2_PIX_FMT_SRGGB8, 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_SGRBG10_1X10, 0, + V4L2_PIX_FMT_SGRBG10DPCM8, 8, }, { V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10, - V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10, 10, }, + V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR8_1X8, + V4L2_PIX_FMT_SBGGR10, 10, }, { V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_MBUS_FMT_SGBRG10_1X10, - V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10, 10, }, + V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_MBUS_FMT_SGBRG8_1X8, + V4L2_PIX_FMT_SGBRG10, 10, }, { V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG10_1X10, - V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10, 10, }, + V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG8_1X8, + V4L2_PIX_FMT_SGRBG10, 10, }, { V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SRGGB10_1X10, - V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10, 10, }, + V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SRGGB8_1X8, + V4L2_PIX_FMT_SRGGB10, 10, }, { V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_MBUS_FMT_SBGGR10_1X10, - V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SBGGR12, 12, }, + V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_MBUS_FMT_SBGGR8_1X8, + V4L2_PIX_FMT_SBGGR12, 12, }, { V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_MBUS_FMT_SGBRG10_1X10, - V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12, 12, }, + V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_MBUS_FMT_SGBRG8_1X8, + V4L2_PIX_FMT_SGBRG12, 12, }, { V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_MBUS_FMT_SGRBG10_1X10, - V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12, 12, }, + V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_MBUS_FMT_SGRBG8_1X8, + V4L2_PIX_FMT_SGRBG12, 12, }, { V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_MBUS_FMT_SRGGB10_1X10, - V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12, 12, }, + V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_MBUS_FMT_SRGGB8_1X8, + V4L2_PIX_FMT_SRGGB12, 12, }, { V4L2_MBUS_FMT_UYVY8_1X16, V4L2_MBUS_FMT_UYVY8_1X16, - V4L2_MBUS_FMT_UYVY8_1X16, V4L2_PIX_FMT_UYVY, 16, }, + V4L2_MBUS_FMT_UYVY8_1X16, 0, + V4L2_PIX_FMT_UYVY, 16, }, { V4L2_MBUS_FMT_YUYV8_1X16, V4L2_MBUS_FMT_YUYV8_1X16, - V4L2_MBUS_FMT_YUYV8_1X16, V4L2_PIX_FMT_YUYV, 16, }, + V4L2_MBUS_FMT_YUYV8_1X16, 0, + V4L2_PIX_FMT_YUYV, 16, }, }; const struct isp_format_info * @@ -98,6 +116,37 @@ omap3isp_video_format_info(enum v4l2_mbus_pixelcode code) } /* + * Decide whether desired output pixel code can be obtained with + * the lane shifter by shifting the input pixel code. + * @in: input pixelcode to shifter + * @out: output pixelcode from shifter + * @additional_shift: # of bits the sensor's LSB is offset from CAMEXT[0] + * + * return true if the combination is possible + * return false otherwise + */ +static bool isp_video_is_shiftable(enum v4l2_mbus_pixelcode in, + enum v4l2_mbus_pixelcode out, + unsigned int additional_shift) +{ + const struct isp_format_info *in_info, *out_info; + + if (in == out) + return true; + + in_info = omap3isp_video_format_info(in); + out_info = omap3isp_video_format_info(out); + + if ((in_info->flavor == 0) || (out_info->flavor == 0)) + return false; + + if (in_info->flavor != out_info->flavor) + return false; + + return in_info->bpp - out_info->bpp + additional_shift <= 6; +} + +/* * isp_video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format * @video: ISP video instance * @mbus: v4l2_mbus_framefmt format (input) @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe) return -EPIPE; while (1) { + unsigned int link_has_shifter; /* Retrieve the sink format */ pad = &subdev->entity.pads[0]; if (!(pad->flags & MEDIA_PAD_FL_SINK)) @@ -275,6 +325,10 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe) return -ENOSPC; } + /* If sink pad is on CCDC, the link has the lane shifter + * in the middle of it. */ + link_has_shifter = subdev == &isp->isp_ccdc.subdev; + /* Retrieve the source format */ pad = media_entity_remote_source(pad); if (pad == NULL || @@ -290,10 +344,24 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe) return -EPIPE; /* Check if the two ends match */ - if (fmt_source.format.code != fmt_sink.format.code || - fmt_source.format.width != fmt_sink.format.width || + if (fmt_source.format.width != fmt_sink.format.width || fmt_source.format.height != fmt_sink.format.height) return -EPIPE; + + if (link_has_shifter) { + unsigned int parallel_shift = 0; + if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) { + struct isp_parallel_platform_data *pdata = + &((struct isp_v4l2_subdevs_group *) + subdev->host_priv)->bus.parallel; + parallel_shift = pdata->data_lane_shift * 2; + } + if (!isp_video_is_shiftable(fmt_source.format.code, + fmt_sink.format.code, + parallel_shift)) + return -EPIPE; + } else if (fmt_source.format.code != fmt_sink.format.code) + return -EPIPE; } return 0; diff --git a/drivers/media/video/omap3-isp/ispvideo.h b/drivers/media/video/omap3-isp/ispvideo.h index 524a1ac..911bea6 100644 --- a/drivers/media/video/omap3-isp/ispvideo.h +++ b/drivers/media/video/omap3-isp/ispvideo.h @@ -49,6 +49,8 @@ struct v4l2_pix_format; * bits. Identical to @code if the format is 10 bits wide or less. * @uncompressed: V4L2 media bus format code for the corresponding uncompressed * format. Identical to @code if the format is not DPCM compressed. + * @flavor: V4L2 media bus format code for the same pixel layout but + * shifted to be 8 bits per pixel. =0 if format is not shiftable. * @pixelformat: V4L2 pixel format FCC identifier * @bpp: Bits per pixel */ @@ -56,6 +58,7 @@ struct isp_format_info { enum v4l2_mbus_pixelcode code; enum v4l2_mbus_pixelcode truncated; enum v4l2_mbus_pixelcode uncompressed; + enum v4l2_mbus_pixelcode flavor; u32 pixelformat; unsigned int bpp; }; -- 1.7.4.1 MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/4] omap3isp: lane shifter support 2011-03-11 8:05 ` [PATCH v3 4/4] omap3isp: lane shifter support Michael Jones @ 2011-03-16 15:44 ` Sakari Ailus 2011-03-16 16:27 ` Laurent Pinchart 0 siblings, 1 reply; 17+ messages in thread From: Sakari Ailus @ 2011-03-16 15:44 UTC (permalink / raw) To: Michael Jones; +Cc: Laurent Pinchart, linux-media, Hans Verkuil Hi Michael, Thanks for the patch. I have some comments below. Michael Jones wrote: > To use the lane shifter, set different pixel formats at each end of > the link at the CCDC input. > > Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> > --- > drivers/media/video/omap3-isp/isp.c | 7 ++- > drivers/media/video/omap3-isp/isp.h | 5 +- > drivers/media/video/omap3-isp/ispccdc.c | 27 ++++++-- > drivers/media/video/omap3-isp/ispvideo.c | 108 ++++++++++++++++++++++++------ > drivers/media/video/omap3-isp/ispvideo.h | 3 + > 5 files changed, 120 insertions(+), 30 deletions(-) > > diff --git a/drivers/media/video/omap3-isp/isp.c b/drivers/media/video/omap3-isp/isp.c > index 08d90fe..866ce09 100644 > --- a/drivers/media/video/omap3-isp/isp.c > +++ b/drivers/media/video/omap3-isp/isp.c > @@ -285,7 +285,8 @@ static void isp_power_settings(struct isp_device *isp, int idle) > */ > void omap3isp_configure_bridge(struct isp_device *isp, > enum ccdc_input_entity input, > - const struct isp_parallel_platform_data *pdata) > + const struct isp_parallel_platform_data *pdata, > + int shift) This goes more or less directly to register, so what about u32? Definitely unsigned at least. > { > u32 ispctrl_val; > > @@ -298,9 +299,9 @@ void omap3isp_configure_bridge(struct isp_device *isp, > switch (input) { > case CCDC_INPUT_PARALLEL: > ispctrl_val |= ISPCTRL_PAR_SER_CLK_SEL_PARALLEL; > - ispctrl_val |= pdata->data_lane_shift << ISPCTRL_SHIFT_SHIFT; > ispctrl_val |= pdata->clk_pol << ISPCTRL_PAR_CLK_POL_SHIFT; > ispctrl_val |= pdata->bridge << ISPCTRL_PAR_BRIDGE_SHIFT; > + shift += pdata->data_lane_shift*2; > break; > > case CCDC_INPUT_CSI2A: > @@ -319,6 +320,8 @@ void omap3isp_configure_bridge(struct isp_device *isp, > return; > } > > + ispctrl_val |= ((shift/2) << ISPCTRL_SHIFT_SHIFT) & ISPCTRL_SHIFT_MASK; > + > ispctrl_val &= ~ISPCTRL_SYNC_DETECT_MASK; > ispctrl_val |= ISPCTRL_SYNC_DETECT_VSRISE; > > diff --git a/drivers/media/video/omap3-isp/isp.h b/drivers/media/video/omap3-isp/isp.h > index 21fa88b..6f0ff1a 100644 > --- a/drivers/media/video/omap3-isp/isp.h > +++ b/drivers/media/video/omap3-isp/isp.h > @@ -130,7 +130,6 @@ struct isp_reg { > > /** > * struct isp_parallel_platform_data - Parallel interface platform data > - * @width: Parallel bus width in bits (8, 10, 11 or 12) > * @data_lane_shift: Data lane shifter > * 0 - CAMEXT[13:0] -> CAM[13:0] > * 1 - CAMEXT[13:2] -> CAM[11:0] > @@ -144,7 +143,6 @@ struct isp_reg { > * ISPCTRL_PAR_BRIDGE_BENDIAN - Big endian > */ > struct isp_parallel_platform_data { > - unsigned int width; > unsigned int data_lane_shift:2; > unsigned int clk_pol:1; > unsigned int bridge:4; > @@ -322,7 +320,8 @@ int omap3isp_pipeline_set_stream(struct isp_pipeline *pipe, > enum isp_pipeline_stream_state state); > void omap3isp_configure_bridge(struct isp_device *isp, > enum ccdc_input_entity input, > - const struct isp_parallel_platform_data *pdata); > + const struct isp_parallel_platform_data *pdata, > + int shift); > > #define ISP_XCLK_NONE -1 > #define ISP_XCLK_A 0 > diff --git a/drivers/media/video/omap3-isp/ispccdc.c b/drivers/media/video/omap3-isp/ispccdc.c > index 23000b6..bbcf619 100644 > --- a/drivers/media/video/omap3-isp/ispccdc.c > +++ b/drivers/media/video/omap3-isp/ispccdc.c > @@ -1120,21 +1120,38 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc) > struct isp_parallel_platform_data *pdata = NULL; > struct v4l2_subdev *sensor; > struct v4l2_mbus_framefmt *format; > + const struct isp_format_info *fmt_info; > + struct v4l2_subdev_format fmt_src; > + unsigned int depth_out = 0; > + unsigned int depth_in = 0; > struct media_pad *pad; > unsigned long flags; > + unsigned int shift; > u32 syn_mode; > u32 ccdc_pattern; > > - if (ccdc->input == CCDC_INPUT_PARALLEL) { > - pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]); > - sensor = media_entity_to_v4l2_subdev(pad->entity); > + pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]); > + sensor = media_entity_to_v4l2_subdev(pad->entity); > + if (ccdc->input == CCDC_INPUT_PARALLEL) > pdata = &((struct isp_v4l2_subdevs_group *)sensor->host_priv) > ->bus.parallel; > + > + /* Compute shift value for lane shifter to configure the bridge. */ > + fmt_src.pad = pad->index; > + fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) { > + fmt_info = omap3isp_video_format_info(fmt_src.format.code); > + depth_in = fmt_info->bpp; > } > > - omap3isp_configure_bridge(isp, ccdc->input, pdata); > + fmt_info = omap3isp_video_format_info > + (isp->isp_ccdc.formats[CCDC_PAD_SINK].code); > + depth_out = fmt_info->bpp; > + > + shift = depth_in - depth_out; > + omap3isp_configure_bridge(isp, ccdc->input, pdata, shift); > > - ccdc->syncif.datsz = pdata ? pdata->width : 10; > + ccdc->syncif.datsz = depth_out; > ccdc_config_sync_if(ccdc, &ccdc->syncif); > > /* CCDC_PAD_SINK */ > diff --git a/drivers/media/video/omap3-isp/ispvideo.c b/drivers/media/video/omap3-isp/ispvideo.c > index 3c3b3c4..c4b9fe1 100644 > --- a/drivers/media/video/omap3-isp/ispvideo.c > +++ b/drivers/media/video/omap3-isp/ispvideo.c > @@ -47,41 +47,59 @@ > > 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_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8, > + V4L2_PIX_FMT_GREY, 8, }, > { V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y10_1X10, > - V4L2_MBUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10, 10, }, > + V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y8_1X8, > + V4L2_PIX_FMT_Y10, 10, }, > { V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y10_1X10, > - V4L2_MBUS_FMT_Y12_1X12, V4L2_PIX_FMT_Y12, 12, }, > + V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y8_1X8, > + V4L2_PIX_FMT_Y12, 12, }, > { V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8, > - V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8, }, > + V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8, > + V4L2_PIX_FMT_SBGGR8, 8, }, > { V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8, > - V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8, }, > + V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8, > + V4L2_PIX_FMT_SGBRG8, 8, }, > { V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8, > - V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8, }, > + V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8, > + V4L2_PIX_FMT_SGRBG8, 8, }, > { V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8, > - V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8, }, > + V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8, > + V4L2_PIX_FMT_SRGGB8, 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_SGRBG10_1X10, 0, > + V4L2_PIX_FMT_SGRBG10DPCM8, 8, }, > { V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10, > - V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10, 10, }, > + V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR8_1X8, > + V4L2_PIX_FMT_SBGGR10, 10, }, > { V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_MBUS_FMT_SGBRG10_1X10, > - V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10, 10, }, > + V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_MBUS_FMT_SGBRG8_1X8, > + V4L2_PIX_FMT_SGBRG10, 10, }, > { V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG10_1X10, > - V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10, 10, }, > + V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG8_1X8, > + V4L2_PIX_FMT_SGRBG10, 10, }, > { V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SRGGB10_1X10, > - V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10, 10, }, > + V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SRGGB8_1X8, > + V4L2_PIX_FMT_SRGGB10, 10, }, > { V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_MBUS_FMT_SBGGR10_1X10, > - V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SBGGR12, 12, }, > + V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_MBUS_FMT_SBGGR8_1X8, > + V4L2_PIX_FMT_SBGGR12, 12, }, > { V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_MBUS_FMT_SGBRG10_1X10, > - V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12, 12, }, > + V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_MBUS_FMT_SGBRG8_1X8, > + V4L2_PIX_FMT_SGBRG12, 12, }, > { V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_MBUS_FMT_SGRBG10_1X10, > - V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12, 12, }, > + V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_MBUS_FMT_SGRBG8_1X8, > + V4L2_PIX_FMT_SGRBG12, 12, }, > { V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_MBUS_FMT_SRGGB10_1X10, > - V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12, 12, }, > + V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_MBUS_FMT_SRGGB8_1X8, > + V4L2_PIX_FMT_SRGGB12, 12, }, > { V4L2_MBUS_FMT_UYVY8_1X16, V4L2_MBUS_FMT_UYVY8_1X16, > - V4L2_MBUS_FMT_UYVY8_1X16, V4L2_PIX_FMT_UYVY, 16, }, > + V4L2_MBUS_FMT_UYVY8_1X16, 0, > + V4L2_PIX_FMT_UYVY, 16, }, > { V4L2_MBUS_FMT_YUYV8_1X16, V4L2_MBUS_FMT_YUYV8_1X16, > - V4L2_MBUS_FMT_YUYV8_1X16, V4L2_PIX_FMT_YUYV, 16, }, > + V4L2_MBUS_FMT_YUYV8_1X16, 0, > + V4L2_PIX_FMT_YUYV, 16, }, > }; > > const struct isp_format_info * > @@ -98,6 +116,37 @@ omap3isp_video_format_info(enum v4l2_mbus_pixelcode code) > } > > /* > + * Decide whether desired output pixel code can be obtained with > + * the lane shifter by shifting the input pixel code. > + * @in: input pixelcode to shifter > + * @out: output pixelcode from shifter > + * @additional_shift: # of bits the sensor's LSB is offset from CAMEXT[0] > + * > + * return true if the combination is possible > + * return false otherwise > + */ > +static bool isp_video_is_shiftable(enum v4l2_mbus_pixelcode in, > + enum v4l2_mbus_pixelcode out, > + unsigned int additional_shift) > +{ > + const struct isp_format_info *in_info, *out_info; > + > + if (in == out) > + return true; > + > + in_info = omap3isp_video_format_info(in); > + out_info = omap3isp_video_format_info(out); > + > + if ((in_info->flavor == 0) || (out_info->flavor == 0)) > + return false; > + > + if (in_info->flavor != out_info->flavor) > + return false; > + > + return in_info->bpp - out_info->bpp + additional_shift <= 6; Currently there are no formats that would behave badly in this check? Perhaps it'd be good idea to take that into consideration. The shift that can be done is even. > +} > + > +/* > * isp_video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format > * @video: ISP video instance > * @mbus: v4l2_mbus_framefmt format (input) > @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe) > return -EPIPE; > > while (1) { > + unsigned int link_has_shifter; link_has_shifter is only used in one place. Would it be cleaner to test below if it's the CCDC? A comment there could be nice, too. > /* Retrieve the sink format */ > pad = &subdev->entity.pads[0]; > if (!(pad->flags & MEDIA_PAD_FL_SINK)) > @@ -275,6 +325,10 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe) > return -ENOSPC; > } > > + /* If sink pad is on CCDC, the link has the lane shifter > + * in the middle of it. */ > + link_has_shifter = subdev == &isp->isp_ccdc.subdev; > + > /* Retrieve the source format */ > pad = media_entity_remote_source(pad); > if (pad == NULL || > @@ -290,10 +344,24 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe) > return -EPIPE; > > /* Check if the two ends match */ > - if (fmt_source.format.code != fmt_sink.format.code || > - fmt_source.format.width != fmt_sink.format.width || > + if (fmt_source.format.width != fmt_sink.format.width || > fmt_source.format.height != fmt_sink.format.height) > return -EPIPE; > + > + if (link_has_shifter) { > + unsigned int parallel_shift = 0; > + if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) { > + struct isp_parallel_platform_data *pdata = > + &((struct isp_v4l2_subdevs_group *) > + subdev->host_priv)->bus.parallel; > + parallel_shift = pdata->data_lane_shift * 2; > + } > + if (!isp_video_is_shiftable(fmt_source.format.code, > + fmt_sink.format.code, > + parallel_shift)) > + return -EPIPE; > + } else if (fmt_source.format.code != fmt_sink.format.code) > + return -EPIPE; > } > > return 0; > diff --git a/drivers/media/video/omap3-isp/ispvideo.h b/drivers/media/video/omap3-isp/ispvideo.h > index 524a1ac..911bea6 100644 > --- a/drivers/media/video/omap3-isp/ispvideo.h > +++ b/drivers/media/video/omap3-isp/ispvideo.h > @@ -49,6 +49,8 @@ struct v4l2_pix_format; > * bits. Identical to @code if the format is 10 bits wide or less. > * @uncompressed: V4L2 media bus format code for the corresponding uncompressed > * format. Identical to @code if the format is not DPCM compressed. > + * @flavor: V4L2 media bus format code for the same pixel layout but > + * shifted to be 8 bits per pixel. =0 if format is not shiftable. > * @pixelformat: V4L2 pixel format FCC identifier > * @bpp: Bits per pixel > */ > @@ -56,6 +58,7 @@ struct isp_format_info { > enum v4l2_mbus_pixelcode code; > enum v4l2_mbus_pixelcode truncated; > enum v4l2_mbus_pixelcode uncompressed; > + enum v4l2_mbus_pixelcode flavor; > u32 pixelformat; > unsigned int bpp; > }; Best regards, -- Sakari Ailus sakari.ailus@maxwell.research.nokia.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/4] omap3isp: lane shifter support 2011-03-16 15:44 ` Sakari Ailus @ 2011-03-16 16:27 ` Laurent Pinchart 2011-03-16 17:08 ` Sakari Ailus 0 siblings, 1 reply; 17+ messages in thread From: Laurent Pinchart @ 2011-03-16 16:27 UTC (permalink / raw) To: Sakari Ailus; +Cc: Michael Jones, linux-media, Hans Verkuil Hi Sakari, Thanks for the comments. On Wednesday 16 March 2011 16:44:49 Sakari Ailus wrote: > Hi Michael, > > Thanks for the patch. I have some comments below. > > Michael Jones wrote: > > To use the lane shifter, set different pixel formats at each end of > > the link at the CCDC input. > > > > Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> > > --- > > > > drivers/media/video/omap3-isp/isp.c | 7 ++- > > drivers/media/video/omap3-isp/isp.h | 5 +- > > drivers/media/video/omap3-isp/ispccdc.c | 27 ++++++-- > > drivers/media/video/omap3-isp/ispvideo.c | 108 > > ++++++++++++++++++++++++------ drivers/media/video/omap3-isp/ispvideo.h > > | 3 + > > 5 files changed, 120 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/media/video/omap3-isp/isp.c > > b/drivers/media/video/omap3-isp/isp.c index 08d90fe..866ce09 100644 > > --- a/drivers/media/video/omap3-isp/isp.c > > +++ b/drivers/media/video/omap3-isp/isp.c > > @@ -285,7 +285,8 @@ static void isp_power_settings(struct isp_device > > *isp, int idle) > > > > */ > > > > void omap3isp_configure_bridge(struct isp_device *isp, > > > > enum ccdc_input_entity input, > > > > - const struct isp_parallel_platform_data *pdata) > > + const struct isp_parallel_platform_data *pdata, > > + int shift) > > This goes more or less directly to register, so what about u32? > Definitely unsigned at least. Agreed. [snip] > > @@ -98,6 +116,37 @@ omap3isp_video_format_info(enum v4l2_mbus_pixelcode > > code) > > > > } > > > > /* > > > > + * Decide whether desired output pixel code can be obtained with > > + * the lane shifter by shifting the input pixel code. > > + * @in: input pixelcode to shifter > > + * @out: output pixelcode from shifter > > + * @additional_shift: # of bits the sensor's LSB is offset from > > CAMEXT[0] + * > > + * return true if the combination is possible > > + * return false otherwise > > + */ > > +static bool isp_video_is_shiftable(enum v4l2_mbus_pixelcode in, > > + enum v4l2_mbus_pixelcode out, > > + unsigned int additional_shift) > > +{ > > + const struct isp_format_info *in_info, *out_info; > > + > > + if (in == out) > > + return true; > > + > > + in_info = omap3isp_video_format_info(in); > > + out_info = omap3isp_video_format_info(out); > > + > > + if ((in_info->flavor == 0) || (out_info->flavor == 0)) > > + return false; > > + > > + if (in_info->flavor != out_info->flavor) > > + return false; > > + > > + return in_info->bpp - out_info->bpp + additional_shift <= 6; > > Currently there are no formats that would behave badly in this check? > Perhaps it'd be good idea to take that into consideration. The shift > that can be done is even. I've asked Michael to remove the check because we have no misbehaving formats :-) Do you think we need to add a check back ? > > +} > > + > > +/* > > > > * isp_video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format > > * @video: ISP video instance > > * @mbus: v4l2_mbus_framefmt format (input) > > > > @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct > > isp_pipeline *pipe) > > > > return -EPIPE; > > > > while (1) { > > > > + unsigned int link_has_shifter; > > link_has_shifter is only used in one place. Would it be cleaner to test > below if it's the CCDC? A comment there could be nice, too. I would like that better as well, but between the line where link_has_shifter is set and the line where it is checked, the subdev variable changes so we can't just check subdev == &isp->isp_ccdc.subdev there. > > /* Retrieve the sink format */ > > pad = &subdev->entity.pads[0]; > > if (!(pad->flags & MEDIA_PAD_FL_SINK)) > > > > @@ -275,6 +325,10 @@ static int isp_video_validate_pipeline(struct > > isp_pipeline *pipe) > > > > return -ENOSPC; > > > > } > > > > + /* If sink pad is on CCDC, the link has the lane shifter > > + * in the middle of it. */ > > + link_has_shifter = subdev == &isp->isp_ccdc.subdev; > > + > > > > /* Retrieve the source format */ > > pad = media_entity_remote_source(pad); > > if (pad == NULL || > > > > @@ -290,10 +344,24 @@ static int isp_video_validate_pipeline(struct > > isp_pipeline *pipe) > > > > return -EPIPE; > > > > /* Check if the two ends match */ > > > > - if (fmt_source.format.code != fmt_sink.format.code || > > - fmt_source.format.width != fmt_sink.format.width || > > + if (fmt_source.format.width != fmt_sink.format.width || > > > > fmt_source.format.height != fmt_sink.format.height) > > > > return -EPIPE; > > > > + > > + if (link_has_shifter) { > > + unsigned int parallel_shift = 0; > > + if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) { > > + struct isp_parallel_platform_data *pdata = > > + &((struct isp_v4l2_subdevs_group *) > > + subdev->host_priv)->bus.parallel; > > + parallel_shift = pdata->data_lane_shift * 2; > > + } > > + if (!isp_video_is_shiftable(fmt_source.format.code, > > + fmt_sink.format.code, > > + parallel_shift)) > > + return -EPIPE; > > + } else if (fmt_source.format.code != fmt_sink.format.code) > > + return -EPIPE; > > > > } > > > > return 0; > > -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/4] omap3isp: lane shifter support 2011-03-16 16:27 ` Laurent Pinchart @ 2011-03-16 17:08 ` Sakari Ailus 2011-03-16 17:46 ` Laurent Pinchart 0 siblings, 1 reply; 17+ messages in thread From: Sakari Ailus @ 2011-03-16 17:08 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Michael Jones, linux-media, Hans Verkuil Laurent Pinchart wrote: > Hi Sakari, Hi Laurent and Michael! ... >>> + return in_info->bpp - out_info->bpp + additional_shift <= 6; >> >> Currently there are no formats that would behave badly in this check? >> Perhaps it'd be good idea to take that into consideration. The shift >> that can be done is even. > > I've asked Michael to remove the check because we have no misbehaving formats > :-) Do you think we need to add a check back ? I think it would be helpful in debugging if someone decides to attach a sensor which supports a shift of non-even bits (8 and 9 bits, for example). In any case an invalid configuration is possible in such case, and I don't think that should be allowed, should it? >>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct >>> isp_pipeline *pipe) >>> >>> return -EPIPE; >>> >>> while (1) { >>> >>> + unsigned int link_has_shifter; >> >> link_has_shifter is only used in one place. Would it be cleaner to test >> below if it's the CCDC? A comment there could be nice, too. > > I would like that better as well, but between the line where link_has_shifter > is set and the line where it is checked, the subdev variable changes so we > can't just check subdev == &isp->isp_ccdc.subdev there. That's definitely valid. I take my comment back. The variable could be called is_ccdc, though, since only the CCDC has that feature. No need to generalise. :-) Cheers, -- Sakari Ailus sakari.ailus@maxwell.research.nokia.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/4] omap3isp: lane shifter support 2011-03-16 17:08 ` Sakari Ailus @ 2011-03-16 17:46 ` Laurent Pinchart 2011-03-17 10:07 ` Michael Jones 0 siblings, 1 reply; 17+ messages in thread From: Laurent Pinchart @ 2011-03-16 17:46 UTC (permalink / raw) To: Sakari Ailus; +Cc: Michael Jones, linux-media, Hans Verkuil Hi Sakari, On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote: > Laurent Pinchart wrote: > > Hi Sakari, > >>> + return in_info->bpp - out_info->bpp + additional_shift <= 6; > >> > >> Currently there are no formats that would behave badly in this check? > >> Perhaps it'd be good idea to take that into consideration. The shift > >> that can be done is even. > > > > I've asked Michael to remove the check because we have no misbehaving > > formats > > > > :-) Do you think we need to add a check back ? > > I think it would be helpful in debugging if someone decides to attach a > sensor which supports a shift of non-even bits (8 and 9 bits, for > example). In any case an invalid configuration is possible in such case, > and I don't think that should be allowed, should it? I agree it shouldn't be allowed, but the ISP driver doesn't support non-even widths at the moment, so there's no big risk. There could be an issue when a non-even width is added to the driver if the developer forgets to update the shift code. Maybe a comment in ispvideo.c above the big formats array would help making sure this is not forgotten ? > >>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct > >>> isp_pipeline *pipe) > >>> > >>> return -EPIPE; > >>> > >>> while (1) { > >>> > >>> + unsigned int link_has_shifter; > >> > >> link_has_shifter is only used in one place. Would it be cleaner to test > >> below if it's the CCDC? A comment there could be nice, too. > > > > I would like that better as well, but between the line where > > link_has_shifter is set and the line where it is checked, the subdev > > variable changes so we can't just check subdev == &isp->isp_ccdc.subdev > > there. > > That's definitely valid. I take my comment back. The variable could be > called is_ccdc, though, since only the CCDC has that feature. No need to > generalise. :-) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/4] omap3isp: lane shifter support 2011-03-16 17:46 ` Laurent Pinchart @ 2011-03-17 10:07 ` Michael Jones 2011-03-17 11:04 ` Laurent Pinchart 0 siblings, 1 reply; 17+ messages in thread From: Michael Jones @ 2011-03-17 10:07 UTC (permalink / raw) To: Sakari Ailus; +Cc: Laurent Pinchart, linux-media, Hans Verkuil Hi Sakari, Thanks for the review. On 03/16/2011 06:46 PM, Laurent Pinchart wrote: > Hi Sakari, > > On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote: >> Laurent Pinchart wrote: >>> Hi Sakari, >>>>> + return in_info->bpp - out_info->bpp + additional_shift <= 6; >>>> >>>> Currently there are no formats that would behave badly in this check? >>>> Perhaps it'd be good idea to take that into consideration. The shift >>>> that can be done is even. >>> >>> I've asked Michael to remove the check because we have no misbehaving >>> formats >>> >>> :-) Do you think we need to add a check back ? >> >> I think it would be helpful in debugging if someone decides to attach a >> sensor which supports a shift of non-even bits (8 and 9 bits, for >> example). In any case an invalid configuration is possible in such case, >> and I don't think that should be allowed, should it? > > I agree it shouldn't be allowed, but the ISP driver doesn't support non-even > widths at the moment, so there's no big risk. There could be an issue when a > non-even width is added to the driver if the developer forgets to update the > shift code. Maybe a comment in ispvideo.c above the big formats array would > help making sure this is not forgotten ? I think now that additional_shift is also being considered which comes from the board file, it makes sense to reintroduce the check for an even shift. As Sakari points out, this would be helpful for debugging if someone tries using .data_lane_shift which is odd. > >>>>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct >>>>> isp_pipeline *pipe) >>>>> >>>>> return -EPIPE; >>>>> >>>>> while (1) { >>>>> >>>>> + unsigned int link_has_shifter; >>>> >>>> link_has_shifter is only used in one place. Would it be cleaner to test >>>> below if it's the CCDC? A comment there could be nice, too. >>> >>> I would like that better as well, but between the line where >>> link_has_shifter is set and the line where it is checked, the subdev >>> variable changes so we can't just check subdev == &isp->isp_ccdc.subdev >>> there. >> >> That's definitely valid. I take my comment back. The variable could be >> called is_ccdc, though, since only the CCDC has that feature. No need to >> generalise. :-) > But this is not a feature of the CCDC, the lane shifter is outside of the CCDC. Each 'while (1)' iteration handles 2 subdevs on each side of one link, so I think it makes sense for a particular iteration to say "this link has", especially when the subdev ptr changes values between the assignment of this var and its usage. "is_ccdc" is vague as to which side of the CCDC we're on. 'link_has_shifter' wasn't intended to be general, it was supposed to mean 'this_is_the_link_with_the_shifter'. If you want to be more specific where that is in the pipeline, maybe 'ccdc_sink_link'? If you just want it to sound less like "this is one of the links with a shifter" and more like "We've found _the_ link with _the_ shifter", it could just be 'shifter_link'. After we iron these two things out, are you guys ready to see v4? -Michael MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/4] omap3isp: lane shifter support 2011-03-17 10:07 ` Michael Jones @ 2011-03-17 11:04 ` Laurent Pinchart 2011-03-17 15:40 ` Sakari Ailus 0 siblings, 1 reply; 17+ messages in thread From: Laurent Pinchart @ 2011-03-17 11:04 UTC (permalink / raw) To: Michael Jones; +Cc: Sakari Ailus, linux-media, Hans Verkuil On Thursday 17 March 2011 11:07:40 Michael Jones wrote: > On 03/16/2011 06:46 PM, Laurent Pinchart wrote: > > On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote: > >> Laurent Pinchart wrote: > >>> Hi Sakari, > >>> > >>>>> + return in_info->bpp - out_info->bpp + additional_shift <= 6; > >>>> > >>>> Currently there are no formats that would behave badly in this check? > >>>> Perhaps it'd be good idea to take that into consideration. The shift > >>>> that can be done is even. > >>> > >>> I've asked Michael to remove the check because we have no misbehaving > >>> formats > >>> > >>> :-) Do you think we need to add a check back ? > >> > >> I think it would be helpful in debugging if someone decides to attach a > >> sensor which supports a shift of non-even bits (8 and 9 bits, for > >> example). In any case an invalid configuration is possible in such case, > >> and I don't think that should be allowed, should it? > > > > I agree it shouldn't be allowed, but the ISP driver doesn't support > > non-even widths at the moment, so there's no big risk. There could be an > > issue when a non-even width is added to the driver if the developer > > forgets to update the shift code. Maybe a comment in ispvideo.c above > > the big formats array would help making sure this is not forgotten ? > > I think now that additional_shift is also being considered which comes > from the board file, it makes sense to reintroduce the check for an even > shift. As Sakari points out, this would be helpful for debugging if > someone tries using .data_lane_shift which is odd. How should we handle such a broken .data_lane_shift value ? Always refuse to start streaming (maybe with a kernel log message) ? Or should we catch it in isp_register_entities() instead ? > >>>>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct > >>>>> isp_pipeline *pipe) > >>>>> > >>>>> return -EPIPE; > >>>>> > >>>>> while (1) { > >>>>> > >>>>> + unsigned int link_has_shifter; > >>>> > >>>> link_has_shifter is only used in one place. Would it be cleaner to > >>>> test below if it's the CCDC? A comment there could be nice, too. > >>> > >>> I would like that better as well, but between the line where > >>> link_has_shifter is set and the line where it is checked, the subdev > >>> variable changes so we can't just check subdev == &isp->isp_ccdc.subdev > >>> there. > >> > >> That's definitely valid. I take my comment back. The variable could be > >> called is_ccdc, though, since only the CCDC has that feature. No need to > >> generalise. :-) > > But this is not a feature of the CCDC, the lane shifter is outside of > the CCDC. Each 'while (1)' iteration handles 2 subdevs on each side of > one link, so I think it makes sense for a particular iteration to say > "this link has", especially when the subdev ptr changes values between > the assignment of this var and its usage. "is_ccdc" is vague as to > which side of the CCDC we're on. 'link_has_shifter' wasn't intended to > be general, it was supposed to mean 'this_is_the_link_with_the_shifter'. > If you want to be more specific where that is in the pipeline, maybe > 'ccdc_sink_link'? If you just want it to sound less like "this is one > of the links with a shifter" and more like "We've found _the_ link with > _the_ shifter", it could just be 'shifter_link'. shifter_link sounds good to me. > After we iron these two things out, are you guys ready to see v4? That's fine with me. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/4] omap3isp: lane shifter support 2011-03-17 11:04 ` Laurent Pinchart @ 2011-03-17 15:40 ` Sakari Ailus 2011-03-17 15:51 ` Michael Jones 0 siblings, 1 reply; 17+ messages in thread From: Sakari Ailus @ 2011-03-17 15:40 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Michael Jones, linux-media, Hans Verkuil Hi Laurent and Michael, Laurent Pinchart wrote: > On Thursday 17 March 2011 11:07:40 Michael Jones wrote: >> On 03/16/2011 06:46 PM, Laurent Pinchart wrote: >>> On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote: >>>> Laurent Pinchart wrote: >>>>> Hi Sakari, >>>>> >>>>>>> + return in_info->bpp - out_info->bpp + additional_shift <= 6; >>>>>> >>>>>> Currently there are no formats that would behave badly in this check? >>>>>> Perhaps it'd be good idea to take that into consideration. The shift >>>>>> that can be done is even. >>>>> >>>>> I've asked Michael to remove the check because we have no misbehaving >>>>> formats >>>>> >>>>> :-) Do you think we need to add a check back ? >>>> >>>> I think it would be helpful in debugging if someone decides to attach a >>>> sensor which supports a shift of non-even bits (8 and 9 bits, for >>>> example). In any case an invalid configuration is possible in such case, >>>> and I don't think that should be allowed, should it? >>> >>> I agree it shouldn't be allowed, but the ISP driver doesn't support >>> non-even widths at the moment, so there's no big risk. There could be an >>> issue when a non-even width is added to the driver if the developer >>> forgets to update the shift code. Maybe a comment in ispvideo.c above >>> the big formats array would help making sure this is not forgotten ? >> >> I think now that additional_shift is also being considered which comes >> from the board file, it makes sense to reintroduce the check for an even >> shift. As Sakari points out, this would be helpful for debugging if >> someone tries using .data_lane_shift which is odd. > > How should we handle such a broken .data_lane_shift value ? Always refuse to > start streaming (maybe with a kernel log message) ? Or should we catch it in > isp_register_entities() instead ? If I understand correctly it's not possible to shift odd bits in any case. It's a hardware limitation. I'd perhaps have just the appropriate register bits in the platform data so that leaves no room for accidental misconfiguration, but this is perhaps just too much work for not much gain. >>>>>>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct >>>>>>> isp_pipeline *pipe) >>>>>>> >>>>>>> return -EPIPE; >>>>>>> >>>>>>> while (1) { >>>>>>> >>>>>>> + unsigned int link_has_shifter; >>>>>> >>>>>> link_has_shifter is only used in one place. Would it be cleaner to >>>>>> test below if it's the CCDC? A comment there could be nice, too. >>>>> >>>>> I would like that better as well, but between the line where >>>>> link_has_shifter is set and the line where it is checked, the subdev >>>>> variable changes so we can't just check subdev == &isp->isp_ccdc.subdev >>>>> there. >>>> >>>> That's definitely valid. I take my comment back. The variable could be >>>> called is_ccdc, though, since only the CCDC has that feature. No need to >>>> generalise. :-) >> >> But this is not a feature of the CCDC, the lane shifter is outside of >> the CCDC. Each 'while (1)' iteration handles 2 subdevs on each side of >> one link, so I think it makes sense for a particular iteration to say >> "this link has", especially when the subdev ptr changes values between >> the assignment of this var and its usage. "is_ccdc" is vague as to >> which side of the CCDC we're on. 'link_has_shifter' wasn't intended to >> be general, it was supposed to mean 'this_is_the_link_with_the_shifter'. >> If you want to be more specific where that is in the pipeline, maybe >> 'ccdc_sink_link'? If you just want it to sound less like "this is one >> of the links with a shifter" and more like "We've found _the_ link with >> _the_ shifter", it could just be 'shifter_link'. > > shifter_link sounds good to me. I agree. >> After we iron these two things out, are you guys ready to see v4? > > That's fine with me. For me, too! Cheers, -- Sakari Ailus sakari.ailus@maxwell.research.nokia.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/4] omap3isp: lane shifter support 2011-03-17 15:40 ` Sakari Ailus @ 2011-03-17 15:51 ` Michael Jones 0 siblings, 0 replies; 17+ messages in thread From: Michael Jones @ 2011-03-17 15:51 UTC (permalink / raw) To: Sakari Ailus; +Cc: Laurent Pinchart, linux-media, Hans Verkuil On 03/17/2011 04:40 PM, Sakari Ailus wrote: > Hi Laurent and Michael, > > Laurent Pinchart wrote: >> On Thursday 17 March 2011 11:07:40 Michael Jones wrote: >>> On 03/16/2011 06:46 PM, Laurent Pinchart wrote: >>>> On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote: >>>>> Laurent Pinchart wrote: >>>>>> Hi Sakari, >>>>>> >>>>>>>> + return in_info->bpp - out_info->bpp + additional_shift <= 6; >>>>>>> >>>>>>> Currently there are no formats that would behave badly in this check? >>>>>>> Perhaps it'd be good idea to take that into consideration. The shift >>>>>>> that can be done is even. >>>>>> >>>>>> I've asked Michael to remove the check because we have no misbehaving >>>>>> formats >>>>>> >>>>>> :-) Do you think we need to add a check back ? >>>>> >>>>> I think it would be helpful in debugging if someone decides to attach a >>>>> sensor which supports a shift of non-even bits (8 and 9 bits, for >>>>> example). In any case an invalid configuration is possible in such case, >>>>> and I don't think that should be allowed, should it? >>>> >>>> I agree it shouldn't be allowed, but the ISP driver doesn't support >>>> non-even widths at the moment, so there's no big risk. There could be an >>>> issue when a non-even width is added to the driver if the developer >>>> forgets to update the shift code. Maybe a comment in ispvideo.c above >>>> the big formats array would help making sure this is not forgotten ? >>> >>> I think now that additional_shift is also being considered which comes >>> from the board file, it makes sense to reintroduce the check for an even >>> shift. As Sakari points out, this would be helpful for debugging if >>> someone tries using .data_lane_shift which is odd. >> >> How should we handle such a broken .data_lane_shift value ? Always refuse to >> start streaming (maybe with a kernel log message) ? Or should we catch it in >> isp_register_entities() instead ? > > If I understand correctly it's not possible to shift odd bits in any > case. It's a hardware limitation. > > I'd perhaps have just the appropriate register bits in the platform data > so that leaves no room for accidental misconfiguration, but this is > perhaps just too much work for not much gain. > Actually, that's the way .data_lane_shift was originally defined (0,1,2,3), and I left it that way to minimize confusion. I was mistaken above when I said that .data_lane_shift could sneak an odd shift to isp_video_is_shiftable(), because .data_lane_shift is multiplied by 2 before getting passed there. So I would like to leave this as is, and it sounds like we have a consensus on this. I'll submit v4 soon, then. thanks, Michael MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-03-17 15:51 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-11 8:05 [PATCH v3 0/4] OMAP3-ISP lane shifter support Michael Jones 2011-03-11 8:05 ` [PATCH v3 1/4] v4l: add V4L2_PIX_FMT_Y12 format Michael Jones 2011-03-11 8:56 ` Laurent Pinchart 2011-03-11 9:21 ` Antonio Ospite 2011-03-11 9:38 ` Michael Jones 2011-03-11 11:15 ` Antonio Ospite 2011-03-11 8:05 ` [PATCH v3 2/4] media: add 8-bit bayer formats and Y12 Michael Jones 2011-03-11 8:05 ` [PATCH v3 3/4] omap3isp: ccdc: support Y10/12, 8-bit bayer fmts Michael Jones 2011-03-11 8:05 ` [PATCH v3 4/4] omap3isp: lane shifter support Michael Jones 2011-03-16 15:44 ` Sakari Ailus 2011-03-16 16:27 ` Laurent Pinchart 2011-03-16 17:08 ` Sakari Ailus 2011-03-16 17:46 ` Laurent Pinchart 2011-03-17 10:07 ` Michael Jones 2011-03-17 11:04 ` Laurent Pinchart 2011-03-17 15:40 ` Sakari Ailus 2011-03-17 15:51 ` Michael Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox