[PATCH 00/14] Various soc_camera related fixes

[PATCH 00/14] Various soc_camera related fixes

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

I had a pile of straightforward fixes pending in one of my git branches
that were all related to soc_camera in some way. Some were part of William's
r-car patch series, but this collects them all in one place.

These fixes are all due to v4l2-compliance failures or warnings (well, except
of the DocBook patch at the end of course).

Regards,

	Hans

Hans Verkuil (14):
  sh-veu: initialize timestamp_flags and copy timestamp info
  sh-veu: don't use COLORSPACE_JPEG.
  tw9910: don't use COLORSPACE_JPEG
  tw9910: init priv->scale and update standard
  ak881x: simplify standard checks
  mt9t112: JPEG -> SRGB
  sh_mobile_ceu_camera: fix querycap
  sh_mobile_ceu_camera: set field to FIELD_NONE
  soc_camera: fix enum_input
  soc_camera: fix expbuf support
  soc_camera: compliance fixes
  soc_camera: pass on streamoff error
  soc_camera: always release queue for queue owner
  DocBook/media: fix bad spacing in VIDIOC_EXPBUF

 Documentation/DocBook/media/v4l/vidioc-expbuf.xml  | 38 ++++++++---------
 drivers/media/i2c/ak881x.c                         |  8 ++--
 drivers/media/i2c/soc_camera/mt9t112.c             |  8 ++--
 drivers/media/i2c/soc_camera/tw9910.c              | 35 ++++++++++++++--
 drivers/media/platform/sh_veu.c                    | 10 ++++-
 .../platform/soc_camera/sh_mobile_ceu_camera.c     |  3 ++
 drivers/media/platform/soc_camera/soc_camera.c     | 48 +++++++++++++---------
 7 files changed, 99 insertions(+), 51 deletions(-)

-- 
2.1.4

[PATCH 01/14] sh-veu: initialize timestamp_flags and copy timestamp info

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This field wasn't set, causing WARN_ON's from the vb2 core.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/sh_veu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c
index 2554f37..77a74d3 100644
--- a/drivers/media/platform/sh_veu.c
+++ b/drivers/media/platform/sh_veu.c
@@ -958,6 +958,7 @@ static int sh_veu_queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->ops = &sh_veu_qops;
 	src_vq->mem_ops = &vb2_dma_contig_memops;
 	src_vq->lock = &veu->fop_lock;
+	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 
 	ret = vb2_queue_init(src_vq);
 	if (ret < 0)
@@ -971,6 +972,7 @@ static int sh_veu_queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->ops = &sh_veu_qops;
 	dst_vq->mem_ops = &vb2_dma_contig_memops;
 	dst_vq->lock = &veu->fop_lock;
+	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 
 	return vb2_queue_init(dst_vq);
 }
@@ -1103,6 +1105,12 @@ static irqreturn_t sh_veu_isr(int irq, void *dev_id)
 	if (!src || !dst)
 		return IRQ_NONE;
 
+	dst->v4l2_buf.timestamp = src->v4l2_buf.timestamp;
+	dst->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+	dst->v4l2_buf.flags |=
+		src->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+	dst->v4l2_buf.timecode = src->v4l2_buf.timecode;
+
 	spin_lock(&veu->lock);
 	v4l2_m2m_buf_done(src, VB2_BUF_STATE_DONE);
 	v4l2_m2m_buf_done(dst, VB2_BUF_STATE_DONE);
-- 
2.1.4

[PATCH 02/14] sh-veu: don't use COLORSPACE_JPEG.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

COLORSPACE_JPEG should only be used for JPEGs. Use SMPTE170M instead,
which is how YCbCr images are usually encoded.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/sh_veu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c
index 77a74d3..f5e3eb3a 100644
--- a/drivers/media/platform/sh_veu.c
+++ b/drivers/media/platform/sh_veu.c
@@ -211,7 +211,7 @@ static enum v4l2_colorspace sh_veu_4cc2cspace(u32 fourcc)
 	case V4L2_PIX_FMT_NV12:
 	case V4L2_PIX_FMT_NV16:
 	case V4L2_PIX_FMT_NV24:
-		return V4L2_COLORSPACE_JPEG;
+		return V4L2_COLORSPACE_SMPTE170M;
 	case V4L2_PIX_FMT_RGB332:
 	case V4L2_PIX_FMT_RGB444:
 	case V4L2_PIX_FMT_RGB565:
-- 
2.1.4

[PATCH 03/14] tw9910: don't use COLORSPACE_JPEG

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This is an SDTV video receiver, so the colorspace should be SMPTE170M.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/soc_camera/tw9910.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
index 42bec9b..df66417 100644
--- a/drivers/media/i2c/soc_camera/tw9910.c
+++ b/drivers/media/i2c/soc_camera/tw9910.c
@@ -711,7 +711,7 @@ static int tw9910_get_fmt(struct v4l2_subdev *sd,
 	mf->width	= priv->scale->width;
 	mf->height	= priv->scale->height;
 	mf->code	= MEDIA_BUS_FMT_UYVY8_2X8;
-	mf->colorspace	= V4L2_COLORSPACE_JPEG;
+	mf->colorspace	= V4L2_COLORSPACE_SMPTE170M;
 	mf->field	= V4L2_FIELD_INTERLACED_BT;
 
 	return 0;
@@ -732,7 +732,7 @@ static int tw9910_s_fmt(struct v4l2_subdev *sd,
 	if (mf->code != MEDIA_BUS_FMT_UYVY8_2X8)
 		return -EINVAL;
 
-	mf->colorspace = V4L2_COLORSPACE_JPEG;
+	mf->colorspace = V4L2_COLORSPACE_SMPTE170M;
 
 	ret = tw9910_set_frame(sd, &width, &height);
 	if (!ret) {
@@ -762,7 +762,7 @@ static int tw9910_set_fmt(struct v4l2_subdev *sd,
 	}
 
 	mf->code = MEDIA_BUS_FMT_UYVY8_2X8;
-	mf->colorspace = V4L2_COLORSPACE_JPEG;
+	mf->colorspace = V4L2_COLORSPACE_SMPTE170M;
 
 	/*
 	 * select suitable norm
-- 
2.1.4

[PATCH 04/14] tw9910: init priv->scale and update standard

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When the standard changes the VACTIVE and VDELAY values need to be updated.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
index df66417..e939c24 100644
--- a/drivers/media/i2c/soc_camera/tw9910.c
+++ b/drivers/media/i2c/soc_camera/tw9910.c
@@ -510,13 +510,39 @@ static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct tw9910_priv *priv = to_tw9910(client);
+	const unsigned hact = 720;
+	const unsigned hdelay = 15;
+	unsigned vact;
+	unsigned vdelay;
+	int ret;
 
 	if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
 		return -EINVAL;
 
 	priv->norm = norm;
+	if (norm & V4L2_STD_525_60) {
+		vact = 240;
+		vdelay = 18;
+		ret = tw9910_mask_set(client, VVBI, 0x10, 0x10);
+	} else {
+		vact = 288;
+		vdelay = 24;
+		ret = tw9910_mask_set(client, VVBI, 0x10, 0x00);
+	}
+	if (!ret)
+		ret = i2c_smbus_write_byte_data(client, CROP_HI,
+			((vdelay >> 2) & 0xc0) |
+			((vact >> 4) & 0x30) |
+			((hdelay >> 6) & 0x0c) |
+			((hact >> 8) & 0x03));
+	if (!ret)
+		ret = i2c_smbus_write_byte_data(client, VDELAY_LO,
+			vdelay & 0xff);
+	if (!ret)
+		ret = i2c_smbus_write_byte_data(client, VACTIVE_LO,
+			vact & 0xff);
 
-	return 0;
+	return ret;
 }
 
 #ifdef CONFIG_VIDEO_ADV_DEBUG
@@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client)
 		 "tw9910 Product ID %0x:%0x\n", id, priv->revision);
 
 	priv->norm = V4L2_STD_NTSC;
+	priv->scale = &tw9910_ntsc_scales[0];
 
 done:
 	tw9910_s_power(&priv->subdev, 0);
-- 
2.1.4

[PATCH 05/14] ak881x: simplify standard checks

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Simplify confusing conditions. This also swaps the checks for NTSC and PAL:
to be consistent with other drivers check for NTSC first. So if the user
sets both NTSC and PAL bits, then NTSC wins.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/ak881x.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ak881x.c b/drivers/media/i2c/ak881x.c
index 2984624..d3b965e 100644
--- a/drivers/media/i2c/ak881x.c
+++ b/drivers/media/i2c/ak881x.c
@@ -156,12 +156,12 @@ static int ak881x_s_std_output(struct v4l2_subdev *sd, v4l2_std_id std)
 	} else if (std == V4L2_STD_PAL_60) {
 		vp1 = 7;
 		ak881x->lines = 480;
-	} else if (std && !(std & ~V4L2_STD_PAL)) {
-		vp1 = 0xf;
-		ak881x->lines = 576;
-	} else if (std && !(std & ~V4L2_STD_NTSC)) {
+	} else if (std & V4L2_STD_NTSC) {
 		vp1 = 0;
 		ak881x->lines = 480;
+	} else if (std & V4L2_STD_PAL) {
+		vp1 = 0xf;
+		ak881x->lines = 576;
 	} else {
 		/* No SECAM or PAL_N/Nc supported */
 		return -EINVAL;
-- 
2.1.4

[PATCH 06/14] mt9t112: JPEG -> SRGB

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The JPEG colorspace should only be used for JPEG encoded images. This is
just a regular sRGB sensor.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/soc_camera/mt9t112.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/mt9t112.c b/drivers/media/i2c/soc_camera/mt9t112.c
index de10a76..2f35d31 100644
--- a/drivers/media/i2c/soc_camera/mt9t112.c
+++ b/drivers/media/i2c/soc_camera/mt9t112.c
@@ -104,22 +104,22 @@ struct mt9t112_priv {
 static const struct mt9t112_format mt9t112_cfmts[] = {
 	{
 		.code		= MEDIA_BUS_FMT_UYVY8_2X8,
-		.colorspace	= V4L2_COLORSPACE_JPEG,
+		.colorspace	= V4L2_COLORSPACE_SRGB,
 		.fmt		= 1,
 		.order		= 0,
 	}, {
 		.code		= MEDIA_BUS_FMT_VYUY8_2X8,
-		.colorspace	= V4L2_COLORSPACE_JPEG,
+		.colorspace	= V4L2_COLORSPACE_SRGB,
 		.fmt		= 1,
 		.order		= 1,
 	}, {
 		.code		= MEDIA_BUS_FMT_YUYV8_2X8,
-		.colorspace	= V4L2_COLORSPACE_JPEG,
+		.colorspace	= V4L2_COLORSPACE_SRGB,
 		.fmt		= 1,
 		.order		= 2,
 	}, {
 		.code		= MEDIA_BUS_FMT_YVYU8_2X8,
-		.colorspace	= V4L2_COLORSPACE_JPEG,
+		.colorspace	= V4L2_COLORSPACE_SRGB,
 		.fmt		= 1,
 		.order		= 3,
 	}, {
-- 
2.1.4

[PATCH 07/14] sh_mobile_ceu_camera: fix querycap

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Fill in the bus_info and driver fields. Found by v4l2-compliance.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
index c5c6c4e..8881efe 100644
--- a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
+++ b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
@@ -1665,6 +1665,8 @@ static int sh_mobile_ceu_querycap(struct soc_camera_host *ici,
 				  struct v4l2_capability *cap)
 {
 	strlcpy(cap->card, "SuperH_Mobile_CEU", sizeof(cap->card));
+	strlcpy(cap->driver, "sh_mobile_ceu", sizeof(cap->driver));
+	strlcpy(cap->bus_info, "platform:sh_mobile_ceu", sizeof(cap->bus_info));
 	cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
 	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
 
-- 
2.1.4

[PATCH 08/14] sh_mobile_ceu_camera: set field to FIELD_NONE

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Make sure that 'field' isn't FIELD_ANY when the driver is
first loaded. Fixes a v4l2-compliance failure.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
index 8881efe..efdeea4 100644
--- a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
+++ b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
@@ -1775,6 +1775,7 @@ static int sh_mobile_ceu_probe(struct platform_device *pdev)
 		pcdev->max_height = pcdev->pdata->max_height;
 		pcdev->flags = pcdev->pdata->flags;
 	}
+	pcdev->field = V4L2_FIELD_NONE;
 
 	if (!pcdev->max_width) {
 		unsigned int v;
-- 
2.1.4

[PATCH 09/14] soc_camera: fix enum_input

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Fill in the std field from the video_device tvnorms field.

This fixes a v4l2-compliance failure.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/soc_camera/soc_camera.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index d708df4..f24062d 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -309,11 +309,14 @@ static int soc_camera_try_fmt_vid_cap(struct file *file, void *priv,
 static int soc_camera_enum_input(struct file *file, void *priv,
 				 struct v4l2_input *inp)
 {
+	struct soc_camera_device *icd = file->private_data;
+
 	if (inp->index != 0)
 		return -EINVAL;
 
 	/* default is camera */
 	inp->type = V4L2_INPUT_TYPE_CAMERA;
+	inp->std = icd->vdev->tvnorms;
 	strcpy(inp->name, "Camera");
 
 	return 0;
-- 
2.1.4

[PATCH 10/14] soc_camera: fix expbuf support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

- For vb1 drivers just return -ENOTTY.
- For vb2 drivers allow vb2_expbuf without there being a stream owner:
  the vb2_expbuf function will return the correct error message in that case.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/soc_camera/soc_camera.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index f24062d..5f1e5a8 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -470,14 +470,13 @@ static int soc_camera_expbuf(struct file *file, void *priv,
 	struct soc_camera_device *icd = file->private_data;
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 
-	if (icd->streamer != file)
-		return -EBUSY;
-
 	/* videobuf2 only */
 	if (ici->ops->init_videobuf)
-		return -EINVAL;
-	else
-		return vb2_expbuf(&icd->vb2_vidq, p);
+		return -ENOTTY;
+
+	if (icd->streamer && icd->streamer != file)
+		return -EBUSY;
+	return vb2_expbuf(&icd->vb2_vidq, p);
 }
 
 /* Always entered with .host_lock held */
-- 
2.1.4

[PATCH 11/14] soc_camera: compliance fixes

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

- REQBUFS(0) will stop streaming, free buffers and release the file ownership.
- Return ENOTTY for create_bufs for a vb1 driver
- Return EBUSY if there is a different streaming owner and set the new owner on
  success.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/soc_camera/soc_camera.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 5f1e5a8..6ce6576 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -384,9 +384,8 @@ static int soc_camera_reqbufs(struct file *file, void *priv,
 		ret = vb2_reqbufs(&icd->vb2_vidq, p);
 	}
 
-	if (!ret && !icd->streamer)
-		icd->streamer = file;
-
+	if (!ret)
+		icd->streamer = p->count ? file : NULL;
 	return ret;
 }
 
@@ -443,12 +442,19 @@ static int soc_camera_create_bufs(struct file *file, void *priv,
 {
 	struct soc_camera_device *icd = file->private_data;
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
+	int ret;
 
 	/* videobuf2 only */
 	if (ici->ops->init_videobuf)
-		return -EINVAL;
-	else
-		return vb2_create_bufs(&icd->vb2_vidq, create);
+		return -ENOTTY;
+
+	if (icd->streamer && icd->streamer != file)
+		return -EBUSY;
+
+	ret = vb2_create_bufs(&icd->vb2_vidq, create);
+	if (!ret)
+		icd->streamer = file;
+	return ret;
 }
 
 static int soc_camera_prepare_buf(struct file *file, void *priv,
-- 
2.1.4

[PATCH 12/14] soc_camera: pass on streamoff error

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If streamoff returned an error, then pass that on to the caller.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/soc_camera/soc_camera.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 6ce6576..0b09281 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1000,6 +1000,7 @@ static int soc_camera_streamoff(struct file *file, void *priv,
 	struct soc_camera_device *icd = file->private_data;
 	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
+	int ret;
 
 	WARN_ON(priv != file->private_data);
 
@@ -1014,13 +1015,13 @@ static int soc_camera_streamoff(struct file *file, void *priv,
 	 * remaining buffers. When the last buffer is freed, stop capture
 	 */
 	if (ici->ops->init_videobuf)
-		videobuf_streamoff(&icd->vb_vidq);
+		ret = videobuf_streamoff(&icd->vb_vidq);
 	else
-		vb2_streamoff(&icd->vb2_vidq, i);
+		ret = vb2_streamoff(&icd->vb2_vidq, i);
 
 	v4l2_subdev_call(sd, video, s_stream, 0);
 
-	return 0;
+	return ret;
 }
 
 static int soc_camera_cropcap(struct file *file, void *fh,
-- 
2.1.4

[PATCH 13/14] soc_camera: always release queue for queue owner

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Always release the queue if the owner closes its filehandle and not when
it is the last open filehandle.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/soc_camera/soc_camera.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 0b09281..9087fed 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -788,20 +788,21 @@ static int soc_camera_close(struct file *file)
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 
 	mutex_lock(&ici->host_lock);
+	if (icd->streamer == file) {
+		if (ici->ops->init_videobuf2)
+			vb2_queue_release(&icd->vb2_vidq);
+		icd->streamer = NULL;
+	}
 	icd->use_count--;
 	if (!icd->use_count) {
 		pm_runtime_suspend(&icd->vdev->dev);
 		pm_runtime_disable(&icd->vdev->dev);
 
-		if (ici->ops->init_videobuf2)
-			vb2_queue_release(&icd->vb2_vidq);
 		__soc_camera_power_off(icd);
 
 		soc_camera_remove_device(icd);
 	}
 
-	if (icd->streamer == file)
-		icd->streamer = NULL;
 	mutex_unlock(&ici->host_lock);
 
 	module_put(ici->ops->owner);
-- 
2.1.4

[PATCH 14/14] DocBook/media: fix bad spacing in VIDIOC_EXPBUF

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The VIDIOC_EXPBUF documentation had spurious spaces that made it irritating
to read. Fix this.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/DocBook/media/v4l/vidioc-expbuf.xml | 38 +++++++++++------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/vidioc-expbuf.xml b/Documentation/DocBook/media/v4l/vidioc-expbuf.xml
index a78c920..0ae0b6a 100644
--- a/Documentation/DocBook/media/v4l/vidioc-expbuf.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-expbuf.xml
@@ -62,28 +62,28 @@ buffer as a DMABUF file at any time after buffers have been allocated with the
 &VIDIOC-REQBUFS; ioctl.</para>
 
 <para> To export a buffer, applications fill &v4l2-exportbuffer;.  The
-<structfield> type </structfield> field is set to the same buffer type as was
-previously used with  &v4l2-requestbuffers;<structfield> type </structfield>.
-Applications must also set the <structfield> index </structfield> field. Valid
+<structfield>type</structfield> field is set to the same buffer type as was
+previously used with &v4l2-requestbuffers; <structfield>type</structfield>.
+Applications must also set the <structfield>index</structfield> field. Valid
 index numbers range from zero to the number of buffers allocated with
-&VIDIOC-REQBUFS; (&v4l2-requestbuffers;<structfield> count </structfield>)
-minus one.  For the multi-planar API, applications set the <structfield> plane
-</structfield> field to the index of the plane to be exported. Valid planes
+&VIDIOC-REQBUFS; (&v4l2-requestbuffers; <structfield>count</structfield>)
+minus one.  For the multi-planar API, applications set the <structfield>plane</structfield>
+field to the index of the plane to be exported. Valid planes
 range from zero to the maximal number of valid planes for the currently active
-format. For the single-planar API, applications must set <structfield> plane
-</structfield> to zero.  Additional flags may be posted in the <structfield>
-flags </structfield> field.  Refer to a manual for open() for details.
+format. For the single-planar API, applications must set <structfield>plane</structfield>
+to zero.  Additional flags may be posted in the <structfield>flags</structfield>
+field.  Refer to a manual for open() for details.
 Currently only O_CLOEXEC, O_RDONLY, O_WRONLY, and O_RDWR are supported.  All
 other fields must be set to zero.
 In the case of multi-planar API, every plane is exported separately using
-multiple <constant> VIDIOC_EXPBUF </constant> calls. </para>
+multiple <constant>VIDIOC_EXPBUF</constant> calls.</para>
 
-<para> After calling <constant>VIDIOC_EXPBUF</constant> the <structfield> fd
-</structfield> field will be set by a driver.  This is a DMABUF file
+<para>After calling <constant>VIDIOC_EXPBUF</constant> the <structfield>fd</structfield>
+field will be set by a driver.  This is a DMABUF file
 descriptor. The application may pass it to other DMABUF-aware devices. Refer to
 <link linkend="dmabuf">DMABUF importing</link> for details about importing
 DMABUF files into V4L2 nodes. It is recommended to close a DMABUF file when it
-is no longer used to allow the associated memory to be reclaimed. </para>
+is no longer used to allow the associated memory to be reclaimed.</para>
   </refsect1>
 
   <refsect1>
@@ -170,9 +170,9 @@ multi-planar API. Otherwise this value must be set to zero. </entry>
 	  <row>
 	    <entry>__u32</entry>
 	    <entry><structfield>flags</structfield></entry>
-	    <entry>Flags for the newly created file, currently only <constant>
-O_CLOEXEC </constant>, <constant>O_RDONLY</constant>, <constant>O_WRONLY
-</constant>, and <constant>O_RDWR</constant> are supported, refer to the manual
+	    <entry>Flags for the newly created file, currently only
+<constant>O_CLOEXEC</constant>, <constant>O_RDONLY</constant>, <constant>O_WRONLY</constant>,
+and <constant>O_RDWR</constant> are supported, refer to the manual
 of open() for more details.</entry>
 	  </row>
 	  <row>
@@ -200,9 +200,9 @@ set the array to zero.</entry>
 	<term><errorcode>EINVAL</errorcode></term>
 	<listitem>
 	  <para>A queue is not in MMAP mode or DMABUF exporting is not
-supported or <structfield> flags </structfield> or <structfield> type
-</structfield> or <structfield> index </structfield> or <structfield> plane
-</structfield> fields are invalid.</para>
+supported or <structfield>flags</structfield> or <structfield>type</structfield>
+or <structfield>index</structfield> or <structfield>plane</structfield> fields
+are invalid.</para>
 	</listitem>
       </varlistentry>
     </variablelist>
-- 
2.1.4

Re: [PATCH 02/14] sh-veu: don't use COLORSPACE_JPEG.

[Guennadi Liakhovetski]

Hi Hans,

I'm not maintaining this driver, so, just

On Mon, 15 Jun 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> COLORSPACE_JPEG should only be used for JPEGs. Use SMPTE170M instead,
> which is how YCbCr images are usually encoded.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

> ---
>  drivers/media/platform/sh_veu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c
> index 77a74d3..f5e3eb3a 100644
> --- a/drivers/media/platform/sh_veu.c
> +++ b/drivers/media/platform/sh_veu.c
> @@ -211,7 +211,7 @@ static enum v4l2_colorspace sh_veu_4cc2cspace(u32 fourcc)
>  	case V4L2_PIX_FMT_NV12:
>  	case V4L2_PIX_FMT_NV16:
>  	case V4L2_PIX_FMT_NV24:
> -		return V4L2_COLORSPACE_JPEG;
> +		return V4L2_COLORSPACE_SMPTE170M;
>  	case V4L2_PIX_FMT_RGB332:
>  	case V4L2_PIX_FMT_RGB444:
>  	case V4L2_PIX_FMT_RGB565:
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in

Re: [PATCH 01/14] sh-veu: initialize timestamp_flags and copy timestamp info

[Guennadi Liakhovetski]

On Mon, 15 Jun 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This field wasn't set, causing WARN_ON's from the vb2 core.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

> ---
>  drivers/media/platform/sh_veu.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c
> index 2554f37..77a74d3 100644
> --- a/drivers/media/platform/sh_veu.c
> +++ b/drivers/media/platform/sh_veu.c
> @@ -958,6 +958,7 @@ static int sh_veu_queue_init(void *priv, struct vb2_queue *src_vq,
>  	src_vq->ops = &sh_veu_qops;
>  	src_vq->mem_ops = &vb2_dma_contig_memops;
>  	src_vq->lock = &veu->fop_lock;
> +	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>  
>  	ret = vb2_queue_init(src_vq);
>  	if (ret < 0)
> @@ -971,6 +972,7 @@ static int sh_veu_queue_init(void *priv, struct vb2_queue *src_vq,
>  	dst_vq->ops = &sh_veu_qops;
>  	dst_vq->mem_ops = &vb2_dma_contig_memops;
>  	dst_vq->lock = &veu->fop_lock;
> +	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>  
>  	return vb2_queue_init(dst_vq);
>  }
> @@ -1103,6 +1105,12 @@ static irqreturn_t sh_veu_isr(int irq, void *dev_id)
>  	if (!src || !dst)
>  		return IRQ_NONE;
>  
> +	dst->v4l2_buf.timestamp = src->v4l2_buf.timestamp;
> +	dst->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +	dst->v4l2_buf.flags |=
> +		src->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +	dst->v4l2_buf.timecode = src->v4l2_buf.timecode;
> +
>  	spin_lock(&veu->lock);
>  	v4l2_m2m_buf_done(src, VB2_BUF_STATE_DONE);
>  	v4l2_m2m_buf_done(dst, VB2_BUF_STATE_DONE);
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in

Re: [PATCH 04/14] tw9910: init priv->scale and update standard

[Guennadi Liakhovetski]

On Mon, 15 Jun 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> When the standard changes the VACTIVE and VDELAY values need to be updated.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
> index df66417..e939c24 100644
> --- a/drivers/media/i2c/soc_camera/tw9910.c
> +++ b/drivers/media/i2c/soc_camera/tw9910.c
> @@ -510,13 +510,39 @@ static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct tw9910_priv *priv = to_tw9910(client);
> +	const unsigned hact = 720;
> +	const unsigned hdelay = 15;
> +	unsigned vact;
> +	unsigned vdelay;
> +	int ret;
>  
>  	if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
>  		return -EINVAL;
>  
>  	priv->norm = norm;
> +	if (norm & V4L2_STD_525_60) {
> +		vact = 240;
> +		vdelay = 18;
> +		ret = tw9910_mask_set(client, VVBI, 0x10, 0x10);
> +	} else {
> +		vact = 288;
> +		vdelay = 24;
> +		ret = tw9910_mask_set(client, VVBI, 0x10, 0x00);
> +	}
> +	if (!ret)
> +		ret = i2c_smbus_write_byte_data(client, CROP_HI,
> +			((vdelay >> 2) & 0xc0) |
> +			((vact >> 4) & 0x30) |
> +			((hdelay >> 6) & 0x0c) |
> +			((hact >> 8) & 0x03));

I personally would find ((x & 0xc0) >> {2,4,6,8}) a bit easier for the 
eyes, but this works as well for me:)

> +	if (!ret)
> +		ret = i2c_smbus_write_byte_data(client, VDELAY_LO,
> +			vdelay & 0xff);
> +	if (!ret)
> +		ret = i2c_smbus_write_byte_data(client, VACTIVE_LO,
> +			vact & 0xff);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
> @@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client)
>  		 "tw9910 Product ID %0x:%0x\n", id, priv->revision);
>  
>  	priv->norm = V4L2_STD_NTSC;
> +	priv->scale = &tw9910_ntsc_scales[0];

Why do you need this? So far everywhere in the code priv->scale is either 
checked or set before use. Don't see why an additional initialisation is 
needed.

Thanks
Guennadi

>  
>  done:
>  	tw9910_s_power(&priv->subdev, 0);
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in

Re: [PATCH 04/14] tw9910: init priv->scale and update standard

[Hans Verkuil]

On 06/21/2015 07:23 PM, Guennadi Liakhovetski wrote:
> On Mon, 15 Jun 2015, Hans Verkuil wrote:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> When the standard changes the VACTIVE and VDELAY values need to be updated.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
>> index df66417..e939c24 100644
>> --- a/drivers/media/i2c/soc_camera/tw9910.c
>> +++ b/drivers/media/i2c/soc_camera/tw9910.c
>> @@ -510,13 +510,39 @@ static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
>>  {
>>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>  	struct tw9910_priv *priv = to_tw9910(client);
>> +	const unsigned hact = 720;
>> +	const unsigned hdelay = 15;
>> +	unsigned vact;
>> +	unsigned vdelay;
>> +	int ret;
>>  
>>  	if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
>>  		return -EINVAL;
>>  
>>  	priv->norm = norm;
>> +	if (norm & V4L2_STD_525_60) {
>> +		vact = 240;
>> +		vdelay = 18;
>> +		ret = tw9910_mask_set(client, VVBI, 0x10, 0x10);
>> +	} else {
>> +		vact = 288;
>> +		vdelay = 24;
>> +		ret = tw9910_mask_set(client, VVBI, 0x10, 0x00);
>> +	}
>> +	if (!ret)
>> +		ret = i2c_smbus_write_byte_data(client, CROP_HI,
>> +			((vdelay >> 2) & 0xc0) |
>> +			((vact >> 4) & 0x30) |
>> +			((hdelay >> 6) & 0x0c) |
>> +			((hact >> 8) & 0x03));
> 
> I personally would find ((x & 0xc0) >> {2,4,6,8}) a bit easier for the 
> eyes, but this works as well for me:)
> 
>> +	if (!ret)
>> +		ret = i2c_smbus_write_byte_data(client, VDELAY_LO,
>> +			vdelay & 0xff);
>> +	if (!ret)
>> +		ret = i2c_smbus_write_byte_data(client, VACTIVE_LO,
>> +			vact & 0xff);
>>  
>> -	return 0;
>> +	return ret;
>>  }
>>  
>>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>> @@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client)
>>  		 "tw9910 Product ID %0x:%0x\n", id, priv->revision);
>>  
>>  	priv->norm = V4L2_STD_NTSC;
>> +	priv->scale = &tw9910_ntsc_scales[0];
> 
> Why do you need this? So far everywhere in the code priv->scale is either 
> checked or set before use. Don't see why an additional initialisation is 
> needed.

If you just start streaming without explicitly setting up formats (which is
allowed), then priv->scale is still NULL.

V4L2 always assumes that there is some initial format configured, and this line
enables that for this driver (NTSC).

Regards,

	Hans

> 
> Thanks
> Guennadi
> 
>>  
>>  done:
>>  	tw9910_s_power(&priv->subdev, 0);
>> -- 
>> 2.1.4
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in

Re: [PATCH 04/14] tw9910: init priv->scale and update standard

[Guennadi Liakhovetski]

Hi Hans,

On Mon, 22 Jun 2015, Hans Verkuil wrote:

> On 06/21/2015 07:23 PM, Guennadi Liakhovetski wrote:
> > On Mon, 15 Jun 2015, Hans Verkuil wrote:
> > 
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> When the standard changes the VACTIVE and VDELAY values need to be updated.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >>  drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++-
> >>  1 file changed, 28 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
> >> index df66417..e939c24 100644
> >> --- a/drivers/media/i2c/soc_camera/tw9910.c
> >> +++ b/drivers/media/i2c/soc_camera/tw9910.c
> >> @@ -510,13 +510,39 @@ static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
> >>  {
> >>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >>  	struct tw9910_priv *priv = to_tw9910(client);
> >> +	const unsigned hact = 720;
> >> +	const unsigned hdelay = 15;
> >> +	unsigned vact;
> >> +	unsigned vdelay;
> >> +	int ret;
> >>  
> >>  	if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
> >>  		return -EINVAL;
> >>  
> >>  	priv->norm = norm;
> >> +	if (norm & V4L2_STD_525_60) {
> >> +		vact = 240;
> >> +		vdelay = 18;
> >> +		ret = tw9910_mask_set(client, VVBI, 0x10, 0x10);
> >> +	} else {
> >> +		vact = 288;
> >> +		vdelay = 24;
> >> +		ret = tw9910_mask_set(client, VVBI, 0x10, 0x00);
> >> +	}
> >> +	if (!ret)
> >> +		ret = i2c_smbus_write_byte_data(client, CROP_HI,
> >> +			((vdelay >> 2) & 0xc0) |
> >> +			((vact >> 4) & 0x30) |
> >> +			((hdelay >> 6) & 0x0c) |
> >> +			((hact >> 8) & 0x03));
> > 
> > I personally would find ((x & 0xc0) >> {2,4,6,8}) a bit easier for the 
> > eyes, but this works as well for me:)
> > 
> >> +	if (!ret)
> >> +		ret = i2c_smbus_write_byte_data(client, VDELAY_LO,
> >> +			vdelay & 0xff);
> >> +	if (!ret)
> >> +		ret = i2c_smbus_write_byte_data(client, VACTIVE_LO,
> >> +			vact & 0xff);
> >>  
> >> -	return 0;
> >> +	return ret;
> >>  }
> >>  
> >>  #ifdef CONFIG_VIDEO_ADV_DEBUG
> >> @@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client)
> >>  		 "tw9910 Product ID %0x:%0x\n", id, priv->revision);
> >>  
> >>  	priv->norm = V4L2_STD_NTSC;
> >> +	priv->scale = &tw9910_ntsc_scales[0];
> > 
> > Why do you need this? So far everywhere in the code priv->scale is either 
> > checked or set before use. Don't see why an additional initialisation is 
> > needed.
> 
> If you just start streaming without explicitly setting up formats (which is
> allowed), then priv->scale is still NULL.

Yes, it can well be NULL, but it is also unused. Before it is used it will 
be set, while it is unused it is allowed to stay NULL.

Thanks
Guennadi

> V4L2 always assumes that there is some initial format configured, and this line
> enables that for this driver (NTSC).
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Thanks
> > Guennadi
> > 
> >>  
> >>  done:
> >>  	tw9910_s_power(&priv->subdev, 0);
> >> -- 
> >> 2.1.4
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in

Re: [PATCH 04/14] tw9910: init priv->scale and update standard

[Hans Verkuil]

On 06/22/2015 09:21 AM, Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> On Mon, 22 Jun 2015, Hans Verkuil wrote:
> 
>> On 06/21/2015 07:23 PM, Guennadi Liakhovetski wrote:
>>> On Mon, 15 Jun 2015, Hans Verkuil wrote:
>>>
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> When the standard changes the VACTIVE and VDELAY values need to be updated.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>>  drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++-
>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
>>>> index df66417..e939c24 100644
>>>> --- a/drivers/media/i2c/soc_camera/tw9910.c
>>>> +++ b/drivers/media/i2c/soc_camera/tw9910.c
>>>> @@ -510,13 +510,39 @@ static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
>>>>  {
>>>>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>>  	struct tw9910_priv *priv = to_tw9910(client);
>>>> +	const unsigned hact = 720;
>>>> +	const unsigned hdelay = 15;
>>>> +	unsigned vact;
>>>> +	unsigned vdelay;
>>>> +	int ret;
>>>>  
>>>>  	if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
>>>>  		return -EINVAL;
>>>>  
>>>>  	priv->norm = norm;
>>>> +	if (norm & V4L2_STD_525_60) {
>>>> +		vact = 240;
>>>> +		vdelay = 18;
>>>> +		ret = tw9910_mask_set(client, VVBI, 0x10, 0x10);
>>>> +	} else {
>>>> +		vact = 288;
>>>> +		vdelay = 24;
>>>> +		ret = tw9910_mask_set(client, VVBI, 0x10, 0x00);
>>>> +	}
>>>> +	if (!ret)
>>>> +		ret = i2c_smbus_write_byte_data(client, CROP_HI,
>>>> +			((vdelay >> 2) & 0xc0) |
>>>> +			((vact >> 4) & 0x30) |
>>>> +			((hdelay >> 6) & 0x0c) |
>>>> +			((hact >> 8) & 0x03));
>>>
>>> I personally would find ((x & 0xc0) >> {2,4,6,8}) a bit easier for the 
>>> eyes, but this works as well for me:)
>>>
>>>> +	if (!ret)
>>>> +		ret = i2c_smbus_write_byte_data(client, VDELAY_LO,
>>>> +			vdelay & 0xff);
>>>> +	if (!ret)
>>>> +		ret = i2c_smbus_write_byte_data(client, VACTIVE_LO,
>>>> +			vact & 0xff);
>>>>  
>>>> -	return 0;
>>>> +	return ret;
>>>>  }
>>>>  
>>>>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>>>> @@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client)
>>>>  		 "tw9910 Product ID %0x:%0x\n", id, priv->revision);
>>>>  
>>>>  	priv->norm = V4L2_STD_NTSC;
>>>> +	priv->scale = &tw9910_ntsc_scales[0];
>>>
>>> Why do you need this? So far everywhere in the code priv->scale is either 
>>> checked or set before use. Don't see why an additional initialisation is 
>>> needed.
>>
>> If you just start streaming without explicitly setting up formats (which is
>> allowed), then priv->scale is still NULL.
> 
> Yes, it can well be NULL, but it is also unused. Before it is used it will 
> be set, while it is unused it is allowed to stay NULL.

No. If you start streaming without the set_fmt op having been called, then
s_stream will return an error since priv->scale is NULL. This is wrong. Since
this driver defaults to NTSC the initial setup should be for NTSC and it should
be ready for streaming.

So priv->scale *is* used: in s_stream. And it is not necessarily set before use.
E.g. if you load the driver and run 'v4l2-ctl --stream-out-mmap' you will hit this
case. It's how I found this bug.

It's a trivial one liner to ensure a valid priv->scale pointer.

Regards,

	Hans

> 
> Thanks
> Guennadi
> 
>> V4L2 always assumes that there is some initial format configured, and this line
>> enables that for this driver (NTSC).
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Thanks
>>> Guennadi
>>>
>>>>  
>>>>  done:
>>>>  	tw9910_s_power(&priv->subdev, 0);
>>>> -- 
>>>> 2.1.4
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in

Re: [PATCH 04/14] tw9910: init priv->scale and update standard

[Hans Verkuil]

Guennadi,

I want to make a pull request for this patch series. This patch is the only
outstanding one. Or do you have to review more patches? I only got Acks for
patches 1 and 2.

Regards,

	Hans

On 06/22/2015 09:29 AM, Hans Verkuil wrote:
> On 06/22/2015 09:21 AM, Guennadi Liakhovetski wrote:
>> Hi Hans,
>>
>> On Mon, 22 Jun 2015, Hans Verkuil wrote:
>>
>>> On 06/21/2015 07:23 PM, Guennadi Liakhovetski wrote:
>>>> On Mon, 15 Jun 2015, Hans Verkuil wrote:
>>>>
>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>
>>>>> When the standard changes the VACTIVE and VDELAY values need to be updated.
>>>>>
>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>> ---
>>>>>  drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++-
>>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
>>>>> index df66417..e939c24 100644
>>>>> --- a/drivers/media/i2c/soc_camera/tw9910.c
>>>>> +++ b/drivers/media/i2c/soc_camera/tw9910.c
>>>>> @@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client)
>>>>>  		 "tw9910 Product ID %0x:%0x\n", id, priv->revision);
>>>>>  
>>>>>  	priv->norm = V4L2_STD_NTSC;
>>>>> +	priv->scale = &tw9910_ntsc_scales[0];
>>>>
>>>> Why do you need this? So far everywhere in the code priv->scale is either 
>>>> checked or set before use. Don't see why an additional initialisation is 
>>>> needed.
>>>
>>> If you just start streaming without explicitly setting up formats (which is
>>> allowed), then priv->scale is still NULL.
>>
>> Yes, it can well be NULL, but it is also unused. Before it is used it will 
>> be set, while it is unused it is allowed to stay NULL.
> 
> No. If you start streaming without the set_fmt op having been called, then
> s_stream will return an error since priv->scale is NULL. This is wrong. Since
> this driver defaults to NTSC the initial setup should be for NTSC and it should
> be ready for streaming.
> 
> So priv->scale *is* used: in s_stream. And it is not necessarily set before use.
> E.g. if you load the driver and run 'v4l2-ctl --stream-out-mmap' you will hit this
> case. It's how I found this bug.
> 
> It's a trivial one liner to ensure a valid priv->scale pointer.
> 
> Regards,
> 
> 	Hans
> 
>>
>> Thanks
>> Guennadi
>>
>>> V4L2 always assumes that there is some initial format configured, and this line
>>> enables that for this driver (NTSC).
>>>
>>> Regards,
>>>
>>> 	Hans

Re: [PATCH 04/14] tw9910: init priv->scale and update standard

[Guennadi Liakhovetski]

Hi Hans,

On Fri, 24 Jul 2015, Hans Verkuil wrote:

> Guennadi,
> 
> I want to make a pull request for this patch series. This patch is the only
> outstanding one.

Right, sorry for the delay. Replying to your explanation below:

> Or do you have to review more patches? I only got Acks for patches 1 and 
> 2.
> 
> Regards,
> 
> 	Hans
> 
> On 06/22/2015 09:29 AM, Hans Verkuil wrote:
> > On 06/22/2015 09:21 AM, Guennadi Liakhovetski wrote:
> >> Hi Hans,
> >>
> >> On Mon, 22 Jun 2015, Hans Verkuil wrote:
> >>
> >>> On 06/21/2015 07:23 PM, Guennadi Liakhovetski wrote:
> >>>> On Mon, 15 Jun 2015, Hans Verkuil wrote:
> >>>>
> >>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>>
> >>>>> When the standard changes the VACTIVE and VDELAY values need to be updated.
> >>>>>
> >>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>> ---
> >>>>>  drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++-
> >>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
> >>>>> index df66417..e939c24 100644
> >>>>> --- a/drivers/media/i2c/soc_camera/tw9910.c
> >>>>> +++ b/drivers/media/i2c/soc_camera/tw9910.c
> >>>>> @@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client)
> >>>>>  		 "tw9910 Product ID %0x:%0x\n", id, priv->revision);
> >>>>>  
> >>>>>  	priv->norm = V4L2_STD_NTSC;
> >>>>> +	priv->scale = &tw9910_ntsc_scales[0];
> >>>>
> >>>> Why do you need this? So far everywhere in the code priv->scale is either 
> >>>> checked or set before use. Don't see why an additional initialisation is 
> >>>> needed.
> >>>
> >>> If you just start streaming without explicitly setting up formats (which is
> >>> allowed), then priv->scale is still NULL.
> >>
> >> Yes, it can well be NULL, but it is also unused. Before it is used it will 
> >> be set, while it is unused it is allowed to stay NULL.
> > 
> > No. If you start streaming without the set_fmt op having been called, then
> > s_stream will return an error since priv->scale is NULL. This is wrong. Since
> > this driver defaults to NTSC the initial setup should be for NTSC and it should
> > be ready for streaming.
> > 
> > So priv->scale *is* used: in s_stream. And it is not necessarily set before use.
> > E.g. if you load the driver and run 'v4l2-ctl --stream-out-mmap' you will hit this
> > case. It's how I found this bug.
> > 
> > It's a trivial one liner to ensure a valid priv->scale pointer.

Yes, you're right, now I see how this can happen. But there's also another 
possibility: if S_FMT fails priv->scale will be set to NULL. If you then 
directly start streaming wouldn't the same problem arise? Or is it valid 
to fail STREAMON after a failed S_FMT? If it is, then of course

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

If that's invalid, then maybe a more extensive fix is needed?

Thanks
Guennadi

> > Regards,
> > 
> > 	Hans
> > 
> >>
> >> Thanks
> >> Guennadi
> >>
> >>> V4L2 always assumes that there is some initial format configured, and this line
> >>> enables that for this driver (NTSC).
> >>>
> >>> Regards,
> >>>
> >>> 	Hans

Re: [PATCH 04/14] tw9910: init priv->scale and update standard

[Hans Verkuil]

On 07/26/2015 12:00 PM, Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> On Fri, 24 Jul 2015, Hans Verkuil wrote:
> 
>> Guennadi,
>>
>> I want to make a pull request for this patch series. This patch is the only
>> outstanding one.
> 
> Right, sorry for the delay. Replying to your explanation below:
> 
>> Or do you have to review more patches? I only got Acks for patches 1 and 
>> 2.
>>
>> Regards,
>>
>> 	Hans
>>
>> On 06/22/2015 09:29 AM, Hans Verkuil wrote:
>>> On 06/22/2015 09:21 AM, Guennadi Liakhovetski wrote:
>>>> Hi Hans,
>>>>
>>>> On Mon, 22 Jun 2015, Hans Verkuil wrote:
>>>>
>>>>> On 06/21/2015 07:23 PM, Guennadi Liakhovetski wrote:
>>>>>> On Mon, 15 Jun 2015, Hans Verkuil wrote:
>>>>>>
>>>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>>>
>>>>>>> When the standard changes the VACTIVE and VDELAY values need to be updated.
>>>>>>>
>>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>>> ---
>>>>>>>  drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++-
>>>>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
>>>>>>> index df66417..e939c24 100644
>>>>>>> --- a/drivers/media/i2c/soc_camera/tw9910.c
>>>>>>> +++ b/drivers/media/i2c/soc_camera/tw9910.c
>>>>>>> @@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client)
>>>>>>>  		 "tw9910 Product ID %0x:%0x\n", id, priv->revision);
>>>>>>>  
>>>>>>>  	priv->norm = V4L2_STD_NTSC;
>>>>>>> +	priv->scale = &tw9910_ntsc_scales[0];
>>>>>>
>>>>>> Why do you need this? So far everywhere in the code priv->scale is either 
>>>>>> checked or set before use. Don't see why an additional initialisation is 
>>>>>> needed.
>>>>>
>>>>> If you just start streaming without explicitly setting up formats (which is
>>>>> allowed), then priv->scale is still NULL.
>>>>
>>>> Yes, it can well be NULL, but it is also unused. Before it is used it will 
>>>> be set, while it is unused it is allowed to stay NULL.
>>>
>>> No. If you start streaming without the set_fmt op having been called, then
>>> s_stream will return an error since priv->scale is NULL. This is wrong. Since
>>> this driver defaults to NTSC the initial setup should be for NTSC and it should
>>> be ready for streaming.
>>>
>>> So priv->scale *is* used: in s_stream. And it is not necessarily set before use.
>>> E.g. if you load the driver and run 'v4l2-ctl --stream-out-mmap' you will hit this
>>> case. It's how I found this bug.
>>>
>>> It's a trivial one liner to ensure a valid priv->scale pointer.
> 
> Yes, you're right, now I see how this can happen. But there's also another 
> possibility: if S_FMT fails priv->scale will be set to NULL. If you then 
> directly start streaming wouldn't the same problem arise? Or is it valid 
> to fail STREAMON after a failed S_FMT? If it is, then of course

The only way S_FMT (or really tw9910_set_frame) can fail is if there are I/O errors.
And then it is OK for STREAMON to fail (you're typically in deep shit anyway if this
happens).

Regards,

	Hans

> 
> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> If that's invalid, then maybe a more extensive fix is needed?
> 
> Thanks
> Guennadi
> 
>>> Regards,
>>>
>>> 	Hans
>>>
>>>>
>>>> Thanks
>>>> Guennadi
>>>>
>>>>> V4L2 always assumes that there is some initial format configured, and this line
>>>>> enables that for this driver (NTSC).
>>>>>
>>>>> Regards,
>>>>>
>>>>> 	Hans
> --
> 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: [PATCH 04/14] tw9910: init priv->scale and update standard

[Guennadi Liakhovetski]

On Tue, 28 Jul 2015, Hans Verkuil wrote:

> On 07/26/2015 12:00 PM, Guennadi Liakhovetski wrote:
> > Hi Hans,
> > 
> > On Fri, 24 Jul 2015, Hans Verkuil wrote:
> > 
> >> Guennadi,
> >>
> >> I want to make a pull request for this patch series. This patch is the only
> >> outstanding one.
> > 
> > Right, sorry for the delay. Replying to your explanation below:
> > 
> >> Or do you have to review more patches? I only got Acks for patches 1 and 
> >> 2.
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >> On 06/22/2015 09:29 AM, Hans Verkuil wrote:
> >>> On 06/22/2015 09:21 AM, Guennadi Liakhovetski wrote:
> >>>> Hi Hans,
> >>>>
> >>>> On Mon, 22 Jun 2015, Hans Verkuil wrote:
> >>>>
> >>>>> On 06/21/2015 07:23 PM, Guennadi Liakhovetski wrote:
> >>>>>> On Mon, 15 Jun 2015, Hans Verkuil wrote:
> >>>>>>
> >>>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>>>>
> >>>>>>> When the standard changes the VACTIVE and VDELAY values need to be updated.
> >>>>>>>
> >>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>>>> ---
> >>>>>>>  drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++-
> >>>>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
> >>>>>>> index df66417..e939c24 100644
> >>>>>>> --- a/drivers/media/i2c/soc_camera/tw9910.c
> >>>>>>> +++ b/drivers/media/i2c/soc_camera/tw9910.c
> >>>>>>> @@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client)
> >>>>>>>  		 "tw9910 Product ID %0x:%0x\n", id, priv->revision);
> >>>>>>>  
> >>>>>>>  	priv->norm = V4L2_STD_NTSC;
> >>>>>>> +	priv->scale = &tw9910_ntsc_scales[0];
> >>>>>>
> >>>>>> Why do you need this? So far everywhere in the code priv->scale is either 
> >>>>>> checked or set before use. Don't see why an additional initialisation is 
> >>>>>> needed.
> >>>>>
> >>>>> If you just start streaming without explicitly setting up formats (which is
> >>>>> allowed), then priv->scale is still NULL.
> >>>>
> >>>> Yes, it can well be NULL, but it is also unused. Before it is used it will 
> >>>> be set, while it is unused it is allowed to stay NULL.
> >>>
> >>> No. If you start streaming without the set_fmt op having been called, then
> >>> s_stream will return an error since priv->scale is NULL. This is wrong. Since
> >>> this driver defaults to NTSC the initial setup should be for NTSC and it should
> >>> be ready for streaming.
> >>>
> >>> So priv->scale *is* used: in s_stream. And it is not necessarily set before use.
> >>> E.g. if you load the driver and run 'v4l2-ctl --stream-out-mmap' you will hit this
> >>> case. It's how I found this bug.
> >>>
> >>> It's a trivial one liner to ensure a valid priv->scale pointer.
> > 
> > Yes, you're right, now I see how this can happen. But there's also another 
> > possibility: if S_FMT fails priv->scale will be set to NULL. If you then 
> > directly start streaming wouldn't the same problem arise? Or is it valid 
> > to fail STREAMON after a failed S_FMT? If it is, then of course
> 
> The only way S_FMT (or really tw9910_set_frame) can fail is if there are I/O errors.
> And then it is OK for STREAMON to fail (you're typically in deep shit anyway if this
> happens).

Ok, then my ack holds:)

Thanks
Guennadi

> Regards,
> 
> 	Hans
> 
> > Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > 
> > If that's invalid, then maybe a more extensive fix is needed?
> > 
> > Thanks
> > Guennadi
> > 
> >>> Regards,
> >>>
> >>> 	Hans
> >>>
> >>>>
> >>>> Thanks
> >>>> Guennadi
> >>>>
> >>>>> V4L2 always assumes that there is some initial format configured, and this line
> >>>>> enables that for this driver (NTSC).
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>> 	Hans