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

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

[Niklas Söderlund]

Hi,

This series fix a number of issues for the rcar-vin driver regarding 
format and capturing. It is based on top of v4.11-rc1 and is 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.

Niklas Söderlund (16):
  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: make use of video_device_alloc() and video_device_release()
  rcar-vin: add missing error check to propagate error
  rcar-vin: fix bug in pixelformat selection

 drivers/media/platform/rcar-vin/rcar-core.c |  33 +++-
 drivers/media/platform/rcar-vin/rcar-dma.c  | 229 ++++++++++++++--------------
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 127 ++++++++-------
 drivers/media/platform/rcar-vin/rcar-vin.h  |  11 +-
 4 files changed, 216 insertions(+), 184 deletions(-)

-- 
2.12.0

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

[Niklas Söderlund]

These two where 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>
---
 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.12.0

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

[Sergei Shtylyov]

Hello!

On 3/14/2017 9:59 PM, Niklas Söderlund wrote:

> These two where forgotten when refactoring the format reset code. If

    s/where/were/?

> 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>
[...]

MBR, Sergei

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:42 Niklas Söderlund wrote:
> These two where 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>

With the commit message typo fixed,

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;
>  }

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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.12.0

Re: [PATCH 02/16] rcar-vin: use rvin_reset_format() in S_DV_TIMINGS

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:43 Niklas Söderlund wrote:
> 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);

vin->source won't be updated anymore. Is this intentional ?

>  }
> 
>  static int rvin_g_dv_timings(struct file *file, void *priv_fh,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 02/16] rcar-vin: use rvin_reset_format() in S_DV_TIMINGS

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2017-05-10 16:22:16 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tuesday 14 Mar 2017 19:59:43 Niklas S�derlund wrote:
> > 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);
> 
> vin->source won't be updated anymore. Is this intentional ?

Yes this is intentional. vin->source cache the frame width and height 
from the source subdevice, and this is done in .vidioc_s_fmt_vid_cap().  

This cacheing was due to a misunderstanding on my part in the port from 
soc_camera and the whole vin->source caching is removed in later patch 
when cleaning up the scaling code in the Gen3 enablement series.

> 
> >  }
> > 
> >  static int rvin_g_dv_timings(struct file *file, void *priv_fh,
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas S�derlund

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

[Niklas Söderlund]

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 restore) 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>
---
 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.12.0

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

[Sergei Shtylyov]

Hello!

On 3/14/2017 9:59 PM, Niklas Söderlund wrote:

> 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 restore) the requested pad

    Restored?

>   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>
> ---
>  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);

    Does this still compile after you removed 'pad'?

>
> -	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);

    And this?

>
> -	cap->pad = pad;
> +	cap->pad = 0;
>
>  	return ret;
>  }
[...]

MBR, Sergei

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

[Niklas Söderlund]

Hi Sergei,

Thanks for your feedback.

On 2017-03-15 12:12:21 +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 3/14/2017 9:59 PM, Niklas S�derlund wrote:
> 
> > 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 restore) the requested pad
> 
>    Restored?

Will update for v2.

> 
> >   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>
> > ---
> >  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);
> 
>    Does this still compile after you removed 'pad'?

Yes, the pad in v4l2_subdev_call() do not refer to the pad variable but 
the pad operations of the subdevice ops struct, the macro is defined as:

#define v4l2_subdev_call(sd, o, f, args...)                     \
        (!(sd) ? -ENODEV : (((sd)->ops->o && (sd)->ops->o->f) ? \
                (sd)->ops->o->f((sd), ##args) : -ENOIOCTLCMD))

So if you expand the macro it looks like:

sd->ops->pad->enum_dv_timings(timings);

I agree it's confusing and I had the same thought the first times I 
looked at it too :-)

> 
> > 
> > -	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);
> 
>    And this?
> 
> > 
> > -	cap->pad = pad;
> > +	cap->pad = 0;
> > 
> >  	return ret;
> >  }
> [...]
> 
> MBR, Sergei
> 

-- 
Regards,
Niklas S�derlund

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:44 Niklas Söderlund wrote:
> 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 restore) 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;
>  }

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

If the subdevice supports dv_timings_cap the driver should not fill in
the standard.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 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.12.0

Re: [PATCH 04/16] rcar-vin: fix standard in input enumeration

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:45 Niklas Söderlund wrote:
> If the subdevice supports dv_timings_cap the driver should not fill in
> the standard.

A subdev could have analog TV and digital TV inputs. However, as the rcar-vin 
driver supports a single input only, this patch is correct. I'd mention this 
fact in the commit message. Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  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));

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

It makes more sens 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 sens.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 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.12.0

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:46 Niklas Söderlund wrote:
> It makes more sens 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 sens.

s/sens/sense/

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

With the typo fixed,

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;

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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 | 38 +++++++++++++++++------------
 1 file changed, 23 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..ce29a21888da48d5 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,15 @@ 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);
+	if (ret < 0)
+		return ret;
+	vin->digital.sink_pad = ret;
 
 	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
 	rvin_reset_format(vin);
-- 
2.12.0

Re: [PATCH 06/16] rcar-vin: refactor pad lookup code

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:47 Niklas Söderlund wrote:
> 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 | 38 +++++++++++++------------
>  1 file changed, 23 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..ce29a21888da48d5 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,15 @@ 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);
> +	if (ret < 0)
> +		return ret;
> +	vin->digital.sink_pad = ret;

The driver didn't previously consider the lack of a sink pad as an error. As 
camera sensor subdevs typically have no sink pad, I don't think you should 
change this.

>  	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
>  	rvin_reset_format(vin);

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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 | 32 ++++++++++++++++++++++++++++-
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 24 ----------------------
 2 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 098a0b1cc10a26ba..d7aba15f6761259b 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,12 +115,28 @@ 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.of.node == 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);
+		if (ret < 0)
+			return ret;
+		vin->digital.sink_pad = ret;
+
 		vin->digital.subdev = subdev;
+
+		vin_dbg(vin, "bound subdev %s source pad: %d sink pad: %d\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 ce29a21888da48d5..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,16 +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);
-	if (ret < 0)
-		return ret;
-	vin->digital.sink_pad = ret;
-
 	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
 	rvin_reset_format(vin);
 
-- 
2.12.0

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:48 Niklas Söderlund wrote:
> 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 | 32 +++++++++++++++++++++++++-
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 24 ----------------------
> 2 files changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 098a0b1cc10a26ba..d7aba15f6761259b 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,12 +115,28 @@ 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.of.node == 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);
> +		if (ret < 0)
> +			return ret;
> +		vin->digital.sink_pad = ret;
> +
>  		vin->digital.subdev = subdev;
> +
> +		vin_dbg(vin, "bound subdev %s source pad: %d sink pad: %d\n",
> +			subdev->name, vin->digital.source_pad,
> +			vin->digital.sink_pad);

As source_pad and sink_pad are unsigned, s/%d/%u/g

>  		return 0;
>  	}
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> ce29a21888da48d5..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,16 +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);
> -	if (ret < 0)
> -		return ret;
> -	vin->digital.sink_pad = ret;
> -
>  	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
>  	rvin_reset_format(vin);

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

Use information about pad index when enumerating mbus codes.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 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 d7aba15f6761259b..c4d4f112da0c9d45 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.12.0

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:49 Niklas Söderlund wrote:
> 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
> d7aba15f6761259b..c4d4f112da0c9d45 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) {

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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>
---
 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.12.0

Re: [PATCH 09/16] rcar-vin: decrease buffers needed to capture

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:50 Niklas Söderlund wrote:
> 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);

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 181 ++++++++++++++---------------
 1 file changed, 90 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..c10d75aa7e71d665 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,87 @@ static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
 	rvin_write(vin, offset, VNMB_REG(slot));
 }
 
+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 +887,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.12.0

Re: [PATCH 10/16] rcar-vin: move functions which acts on hardware

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:51 Niklas Söderlund wrote:
> 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>
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 181 ++++++++++++-------------
> 1 file changed, 90 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..c10d75aa7e71d665 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,87 @@ static void rvin_set_slot_addr(struct rvin_dev *vin,
> int slot, dma_addr_t addr) rvin_write(vin, offset, VNMB_REG(slot));
>  }
> 
> +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 +887,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 */

You're loosing this comment. With this fixed (or not, if there's a good 
reason),

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -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;

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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>
---
 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 c10d75aa7e71d665..f7776592b9a13d41 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -404,7 +404,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);
 
@@ -1061,22 +1075,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.12.0

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:52 Niklas Söderlund wrote:
> 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
> c10d75aa7e71d665..f7776592b9a13d41 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -404,7 +404,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);
> 
> @@ -1061,22 +1075,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);

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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 of continue to capture using the same capture mode as before
the stall allow the driver to choose between single and continuous mode
base 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 don'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>
---
 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 f7776592b9a13d41..bd1ccb70ae2bc47e 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -428,6 +428,8 @@ static int rvin_capture_start(struct rvin_dev *vin)
 
 	rvin_capture_on(vin);
 
+	vin->state = RUNNING;
+
 	return 0;
 }
 
@@ -906,7 +908,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);
@@ -968,8 +970,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 {
 		/*
@@ -1054,8 +1068,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);
 }
@@ -1072,7 +1085,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.12.0

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:53 Niklas Söderlund wrote:
> 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 of continue to capture using the same capture mode as before

s/of of continue/of continuing/

> the stall allow the driver to choose between single and continuous mode
> base on free buffer availability. Do this by stopping capturing when the

s/base/based/

> 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 don't feed it buffers fast enough. Or the other way around, if

s/don't/doesn't/

> 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>

I have a feeling that the streaming code is a bit fragile, but it doesn't seem 
that this patch is making it worse, so we can rework it later.

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
> f7776592b9a13d41..bd1ccb70ae2bc47e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -428,6 +428,8 @@ static int rvin_capture_start(struct rvin_dev *vin)
> 
>  	rvin_capture_on(vin);
> 
> +	vin->state = RUNNING;
> +
>  	return 0;
>  }
> 
> @@ -906,7 +908,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);
> @@ -968,8 +970,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 {
>  		/*
> @@ -1054,8 +1068,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);
>  }
> @@ -1072,7 +1085,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);

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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>
---
 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 bd1ccb70ae2bc47e..c5fa176ac9d8cc4a 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -396,12 +396,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;
@@ -435,7 +429,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.12.0

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:54 Niklas Söderlund wrote:
> 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
> bd1ccb70ae2bc47e..c5fa176ac9d8cc4a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -396,12 +396,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;
> @@ -435,7 +429,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);

-- 
Regards,

Laurent Pinchart

[PATCH 14/16] rcar-vin: make use of video_device_alloc() and video_device_release()

[Niklas Söderlund]

Make use of the helper functions video_device_alloc() and
video_device_release() to control the lifetime of the struct
video_device.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index be6f41bf82ac3bc5..c40f5bc3e3d26472 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -489,7 +489,7 @@ static int rvin_enum_input(struct file *file, void *priv,
 		i->std = 0;
 	} else {
 		i->capabilities = V4L2_IN_CAP_STD;
-		i->std = vin->vdev.tvnorms;
+		i->std = vin->vdev->tvnorms;
 	}
 
 	strlcpy(i->name, "Camera", sizeof(i->name));
@@ -752,8 +752,8 @@ static int rvin_initialize_device(struct file *file)
 	if (ret < 0)
 		return ret;
 
-	pm_runtime_enable(&vin->vdev.dev);
-	ret = pm_runtime_resume(&vin->vdev.dev);
+	pm_runtime_enable(&vin->vdev->dev);
+	ret = pm_runtime_resume(&vin->vdev->dev);
 	if (ret < 0 && ret != -ENOSYS)
 		goto eresume;
 
@@ -771,7 +771,7 @@ static int rvin_initialize_device(struct file *file)
 
 	return 0;
 esfmt:
-	pm_runtime_disable(&vin->vdev.dev);
+	pm_runtime_disable(&vin->vdev->dev);
 eresume:
 	rvin_power_off(vin);
 
@@ -823,8 +823,8 @@ static int rvin_release(struct file *file)
 	 * Then de-initialize hw module.
 	 */
 	if (fh_singular) {
-		pm_runtime_suspend(&vin->vdev.dev);
-		pm_runtime_disable(&vin->vdev.dev);
+		pm_runtime_suspend(&vin->vdev->dev);
+		pm_runtime_disable(&vin->vdev->dev);
 		rvin_power_off(vin);
 	}
 
@@ -846,13 +846,13 @@ static const struct v4l2_file_operations rvin_fops = {
 void rvin_v4l2_remove(struct rvin_dev *vin)
 {
 	v4l2_info(&vin->v4l2_dev, "Removing %s\n",
-		  video_device_node_name(&vin->vdev));
+		  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);
+	video_unregister_device(vin->vdev);
 }
 
 static void rvin_notify(struct v4l2_subdev *sd,
@@ -863,7 +863,7 @@ static void rvin_notify(struct v4l2_subdev *sd,
 
 	switch (notification) {
 	case V4L2_DEVICE_NOTIFY_EVENT:
-		v4l2_event_queue(&vin->vdev, arg);
+		v4l2_event_queue(vin->vdev, arg);
 		break;
 	default:
 		break;
@@ -872,7 +872,7 @@ static void rvin_notify(struct v4l2_subdev *sd,
 
 int rvin_v4l2_probe(struct rvin_dev *vin)
 {
-	struct video_device *vdev = &vin->vdev;
+	struct video_device *vdev;
 	struct v4l2_subdev *sd = vin_to_source(vin);
 	int ret;
 
@@ -880,16 +880,18 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 
 	vin->v4l2_dev.notify = rvin_notify;
 
-	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
+	vdev = video_device_alloc();
+
+	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vdev->tvnorms);
 	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
 		return ret;
 
-	if (vin->vdev.tvnorms == 0) {
+	if (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);
+		v4l2_disable_ioctl(vdev, VIDIOC_G_STD);
+		v4l2_disable_ioctl(vdev, VIDIOC_S_STD);
+		v4l2_disable_ioctl(vdev, VIDIOC_QUERYSTD);
+		v4l2_disable_ioctl(vdev, VIDIOC_ENUMSTD);
 	}
 
 	/* Add the controls */
@@ -913,7 +915,7 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 	vdev->v4l2_dev = &vin->v4l2_dev;
 	vdev->queue = &vin->queue;
 	strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
-	vdev->release = video_device_release_empty;
+	vdev->release = video_device_release;
 	vdev->ioctl_ops = &rvin_ioctl_ops;
 	vdev->lock = &vin->lock;
 	vdev->ctrl_handler = &vin->ctrl_handler;
@@ -923,16 +925,18 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
 	rvin_reset_format(vin);
 
-	ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
+	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
 	if (ret) {
 		vin_err(vin, "Failed to register video device\n");
 		return ret;
 	}
 
-	video_set_drvdata(&vin->vdev, vin);
+	video_set_drvdata(vdev, vin);
 
 	v4l2_info(&vin->v4l2_dev, "Device registered as %s\n",
-		  video_device_node_name(&vin->vdev));
+		  video_device_node_name(vdev));
+
+	vin->vdev = vdev;
 
 	return ret;
 }
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 9bfb5a7c4dc4f215..9454ef80bc2b3961 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -122,7 +122,7 @@ struct rvin_dev {
 	void __iomem *base;
 	enum chip_id chip;
 
-	struct video_device vdev;
+	struct video_device *vdev;
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_async_notifier notifier;
-- 
2.12.0

Re: [PATCH 14/16] rcar-vin: make use of video_device_alloc() and video_device_release()

[Laurent Pinchart]

Hi Niklas,

On Tuesday 14 Mar 2017 19:59:55 Niklas Söderlund wrote:
> Make use of the helper functions video_device_alloc() and
> video_device_release() to control the lifetime of the struct
> video_device.

It's nice to see you considering lifetime management issues, but this isn't 
enough. The rvin_release() function accesses the rvin_dev structure, so you 
need to keep this around until all references to the video device have been 
dropped. This patch won't do so.

I would instead keep the video_device instance embedded in rvin_dev, and 
implement a custom release handler that will kfree() the rvin_dev instance. 
You will obviously need to replace devm_kzalloc() with kzalloc() to allocate 
the rvin_dev.

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 44 ++++++++++++++------------
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  2 +-
>  2 files changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> be6f41bf82ac3bc5..c40f5bc3e3d26472 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -489,7 +489,7 @@ static int rvin_enum_input(struct file *file, void
> *priv, i->std = 0;
>  	} else {
>  		i->capabilities = V4L2_IN_CAP_STD;
> -		i->std = vin->vdev.tvnorms;
> +		i->std = vin->vdev->tvnorms;
>  	}
> 
>  	strlcpy(i->name, "Camera", sizeof(i->name));
> @@ -752,8 +752,8 @@ static int rvin_initialize_device(struct file *file)
>  	if (ret < 0)
>  		return ret;
> 
> -	pm_runtime_enable(&vin->vdev.dev);
> -	ret = pm_runtime_resume(&vin->vdev.dev);
> +	pm_runtime_enable(&vin->vdev->dev);
> +	ret = pm_runtime_resume(&vin->vdev->dev);
>  	if (ret < 0 && ret != -ENOSYS)
>  		goto eresume;
> 
> @@ -771,7 +771,7 @@ static int rvin_initialize_device(struct file *file)
> 
>  	return 0;
>  esfmt:
> -	pm_runtime_disable(&vin->vdev.dev);
> +	pm_runtime_disable(&vin->vdev->dev);
>  eresume:
>  	rvin_power_off(vin);
> 
> @@ -823,8 +823,8 @@ static int rvin_release(struct file *file)
>  	 * Then de-initialize hw module.
>  	 */
>  	if (fh_singular) {
> -		pm_runtime_suspend(&vin->vdev.dev);
> -		pm_runtime_disable(&vin->vdev.dev);
> +		pm_runtime_suspend(&vin->vdev->dev);
> +		pm_runtime_disable(&vin->vdev->dev);
>  		rvin_power_off(vin);
>  	}
> 
> @@ -846,13 +846,13 @@ static const struct v4l2_file_operations rvin_fops = {
> void rvin_v4l2_remove(struct rvin_dev *vin)
>  {
>  	v4l2_info(&vin->v4l2_dev, "Removing %s\n",
> -		  video_device_node_name(&vin->vdev));
> +		  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);
> +	video_unregister_device(vin->vdev);
>  }
> 
>  static void rvin_notify(struct v4l2_subdev *sd,
> @@ -863,7 +863,7 @@ static void rvin_notify(struct v4l2_subdev *sd,
> 
>  	switch (notification) {
>  	case V4L2_DEVICE_NOTIFY_EVENT:
> -		v4l2_event_queue(&vin->vdev, arg);
> +		v4l2_event_queue(vin->vdev, arg);
>  		break;
>  	default:
>  		break;
> @@ -872,7 +872,7 @@ static void rvin_notify(struct v4l2_subdev *sd,
> 
>  int rvin_v4l2_probe(struct rvin_dev *vin)
>  {
> -	struct video_device *vdev = &vin->vdev;
> +	struct video_device *vdev;
>  	struct v4l2_subdev *sd = vin_to_source(vin);
>  	int ret;
> 
> @@ -880,16 +880,18 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> 
>  	vin->v4l2_dev.notify = rvin_notify;
> 
> -	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> +	vdev = video_device_alloc();
> +
> +	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vdev->tvnorms);
>  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>  		return ret;
> 
> -	if (vin->vdev.tvnorms == 0) {
> +	if (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);
> +		v4l2_disable_ioctl(vdev, VIDIOC_G_STD);
> +		v4l2_disable_ioctl(vdev, VIDIOC_S_STD);
> +		v4l2_disable_ioctl(vdev, VIDIOC_QUERYSTD);
> +		v4l2_disable_ioctl(vdev, VIDIOC_ENUMSTD);
>  	}
> 
>  	/* Add the controls */
> @@ -913,7 +915,7 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>  	vdev->v4l2_dev = &vin->v4l2_dev;
>  	vdev->queue = &vin->queue;
>  	strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
> -	vdev->release = video_device_release_empty;
> +	vdev->release = video_device_release;
>  	vdev->ioctl_ops = &rvin_ioctl_ops;
>  	vdev->lock = &vin->lock;
>  	vdev->ctrl_handler = &vin->ctrl_handler;
> @@ -923,16 +925,18 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>  	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
>  	rvin_reset_format(vin);
> 
> -	ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
> +	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
>  	if (ret) {
>  		vin_err(vin, "Failed to register video device\n");
>  		return ret;
>  	}
> 
> -	video_set_drvdata(&vin->vdev, vin);
> +	video_set_drvdata(vdev, vin);
> 
>  	v4l2_info(&vin->v4l2_dev, "Device registered as %s\n",
> -		  video_device_node_name(&vin->vdev));
> +		  video_device_node_name(vdev));
> +
> +	vin->vdev = vdev;
> 
>  	return ret;
>  }
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> 9bfb5a7c4dc4f215..9454ef80bc2b3961 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -122,7 +122,7 @@ struct rvin_dev {
>  	void __iomem *base;
>  	enum chip_id chip;
> 
> -	struct video_device vdev;
> +	struct video_device *vdev;
>  	struct v4l2_device v4l2_dev;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_async_notifier notifier;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 14/16] rcar-vin: make use of video_device_alloc() and video_device_release()

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2017-05-10 16:36:03 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Tuesday 14 Mar 2017 19:59:55 Niklas S�derlund wrote:
> > Make use of the helper functions video_device_alloc() and
> > video_device_release() to control the lifetime of the struct
> > video_device.
> 
> It's nice to see you considering lifetime management issues, but this isn't 
> enough. The rvin_release() function accesses the rvin_dev structure, so you 
> need to keep this around until all references to the video device have been 
> dropped. This patch won't do so.

I see your point, and it's a good catch I missed!

> 
> I would instead keep the video_device instance embedded in rvin_dev, and 
> implement a custom release handler that will kfree() the rvin_dev instance. 
> You will obviously need to replace devm_kzalloc() with kzalloc() to allocate 
> the rvin_dev.

Would it not be simpler to remove the usage of the video device from 
rvin_release()? When I check the code the only usage of vin->vdev in 
paths from the rvin_release() is in relation to pm_runtime_* calls like:

pm_runtime_suspend(&vin->vdev->dev);
pm_runtime_disable(&vin->vdev->dev);

And those can just as easily (and probably should) be called like:

pm_runtime_suspend(&vin->dev);
pm_runtime_disable(&vin->dev);

I had plan to fix the usage of the PM calls at a later time when also 
addressing suspend/resume for this driver, but cleaning up the PM calls 
can just as easily be done now.

I think it's better to use the helper functions to manage the video 
device if its possible, do you agree with this?

> 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 44 ++++++++++++++------------
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  2 +-
> >  2 files changed, 25 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > be6f41bf82ac3bc5..c40f5bc3e3d26472 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -489,7 +489,7 @@ static int rvin_enum_input(struct file *file, void
> > *priv, i->std = 0;
> >  	} else {
> >  		i->capabilities = V4L2_IN_CAP_STD;
> > -		i->std = vin->vdev.tvnorms;
> > +		i->std = vin->vdev->tvnorms;
> >  	}
> > 
> >  	strlcpy(i->name, "Camera", sizeof(i->name));
> > @@ -752,8 +752,8 @@ static int rvin_initialize_device(struct file *file)
> >  	if (ret < 0)
> >  		return ret;
> > 
> > -	pm_runtime_enable(&vin->vdev.dev);
> > -	ret = pm_runtime_resume(&vin->vdev.dev);
> > +	pm_runtime_enable(&vin->vdev->dev);
> > +	ret = pm_runtime_resume(&vin->vdev->dev);
> >  	if (ret < 0 && ret != -ENOSYS)
> >  		goto eresume;
> > 
> > @@ -771,7 +771,7 @@ static int rvin_initialize_device(struct file *file)
> > 
> >  	return 0;
> >  esfmt:
> > -	pm_runtime_disable(&vin->vdev.dev);
> > +	pm_runtime_disable(&vin->vdev->dev);
> >  eresume:
> >  	rvin_power_off(vin);
> > 
> > @@ -823,8 +823,8 @@ static int rvin_release(struct file *file)
> >  	 * Then de-initialize hw module.
> >  	 */
> >  	if (fh_singular) {
> > -		pm_runtime_suspend(&vin->vdev.dev);
> > -		pm_runtime_disable(&vin->vdev.dev);
> > +		pm_runtime_suspend(&vin->vdev->dev);
> > +		pm_runtime_disable(&vin->vdev->dev);
> >  		rvin_power_off(vin);
> >  	}
> > 
> > @@ -846,13 +846,13 @@ static const struct v4l2_file_operations rvin_fops = {
> > void rvin_v4l2_remove(struct rvin_dev *vin)
> >  {
> >  	v4l2_info(&vin->v4l2_dev, "Removing %s\n",
> > -		  video_device_node_name(&vin->vdev));
> > +		  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);
> > +	video_unregister_device(vin->vdev);
> >  }
> > 
> >  static void rvin_notify(struct v4l2_subdev *sd,
> > @@ -863,7 +863,7 @@ static void rvin_notify(struct v4l2_subdev *sd,
> > 
> >  	switch (notification) {
> >  	case V4L2_DEVICE_NOTIFY_EVENT:
> > -		v4l2_event_queue(&vin->vdev, arg);
> > +		v4l2_event_queue(vin->vdev, arg);
> >  		break;
> >  	default:
> >  		break;
> > @@ -872,7 +872,7 @@ static void rvin_notify(struct v4l2_subdev *sd,
> > 
> >  int rvin_v4l2_probe(struct rvin_dev *vin)
> >  {
> > -	struct video_device *vdev = &vin->vdev;
> > +	struct video_device *vdev;
> >  	struct v4l2_subdev *sd = vin_to_source(vin);
> >  	int ret;
> > 
> > @@ -880,16 +880,18 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> > 
> >  	vin->v4l2_dev.notify = rvin_notify;
> > 
> > -	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> > +	vdev = video_device_alloc();
> > +
> > +	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vdev->tvnorms);
> >  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> >  		return ret;
> > 
> > -	if (vin->vdev.tvnorms == 0) {
> > +	if (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);
> > +		v4l2_disable_ioctl(vdev, VIDIOC_G_STD);
> > +		v4l2_disable_ioctl(vdev, VIDIOC_S_STD);
> > +		v4l2_disable_ioctl(vdev, VIDIOC_QUERYSTD);
> > +		v4l2_disable_ioctl(vdev, VIDIOC_ENUMSTD);
> >  	}
> > 
> >  	/* Add the controls */
> > @@ -913,7 +915,7 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> >  	vdev->v4l2_dev = &vin->v4l2_dev;
> >  	vdev->queue = &vin->queue;
> >  	strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
> > -	vdev->release = video_device_release_empty;
> > +	vdev->release = video_device_release;
> >  	vdev->ioctl_ops = &rvin_ioctl_ops;
> >  	vdev->lock = &vin->lock;
> >  	vdev->ctrl_handler = &vin->ctrl_handler;
> > @@ -923,16 +925,18 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> >  	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
> >  	rvin_reset_format(vin);
> > 
> > -	ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
> > +	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> >  	if (ret) {
> >  		vin_err(vin, "Failed to register video device\n");
> >  		return ret;
> >  	}
> > 
> > -	video_set_drvdata(&vin->vdev, vin);
> > +	video_set_drvdata(vdev, vin);
> > 
> >  	v4l2_info(&vin->v4l2_dev, "Device registered as %s\n",
> > -		  video_device_node_name(&vin->vdev));
> > +		  video_device_node_name(vdev));
> > +
> > +	vin->vdev = vdev;
> > 
> >  	return ret;
> >  }
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> > b/drivers/media/platform/rcar-vin/rcar-vin.h index
> > 9bfb5a7c4dc4f215..9454ef80bc2b3961 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -122,7 +122,7 @@ struct rvin_dev {
> >  	void __iomem *base;
> >  	enum chip_id chip;
> > 
> > -	struct video_device vdev;
> > +	struct video_device *vdev;
> >  	struct v4l2_device v4l2_dev;
> >  	struct v4l2_ctrl_handler ctrl_handler;
> >  	struct v4l2_async_notifier notifier;
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 14/16] rcar-vin: make use of video_device_alloc() and video_device_release()

[Laurent Pinchart]

Hi Niklas,

On Saturday 20 May 2017 20:27:41 Niklas Söderlund wrote:
> On 2017-05-10 16:36:03 +0300, Laurent Pinchart wrote:
> > On Tuesday 14 Mar 2017 19:59:55 Niklas Söderlund wrote:
> >> Make use of the helper functions video_device_alloc() and
> >> video_device_release() to control the lifetime of the struct
> >> video_device.
> > 
> > It's nice to see you considering lifetime management issues, but this
> > isn't enough. The rvin_release() function accesses the rvin_dev structure,
> > so you need to keep this around until all references to the video device
> > have been dropped. This patch won't do so.
> 
> I see your point, and it's a good catch I missed!
> 
> > I would instead keep the video_device instance embedded in rvin_dev, and
> > implement a custom release handler that will kfree() the rvin_dev
> > instance. You will obviously need to replace devm_kzalloc() with kzalloc()
> > to allocate the rvin_dev.
> 
> Would it not be simpler to remove the usage of the video device from
> rvin_release()? When I check the code the only usage of vin->vdev in
> paths from the rvin_release() is in relation to pm_runtime_* calls like:
> 
> pm_runtime_suspend(&vin->vdev->dev);
> pm_runtime_disable(&vin->vdev->dev);
> 
> And those can just as easily (and probably should) be called like:
> 
> pm_runtime_suspend(&vin->dev);
> pm_runtime_disable(&vin->dev);
> 
> I had plan to fix the usage of the PM calls at a later time when also
> addressing suspend/resume for this driver, but cleaning up the PM calls
> can just as easily be done now.

You would still access the vin structure, so you need to refcount that one 
anyway. Refcounting of video_device then comes for free if you embed it in the 
vin structure. It's actually simpler to embed the video_device instance than 
managing its lifetime separately.

> I think it's better to use the helper functions to manage the video
> device if its possible, do you agree with this?

Only when it makes sense :-) The video_device_release_empty() and 
video_device_release() functions were bad idea, they completely circumvent 
lifetime management. We need to go in the other direction in the V4L2 core.

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


-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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>
---
 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 c40f5bc3e3d26472..956092ba6ef9bc6f 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.12.0

Re: [PATCH 15/16] rcar-vin: add missing error check to propagate error

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:56 Niklas Söderlund wrote:
> 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
> c40f5bc3e3d26472..956092ba6ef9bc6f 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:

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

If the requested pixelformat is not supported only revert to the current
pixelformat, do not revert the entire format. Also if the pixelformat
needs to be reverted the pixel information needs to be fetched once
more.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 956092ba6ef9bc6f..27b7733e96afe3e9 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -226,9 +226,8 @@ static int __rvin_try_format(struct rvin_dev *vin,
 	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;
+		pix->pixelformat = vin->format.pixelformat;
+		info = rvin_format_from_pixel(pix->pixelformat);
 	}
 
 	/* Always recalculate */
-- 
2.12.0

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:57 Niklas Söderlund wrote:
> If the requested pixelformat is not supported only revert to the current
> pixelformat, do not revert the entire format. Also if the pixelformat
> needs to be reverted the pixel information needs to be fetched once
> more.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 956092ba6ef9bc6f..27b7733e96afe3e9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -226,9 +226,8 @@ static int __rvin_try_format(struct rvin_dev *vin,
>  	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;
> +		pix->pixelformat = vin->format.pixelformat;

You should set a fixed default in this case to achieve a more deterministic 
behaviour. You can for instance pick the first entry of, which will also save 
you from calling rvin_format_from_pixel() as you can then replace it with 
&rvin_formats[0].

> +		info = rvin_format_from_pixel(pix->pixelformat);
>  	}
> 
>  	/* Always recalculate */

-- 
Regards,

Laurent Pinchart