[PATCH RFC v3 0/5] s5p-fimc: Add interleaved image data capture support

[PATCH RFC v3 0/5] s5p-fimc: Add interleaved image data capture support

[Sylwester Nawrocki]

Hi All,

This patch series adds device/vendor specific media bus pixel code section
and defines S5C73MX camera specific media bus pixel code, along with
corresponding fourcc.

The third patch adds support for MIPI-CSI2 Embedded Data capture in
Samsung S5P/Exynos MIPI-CSIS device. It depends on patch
"[PATCH RFC] V4L: Add s_rx_buffer subdev video operation".

The fourth patch extends s5p-fimc driver to allow it to support
2-planar V4L2_PIX_FMT_S5C_UYVY_JPG format. More details can be found
in the patch summary. The [get/set]_frame_desc subdev callback are
used only to retrieve from a sensor subdev required buffer size.
It depends on patch
"[PATCH RFC] V4L: Add get/set_frame_desc subdev callbacks"

The fifth patch adds [get/set]_frame_desc op handlers to the m5mols
driver as an example. I prepared also similar patch for S5C73M3
sensor where 2 frame description entries are used, but that driver
is not yet mainlined due to a few missing items in V4L2 required
to fully control it, so I didn't include that patch in this series.

Changes since v2:
 - addressed review comments on patches adding the media bus pixel code
   and the fourcc,
 - minor cleanup of patch 4/5.

This series with all dependencies can be found in git repository
git://git.infradead.org/users/kmpark/linux-samsung v4l-framedesc

(http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/v4l-framedesc)

Thanks,
Sylwester

Sylwester Nawrocki (5):
  V4L: Add V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8 media bus format
  V4L: Add V4L2_PIX_FMT_S5C_UYVY_JPG fourcc definition
  s5p-csis: Add support for non-image data packets capture
  s5p-fimc: Add support for V4L2_PIX_FMT_S5C_UYVY_JPG fourcc
  m5mols: Implement .get_frame_desc subdev callback

 Documentation/DocBook/media/v4l/compat.xml         |   4 +
 Documentation/DocBook/media/v4l/pixfmt.xml         |  28 +++++
 Documentation/DocBook/media/v4l/subdev-formats.xml |  44 +++++++
 drivers/media/i2c/m5mols/m5mols.h                  |   9 ++
 drivers/media/i2c/m5mols/m5mols_capture.c          |   3 +
 drivers/media/i2c/m5mols/m5mols_core.c             |  47 +++++++
 drivers/media/i2c/m5mols/m5mols_reg.h              |   1 +
 drivers/media/platform/s5p-fimc/fimc-capture.c     | 135 ++++++++++++++++++---
 drivers/media/platform/s5p-fimc/fimc-core.c        |  19 ++-
 drivers/media/platform/s5p-fimc/fimc-core.h        |  28 ++++-
 drivers/media/platform/s5p-fimc/fimc-reg.c         |  23 +++-
 drivers/media/platform/s5p-fimc/fimc-reg.h         |   3 +-
 drivers/media/platform/s5p-fimc/mipi-csis.c        |  59 ++++++++-
 include/linux/v4l2-mediabus.h                      |   5 +
 include/linux/videodev2.h                          |   1 +
 15 files changed, 376 insertions(+), 33 deletions(-)

--
1.7.11.3

[PATCH RFC v3 1/5] V4L: Add V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8 media bus format

[Sylwester Nawrocki]

This patch adds media bus pixel code for the interleaved JPEG/UYVY
image format used by S5C73MX Samsung cameras. This interleaved image
data is transferred on MIPI-CSI2 bus as User Defined Byte-based Data.

It also defines an experimental vendor and device specific media bus
formats section and adds related DocBook documentation.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/DocBook/media/v4l/compat.xml         |  4 ++
 Documentation/DocBook/media/v4l/subdev-formats.xml | 44 ++++++++++++++++++++++
 include/linux/v4l2-mediabus.h                      |  5 +++
 3 files changed, 53 insertions(+)

diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml
index 802c1ab..b12cddb 100644
--- a/Documentation/DocBook/media/v4l/compat.xml
+++ b/Documentation/DocBook/media/v4l/compat.xml
@@ -2612,6 +2612,10 @@ ioctls.</para>
         <listitem>
 	  <para>Exporting DMABUF files using &VIDIOC-EXPBUF; ioctl.</para>
         </listitem>
+        <listitem>
+	  <para>Vendor and device specific media bus pixel formats.
+	    <xref linkend="v4l2-mbus-vendor-spec-fmts" />.</para>
+        </listitem>
       </itemizedlist>
     </section>
 
diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml b/Documentation/DocBook/media/v4l/subdev-formats.xml
index 49c532e..a0a9364 100644
--- a/Documentation/DocBook/media/v4l/subdev-formats.xml
+++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
@@ -2565,5 +2565,49 @@
 	</tgroup>
       </table>
     </section>
+
+    <section id="v4l2-mbus-vendor-spec-fmts">
+      <title>Vendor and Device Specific Formats</title>
+
+      <note>
+	<title>Experimental</title>
+	<para>This is an <link linkend="experimental">experimental</link>
+interface and may change in the future.</para>
+      </note>
+
+      <para>This section lists complex data formats that are either vendor or
+	device specific.
+      </para>
+
+      <para>The following table lists the existing vendor and device specific
+	formats.</para>
+
+      <table pgwide="0" frame="none" id="v4l2-mbus-pixelcode-vendor-specific">
+	<title>Vendor and device specific formats</title>
+	<tgroup cols="3">
+	  <colspec colname="id" align="left" />
+	  <colspec colname="code" align="left"/>
+	  <colspec colname="remarks" align="left"/>
+	  <thead>
+	    <row>
+	      <entry>Identifier</entry>
+	      <entry>Code</entry>
+	      <entry>Comments</entry>
+	    </row>
+	  </thead>
+	  <tbody valign="top">
+	    <row id="V4L2-MBUS-FMT-S5C-UYVY-JPEG-1X8">
+	      <entry>V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8</entry>
+	      <entry>0x5001</entry>
+	      <entry>
+		Interleaved raw UYVY and JPEG image format with embedded
+		meta-data used by Samsung S3C73MX camera sensors.
+	      </entry>
+	    </row>
+	  </tbody>
+	</tgroup>
+      </table>
+    </section>
+
   </section>
 </section>
diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
index 5ea7f75..7d64e0e 100644
--- a/include/linux/v4l2-mediabus.h
+++ b/include/linux/v4l2-mediabus.h
@@ -92,6 +92,11 @@ enum v4l2_mbus_pixelcode {
 
 	/* JPEG compressed formats - next is 0x4002 */
 	V4L2_MBUS_FMT_JPEG_1X8 = 0x4001,
+
+	/* Vendor specific formats - next is 0x5002 */
+
+	/* S5C73M3 sensor specific interleaved UYVY and JPEG */
+	V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8 = 0x5001,
 };
 
 /**
-- 
1.7.11.3

[PATCH RFC v3 2/5] V4L: Add V4L2_PIX_FMT_S5C_UYVY_JPG fourcc definition

[Sylwester Nawrocki]

This patch adds definition of the Samsung S5C73M3 camera specific
image format. V4L2_PIX_FMT_S5C_UYVY_JPG is a two-planar format,
the  first plane contains interleaved UYVY and JPEG data followed
by meta-data. The second plane contains additional meta-data needed
for extracting JPEG and UYVY data stream from the first plane.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/DocBook/media/v4l/pixfmt.xml | 28 ++++++++++++++++++++++++++++
 include/linux/videodev2.h                  |  1 +
 2 files changed, 29 insertions(+)

diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml b/Documentation/DocBook/media/v4l/pixfmt.xml
index 1ddbfab..21284ba 100644
--- a/Documentation/DocBook/media/v4l/pixfmt.xml
+++ b/Documentation/DocBook/media/v4l/pixfmt.xml
@@ -996,6 +996,34 @@ the other bits are set to 0.</entry>
 	    <entry>Old 6-bit greyscale format. Only the most significant 6 bits of each byte are used,
 the other bits are set to 0.</entry>
 	  </row>
+	  <row id="V4L2-PIX-FMT-S5C-UYVY-JPG">
+	    <entry><constant>V4L2_PIX_FMT_S5C_UYVY_JPG</constant></entry>
+	    <entry>'S5CI'</entry>
+	    <entry>Two-planar format used by Samsung S5C73MX cameras. The
+first plane contains interleaved JPEG and UYVY image data, followed by meta data
+in form of an array of offsets to the UYVY data blocks. The actual pointer array
+follows immediately the interleaved JPEG/UYVY data, the number of entries in
+this array equals the height of the UYVY image. Each entry is a 4-byte unsigned
+integer in big endian order and it's an offset to a single pixel line of the
+UYVY image. The first plane can start either with JPEG or UYVY data chunk. The
+size of a single UYVY block equals the UYVY image's width multiplied by 2. The
+size of a JPEG chunk depends on the image and can vary with each line.
+<para>The second plane, at an offset of 4084 bytes, contains a 4-byte offset to
+the pointer array in the first plane. This offset is followed by a 4-byte value
+indicating size of the pointer array. All numbers in the second plane are also
+in big endian order. Remaining data in the first plane is undefined. The
+information in the second plane allows to easily find location of the pointer
+array, which can be different for each frame. The size of the pointer array is
+constant for given UYVY image height.</para>
+<para>In order to extract UYVY and JPEG frames an application can initially set
+a data pointer to the start of first plane and then add an offset from the first
+entry of the pointers table. Such a pointer indicates start of an UYVY image
+pixel line. Whole UYVY line can be copied to a separate buffer. These steps
+should be repeated for each line, i.e. the number of entries in the pointer
+array. Anything what's in between the UYVY lines is JPEG data and should be
+concatenated to form the JPEG stream. </para>
+</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 8d29bb2..6c82ff5 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -436,6 +436,7 @@ struct v4l2_pix_format {
 #define V4L2_PIX_FMT_KONICA420  v4l2_fourcc('K', 'O', 'N', 'I') /* YUV420 planar in blocks of 256 pixels */
 #define V4L2_PIX_FMT_JPGL	v4l2_fourcc('J', 'P', 'G', 'L') /* JPEG-Lite */
 #define V4L2_PIX_FMT_SE401      v4l2_fourcc('S', '4', '0', '1') /* se401 janggu compressed rgb */
+#define V4L2_PIX_FMT_S5C_UYVY_JPG v4l2_fourcc('S', '5', 'C', 'J') /* S5C73M3 interleaved UYVY/JPEG */
 
 /*
  *	F O R M A T   E N U M E R A T I O N
-- 
1.7.11.3

[PATCH RFC v3 3/5] s5p-csis: Add support for non-image data packets capture

[Sylwester Nawrocki]

MIPI-CSI has internal memory mapped buffers for the frame embedded
(non-image) data. There are two buffers, for even and odd frames which
need to be saved after an interrupt is raised. The packet data buffers
size is 4 KiB and there is no status register in the hardware where the
actual non-image data size can be read from. Hence the driver copies
whole packet data buffer into a buffer provided by the FIMC driver.
This will form a separate plane in the user buffer.

When FIMC DMA engine is stopped by the driver due the to user space
not keeping up with buffer de-queuing the MIPI-CSIS will still run,
however it must discard data which is not captured by FIMC. Which
frames are actually captured by MIPI-CSIS is determined by means of
the s_rx_buffer subdev callback. When it is not called after a single
embedded data frame has been captured and copied and before next
embedded data frame interrupt occurrs, subsequent embedded data frames
will be dropped.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-fimc/mipi-csis.c | 51 +++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/media/platform/s5p-fimc/mipi-csis.c b/drivers/media/platform/s5p-fimc/mipi-csis.c
index c1c5c86..306410f 100644
--- a/drivers/media/platform/s5p-fimc/mipi-csis.c
+++ b/drivers/media/platform/s5p-fimc/mipi-csis.c
@@ -98,6 +98,11 @@ MODULE_PARM_DESC(debug, "Debug level (0-2)");
 #define CSIS_MAX_PIX_WIDTH		0xffff
 #define CSIS_MAX_PIX_HEIGHT		0xffff
 
+/* Non-image packet data buffers */
+#define S5PCSIS_PKTDATA_ODD		0x2000
+#define S5PCSIS_PKTDATA_EVEN		0x3000
+#define S5PCSIS_PKTDATA_SIZE		SZ_4K
+
 enum {
 	CSIS_CLK_MUX,
 	CSIS_CLK_GATE,
@@ -144,6 +149,11 @@ static const struct s5pcsis_event s5pcsis_events[] = {
 };
 #define S5PCSIS_NUM_EVENTS ARRAY_SIZE(s5pcsis_events)
 
+struct csis_pktbuf {
+	u32 *data;
+	unsigned int len;
+};
+
 /**
  * struct csis_state - the driver's internal state data structure
  * @lock: mutex serializing the subdev and power management operations,
@@ -160,6 +170,7 @@ static const struct s5pcsis_event s5pcsis_events[] = {
  * @csis_fmt: current CSIS pixel format
  * @format: common media bus format for the source and sink pad
  * @slock: spinlock protecting structure members below
+ * @pkt_buf: the frame embedded (non-image) data buffer
  * @events: MIPI-CSIS event (error) counters
  */
 struct csis_state {
@@ -177,6 +188,7 @@ struct csis_state {
 	struct v4l2_mbus_framefmt format;
 
 	struct spinlock slock;
+	struct csis_pktbuf pkt_buf;
 	struct s5pcsis_event events[S5PCSIS_NUM_EVENTS];
 };
 
@@ -534,6 +546,21 @@ static int s5pcsis_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 	return 0;
 }
 
+static int s5pcsis_s_rx_buffer(struct v4l2_subdev *sd, void *buf,
+			       unsigned int *size)
+{
+	struct csis_state *state = sd_to_csis_state(sd);
+	unsigned long flags;
+
+	spin_lock_irqsave(&state->slock, flags);
+	*size = min_t(unsigned int, *size, S5PCSIS_PKTDATA_SIZE);
+	state->pkt_buf.data = buf;
+	state->pkt_buf.len = *size;
+	spin_unlock_irqrestore(&state->slock, flags);
+
+	return 0;
+}
+
 static int s5pcsis_log_status(struct v4l2_subdev *sd)
 {
 	struct csis_state *state = sd_to_csis_state(sd);
@@ -571,6 +598,7 @@ static struct v4l2_subdev_pad_ops s5pcsis_pad_ops = {
 };
 
 static struct v4l2_subdev_video_ops s5pcsis_video_ops = {
+	.s_rx_buffer = s5pcsis_s_rx_buffer,
 	.s_stream = s5pcsis_s_stream,
 };
 
@@ -580,16 +608,39 @@ static struct v4l2_subdev_ops s5pcsis_subdev_ops = {
 	.video = &s5pcsis_video_ops,
 };
 
+static void s5pcsis_pkt_copy(struct csis_state *state, u32 *buf,
+			     u32 offset, u32 size)
+{
+	int i;
+
+	for (i = 0; i < S5PCSIS_PKTDATA_SIZE; i += 4, buf++)
+		*buf = s5pcsis_read(state, offset + i);
+}
+
 static irqreturn_t s5pcsis_irq_handler(int irq, void *dev_id)
 {
 	struct csis_state *state = dev_id;
+	struct csis_pktbuf *pktbuf = &state->pkt_buf;
 	unsigned long flags;
 	u32 status;
 
 	status = s5pcsis_read(state, S5PCSIS_INTSRC);
+	s5pcsis_write(state, S5PCSIS_INTSRC, status);
 
 	spin_lock_irqsave(&state->slock, flags);
 
+	if ((status & S5PCSIS_INTSRC_NON_IMAGE_DATA) && pktbuf->data) {
+		u32 offset;
+
+		if (status & S5PCSIS_INTSRC_EVEN)
+			offset = S5PCSIS_PKTDATA_EVEN;
+		else
+			offset = S5PCSIS_PKTDATA_ODD;
+
+		s5pcsis_pkt_copy(state, pktbuf->data, offset, pktbuf->len);
+		pktbuf->data = NULL;
+	}
+
 	/* Update the event/error counters */
 	if ((status & S5PCSIS_INTSRC_ERRORS) || debug) {
 		int i;
-- 
1.7.11.3

[PATCH RFC v3 4/5] s5p-fimc: Add support for V4L2_PIX_FMT_S5C_UYVY_JPG fourcc

[Sylwester Nawrocki]

The V4L2_PIX_FMT_S5C_YUYV_JPG image formats consists of 2 planes, the
first containing interleaved JPEG/YUYV data and the second containing
meta data describing the interleaving method.

The image data is transferred with MIPI-CSI "User Defined Byte-Based
Data 1" type and is captured to memory by FIMC DMA engine.

The meta data is transferred using MIPI-CSI2 "Embedded 8-bit non Image
Data" and it is captured in the MIPI-CSI slave device and copied to
the bridge provided buffer.

To make sure the size of allocated buffers is correct for the subdevs
configuration when VIDIOC_STREAMON ioctl is invoked, an additional
check is added at the video pipeline validation function.

Flag FMT_FLAGS_COMPRESSED indicates the buffer size must be retrieved
from a sensor subdev.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-fimc/fimc-capture.c | 135 +++++++++++++++++++++----
 drivers/media/platform/s5p-fimc/fimc-core.c    |  19 +++-
 drivers/media/platform/s5p-fimc/fimc-core.h    |  28 ++++-
 drivers/media/platform/s5p-fimc/fimc-reg.c     |  23 ++++-
 drivers/media/platform/s5p-fimc/fimc-reg.h     |   3 +-
 drivers/media/platform/s5p-fimc/mipi-csis.c    |   8 +-
 6 files changed, 183 insertions(+), 33 deletions(-)

diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
index 8a0908b..304b6fd 100644
--- a/drivers/media/platform/s5p-fimc/fimc-capture.c
+++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
@@ -177,7 +177,9 @@ static int fimc_capture_config_update(struct fimc_ctx *ctx)
 
 void fimc_capture_irq_handler(struct fimc_dev *fimc, int deq_buf)
 {
+	struct v4l2_subdev *csis = fimc->pipeline.subdevs[IDX_CSIS];
 	struct fimc_vid_cap *cap = &fimc->vid_cap;
+	struct fimc_frame *f = &cap->ctx->d_frame;
 	struct fimc_vid_buffer *v_buf;
 	struct timeval *tv;
 	struct timespec ts;
@@ -216,6 +218,25 @@ void fimc_capture_irq_handler(struct fimc_dev *fimc, int deq_buf)
 		if (++cap->buf_index >= FIMC_MAX_OUT_BUFS)
 			cap->buf_index = 0;
 	}
+	/*
+	 * Set up a buffer at MIPI-CSIS if current image format
+	 * requires the frame embedded data capture.
+	 */
+	if (f->fmt->mdataplanes && !list_empty(&cap->active_buf_q)) {
+		unsigned int plane = ffs(f->fmt->mdataplanes) - 1;
+		unsigned int size = f->payload[plane];
+		s32 index = fimc_hw_get_frame_index(fimc);
+		void *vaddr;
+
+		list_for_each_entry(v_buf, &cap->active_buf_q, list) {
+			if (v_buf->index != index)
+				continue;
+			vaddr = vb2_plane_vaddr(&v_buf->vb, plane);
+			v4l2_subdev_call(csis, video, s_rx_buffer,
+					 vaddr, &size);
+			break;
+		}
+	}
 
 	if (cap->active_buf_cnt == 0) {
 		if (deq_buf)
@@ -351,6 +372,8 @@ static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
 		unsigned int size = (wh * fmt->depth[i]) / 8;
 		if (pixm)
 			sizes[i] = max(size, pixm->plane_fmt[i].sizeimage);
+		else if (fimc_fmt_is_user_defined(fmt->color))
+			sizes[i] = frame->payload[i];
 		else
 			sizes[i] = max_t(u32, size, frame->payload[i]);
 
@@ -611,10 +634,10 @@ static struct fimc_fmt *fimc_capture_try_format(struct fimc_ctx *ctx,
 	u32 mask = FMT_FLAGS_CAM;
 	struct fimc_fmt *ffmt;
 
-	/* Color conversion from/to JPEG is not supported */
+	/* Conversion from/to JPEG or User Defined format is not supported */
 	if (code && ctx->s_frame.fmt && pad == FIMC_SD_PAD_SOURCE &&
-	    fimc_fmt_is_jpeg(ctx->s_frame.fmt->color))
-		*code = V4L2_MBUS_FMT_JPEG_1X8;
+	    fimc_fmt_is_user_defined(ctx->s_frame.fmt->color))
+		*code = ctx->s_frame.fmt->mbus_code;
 
 	if (fourcc && *fourcc != V4L2_PIX_FMT_JPEG && pad != FIMC_SD_PAD_SINK)
 		mask |= FMT_FLAGS_M2M;
@@ -628,18 +651,19 @@ static struct fimc_fmt *fimc_capture_try_format(struct fimc_ctx *ctx,
 		*fourcc = ffmt->fourcc;
 
 	if (pad == FIMC_SD_PAD_SINK) {
-		max_w = fimc_fmt_is_jpeg(ffmt->color) ?
+		max_w = fimc_fmt_is_user_defined(ffmt->color) ?
 			pl->scaler_dis_w : pl->scaler_en_w;
 		/* Apply the camera input interface pixel constraints */
 		v4l_bound_align_image(width, max_t(u32, *width, 32), max_w, 4,
 				      height, max_t(u32, *height, 32),
 				      FIMC_CAMIF_MAX_HEIGHT,
-				      fimc_fmt_is_jpeg(ffmt->color) ? 3 : 1,
+				      fimc_fmt_is_user_defined(ffmt->color) ?
+				      3 : 1,
 				      0);
 		return ffmt;
 	}
 	/* Can't scale or crop in transparent (JPEG) transfer mode */
-	if (fimc_fmt_is_jpeg(ffmt->color)) {
+	if (fimc_fmt_is_user_defined(ffmt->color)) {
 		*width  = ctx->s_frame.f_width;
 		*height = ctx->s_frame.f_height;
 		return ffmt;
@@ -684,7 +708,7 @@ static void fimc_capture_try_selection(struct fimc_ctx *ctx,
 	u32 max_sc_h, max_sc_v;
 
 	/* In JPEG transparent transfer mode cropping is not supported */
-	if (fimc_fmt_is_jpeg(ctx->d_frame.fmt->color)) {
+	if (fimc_fmt_is_user_defined(ctx->d_frame.fmt->color)) {
 		r->width  = sink->f_width;
 		r->height = sink->f_height;
 		r->left   = r->top = 0;
@@ -847,6 +871,48 @@ static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
 	return 0;
 }
 
+/**
+ * fimc_get_sensor_frame_desc - query the sensor for media bus frame parameters
+ * @sensor: pointer to the sensor subdev
+ * @plane_fmt: provides plane sizes corresponding to the frame layout entries
+ * @try: true to set the frame parameters, false to query only
+ *
+ * This function is used by this driver only for compressed/blob data formats.
+ */
+static int fimc_get_sensor_frame_desc(struct v4l2_subdev *sensor,
+				      struct v4l2_plane_pix_format *plane_fmt,
+				      unsigned int num_planes, bool try)
+{
+	struct v4l2_mbus_frame_desc fd;
+	int i, ret;
+
+	for (i = 0; i < num_planes; i++)
+		fd.entry[i].length = plane_fmt[i].sizeimage;
+
+	if (try)
+		ret = v4l2_subdev_call(sensor, pad, set_frame_desc, 0, &fd);
+	else
+		ret = v4l2_subdev_call(sensor, pad, get_frame_desc, 0, &fd);
+
+	if (ret < 0)
+		return ret;
+
+	if (num_planes != fd.num_entries)
+		return -EINVAL;
+
+	for (i = 0; i < num_planes; i++)
+		plane_fmt[i].sizeimage = fd.entry[i].length;
+
+	if (fd.entry[0].length > FIMC_MAX_JPEG_BUF_SIZE) {
+		v4l2_err(sensor->v4l2_dev,  "Unsupported buffer size: %u\n",
+			 fd.entry[0].length);
+
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int fimc_cap_g_fmt_mplane(struct file *file, void *fh,
 				 struct v4l2_format *f)
 {
@@ -865,7 +931,7 @@ static int fimc_cap_try_fmt_mplane(struct file *file, void *fh,
 	struct v4l2_mbus_framefmt mf;
 	struct fimc_fmt *ffmt = NULL;
 
-	if (pix->pixelformat == V4L2_PIX_FMT_JPEG) {
+	if (fimc_jpeg_fourcc(pix->pixelformat)) {
 		fimc_capture_try_format(ctx, &pix->width, &pix->height,
 					NULL, &pix->pixelformat,
 					FIMC_SD_PAD_SINK);
@@ -879,25 +945,32 @@ static int fimc_cap_try_fmt_mplane(struct file *file, void *fh,
 		return -EINVAL;
 
 	if (!fimc->vid_cap.user_subdev_api) {
-		mf.width  = pix->width;
+		mf.width = pix->width;
 		mf.height = pix->height;
-		mf.code   = ffmt->mbus_code;
+		mf.code = ffmt->mbus_code;
 		fimc_md_graph_lock(fimc);
 		fimc_pipeline_try_format(ctx, &mf, &ffmt, false);
 		fimc_md_graph_unlock(fimc);
-
-		pix->width	 = mf.width;
-		pix->height	 = mf.height;
+		pix->width = mf.width;
+		pix->height = mf.height;
 		if (ffmt)
 			pix->pixelformat = ffmt->fourcc;
 	}
 
 	fimc_adjust_mplane_format(ffmt, pix->width, pix->height, pix);
+
+	if (ffmt->flags & FMT_FLAGS_COMPRESSED)
+		fimc_get_sensor_frame_desc(fimc->pipeline.subdevs[IDX_SENSOR],
+					pix->plane_fmt, ffmt->memplanes, true);
+
 	return 0;
 }
 
-static void fimc_capture_mark_jpeg_xfer(struct fimc_ctx *ctx, bool jpeg)
+static void fimc_capture_mark_jpeg_xfer(struct fimc_ctx *ctx,
+					enum fimc_color_fmt color)
 {
+	bool jpeg = fimc_fmt_is_user_defined(color);
+
 	ctx->scaler.enabled = !jpeg;
 	fimc_ctrls_activate(ctx, !jpeg);
 
@@ -920,7 +993,7 @@ static int fimc_capture_set_format(struct fimc_dev *fimc, struct v4l2_format *f)
 		return -EBUSY;
 
 	/* Pre-configure format at camera interface input, for JPEG only */
-	if (pix->pixelformat == V4L2_PIX_FMT_JPEG) {
+	if (fimc_jpeg_fourcc(pix->pixelformat)) {
 		fimc_capture_try_format(ctx, &pix->width, &pix->height,
 					NULL, &pix->pixelformat,
 					FIMC_SD_PAD_SINK);
@@ -953,7 +1026,16 @@ static int fimc_capture_set_format(struct fimc_dev *fimc, struct v4l2_format *f)
 	}
 
 	fimc_adjust_mplane_format(ff->fmt, pix->width, pix->height, pix);
-	for (i = 0; i < ff->fmt->colplanes; i++)
+
+	if (ff->fmt->flags & FMT_FLAGS_COMPRESSED) {
+		ret = fimc_get_sensor_frame_desc(fimc->pipeline.subdevs[IDX_SENSOR],
+					pix->plane_fmt, ff->fmt->memplanes,
+					true);
+		if (ret < 0)
+			return ret;
+	}
+
+	for (i = 0; i < ff->fmt->memplanes; i++)
 		ff->payload[i] = pix->plane_fmt[i].sizeimage;
 
 	set_frame_bounds(ff, pix->width, pix->height);
@@ -961,7 +1043,7 @@ static int fimc_capture_set_format(struct fimc_dev *fimc, struct v4l2_format *f)
 	if (!(ctx->state & FIMC_COMPOSE))
 		set_frame_crop(ff, 0, 0, pix->width, pix->height);
 
-	fimc_capture_mark_jpeg_xfer(ctx, fimc_fmt_is_jpeg(ff->fmt->color));
+	fimc_capture_mark_jpeg_xfer(ctx, ff->fmt->color);
 
 	/* Reset cropping and set format at the camera interface input */
 	if (!fimc->vid_cap.user_subdev_api) {
@@ -1063,6 +1145,23 @@ static int fimc_pipeline_validate(struct fimc_dev *fimc)
 		    src_fmt.format.height != sink_fmt.format.height ||
 		    src_fmt.format.code != sink_fmt.format.code)
 			return -EPIPE;
+
+		if (sd == fimc->pipeline.subdevs[IDX_SENSOR] &&
+		    fimc_user_defined_mbus_fmt(src_fmt.format.code)) {
+			struct v4l2_plane_pix_format plane_fmt[FIMC_MAX_PLANES];
+			struct fimc_frame *frame = &vid_cap->ctx->d_frame;
+			unsigned int i;
+
+			ret = fimc_get_sensor_frame_desc(sd, plane_fmt,
+							 frame->fmt->memplanes,
+							 false);
+			if (ret < 0)
+				return -EPIPE;
+
+			for (i = 0; i < frame->fmt->memplanes; i++)
+				if (frame->payload[i] < plane_fmt[i].sizeimage)
+					return -EPIPE;
+		}
 	}
 	return 0;
 }
@@ -1433,7 +1532,7 @@ static int fimc_subdev_set_fmt(struct v4l2_subdev *sd,
 	/* Update RGB Alpha control state and value range */
 	fimc_alpha_ctrl_update(ctx);
 
-	fimc_capture_mark_jpeg_xfer(ctx, fimc_fmt_is_jpeg(ffmt->color));
+	fimc_capture_mark_jpeg_xfer(ctx, ffmt->color);
 
 	ff = fmt->pad == FIMC_SD_PAD_SINK ?
 		&ctx->s_frame : &ctx->d_frame;
diff --git a/drivers/media/platform/s5p-fimc/fimc-core.c b/drivers/media/platform/s5p-fimc/fimc-core.c
index 93be260..b5e3a74 100644
--- a/drivers/media/platform/s5p-fimc/fimc-core.c
+++ b/drivers/media/platform/s5p-fimc/fimc-core.c
@@ -184,7 +184,17 @@ static struct fimc_fmt fimc_formats[] = {
 		.memplanes	= 1,
 		.colplanes	= 1,
 		.mbus_code	= V4L2_MBUS_FMT_JPEG_1X8,
-		.flags		= FMT_FLAGS_CAM,
+		.flags		= FMT_FLAGS_CAM | FMT_FLAGS_COMPRESSED,
+	}, {
+		.name		= "S5C73MX interleaved UYVY/JPEG",
+		.fourcc		= V4L2_PIX_FMT_S5C_UYVY_JPG,
+		.color		= FIMC_FMT_YUYV_JPEG,
+		.depth		= { 8 },
+		.memplanes	= 2,
+		.colplanes	= 1,
+		.mdataplanes	= 0x2, /* plane 1 holds frame meta data */
+		.mbus_code	= V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8,
+		.flags		= FMT_FLAGS_CAM | FMT_FLAGS_COMPRESSED,
 	},
 };
 
@@ -371,7 +381,7 @@ int fimc_prepare_addr(struct fimc_ctx *ctx, struct vb2_buffer *vb,
 		default:
 			return -EINVAL;
 		}
-	} else {
+	} else if (!frame->fmt->mdataplanes) {
 		if (frame->fmt->memplanes >= 2)
 			paddr->cb = vb2_dma_contig_plane_dma_addr(vb, 1);
 
@@ -698,6 +708,11 @@ int fimc_fill_format(struct fimc_frame *frame, struct v4l2_format *f)
 		if (frame->fmt->colplanes == 1) /* packed formats */
 			bpl = (bpl * frame->fmt->depth[0]) / 8;
 		pixm->plane_fmt[i].bytesperline = bpl;
+
+		if (frame->fmt->flags & FMT_FLAGS_COMPRESSED) {
+			pixm->plane_fmt[i].sizeimage = frame->payload[i];
+			continue;
+		}
 		pixm->plane_fmt[i].sizeimage = (frame->o_width *
 			frame->o_height * frame->fmt->depth[i]) / 8;
 	}
diff --git a/drivers/media/platform/s5p-fimc/fimc-core.h b/drivers/media/platform/s5p-fimc/fimc-core.h
index cd716ba..c0040d7 100644
--- a/drivers/media/platform/s5p-fimc/fimc-core.h
+++ b/drivers/media/platform/s5p-fimc/fimc-core.h
@@ -40,6 +40,8 @@
 #define SCALER_MAX_VRATIO	64
 #define DMA_MIN_SIZE		8
 #define FIMC_CAMIF_MAX_HEIGHT	0x2000
+#define FIMC_MAX_JPEG_BUF_SIZE	(10 * SZ_1M)
+#define FIMC_MAX_PLANES		3
 
 /* indices to the clocks array */
 enum {
@@ -83,7 +85,7 @@ enum fimc_datapath {
 };
 
 enum fimc_color_fmt {
-	FIMC_FMT_RGB444 = 0x10,
+	FIMC_FMT_RGB444	= 0x10,
 	FIMC_FMT_RGB555,
 	FIMC_FMT_RGB565,
 	FIMC_FMT_RGB666,
@@ -95,14 +97,15 @@ enum fimc_color_fmt {
 	FIMC_FMT_CBYCRY422,
 	FIMC_FMT_CRYCBY422,
 	FIMC_FMT_YCBCR444_LOCAL,
-	FIMC_FMT_JPEG = 0x40,
-	FIMC_FMT_RAW8 = 0x80,
+	FIMC_FMT_RAW8 = 0x40,
 	FIMC_FMT_RAW10,
 	FIMC_FMT_RAW12,
+	FIMC_FMT_JPEG = 0x80,
+	FIMC_FMT_YUYV_JPEG = 0x100,
 };
 
+#define fimc_fmt_is_user_defined(x) (!!((x) & 0x180))
 #define fimc_fmt_is_rgb(x) (!!((x) & 0x10))
-#define fimc_fmt_is_jpeg(x) (!!((x) & 0x40))
 
 #define IS_M2M(__strt) ((__strt) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE || \
 			__strt == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
@@ -139,6 +142,7 @@ enum fimc_color_fmt {
  * @memplanes: number of physically non-contiguous data planes
  * @colplanes: number of physically contiguous data planes
  * @depth: per plane driver's private 'number of bits per pixel'
+ * @mdataplanes: bitmask indicating meta data plane(s), (1 << plane_no)
  * @flags: flags indicating which operation mode format applies to
  */
 struct fimc_fmt {
@@ -149,12 +153,14 @@ struct fimc_fmt {
 	u16	memplanes;
 	u16	colplanes;
 	u8	depth[VIDEO_MAX_PLANES];
+	u16	mdataplanes;
 	u16	flags;
 #define FMT_FLAGS_CAM		(1 << 0)
 #define FMT_FLAGS_M2M_IN	(1 << 1)
 #define FMT_FLAGS_M2M_OUT	(1 << 2)
 #define FMT_FLAGS_M2M		(1 << 1 | 1 << 2)
 #define FMT_HAS_ALPHA		(1 << 3)
+#define FMT_FLAGS_COMPRESSED	(1 << 4)
 };
 
 /**
@@ -272,7 +278,7 @@ struct fimc_frame {
 	u32	offs_v;
 	u32	width;
 	u32	height;
-	unsigned long		payload[VIDEO_MAX_PLANES];
+	unsigned int		payload[VIDEO_MAX_PLANES];
 	struct fimc_addr	paddr;
 	struct fimc_dma_offset	dma_offset;
 	struct fimc_fmt		*fmt;
@@ -577,6 +583,18 @@ static inline int tiled_fmt(struct fimc_fmt *fmt)
 	return fmt->fourcc == V4L2_PIX_FMT_NV12MT;
 }
 
+static inline bool fimc_jpeg_fourcc(u32 pixelformat)
+{
+	return (pixelformat == V4L2_PIX_FMT_JPEG ||
+		pixelformat == V4L2_PIX_FMT_S5C_UYVY_JPG);
+}
+
+static inline bool fimc_user_defined_mbus_fmt(u32 code)
+{
+	return (code == V4L2_MBUS_FMT_JPEG_1X8 ||
+		code == V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8);
+}
+
 /* Return the alpha component bit mask */
 static inline int fimc_get_alpha_mask(struct fimc_fmt *fmt)
 {
diff --git a/drivers/media/platform/s5p-fimc/fimc-reg.c b/drivers/media/platform/s5p-fimc/fimc-reg.c
index 783408f..2c9d0c0 100644
--- a/drivers/media/platform/s5p-fimc/fimc-reg.c
+++ b/drivers/media/platform/s5p-fimc/fimc-reg.c
@@ -625,7 +625,7 @@ int fimc_hw_set_camera_source(struct fimc_dev *fimc,
 				cfg |= FIMC_REG_CISRCFMT_ITU601_16BIT;
 		} /* else defaults to ITU-R BT.656 8-bit */
 	} else if (cam->bus_type == FIMC_MIPI_CSI2) {
-		if (fimc_fmt_is_jpeg(f->fmt->color))
+		if (fimc_fmt_is_user_defined(f->fmt->color))
 			cfg |= FIMC_REG_CISRCFMT_ITU601_8BIT;
 	}
 
@@ -680,6 +680,7 @@ int fimc_hw_set_camera_type(struct fimc_dev *fimc,
 			tmp = FIMC_REG_CSIIMGFMT_YCBCR422_8BIT;
 			break;
 		case V4L2_MBUS_FMT_JPEG_1X8:
+		case V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8:
 			tmp = FIMC_REG_CSIIMGFMT_USER(1);
 			cfg |= FIMC_REG_CIGCTRL_CAM_JPEG;
 			break;
@@ -744,13 +745,13 @@ void fimc_hw_dis_capture(struct fimc_dev *dev)
 }
 
 /* Return an index to the buffer actually being written. */
-u32 fimc_hw_get_frame_index(struct fimc_dev *dev)
+s32 fimc_hw_get_frame_index(struct fimc_dev *dev)
 {
-	u32 reg;
+	s32 reg;
 
 	if (dev->variant->has_cistatus2) {
-		reg = readl(dev->regs + FIMC_REG_CISTATUS2) & 0x3F;
-		return reg > 0 ? --reg : reg;
+		reg = readl(dev->regs + FIMC_REG_CISTATUS2) & 0x3f;
+		return reg - 1;
 	}
 
 	reg = readl(dev->regs + FIMC_REG_CISTATUS);
@@ -759,6 +760,18 @@ u32 fimc_hw_get_frame_index(struct fimc_dev *dev)
 		FIMC_REG_CISTATUS_FRAMECNT_SHIFT;
 }
 
+/* Return an index to the buffer being written previously. */
+s32 fimc_hw_get_prev_frame_index(struct fimc_dev *dev)
+{
+	s32 reg;
+
+	if (!dev->variant->has_cistatus2)
+		return -1;
+
+	reg = readl(dev->regs + FIMC_REG_CISTATUS2);
+	return ((reg >> 7) & 0x3f) - 1;
+}
+
 /* Locking: the caller holds fimc->slock */
 void fimc_activate_capture(struct fimc_ctx *ctx)
 {
diff --git a/drivers/media/platform/s5p-fimc/fimc-reg.h b/drivers/media/platform/s5p-fimc/fimc-reg.h
index 579ac8a..b6abfc7 100644
--- a/drivers/media/platform/s5p-fimc/fimc-reg.h
+++ b/drivers/media/platform/s5p-fimc/fimc-reg.h
@@ -307,7 +307,8 @@ void fimc_hw_clear_irq(struct fimc_dev *dev);
 void fimc_hw_enable_scaler(struct fimc_dev *dev, bool on);
 void fimc_hw_activate_input_dma(struct fimc_dev *dev, bool on);
 void fimc_hw_dis_capture(struct fimc_dev *dev);
-u32 fimc_hw_get_frame_index(struct fimc_dev *dev);
+s32 fimc_hw_get_frame_index(struct fimc_dev *dev);
+s32 fimc_hw_get_prev_frame_index(struct fimc_dev *dev);
 void fimc_activate_capture(struct fimc_ctx *ctx);
 void fimc_deactivate_capture(struct fimc_dev *fimc);
 
diff --git a/drivers/media/platform/s5p-fimc/mipi-csis.c b/drivers/media/platform/s5p-fimc/mipi-csis.c
index 306410f..384a5df 100644
--- a/drivers/media/platform/s5p-fimc/mipi-csis.c
+++ b/drivers/media/platform/s5p-fimc/mipi-csis.c
@@ -216,7 +216,11 @@ static const struct csis_pix_format s5pcsis_formats[] = {
 		.code = V4L2_MBUS_FMT_JPEG_1X8,
 		.fmt_reg = S5PCSIS_CFG_FMT_USER(1),
 		.data_alignment = 32,
-	},
+	}, {
+		.code = V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8,
+		.fmt_reg = S5PCSIS_CFG_FMT_USER(1),
+		.data_alignment = 32,
+	}
 };
 
 #define s5pcsis_write(__csis, __r, __v) writel(__v, __csis->regs + __r)
@@ -280,7 +284,7 @@ static void __s5pcsis_set_format(struct csis_state *state)
 	struct v4l2_mbus_framefmt *mf = &state->format;
 	u32 val;
 
-	v4l2_dbg(1, debug, &state->sd, "fmt: %d, %d x %d\n",
+	v4l2_dbg(1, debug, &state->sd, "fmt: %#x, %d x %d\n",
 		 mf->code, mf->width, mf->height);
 
 	/* Color format */
-- 
1.7.11.3

[PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Sylwester Nawrocki]

.get_frame_desc can be used by host interface driver to query
properties of captured frames, e.g. required memory buffer size.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/i2c/m5mols/m5mols.h         |  9 ++++++
 drivers/media/i2c/m5mols/m5mols_capture.c |  3 ++
 drivers/media/i2c/m5mols/m5mols_core.c    | 47 +++++++++++++++++++++++++++++++
 drivers/media/i2c/m5mols/m5mols_reg.h     |  1 +
 4 files changed, 60 insertions(+)

diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
index 15d3a4f..de3b755 100644
--- a/drivers/media/i2c/m5mols/m5mols.h
+++ b/drivers/media/i2c/m5mols/m5mols.h
@@ -19,6 +19,13 @@
 #include <media/v4l2-subdev.h>
 #include "m5mols_reg.h"
 
+
+/* An amount of data transmitted in addition to the value
+ * determined by CAPP_JPEG_SIZE_MAX register.
+ */
+#define M5MOLS_JPEG_TAGS_SIZE		0x20000
+#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
+
 extern int m5mols_debug;
 
 enum m5mols_restype {
@@ -67,12 +74,14 @@ struct m5mols_exif {
 /**
  * struct m5mols_capture - Structure for the capture capability
  * @exif: EXIF information
+ * @buf_size: internal JPEG frame buffer size, in bytes
  * @main: size in bytes of the main image
  * @thumb: size in bytes of the thumb image, if it was accompanied
  * @total: total size in bytes of the produced image
  */
 struct m5mols_capture {
 	struct m5mols_exif exif;
+	unsigned int buf_size;
 	u32 main;
 	u32 thumb;
 	u32 total;
diff --git a/drivers/media/i2c/m5mols/m5mols_capture.c b/drivers/media/i2c/m5mols/m5mols_capture.c
index cb243bd..ab34cce 100644
--- a/drivers/media/i2c/m5mols/m5mols_capture.c
+++ b/drivers/media/i2c/m5mols/m5mols_capture.c
@@ -105,6 +105,7 @@ static int m5mols_capture_info(struct m5mols_info *info)
 
 int m5mols_start_capture(struct m5mols_info *info)
 {
+	unsigned int framesize = info->cap.buf_size - M5MOLS_JPEG_TAGS_SIZE;
 	struct v4l2_subdev *sd = &info->sd;
 	int ret;
 
@@ -121,6 +122,8 @@ int m5mols_start_capture(struct m5mols_info *info)
 	if (!ret)
 		ret = m5mols_write(sd, CAPP_MAIN_IMAGE_SIZE, info->resolution);
 	if (!ret)
+		ret = m5mols_write(sd, CAPP_JPEG_SIZE_MAX, framesize);
+	if (!ret)
 		ret = m5mols_set_mode(info, REG_CAPTURE);
 	if (!ret)
 		/* Wait until a frame is captured to ISP internal memory */
diff --git a/drivers/media/i2c/m5mols/m5mols_core.c b/drivers/media/i2c/m5mols/m5mols_core.c
index 933014f..c780689 100644
--- a/drivers/media/i2c/m5mols/m5mols_core.c
+++ b/drivers/media/i2c/m5mols/m5mols_core.c
@@ -599,6 +599,51 @@ static int m5mols_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 	return ret;
 }
 
+static int m5mols_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				 struct v4l2_mbus_frame_desc *fd)
+{
+	struct m5mols_info *info = to_m5mols(sd);
+
+	if (pad != 0 || fd == NULL)
+		return -EINVAL;
+
+	mutex_lock(&info->lock);
+	/*
+	 * .get_frame_desc is only used for compressed formats,
+	 * thus we always return the capture frame parameters here.
+	 */
+	fd->entry[0].length = info->cap.buf_size;
+	fd->entry[0].pixelcode = info->ffmt[M5MOLS_RESTYPE_CAPTURE].code;
+	mutex_unlock(&info->lock);
+
+	fd->entry[0].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
+	fd->num_entries = 1;
+
+	return 0;
+}
+
+static int m5mols_set_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				 struct v4l2_mbus_frame_desc *fd)
+{
+	struct m5mols_info *info = to_m5mols(sd);
+	struct v4l2_mbus_framefmt *mf = &info->ffmt[M5MOLS_RESTYPE_CAPTURE];
+
+	if (pad != 0 || fd == NULL)
+		return -EINVAL;
+
+	fd->entry[0].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
+	fd->num_entries = 1;
+	fd->entry[0].length = clamp_t(u32, fd->entry[0].length,
+				      mf->width * mf->height,
+				      M5MOLS_MAIN_JPEG_SIZE_MAX);
+	mutex_lock(&info->lock);
+	info->cap.buf_size = fd->entry[0].length;
+	mutex_unlock(&info->lock);
+
+	return 0;
+}
+
+
 static int m5mols_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_fh *fh,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -615,6 +660,8 @@ static struct v4l2_subdev_pad_ops m5mols_pad_ops = {
 	.enum_mbus_code	= m5mols_enum_mbus_code,
 	.get_fmt	= m5mols_get_fmt,
 	.set_fmt	= m5mols_set_fmt,
+	.get_frame_desc	= m5mols_get_frame_desc,
+	.set_frame_desc	= m5mols_set_frame_desc,
 };
 
 /**
diff --git a/drivers/media/i2c/m5mols/m5mols_reg.h b/drivers/media/i2c/m5mols/m5mols_reg.h
index 14d4be7..58d8027 100644
--- a/drivers/media/i2c/m5mols/m5mols_reg.h
+++ b/drivers/media/i2c/m5mols/m5mols_reg.h
@@ -310,6 +310,7 @@
 #define REG_JPEG		0x10
 
 #define CAPP_MAIN_IMAGE_SIZE	I2C_REG(CAT_CAPT_PARM, 0x01, 1)
+#define CAPP_JPEG_SIZE_MAX	I2C_REG(CAT_CAPT_PARM, 0x0f, 4)
 #define CAPP_JPEG_RATIO		I2C_REG(CAT_CAPT_PARM, 0x17, 1)
 
 #define CAPP_MCC_MODE		I2C_REG(CAT_CAPT_PARM, 0x1d, 1)
-- 
1.7.11.3

Re: [PATCH RFC v3 2/5] V4L: Add V4L2_PIX_FMT_S5C_UYVY_JPG fourcc definition

[Laurent Pinchart]

Hi Sylwester,

Thank you for the patch.

On Wednesday 26 September 2012 17:54:10 Sylwester Nawrocki wrote:
> This patch adds definition of the Samsung S5C73M3 camera specific
> image format. V4L2_PIX_FMT_S5C_UYVY_JPG is a two-planar format,
> the  first plane contains interleaved UYVY and JPEG data followed
> by meta-data. The second plane contains additional meta-data needed
> for extracting JPEG and UYVY data stream from the first plane.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  Documentation/DocBook/media/v4l/pixfmt.xml | 28 +++++++++++++++++++++++++++
>  include/linux/videodev2.h                  |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml
> b/Documentation/DocBook/media/v4l/pixfmt.xml index 1ddbfab..21284ba 100644
> --- a/Documentation/DocBook/media/v4l/pixfmt.xml
> +++ b/Documentation/DocBook/media/v4l/pixfmt.xml
> @@ -996,6 +996,34 @@ the other bits are set to 0.</entry>
>  	    <entry>Old 6-bit greyscale format. Only the most significant 6 bits 
of
> each byte are used, the other bits are set to 0.</entry>
>  	  </row>
> +	  <row id="V4L2-PIX-FMT-S5C-UYVY-JPG">
> +	    <entry><constant>V4L2_PIX_FMT_S5C_UYVY_JPG</constant></entry>
> +	    <entry>'S5CI'</entry>
> +	    <entry>Two-planar format used by Samsung S5C73MX cameras. The
> +first plane contains interleaved JPEG and UYVY image data, followed by meta
> data in form of an array of offsets to the UYVY data blocks. The actual
> pointer array follows immediately the interleaved JPEG/UYVY data, the number
> of entries in this array equals the height of the UYVY image. Each entry is
> a 4-byte unsigned integer in big endian order and it's an offset to a single
> pixel line of the UYVY image. The first plane can start either with JPEG or
> UYVY data chunk. The size of a single UYVY block equals the UYVY image's
> width multiplied by 2. The size of a JPEG chunk depends on the image and can
> vary with each line.
> +<para>The second plane, at an offset of 4084 bytes, contains a 4-byte
> offset to the pointer array in the first plane. This offset is followed by a
> 4-byte value indicating size of the pointer array. All numbers in the second
> plane are also in big endian order. Remaining data in the first plane is
> undefined.

Do you mean "remaining data in the second plane is undefined." ?

Can it still be useful for some applications, or is it complete garbage ?

> The information in the second plane allows to easily find location of the
> pointer array, which can be different for each frame. The size of the
> pointer array is +constant for given UYVY image height.</para>
> +<para>In order to extract UYVY and JPEG frames an application can initially
> set a data pointer to the start of first plane and then add an offset from
> the first entry of the pointers table. Such a pointer indicates start of an
> UYVY image pixel line. Whole UYVY line can be copied to a separate buffer.
> These steps should be repeated for each line, i.e. the number of entries in
> the pointer array. Anything what's in between the UYVY lines is JPEG data
> and should be concatenated to form the JPEG stream. </para>
> +</entry>
> +	  </row>
>  	</tbody>
>        </tgroup>
>      </table>
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 8d29bb2..6c82ff5 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -436,6 +436,7 @@ struct v4l2_pix_format {
>  #define V4L2_PIX_FMT_KONICA420  v4l2_fourcc('K', 'O', 'N', 'I') /* YUV420
> planar in blocks of 256 pixels */ #define
> V4L2_PIX_FMT_JPGL	v4l2_fourcc('J', 'P', 'G', 'L') /* JPEG-Lite */ #define
> V4L2_PIX_FMT_SE401      v4l2_fourcc('S', '4', '0', '1') /* se401 janggu
> compressed rgb */ +#define V4L2_PIX_FMT_S5C_UYVY_JPG v4l2_fourcc('S', '5',
> 'C', 'J') /* S5C73M3 interleaved UYVY/JPEG */
> 
>  /*
>   *	F O R M A T   E N U M E R A T I O N
-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC v3 2/5] V4L: Add V4L2_PIX_FMT_S5C_UYVY_JPG fourcc definition

[Laurent Pinchart]

Hi Sylwester,

On Thursday 27 September 2012 19:18:13 Sylwester Nawrocki wrote:
> On 09/27/2012 01:10 PM, Laurent Pinchart wrote:
> > On Wednesday 26 September 2012 17:54:10 Sylwester Nawrocki wrote:
> >> This patch adds definition of the Samsung S5C73M3 camera specific
> >> image format. V4L2_PIX_FMT_S5C_UYVY_JPG is a two-planar format,
> >> the  first plane contains interleaved UYVY and JPEG data followed
> >> by meta-data. The second plane contains additional meta-data needed
> >> for extracting JPEG and UYVY data stream from the first plane.
> >> 
> >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >> ---
> >> 
> >>   Documentation/DocBook/media/v4l/pixfmt.xml | 28 +++++++++++++++++++++++
> >>   include/linux/videodev2.h                  |  1 +
> >>   2 files changed, 29 insertions(+)
> >> 
> >> diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml
> >> b/Documentation/DocBook/media/v4l/pixfmt.xml index 1ddbfab..21284ba
> >> 100644
> >> --- a/Documentation/DocBook/media/v4l/pixfmt.xml
> >> +++ b/Documentation/DocBook/media/v4l/pixfmt.xml
> >> @@ -996,6 +996,34 @@ the other bits are set to 0.</entry>
> >> 
> >>   	<entry>Old 6-bit greyscale format. Only the most significant 6 bits
> > 
> > of
> > 
> >> each byte are used, the other bits are set to 0.</entry>
> >> 
> >>   	</row>
> >> 
> >> +	<row id="V4L2-PIX-FMT-S5C-UYVY-JPG">
> >> +	<entry><constant>V4L2_PIX_FMT_S5C_UYVY_JPG</constant></entry>
> >> +	<entry>'S5CI'</entry>
> >> +	<entry>Two-planar format used by Samsung S5C73MX cameras. The
> >> +first plane contains interleaved JPEG and UYVY image data, followed by
> >> meta data in form of an array of offsets to the UYVY data blocks. The
> >> actual pointer array follows immediately the interleaved JPEG/UYVY data,
> >> the number of entries in this array equals the height of the UYVY image.
> >> Each entry is a 4-byte unsigned integer in big endian order and it's an
> >> offset to a single pixel line of the UYVY image. The first plane can
> >> start either with JPEG or UYVY data chunk. The size of a single UYVY
> >> block equals the UYVY image's width multiplied by 2. The size of a JPEG
> >> chunk depends on the image and can vary with each line.
> >> +<para>The second plane, at an offset of 4084 bytes, contains a 4-byte
> >> offset to the pointer array in the first plane. This offset is followed
> >> by a 4-byte value indicating size of the pointer array. All numbers in
> >> the second plane are also in big endian order. Remaining data in the
> >> first plane is undefined.
> > 
> > Do you mean "remaining data in the second plane is undefined." ?
> 
> Ah, right. Thank you for catching it.
> 
> > Can it still be useful for some applications, or is it complete garbage ?
> 
> I don't really know... I don't have good documentation for this. It is
> very likely there is some more interesting data in this 4 kiB buffer.
> So by "undefined" I meant "has undefined meaning" or "I don't know what's
> there at the moment"... From the requirements, it's only needed to decode
> the interleaved data and very likely won't be needed for anything else.
> 
> Perhaps I should change this sentence to
> "Remaining data in the first plane has undefined meaning." or drop it
> entirely ?

I'm fine with the text you proposed, I was just wondering.

> >> The information in the second plane allows to easily find location of the
> >> pointer array, which can be different for each frame. The size of the
> >> pointer array is +constant for given UYVY image height.</para>
> >> +<para>In order to extract UYVY and JPEG frames an application can
> >> initially set a data pointer to the start of first plane and then add an
> >> offset from the first entry of the pointers table. Such a pointer
> >> indicates start of an UYVY image pixel line. Whole UYVY line can be
> >> copied to a separate buffer. These steps should be repeated for each
> >> line, i.e. the number of entries in the pointer array. Anything what's
> >> in between the UYVY lines is JPEG data and should be concatenated to
> >> form the JPEG stream.</para>
> >> +</entry>
> >> +	</row>
> >> 
> >>   	</tbody>
> >>   	
> >>         </tgroup>
> >>       
> >>       </table>
> >> 
> >> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> >> index 8d29bb2..6c82ff5 100644
> >> --- a/include/linux/videodev2.h
> >> +++ b/include/linux/videodev2.h
> >> @@ -436,6 +436,7 @@ struct v4l2_pix_format {
> >> 
> >>   #define V4L2_PIX_FMT_KONICA420  v4l2_fourcc('K', 'O', 'N', 'I') /*
> >>   YUV420
> >> 
> >> planar in blocks of 256 pixels */ #define
> >> V4L2_PIX_FMT_JPGL	v4l2_fourcc('J', 'P', 'G', 'L') /* JPEG-Lite */ #define
> >> V4L2_PIX_FMT_SE401      v4l2_fourcc('S', '4', '0', '1') /* se401 janggu
> >> compressed rgb */ +#define V4L2_PIX_FMT_S5C_UYVY_JPG v4l2_fourcc('S',
> >> '5',
> >> 'C', 'J') /* S5C73M3 interleaved UYVY/JPEG */
> >> 
> >>   /*
> >>   
> >>    *	F O R M A T   E N U M E R A T I O N

-- 
Regards,

Laurent Pinchart

Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Jan Hoogenraad]

On my ubuntu 10.4 system

Linux 2.6.32-43-generic-pae #97-Ubuntu SMP Wed Sep 5 16:59:17 UTC 2012
i686 GNU/Linux

this patch breaks compilation of media_build.
The constant SZ_1M is not defined in the includes on my system

Do you know what can be done about this ?

---

/home/jhh/dvb/media_build/v4l/m5mols_core.c: In function
'm5mols_set_frame_desc':
/home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: 'SZ_1M'
undeclared (first use in this function)
/home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: (Each undeclared
identifier is reported only once
/home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: for each
function it appears in.)



Sylwester Nawrocki wrote:
> .get_frame_desc can be used by host interface driver to query
> properties of captured frames, e.g. required memory buffer size.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/i2c/m5mols/m5mols.h         |  9 ++++++
>  drivers/media/i2c/m5mols/m5mols_capture.c |  3 ++
>  drivers/media/i2c/m5mols/m5mols_core.c    | 47 +++++++++++++++++++++++++++++++
>  drivers/media/i2c/m5mols/m5mols_reg.h     |  1 +
>  4 files changed, 60 insertions(+)
> 
> diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
> index 15d3a4f..de3b755 100644
> --- a/drivers/media/i2c/m5mols/m5mols.h
> +++ b/drivers/media/i2c/m5mols/m5mols.h
> @@ -19,6 +19,13 @@
>  #include <media/v4l2-subdev.h>
>  #include "m5mols_reg.h"
>  
> +
> +/* An amount of data transmitted in addition to the value
> + * determined by CAPP_JPEG_SIZE_MAX register.
> + */
> +#define M5MOLS_JPEG_TAGS_SIZE		0x20000
> +#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
> +
>  extern int m5mols_debug;
>  
>  enum m5mols_restype {
> @@ -67,12 +74,14 @@ struct m5mols_exif {
>  /**
>   * struct m5mols_capture - Structure for the capture capability
>   * @exif: EXIF information
> + * @buf_size: internal JPEG frame buffer size, in bytes
>   * @main: size in bytes of the main image
>   * @thumb: size in bytes of the thumb image, if it was accompanied
>   * @total: total size in bytes of the produced image
>   */
>  struct m5mols_capture {
>  	struct m5mols_exif exif;
> +	unsigned int buf_size;
>  	u32 main;
>  	u32 thumb;
>  	u32 total;
> diff --git a/drivers/media/i2c/m5mols/m5mols_capture.c b/drivers/media/i2c/m5mols/m5mols_capture.c
> index cb243bd..ab34cce 100644
> --- a/drivers/media/i2c/m5mols/m5mols_capture.c
> +++ b/drivers/media/i2c/m5mols/m5mols_capture.c
> @@ -105,6 +105,7 @@ static int m5mols_capture_info(struct m5mols_info *info)
>  
>  int m5mols_start_capture(struct m5mols_info *info)
>  {
> +	unsigned int framesize = info->cap.buf_size - M5MOLS_JPEG_TAGS_SIZE;
>  	struct v4l2_subdev *sd = &info->sd;
>  	int ret;
>  
> @@ -121,6 +122,8 @@ int m5mols_start_capture(struct m5mols_info *info)
>  	if (!ret)
>  		ret = m5mols_write(sd, CAPP_MAIN_IMAGE_SIZE, info->resolution);
>  	if (!ret)
> +		ret = m5mols_write(sd, CAPP_JPEG_SIZE_MAX, framesize);
> +	if (!ret)
>  		ret = m5mols_set_mode(info, REG_CAPTURE);
>  	if (!ret)
>  		/* Wait until a frame is captured to ISP internal memory */
> diff --git a/drivers/media/i2c/m5mols/m5mols_core.c b/drivers/media/i2c/m5mols/m5mols_core.c
> index 933014f..c780689 100644
> --- a/drivers/media/i2c/m5mols/m5mols_core.c
> +++ b/drivers/media/i2c/m5mols/m5mols_core.c
> @@ -599,6 +599,51 @@ static int m5mols_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>  	return ret;
>  }
>  
> +static int m5mols_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				 struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +
> +	if (pad != 0 || fd == NULL)
> +		return -EINVAL;
> +
> +	mutex_lock(&info->lock);
> +	/*
> +	 * .get_frame_desc is only used for compressed formats,
> +	 * thus we always return the capture frame parameters here.
> +	 */
> +	fd->entry[0].length = info->cap.buf_size;
> +	fd->entry[0].pixelcode = info->ffmt[M5MOLS_RESTYPE_CAPTURE].code;
> +	mutex_unlock(&info->lock);
> +
> +	fd->entry[0].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
> +	fd->num_entries = 1;
> +
> +	return 0;
> +}
> +
> +static int m5mols_set_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				 struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	struct v4l2_mbus_framefmt *mf = &info->ffmt[M5MOLS_RESTYPE_CAPTURE];
> +
> +	if (pad != 0 || fd == NULL)
> +		return -EINVAL;
> +
> +	fd->entry[0].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
> +	fd->num_entries = 1;
> +	fd->entry[0].length = clamp_t(u32, fd->entry[0].length,
> +				      mf->width * mf->height,
> +				      M5MOLS_MAIN_JPEG_SIZE_MAX);
> +	mutex_lock(&info->lock);
> +	info->cap.buf_size = fd->entry[0].length;
> +	mutex_unlock(&info->lock);
> +
> +	return 0;
> +}
> +
> +
>  static int m5mols_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_fh *fh,
>  				 struct v4l2_subdev_mbus_code_enum *code)
> @@ -615,6 +660,8 @@ static struct v4l2_subdev_pad_ops m5mols_pad_ops = {
>  	.enum_mbus_code	= m5mols_enum_mbus_code,
>  	.get_fmt	= m5mols_get_fmt,
>  	.set_fmt	= m5mols_set_fmt,
> +	.get_frame_desc	= m5mols_get_frame_desc,
> +	.set_frame_desc	= m5mols_set_frame_desc,
>  };
>  
>  /**
> diff --git a/drivers/media/i2c/m5mols/m5mols_reg.h b/drivers/media/i2c/m5mols/m5mols_reg.h
> index 14d4be7..58d8027 100644
> --- a/drivers/media/i2c/m5mols/m5mols_reg.h
> +++ b/drivers/media/i2c/m5mols/m5mols_reg.h
> @@ -310,6 +310,7 @@
>  #define REG_JPEG		0x10
>  
>  #define CAPP_MAIN_IMAGE_SIZE	I2C_REG(CAT_CAPT_PARM, 0x01, 1)
> +#define CAPP_JPEG_SIZE_MAX	I2C_REG(CAT_CAPT_PARM, 0x0f, 4)
>  #define CAPP_JPEG_RATIO		I2C_REG(CAT_CAPT_PARM, 0x17, 1)
>  
>  #define CAPP_MCC_MODE		I2C_REG(CAT_CAPT_PARM, 0x1d, 1)
> 


-- 
Jan Hoogenraad
Hoogenraad Interface Services
Postbus 2717
3500 GS Utrecht

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Sylwester Nawrocki]

Hello Jan,

On 10/06/2012 05:24 PM, Jan Hoogenraad wrote:
> On my ubuntu 10.4 system
> 
> Linux 2.6.32-43-generic-pae #97-Ubuntu SMP Wed Sep 5 16:59:17 UTC 2012
> i686 GNU/Linux
> 
> this patch breaks compilation of media_build.
> The constant SZ_1M is not defined in the includes on my system
> 
> Do you know what can be done about this ?
> 
> ---
> 
> /home/jhh/dvb/media_build/v4l/m5mols_core.c: In function
> 'm5mols_set_frame_desc':
> /home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: 'SZ_1M'
> undeclared (first use in this function)
> /home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: (Each undeclared
> identifier is reported only once
> /home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: for each
> function it appears in.)

Thanks for reporting this issue. You most likely don't need the M-5MOLS
camera sensor driver on you system so one option is to just disable it
at kernel config. Make sure CONFIG_VIDEO_M5MOLS is not set, it can be 
unselected at menuconfig

 -> Device Drivers
    -> Multimedia
      -> Encoders, decoders, sensors and other helper chips
         < > Fujitsu M-5MOLS 8MP sensor support

The below patch which is intended to fix this issue won't work for
media drivers backport builds on kernels older than 3.6, so m5mols
driver should not be built for kernel versions < 3.6.

8<-------------------------------------------------------------------
>From 3e138ea603c9e5102452554cb14e4b404ce306e0 Mon Sep 17 00:00:00 2001
From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
Date: Sat, 6 Oct 2012 20:04:40 +0200
Subject: [PATCH] m5mols: Add missing #include <linux/sizes.h>

Include <linux/sizes.h> header that is missing after commit ab7ef22419927
"[media] m5mols: Implement .get_frame_desc subdev callback".
It prevents possible build errors due to undefined SZ_1M.

Reported-by: Jan Hoogenraad <jan-conceptronic@hoogenraad.net>
Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
---
 drivers/media/i2c/m5mols/m5mols.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
index 4ab8b37..90a6c52 100644
--- a/drivers/media/i2c/m5mols/m5mols.h
+++ b/drivers/media/i2c/m5mols/m5mols.h
@@ -16,6 +16,7 @@
 #ifndef M5MOLS_H
 #define M5MOLS_H
 
+#include <linux/sizes.h>
 #include <media/v4l2-subdev.h>
 #include "m5mols_reg.h"
 
-- 
1.7.4.1
8<-------------------------------------------------------------------

--

Regards,
Sylwester

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Jan Hoogenraad]

Thanks.

I see several drivers disabled for lower kernel versions in my Kconfig file.
I am not sure how this is accomplished, but it would be helpful if the
Fujitsu M-5MOLS 8MP sensor support
is automatically disabled for kernel < 3.6

I fixed it in my version by replacing SZ_1M by (1024*1024).
I did not need the driver, but at least it compiled ...


Sylwester Nawrocki wrote:
> Hello Jan,
> 
> On 10/06/2012 05:24 PM, Jan Hoogenraad wrote:
>> On my ubuntu 10.4 system
>>
>> Linux 2.6.32-43-generic-pae #97-Ubuntu SMP Wed Sep 5 16:59:17 UTC 2012
>> i686 GNU/Linux
>>
>> this patch breaks compilation of media_build.
>> The constant SZ_1M is not defined in the includes on my system
>>
>> Do you know what can be done about this ?
>>
>> ---
>>
>> /home/jhh/dvb/media_build/v4l/m5mols_core.c: In function
>> 'm5mols_set_frame_desc':
>> /home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: 'SZ_1M'
>> undeclared (first use in this function)
>> /home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: (Each undeclared
>> identifier is reported only once
>> /home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: for each
>> function it appears in.)
> 
> Thanks for reporting this issue. You most likely don't need the M-5MOLS
> camera sensor driver on you system so one option is to just disable it
> at kernel config. Make sure CONFIG_VIDEO_M5MOLS is not set, it can be 
> unselected at menuconfig
> 
>  -> Device Drivers
>     -> Multimedia
>       -> Encoders, decoders, sensors and other helper chips
>          < > Fujitsu M-5MOLS 8MP sensor support
> 
> The below patch which is intended to fix this issue won't work for
> media drivers backport builds on kernels older than 3.6, so m5mols
> driver should not be built for kernel versions < 3.6.
> 
> 8<-------------------------------------------------------------------
>>From 3e138ea603c9e5102452554cb14e4b404ce306e0 Mon Sep 17 00:00:00 2001
> From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> Date: Sat, 6 Oct 2012 20:04:40 +0200
> Subject: [PATCH] m5mols: Add missing #include <linux/sizes.h>
> 
> Include <linux/sizes.h> header that is missing after commit ab7ef22419927
> "[media] m5mols: Implement .get_frame_desc subdev callback".
> It prevents possible build errors due to undefined SZ_1M.
> 
> Reported-by: Jan Hoogenraad <jan-conceptronic@hoogenraad.net>
> Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> ---
>  drivers/media/i2c/m5mols/m5mols.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
> index 4ab8b37..90a6c52 100644
> --- a/drivers/media/i2c/m5mols/m5mols.h
> +++ b/drivers/media/i2c/m5mols/m5mols.h
> @@ -16,6 +16,7 @@
>  #ifndef M5MOLS_H
>  #define M5MOLS_H
>  
> +#include <linux/sizes.h>
>  #include <media/v4l2-subdev.h>
>  #include "m5mols_reg.h"
>  
> 


-- 
Jan Hoogenraad
Hoogenraad Interface Services
Postbus 2717
3500 GS Utrecht

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Sylwester Nawrocki]

On 10/06/2012 08:43 PM, Jan Hoogenraad wrote:
> Thanks.
>
> I see several drivers disabled for lower kernel versions in my Kconfig file.
> I am not sure how this is accomplished, but it would be helpful if the
> Fujitsu M-5MOLS 8MP sensor support
> is automatically disabled for kernel<  3.6
>
> I fixed it in my version by replacing SZ_1M by (1024*1024).
> I did not need the driver, but at least it compiled ...

A patch for v4l/versions.txt is needed [1].
I'll see if I can prepare that.

http://git.linuxtv.org/media_build.git/history/5d00dba6aaf0f91a742d90fd1e12e0fb2d36253e:/v4l/versions.txt 

RE: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Michael West]

This patch changes versions.txt and disables  VIDEO_M5MOLS which fixed the build for my 3.2 kernel but looking at the logs it looks like this is not the way to fix it as it's not just a 3.6+ problem as it does not build on 3.6 as well...  So probably best to find why it doesn't build on the current kernel first.

---
 v4l/versions.txt |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/v4l/versions.txt b/v4l/versions.txt
index 328651e..349695c 100644
--- a/v4l/versions.txt
+++ b/v4l/versions.txt
@@ -4,6 +4,8 @@
 [3.6.0]
 # needs devm_clk_get, clk_enable, clk_disable
 VIDEO_CODA
+# broken add reason here
+VIDEO_M5MOLS
 
 [3.4.0]
 # needs devm_regulator_bulk_get
-- 
1.7.9.5

>On 10/06/2012 08:43 PM, Jan Hoogenraad wrote:
>> Thanks.
>>
>> I see several drivers disabled for lower kernel versions in my Kconfig file.
>> I am not sure how this is accomplished, but it would be helpful if the 
>> Fujitsu M-5MOLS 8MP sensor support is automatically disabled for 
>> kernel<  3.6
>>
>> I fixed it in my version by replacing SZ_1M by (1024*1024).
>> I did not need the driver, but at least it compiled ...
>
>A patch for v4l/versions.txt is needed [1].
>I'll see if I can prepare that.
>http://git.linuxtv.org/media_build.git/history/5d00dba6aaf0f91a742d90fd1e12e0fb2d36253e:/v4l/versions.txt 

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Hans Verkuil]

On Sun October 7 2012 03:19:48 Michael West wrote:
> This patch changes versions.txt and disables  VIDEO_M5MOLS which fixed the build for my 3.2 kernel but looking at the logs it looks like this is not the way to fix it as it's not just a 3.6+ problem as it does not build on 3.6 as well...  So probably best to find why it doesn't build on the current kernel first.

FYI, I'll be fixing this and other build problems in media_build on Monday.

Regards,

	Hans

> 
> ---
>  v4l/versions.txt |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/v4l/versions.txt b/v4l/versions.txt
> index 328651e..349695c 100644
> --- a/v4l/versions.txt
> +++ b/v4l/versions.txt
> @@ -4,6 +4,8 @@
>  [3.6.0]
>  # needs devm_clk_get, clk_enable, clk_disable
>  VIDEO_CODA
> +# broken add reason here
> +VIDEO_M5MOLS
>  
>  [3.4.0]
>  # needs devm_regulator_bulk_get
> >On 10/06/2012 08:43 PM, Jan Hoogenraad wrote:
> >> Thanks.
> >>
> >> I see several drivers disabled for lower kernel versions in my Kconfig file.
> >> I am not sure how this is accomplished, but it would be helpful if the 
> >> Fujitsu M-5MOLS 8MP sensor support is automatically disabled for 
> >> kernel<  3.6
> >>
> >> I fixed it in my version by replacing SZ_1M by (1024*1024).
> >> I did not need the driver, but at least it compiled ...
> >
> >A patch for v4l/versions.txt is needed [1].
> >I'll see if I can prepare that.
> >http://git.linuxtv.org/media_build.git/history/5d00dba6aaf0f91a742d90fd1e12e0fb2d36253e:/v4l/versions.txt 
> 
> 

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Sylwester Nawrocki]

On 10/07/2012 03:19 AM, Michael West wrote:
> This patch changes versions.txt and disables  VIDEO_M5MOLS which 
> fixed the build for my 3.2 kernel but looking at the logs it looks
> like this is not the way to fix it as it's not just a 3.6+ problem
> as it does not build on 3.6 as well...  So probably best to find 
> why it doesn't build on the current kernel first.

To fix the build on kernels 3.6+ <linux/sizes.h> just needs to be 
inclcuded in m5mols.h. This is what my patch from previous message 
in this thread does. But this will break again on kernel versions 
_3.5 and lower_ where <linux/sizes.h> doesn't exist. I thought
originally it could have been simply replaced there with <asm/sizes.h>, 
but not all architectures have it

$ git grep  "#define SZ_1M" v2.6.32
v2.6.32:arch/arm/include/asm/sizes.h:#define SZ_1M                           0x00100000
v2.6.32:arch/sh/include/asm/sizes.h:#define SZ_1M                           0x00100000

$ git grep  "#define SZ_1M" v3.6-rc5
v3.6-rc5:drivers/base/dma-contiguous.c:#define SZ_1M (1 << 20)
v3.6-rc5:include/linux/sizes.h:#define SZ_1M                            0x00100000


Let's just use the below patch to solve this build break, this way
there is no need to touch anything at media_build.

>From 11adc6956f3fe87c897aa6add08f8437422969a8 Mon Sep 17 00:00:00 2001
From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
Date: Sun, 7 Oct 2012 13:04:37 +0200
Subject: [PATCH] m5mols: Replace SZ_1M with explicit value

SZ_1M macro definition was introduced in commit ab7ef22419927
"[media] m5mols: Implement .get_frame_desc subdev callback"
but required <linux/sizes.h> header was not included. To prevent
build errors with older kernels where <linux/sizes.h> doesn't exist
use explicit value rather than SZ_1M.

Reported-by: Jan Hoogenraad <jan-conceptronic@hoogenraad.net>
Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
---
 drivers/media/i2c/m5mols/m5mols.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
index 4ab8b37..30654f5 100644
--- a/drivers/media/i2c/m5mols/m5mols.h
+++ b/drivers/media/i2c/m5mols/m5mols.h
@@ -24,7 +24,7 @@
  * determined by CAPP_JPEG_SIZE_MAX register.
  */
 #define M5MOLS_JPEG_TAGS_SIZE		0x20000
-#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
+#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * 1024 * 1024)
 
 extern int m5mols_debug;
 
-- 
1.7.4.1

> ---
>   v4l/versions.txt |    2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/v4l/versions.txt b/v4l/versions.txt
> index 328651e..349695c 100644
> --- a/v4l/versions.txt
> +++ b/v4l/versions.txt
> @@ -4,6 +4,8 @@
>   [3.6.0]
>   # needs devm_clk_get, clk_enable, clk_disable
>   VIDEO_CODA
> +# broken add reason here
> +VIDEO_M5MOLS

This was supposed to be under [3.5.0].

> 
>   [3.4.0]
>   # needs devm_regulator_bulk_get
> -- 1.7.9.5

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Hans Verkuil]

On Sun October 7 2012 13:13:36 Sylwester Nawrocki wrote:
> On 10/07/2012 03:19 AM, Michael West wrote:
> > This patch changes versions.txt and disables  VIDEO_M5MOLS which 
> > fixed the build for my 3.2 kernel but looking at the logs it looks
> > like this is not the way to fix it as it's not just a 3.6+ problem
> > as it does not build on 3.6 as well...  So probably best to find 
> > why it doesn't build on the current kernel first.
> 
> To fix the build on kernels 3.6+ <linux/sizes.h> just needs to be 
> inclcuded in m5mols.h. This is what my patch from previous message 
> in this thread does. But this will break again on kernel versions 
> _3.5 and lower_ where <linux/sizes.h> doesn't exist. I thought
> originally it could have been simply replaced there with <asm/sizes.h>, 
> but not all architectures have it
> 
> $ git grep  "#define SZ_1M" v2.6.32
> v2.6.32:arch/arm/include/asm/sizes.h:#define SZ_1M                           0x00100000
> v2.6.32:arch/sh/include/asm/sizes.h:#define SZ_1M                           0x00100000
> 
> $ git grep  "#define SZ_1M" v3.6-rc5
> v3.6-rc5:drivers/base/dma-contiguous.c:#define SZ_1M (1 << 20)
> v3.6-rc5:include/linux/sizes.h:#define SZ_1M                            0x00100000
> 
> 
> Let's just use the below patch to solve this build break, this way
> there is no need to touch anything at media_build.
> 
> From 11adc6956f3fe87c897aa6add08f8437422969a8 Mon Sep 17 00:00:00 2001
> From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> Date: Sun, 7 Oct 2012 13:04:37 +0200
> Subject: [PATCH] m5mols: Replace SZ_1M with explicit value
> 
> SZ_1M macro definition was introduced in commit ab7ef22419927
> "[media] m5mols: Implement .get_frame_desc subdev callback"
> but required <linux/sizes.h> header was not included. To prevent
> build errors with older kernels where <linux/sizes.h> doesn't exist
> use explicit value rather than SZ_1M.
> 
> Reported-by: Jan Hoogenraad <jan-conceptronic@hoogenraad.net>
> Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Note: until this patch is merged I am disabling this driver in media_build
since right now it doesn't compile at all. Please notify me when this is
fixed in media_tree.git so that I can enable it again.

Regards,

	Hans

> ---
>  drivers/media/i2c/m5mols/m5mols.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
> index 4ab8b37..30654f5 100644
> --- a/drivers/media/i2c/m5mols/m5mols.h
> +++ b/drivers/media/i2c/m5mols/m5mols.h
> @@ -24,7 +24,7 @@
>   * determined by CAPP_JPEG_SIZE_MAX register.
>   */
>  #define M5MOLS_JPEG_TAGS_SIZE		0x20000
> -#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
> +#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * 1024 * 1024)
>  
>  extern int m5mols_debug;
>  
> > ---
> >   v4l/versions.txt |    2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/v4l/versions.txt b/v4l/versions.txt
> > index 328651e..349695c 100644
> > --- a/v4l/versions.txt
> > +++ b/v4l/versions.txt
> > @@ -4,6 +4,8 @@
> >   [3.6.0]
> >   # needs devm_clk_get, clk_enable, clk_disable
> >   VIDEO_CODA
> > +# broken add reason here
> > +VIDEO_M5MOLS
> 
> This was supposed to be under [3.5.0].
> 
> > 
> >   [3.4.0]
> >   # needs devm_regulator_bulk_get
> > -- 1.7.9.5
> 

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Laurent Pinchart]

Hi,

When did the {get,set}_frame_desc subdev operations reach mainline ? Last I 
knew is that they were an RFC, and they're now suddenly in, without even one 
ack from an embedded v4l developer :-S I'm not totally happy with that.

On Saturday 06 October 2012 17:24:22 Jan Hoogenraad wrote:
> On my ubuntu 10.4 system
> 
> Linux 2.6.32-43-generic-pae #97-Ubuntu SMP Wed Sep 5 16:59:17 UTC 2012
> i686 GNU/Linux
> 
> this patch breaks compilation of media_build.
> The constant SZ_1M is not defined in the includes on my system
> 
> Do you know what can be done about this ?
> 
> ---
> 
> /home/jhh/dvb/media_build/v4l/m5mols_core.c: In function
> 'm5mols_set_frame_desc':
> /home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: 'SZ_1M'
> undeclared (first use in this function)
> /home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: (Each undeclared
> identifier is reported only once
> /home/jhh/dvb/media_build/v4l/m5mols_core.c:636: error: for each
> function it appears in.)
> 
> Sylwester Nawrocki wrote:
> > .get_frame_desc can be used by host interface driver to query
> > properties of captured frames, e.g. required memory buffer size.
> > 
> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > 
> >  drivers/media/i2c/m5mols/m5mols.h         |  9 ++++++
> >  drivers/media/i2c/m5mols/m5mols_capture.c |  3 ++
> >  drivers/media/i2c/m5mols/m5mols_core.c    | 47
> >  +++++++++++++++++++++++++++++++ drivers/media/i2c/m5mols/m5mols_reg.h   
> >   |  1 +
> >  4 files changed, 60 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/m5mols/m5mols.h
> > b/drivers/media/i2c/m5mols/m5mols.h index 15d3a4f..de3b755 100644
> > --- a/drivers/media/i2c/m5mols/m5mols.h
> > +++ b/drivers/media/i2c/m5mols/m5mols.h
> > @@ -19,6 +19,13 @@
> > 
> >  #include <media/v4l2-subdev.h>
> >  #include "m5mols_reg.h"
> > 
> > +
> > +/* An amount of data transmitted in addition to the value
> > + * determined by CAPP_JPEG_SIZE_MAX register.
> > + */
> > +#define M5MOLS_JPEG_TAGS_SIZE		0x20000
> > +#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
> > +
> > 
> >  extern int m5mols_debug;
> >  
> >  enum m5mols_restype {
> > 
> > @@ -67,12 +74,14 @@ struct m5mols_exif {
> > 
> >  /**
> >  
> >   * struct m5mols_capture - Structure for the capture capability
> >   * @exif: EXIF information
> > 
> > + * @buf_size: internal JPEG frame buffer size, in bytes
> > 
> >   * @main: size in bytes of the main image
> >   * @thumb: size in bytes of the thumb image, if it was accompanied
> >   * @total: total size in bytes of the produced image
> >   */
> >  
> >  struct m5mols_capture {
> >  
> >  	struct m5mols_exif exif;
> > 
> > +	unsigned int buf_size;
> > 
> >  	u32 main;
> >  	u32 thumb;
> >  	u32 total;
> > 
> > diff --git a/drivers/media/i2c/m5mols/m5mols_capture.c
> > b/drivers/media/i2c/m5mols/m5mols_capture.c index cb243bd..ab34cce 100644
> > --- a/drivers/media/i2c/m5mols/m5mols_capture.c
> > +++ b/drivers/media/i2c/m5mols/m5mols_capture.c
> > @@ -105,6 +105,7 @@ static int m5mols_capture_info(struct m5mols_info
> > *info)> 
> >  int m5mols_start_capture(struct m5mols_info *info)
> >  {
> > 
> > +	unsigned int framesize = info->cap.buf_size - M5MOLS_JPEG_TAGS_SIZE;
> > 
> >  	struct v4l2_subdev *sd = &info->sd;
> >  	int ret;
> > 
> > @@ -121,6 +122,8 @@ int m5mols_start_capture(struct m5mols_info *info)
> > 
> >  	if (!ret)
> >  	
> >  		ret = m5mols_write(sd, CAPP_MAIN_IMAGE_SIZE, info->resolution);
> >  	
> >  	if (!ret)
> > 
> > +		ret = m5mols_write(sd, CAPP_JPEG_SIZE_MAX, framesize);
> > +	if (!ret)
> > 
> >  		ret = m5mols_set_mode(info, REG_CAPTURE);
> >  	
> >  	if (!ret)
> >  	
> >  		/* Wait until a frame is captured to ISP internal memory */
> > 
> > diff --git a/drivers/media/i2c/m5mols/m5mols_core.c
> > b/drivers/media/i2c/m5mols/m5mols_core.c index 933014f..c780689 100644
> > --- a/drivers/media/i2c/m5mols/m5mols_core.c
> > +++ b/drivers/media/i2c/m5mols/m5mols_core.c
> > @@ -599,6 +599,51 @@ static int m5mols_set_fmt(struct v4l2_subdev *sd,
> > struct v4l2_subdev_fh *fh,> 
> >  	return ret;
> >  
> >  }
> > 
> > +static int m5mols_get_frame_desc(struct v4l2_subdev *sd, unsigned int
> > pad,
> > +				 struct v4l2_mbus_frame_desc *fd)
> > +{
> > +	struct m5mols_info *info = to_m5mols(sd);
> > +
> > +	if (pad != 0 || fd == NULL)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&info->lock);
> > +	/*
> > +	 * .get_frame_desc is only used for compressed formats,
> > +	 * thus we always return the capture frame parameters here.
> > +	 */
> > +	fd->entry[0].length = info->cap.buf_size;
> > +	fd->entry[0].pixelcode = info->ffmt[M5MOLS_RESTYPE_CAPTURE].code;
> > +	mutex_unlock(&info->lock);
> > +
> > +	fd->entry[0].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
> > +	fd->num_entries = 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static int m5mols_set_frame_desc(struct v4l2_subdev *sd, unsigned int
> > pad,
> > +				 struct v4l2_mbus_frame_desc *fd)
> > +{
> > +	struct m5mols_info *info = to_m5mols(sd);
> > +	struct v4l2_mbus_framefmt *mf = &info->ffmt[M5MOLS_RESTYPE_CAPTURE];
> > +
> > +	if (pad != 0 || fd == NULL)
> > +		return -EINVAL;
> > +
> > +	fd->entry[0].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
> > +	fd->num_entries = 1;
> > +	fd->entry[0].length = clamp_t(u32, fd->entry[0].length,
> > +				      mf->width * mf->height,
> > +				      M5MOLS_MAIN_JPEG_SIZE_MAX);
> > +	mutex_lock(&info->lock);
> > +	info->cap.buf_size = fd->entry[0].length;
> > +	mutex_unlock(&info->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +
> > 
> >  static int m5mols_enum_mbus_code(struct v4l2_subdev *sd,
> >  
> >  				 struct v4l2_subdev_fh *fh,
> >  				 struct v4l2_subdev_mbus_code_enum *code)
> > 
> > @@ -615,6 +660,8 @@ static struct v4l2_subdev_pad_ops m5mols_pad_ops = {
> > 
> >  	.enum_mbus_code	= m5mols_enum_mbus_code,
> >  	.get_fmt	= m5mols_get_fmt,
> >  	.set_fmt	= m5mols_set_fmt,
> > 
> > +	.get_frame_desc	= m5mols_get_frame_desc,
> > +	.set_frame_desc	= m5mols_set_frame_desc,
> > 
> >  };
> >  
> >  /**
> > 
> > diff --git a/drivers/media/i2c/m5mols/m5mols_reg.h
> > b/drivers/media/i2c/m5mols/m5mols_reg.h index 14d4be7..58d8027 100644
> > --- a/drivers/media/i2c/m5mols/m5mols_reg.h
> > +++ b/drivers/media/i2c/m5mols/m5mols_reg.h
> > @@ -310,6 +310,7 @@
> > 
> >  #define REG_JPEG		0x10
> >  
> >  #define CAPP_MAIN_IMAGE_SIZE	I2C_REG(CAT_CAPT_PARM, 0x01, 1)
> > 
> > +#define CAPP_JPEG_SIZE_MAX	I2C_REG(CAT_CAPT_PARM, 0x0f, 4)
> > 
> >  #define CAPP_JPEG_RATIO		I2C_REG(CAT_CAPT_PARM, 0x17, 1)
> >  
> >  #define CAPP_MCC_MODE		I2C_REG(CAT_CAPT_PARM, 0x1d, 1)
-- 
Regards,

Laurent Pinchart

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Sylwester Nawrocki]

Hi Laurent

On 10/08/2012 10:42 PM, Laurent Pinchart wrote:
> Hi,
> 
> When did the {get,set}_frame_desc subdev operations reach mainline ? Last I 
> knew is that they were an RFC, and they're now suddenly in, without even one 
> ack from an embedded v4l developer :-S I'm not totally happy with that.

Sorry to hear now you have issues with those patches. I've sent a pull request
on last Wednesday [1], after 3 RFC versions of the whole change set. I've Cced
many people on these patches, including you and Sakari. I have addressed comments 
to some patches you raised and I felt you're generally OK or don't have 
objections (since there was no comments on some patches and I explicitly 
mentioned there I'd like to get them in v3.7). I'm sorry if it was otherwise.

The problem I'm trying to address with those patches have been there for us for
over _one year_ [1], making JPEG capture with s5p-fimc effectively unusable in
the mainline kernel.

We've decided extending struct v4l2_mbus_framefmt was not an option, since it
is exposed to user-space now. Also using controls turned out to not be helpful,
since multiple values need to be passed, where there are multiple logical
streams transmitted by e.g. CSI-2 transmitter in single video frame [2].

It seemed like there is a general agreement on LMML to use the frame 
descriptor callbacks instead. These callbacks are all in-kernel API and can
be changed any time. I'll be happy adapt the drivers to whatever sane changes
are proposed. I've stated in the commit description it's just an preliminary
form. And it at least let's us to move forward and carry on with more serious
problems.

I'll try to do better on letting people know, when sending things upstream
in future.

--

Regards,
Sylwester

[1] http://patchwork.linuxtv.org/patch/14875
[2] https://patchwork.kernel.org/patch/1365451

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Mauro Carvalho Chehab]

Em Mon, 8 Oct 2012 15:03:36 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Sun October 7 2012 13:13:36 Sylwester Nawrocki wrote:
> > On 10/07/2012 03:19 AM, Michael West wrote:
> > > This patch changes versions.txt and disables  VIDEO_M5MOLS which 
> > > fixed the build for my 3.2 kernel but looking at the logs it looks
> > > like this is not the way to fix it as it's not just a 3.6+ problem
> > > as it does not build on 3.6 as well...  So probably best to find 
> > > why it doesn't build on the current kernel first.
> > 
> > To fix the build on kernels 3.6+ <linux/sizes.h> just needs to be 
> > inclcuded in m5mols.h. This is what my patch from previous message 
> > in this thread does. But this will break again on kernel versions 
> > _3.5 and lower_ where <linux/sizes.h> doesn't exist. I thought
> > originally it could have been simply replaced there with <asm/sizes.h>, 
> > but not all architectures have it
> > 
> > $ git grep  "#define SZ_1M" v2.6.32
> > v2.6.32:arch/arm/include/asm/sizes.h:#define SZ_1M                           0x00100000
> > v2.6.32:arch/sh/include/asm/sizes.h:#define SZ_1M                           0x00100000
> > 
> > $ git grep  "#define SZ_1M" v3.6-rc5
> > v3.6-rc5:drivers/base/dma-contiguous.c:#define SZ_1M (1 << 20)
> > v3.6-rc5:include/linux/sizes.h:#define SZ_1M                            0x00100000
> > 
> > 
> > Let's just use the below patch to solve this build break, this way
> > there is no need to touch anything at media_build.
> > 
> > From 11adc6956f3fe87c897aa6add08f8437422969a8 Mon Sep 17 00:00:00 2001
> > From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > Date: Sun, 7 Oct 2012 13:04:37 +0200
> > Subject: [PATCH] m5mols: Replace SZ_1M with explicit value
> > 
> > SZ_1M macro definition was introduced in commit ab7ef22419927
> > "[media] m5mols: Implement .get_frame_desc subdev callback"
> > but required <linux/sizes.h> header was not included. To prevent
> > build errors with older kernels where <linux/sizes.h> doesn't exist
> > use explicit value rather than SZ_1M.
> > 
> > Reported-by: Jan Hoogenraad <jan-conceptronic@hoogenraad.net>
> > Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> 
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Note: until this patch is merged I am disabling this driver in media_build
> since right now it doesn't compile at all. Please notify me when this is
> fixed in media_tree.git so that I can enable it again.
> 
> Regards,
> 
> 	Hans
> 
> > ---
> >  drivers/media/i2c/m5mols/m5mols.h |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
> > index 4ab8b37..30654f5 100644
> > --- a/drivers/media/i2c/m5mols/m5mols.h
> > +++ b/drivers/media/i2c/m5mols/m5mols.h
> > @@ -24,7 +24,7 @@
> >   * determined by CAPP_JPEG_SIZE_MAX register.
> >   */
> >  #define M5MOLS_JPEG_TAGS_SIZE		0x20000
> > -#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
> > +#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * 1024 * 1024)

Nah! Please don't do that! we shouldn't be patching a driver upstream
just because it broke media_build. Also, fixing it there is as simple as
adding something similar to this at compat.h:

#ifndef SZ_1M
	#define SZ_1m (1024 * 1024)
#endif

> >  
> >  extern int m5mols_debug;
> >  
> > > ---
> > >   v4l/versions.txt |    2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/v4l/versions.txt b/v4l/versions.txt
> > > index 328651e..349695c 100644
> > > --- a/v4l/versions.txt
> > > +++ b/v4l/versions.txt
> > > @@ -4,6 +4,8 @@
> > >   [3.6.0]
> > >   # needs devm_clk_get, clk_enable, clk_disable
> > >   VIDEO_CODA
> > > +# broken add reason here
> > > +VIDEO_M5MOLS
> > 
> > This was supposed to be under [3.5.0].
> > 
> > > 
> > >   [3.4.0]
> > >   # needs devm_regulator_bulk_get
> > > -- 1.7.9.5
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Regards,
Mauro

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Hans Verkuil]

On Wed October 10 2012 03:05:30 Mauro Carvalho Chehab wrote:
> Em Mon, 8 Oct 2012 15:03:36 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On Sun October 7 2012 13:13:36 Sylwester Nawrocki wrote:
> > > On 10/07/2012 03:19 AM, Michael West wrote:
> > > > This patch changes versions.txt and disables  VIDEO_M5MOLS which 
> > > > fixed the build for my 3.2 kernel but looking at the logs it looks
> > > > like this is not the way to fix it as it's not just a 3.6+ problem
> > > > as it does not build on 3.6 as well...  So probably best to find 
> > > > why it doesn't build on the current kernel first.
> > > 
> > > To fix the build on kernels 3.6+ <linux/sizes.h> just needs to be 
> > > inclcuded in m5mols.h. This is what my patch from previous message 
> > > in this thread does. But this will break again on kernel versions 
> > > _3.5 and lower_ where <linux/sizes.h> doesn't exist. I thought
> > > originally it could have been simply replaced there with <asm/sizes.h>, 
> > > but not all architectures have it
> > > 
> > > $ git grep  "#define SZ_1M" v2.6.32
> > > v2.6.32:arch/arm/include/asm/sizes.h:#define SZ_1M                           0x00100000
> > > v2.6.32:arch/sh/include/asm/sizes.h:#define SZ_1M                           0x00100000
> > > 
> > > $ git grep  "#define SZ_1M" v3.6-rc5
> > > v3.6-rc5:drivers/base/dma-contiguous.c:#define SZ_1M (1 << 20)
> > > v3.6-rc5:include/linux/sizes.h:#define SZ_1M                            0x00100000
> > > 
> > > 
> > > Let's just use the below patch to solve this build break, this way
> > > there is no need to touch anything at media_build.
> > > 
> > > From 11adc6956f3fe87c897aa6add08f8437422969a8 Mon Sep 17 00:00:00 2001
> > > From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > > Date: Sun, 7 Oct 2012 13:04:37 +0200
> > > Subject: [PATCH] m5mols: Replace SZ_1M with explicit value
> > > 
> > > SZ_1M macro definition was introduced in commit ab7ef22419927
> > > "[media] m5mols: Implement .get_frame_desc subdev callback"
> > > but required <linux/sizes.h> header was not included. To prevent
> > > build errors with older kernels where <linux/sizes.h> doesn't exist
> > > use explicit value rather than SZ_1M.
> > > 
> > > Reported-by: Jan Hoogenraad <jan-conceptronic@hoogenraad.net>
> > > Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > 
> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Note: until this patch is merged I am disabling this driver in media_build
> > since right now it doesn't compile at all. Please notify me when this is
> > fixed in media_tree.git so that I can enable it again.
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > ---
> > >  drivers/media/i2c/m5mols/m5mols.h |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
> > > index 4ab8b37..30654f5 100644
> > > --- a/drivers/media/i2c/m5mols/m5mols.h
> > > +++ b/drivers/media/i2c/m5mols/m5mols.h
> > > @@ -24,7 +24,7 @@
> > >   * determined by CAPP_JPEG_SIZE_MAX register.
> > >   */
> > >  #define M5MOLS_JPEG_TAGS_SIZE		0x20000
> > > -#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
> > > +#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * 1024 * 1024)
> 
> Nah! Please don't do that! we shouldn't be patching a driver upstream
> just because it broke media_build. Also, fixing it there is as simple as
> adding something similar to this at compat.h:
> 
> #ifndef SZ_1M
> 	#define SZ_1m (1024 * 1024)
> #endif

Actually, I prefer 1024 * 1024 over SZ_1M. The alternative patch is this:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg53424.html

Note that the arm architecture will pull in linux/sizes.h, but the x86 arch
doesn't, so this driver won't compile with x86. It's a real driver bug that
has nothing to do with media_build.

So you need to merge one of these two patches to fix this problem. I prefer
the first, but the second is fine too.

Regards,

	Hans

> 
> > >  
> > >  extern int m5mols_debug;
> > >  
> > > > ---
> > > >   v4l/versions.txt |    2 ++
> > > >   1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/v4l/versions.txt b/v4l/versions.txt
> > > > index 328651e..349695c 100644
> > > > --- a/v4l/versions.txt
> > > > +++ b/v4l/versions.txt
> > > > @@ -4,6 +4,8 @@
> > > >   [3.6.0]
> > > >   # needs devm_clk_get, clk_enable, clk_disable
> > > >   VIDEO_CODA
> > > > +# broken add reason here
> > > > +VIDEO_M5MOLS
> > > 
> > > This was supposed to be under [3.5.0].
> > > 
> > > > 
> > > >   [3.4.0]
> > > >   # needs devm_regulator_bulk_get
> > > > -- 1.7.9.5
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Sylwester Nawrocki]

On 10/10/2012 08:27 AM, Hans Verkuil wrote:
>>>> diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
>>>> index 4ab8b37..30654f5 100644
>>>> --- a/drivers/media/i2c/m5mols/m5mols.h
>>>> +++ b/drivers/media/i2c/m5mols/m5mols.h
>>>> @@ -24,7 +24,7 @@
>>>>   * determined by CAPP_JPEG_SIZE_MAX register.
>>>>   */
>>>>  #define M5MOLS_JPEG_TAGS_SIZE		0x20000
>>>> -#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
>>>> +#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * 1024 * 1024)
>>
>> Nah! Please don't do that! we shouldn't be patching a driver upstream
>> just because it broke media_build. Also, fixing it there is as simple as
>> adding something similar to this at compat.h:
>>
>> #ifndef SZ_1M
>> 	#define SZ_1m (1024 * 1024)
>> #endif
> 
> Actually, I prefer 1024 * 1024 over SZ_1M. The alternative patch is this:
> 
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg53424.html
> 
> Note that the arm architecture will pull in linux/sizes.h, but the x86 arch
> doesn't, so this driver won't compile with x86. It's a real driver bug that
> has nothing to do with media_build.
> 
> So you need to merge one of these two patches to fix this problem. I prefer
> the first, but the second is fine too.

Yes, there is a bug in a driver now, even though the driver compiles on 
ARM, where linux/sizes.h is included indirectly, it won't build on other
architectures where it's not the case. I'm not sure which patch is better,
but one of them needs to be applied. Otherwise we're going to see bug 
reports from people building kernel 3.6+ with allyesconfig, etc..

--

Regards,
Sylwester

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Mauro Carvalho Chehab]

Em Wed, 10 Oct 2012 08:27:26 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Wed October 10 2012 03:05:30 Mauro Carvalho Chehab wrote:
> > Em Mon, 8 Oct 2012 15:03:36 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > 
> > > On Sun October 7 2012 13:13:36 Sylwester Nawrocki wrote:
> > > > On 10/07/2012 03:19 AM, Michael West wrote:
> > > > > This patch changes versions.txt and disables  VIDEO_M5MOLS which 
> > > > > fixed the build for my 3.2 kernel but looking at the logs it looks
> > > > > like this is not the way to fix it as it's not just a 3.6+ problem
> > > > > as it does not build on 3.6 as well...  So probably best to find 
> > > > > why it doesn't build on the current kernel first.
> > > > 
> > > > To fix the build on kernels 3.6+ <linux/sizes.h> just needs to be 
> > > > inclcuded in m5mols.h. This is what my patch from previous message 
> > > > in this thread does. But this will break again on kernel versions 
> > > > _3.5 and lower_ where <linux/sizes.h> doesn't exist. I thought
> > > > originally it could have been simply replaced there with <asm/sizes.h>, 
> > > > but not all architectures have it
> > > > 
> > > > $ git grep  "#define SZ_1M" v2.6.32
> > > > v2.6.32:arch/arm/include/asm/sizes.h:#define SZ_1M                           0x00100000
> > > > v2.6.32:arch/sh/include/asm/sizes.h:#define SZ_1M                           0x00100000
> > > > 
> > > > $ git grep  "#define SZ_1M" v3.6-rc5
> > > > v3.6-rc5:drivers/base/dma-contiguous.c:#define SZ_1M (1 << 20)
> > > > v3.6-rc5:include/linux/sizes.h:#define SZ_1M                            0x00100000
> > > > 
> > > > 
> > > > Let's just use the below patch to solve this build break, this way
> > > > there is no need to touch anything at media_build.
> > > > 
> > > > From 11adc6956f3fe87c897aa6add08f8437422969a8 Mon Sep 17 00:00:00 2001
> > > > From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > > > Date: Sun, 7 Oct 2012 13:04:37 +0200
> > > > Subject: [PATCH] m5mols: Replace SZ_1M with explicit value
> > > > 
> > > > SZ_1M macro definition was introduced in commit ab7ef22419927
> > > > "[media] m5mols: Implement .get_frame_desc subdev callback"
> > > > but required <linux/sizes.h> header was not included. To prevent
> > > > build errors with older kernels where <linux/sizes.h> doesn't exist
> > > > use explicit value rather than SZ_1M.
> > > > 
> > > > Reported-by: Jan Hoogenraad <jan-conceptronic@hoogenraad.net>
> > > > Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > > 
> > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > 
> > > Note: until this patch is merged I am disabling this driver in media_build
> > > since right now it doesn't compile at all. Please notify me when this is
> > > fixed in media_tree.git so that I can enable it again.
> > > 
> > > Regards,
> > > 
> > > 	Hans
> > > 
> > > > ---
> > > >  drivers/media/i2c/m5mols/m5mols.h |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
> > > > index 4ab8b37..30654f5 100644
> > > > --- a/drivers/media/i2c/m5mols/m5mols.h
> > > > +++ b/drivers/media/i2c/m5mols/m5mols.h
> > > > @@ -24,7 +24,7 @@
> > > >   * determined by CAPP_JPEG_SIZE_MAX register.
> > > >   */
> > > >  #define M5MOLS_JPEG_TAGS_SIZE		0x20000
> > > > -#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
> > > > +#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * 1024 * 1024)
> > 
> > Nah! Please don't do that! we shouldn't be patching a driver upstream
> > just because it broke media_build. Also, fixing it there is as simple as
> > adding something similar to this at compat.h:
> > 
> > #ifndef SZ_1M
> > 	#define SZ_1m (1024 * 1024)
> > #endif
> 
> Actually, I prefer 1024 * 1024 over SZ_1M. 

I also think that this obfuscates the code, but the right place to discuss it is
not here; it is with whomever is proposing those defines, and the ones
that acked with it:

$ git log include/linux/sizes.h 
commit dccd2304cc907c4b4d2920eeb24b055320fe942e
Author: Alessandro Rubini <rubini@gnudd.com>
Date:   Sun Jun 24 12:46:05 2012 +0100

    ARM: 7430/1: sizes.h: move from asm-generic to <linux/sizes.h>
    
    sizes.h is used throughout the AMBA code and drivers, so the header
    should be available to everyone in order to driver AMBA/PrimeCell
    peripherals behind a PCI bridge where the host can be any platform
    (I'm doing it under x86).
    
    At this step <asm-generic/sizes.h> includes <linux/sizes.h>,
    to allow a grace period for both in-tree and out-of-tree drivers.
    
    Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
    Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
    Acked-by: Linus Walleij <linus.walleij@linaro.org>
    Cc: Alan Cox <alan@linux.intel.com>
    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Provided that this is now officially part of the Kernel internal ABI,
we should not nack or revert changes on codes that would be using it.

> The alternative patch is this:
> 
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg53424.html

If this patch makes sense upstream (e. g. if is there any scenario where
linux/sizes.h is not implicitly included), then applying it would actually
be a fix, and such patch should be included.

Do you have any .config where such compilation breakage happen upstream?

If, otherwise, this is not the case, we should just fix it at the media
build, by either not compiling those drivers or by providing a backward
compatibility at compat.h.

Btw, just not compiling m5mols is likely the best approach, as I 
seriously doubt that anyone using this driver is using the media-build stuff[1].

> Note that the arm architecture will pull in linux/sizes.h, but the x86 arch
> doesn't, so this driver won't compile with x86. It's a real driver bug that
> has nothing to do with media_build.

Well, linux/sizes.h is not an arch-dependent header.

> 
> So you need to merge one of these two patches to fix this problem. I prefer
> the first, but the second is fine too.
> 

[1] Had you ever tested this driver? I was told that this device
doesn't even work with the upstreamed version.

Regards,
Mauro

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Hans Verkuil]

On Wed 10 October 2012 12:39:39 Mauro Carvalho Chehab wrote:
> Em Wed, 10 Oct 2012 08:27:26 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On Wed October 10 2012 03:05:30 Mauro Carvalho Chehab wrote:
> > > Em Mon, 8 Oct 2012 15:03:36 +0200
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > 
> > > > On Sun October 7 2012 13:13:36 Sylwester Nawrocki wrote:
> > > > > On 10/07/2012 03:19 AM, Michael West wrote:
> > > > > > This patch changes versions.txt and disables  VIDEO_M5MOLS which 
> > > > > > fixed the build for my 3.2 kernel but looking at the logs it looks
> > > > > > like this is not the way to fix it as it's not just a 3.6+ problem
> > > > > > as it does not build on 3.6 as well...  So probably best to find 
> > > > > > why it doesn't build on the current kernel first.
> > > > > 
> > > > > To fix the build on kernels 3.6+ <linux/sizes.h> just needs to be 
> > > > > inclcuded in m5mols.h. This is what my patch from previous message 
> > > > > in this thread does. But this will break again on kernel versions 
> > > > > _3.5 and lower_ where <linux/sizes.h> doesn't exist. I thought
> > > > > originally it could have been simply replaced there with <asm/sizes.h>, 
> > > > > but not all architectures have it
> > > > > 
> > > > > $ git grep  "#define SZ_1M" v2.6.32
> > > > > v2.6.32:arch/arm/include/asm/sizes.h:#define SZ_1M                           0x00100000
> > > > > v2.6.32:arch/sh/include/asm/sizes.h:#define SZ_1M                           0x00100000
> > > > > 
> > > > > $ git grep  "#define SZ_1M" v3.6-rc5
> > > > > v3.6-rc5:drivers/base/dma-contiguous.c:#define SZ_1M (1 << 20)
> > > > > v3.6-rc5:include/linux/sizes.h:#define SZ_1M                            0x00100000
> > > > > 
> > > > > 
> > > > > Let's just use the below patch to solve this build break, this way
> > > > > there is no need to touch anything at media_build.
> > > > > 
> > > > > From 11adc6956f3fe87c897aa6add08f8437422969a8 Mon Sep 17 00:00:00 2001
> > > > > From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > > > > Date: Sun, 7 Oct 2012 13:04:37 +0200
> > > > > Subject: [PATCH] m5mols: Replace SZ_1M with explicit value
> > > > > 
> > > > > SZ_1M macro definition was introduced in commit ab7ef22419927
> > > > > "[media] m5mols: Implement .get_frame_desc subdev callback"
> > > > > but required <linux/sizes.h> header was not included. To prevent
> > > > > build errors with older kernels where <linux/sizes.h> doesn't exist
> > > > > use explicit value rather than SZ_1M.
> > > > > 
> > > > > Reported-by: Jan Hoogenraad <jan-conceptronic@hoogenraad.net>
> > > > > Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > > > 
> > > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > 
> > > > Note: until this patch is merged I am disabling this driver in media_build
> > > > since right now it doesn't compile at all. Please notify me when this is
> > > > fixed in media_tree.git so that I can enable it again.
> > > > 
> > > > Regards,
> > > > 
> > > > 	Hans
> > > > 
> > > > > ---
> > > > >  drivers/media/i2c/m5mols/m5mols.h |    2 +-
> > > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
> > > > > index 4ab8b37..30654f5 100644
> > > > > --- a/drivers/media/i2c/m5mols/m5mols.h
> > > > > +++ b/drivers/media/i2c/m5mols/m5mols.h
> > > > > @@ -24,7 +24,7 @@
> > > > >   * determined by CAPP_JPEG_SIZE_MAX register.
> > > > >   */
> > > > >  #define M5MOLS_JPEG_TAGS_SIZE		0x20000
> > > > > -#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
> > > > > +#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * 1024 * 1024)
> > > 
> > > Nah! Please don't do that! we shouldn't be patching a driver upstream
> > > just because it broke media_build. Also, fixing it there is as simple as
> > > adding something similar to this at compat.h:
> > > 
> > > #ifndef SZ_1M
> > > 	#define SZ_1m (1024 * 1024)
> > > #endif
> > 
> > Actually, I prefer 1024 * 1024 over SZ_1M. 
> 
> I also think that this obfuscates the code, but the right place to discuss it is
> not here; it is with whomever is proposing those defines, and the ones
> that acked with it:
> 
> $ git log include/linux/sizes.h 
> commit dccd2304cc907c4b4d2920eeb24b055320fe942e
> Author: Alessandro Rubini <rubini@gnudd.com>
> Date:   Sun Jun 24 12:46:05 2012 +0100
> 
>     ARM: 7430/1: sizes.h: move from asm-generic to <linux/sizes.h>
>     
>     sizes.h is used throughout the AMBA code and drivers, so the header
>     should be available to everyone in order to driver AMBA/PrimeCell
>     peripherals behind a PCI bridge where the host can be any platform
>     (I'm doing it under x86).
>     
>     At this step <asm-generic/sizes.h> includes <linux/sizes.h>,
>     to allow a grace period for both in-tree and out-of-tree drivers.
>     
>     Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
>     Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
>     Acked-by: Linus Walleij <linus.walleij@linaro.org>
>     Cc: Alan Cox <alan@linux.intel.com>
>     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> Provided that this is now officially part of the Kernel internal ABI,
> we should not nack or revert changes on codes that would be using it.
> 
> > The alternative patch is this:
> > 
> > http://www.mail-archive.com/linux-media@vger.kernel.org/msg53424.html
> 
> If this patch makes sense upstream (e. g. if is there any scenario where
> linux/sizes.h is not implicitly included), then applying it would actually
> be a fix, and such patch should be included.
> 
> Do you have any .config where such compilation breakage happen upstream?

Just enable the sensor driver for x86. It will fail to compile in the for_v3.7
git branch. Again, this has nothing to do with the media_build. It's just a
missing include that breaks compilation unless you're compiling for arm.

> 
> If, otherwise, this is not the case, we should just fix it at the media
> build, by either not compiling those drivers or by providing a backward
> compatibility at compat.h.
> 
> Btw, just not compiling m5mols is likely the best approach, as I 
> seriously doubt that anyone using this driver is using the media-build stuff[1].
> 
> > Note that the arm architecture will pull in linux/sizes.h, but the x86 arch
> > doesn't, so this driver won't compile with x86. It's a real driver bug that
> > has nothing to do with media_build.
> 
> Well, linux/sizes.h is not an arch-dependent header.

linux/sizes.h is included by arch/arm/include/asm/memory.h, which is included
by other headers. So when compiling on arm, this header is pulled in for you,
when compiling on x86 you have to include it manually.

To fix this you either need to include <linux/sizes.h> explicitly, or stop using
SZ_1M. Forget about media_build, that's a red-herring. It's just a missing header
problem.

Regards,

	Hans

> 
> > 
> > So you need to merge one of these two patches to fix this problem. I prefer
> > the first, but the second is fine too.
> > 
> 
> [1] Had you ever tested this driver? I was told that this device
> doesn't even work with the upstreamed version.
> 
> Regards,
> Mauro
> 

Re: Media_build broken by [PATCH RFC v3 5/5] m5mols: Implement .get_frame_desc subdev callback

[Mauro Carvalho Chehab]

Em Wed, 10 Oct 2012 12:52:48 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Wed 10 October 2012 12:39:39 Mauro Carvalho Chehab wrote:
> > Em Wed, 10 Oct 2012 08:27:26 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > 
> > > On Wed October 10 2012 03:05:30 Mauro Carvalho Chehab wrote:
> > > > Em Mon, 8 Oct 2012 15:03:36 +0200
> > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > > 
> > > > > On Sun October 7 2012 13:13:36 Sylwester Nawrocki wrote:
> > > > > > On 10/07/2012 03:19 AM, Michael West wrote:
> > > > > > > This patch changes versions.txt and disables  VIDEO_M5MOLS which 
> > > > > > > fixed the build for my 3.2 kernel but looking at the logs it looks
> > > > > > > like this is not the way to fix it as it's not just a 3.6+ problem
> > > > > > > as it does not build on 3.6 as well...  So probably best to find 
> > > > > > > why it doesn't build on the current kernel first.
> > > > > > 
> > > > > > To fix the build on kernels 3.6+ <linux/sizes.h> just needs to be 
> > > > > > inclcuded in m5mols.h. This is what my patch from previous message 
> > > > > > in this thread does. But this will break again on kernel versions 
> > > > > > _3.5 and lower_ where <linux/sizes.h> doesn't exist. I thought
> > > > > > originally it could have been simply replaced there with <asm/sizes.h>, 
> > > > > > but not all architectures have it
> > > > > > 
> > > > > > $ git grep  "#define SZ_1M" v2.6.32
> > > > > > v2.6.32:arch/arm/include/asm/sizes.h:#define SZ_1M                           0x00100000
> > > > > > v2.6.32:arch/sh/include/asm/sizes.h:#define SZ_1M                           0x00100000
> > > > > > 
> > > > > > $ git grep  "#define SZ_1M" v3.6-rc5
> > > > > > v3.6-rc5:drivers/base/dma-contiguous.c:#define SZ_1M (1 << 20)
> > > > > > v3.6-rc5:include/linux/sizes.h:#define SZ_1M                            0x00100000
> > > > > > 
> > > > > > 
> > > > > > Let's just use the below patch to solve this build break, this way
> > > > > > there is no need to touch anything at media_build.
> > > > > > 
> > > > > > From 11adc6956f3fe87c897aa6add08f8437422969a8 Mon Sep 17 00:00:00 2001
> > > > > > From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > > > > > Date: Sun, 7 Oct 2012 13:04:37 +0200
> > > > > > Subject: [PATCH] m5mols: Replace SZ_1M with explicit value
> > > > > > 
> > > > > > SZ_1M macro definition was introduced in commit ab7ef22419927
> > > > > > "[media] m5mols: Implement .get_frame_desc subdev callback"
> > > > > > but required <linux/sizes.h> header was not included. To prevent
> > > > > > build errors with older kernels where <linux/sizes.h> doesn't exist
> > > > > > use explicit value rather than SZ_1M.
> > > > > > 
> > > > > > Reported-by: Jan Hoogenraad <jan-conceptronic@hoogenraad.net>
> > > > > > Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > > > > 
> > > > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > > 
> > > > > Note: until this patch is merged I am disabling this driver in media_build
> > > > > since right now it doesn't compile at all. Please notify me when this is
> > > > > fixed in media_tree.git so that I can enable it again.
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > 	Hans
> > > > > 
> > > > > > ---
> > > > > >  drivers/media/i2c/m5mols/m5mols.h |    2 +-
> > > > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/i2c/m5mols/m5mols.h b/drivers/media/i2c/m5mols/m5mols.h
> > > > > > index 4ab8b37..30654f5 100644
> > > > > > --- a/drivers/media/i2c/m5mols/m5mols.h
> > > > > > +++ b/drivers/media/i2c/m5mols/m5mols.h
> > > > > > @@ -24,7 +24,7 @@
> > > > > >   * determined by CAPP_JPEG_SIZE_MAX register.
> > > > > >   */
> > > > > >  #define M5MOLS_JPEG_TAGS_SIZE		0x20000
> > > > > > -#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
> > > > > > +#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * 1024 * 1024)
> > > > 
> > > > Nah! Please don't do that! we shouldn't be patching a driver upstream
> > > > just because it broke media_build. Also, fixing it there is as simple as
> > > > adding something similar to this at compat.h:
> > > > 
> > > > #ifndef SZ_1M
> > > > 	#define SZ_1m (1024 * 1024)
> > > > #endif
> > > 
> > > Actually, I prefer 1024 * 1024 over SZ_1M. 
> > 
> > I also think that this obfuscates the code, but the right place to discuss it is
> > not here; it is with whomever is proposing those defines, and the ones
> > that acked with it:
> > 
> > $ git log include/linux/sizes.h 
> > commit dccd2304cc907c4b4d2920eeb24b055320fe942e
> > Author: Alessandro Rubini <rubini@gnudd.com>
> > Date:   Sun Jun 24 12:46:05 2012 +0100
> > 
> >     ARM: 7430/1: sizes.h: move from asm-generic to <linux/sizes.h>
> >     
> >     sizes.h is used throughout the AMBA code and drivers, so the header
> >     should be available to everyone in order to driver AMBA/PrimeCell
> >     peripherals behind a PCI bridge where the host can be any platform
> >     (I'm doing it under x86).
> >     
> >     At this step <asm-generic/sizes.h> includes <linux/sizes.h>,
> >     to allow a grace period for both in-tree and out-of-tree drivers.
> >     
> >     Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
> >     Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
> >     Acked-by: Linus Walleij <linus.walleij@linaro.org>
> >     Cc: Alan Cox <alan@linux.intel.com>
> >     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > 
> > Provided that this is now officially part of the Kernel internal ABI,
> > we should not nack or revert changes on codes that would be using it.
> > 
> > > The alternative patch is this:
> > > 
> > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg53424.html
> > 
> > If this patch makes sense upstream (e. g. if is there any scenario where
> > linux/sizes.h is not implicitly included), then applying it would actually
> > be a fix, and such patch should be included.
> > 
> > Do you have any .config where such compilation breakage happen upstream?
> 
> Just enable the sensor driver for x86. It will fail to compile in the for_v3.7
> git branch. Again, this has nothing to do with the media_build. It's just a
> missing include that breaks compilation unless you're compiling for arm.
> 
> > 
> > If, otherwise, this is not the case, we should just fix it at the media
> > build, by either not compiling those drivers or by providing a backward
> > compatibility at compat.h.
> > 
> > Btw, just not compiling m5mols is likely the best approach, as I 
> > seriously doubt that anyone using this driver is using the media-build stuff[1].
> > 
> > > Note that the arm architecture will pull in linux/sizes.h, but the x86 arch
> > > doesn't, so this driver won't compile with x86. It's a real driver bug that
> > > has nothing to do with media_build.
> > 
> > Well, linux/sizes.h is not an arch-dependent header.
> 
> linux/sizes.h is included by arch/arm/include/asm/memory.h, which is included
> by other headers. So when compiling on arm, this header is pulled in for you,
> when compiling on x86 you have to include it manually.
> 
> To fix this you either need to include <linux/sizes.h> explicitly, or stop using
> SZ_1M. Forget about media_build, that's a red-herring. It's just a missing header
> problem.

Ok. Sylwester (or Hans), please send me the "include linux/sizes.h" variant then.

I suspect we'll see other "SZ_1M" patches in the future, so it makes no sense
to swim against the waves.

Regards,
Mauro