[RFC] v4l: Compressed data formats on the video bus

[RFC] v4l: Compressed data formats on the video bus

[Sylwester Nawrocki]

Hello,

This patch series tries to solve the issue presented in my other posting [1] by
extending the media bus format data structure with a new parameter, rather than
adding a new subdev video ops callback.

Although there are opinions [2] the 'framesamples' parameter should not be a part
of v4l2_mbus_framefmt data structure, it might be a better approach than creating 
a new subdev callback for a few reasons: 

 - the frame size parameter is usually needed altogether with other parameters
   already included in struct v4l2_mbus_framefmt;
 - it allows to clearly associate the frame length with the media entity pads;
 - the semantics is more straightforward and would yield simpler implementations,
   it's similar to current 'sizeimage' handling at struct v4l2_pix_format;
 - the applications could simply propagate the 'framesamples' value along the 
   pipeline, starting from the data source (sensor) subdev, only for compressed 
   data formats;


The new struct v4l2_mbus_framefmt would look as follows:

struct v4l2_mbus_framefmt {
	__u32			width;
	__u32			height;
	__u32			code;
	__u32			field;
	__u32			colorspace;
	__u32			framesamples;
	__u32			reserved[6];
};

The proposed semantics for the 'framesamples' parameter is roughly as follows:

 - the value is propagated at video pipeline entities where 'code' indicates
   compressed format;
 - the subdevs adjust the value if needed;
 - although currently there is only one compressed data format at the media 
   bus - V4L2_MBUS_FMT_JPEG_1X8 which corresponds to V4L2_PIX_FMT_JPEG and
   one sample at the media bus equals to one byte in memory, it is assumed
   that the host knows exactly what is framesamples/sizeimage ratio and it will 
   validate framesamples/sizeimage values before starting streaming;
 - the host will query internally a relevant subdev to properly handle 'sizeimage' 
   at the VIDIOC_TRY/S_FMT ioctl 
      

Comments ?

--- 
Regards, 
Sylwester Nawrocki 
Samsung Poland R&D Center

[1] http://www.spinics.net/lists/linux-media/msg39703.html 
[2] http://www.spinics.net/lists/linux-media/msg37703.html

[RFC/PATCH v1 1/3] v4l: Add new framesamples field to struct v4l2_mbus_framefmt

[Sylwester Nawrocki]

The purpose of the new field is to allow the video pipeline elements to
negotiate memory buffer size for compressed data frames, where the buffer
size cannot be derived from pixel width and height and the pixel code.

For VIDIOC_SUBDEV_S_FMT and VIDIOC_SUBDEV_G_FMT ioctls, the framesamples
parameter should be calculated by the driver from pixel width, height,
color format and other parameters if required and returned to the caller.
This applies to compressed data formats only.

The application should propagate the framesamples value, whatever returned
at the first sub-device within a data pipeline, i.e. at the pipeline's data
source.

For compressed data formats the host drivers should internally validate
the framesamples parameter values before streaming is enabled, to make sure
the memory buffer size requirements are satisfied along the pipeline.

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

diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml b/Documentation/DocBook/media/v4l/subdev-formats.xml
index 49c532e..d0827b4 100644
--- a/Documentation/DocBook/media/v4l/subdev-formats.xml
+++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
@@ -35,7 +35,12 @@
 	</row>
 	<row>
 	  <entry>__u32</entry>
-	  <entry><structfield>reserved</structfield>[7]</entry>
+	  <entry><structfield>framesamples</structfield></entry>
+	  <entry>Number of data samples on media bus per frame.</entry>
+	</row>
+	<row>
+	  <entry>__u32</entry>
+	  <entry><structfield>reserved</structfield>[6]</entry>
 	  <entry>Reserved for future extensions. Applications and drivers must
 	  set the array to zero.</entry>
 	</row>
diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
index 5ea7f75..ce776e8 100644
--- a/include/linux/v4l2-mediabus.h
+++ b/include/linux/v4l2-mediabus.h
@@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
  * @code:	data format code (from enum v4l2_mbus_pixelcode)
  * @field:	used interlacing type (from enum v4l2_field)
  * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
+ * @framesamples: number of data samples per frame
  */
 struct v4l2_mbus_framefmt {
 	__u32			width;
@@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
 	__u32			code;
 	__u32			field;
 	__u32			colorspace;
-	__u32			reserved[7];
+	__u32			framesamples;
+	__u32			reserved[6];
 };
 
 #endif
-- 
1.7.7.2

[RFC/PATCH v1 2/3] m5mols: Add buffer size configuration support for compressed data

[Sylwester Nawrocki]

Use new struct v4l2_mbus_framefmt framesamples field to return maximum
size of data transmitted per single frame. The framesamples value is
adjusted according to pixel format and configured in the ISP registers.

Except the pixel width and height, the frame size can also be made
dependent on JPEG quality ratio, when corresponding control is available
at the sub-device interface.

To ensure the pixel format is not changed after streaming is activated
the media entity 'stream_count' is checked before the format change
is attempted.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/m5mols/m5mols.h         |    8 ++++++++
 drivers/media/video/m5mols/m5mols_capture.c |    4 ++++
 drivers/media/video/m5mols/m5mols_core.c    |   18 ++++++++++++++++++
 drivers/media/video/m5mols/m5mols_reg.h     |    2 ++
 4 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/m5mols/m5mols.h b/drivers/media/video/m5mols/m5mols.h
index 82c8817..d729812 100644
--- a/drivers/media/video/m5mols/m5mols.h
+++ b/drivers/media/video/m5mols/m5mols.h
@@ -16,9 +16,17 @@
 #ifndef M5MOLS_H
 #define M5MOLS_H
 
+#include <asm/sizes.h>
 #include <media/v4l2-subdev.h>
 #include "m5mols_reg.h"
 
+/*
+ * This is an amount of data transmitted in addition to the configured
+ * JPEG_SIZE_MAX value.
+ */
+#define M5MOLS_JPEG_TAGS_SIZE		0x20000
+#define M5MOLS_MAIN_JPEG_SIZE_MAX	(5 * SZ_1M)
+
 extern int m5mols_debug;
 
 #define to_m5mols(__sd)	container_of(__sd, struct m5mols_info, sd)
diff --git a/drivers/media/video/m5mols/m5mols_capture.c b/drivers/media/video/m5mols/m5mols_capture.c
index 3248ac8..c8da22f 100644
--- a/drivers/media/video/m5mols/m5mols_capture.c
+++ b/drivers/media/video/m5mols/m5mols_capture.c
@@ -119,6 +119,7 @@ static int m5mols_capture_info(struct m5mols_info *info)
 
 int m5mols_start_capture(struct m5mols_info *info)
 {
+	struct v4l2_mbus_framefmt *mf = &info->ffmt[info->res_type];
 	struct v4l2_subdev *sd = &info->sd;
 	u8 resolution = info->resolution;
 	int timeout;
@@ -170,6 +171,9 @@ int m5mols_start_capture(struct m5mols_info *info)
 	if (!ret)
 		ret = m5mols_write(sd, CAPP_MAIN_IMAGE_SIZE, resolution);
 	if (!ret)
+		ret = m5mols_write(sd, CAPP_JPEG_SIZE_MAX,
+				   mf->framesamples - M5MOLS_JPEG_TAGS_SIZE);
+	if (!ret)
 		ret = m5mols_enable_interrupt(sd, REG_INT_CAPTURE);
 	if (!ret)
 		ret = m5mols_write(sd, CAPC_START, REG_CAP_START_MAIN);
diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
index b5957d7..995165c 100644
--- a/drivers/media/video/m5mols/m5mols_core.c
+++ b/drivers/media/video/m5mols/m5mols_core.c
@@ -505,6 +505,16 @@ static struct v4l2_mbus_framefmt *__find_format(struct m5mols_info *info,
 	return &info->ffmt[type];
 }
 
+static inline void m5mols_get_buffer_size(struct m5mols_info *info,
+					  struct v4l2_mbus_framefmt *mf)
+{
+	u32 min_sz = mf->width * mf->height + M5MOLS_JPEG_TAGS_SIZE;
+
+	/* Clamp provided value to the maximum needed */
+	mf->framesamples = clamp_t(u32, mf->framesamples, min_sz,
+				   M5MOLS_MAIN_JPEG_SIZE_MAX);
+}
+
 static int m5mols_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 			  struct v4l2_subdev_format *fmt)
 {
@@ -537,6 +547,14 @@ static int m5mols_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 	if (!sfmt)
 		return 0;
 
+	if (format->code == V4L2_MBUS_FMT_JPEG_1X8) {
+		mutex_lock(&sd->entity.parent->graph_mutex);
+		ret = sd->entity.stream_count;
+		mutex_unlock(&sd->entity.parent->graph_mutex);
+		if (ret > 0)
+			return -EBUSY;
+		m5mols_get_buffer_size(info, format);
+	}
 
 	format->code = m5mols_default_ffmt[type].code;
 	format->colorspace = V4L2_COLORSPACE_JPEG;
diff --git a/drivers/media/video/m5mols/m5mols_reg.h b/drivers/media/video/m5mols/m5mols_reg.h
index c755bd6..d44f5cc 100644
--- a/drivers/media/video/m5mols/m5mols_reg.h
+++ b/drivers/media/video/m5mols/m5mols_reg.h
@@ -342,6 +342,7 @@
  */
 #define CATB_YUVOUT_MAIN	0x00
 #define CATB_MAIN_IMAGE_SIZE	0x01
+#define CATB_JPEG_SIZE_MAX	0x0f
 #define CATB_MCC_MODE		0x1d
 #define CATB_WDR_EN		0x2c
 #define CATB_LIGHT_CTRL		0x40
@@ -354,6 +355,7 @@
 #define REG_JPEG		0x10
 
 #define CAPP_MAIN_IMAGE_SIZE	I2C_REG(CAT_CAPT_PARM, CATB_MAIN_IMAGE_SIZE, 1)
+#define CAPP_JPEG_SIZE_MAX	I2C_REG(CAT_CAPT_PARM, CATB_JPEG_SIZE_MAX, 4)
 
 #define CAPP_MCC_MODE		I2C_REG(CAT_CAPT_PARM, CATB_MCC_MODE, 1)
 #define REG_MCC_OFF		0x00
-- 
1.7.7.2

[RFC/PATCH v1 3/3] s5p-fimc: Add support for media bus framesamples parameter

[Sylwester Nawrocki]

The framesamples field of struct v4l2_mbus_framefmt is used to retrieve
an exact required maximum memory buffer size for a compressed data frame
from the sensor subdev. This allows to avoid allocating huge buffers
at the host driver.

To make sure the size of allocated buffers is correct for a subdev
configuration during VIDIOC_STREAMON ioctl, the video pipeline validation
has been extended with an additional check.

Flag FMT_FLAGS_COMPRESSED indicates the buffer size should to be determined
through struct v4l2_mbus_framefmt::framesamples parameter.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-capture.c |   53 +++++++++++++++++++++++++--
 drivers/media/video/s5p-fimc/fimc-core.c    |    7 +++-
 drivers/media/video/s5p-fimc/fimc-core.h    |    9 +++--
 3 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c
index 82d9ab6..c8c2bb0 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -246,6 +246,10 @@ static unsigned int get_plane_size(struct fimc_frame *fr, unsigned int plane)
 {
 	if (!fr || plane >= fr->fmt->memplanes)
 		return 0;
+
+	if (fimc_fmt_is_jpeg(fr->fmt->color))
+		return fr->payload[0];
+
 	return fr->f_width * fr->f_height * fr->fmt->depth[plane] / 8;
 }
 
@@ -718,6 +722,29 @@ static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
 	return 0;
 }
 
+/* Query the sensor for required buffer size (applicable to compressed data). */
+static int fimc_capture_get_sizeimage(struct fimc_dev *fimc, unsigned int *size)
+{
+	struct v4l2_subdev *sd = fimc->pipeline.sensor;
+	struct v4l2_subdev_format sfmt;
+	int ret;
+
+	sfmt.pad = 0;
+	sfmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sfmt);
+	if (ret < 0)
+		return ret;
+
+	if (sfmt.format.framesamples > FIMC_MAX_JPEG_BUF_SIZE) {
+		v4l2_err(sd, "Unsupported frame buffer size\n");
+		return -EINVAL;
+	}
+
+	*size = sfmt.format.framesamples;
+
+	return 0;
+}
+
 static int fimc_cap_g_fmt_mplane(struct file *file, void *fh,
 				 struct v4l2_format *f)
 {
@@ -770,7 +797,11 @@ static int fimc_cap_try_fmt_mplane(struct file *file, void *fh,
 	}
 
 	fimc_adjust_mplane_format(ffmt, pix->width, pix->height, pix);
-	return 0;
+
+	if (!(ffmt->flags & FMT_FLAGS_COMPRESSED))
+		return 0;
+
+	return fimc_capture_get_sizeimage(fimc, &pix->plane_fmt[0].sizeimage);
 }
 
 static void fimc_capture_mark_jpeg_xfer(struct fimc_ctx *ctx, bool jpeg)
@@ -827,9 +858,17 @@ static int fimc_capture_set_format(struct fimc_dev *fimc, struct v4l2_format *f)
 		pix->height = mf->height;
 	}
 	fimc_adjust_mplane_format(ff->fmt, pix->width, pix->height, pix);
-	for (i = 0; i < ff->fmt->colplanes; i++)
-		ff->payload[i] =
-			(pix->width * pix->height * ff->fmt->depth[i]) / 8;
+
+	if (ff->fmt->flags & FMT_FLAGS_COMPRESSED) {
+		ret = fimc_capture_get_sizeimage(fimc, &ff->payload[0]);
+		if (ret < 0)
+			return ret;
+		pix->plane_fmt[0].sizeimage = ff->payload[0];
+	} else {
+		for (i = 0; i < ff->fmt->colplanes; i++)
+			ff->payload[i] = pix->width * pix->height *
+					 ff->fmt->depth[i] / 8;
+	}
 
 	set_frame_bounds(ff, pix->width, pix->height);
 	/* Reset the composition rectangle if not yet configured */
@@ -938,6 +977,12 @@ 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.sensor &&
+		    src_fmt.format.code == V4L2_MBUS_FMT_JPEG_1X8 &&
+		    vid_cap->ctx->d_frame.payload[0] <
+		    src_fmt.format.framesamples)
+			return -EPIPE;
 	}
 	return 0;
 }
diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c
index 567e9ea..c30a219 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -167,7 +167,7 @@ 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,
 	},
 };
 
@@ -900,6 +900,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/video/s5p-fimc/fimc-core.h b/drivers/media/video/s5p-fimc/fimc-core.h
index c7f01c4..25b2a86 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.h
+++ b/drivers/media/video/s5p-fimc/fimc-core.h
@@ -17,6 +17,7 @@
 #include <linux/types.h>
 #include <linux/videodev2.h>
 #include <linux/io.h>
+#include <asm/sizes.h>
 
 #include <media/media-entity.h>
 #include <media/videobuf2-core.h>
@@ -44,6 +45,7 @@
 #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)
 
 /* indices to the clocks array */
 enum {
@@ -160,8 +162,9 @@ struct fimc_fmt {
 	u16	colplanes;
 	u8	depth[VIDEO_MAX_PLANES];
 	u16	flags;
-#define FMT_FLAGS_CAM	(1 << 0)
-#define FMT_FLAGS_M2M	(1 << 1)
+#define FMT_FLAGS_CAM		(1 << 0)
+#define FMT_FLAGS_M2M		(1 << 1)
+#define FMT_FLAGS_COMPRESSED	(1 << 2)
 };
 
 /**
@@ -279,7 +282,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;
-- 
1.7.7.2

Re: [RFC/PATCH v1 1/3] v4l: Add new framesamples field to struct v4l2_mbus_framefmt

[Hans Verkuil]

On Tuesday, November 22, 2011 10:55:38 Sylwester Nawrocki wrote:
> The purpose of the new field is to allow the video pipeline elements to
> negotiate memory buffer size for compressed data frames, where the buffer
> size cannot be derived from pixel width and height and the pixel code.
> 
> For VIDIOC_SUBDEV_S_FMT and VIDIOC_SUBDEV_G_FMT ioctls, the framesamples
> parameter should be calculated by the driver from pixel width, height,
> color format and other parameters if required and returned to the caller.
> This applies to compressed data formats only.
> 
> The application should propagate the framesamples value, whatever returned
> at the first sub-device within a data pipeline, i.e. at the pipeline's data
> source.
> 
> For compressed data formats the host drivers should internally validate
> the framesamples parameter values before streaming is enabled, to make sure
> the memory buffer size requirements are satisfied along the pipeline.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  Documentation/DocBook/media/v4l/subdev-formats.xml |    7 ++++++-
>  include/linux/v4l2-mediabus.h                      |    4 +++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml b/Documentation/DocBook/media/v4l/subdev-formats.xml
> index 49c532e..d0827b4 100644
> --- a/Documentation/DocBook/media/v4l/subdev-formats.xml
> +++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
> @@ -35,7 +35,12 @@
>  	</row>
>  	<row>
>  	  <entry>__u32</entry>
> -	  <entry><structfield>reserved</structfield>[7]</entry>
> +	  <entry><structfield>framesamples</structfield></entry>
> +	  <entry>Number of data samples on media bus per frame.</entry>

Is this the *maximum* number of data samples, or is this the *required* number
of data samples?

I think you mean 'maximum', but the documentation does not actually state that.

It should also clearly state that this field is used only for compressed
formats (right?). Should drivers be required to set this to 0 for uncompressed
formats?

Regards,

	Hans

> +	</row>
> +	<row>
> +	  <entry>__u32</entry>
> +	  <entry><structfield>reserved</structfield>[6]</entry>
>  	  <entry>Reserved for future extensions. Applications and drivers must
>  	  set the array to zero.</entry>
>  	</row>
> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
> index 5ea7f75..ce776e8 100644
> --- a/include/linux/v4l2-mediabus.h
> +++ b/include/linux/v4l2-mediabus.h
> @@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
>   * @code:	data format code (from enum v4l2_mbus_pixelcode)
>   * @field:	used interlacing type (from enum v4l2_field)
>   * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
> + * @framesamples: number of data samples per frame
>   */
>  struct v4l2_mbus_framefmt {
>  	__u32			width;
> @@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
>  	__u32			code;
>  	__u32			field;
>  	__u32			colorspace;
> -	__u32			reserved[7];
> +	__u32			framesamples;
> +	__u32			reserved[6];
>  };
>  
>  #endif
> 

Re: [RFC/PATCH v1 1/3] v4l: Add new framesamples field to struct v4l2_mbus_framefmt

[Sylwester Nawrocki]

On 11/22/2011 11:48 AM, Hans Verkuil wrote:
> On Tuesday, November 22, 2011 10:55:38 Sylwester Nawrocki wrote:
>> The purpose of the new field is to allow the video pipeline elements to
>> negotiate memory buffer size for compressed data frames, where the buffer
>> size cannot be derived from pixel width and height and the pixel code.
>>
>> For VIDIOC_SUBDEV_S_FMT and VIDIOC_SUBDEV_G_FMT ioctls, the framesamples
>> parameter should be calculated by the driver from pixel width, height,
>> color format and other parameters if required and returned to the caller.
>> This applies to compressed data formats only.
>>
>> The application should propagate the framesamples value, whatever returned
>> at the first sub-device within a data pipeline, i.e. at the pipeline's data
>> source.
>>
>> For compressed data formats the host drivers should internally validate
>> the framesamples parameter values before streaming is enabled, to make sure
>> the memory buffer size requirements are satisfied along the pipeline.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  Documentation/DocBook/media/v4l/subdev-formats.xml |    7 ++++++-
>>  include/linux/v4l2-mediabus.h                      |    4 +++-
>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml b/Documentation/DocBook/media/v4l/subdev-formats.xml
>> index 49c532e..d0827b4 100644
>> --- a/Documentation/DocBook/media/v4l/subdev-formats.xml
>> +++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
>> @@ -35,7 +35,12 @@
>>  	</row>
>>  	<row>
>>  	  <entry>__u32</entry>
>> -	  <entry><structfield>reserved</structfield>[7]</entry>
>> +	  <entry><structfield>framesamples</structfield></entry>
>> +	  <entry>Number of data samples on media bus per frame.</entry>
> 
> Is this the *maximum* number of data samples, or is this the *required* number
> of data samples?
> 
> I think you mean 'maximum', but the documentation does not actually state that.

I forgot to mention I intended to update the DocBook part after there is a
common agreement on the new field. 

And yes, it's meant to be the maximum number of data samples.

> 
> It should also clearly state that this field is used only for compressed
> formats (right?). Should drivers be required to set this to 0 for uncompressed
> formats?

Yes, it's only for compressed formats. I'll update the DocBook for the next
iteration so it contains the information from current changelogs.

I think it's reasonable to require the drivers to clear the field for raw formats.
I'm going to add this requirement in the next version of this patch.

> 
> Regards,
> 
> 	Hans
> 
>> +	</row>
>> +	<row>
>> +	  <entry>__u32</entry>
>> +	  <entry><structfield>reserved</structfield>[6]</entry>
>>  	  <entry>Reserved for future extensions. Applications and drivers must
>>  	  set the array to zero.</entry>
>>  	</row>
>> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
>> index 5ea7f75..ce776e8 100644
>> --- a/include/linux/v4l2-mediabus.h
>> +++ b/include/linux/v4l2-mediabus.h
>> @@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
>>   * @code:	data format code (from enum v4l2_mbus_pixelcode)
>>   * @field:	used interlacing type (from enum v4l2_field)
>>   * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
>> + * @framesamples: number of data samples per frame
>>   */
>>  struct v4l2_mbus_framefmt {
>>  	__u32			width;
>> @@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
>>  	__u32			code;
>>  	__u32			field;
>>  	__u32			colorspace;
>> -	__u32			reserved[7];
>> +	__u32			framesamples;
>> +	__u32			reserved[6];
>>  };
>>  
>>  #endif
>>

--
Regards,
Sylwester

Re: [RFC/PATCH v1 1/3] v4l: Add new framesamples field to struct v4l2_mbus_framefmt

[Tomasz Stanislawski]

Hi

On 11/22/2011 10:55 AM, Sylwester Nawrocki wrote:
> The purpose of the new field is to allow the video pipeline elements to
> negotiate memory buffer size for compressed data frames, where the buffer
> size cannot be derived from pixel width and height and the pixel code.
>
> For VIDIOC_SUBDEV_S_FMT and VIDIOC_SUBDEV_G_FMT ioctls, the framesamples
> parameter should be calculated by the driver from pixel width, height,
> color format and other parameters if required and returned to the caller.
> This applies to compressed data formats only.
>
> The application should propagate the framesamples value, whatever returned
> at the first sub-device within a data pipeline, i.e. at the pipeline's data
> source.
>
> For compressed data formats the host drivers should internally validate
> the framesamples parameter values before streaming is enabled, to make sure
> the memory buffer size requirements are satisfied along the pipeline.
>
> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> ---
>   Documentation/DocBook/media/v4l/subdev-formats.xml |    7 ++++++-
>   include/linux/v4l2-mediabus.h                      |    4 +++-
>   2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml b/Documentation/DocBook/media/v4l/subdev-formats.xml
> index 49c532e..d0827b4 100644
> --- a/Documentation/DocBook/media/v4l/subdev-formats.xml
> +++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
> @@ -35,7 +35,12 @@
>   	</row>
>   	<row>
>   	<entry>__u32</entry>
> -	<entry><structfield>reserved</structfield>[7]</entry>
> +	<entry><structfield>framesamples</structfield></entry>

Why you do not use name sizeimage?
It is used in struct v4l2_plane_pix_format and struct v4l2_pix_format?

Should old drivers be modified to update this field?

Best regards,
Tomasz Stanislawski

> +	<entry>Number of data samples on media bus per frame.</entry>
> +	</row>
> +	<row>
> +	<entry>__u32</entry>
> +	<entry><structfield>reserved</structfield>[6]</entry>
>   	<entry>Reserved for future extensions. Applications and drivers must
>   	  set the array to zero.</entry>
>   	</row>
> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
> index 5ea7f75..ce776e8 100644
> --- a/include/linux/v4l2-mediabus.h
> +++ b/include/linux/v4l2-mediabus.h
> @@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
>    * @code:	data format code (from enum v4l2_mbus_pixelcode)
>    * @field:	used interlacing type (from enum v4l2_field)
>    * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
> + * @framesamples: number of data samples per frame
>    */
>   struct v4l2_mbus_framefmt {
>   	__u32			width;
> @@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
>   	__u32			code;
>   	__u32			field;
>   	__u32			colorspace;
> -	__u32			reserved[7];
> +	__u32			framesamples;
> +	__u32			reserved[6];
>   };
>
>   #endif

Re: [RFC/PATCH v1 1/3] v4l: Add new framesamples field to struct v4l2_mbus_framefmt

[Sylwester Nawrocki]

Hi Tomasz,

On 11/22/2011 12:07 PM, Tomasz Stanislawski wrote:
> On 11/22/2011 10:55 AM, Sylwester Nawrocki wrote:
>> The purpose of the new field is to allow the video pipeline elements to
>> negotiate memory buffer size for compressed data frames, where the buffer
>> size cannot be derived from pixel width and height and the pixel code.
>>
>> For VIDIOC_SUBDEV_S_FMT and VIDIOC_SUBDEV_G_FMT ioctls, the framesamples
>> parameter should be calculated by the driver from pixel width, height,
>> color format and other parameters if required and returned to the caller.
>> This applies to compressed data formats only.
>>
>> The application should propagate the framesamples value, whatever returned
>> at the first sub-device within a data pipeline, i.e. at the pipeline's data
>> source.
>>
>> For compressed data formats the host drivers should internally validate
>> the framesamples parameter values before streaming is enabled, to make sure
>> the memory buffer size requirements are satisfied along the pipeline.
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   Documentation/DocBook/media/v4l/subdev-formats.xml |    7 ++++++-
>>   include/linux/v4l2-mediabus.h                      |    4 +++-
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml
>> b/Documentation/DocBook/media/v4l/subdev-formats.xml
>> index 49c532e..d0827b4 100644
>> --- a/Documentation/DocBook/media/v4l/subdev-formats.xml
>> +++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
>> @@ -35,7 +35,12 @@
>>       </row>
>>       <row>
>>       <entry>__u32</entry>
>> -    <entry><structfield>reserved</structfield>[7]</entry>
>> +    <entry><structfield>framesamples</structfield></entry>
> 
> Why you do not use name sizeimage?

The media bus format data structure describes data as seen on the media bus,
in general single subdevs might not know how data samples are translated into
data in memory. So, not in my opinion only, the name 'framesamples' is more 
appropriate, even though the translation of some media bus data formats into 
data in memory is straightforward.
We've discussed this roughly several times, e.g. during the Cambourne meeting.

> It is used in struct v4l2_plane_pix_format and struct v4l2_pix_format?
> 
> Should old drivers be modified to update this field?

I think the drivers that expose a sub-device node to user space might need to
be modified to set the framesamples field to 0 for raw formats. There is about
9 of them in the mainline AFAICS, and only two use compressed formats.


--
Regards,
Sylwester