[PATCH v2 00/17] rcar-vin: fix issues with format and capturing

[PATCH v2 00/17] rcar-vin: fix issues with format and capturing

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Hi,

This series fix a number of issues for the rcar-vin driver regarding
format and capturing. It is based on top of '[GIT PULL FOR v4.13] V4L2 
fwnode support' and tested on Koelsch.

Parts of this series where previously part of '[PATCH 00/11] media:
rcar-vin: fix OPS and format/pad index issues'. But after good reviews
a large part of that series where dropped.

* Changes since v1
- Do not consider the lack of a subdevice sink pad an error instead
  default to use pad 0 in such case. This keeps the behavior of the
  driver which previous versions of this series changed.
- When printing pad numbers print using %u not %d as they are unsigned.
- Keep comment in code which was lost when moving code around.
- If trying to set an unsupported pixel format do not switch the current
  format, instead switch to the first supported format of the driver to
  achieve a more deterministic behaviour.
- Add new patch to remove redundant checks in the async bind/unbind
  callbacks.
- Clarify some commit messages, thanks Laurent!
- Fix typos in commit messages, thanks Sergei and Laurent!
- Add Reviewed-by tags from Lauren

Niklas Söderlund (17):
  rcar-vin: reset bytesperline and sizeimage when resetting format
  rcar-vin: use rvin_reset_format() in S_DV_TIMINGS
  rcar-vin: fix how pads are handled for v4l2 subdevice operations
  rcar-vin: fix standard in input enumeration
  rcar-vin: move subdev source and sink pad index to rvin_graph_entity
  rcar-vin: refactor pad lookup code
  rcar-vin: move pad lookup to async bound handler
  rcar-vin: use pad information when verifying media bus format
  rcar-vin: decrease buffers needed to capture
  rcar-vin: move functions which acts on hardware
  rcar-vin: select capture mode based on free buffers
  rcar-vin: allow switch between capturing modes when stalling
  rcar-vin: refactor and fold in function after stall handling rework
  rcar-vin: remove subdevice matching from bind and unbind callbacks
  rcar-vin: register the video device at probe time
  rcar-vin: add missing error check to propagate error
  rcar-vin: fix bug in pixelformat selection

 drivers/media/platform/rcar-vin/rcar-core.c |  96 +++++++++---
 drivers/media/platform/rcar-vin/rcar-dma.c  | 230 ++++++++++++++--------------
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 139 ++++++-----------
 drivers/media/platform/rcar-vin/rcar-vin.h  |  10 +-
 4 files changed, 248 insertions(+), 227 deletions(-)

-- 
2.13.0

[PATCH v2 01/17] rcar-vin: reset bytesperline and sizeimage when resetting format

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

These two were forgotten when refactoring the format reset code. If
they are not also reset at the same time as width and height the format
returned from G_FMT will not match reality.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 2bbe6d495fa634da..69bc4cfea6a8aeb5 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -151,6 +151,9 @@ static int rvin_reset_format(struct rvin_dev *vin)
 
 	rvin_reset_crop_compose(vin);
 
+	vin->format.bytesperline = rvin_format_bytesperline(&vin->format);
+	vin->format.sizeimage = rvin_format_sizeimage(&vin->format);
+
 	return 0;
 }
 
-- 
2.13.0

[PATCH v2 02/17] rcar-vin: use rvin_reset_format() in S_DV_TIMINGS

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Use rvin_reset_format() in rvin_s_dv_timings() instead of just resetting
a few fields. This fixes an issue where the field format was not
properly set after S_DV_TIMINGS.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 69bc4cfea6a8aeb5..7ca27599b9982ffc 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -573,12 +573,8 @@ static int rvin_s_dv_timings(struct file *file, void *priv_fh,
 	if (ret)
 		return ret;
 
-	vin->source.width = timings->bt.width;
-	vin->source.height = timings->bt.height;
-	vin->format.width = timings->bt.width;
-	vin->format.height = timings->bt.height;
-
-	return 0;
+	/* Changing the timings will change the width/height */
+	return rvin_reset_format(vin);
 }
 
 static int rvin_g_dv_timings(struct file *file, void *priv_fh,
-- 
2.13.0

[PATCH v2 03/17] rcar-vin: fix how pads are handled for v4l2 subdevice operations

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The rcar-vin driver only uses one pad, pad number 0.

- All v4l2 operations that did not check that the requested operation
  was for pad 0 have been updated with a check to enforce this.

- All v4l2 operations that stored (and later restored) the requested pad
  before substituting it for the subdevice pad number have been updated
  to not store the incoming pad and simply restore it to 0 after the
  subdevice operation is complete.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 7ca27599b9982ffc..610f59e2a9142622 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -550,14 +550,16 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
 {
 	struct rvin_dev *vin = video_drvdata(file);
 	struct v4l2_subdev *sd = vin_to_source(vin);
-	int pad, ret;
+	int ret;
+
+	if (timings->pad)
+		return -EINVAL;
 
-	pad = timings->pad;
 	timings->pad = vin->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
 
-	timings->pad = pad;
+	timings->pad = 0;
 
 	return ret;
 }
@@ -600,14 +602,16 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
 {
 	struct rvin_dev *vin = video_drvdata(file);
 	struct v4l2_subdev *sd = vin_to_source(vin);
-	int pad, ret;
+	int ret;
+
+	if (cap->pad)
+		return -EINVAL;
 
-	pad = cap->pad;
 	cap->pad = vin->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
 
-	cap->pad = pad;
+	cap->pad = 0;
 
 	return ret;
 }
@@ -616,17 +620,16 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
 {
 	struct rvin_dev *vin = video_drvdata(file);
 	struct v4l2_subdev *sd = vin_to_source(vin);
-	int input, ret;
+	int ret;
 
 	if (edid->pad)
 		return -EINVAL;
 
-	input = edid->pad;
 	edid->pad = vin->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
 
-	edid->pad = input;
+	edid->pad = 0;
 
 	return ret;
 }
@@ -635,17 +638,16 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
 {
 	struct rvin_dev *vin = video_drvdata(file);
 	struct v4l2_subdev *sd = vin_to_source(vin);
-	int input, ret;
+	int ret;
 
 	if (edid->pad)
 		return -EINVAL;
 
-	input = edid->pad;
 	edid->pad = vin->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
 
-	edid->pad = input;
+	edid->pad = 0;
 
 	return ret;
 }
-- 
2.13.0

[PATCH v2 04/17] rcar-vin: fix standard in input enumeration

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The driver supports a single input only, which can be either analog or
digital. If the subdevice supports dv_timings_cap the input is digital
and the driver should not fill in the standard.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 610f59e2a9142622..7be52c2036bb35fc 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -483,10 +483,14 @@ static int rvin_enum_input(struct file *file, void *priv,
 		return ret;
 
 	i->type = V4L2_INPUT_TYPE_CAMERA;
-	i->std = vin->vdev.tvnorms;
 
-	if (v4l2_subdev_has_op(sd, pad, dv_timings_cap))
+	if (v4l2_subdev_has_op(sd, pad, dv_timings_cap)) {
 		i->capabilities = V4L2_IN_CAP_DV_TIMINGS;
+		i->std = 0;
+	} else {
+		i->capabilities = V4L2_IN_CAP_STD;
+		i->std = vin->vdev.tvnorms;
+	}
 
 	strlcpy(i->name, "Camera", sizeof(i->name));
 
-- 
2.13.0

[PATCH v2 05/17] rcar-vin: move subdev source and sink pad index to rvin_graph_entity

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

It makes more sense to store the sink and source pads in struct
rvin_graph_entity since that contains other subdevice related
information.

The data type to store pad information in is unsigned int and not int,
change this. While we are at it drop the _idx suffix from the names,
this never made sense.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 20 ++++++++++----------
 drivers/media/platform/rcar-vin/rcar-vin.h  |  9 +++++----
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 7be52c2036bb35fc..1a75191539b0e7d7 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -111,7 +111,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
 	struct v4l2_mbus_framefmt *mf = &fmt.format;
 	int ret;
 
-	fmt.pad = vin->src_pad_idx;
+	fmt.pad = vin->digital.source_pad;
 
 	ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
 	if (ret)
@@ -178,7 +178,7 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
 	if (pad_cfg == NULL)
 		return -ENOMEM;
 
-	format.pad = vin->src_pad_idx;
+	format.pad = vin->digital.source_pad;
 
 	field = pix->field;
 
@@ -559,7 +559,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
 	if (timings->pad)
 		return -EINVAL;
 
-	timings->pad = vin->sink_pad_idx;
+	timings->pad = vin->digital.sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
 
@@ -611,7 +611,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
 	if (cap->pad)
 		return -EINVAL;
 
-	cap->pad = vin->sink_pad_idx;
+	cap->pad = vin->digital.sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
 
@@ -629,7 +629,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
 	if (edid->pad)
 		return -EINVAL;
 
-	edid->pad = vin->sink_pad_idx;
+	edid->pad = vin->digital.sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
 
@@ -647,7 +647,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
 	if (edid->pad)
 		return -EINVAL;
 
-	edid->pad = vin->sink_pad_idx;
+	edid->pad = vin->digital.sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
 
@@ -920,19 +920,19 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
 		V4L2_CAP_READWRITE;
 
-	vin->src_pad_idx = 0;
+	vin->digital.source_pad = 0;
 	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
 		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SOURCE)
 			break;
 	if (pad_idx >= sd->entity.num_pads)
 		return -EINVAL;
 
-	vin->src_pad_idx = pad_idx;
+	vin->digital.source_pad = pad_idx;
 
-	vin->sink_pad_idx = 0;
+	vin->digital.sink_pad = 0;
 	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
 		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
-			vin->sink_pad_idx = pad_idx;
+			vin->digital.sink_pad = pad_idx;
 			break;
 		}
 
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 727e215c08718eb9..9bfb5a7c4dc4f215 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -74,6 +74,8 @@ struct rvin_video_format {
  * @subdev:	subdevice matched using async framework
  * @code:	Media bus format from source
  * @mbus_cfg:	Media bus format from DT
+ * @source_pad:	source pad of remote subdevice
+ * @sink_pad:	sink pad of remote subdevice
  */
 struct rvin_graph_entity {
 	struct v4l2_async_subdev asd;
@@ -81,6 +83,9 @@ struct rvin_graph_entity {
 
 	u32 code;
 	struct v4l2_mbus_config mbus_cfg;
+
+	unsigned int source_pad;
+	unsigned int sink_pad;
 };
 
 /**
@@ -91,8 +96,6 @@ struct rvin_graph_entity {
  *
  * @vdev:		V4L2 video device associated with VIN
  * @v4l2_dev:		V4L2 device
- * @src_pad_idx:	source pad index for media controller drivers
- * @sink_pad_idx:	sink pad index for media controller drivers
  * @ctrl_handler:	V4L2 control handler
  * @notifier:		V4L2 asynchronous subdevs notifier
  * @digital:		entity in the DT for local digital subdevice
@@ -121,8 +124,6 @@ struct rvin_dev {
 
 	struct video_device vdev;
 	struct v4l2_device v4l2_dev;
-	int src_pad_idx;
-	int sink_pad_idx;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_async_notifier notifier;
 	struct rvin_graph_entity digital;
-- 
2.13.0

[PATCH v2 06/17] rcar-vin: refactor pad lookup code

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The pad lookup code can be broken out to increase readability and to
reduce code duplication.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 36 +++++++++++++++++------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 1a75191539b0e7d7..90ea582fb48e3cb5 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -870,11 +870,25 @@ static void rvin_notify(struct v4l2_subdev *sd,
 	}
 }
 
+static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
+{
+	unsigned int pad;
+
+	if (sd->entity.num_pads <= 1)
+		return 0;
+
+	for (pad = 0; pad < sd->entity.num_pads; pad++)
+		if (sd->entity.pads[pad].flags & direction)
+			return pad;
+
+	return -EINVAL;
+}
+
 int rvin_v4l2_probe(struct rvin_dev *vin)
 {
 	struct video_device *vdev = &vin->vdev;
 	struct v4l2_subdev *sd = vin_to_source(vin);
-	int pad_idx, ret;
+	int ret;
 
 	v4l2_set_subdev_hostdata(sd, vin);
 
@@ -920,21 +934,13 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
 		V4L2_CAP_READWRITE;
 
-	vin->digital.source_pad = 0;
-	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
-		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SOURCE)
-			break;
-	if (pad_idx >= sd->entity.num_pads)
-		return -EINVAL;
-
-	vin->digital.source_pad = pad_idx;
+	ret = rvin_find_pad(sd, MEDIA_PAD_FL_SOURCE);
+	if (ret < 0)
+		return ret;
+	vin->digital.source_pad = ret;
 
-	vin->digital.sink_pad = 0;
-	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
-		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
-			vin->digital.sink_pad = pad_idx;
-			break;
-		}
+	ret = rvin_find_pad(sd, MEDIA_PAD_FL_SINK);
+	vin->digital.sink_pad = ret < 0 ? 0 : ret;
 
 	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
 	rvin_reset_format(vin);
-- 
2.13.0

[PATCH v2 07/17] rcar-vin: move pad lookup to async bound handler

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Information about pads will be needed when enumerating the media bus
codes in the async complete handler which is run before
rvin_v4l2_probe(). Move the pad lookup to the async bound handler so
they are available when needed.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 30 ++++++++++++++++++++++++++++-
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 22 ---------------------
 2 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 264604a9bcf86110..58fd04a7e9f1151b 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -31,6 +31,20 @@
 
 #define notifier_to_vin(n) container_of(n, struct rvin_dev, notifier)
 
+static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
+{
+	unsigned int pad;
+
+	if (sd->entity.num_pads <= 1)
+		return 0;
+
+	for (pad = 0; pad < sd->entity.num_pads; pad++)
+		if (sd->entity.pads[pad].flags & direction)
+			return pad;
+
+	return -EINVAL;
+}
+
 static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
 {
 	struct v4l2_subdev *sd = entity->subdev;
@@ -101,13 +115,27 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
 				     struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
+	int ret;
 
 	v4l2_set_subdev_hostdata(subdev, vin);
 
 	if (vin->digital.asd.match.fwnode.fwnode ==
 	    of_fwnode_handle(subdev->dev->of_node)) {
-		vin_dbg(vin, "bound digital subdev %s\n", subdev->name);
+		/* Find surce and sink pad of remote subdevice */
+
+		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
+		if (ret < 0)
+			return ret;
+		vin->digital.source_pad = ret;
+
+		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
+		vin->digital.sink_pad = ret < 0 ? 0 : ret;
+
 		vin->digital.subdev = subdev;
+
+		vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
+			subdev->name, vin->digital.source_pad,
+			vin->digital.sink_pad);
 		return 0;
 	}
 
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 90ea582fb48e3cb5..be6f41bf82ac3bc5 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -870,20 +870,6 @@ static void rvin_notify(struct v4l2_subdev *sd,
 	}
 }
 
-static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
-{
-	unsigned int pad;
-
-	if (sd->entity.num_pads <= 1)
-		return 0;
-
-	for (pad = 0; pad < sd->entity.num_pads; pad++)
-		if (sd->entity.pads[pad].flags & direction)
-			return pad;
-
-	return -EINVAL;
-}
-
 int rvin_v4l2_probe(struct rvin_dev *vin)
 {
 	struct video_device *vdev = &vin->vdev;
@@ -934,14 +920,6 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
 		V4L2_CAP_READWRITE;
 
-	ret = rvin_find_pad(sd, MEDIA_PAD_FL_SOURCE);
-	if (ret < 0)
-		return ret;
-	vin->digital.source_pad = ret;
-
-	ret = rvin_find_pad(sd, MEDIA_PAD_FL_SINK);
-	vin->digital.sink_pad = ret < 0 ? 0 : ret;
-
 	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
 	rvin_reset_format(vin);
 
-- 
2.13.0

[PATCH v2 08/17] rcar-vin: use pad information when verifying media bus format

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Use information about pad index when enumerating mbus codes.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 58fd04a7e9f1151b..e67e4a57baadc3fb 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -53,6 +53,7 @@ static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
 	};
 
 	code.index = 0;
+	code.pad = entity->source_pad;
 	while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) {
 		code.index++;
 		switch (code.code) {
-- 
2.13.0

[PATCH v2 09/17] rcar-vin: decrease buffers needed to capture

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

It's possible to grab frames using only one buffer, this should never
have been set to anything else then 1.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 9ccd5ff55e192514..c37f7a2993fb5565 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1183,7 +1183,7 @@ int rvin_dma_probe(struct rvin_dev *vin, int irq)
 	q->ops = &rvin_qops;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
-	q->min_buffers_needed = 2;
+	q->min_buffers_needed = 1;
 	q->dev = vin->dev;
 
 	ret = vb2_queue_init(q);
-- 
2.13.0

[PATCH v2 10/17] rcar-vin: move functions which acts on hardware

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

This only moves whole structs, defines and functions around, no code is
changed inside any function. The reason for moving this code around is
to prepare for refactoring and fixing of a start/stop stream bug without
having to use forward declarations.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 182 ++++++++++++++---------------
 1 file changed, 91 insertions(+), 91 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index c37f7a2993fb5565..11e9799564299fca 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -119,6 +119,15 @@
 #define VNDMR2_FTEV		(1 << 17)
 #define VNDMR2_VLV(n)		((n & 0xf) << 12)
 
+struct rvin_buffer {
+	struct vb2_v4l2_buffer vb;
+	struct list_head list;
+};
+
+#define to_buf_list(vb2_buffer) (&container_of(vb2_buffer, \
+					       struct rvin_buffer, \
+					       vb)->list)
+
 static void rvin_write(struct rvin_dev *vin, u32 value, u32 offset)
 {
 	iowrite32(value, vin->base + offset);
@@ -269,48 +278,6 @@ static int rvin_setup(struct rvin_dev *vin)
 	return 0;
 }
 
-static void rvin_capture_on(struct rvin_dev *vin)
-{
-	vin_dbg(vin, "Capture on in %s mode\n",
-		vin->continuous ? "continuous" : "single");
-
-	if (vin->continuous)
-		/* Continuous Frame Capture Mode */
-		rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
-	else
-		/* Single Frame Capture Mode */
-		rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
-}
-
-static void rvin_capture_off(struct rvin_dev *vin)
-{
-	/* Set continuous & single transfer off */
-	rvin_write(vin, 0, VNFC_REG);
-}
-
-static int rvin_capture_start(struct rvin_dev *vin)
-{
-	int ret;
-
-	rvin_crop_scale_comp(vin);
-
-	ret = rvin_setup(vin);
-	if (ret)
-		return ret;
-
-	rvin_capture_on(vin);
-
-	return 0;
-}
-
-static void rvin_capture_stop(struct rvin_dev *vin)
-{
-	rvin_capture_off(vin);
-
-	/* Disable module */
-	rvin_write(vin, rvin_read(vin, VNMC_REG) & ~VNMC_ME, VNMC_REG);
-}
-
 static void rvin_disable_interrupts(struct rvin_dev *vin)
 {
 	rvin_write(vin, 0, VNIE_REG);
@@ -377,6 +344,88 @@ static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
 	rvin_write(vin, offset, VNMB_REG(slot));
 }
 
+/* Moves a buffer from the queue to the HW slots */
+static bool rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
+{
+	struct rvin_buffer *buf;
+	struct vb2_v4l2_buffer *vbuf;
+	dma_addr_t phys_addr_top;
+
+	if (vin->queue_buf[slot] != NULL)
+		return true;
+
+	if (list_empty(&vin->buf_list))
+		return false;
+
+	vin_dbg(vin, "Filling HW slot: %d\n", slot);
+
+	/* Keep track of buffer we give to HW */
+	buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
+	vbuf = &buf->vb;
+	list_del_init(to_buf_list(vbuf));
+	vin->queue_buf[slot] = vbuf;
+
+	/* Setup DMA */
+	phys_addr_top = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
+	rvin_set_slot_addr(vin, slot, phys_addr_top);
+
+	return true;
+}
+
+static bool rvin_fill_hw(struct rvin_dev *vin)
+{
+	int slot, limit;
+
+	limit = vin->continuous ? HW_BUFFER_NUM : 1;
+
+	for (slot = 0; slot < limit; slot++)
+		if (!rvin_fill_hw_slot(vin, slot))
+			return false;
+	return true;
+}
+
+static void rvin_capture_on(struct rvin_dev *vin)
+{
+	vin_dbg(vin, "Capture on in %s mode\n",
+		vin->continuous ? "continuous" : "single");
+
+	if (vin->continuous)
+		/* Continuous Frame Capture Mode */
+		rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
+	else
+		/* Single Frame Capture Mode */
+		rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
+}
+
+static void rvin_capture_off(struct rvin_dev *vin)
+{
+	/* Set continuous & single transfer off */
+	rvin_write(vin, 0, VNFC_REG);
+}
+
+static int rvin_capture_start(struct rvin_dev *vin)
+{
+	int ret;
+
+	rvin_crop_scale_comp(vin);
+
+	ret = rvin_setup(vin);
+	if (ret)
+		return ret;
+
+	rvin_capture_on(vin);
+
+	return 0;
+}
+
+static void rvin_capture_stop(struct rvin_dev *vin)
+{
+	rvin_capture_off(vin);
+
+	/* Disable module */
+	rvin_write(vin, rvin_read(vin, VNMC_REG) & ~VNMC_ME, VNMC_REG);
+}
+
 /* -----------------------------------------------------------------------------
  * Crop and Scaling Gen2
  */
@@ -839,55 +888,6 @@ void rvin_scale_try(struct rvin_dev *vin, struct v4l2_pix_format *pix,
 #define RVIN_TIMEOUT_MS 100
 #define RVIN_RETRIES 10
 
-struct rvin_buffer {
-	struct vb2_v4l2_buffer vb;
-	struct list_head list;
-};
-
-#define to_buf_list(vb2_buffer) (&container_of(vb2_buffer, \
-					       struct rvin_buffer, \
-					       vb)->list)
-
-/* Moves a buffer from the queue to the HW slots */
-static bool rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
-{
-	struct rvin_buffer *buf;
-	struct vb2_v4l2_buffer *vbuf;
-	dma_addr_t phys_addr_top;
-
-	if (vin->queue_buf[slot] != NULL)
-		return true;
-
-	if (list_empty(&vin->buf_list))
-		return false;
-
-	vin_dbg(vin, "Filling HW slot: %d\n", slot);
-
-	/* Keep track of buffer we give to HW */
-	buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
-	vbuf = &buf->vb;
-	list_del_init(to_buf_list(vbuf));
-	vin->queue_buf[slot] = vbuf;
-
-	/* Setup DMA */
-	phys_addr_top = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
-	rvin_set_slot_addr(vin, slot, phys_addr_top);
-
-	return true;
-}
-
-static bool rvin_fill_hw(struct rvin_dev *vin)
-{
-	int slot, limit;
-
-	limit = vin->continuous ? HW_BUFFER_NUM : 1;
-
-	for (slot = 0; slot < limit; slot++)
-		if (!rvin_fill_hw_slot(vin, slot))
-			return false;
-	return true;
-}
-
 static irqreturn_t rvin_irq(int irq, void *data)
 {
 	struct rvin_dev *vin = data;
-- 
2.13.0

[PATCH v2 11/17] rcar-vin: select capture mode based on free buffers

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Instead of selecting single or continuous capture mode based on how many
buffers userspace intends to give us select capture mode based on number
of free buffers we can allocate to hardware when the stream is started.

This change is a prerequisite to enable the driver to switch from
continuous to single capture mode (or the other way around) when the
driver is stalled by userspace not feeding it buffers as fast as it
consumes it.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 31 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 11e9799564299fca..47d78f68d715d945 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -405,7 +405,21 @@ static void rvin_capture_off(struct rvin_dev *vin)
 
 static int rvin_capture_start(struct rvin_dev *vin)
 {
-	int ret;
+	struct rvin_buffer *buf, *node;
+	int bufs, ret;
+
+	/* Count number of free buffers */
+	bufs = 0;
+	list_for_each_entry_safe(buf, node, &vin->buf_list, list)
+		bufs++;
+
+	/* Continuous capture requires more buffers then there are HW slots */
+	vin->continuous = bufs > HW_BUFFER_NUM;
+
+	if (!rvin_fill_hw(vin)) {
+		vin_err(vin, "HW not ready to start, not enough buffers available\n");
+		return -EINVAL;
+	}
 
 	rvin_crop_scale_comp(vin);
 
@@ -1062,22 +1076,7 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 	vin->state = RUNNING;
 	vin->sequence = 0;
 
-	/* Continuous capture requires more buffers then there are HW slots */
-	vin->continuous = count > HW_BUFFER_NUM;
-
-	/*
-	 * This should never happen but if we don't have enough
-	 * buffers for HW bail out
-	 */
-	if (!rvin_fill_hw(vin)) {
-		vin_err(vin, "HW not ready to start, not enough buffers available\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
 	ret = rvin_capture_start(vin);
-out:
-	/* Return all buffers if something went wrong */
 	if (ret) {
 		return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
 		v4l2_subdev_call(sd, video, s_stream, 0);
-- 
2.13.0

[PATCH v2 12/17] rcar-vin: allow switch between capturing modes when stalling

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

If userspace can't feed the driver with buffers as fast as the driver
consumes them the driver will stop video capturing and wait for more
buffers from userspace, the driver is stalled. Once it have been feed
one or more free buffers it will recover from the stall and resume
capturing.

Instead of continue to capture using the same capture mode as before the
stall allow the driver to choose between single and continuous mode
based on free buffer availability. Do this by stopping capturing when
the driver becomes stalled and restart capturing once it continues. By
doing
this the capture mode will be evaluated each time the driver is
recovering from a stall.

This behavior is needed to fix a bug where continuous capturing mode is
used, userspace is about to stop the stream and is waiting for the last
buffers to be returned from the driver and is not queuing any new
buffers. In this case the driver becomes stalled when there are only 3
buffers remaining streaming will never resume since the driver is
waiting for userspace to feed it more buffers before it can continue
streaming.  With this fix the driver will then switch to single capture
mode for the last 3 buffers and a deadlock is avoided. The issue can be
demonstrated using yavta.

$ yavta -f RGB565 -s 640x480 -n 4 --capture=10  /dev/video22
Device /dev/video22 opened.
Device `R_Car_VIN' on `platform:e6ef1000.video' (driver 'rcar_vin') supports video, capture, without mplanes.
Video format set: RGB565 (50424752) 640x480 (stride 1280) field interlaced buffer size 614400
Video format: RGB565 (50424752) 640x480 (stride 1280) field interlaced buffer size 614400
4 buffers requested.
length: 614400 offset: 0 timestamp type/source: mono/EoF
Buffer 0/0 mapped at address 0xb6cc7000.
length: 614400 offset: 614400 timestamp type/source: mono/EoF
Buffer 1/0 mapped at address 0xb6c31000.
length: 614400 offset: 1228800 timestamp type/source: mono/EoF
Buffer 2/0 mapped at address 0xb6b9b000.
length: 614400 offset: 1843200 timestamp type/source: mono/EoF
Buffer 3/0 mapped at address 0xb6b05000.
0 (0) [-] interlaced 0 614400 B 38.240285 38.240303 12.421 fps ts mono/EoF
1 (1) [-] interlaced 1 614400 B 38.282329 38.282346 23.785 fps ts mono/EoF
2 (2) [-] interlaced 2 614400 B 38.322324 38.322338 25.003 fps ts mono/EoF
3 (3) [-] interlaced 3 614400 B 38.362318 38.362333 25.004 fps ts mono/EoF
4 (0) [-] interlaced 4 614400 B 38.402313 38.402328 25.003 fps ts mono/EoF
5 (1) [-] interlaced 5 614400 B 38.442307 38.442321 25.004 fps ts mono/EoF
6 (2) [-] interlaced 6 614400 B 38.482301 38.482316 25.004 fps ts mono/EoF
7 (3) [-] interlaced 7 614400 B 38.522295 38.522312 25.004 fps ts mono/EoF
8 (0) [-] interlaced 8 614400 B 38.562290 38.562306 25.003 fps ts mono/EoF
<blocks forever, waiting for the last buffer>

This fix also allow the driver to switch to single capture mode if
userspace doesn't feed it buffers fast enough. Or the other way around,
if userspace suddenly feeds the driver buffers faster it can switch to
continues capturing mode.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 47d78f68d715d945..ae4febede5f79f28 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -429,6 +429,8 @@ static int rvin_capture_start(struct rvin_dev *vin)
 
 	rvin_capture_on(vin);
 
+	vin->state = RUNNING;
+
 	return 0;
 }
 
@@ -907,7 +909,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
 	struct rvin_dev *vin = data;
 	u32 int_status, vnms;
 	int slot;
-	unsigned int sequence, handled = 0;
+	unsigned int i, sequence, handled = 0;
 	unsigned long flags;
 
 	spin_lock_irqsave(&vin->qlock, flags);
@@ -969,8 +971,20 @@ static irqreturn_t rvin_irq(int irq, void *data)
 		 * the VnMBm registers.
 		 */
 		if (vin->continuous) {
-			rvin_capture_off(vin);
+			rvin_capture_stop(vin);
 			vin_dbg(vin, "IRQ %02d: hw not ready stop\n", sequence);
+
+			/* Maybe we can continue in single capture mode */
+			for (i = 0; i < HW_BUFFER_NUM; i++) {
+				if (vin->queue_buf[i]) {
+					list_add(to_buf_list(vin->queue_buf[i]),
+						 &vin->buf_list);
+					vin->queue_buf[i] = NULL;
+				}
+			}
+
+			if (!list_empty(&vin->buf_list))
+				rvin_capture_start(vin);
 		}
 	} else {
 		/*
@@ -1055,8 +1069,7 @@ static void rvin_buffer_queue(struct vb2_buffer *vb)
 	 * capturing if HW is ready to continue.
 	 */
 	if (vin->state == STALLED)
-		if (rvin_fill_hw(vin))
-			rvin_capture_on(vin);
+		rvin_capture_start(vin);
 
 	spin_unlock_irqrestore(&vin->qlock, flags);
 }
@@ -1073,7 +1086,6 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	spin_lock_irqsave(&vin->qlock, flags);
 
-	vin->state = RUNNING;
 	vin->sequence = 0;
 
 	ret = rvin_capture_start(vin);
-- 
2.13.0

[PATCH v2 13/17] rcar-vin: refactor and fold in function after stall handling rework

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

With the driver stopping and starting the stream each time the driver is
stalled rvin_capture_off() can be folded in to the only caller.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index ae4febede5f79f28..b136844499f677cf 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -397,12 +397,6 @@ static void rvin_capture_on(struct rvin_dev *vin)
 		rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
 }
 
-static void rvin_capture_off(struct rvin_dev *vin)
-{
-	/* Set continuous & single transfer off */
-	rvin_write(vin, 0, VNFC_REG);
-}
-
 static int rvin_capture_start(struct rvin_dev *vin)
 {
 	struct rvin_buffer *buf, *node;
@@ -436,7 +430,8 @@ static int rvin_capture_start(struct rvin_dev *vin)
 
 static void rvin_capture_stop(struct rvin_dev *vin)
 {
-	rvin_capture_off(vin);
+	/* Set continuous & single transfer off */
+	rvin_write(vin, 0, VNFC_REG);
 
 	/* Disable module */
 	rvin_write(vin, rvin_read(vin, VNMC_REG) & ~VNMC_ME, VNMC_REG);
-- 
2.13.0

[PATCH v2 14/17] rcar-vin: remove subdevice matching from bind and unbind callbacks

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

There is only one subdevice registered with the async framework so there
is no need for the driver to check which subdevice is bound or unbound.
Remove these checks since the async framework preforms this.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 40 +++++++++++------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index e67e4a57baadc3fb..dcca906ba58435f5 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -101,14 +101,9 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
 
-	if (vin->digital.subdev == subdev) {
-		vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
-		rvin_v4l2_remove(vin);
-		vin->digital.subdev = NULL;
-		return;
-	}
-
-	vin_err(vin, "no entity for subdev %s to unbind\n", subdev->name);
+	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
+	rvin_v4l2_remove(vin);
+	vin->digital.subdev = NULL;
 }
 
 static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
@@ -120,28 +115,23 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
 
 	v4l2_set_subdev_hostdata(subdev, vin);
 
-	if (vin->digital.asd.match.fwnode.fwnode ==
-	    of_fwnode_handle(subdev->dev->of_node)) {
-		/* Find surce and sink pad of remote subdevice */
+	/* Find surce and sink pad of remote subdevice */
 
-		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
-		if (ret < 0)
-			return ret;
-		vin->digital.source_pad = ret;
+	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
+	if (ret < 0)
+		return ret;
+	vin->digital.source_pad = ret;
 
-		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
-		vin->digital.sink_pad = ret < 0 ? 0 : ret;
+	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
+	vin->digital.sink_pad = ret < 0 ? 0 : ret;
 
-		vin->digital.subdev = subdev;
+	vin->digital.subdev = subdev;
 
-		vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
-			subdev->name, vin->digital.source_pad,
-			vin->digital.sink_pad);
-		return 0;
-	}
+	vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
+		subdev->name, vin->digital.source_pad,
+		vin->digital.sink_pad);
 
-	vin_err(vin, "no entity for subdev %s to bind\n", subdev->name);
-	return -EINVAL;
+	return 0;
 }
 
 static int rvin_digitial_parse_v4l2(struct rvin_dev *vin,
-- 
2.13.0

[PATCH v2 15/17] rcar-vin: register the video device at probe time

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The driver registers the video device from the async complete callback
and unregistered in the async unbind callback. This creates problems if
if the subdevice is bound, unbound and later rebound. The second time
video_register_device() is called it fails:

   kobject (eb3be918): tried to init an initialized object, something is seriously wrong.

To prevent this register the video device at prob time and don't allow
user-space to open the video device if the subdevice have not yet been
bound.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 47 ++++++++++++++++++++++++++---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 ++++----------------------
 drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
 3 files changed, 50 insertions(+), 40 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index dcca906ba58435f5..0e1757301a0bca1e 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -74,6 +74,7 @@ static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
 static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct v4l2_subdev *sd = vin_to_source(vin);
 	int ret;
 
 	/* Verify subdevices mbus format */
@@ -92,7 +93,35 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 		return ret;
 	}
 
-	return rvin_v4l2_probe(vin);
+	/* Add the controls */
+	/*
+	 * Currently the subdev with the largest number of controls (13) is
+	 * ov6550. So let's pick 16 as a hint for the control handler. Note
+	 * that this is a hint only: too large and you waste some memory, too
+	 * small and there is a (very) small performance hit when looking up
+	 * controls in the internal hash.
+	 */
+	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
+	if (ret < 0)
+		return ret;
+
+	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
+	if (ret < 0)
+		return ret;
+
+	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
+	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
+		return ret;
+
+	if (vin->vdev.tvnorms == 0) {
+		/* Disable the STD API if there are no tvnorms defined */
+		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
+		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
+		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
+		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
+	}
+
+	return rvin_reset_format(vin);
 }
 
 static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
@@ -102,7 +131,7 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
 	struct rvin_dev *vin = notifier_to_vin(notifier);
 
 	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
-	rvin_v4l2_remove(vin);
+	v4l2_ctrl_handler_free(&vin->ctrl_handler);
 	vin->digital.subdev = NULL;
 }
 
@@ -290,9 +319,13 @@ static int rcar_vin_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = rvin_v4l2_probe(vin);
+	if (ret)
+		goto error_dma;
+
 	ret = rvin_digital_graph_init(vin);
 	if (ret < 0)
-		goto error;
+		goto error_v4l2;
 
 	pm_suspend_ignore_children(&pdev->dev, true);
 	pm_runtime_enable(&pdev->dev);
@@ -300,7 +333,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, vin);
 
 	return 0;
-error:
+error_v4l2:
+	rvin_v4l2_remove(vin);
+error_dma:
 	rvin_dma_remove(vin);
 
 	return ret;
@@ -314,6 +349,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
 
 	v4l2_async_notifier_unregister(&vin->notifier);
 
+	/* Checks internaly if handlers have been init or not */
+	v4l2_ctrl_handler_free(&vin->ctrl_handler);
+
+	rvin_v4l2_remove(vin);
 	rvin_dma_remove(vin);
 
 	return 0;
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index be6f41bf82ac3bc5..6f1c27fc828fe57e 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -103,7 +103,7 @@ static void rvin_reset_crop_compose(struct rvin_dev *vin)
 	vin->compose.height = vin->format.height;
 }
 
-static int rvin_reset_format(struct rvin_dev *vin)
+int rvin_reset_format(struct rvin_dev *vin)
 {
 	struct v4l2_subdev_format fmt = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
@@ -785,6 +785,11 @@ static int rvin_open(struct file *file)
 
 	mutex_lock(&vin->lock);
 
+	if (!vin->digital.subdev) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
 	file->private_data = vin;
 
 	ret = v4l2_fh_open(file);
@@ -848,9 +853,6 @@ void rvin_v4l2_remove(struct rvin_dev *vin)
 	v4l2_info(&vin->v4l2_dev, "Removing %s\n",
 		  video_device_node_name(&vin->vdev));
 
-	/* Checks internaly if handlers have been init or not */
-	v4l2_ctrl_handler_free(&vin->ctrl_handler);
-
 	/* Checks internaly if vdev have been init or not */
 	video_unregister_device(&vin->vdev);
 }
@@ -873,41 +875,10 @@ static void rvin_notify(struct v4l2_subdev *sd,
 int rvin_v4l2_probe(struct rvin_dev *vin)
 {
 	struct video_device *vdev = &vin->vdev;
-	struct v4l2_subdev *sd = vin_to_source(vin);
 	int ret;
 
-	v4l2_set_subdev_hostdata(sd, vin);
-
 	vin->v4l2_dev.notify = rvin_notify;
 
-	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
-	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
-		return ret;
-
-	if (vin->vdev.tvnorms == 0) {
-		/* Disable the STD API if there are no tvnorms defined */
-		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
-		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
-		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
-		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
-	}
-
-	/* Add the controls */
-	/*
-	 * Currently the subdev with the largest number of controls (13) is
-	 * ov6550. So let's pick 16 as a hint for the control handler. Note
-	 * that this is a hint only: too large and you waste some memory, too
-	 * small and there is a (very) small performance hit when looking up
-	 * controls in the internal hash.
-	 */
-	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
-	if (ret < 0)
-		return ret;
-
-	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
-	if (ret < 0)
-		return ret;
-
 	/* video node */
 	vdev->fops = &rvin_fops;
 	vdev->v4l2_dev = &vin->v4l2_dev;
@@ -921,7 +892,6 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 		V4L2_CAP_READWRITE;
 
 	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
-	rvin_reset_format(vin);
 
 	ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
 	if (ret) {
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 9bfb5a7c4dc4f215..9d0d4a5001b6ccd8 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -158,6 +158,7 @@ void rvin_dma_remove(struct rvin_dev *vin);
 
 int rvin_v4l2_probe(struct rvin_dev *vin);
 void rvin_v4l2_remove(struct rvin_dev *vin);
+int rvin_reset_format(struct rvin_dev *vin);
 
 const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat);
 
-- 
2.13.0

[PATCH v2 16/17] rcar-vin: add missing error check to propagate error

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The return value of __rvin_try_format_source is not checked, add a check
and propagate the error.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 6f1c27fc828fe57e..de71e5fa8b10cb5e 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -208,6 +208,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
 {
 	const struct rvin_video_format *info;
 	u32 rwidth, rheight, walign;
+	int ret;
 
 	/* Requested */
 	rwidth = pix->width;
@@ -235,7 +236,9 @@ static int __rvin_try_format(struct rvin_dev *vin,
 	pix->sizeimage = 0;
 
 	/* Limit to source capabilities */
-	__rvin_try_format_source(vin, which, pix, source);
+	ret = __rvin_try_format_source(vin, which, pix, source);
+	if (ret)
+		return ret;
 
 	switch (pix->field) {
 	case V4L2_FIELD_TOP:
-- 
2.13.0

[PATCH v2 17/17] rcar-vin: fix bug in pixelformat selection

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

If the requested pixelformat is not supported fallback to the default
format, do not revert the entire format.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index de71e5fa8b10cb5e..81ff59c3b4744075 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -206,7 +206,6 @@ static int __rvin_try_format(struct rvin_dev *vin,
 			     struct v4l2_pix_format *pix,
 			     struct rvin_source_fmt *source)
 {
-	const struct rvin_video_format *info;
 	u32 rwidth, rheight, walign;
 	int ret;
 
@@ -218,17 +217,11 @@ static int __rvin_try_format(struct rvin_dev *vin,
 	if (pix->field == V4L2_FIELD_ANY)
 		pix->field = vin->format.field;
 
-	/*
-	 * Retrieve format information and select the current format if the
-	 * requested format isn't supported.
-	 */
-	info = rvin_format_from_pixel(pix->pixelformat);
-	if (!info) {
-		vin_dbg(vin, "Format %x not found, keeping %x\n",
-			pix->pixelformat, vin->format.pixelformat);
-		*pix = vin->format;
-		pix->width = rwidth;
-		pix->height = rheight;
+	/* If requested format is not supported fallback to the default */
+	if (!rvin_format_from_pixel(pix->pixelformat)) {
+		vin_dbg(vin, "Format 0x%x not found, using default 0x%x\n",
+			pix->pixelformat, RVIN_DEFAULT_FORMAT);
+		pix->pixelformat = RVIN_DEFAULT_FORMAT;
 	}
 
 	/* Always recalculate */
-- 
2.13.0

Re: [PATCH v2 17/17] rcar-vin: fix bug in pixelformat selection

[Sergei Shtylyov]

Hello!

On 5/24/2017 3:15 AM, Niklas Söderlund wrote:

> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> If the requested pixelformat is not supported fallback to the default
> format, do not revert the entire format.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index de71e5fa8b10cb5e..81ff59c3b4744075 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
[...]
> @@ -218,17 +217,11 @@ static int __rvin_try_format(struct rvin_dev *vin,
>  	if (pix->field == V4L2_FIELD_ANY)
>  		pix->field = vin->format.field;
>
> -	/*
> -	 * Retrieve format information and select the current format if the
> -	 * requested format isn't supported.
> -	 */
> -	info = rvin_format_from_pixel(pix->pixelformat);
> -	if (!info) {
> -		vin_dbg(vin, "Format %x not found, keeping %x\n",
> -			pix->pixelformat, vin->format.pixelformat);
> -		*pix = vin->format;
> -		pix->width = rwidth;
> -		pix->height = rheight;
> +	/* If requested format is not supported fallback to the default */
> +	if (!rvin_format_from_pixel(pix->pixelformat)) {
> +		vin_dbg(vin, "Format 0x%x not found, using default 0x%x\n",

     s/0x%x/%#x/, maybe?

> +			pix->pixelformat, RVIN_DEFAULT_FORMAT);
> +		pix->pixelformat = RVIN_DEFAULT_FORMAT;
>  	}
>
>  	/* Always recalculate */

MBR, Sergei

Re: [PATCH v2 15/17] rcar-vin: register the video device at probe time

[Hans Verkuil]

Hi Niklas,

On 05/24/2017 02:15 AM, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> The driver registers the video device from the async complete callback
> and unregistered in the async unbind callback. This creates problems if
> if the subdevice is bound, unbound and later rebound. The second time
> video_register_device() is called it fails:
> 
>     kobject (eb3be918): tried to init an initialized object, something is seriously wrong.
> 
> To prevent this register the video device at prob time and don't allow
> user-space to open the video device if the subdevice have not yet been
> bound.

This patch feels wrong. Creating the video device in the notify_complete seems
right to me, so the problem is much more likely in the removal of the video device.

What *exactly* goes wrong here?

FYI: I'm taking all other patches of this series, but I leave this one out
as I am not convinced of this patch. Having to block open() calls is always
a bad sign.

Regards,

	Hans

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>   drivers/media/platform/rcar-vin/rcar-core.c | 47 ++++++++++++++++++++++++++---
>   drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 ++++----------------------
>   drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
>   3 files changed, 50 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index dcca906ba58435f5..0e1757301a0bca1e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -74,6 +74,7 @@ static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
>   static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
>   {
>   	struct rvin_dev *vin = notifier_to_vin(notifier);
> +	struct v4l2_subdev *sd = vin_to_source(vin);
>   	int ret;
>   
>   	/* Verify subdevices mbus format */
> @@ -92,7 +93,35 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
>   		return ret;
>   	}
>   
> -	return rvin_v4l2_probe(vin);
> +	/* Add the controls */
> +	/*
> +	 * Currently the subdev with the largest number of controls (13) is
> +	 * ov6550. So let's pick 16 as a hint for the control handler. Note
> +	 * that this is a hint only: too large and you waste some memory, too
> +	 * small and there is a (very) small performance hit when looking up
> +	 * controls in the internal hash.
> +	 */
> +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> +	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> +		return ret;
> +
> +	if (vin->vdev.tvnorms == 0) {
> +		/* Disable the STD API if there are no tvnorms defined */
> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
> +	}
> +
> +	return rvin_reset_format(vin);
>   }
>   
>   static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> @@ -102,7 +131,7 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
>   	struct rvin_dev *vin = notifier_to_vin(notifier);
>   
>   	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
> -	rvin_v4l2_remove(vin);
> +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
>   	vin->digital.subdev = NULL;
>   }
>   
> @@ -290,9 +319,13 @@ static int rcar_vin_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> +	ret = rvin_v4l2_probe(vin);
> +	if (ret)
> +		goto error_dma;
> +
>   	ret = rvin_digital_graph_init(vin);
>   	if (ret < 0)
> -		goto error;
> +		goto error_v4l2;
>   
>   	pm_suspend_ignore_children(&pdev->dev, true);
>   	pm_runtime_enable(&pdev->dev);
> @@ -300,7 +333,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, vin);
>   
>   	return 0;
> -error:
> +error_v4l2:
> +	rvin_v4l2_remove(vin);
> +error_dma:
>   	rvin_dma_remove(vin);
>   
>   	return ret;
> @@ -314,6 +349,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
>   
>   	v4l2_async_notifier_unregister(&vin->notifier);
>   
> +	/* Checks internaly if handlers have been init or not */
> +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +
> +	rvin_v4l2_remove(vin);
>   	rvin_dma_remove(vin);
>   
>   	return 0;
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index be6f41bf82ac3bc5..6f1c27fc828fe57e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -103,7 +103,7 @@ static void rvin_reset_crop_compose(struct rvin_dev *vin)
>   	vin->compose.height = vin->format.height;
>   }
>   
> -static int rvin_reset_format(struct rvin_dev *vin)
> +int rvin_reset_format(struct rvin_dev *vin)
>   {
>   	struct v4l2_subdev_format fmt = {
>   		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> @@ -785,6 +785,11 @@ static int rvin_open(struct file *file)
>   
>   	mutex_lock(&vin->lock);
>   
> +	if (!vin->digital.subdev) {
> +		ret = -ENODEV;
> +		goto unlock;
> +	}
> +
>   	file->private_data = vin;
>   
>   	ret = v4l2_fh_open(file);
> @@ -848,9 +853,6 @@ void rvin_v4l2_remove(struct rvin_dev *vin)
>   	v4l2_info(&vin->v4l2_dev, "Removing %s\n",
>   		  video_device_node_name(&vin->vdev));
>   
> -	/* Checks internaly if handlers have been init or not */
> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -
>   	/* Checks internaly if vdev have been init or not */
>   	video_unregister_device(&vin->vdev);
>   }
> @@ -873,41 +875,10 @@ static void rvin_notify(struct v4l2_subdev *sd,
>   int rvin_v4l2_probe(struct rvin_dev *vin)
>   {
>   	struct video_device *vdev = &vin->vdev;
> -	struct v4l2_subdev *sd = vin_to_source(vin);
>   	int ret;
>   
> -	v4l2_set_subdev_hostdata(sd, vin);
> -
>   	vin->v4l2_dev.notify = rvin_notify;
>   
> -	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> -		return ret;
> -
> -	if (vin->vdev.tvnorms == 0) {
> -		/* Disable the STD API if there are no tvnorms defined */
> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
> -	}
> -
> -	/* Add the controls */
> -	/*
> -	 * Currently the subdev with the largest number of controls (13) is
> -	 * ov6550. So let's pick 16 as a hint for the control handler. Note
> -	 * that this is a hint only: too large and you waste some memory, too
> -	 * small and there is a (very) small performance hit when looking up
> -	 * controls in the internal hash.
> -	 */
> -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
> -	if (ret < 0)
> -		return ret;
> -
>   	/* video node */
>   	vdev->fops = &rvin_fops;
>   	vdev->v4l2_dev = &vin->v4l2_dev;
> @@ -921,7 +892,6 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>   		V4L2_CAP_READWRITE;
>   
>   	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
> -	rvin_reset_format(vin);
>   
>   	ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
>   	if (ret) {
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 9bfb5a7c4dc4f215..9d0d4a5001b6ccd8 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -158,6 +158,7 @@ void rvin_dma_remove(struct rvin_dev *vin);
>   
>   int rvin_v4l2_probe(struct rvin_dev *vin);
>   void rvin_v4l2_remove(struct rvin_dev *vin);
> +int rvin_reset_format(struct rvin_dev *vin);
>   
>   const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat);
>   
> 

Re: [PATCH v2 15/17] rcar-vin: register the video device at probe time

[Hans Verkuil]

On 05/29/2017 08:52 AM, Hans Verkuil wrote:
> Hi Niklas,
> 
> On 05/24/2017 02:15 AM, Niklas Söderlund wrote:
>> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>
>> The driver registers the video device from the async complete callback
>> and unregistered in the async unbind callback. This creates problems if
>> if the subdevice is bound, unbound and later rebound. The second time
>> video_register_device() is called it fails:
>>
>>      kobject (eb3be918): tried to init an initialized object, something is seriously wrong.
>>
>> To prevent this register the video device at prob time and don't allow
>> user-space to open the video device if the subdevice have not yet been
>> bound.
> 
> This patch feels wrong. Creating the video device in the notify_complete seems
> right to me, so the problem is much more likely in the removal of the video device.
> 
> What *exactly* goes wrong here?
> 
> FYI: I'm taking all other patches of this series,

Oops, I saw Sakari had two comments. I'll wait for a v3 then.

If you make a v3 with Sakari's suggestions and drop this patch, then I can merge
it and make a pull request for it.

This patch can be handled separately from the other the patches.

Regards,

	Hans

 > but I leave this one out
> as I am not convinced of this patch. Having to block open() calls is always
> a bad sign.
> 
> Regards,
> 
> 	Hans
> 
>>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> ---
>>    drivers/media/platform/rcar-vin/rcar-core.c | 47 ++++++++++++++++++++++++++---
>>    drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 ++++----------------------
>>    drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
>>    3 files changed, 50 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
>> index dcca906ba58435f5..0e1757301a0bca1e 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
>> @@ -74,6 +74,7 @@ static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
>>    static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
>>    {
>>    	struct rvin_dev *vin = notifier_to_vin(notifier);
>> +	struct v4l2_subdev *sd = vin_to_source(vin);
>>    	int ret;
>>    
>>    	/* Verify subdevices mbus format */
>> @@ -92,7 +93,35 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
>>    		return ret;
>>    	}
>>    
>> -	return rvin_v4l2_probe(vin);
>> +	/* Add the controls */
>> +	/*
>> +	 * Currently the subdev with the largest number of controls (13) is
>> +	 * ov6550. So let's pick 16 as a hint for the control handler. Note
>> +	 * that this is a hint only: too large and you waste some memory, too
>> +	 * small and there is a (very) small performance hit when looking up
>> +	 * controls in the internal hash.
>> +	 */
>> +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
>> +	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>> +		return ret;
>> +
>> +	if (vin->vdev.tvnorms == 0) {
>> +		/* Disable the STD API if there are no tvnorms defined */
>> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
>> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
>> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
>> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
>> +	}
>> +
>> +	return rvin_reset_format(vin);
>>    }
>>    
>>    static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
>> @@ -102,7 +131,7 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
>>    	struct rvin_dev *vin = notifier_to_vin(notifier);
>>    
>>    	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
>> -	rvin_v4l2_remove(vin);
>> +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
>>    	vin->digital.subdev = NULL;
>>    }
>>    
>> @@ -290,9 +319,13 @@ static int rcar_vin_probe(struct platform_device *pdev)
>>    	if (ret)
>>    		return ret;
>>    
>> +	ret = rvin_v4l2_probe(vin);
>> +	if (ret)
>> +		goto error_dma;
>> +
>>    	ret = rvin_digital_graph_init(vin);
>>    	if (ret < 0)
>> -		goto error;
>> +		goto error_v4l2;
>>    
>>    	pm_suspend_ignore_children(&pdev->dev, true);
>>    	pm_runtime_enable(&pdev->dev);
>> @@ -300,7 +333,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
>>    	platform_set_drvdata(pdev, vin);
>>    
>>    	return 0;
>> -error:
>> +error_v4l2:
>> +	rvin_v4l2_remove(vin);
>> +error_dma:
>>    	rvin_dma_remove(vin);
>>    
>>    	return ret;
>> @@ -314,6 +349,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
>>    
>>    	v4l2_async_notifier_unregister(&vin->notifier);
>>    
>> +	/* Checks internaly if handlers have been init or not */
>> +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
>> +
>> +	rvin_v4l2_remove(vin);
>>    	rvin_dma_remove(vin);
>>    
>>    	return 0;
>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> index be6f41bf82ac3bc5..6f1c27fc828fe57e 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> @@ -103,7 +103,7 @@ static void rvin_reset_crop_compose(struct rvin_dev *vin)
>>    	vin->compose.height = vin->format.height;
>>    }
>>    
>> -static int rvin_reset_format(struct rvin_dev *vin)
>> +int rvin_reset_format(struct rvin_dev *vin)
>>    {
>>    	struct v4l2_subdev_format fmt = {
>>    		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> @@ -785,6 +785,11 @@ static int rvin_open(struct file *file)
>>    
>>    	mutex_lock(&vin->lock);
>>    
>> +	if (!vin->digital.subdev) {
>> +		ret = -ENODEV;
>> +		goto unlock;
>> +	}
>> +
>>    	file->private_data = vin;
>>    
>>    	ret = v4l2_fh_open(file);
>> @@ -848,9 +853,6 @@ void rvin_v4l2_remove(struct rvin_dev *vin)
>>    	v4l2_info(&vin->v4l2_dev, "Removing %s\n",
>>    		  video_device_node_name(&vin->vdev));
>>    
>> -	/* Checks internaly if handlers have been init or not */
>> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
>> -
>>    	/* Checks internaly if vdev have been init or not */
>>    	video_unregister_device(&vin->vdev);
>>    }
>> @@ -873,41 +875,10 @@ static void rvin_notify(struct v4l2_subdev *sd,
>>    int rvin_v4l2_probe(struct rvin_dev *vin)
>>    {
>>    	struct video_device *vdev = &vin->vdev;
>> -	struct v4l2_subdev *sd = vin_to_source(vin);
>>    	int ret;
>>    
>> -	v4l2_set_subdev_hostdata(sd, vin);
>> -
>>    	vin->v4l2_dev.notify = rvin_notify;
>>    
>> -	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
>> -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>> -		return ret;
>> -
>> -	if (vin->vdev.tvnorms == 0) {
>> -		/* Disable the STD API if there are no tvnorms defined */
>> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
>> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
>> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
>> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
>> -	}
>> -
>> -	/* Add the controls */
>> -	/*
>> -	 * Currently the subdev with the largest number of controls (13) is
>> -	 * ov6550. So let's pick 16 as a hint for the control handler. Note
>> -	 * that this is a hint only: too large and you waste some memory, too
>> -	 * small and there is a (very) small performance hit when looking up
>> -	 * controls in the internal hash.
>> -	 */
>> -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
>> -	if (ret < 0)
>> -		return ret;
>> -
>>    	/* video node */
>>    	vdev->fops = &rvin_fops;
>>    	vdev->v4l2_dev = &vin->v4l2_dev;
>> @@ -921,7 +892,6 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>>    		V4L2_CAP_READWRITE;
>>    
>>    	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
>> -	rvin_reset_format(vin);
>>    
>>    	ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
>>    	if (ret) {
>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
>> index 9bfb5a7c4dc4f215..9d0d4a5001b6ccd8 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
>> @@ -158,6 +158,7 @@ void rvin_dma_remove(struct rvin_dev *vin);
>>    
>>    int rvin_v4l2_probe(struct rvin_dev *vin);
>>    void rvin_v4l2_remove(struct rvin_dev *vin);
>> +int rvin_reset_format(struct rvin_dev *vin);
>>    
>>    const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat);
>>    
>>
> 

Re: [PATCH v2 15/17] rcar-vin: register the video device at probe time

[Niklas Söderlund]

Hi Hans,

Thanks for taking the time to look at this :-)

On 2017-05-29 08:56:31 +0200, Hans Verkuil wrote:
> On 05/29/2017 08:52 AM, Hans Verkuil wrote:
> > Hi Niklas,
> > 
> > On 05/24/2017 02:15 AM, Niklas Söderlund wrote:
> > > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > 
> > > The driver registers the video device from the async complete callback
> > > and unregistered in the async unbind callback. This creates problems if
> > > if the subdevice is bound, unbound and later rebound. The second time
> > > video_register_device() is called it fails:
> > > 
> > >      kobject (eb3be918): tried to init an initialized object, something is seriously wrong.
> > > 
> > > To prevent this register the video device at prob time and don't allow
> > > user-space to open the video device if the subdevice have not yet been
> > > bound.
> > 
> > This patch feels wrong. Creating the video device in the notify_complete seems
> > right to me, so the problem is much more likely in the removal of the video device.
> > 
> > What *exactly* goes wrong here

When calling video_register_device() it fails since the device structure 
have already been registered once. So it is not possible to register, 
unregister and then register the same video device struct. The other 
solution to this is to memset the whole embedded video device struct to 
zero  before initializing it and calling video_register_device(), but 
that feels more wrong. Let me know what you think and I will rework this 
patch.


> > 
> > FYI: I'm taking all other patches of this series,
> 
> Oops, I saw Sakari had two comments. I'll wait for a v3 then.
> 
> If you make a v3 with Sakari's suggestions and drop this patch, then I can merge
> it and make a pull request for it.

I can't find Sakaris comments in my inbox or on the ML for this thread.  
Where did you see them?

> 
> This patch can be handled separately from the other the patches.
> 
> Regards,
> 
> 	Hans
> 
> > but I leave this one out
> > as I am not convinced of this patch. Having to block open() calls is always
> > a bad sign.
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >    drivers/media/platform/rcar-vin/rcar-core.c | 47 ++++++++++++++++++++++++++---
> > >    drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 ++++----------------------
> > >    drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
> > >    3 files changed, 50 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index dcca906ba58435f5..0e1757301a0bca1e 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -74,6 +74,7 @@ static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
> > >    static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> > >    {
> > >    	struct rvin_dev *vin = notifier_to_vin(notifier);
> > > +	struct v4l2_subdev *sd = vin_to_source(vin);
> > >    	int ret;
> > >    	/* Verify subdevices mbus format */
> > > @@ -92,7 +93,35 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> > >    		return ret;
> > >    	}
> > > -	return rvin_v4l2_probe(vin);
> > > +	/* Add the controls */
> > > +	/*
> > > +	 * Currently the subdev with the largest number of controls (13) is
> > > +	 * ov6550. So let's pick 16 as a hint for the control handler. Note
> > > +	 * that this is a hint only: too large and you waste some memory, too
> > > +	 * small and there is a (very) small performance hit when looking up
> > > +	 * controls in the internal hash.
> > > +	 */
> > > +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> > > +	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> > > +		return ret;
> > > +
> > > +	if (vin->vdev.tvnorms == 0) {
> > > +		/* Disable the STD API if there are no tvnorms defined */
> > > +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
> > > +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
> > > +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
> > > +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
> > > +	}
> > > +
> > > +	return rvin_reset_format(vin);
> > >    }
> > >    static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> > > @@ -102,7 +131,7 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> > >    	struct rvin_dev *vin = notifier_to_vin(notifier);
> > >    	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
> > > -	rvin_v4l2_remove(vin);
> > > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > >    	vin->digital.subdev = NULL;
> > >    }
> > > @@ -290,9 +319,13 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > >    	if (ret)
> > >    		return ret;
> > > +	ret = rvin_v4l2_probe(vin);
> > > +	if (ret)
> > > +		goto error_dma;
> > > +
> > >    	ret = rvin_digital_graph_init(vin);
> > >    	if (ret < 0)
> > > -		goto error;
> > > +		goto error_v4l2;
> > >    	pm_suspend_ignore_children(&pdev->dev, true);
> > >    	pm_runtime_enable(&pdev->dev);
> > > @@ -300,7 +333,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > >    	platform_set_drvdata(pdev, vin);
> > >    	return 0;
> > > -error:
> > > +error_v4l2:
> > > +	rvin_v4l2_remove(vin);
> > > +error_dma:
> > >    	rvin_dma_remove(vin);
> > >    	return ret;
> > > @@ -314,6 +349,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
> > >    	v4l2_async_notifier_unregister(&vin->notifier);
> > > +	/* Checks internaly if handlers have been init or not */
> > > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > +
> > > +	rvin_v4l2_remove(vin);
> > >    	rvin_dma_remove(vin);
> > >    	return 0;
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > index be6f41bf82ac3bc5..6f1c27fc828fe57e 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > @@ -103,7 +103,7 @@ static void rvin_reset_crop_compose(struct rvin_dev *vin)
> > >    	vin->compose.height = vin->format.height;
> > >    }
> > > -static int rvin_reset_format(struct rvin_dev *vin)
> > > +int rvin_reset_format(struct rvin_dev *vin)
> > >    {
> > >    	struct v4l2_subdev_format fmt = {
> > >    		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > > @@ -785,6 +785,11 @@ static int rvin_open(struct file *file)
> > >    	mutex_lock(&vin->lock);
> > > +	if (!vin->digital.subdev) {
> > > +		ret = -ENODEV;
> > > +		goto unlock;
> > > +	}
> > > +
> > >    	file->private_data = vin;
> > >    	ret = v4l2_fh_open(file);
> > > @@ -848,9 +853,6 @@ void rvin_v4l2_remove(struct rvin_dev *vin)
> > >    	v4l2_info(&vin->v4l2_dev, "Removing %s\n",
> > >    		  video_device_node_name(&vin->vdev));
> > > -	/* Checks internaly if handlers have been init or not */
> > > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > -
> > >    	/* Checks internaly if vdev have been init or not */
> > >    	video_unregister_device(&vin->vdev);
> > >    }
> > > @@ -873,41 +875,10 @@ static void rvin_notify(struct v4l2_subdev *sd,
> > >    int rvin_v4l2_probe(struct rvin_dev *vin)
> > >    {
> > >    	struct video_device *vdev = &vin->vdev;
> > > -	struct v4l2_subdev *sd = vin_to_source(vin);
> > >    	int ret;
> > > -	v4l2_set_subdev_hostdata(sd, vin);
> > > -
> > >    	vin->v4l2_dev.notify = rvin_notify;
> > > -	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> > > -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> > > -		return ret;
> > > -
> > > -	if (vin->vdev.tvnorms == 0) {
> > > -		/* Disable the STD API if there are no tvnorms defined */
> > > -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
> > > -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
> > > -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
> > > -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
> > > -	}
> > > -
> > > -	/* Add the controls */
> > > -	/*
> > > -	 * Currently the subdev with the largest number of controls (13) is
> > > -	 * ov6550. So let's pick 16 as a hint for the control handler. Note
> > > -	 * that this is a hint only: too large and you waste some memory, too
> > > -	 * small and there is a (very) small performance hit when looking up
> > > -	 * controls in the internal hash.
> > > -	 */
> > > -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> > > -	if (ret < 0)
> > > -		return ret;
> > > -
> > > -	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
> > > -	if (ret < 0)
> > > -		return ret;
> > > -
> > >    	/* video node */
> > >    	vdev->fops = &rvin_fops;
> > >    	vdev->v4l2_dev = &vin->v4l2_dev;
> > > @@ -921,7 +892,6 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> > >    		V4L2_CAP_READWRITE;
> > >    	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
> > > -	rvin_reset_format(vin);
> > >    	ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
> > >    	if (ret) {
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > index 9bfb5a7c4dc4f215..9d0d4a5001b6ccd8 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > @@ -158,6 +158,7 @@ void rvin_dma_remove(struct rvin_dev *vin);
> > >    int rvin_v4l2_probe(struct rvin_dev *vin);
> > >    void rvin_v4l2_remove(struct rvin_dev *vin);
> > > +int rvin_reset_format(struct rvin_dev *vin);
> > >    const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat);
> > > 
> > 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 15/17] rcar-vin: register the video device at probe time

[Niklas Söderlund]

Hi Hans,

Thanks for taking the time to look at this :-)

On 2017-05-29 08:56:31 +0200, Hans Verkuil wrote:
> On 05/29/2017 08:52 AM, Hans Verkuil wrote:
> > Hi Niklas,
> > 
> > On 05/24/2017 02:15 AM, Niklas S�derlund wrote:
> > > From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > > 
> > > The driver registers the video device from the async complete callback
> > > and unregistered in the async unbind callback. This creates problems if
> > > if the subdevice is bound, unbound and later rebound. The second time
> > > video_register_device() is called it fails:
> > > 
> > >      kobject (eb3be918): tried to init an initialized object, something is seriously wrong.
> > > 
> > > To prevent this register the video device at prob time and don't allow
> > > user-space to open the video device if the subdevice have not yet been
> > > bound.
> > 
> > This patch feels wrong. Creating the video device in the notify_complete seems
> > right to me, so the problem is much more likely in the removal of the video device.
> > 
> > What *exactly* goes wrong here

When calling video_register_device() it fails since the device structure 
have already been registered once. So it is not possible to register, 
unregister and then register the same video device struct. The other 
solution to this is to memset the whole embedded video device struct to 
zero  before initializing it and calling video_register_device(), but 
that feels more wrong. Let me know what you think and I will rework this 
patch.


> > 
> > FYI: I'm taking all other patches of this series,
> 
> Oops, I saw Sakari had two comments. I'll wait for a v3 then.
> 
> If you make a v3 with Sakari's suggestions and drop this patch, then I can merge
> it and make a pull request for it.

I can't find Sakaris comments in my inbox or on the ML for this thread.  
Where did you see them?

> 
> This patch can be handled separately from the other the patches.
> 
> Regards,
> 
> 	Hans
> 
> > but I leave this one out
> > as I am not convinced of this patch. Having to block open() calls is always
> > a bad sign.
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > 
> > > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >    drivers/media/platform/rcar-vin/rcar-core.c | 47 ++++++++++++++++++++++++++---
> > >    drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 ++++----------------------
> > >    drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
> > >    3 files changed, 50 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index dcca906ba58435f5..0e1757301a0bca1e 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -74,6 +74,7 @@ static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
> > >    static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> > >    {
> > >    	struct rvin_dev *vin = notifier_to_vin(notifier);
> > > +	struct v4l2_subdev *sd = vin_to_source(vin);
> > >    	int ret;
> > >    	/* Verify subdevices mbus format */
> > > @@ -92,7 +93,35 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> > >    		return ret;
> > >    	}
> > > -	return rvin_v4l2_probe(vin);
> > > +	/* Add the controls */
> > > +	/*
> > > +	 * Currently the subdev with the largest number of controls (13) is
> > > +	 * ov6550. So let's pick 16 as a hint for the control handler. Note
> > > +	 * that this is a hint only: too large and you waste some memory, too
> > > +	 * small and there is a (very) small performance hit when looking up
> > > +	 * controls in the internal hash.
> > > +	 */
> > > +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> > > +	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> > > +		return ret;
> > > +
> > > +	if (vin->vdev.tvnorms == 0) {
> > > +		/* Disable the STD API if there are no tvnorms defined */
> > > +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
> > > +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
> > > +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
> > > +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
> > > +	}
> > > +
> > > +	return rvin_reset_format(vin);
> > >    }
> > >    static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> > > @@ -102,7 +131,7 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> > >    	struct rvin_dev *vin = notifier_to_vin(notifier);
> > >    	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
> > > -	rvin_v4l2_remove(vin);
> > > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > >    	vin->digital.subdev = NULL;
> > >    }
> > > @@ -290,9 +319,13 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > >    	if (ret)
> > >    		return ret;
> > > +	ret = rvin_v4l2_probe(vin);
> > > +	if (ret)
> > > +		goto error_dma;
> > > +
> > >    	ret = rvin_digital_graph_init(vin);
> > >    	if (ret < 0)
> > > -		goto error;
> > > +		goto error_v4l2;
> > >    	pm_suspend_ignore_children(&pdev->dev, true);
> > >    	pm_runtime_enable(&pdev->dev);
> > > @@ -300,7 +333,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > >    	platform_set_drvdata(pdev, vin);
> > >    	return 0;
> > > -error:
> > > +error_v4l2:
> > > +	rvin_v4l2_remove(vin);
> > > +error_dma:
> > >    	rvin_dma_remove(vin);
> > >    	return ret;
> > > @@ -314,6 +349,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
> > >    	v4l2_async_notifier_unregister(&vin->notifier);
> > > +	/* Checks internaly if handlers have been init or not */
> > > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > +
> > > +	rvin_v4l2_remove(vin);
> > >    	rvin_dma_remove(vin);
> > >    	return 0;
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > index be6f41bf82ac3bc5..6f1c27fc828fe57e 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > @@ -103,7 +103,7 @@ static void rvin_reset_crop_compose(struct rvin_dev *vin)
> > >    	vin->compose.height = vin->format.height;
> > >    }
> > > -static int rvin_reset_format(struct rvin_dev *vin)
> > > +int rvin_reset_format(struct rvin_dev *vin)
> > >    {
> > >    	struct v4l2_subdev_format fmt = {
> > >    		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > > @@ -785,6 +785,11 @@ static int rvin_open(struct file *file)
> > >    	mutex_lock(&vin->lock);
> > > +	if (!vin->digital.subdev) {
> > > +		ret = -ENODEV;
> > > +		goto unlock;
> > > +	}
> > > +
> > >    	file->private_data = vin;
> > >    	ret = v4l2_fh_open(file);
> > > @@ -848,9 +853,6 @@ void rvin_v4l2_remove(struct rvin_dev *vin)
> > >    	v4l2_info(&vin->v4l2_dev, "Removing %s\n",
> > >    		  video_device_node_name(&vin->vdev));
> > > -	/* Checks internaly if handlers have been init or not */
> > > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > -
> > >    	/* Checks internaly if vdev have been init or not */
> > >    	video_unregister_device(&vin->vdev);
> > >    }
> > > @@ -873,41 +875,10 @@ static void rvin_notify(struct v4l2_subdev *sd,
> > >    int rvin_v4l2_probe(struct rvin_dev *vin)
> > >    {
> > >    	struct video_device *vdev = &vin->vdev;
> > > -	struct v4l2_subdev *sd = vin_to_source(vin);
> > >    	int ret;
> > > -	v4l2_set_subdev_hostdata(sd, vin);
> > > -
> > >    	vin->v4l2_dev.notify = rvin_notify;
> > > -	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> > > -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> > > -		return ret;
> > > -
> > > -	if (vin->vdev.tvnorms == 0) {
> > > -		/* Disable the STD API if there are no tvnorms defined */
> > > -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
> > > -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
> > > -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
> > > -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
> > > -	}
> > > -
> > > -	/* Add the controls */
> > > -	/*
> > > -	 * Currently the subdev with the largest number of controls (13) is
> > > -	 * ov6550. So let's pick 16 as a hint for the control handler. Note
> > > -	 * that this is a hint only: too large and you waste some memory, too
> > > -	 * small and there is a (very) small performance hit when looking up
> > > -	 * controls in the internal hash.
> > > -	 */
> > > -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> > > -	if (ret < 0)
> > > -		return ret;
> > > -
> > > -	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
> > > -	if (ret < 0)
> > > -		return ret;
> > > -
> > >    	/* video node */
> > >    	vdev->fops = &rvin_fops;
> > >    	vdev->v4l2_dev = &vin->v4l2_dev;
> > > @@ -921,7 +892,6 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> > >    		V4L2_CAP_READWRITE;
> > >    	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
> > > -	rvin_reset_format(vin);
> > >    	ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
> > >    	if (ret) {
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > index 9bfb5a7c4dc4f215..9d0d4a5001b6ccd8 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > @@ -158,6 +158,7 @@ void rvin_dma_remove(struct rvin_dev *vin);
> > >    int rvin_v4l2_probe(struct rvin_dev *vin);
> > >    void rvin_v4l2_remove(struct rvin_dev *vin);
> > > +int rvin_reset_format(struct rvin_dev *vin);
> > >    const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat);
> > > 
> > 
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2 15/17] rcar-vin: register the video device at probe time

[Hans Verkuil]

On 05/29/2017 09:49 AM, Niklas Söderlund wrote:
> Hi Hans,
> 
> Thanks for taking the time to look at this :-)
> 
> On 2017-05-29 08:56:31 +0200, Hans Verkuil wrote:
>> On 05/29/2017 08:52 AM, Hans Verkuil wrote:
>>> Hi Niklas,
>>>
>>> On 05/24/2017 02:15 AM, Niklas Söderlund wrote:
>>>> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>
>>>> The driver registers the video device from the async complete callback
>>>> and unregistered in the async unbind callback. This creates problems if
>>>> if the subdevice is bound, unbound and later rebound. The second time
>>>> video_register_device() is called it fails:
>>>>
>>>>       kobject (eb3be918): tried to init an initialized object, something is seriously wrong.
>>>>
>>>> To prevent this register the video device at prob time and don't allow
>>>> user-space to open the video device if the subdevice have not yet been
>>>> bound.
>>>
>>> This patch feels wrong. Creating the video device in the notify_complete seems
>>> right to me, so the problem is much more likely in the removal of the video device.
>>>
>>> What *exactly* goes wrong here
> 
> When calling video_register_device() it fails since the device structure
> have already been registered once. So it is not possible to register,
> unregister and then register the same video device struct. The other
> solution to this is to memset the whole embedded video device struct to
> zero  before initializing it and calling video_register_device(), but
> that feels more wrong. Let me know what you think and I will rework this
> patch.
> 
> 
>>>
>>> FYI: I'm taking all other patches of this series,
>>
>> Oops, I saw Sakari had two comments. I'll wait for a v3 then.
>>
>> If you make a v3 with Sakari's suggestions and drop this patch, then I can merge
>> it and make a pull request for it.
> 
> I can't find Sakaris comments in my inbox or on the ML for this thread.
> Where did you see them?

Sorry, my mistake. Those comments were for the

"[PATCH v2 0/2] media: entity: add operation to help map DT node to media pad"

patch series.

Never mind. I'm going to merge all but this patch and get back to you on this one.

Regards,

	Hans

Re: [PATCH v2 15/17] rcar-vin: register the video device at probe time

[Niklas Söderlund]

Hi Hans,

On 2017-05-29 09:55:07 +0200, Hans Verkuil wrote:
> On 05/29/2017 09:49 AM, Niklas Söderlund wrote:
> > Hi Hans,
> > 
> > Thanks for taking the time to look at this :-)
> > 
> > On 2017-05-29 08:56:31 +0200, Hans Verkuil wrote:
> > > On 05/29/2017 08:52 AM, Hans Verkuil wrote:
> > > > Hi Niklas,
> > > > 
> > > > On 05/24/2017 02:15 AM, Niklas Söderlund wrote:
> > > > > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > > 
> > > > > The driver registers the video device from the async complete callback
> > > > > and unregistered in the async unbind callback. This creates problems if
> > > > > if the subdevice is bound, unbound and later rebound. The second time
> > > > > video_register_device() is called it fails:
> > > > > 
> > > > >       kobject (eb3be918): tried to init an initialized object, something is seriously wrong.
> > > > > 
> > > > > To prevent this register the video device at prob time and don't allow
> > > > > user-space to open the video device if the subdevice have not yet been
> > > > > bound.
> > > > 
> > > > This patch feels wrong. Creating the video device in the notify_complete seems
> > > > right to me, so the problem is much more likely in the removal of the video device.
> > > > 
> > > > What *exactly* goes wrong here
> > 
> > When calling video_register_device() it fails since the device structure
> > have already been registered once. So it is not possible to register,
> > unregister and then register the same video device struct. The other
> > solution to this is to memset the whole embedded video device struct to
> > zero  before initializing it and calling video_register_device(), but
> > that feels more wrong. Let me know what you think and I will rework this
> > patch.
> > 
> > 
> > > > 
> > > > FYI: I'm taking all other patches of this series,
> > > 
> > > Oops, I saw Sakari had two comments. I'll wait for a v3 then.
> > > 
> > > If you make a v3 with Sakari's suggestions and drop this patch, then I can merge
> > > it and make a pull request for it.
> > 
> > I can't find Sakaris comments in my inbox or on the ML for this thread.
> > Where did you see them?
> 
> Sorry, my mistake. Those comments were for the
> 
> "[PATCH v2 0/2] media: entity: add operation to help map DT node to media pad"
> 
> patch series.
> 
> Never mind. I'm going to merge all but this patch and get back to you on this one.

Thanks!

> 
> Regards,
> 
> 	Hans

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 15/17] rcar-vin: register the video device at probe time

[Niklas Söderlund]

Hi Hans,

On 2017-05-29 09:55:07 +0200, Hans Verkuil wrote:
> On 05/29/2017 09:49 AM, Niklas S�derlund wrote:
> > Hi Hans,
> > 
> > Thanks for taking the time to look at this :-)
> > 
> > On 2017-05-29 08:56:31 +0200, Hans Verkuil wrote:
> > > On 05/29/2017 08:52 AM, Hans Verkuil wrote:
> > > > Hi Niklas,
> > > > 
> > > > On 05/24/2017 02:15 AM, Niklas S�derlund wrote:
> > > > > From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > > > > 
> > > > > The driver registers the video device from the async complete callback
> > > > > and unregistered in the async unbind callback. This creates problems if
> > > > > if the subdevice is bound, unbound and later rebound. The second time
> > > > > video_register_device() is called it fails:
> > > > > 
> > > > >       kobject (eb3be918): tried to init an initialized object, something is seriously wrong.
> > > > > 
> > > > > To prevent this register the video device at prob time and don't allow
> > > > > user-space to open the video device if the subdevice have not yet been
> > > > > bound.
> > > > 
> > > > This patch feels wrong. Creating the video device in the notify_complete seems
> > > > right to me, so the problem is much more likely in the removal of the video device.
> > > > 
> > > > What *exactly* goes wrong here
> > 
> > When calling video_register_device() it fails since the device structure
> > have already been registered once. So it is not possible to register,
> > unregister and then register the same video device struct. The other
> > solution to this is to memset the whole embedded video device struct to
> > zero  before initializing it and calling video_register_device(), but
> > that feels more wrong. Let me know what you think and I will rework this
> > patch.
> > 
> > 
> > > > 
> > > > FYI: I'm taking all other patches of this series,
> > > 
> > > Oops, I saw Sakari had two comments. I'll wait for a v3 then.
> > > 
> > > If you make a v3 with Sakari's suggestions and drop this patch, then I can merge
> > > it and make a pull request for it.
> > 
> > I can't find Sakaris comments in my inbox or on the ML for this thread.
> > Where did you see them?
> 
> Sorry, my mistake. Those comments were for the
> 
> "[PATCH v2 0/2] media: entity: add operation to help map DT node to media pad"
> 
> patch series.
> 
> Never mind. I'm going to merge all but this patch and get back to you on this one.

Thanks!

> 
> Regards,
> 
> 	Hans

-- 
Regards,
Niklas S�derlund