[PATCH/RFC v2 00/15] Add multiplexed pad streaming support

[PATCH/RFC v2 00/15] Add multiplexed pad streaming support

[Niklas Söderlund]

Hi,

This is the second attempt to add streaming support to multiplexed pads.  
The first attempt was not aware of Sakari's work. His work have now been 
taken into account and this series depends on his series together with 
the master of media-tree.

git://linuxtv.org/sailus/media_tree.git#vc

It also depends on the latest out-of-tree patches for R-Car VIN and 
CSI-2 as these drivers together with the in-tree driver adv748x have 
been used to prove functionality of this series. Test procedure includes 
changing which CSI-2 VC the adv7482 outputs on (using the module 
parameter introduced in this patch-set) and verify that the R-Car CSI-2 
and VIN can receive that particular VC.

A second hardware setup have also been used to verify functionality 
based on the MAX9286 chip, which in contrast to the outputs multiple 
CSI-2 virtual channels. Unfortunate the driver side for the MAX9286 and 
the sensors RDACM20 is still in a prototype stage so the patches to 
enable multiplexed pads on that setup is not included in this patch-set.

The problem this patch-set is trying to solve is that there is no way in 
the v4l2 framework to describe and control links between subdevices 
which carry more then one video stream, for example a CSI-2 bus which 
can have 4 virtual channels carrying different video streams.

The idea is that on both sides of the multiplexed media link there are
one multiplexer subdevice and one demultiplexer subdevice. These two
subdevices can't do any format conversions, there sole purpose is to
(de)multiplex the CSI-2 link. If there is hardware which can do both
CSI-2 multiplexing and format conversions they can be modeled as two
subdevices from the same device driver.

        +------------------+              +------------------+
     +-------+  subdev 1   |              |  subdev 2   +-------+
  +--+ Pad 1 |             |              |             | Pad 3 +---+
     +--+----+   +---------+---+      +---+---------+   +----+--+
        |        | Muxed pad A +------+ Muxed pad B |        |
     +--+----+   +---------+---+      +---+---------+   +----+--+
  +--+ Pad 2 |             |              |             | Pad 4 +---+
     +-------+             |              |             +-------+
        +------------------+              +------------------+

In the example above Pad 1 is routed to Pad 3 and Pad 2 to Pad 4,
and the video data for both of them travels the link between pad A and
B. The routing between the pads inside subdev 1 and subdev 2 are 
controlled and communicated to user-space using the [GS]_ROUTING subdev 
ioctls (from Sakari's patch-set). I have patches for v4l2-ctl which 
creates a user-space interface for these now ioctls which I will post in 
a separate thread. These routes are also used to perform format 
validation between pad 1-3 and pad 2-4, the format validation is also 
part of Sakari's patch-set.

Obviously PATCH 01/15 is a RFC and if it is judged to be OK it should be 
split out to a separate patch and updated to move the .s_stream() 
operation from video ops to pad ops instead of adding a new one. I have 
posted a similar patch for this last year but it did not get much 
attention. For this RFC it's enough to add a new operation as to prove 
functionality.

A big thanks to Laurent and Sakari for being really nice and taking time
helping me grasp all the possibilities and issues with this problem, all
cred to them and all blame to me for misunderstanding there guidance :-)

Niklas Söderlund (15):
  v4l2-subdev.h: add pad and stream aware s_stream
  rcar-vin: use pad as the starting point for a pipeline
  rcar-vin: use the pad and stream aware s_stream
  rcar-csi2: switch to pad and stream aware s_stream
  rcar-csi2: count usage for each source pad
  rcar-csi2: use frame description information when propagating
    .s_stream()
  rcar-csi2: use frame description information to configure CSI-2 bus
  rcar-csi2: add get_routing support
  adv748x: csi2: add module param for virtual channel
  adv748x: csi2: add translation from pixelcode to CSI-2 datatype
  adv748x: csi2: implement get_frame_desc
  adv748x: csi2: switch to pad and stream aware s_stream
  adv748x: csi2: only allow formats on sink pads
  adv748x: csi2: add get_routing support
  adv748x: afe: add routing support

 drivers/media/i2c/adv748x/adv748x-afe.c     |  66 +++++++
 drivers/media/i2c/adv748x/adv748x-core.c    |  10 ++
 drivers/media/i2c/adv748x/adv748x-csi2.c    |  96 +++++++++-
 drivers/media/i2c/adv748x/adv748x.h         |   1 +
 drivers/media/platform/rcar-vin/rcar-csi2.c | 267 +++++++++++++++++++---------
 drivers/media/platform/rcar-vin/rcar-dma.c  |  14 +-
 include/media/v4l2-subdev.h                 |   5 +
 7 files changed, 365 insertions(+), 94 deletions(-)

-- 
2.15.1

[PATCH/RFC v2 01/15] v4l2-subdev.h: add pad and stream aware s_stream

[Niklas Söderlund]

To be able to start and stop individual streams of a multiplexed pad the
s_stream operation needs to be both pad and stream aware. Add a new
operation to pad ops to facilitate this.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 include/media/v4l2-subdev.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a30a94fad8dbacde..7288209338a48fda 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -669,6 +669,9 @@ struct v4l2_subdev_pad_config {
  *
  * @set_frame_desc: set the low level media bus frame parameters, @fd array
  *                  may be adjusted by the subdev driver to device capabilities.
+ *
+ * @s_stream: used to notify the driver that a stream will start or has
+ *	stopped.
  */
 struct v4l2_subdev_pad_ops {
 	int (*init_cfg)(struct v4l2_subdev *sd,
@@ -713,6 +716,8 @@ struct v4l2_subdev_pad_ops {
 			   struct v4l2_subdev_routing *route);
 	int (*set_routing)(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_routing *route);
+	int (*s_stream)(struct v4l2_subdev *sd, unsigned int pad,
+			unsigned int stream, int enable);
 };
 
 /**
-- 
2.15.1

Re: [PATCH/RFC v2 01/15] v4l2-subdev.h: add pad and stream aware s_stream

[Sakari Ailus]

Hejssan Niklas,

Tack för uppdaterade lappor!

On Thu, Dec 14, 2017 at 08:08:21PM +0100, Niklas Söderlund wrote:
> To be able to start and stop individual streams of a multiplexed pad the
> s_stream operation needs to be both pad and stream aware. Add a new
> operation to pad ops to facilitate this.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  include/media/v4l2-subdev.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a30a94fad8dbacde..7288209338a48fda 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -669,6 +669,9 @@ struct v4l2_subdev_pad_config {
>   *
>   * @set_frame_desc: set the low level media bus frame parameters, @fd array
>   *                  may be adjusted by the subdev driver to device capabilities.
> + *
> + * @s_stream: used to notify the driver that a stream will start or has
> + *	stopped.

This is actually the callback which is used to control the stream state.
The above suggests that it's a notification of something that has happened
(or about to happen). How about:

Enable or disable streaming on a sub-device pad.

>   */
>  struct v4l2_subdev_pad_ops {
>  	int (*init_cfg)(struct v4l2_subdev *sd,
> @@ -713,6 +716,8 @@ struct v4l2_subdev_pad_ops {
>  			   struct v4l2_subdev_routing *route);
>  	int (*set_routing)(struct v4l2_subdev *sd,
>  			   struct v4l2_subdev_routing *route);
> +	int (*s_stream)(struct v4l2_subdev *sd, unsigned int pad,
> +			unsigned int stream, int enable);

How about bool for enable?

>  };
>  
>  /**

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH/RFC v2 01/15] v4l2-subdev.h: add pad and stream aware s_stream

[Niklas Söderlund]

Hej Sakari,

Tack för dina kommentarer.

On 2017-12-15 13:51:46 +0200, Sakari Ailus wrote:
> Hejssan Niklas,
> 
> Tack för uppdaterade lappor!
> 
> On Thu, Dec 14, 2017 at 08:08:21PM +0100, Niklas Söderlund wrote:
> > To be able to start and stop individual streams of a multiplexed pad the
> > s_stream operation needs to be both pad and stream aware. Add a new
> > operation to pad ops to facilitate this.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  include/media/v4l2-subdev.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index a30a94fad8dbacde..7288209338a48fda 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -669,6 +669,9 @@ struct v4l2_subdev_pad_config {
> >   *
> >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> >   *                  may be adjusted by the subdev driver to device capabilities.
> > + *
> > + * @s_stream: used to notify the driver that a stream will start or has
> > + *	stopped.
> 
> This is actually the callback which is used to control the stream state.
> The above suggests that it's a notification of something that has happened
> (or about to happen). How about:
> 
> Enable or disable streaming on a sub-device pad.

Better, will use it in next version.

> 
> >   */
> >  struct v4l2_subdev_pad_ops {
> >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > @@ -713,6 +716,8 @@ struct v4l2_subdev_pad_ops {
> >  			   struct v4l2_subdev_routing *route);
> >  	int (*set_routing)(struct v4l2_subdev *sd,
> >  			   struct v4l2_subdev_routing *route);
> > +	int (*s_stream)(struct v4l2_subdev *sd, unsigned int pad,
> > +			unsigned int stream, int enable);
> 
> How about bool for enable?

I tried to use what the current s_stream uses, but I be happy to make 
enable a bool in the next version.

> 
> >  };
> >  
> >  /**
> 
> -- 
> Sakari Ailus
> sakari.ailus@linux.intel.com

-- 
Regards,
Niklas Söderlund

[PATCH/RFC v2 02/15] rcar-vin: use pad as the starting point for a pipeline

[Niklas Söderlund]

The pipeline will be moved from the entity to the pads; reflect this in
the media pipeline function API.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 03a914361a33125c..cf30e5fceb1d493a 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1179,7 +1179,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
 		return -EPIPE;
 
 	if (!on) {
-		media_pipeline_stop(&vin->vdev.entity);
+		media_pipeline_stop(vin->vdev.entity.pads);
 		return v4l2_subdev_call(sd, video, s_stream, 0);
 	}
 
@@ -1235,15 +1235,15 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
 	    fmt.format.height != vin->format.height)
 		return -EPIPE;
 
-	pipe = sd->entity.pipe ? sd->entity.pipe : &vin->vdev.pipe;
-	if (media_pipeline_start(&vin->vdev.entity, pipe))
+	pipe = sd->entity.pads->pipe ? sd->entity.pads->pipe : &vin->vdev.pipe;
+	if (media_pipeline_start(vin->vdev.entity.pads, pipe))
 		return -EPIPE;
 
 	ret = v4l2_subdev_call(sd, video, s_stream, 1);
 	if (ret == -ENOIOCTLCMD)
 		ret = 0;
 	if (ret)
-		media_pipeline_stop(&vin->vdev.entity);
+		media_pipeline_stop(vin->vdev.entity.pads);
 
 	return ret;
 }
-- 
2.15.1

Re: [PATCH/RFC v2 02/15] rcar-vin: use pad as the starting point for a pipeline

[Sakari Ailus]

On Thu, Dec 14, 2017 at 08:08:22PM +0100, Niklas Söderlund wrote:
> The pipeline will be moved from the entity to the pads; reflect this in
> the media pipeline function API.

I'll merge this to "media: entity: Use pad as the starting point for a
pipeline" if you're fine with that.

I haven't compiled everything for some time, and newly added drivers may be
lacking these changes. I'll re-check that soonish.

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 03a914361a33125c..cf30e5fceb1d493a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1179,7 +1179,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
>  		return -EPIPE;
>  
>  	if (!on) {
> -		media_pipeline_stop(&vin->vdev.entity);
> +		media_pipeline_stop(vin->vdev.entity.pads);
>  		return v4l2_subdev_call(sd, video, s_stream, 0);
>  	}
>  
> @@ -1235,15 +1235,15 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
>  	    fmt.format.height != vin->format.height)
>  		return -EPIPE;
>  
> -	pipe = sd->entity.pipe ? sd->entity.pipe : &vin->vdev.pipe;
> -	if (media_pipeline_start(&vin->vdev.entity, pipe))
> +	pipe = sd->entity.pads->pipe ? sd->entity.pads->pipe : &vin->vdev.pipe;
> +	if (media_pipeline_start(vin->vdev.entity.pads, pipe))
>  		return -EPIPE;
>  
>  	ret = v4l2_subdev_call(sd, video, s_stream, 1);
>  	if (ret == -ENOIOCTLCMD)
>  		ret = 0;
>  	if (ret)
> -		media_pipeline_stop(&vin->vdev.entity);
> +		media_pipeline_stop(vin->vdev.entity.pads);
>  
>  	return ret;
>  }
> -- 
> 2.15.1
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH/RFC v2 02/15] rcar-vin: use pad as the starting point for a pipeline

[Niklas Söderlund]

Hej Sakari,

Tack för dina kommentarer.

On 2017-12-15 13:54:02 +0200, Sakari Ailus wrote:
> On Thu, Dec 14, 2017 at 08:08:22PM +0100, Niklas Söderlund wrote:
> > The pipeline will be moved from the entity to the pads; reflect this in
> > the media pipeline function API.
> 
> I'll merge this to "media: entity: Use pad as the starting point for a
> pipeline" if you're fine with that.

I'm fine with that, the issue is that the rcar-vin Gen3 driver is not 
yet upstream :-( If it makes it upstream before the work in your vc 
branch feel free to squash this in. Until then I fear I need to keep 
carry this in this series.

> 
> I haven't compiled everything for some time, and newly added drivers may be
> lacking these changes. I'll re-check that soonish.
> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 03a914361a33125c..cf30e5fceb1d493a 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -1179,7 +1179,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
> >  		return -EPIPE;
> >  
> >  	if (!on) {
> > -		media_pipeline_stop(&vin->vdev.entity);
> > +		media_pipeline_stop(vin->vdev.entity.pads);
> >  		return v4l2_subdev_call(sd, video, s_stream, 0);
> >  	}
> >  
> > @@ -1235,15 +1235,15 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
> >  	    fmt.format.height != vin->format.height)
> >  		return -EPIPE;
> >  
> > -	pipe = sd->entity.pipe ? sd->entity.pipe : &vin->vdev.pipe;
> > -	if (media_pipeline_start(&vin->vdev.entity, pipe))
> > +	pipe = sd->entity.pads->pipe ? sd->entity.pads->pipe : &vin->vdev.pipe;
> > +	if (media_pipeline_start(vin->vdev.entity.pads, pipe))
> >  		return -EPIPE;
> >  
> >  	ret = v4l2_subdev_call(sd, video, s_stream, 1);
> >  	if (ret == -ENOIOCTLCMD)
> >  		ret = 0;
> >  	if (ret)
> > -		media_pipeline_stop(&vin->vdev.entity);
> > +		media_pipeline_stop(vin->vdev.entity.pads);
> >  
> >  	return ret;
> >  }
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Sakari Ailus
> sakari.ailus@linux.intel.com

-- 
Regards,
Niklas Söderlund

Re: [PATCH/RFC v2 02/15] rcar-vin: use pad as the starting point for a pipeline

[Sakari Ailus]

On Tue, Dec 19, 2017 at 12:08:56AM +0100, Niklas Söderlund wrote:
> Hej Sakari,
> 
> Tack för dina kommentarer.
> 
> On 2017-12-15 13:54:02 +0200, Sakari Ailus wrote:
> > On Thu, Dec 14, 2017 at 08:08:22PM +0100, Niklas Söderlund wrote:
> > > The pipeline will be moved from the entity to the pads; reflect this in
> > > the media pipeline function API.
> > 
> > I'll merge this to "media: entity: Use pad as the starting point for a
> > pipeline" if you're fine with that.
> 
> I'm fine with that, the issue is that the rcar-vin Gen3 driver is not 
> yet upstream :-( If it makes it upstream before the work in your vc 
> branch feel free to squash this in. Until then I fear I need to keep 
> carry this in this series.

Oops, I thought it already was there. Anyway, no changes then.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH/RFC v2 03/15] rcar-vin: use the pad and stream aware s_stream

[Niklas Söderlund]

To work with multiplexed streams the pad and stream aware s_stream
operation needs to be used.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index cf30e5fceb1d493a..8435491535060eae 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1180,7 +1180,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
 
 	if (!on) {
 		media_pipeline_stop(vin->vdev.entity.pads);
-		return v4l2_subdev_call(sd, video, s_stream, 0);
+		return v4l2_subdev_call(sd, pad, s_stream, pad->index, 0, 0);
 	}
 
 	fmt.pad = pad->index;
@@ -1239,12 +1239,14 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
 	if (media_pipeline_start(vin->vdev.entity.pads, pipe))
 		return -EPIPE;
 
-	ret = v4l2_subdev_call(sd, video, s_stream, 1);
+	ret = v4l2_subdev_call(sd, pad, s_stream, pad->index, 0, 1);
 	if (ret == -ENOIOCTLCMD)
 		ret = 0;
 	if (ret)
 		media_pipeline_stop(vin->vdev.entity.pads);
 
+	vin_dbg(vin, "pad: %u stream: 0 enable: %d\n", pad->index, on);
+
 	return ret;
 }
 
-- 
2.15.1

Re: [PATCH/RFC v2 03/15] rcar-vin: use the pad and stream aware s_stream

[Sakari Ailus]

Hej,

On Thu, Dec 14, 2017 at 08:08:23PM +0100, Niklas Söderlund wrote:
> To work with multiplexed streams the pad and stream aware s_stream
> operation needs to be used.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index cf30e5fceb1d493a..8435491535060eae 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1180,7 +1180,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
>  
>  	if (!on) {
>  		media_pipeline_stop(vin->vdev.entity.pads);
> -		return v4l2_subdev_call(sd, video, s_stream, 0);
> +		return v4l2_subdev_call(sd, pad, s_stream, pad->index, 0, 0);

Have you thought of adding a wrapper for the s_stream callback?

I think you should either change all s_stream callbacks from video to pad,
or add a wrapper which then calls the video op instead of the pad op if the
pad op does not exist. Otherwise we again have two non-interoperable
classes of drivers for no good reason.

Thinking about it, I'm not all that certain changing all instances would be
that much work in the end; it should be done anyway. Devices that have a
single stream (i.e. everything right now) just don't care about the pad
number.

>  	}
>  
>  	fmt.pad = pad->index;
> @@ -1239,12 +1239,14 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
>  	if (media_pipeline_start(vin->vdev.entity.pads, pipe))
>  		return -EPIPE;
>  
> -	ret = v4l2_subdev_call(sd, video, s_stream, 1);
> +	ret = v4l2_subdev_call(sd, pad, s_stream, pad->index, 0, 1);
>  	if (ret == -ENOIOCTLCMD)
>  		ret = 0;
>  	if (ret)
>  		media_pipeline_stop(vin->vdev.entity.pads);
>  
> +	vin_dbg(vin, "pad: %u stream: 0 enable: %d\n", pad->index, on);
> +
>  	return ret;
>  }
>  
> -- 
> 2.15.1
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH/RFC v2 03/15] rcar-vin: use the pad and stream aware s_stream

[Niklas Söderlund]

Hej Sakari,

Tack för dina kommentarer.

On 2017-12-15 14:07:39 +0200, Sakari Ailus wrote:
> Hej,
> 
> On Thu, Dec 14, 2017 at 08:08:23PM +0100, Niklas Söderlund wrote:
> > To work with multiplexed streams the pad and stream aware s_stream
> > operation needs to be used.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index cf30e5fceb1d493a..8435491535060eae 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -1180,7 +1180,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
> >  
> >  	if (!on) {
> >  		media_pipeline_stop(vin->vdev.entity.pads);
> > -		return v4l2_subdev_call(sd, video, s_stream, 0);
> > +		return v4l2_subdev_call(sd, pad, s_stream, pad->index, 0, 0);
> 
> Have you thought of adding a wrapper for the s_stream callback?

I toyed with the idea, then I reached the same conclusion you do bellow.

> 
> I think you should either change all s_stream callbacks from video to pad,
> or add a wrapper which then calls the video op instead of the pad op if the
> pad op does not exist. Otherwise we again have two non-interoperable
> classes of drivers for no good reason.

Agreed.

> 
> Thinking about it, I'm not all that certain changing all instances would be
> that much work in the end; it should be done anyway. Devices that have a
> single stream (i.e. everything right now) just don't care about the pad
> number.

I tried to cover this in the cover-letter, I believe the correct 
approach is probably to move s_stream() from video ops to pad ops in the 
long run. I did post a similar patch for this a while ago [1] but I fear 
it's outdated by now. Before I refreshed that particular patch I was 
interested on the feedback on this from this series as I don't want to 
send out a patch touching so many drivers without at least some 
discussion :-)

I would be find with both the helper and a full conversion approach. Or 
to do it in stages, add a helper now and slowly convert all drivers and 
then removing the helper. What would be your preferred way of dealing 
with this?

1.  http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2016-June/091250.html

> 
> >  	}
> >  
> >  	fmt.pad = pad->index;
> > @@ -1239,12 +1239,14 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
> >  	if (media_pipeline_start(vin->vdev.entity.pads, pipe))
> >  		return -EPIPE;
> >  
> > -	ret = v4l2_subdev_call(sd, video, s_stream, 1);
> > +	ret = v4l2_subdev_call(sd, pad, s_stream, pad->index, 0, 1);
> >  	if (ret == -ENOIOCTLCMD)
> >  		ret = 0;
> >  	if (ret)
> >  		media_pipeline_stop(vin->vdev.entity.pads);
> >  
> > +	vin_dbg(vin, "pad: %u stream: 0 enable: %d\n", pad->index, on);
> > +
> >  	return ret;
> >  }
> >  
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Sakari Ailus
> sakari.ailus@linux.intel.com

-- 
Regards,
Niklas Söderlund

[PATCH/RFC v2 04/15] rcar-csi2: switch to pad and stream aware s_stream

[Niklas Söderlund]

Switch the driver to implement the pad and stream aware s_stream
operation. This is needed to enable to support to start and stop
individual streams on a multiplexed pad.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index d8751add48fc1322..8ce0bfeef1113f9c 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -625,12 +625,17 @@ static int rcar_csi2_sd_info(struct rcar_csi2 *priv, struct v4l2_subdev **sd)
 	return 0;
 }
 
-static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable)
+static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
+			      unsigned int stream, int enable)
 {
 	struct rcar_csi2 *priv = sd_to_csi2(sd);
 	struct v4l2_subdev *nextsd;
 	int ret;
 
+	/* Only one stream on each source pad */
+	if (stream != 0)
+		return -EINVAL;
+
 	mutex_lock(&priv->lock);
 
 	ret = rcar_csi2_sd_info(priv, &nextsd);
@@ -699,17 +704,13 @@ static int rcar_csi2_get_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
-	.s_stream = rcar_csi2_s_stream,
-};
-
 static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
 	.set_fmt = rcar_csi2_set_pad_format,
 	.get_fmt = rcar_csi2_get_pad_format,
+	.s_stream = rcar_csi2_s_stream,
 };
 
 static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
-	.video	= &rcar_csi2_video_ops,
 	.pad	= &rcar_csi2_pad_ops,
 };
 
-- 
2.15.1

[PATCH/RFC v2 05/15] rcar-csi2: count usage for each source pad

[Niklas Söderlund]

The R-Car CSI-2 hardware can output the same virtual channel
simultaneously to more then one R-Car VIN. For this reason we need to
move the usage counting from the global device to each source pad.

If a source pads usage count go from 0 to 1 we need to signal that a new
stream should start, likewise if it go from 1 to 0 we need to stop a
stream. At the same time we only want to start or stop the R-Car
CSI-2 device only on the first or last stream.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 8ce0bfeef1113f9c..e0f56cc3d25179a9 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -328,6 +328,14 @@ enum rcar_csi2_pads {
 	NR_OF_RCAR_CSI2_PAD,
 };
 
+static int rcar_csi2_pad_to_vc(unsigned int pad)
+{
+	if (pad < RCAR_CSI2_SOURCE_VC0 || pad > RCAR_CSI2_SOURCE_VC3)
+		return -EINVAL;
+
+	return pad - RCAR_CSI2_SOURCE_VC0;
+}
+
 struct rcar_csi2_info {
 	const struct phypll_hsfreqrange *hsfreqrange;
 	const struct phtw_testdin_data *testdin_data;
@@ -350,7 +358,7 @@ struct rcar_csi2 {
 	struct v4l2_mbus_framefmt mf;
 
 	struct mutex lock;
-	int stream_count;
+	int stream_count[4];
 
 	unsigned short lanes;
 	unsigned char lane_swap[4];
@@ -630,7 +638,13 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
 {
 	struct rcar_csi2 *priv = sd_to_csi2(sd);
 	struct v4l2_subdev *nextsd;
-	int ret;
+	unsigned int i, count = 0;
+	int ret, vc;
+
+	/* Only allow stream control on source pads and valid vc */
+	vc = rcar_csi2_pad_to_vc(pad);
+	if (vc < 0)
+		return vc;
 
 	/* Only one stream on each source pad */
 	if (stream != 0)
@@ -642,7 +656,10 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
 	if (ret)
 		goto out;
 
-	if (enable && priv->stream_count == 0) {
+	for (i = 0; i < 4; i++)
+		count += priv->stream_count[i];
+
+	if (enable && count == 0) {
 		pm_runtime_get_sync(priv->dev);
 
 		ret = rcar_csi2_start(priv);
@@ -650,20 +667,23 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
 			pm_runtime_put(priv->dev);
 			goto out;
 		}
+	} else if (!enable && count == 1) {
+		rcar_csi2_stop(priv);
+		pm_runtime_put(priv->dev);
+	}
 
+	if (enable && priv->stream_count[vc] == 0) {
 		ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
 		if (ret) {
 			rcar_csi2_stop(priv);
 			pm_runtime_put(priv->dev);
 			goto out;
 		}
-	} else if (!enable && priv->stream_count == 1) {
-		rcar_csi2_stop(priv);
+	} else if (!enable && priv->stream_count[vc] == 1) {
 		ret = v4l2_subdev_call(nextsd, video, s_stream, 0);
-		pm_runtime_put(priv->dev);
 	}
 
-	priv->stream_count += enable ? 1 : -1;
+	priv->stream_count[vc] += enable ? 1 : -1;
 out:
 	mutex_unlock(&priv->lock);
 
@@ -919,7 +939,9 @@ static int rcar_csi2_probe(struct platform_device *pdev)
 	priv->dev = &pdev->dev;
 
 	mutex_init(&priv->lock);
-	priv->stream_count = 0;
+
+	for (i = 0; i < 4; i++)
+		priv->stream_count[i] = 0;
 
 	ret = rcar_csi2_probe_resources(priv, pdev);
 	if (ret) {
-- 
2.15.1

Re: [PATCH/RFC v2 05/15] rcar-csi2: count usage for each source pad

[Sakari Ailus]

On Thu, Dec 14, 2017 at 08:08:25PM +0100, Niklas Söderlund wrote:
> The R-Car CSI-2 hardware can output the same virtual channel
> simultaneously to more then one R-Car VIN. For this reason we need to
> move the usage counting from the global device to each source pad.
> 
> If a source pads usage count go from 0 to 1 we need to signal that a new
> stream should start, likewise if it go from 1 to 0 we need to stop a
> stream. At the same time we only want to start or stop the R-Car
> CSI-2 device only on the first or last stream.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 38 +++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 8ce0bfeef1113f9c..e0f56cc3d25179a9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -328,6 +328,14 @@ enum rcar_csi2_pads {
>  	NR_OF_RCAR_CSI2_PAD,
>  };
>  
> +static int rcar_csi2_pad_to_vc(unsigned int pad)
> +{
> +	if (pad < RCAR_CSI2_SOURCE_VC0 || pad > RCAR_CSI2_SOURCE_VC3)
> +		return -EINVAL;
> +
> +	return pad - RCAR_CSI2_SOURCE_VC0;
> +}
> +
>  struct rcar_csi2_info {
>  	const struct phypll_hsfreqrange *hsfreqrange;
>  	const struct phtw_testdin_data *testdin_data;
> @@ -350,7 +358,7 @@ struct rcar_csi2 {
>  	struct v4l2_mbus_framefmt mf;
>  
>  	struct mutex lock;
> -	int stream_count;
> +	int stream_count[4];

Why 4?

>  
>  	unsigned short lanes;
>  	unsigned char lane_swap[4];
> @@ -630,7 +638,13 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
>  {
>  	struct rcar_csi2 *priv = sd_to_csi2(sd);
>  	struct v4l2_subdev *nextsd;
> -	int ret;
> +	unsigned int i, count = 0;
> +	int ret, vc;
> +
> +	/* Only allow stream control on source pads and valid vc */
> +	vc = rcar_csi2_pad_to_vc(pad);
> +	if (vc < 0)
> +		return vc;
>  
>  	/* Only one stream on each source pad */
>  	if (stream != 0)
> @@ -642,7 +656,10 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
>  	if (ret)
>  		goto out;
>  
> -	if (enable && priv->stream_count == 0) {
> +	for (i = 0; i < 4; i++)
> +		count += priv->stream_count[i];
> +
> +	if (enable && count == 0) {
>  		pm_runtime_get_sync(priv->dev);
>  
>  		ret = rcar_csi2_start(priv);
> @@ -650,20 +667,23 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
>  			pm_runtime_put(priv->dev);
>  			goto out;
>  		}
> +	} else if (!enable && count == 1) {
> +		rcar_csi2_stop(priv);
> +		pm_runtime_put(priv->dev);
> +	}
>  
> +	if (enable && priv->stream_count[vc] == 0) {
>  		ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
>  		if (ret) {
>  			rcar_csi2_stop(priv);
>  			pm_runtime_put(priv->dev);
>  			goto out;
>  		}
> -	} else if (!enable && priv->stream_count == 1) {
> -		rcar_csi2_stop(priv);
> +	} else if (!enable && priv->stream_count[vc] == 1) {

Do I understand correctly that you can start and streams here one by one,
independently of each other?

Sometimes this might not be the case. I wonder if we should have a way to
tell that to the caller.

>  		ret = v4l2_subdev_call(nextsd, video, s_stream, 0);
> -		pm_runtime_put(priv->dev);
>  	}
>  
> -	priv->stream_count += enable ? 1 : -1;
> +	priv->stream_count[vc] += enable ? 1 : -1;
>  out:
>  	mutex_unlock(&priv->lock);
>  
> @@ -919,7 +939,9 @@ static int rcar_csi2_probe(struct platform_device *pdev)
>  	priv->dev = &pdev->dev;
>  
>  	mutex_init(&priv->lock);
> -	priv->stream_count = 0;
> +
> +	for (i = 0; i < 4; i++)
> +		priv->stream_count[i] = 0;
>  
>  	ret = rcar_csi2_probe_resources(priv, pdev);
>  	if (ret) {
> -- 
> 2.15.1
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH/RFC v2 05/15] rcar-csi2: count usage for each source pad

[Niklas Söderlund]

Hi Sakari,

Tack för dina kommentarer.

On 2017-12-15 14:25:27 +0200, Sakari Ailus wrote:
> On Thu, Dec 14, 2017 at 08:08:25PM +0100, Niklas Söderlund wrote:
> > The R-Car CSI-2 hardware can output the same virtual channel
> > simultaneously to more then one R-Car VIN. For this reason we need to
> > move the usage counting from the global device to each source pad.
> > 
> > If a source pads usage count go from 0 to 1 we need to signal that a new
> > stream should start, likewise if it go from 1 to 0 we need to stop a
> > stream. At the same time we only want to start or stop the R-Car
> > CSI-2 device only on the first or last stream.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 38 +++++++++++++++++++++++------
> >  1 file changed, 30 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 8ce0bfeef1113f9c..e0f56cc3d25179a9 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -328,6 +328,14 @@ enum rcar_csi2_pads {
> >  	NR_OF_RCAR_CSI2_PAD,
> >  };
> >  
> > +static int rcar_csi2_pad_to_vc(unsigned int pad)
> > +{
> > +	if (pad < RCAR_CSI2_SOURCE_VC0 || pad > RCAR_CSI2_SOURCE_VC3)
> > +		return -EINVAL;
> > +
> > +	return pad - RCAR_CSI2_SOURCE_VC0;
> > +}
> > +
> >  struct rcar_csi2_info {
> >  	const struct phypll_hsfreqrange *hsfreqrange;
> >  	const struct phtw_testdin_data *testdin_data;
> > @@ -350,7 +358,7 @@ struct rcar_csi2 {
> >  	struct v4l2_mbus_framefmt mf;
> >  
> >  	struct mutex lock;
> > -	int stream_count;
> > +	int stream_count[4];
> 
> Why 4?

There are 4 source pads connected to up to 8 different remote sink pads.

> 
> >  
> >  	unsigned short lanes;
> >  	unsigned char lane_swap[4];
> > @@ -630,7 +638,13 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> >  {
> >  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> >  	struct v4l2_subdev *nextsd;
> > -	int ret;
> > +	unsigned int i, count = 0;
> > +	int ret, vc;
> > +
> > +	/* Only allow stream control on source pads and valid vc */
> > +	vc = rcar_csi2_pad_to_vc(pad);
> > +	if (vc < 0)
> > +		return vc;
> >  
> >  	/* Only one stream on each source pad */
> >  	if (stream != 0)
> > @@ -642,7 +656,10 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (enable && priv->stream_count == 0) {
> > +	for (i = 0; i < 4; i++)
> > +		count += priv->stream_count[i];
> > +
> > +	if (enable && count == 0) {
> >  		pm_runtime_get_sync(priv->dev);
> >  
> >  		ret = rcar_csi2_start(priv);
> > @@ -650,20 +667,23 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> >  			pm_runtime_put(priv->dev);
> >  			goto out;
> >  		}
> > +	} else if (!enable && count == 1) {
> > +		rcar_csi2_stop(priv);
> > +		pm_runtime_put(priv->dev);
> > +	}
> >  
> > +	if (enable && priv->stream_count[vc] == 0) {
> >  		ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
> >  		if (ret) {
> >  			rcar_csi2_stop(priv);
> >  			pm_runtime_put(priv->dev);
> >  			goto out;
> >  		}
> > -	} else if (!enable && priv->stream_count == 1) {
> > -		rcar_csi2_stop(priv);
> > +	} else if (!enable && priv->stream_count[vc] == 1) {
> 
> Do I understand correctly that you can start and streams here one by one,
> independently of each other?

That is still an area we are figuring out. At this time I don't know if 
the hardware is capable of starting and stopping individual streams. We 
are working with our setup to try and get stuff up and running but we 
are having issues at our sensor side. For our experiments I wished to 
prepare to test this as I know I can route a single CSI-2 VC to more 
then one video device consumer and start and stop them independently.

Maybe it will be different once we manage to get more simultaneously VCs 
running.

> 
> Sometimes this might not be the case. I wonder if we should have a way to
> tell that to the caller.

You make a good point. How should this really be handeld? Maybe I'm to 
focused on my test setup which might not work as I think it dose. Would 
you say a better approach would be to drop the stream parameter to pad 
ops s_stream() and just start all streams of routes who are enabled? 

> 
> >  		ret = v4l2_subdev_call(nextsd, video, s_stream, 0);
> > -		pm_runtime_put(priv->dev);
> >  	}
> >  
> > -	priv->stream_count += enable ? 1 : -1;
> > +	priv->stream_count[vc] += enable ? 1 : -1;
> >  out:
> >  	mutex_unlock(&priv->lock);
> >  
> > @@ -919,7 +939,9 @@ static int rcar_csi2_probe(struct platform_device *pdev)
> >  	priv->dev = &pdev->dev;
> >  
> >  	mutex_init(&priv->lock);
> > -	priv->stream_count = 0;
> > +
> > +	for (i = 0; i < 4; i++)
> > +		priv->stream_count[i] = 0;
> >  
> >  	ret = rcar_csi2_probe_resources(priv, pdev);
> >  	if (ret) {
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Sakari Ailus
> sakari.ailus@linux.intel.com

-- 
Regards,
Niklas Söderlund

Re: [PATCH/RFC v2 05/15] rcar-csi2: count usage for each source pad

[Sakari Ailus]

Hejssan,

On Tue, Dec 19, 2017 at 12:38:51AM +0100, Niklas Söderlund wrote:
> Hi Sakari,
> 
> Tack för dina kommentarer.
> 
> On 2017-12-15 14:25:27 +0200, Sakari Ailus wrote:
> > On Thu, Dec 14, 2017 at 08:08:25PM +0100, Niklas Söderlund wrote:
> > > The R-Car CSI-2 hardware can output the same virtual channel
> > > simultaneously to more then one R-Car VIN. For this reason we need to
> > > move the usage counting from the global device to each source pad.
> > > 
> > > If a source pads usage count go from 0 to 1 we need to signal that a new
> > > stream should start, likewise if it go from 1 to 0 we need to stop a
> > > stream. At the same time we only want to start or stop the R-Car
> > > CSI-2 device only on the first or last stream.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 38 +++++++++++++++++++++++------
> > >  1 file changed, 30 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > index 8ce0bfeef1113f9c..e0f56cc3d25179a9 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -328,6 +328,14 @@ enum rcar_csi2_pads {
> > >  	NR_OF_RCAR_CSI2_PAD,
> > >  };
> > >  
> > > +static int rcar_csi2_pad_to_vc(unsigned int pad)
> > > +{
> > > +	if (pad < RCAR_CSI2_SOURCE_VC0 || pad > RCAR_CSI2_SOURCE_VC3)
> > > +		return -EINVAL;
> > > +
> > > +	return pad - RCAR_CSI2_SOURCE_VC0;
> > > +}
> > > +
> > >  struct rcar_csi2_info {
> > >  	const struct phypll_hsfreqrange *hsfreqrange;
> > >  	const struct phtw_testdin_data *testdin_data;
> > > @@ -350,7 +358,7 @@ struct rcar_csi2 {
> > >  	struct v4l2_mbus_framefmt mf;
> > >  
> > >  	struct mutex lock;
> > > -	int stream_count;
> > > +	int stream_count[4];
> > 
> > Why 4?
> 
> There are 4 source pads connected to up to 8 different remote sink pads.

Could you use a #define (macro) for these numbers?

> 
> > 
> > >  
> > >  	unsigned short lanes;
> > >  	unsigned char lane_swap[4];
> > > @@ -630,7 +638,13 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> > >  {
> > >  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > >  	struct v4l2_subdev *nextsd;
> > > -	int ret;
> > > +	unsigned int i, count = 0;
> > > +	int ret, vc;
> > > +
> > > +	/* Only allow stream control on source pads and valid vc */
> > > +	vc = rcar_csi2_pad_to_vc(pad);
> > > +	if (vc < 0)
> > > +		return vc;
> > >  
> > >  	/* Only one stream on each source pad */
> > >  	if (stream != 0)
> > > @@ -642,7 +656,10 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> > >  	if (ret)
> > >  		goto out;
> > >  
> > > -	if (enable && priv->stream_count == 0) {
> > > +	for (i = 0; i < 4; i++)
> > > +		count += priv->stream_count[i];
> > > +
> > > +	if (enable && count == 0) {
> > >  		pm_runtime_get_sync(priv->dev);
> > >  
> > >  		ret = rcar_csi2_start(priv);
> > > @@ -650,20 +667,23 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> > >  			pm_runtime_put(priv->dev);
> > >  			goto out;
> > >  		}
> > > +	} else if (!enable && count == 1) {
> > > +		rcar_csi2_stop(priv);
> > > +		pm_runtime_put(priv->dev);
> > > +	}
> > >  
> > > +	if (enable && priv->stream_count[vc] == 0) {
> > >  		ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
> > >  		if (ret) {
> > >  			rcar_csi2_stop(priv);
> > >  			pm_runtime_put(priv->dev);
> > >  			goto out;
> > >  		}
> > > -	} else if (!enable && priv->stream_count == 1) {
> > > -		rcar_csi2_stop(priv);
> > > +	} else if (!enable && priv->stream_count[vc] == 1) {
> > 
> > Do I understand correctly that you can start and streams here one by one,
> > independently of each other?
> 
> That is still an area we are figuring out. At this time I don't know if 
> the hardware is capable of starting and stopping individual streams. We 
> are working with our setup to try and get stuff up and running but we 
> are having issues at our sensor side. For our experiments I wished to 
> prepare to test this as I know I can route a single CSI-2 VC to more 
> then one video device consumer and start and stop them independently.
> 
> Maybe it will be different once we manage to get more simultaneously VCs 
> running.

I guess most hardware needs to be fully configured with all streams and
what not until it can be started. Could you figure this out from the
configured routes?

> 
> > 
> > Sometimes this might not be the case. I wonder if we should have a way to
> > tell that to the caller.
> 
> You make a good point. How should this really be handeld? Maybe I'm to 
> focused on my test setup which might not work as I think it dose. Would 
> you say a better approach would be to drop the stream parameter to pad 
> ops s_stream() and just start all streams of routes who are enabled? 

Good questions. V4L2 streams start one by one, but if this functionality is
moved to Media controller (with request API, as discussed in Prague
(meeting notes still pending)), there should be a way to tell to start
multiple streams at once.

V4L2 doesn't really have a concept of a stream and starting and stopping a
stream is dependent on streaming start and stop on the video node. That
works as long as there is a video node related to the stream. It'd be nice
to address that at the same time. In any case, that won't happen in the
near future.

For now, I guess we can just proceed with the current implementation
(s_stream pad op, with integer argument specifying the stream). Drivers
that need to start streaming until everything is configured, wait until
this is the case and then continue propagating the streaming state.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH/RFC v2 06/15] rcar-csi2: use frame description information when propagating .s_stream()

[Niklas Söderlund]

Use the frame description from the remote subdevice of the rcar-csi2's
sink pad to get the remote pad and stream pad needed to propagate the
.s_stream() operation.

The CSI-2 virtual channel which should be acted upon can be determined
by looking at which of the rcar-csi2 source pad the .s_stream() was
called on. This is because the rcar-csi2 acts as a demultiplexer for the
CSI-2 link on the one sink pad and outputs each virtual channel on a
distinct and known source pad.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index e0f56cc3d25179a9..6b607b2e31e26063 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -614,20 +614,31 @@ static void rcar_csi2_stop(struct rcar_csi2 *priv)
 	rcar_csi2_reset(priv);
 }
 
-static int rcar_csi2_sd_info(struct rcar_csi2 *priv, struct v4l2_subdev **sd)
+static int rcar_csi2_get_source_info(struct rcar_csi2 *priv,
+				     struct v4l2_subdev **subdev,
+				     unsigned int *pad,
+				     struct v4l2_mbus_frame_desc *fd)
 {
-	struct media_pad *pad;
+	struct media_pad *remote_pad;
 
-	pad = media_entity_remote_pad(&priv->pads[RCAR_CSI2_SINK]);
-	if (!pad) {
-		dev_err(priv->dev, "Could not find remote pad\n");
+	/* Get source subdevice and pad */
+	remote_pad = media_entity_remote_pad(&priv->pads[RCAR_CSI2_SINK]);
+	if (!remote_pad) {
+		dev_err(priv->dev, "Could not find remote source pad\n");
 		return -ENODEV;
 	}
+	*subdev = media_entity_to_v4l2_subdev(remote_pad->entity);
+	*pad = remote_pad->index;
 
-	*sd = media_entity_to_v4l2_subdev(pad->entity);
-	if (!*sd) {
-		dev_err(priv->dev, "Could not find remote subdevice\n");
-		return -ENODEV;
+	/* Get frame descriptor */
+	if (v4l2_subdev_call(*subdev, pad, get_frame_desc, *pad, fd)) {
+		dev_err(priv->dev, "Could not read frame desc\n");
+		return -EINVAL;
+	}
+
+	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
+		dev_err(priv->dev, "Frame desc do not describe CSI-2 link");
+		return -EINVAL;
 	}
 
 	return 0;
@@ -637,9 +648,10 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
 			      unsigned int stream, int enable)
 {
 	struct rcar_csi2 *priv = sd_to_csi2(sd);
+	struct v4l2_mbus_frame_desc fd;
 	struct v4l2_subdev *nextsd;
-	unsigned int i, count = 0;
-	int ret, vc;
+	unsigned int i, rpad, count = 0;
+	int ret, vc, rstream = -1;
 
 	/* Only allow stream control on source pads and valid vc */
 	vc = rcar_csi2_pad_to_vc(pad);
@@ -650,11 +662,23 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
 	if (stream != 0)
 		return -EINVAL;
 
-	mutex_lock(&priv->lock);
-
-	ret = rcar_csi2_sd_info(priv, &nextsd);
+	/* Get information about multiplexed link */
+	ret = rcar_csi2_get_source_info(priv, &nextsd, &rpad, &fd);
 	if (ret)
-		goto out;
+		return ret;
+
+	/* Get stream on multiplexed link */
+	for (i = 0; i < fd.num_entries; i++)
+		if (fd.entry[i].bus.csi2.channel == vc)
+			rstream = fd.entry[i].stream;
+
+	if (rstream < 0) {
+		dev_err(priv->dev, "Could not find stream for vc %u\n", vc);
+		return -EINVAL;
+	}
+
+	/* Start or stop the requested stream */
+	mutex_lock(&priv->lock);
 
 	for (i = 0; i < 4; i++)
 		count += priv->stream_count[i];
@@ -673,14 +697,14 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
 	}
 
 	if (enable && priv->stream_count[vc] == 0) {
-		ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
+		ret = v4l2_subdev_call(nextsd, pad, s_stream, rpad, rstream, 1);
 		if (ret) {
 			rcar_csi2_stop(priv);
 			pm_runtime_put(priv->dev);
 			goto out;
 		}
 	} else if (!enable && priv->stream_count[vc] == 1) {
-		ret = v4l2_subdev_call(nextsd, video, s_stream, 0);
+		ret = v4l2_subdev_call(nextsd, pad, s_stream, rpad, rstream, 0);
 	}
 
 	priv->stream_count[vc] += enable ? 1 : -1;
-- 
2.15.1

Re: [PATCH/RFC v2 06/15] rcar-csi2: use frame description information when propagating .s_stream()

[Sakari Ailus]

Hi Niklas,

On Thu, Dec 14, 2017 at 08:08:26PM +0100, Niklas Söderlund wrote:
> Use the frame description from the remote subdevice of the rcar-csi2's
> sink pad to get the remote pad and stream pad needed to propagate the
> .s_stream() operation.
> 
> The CSI-2 virtual channel which should be acted upon can be determined
> by looking at which of the rcar-csi2 source pad the .s_stream() was
> called on. This is because the rcar-csi2 acts as a demultiplexer for the
> CSI-2 link on the one sink pad and outputs each virtual channel on a
> distinct and known source pad.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 58 ++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index e0f56cc3d25179a9..6b607b2e31e26063 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -614,20 +614,31 @@ static void rcar_csi2_stop(struct rcar_csi2 *priv)
>  	rcar_csi2_reset(priv);
>  }
>  
> -static int rcar_csi2_sd_info(struct rcar_csi2 *priv, struct v4l2_subdev **sd)
> +static int rcar_csi2_get_source_info(struct rcar_csi2 *priv,
> +				     struct v4l2_subdev **subdev,

I wonder if using struct media_pad for this would be cleaner.

> +				     unsigned int *pad,
> +				     struct v4l2_mbus_frame_desc *fd)
>  {
> -	struct media_pad *pad;
> +	struct media_pad *remote_pad;
>  
> -	pad = media_entity_remote_pad(&priv->pads[RCAR_CSI2_SINK]);
> -	if (!pad) {
> -		dev_err(priv->dev, "Could not find remote pad\n");
> +	/* Get source subdevice and pad */
> +	remote_pad = media_entity_remote_pad(&priv->pads[RCAR_CSI2_SINK]);
> +	if (!remote_pad) {
> +		dev_err(priv->dev, "Could not find remote source pad\n");
>  		return -ENODEV;
>  	}
> +	*subdev = media_entity_to_v4l2_subdev(remote_pad->entity);
> +	*pad = remote_pad->index;
>  
> -	*sd = media_entity_to_v4l2_subdev(pad->entity);
> -	if (!*sd) {
> -		dev_err(priv->dev, "Could not find remote subdevice\n");
> -		return -ENODEV;
> +	/* Get frame descriptor */
> +	if (v4l2_subdev_call(*subdev, pad, get_frame_desc, *pad, fd)) {
> +		dev_err(priv->dev, "Could not read frame desc\n");
> +		return -EINVAL;
> +	}
> +
> +	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> +		dev_err(priv->dev, "Frame desc do not describe CSI-2 link");
> +		return -EINVAL;

I think this should also work with drivers that do not support frame
descriptors.

Alternatively support could be added for all drivers. In practice this
would mean having a few bus specific implementations of get_frame_desc op
that would dig the information from the frame format.

Perhaps the former option would make sense here, for now.

>  	}
>  
>  	return 0;
> @@ -637,9 +648,10 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
>  			      unsigned int stream, int enable)
>  {
>  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +	struct v4l2_mbus_frame_desc fd;
>  	struct v4l2_subdev *nextsd;
> -	unsigned int i, count = 0;
> -	int ret, vc;
> +	unsigned int i, rpad, count = 0;
> +	int ret, vc, rstream = -1;
>  
>  	/* Only allow stream control on source pads and valid vc */
>  	vc = rcar_csi2_pad_to_vc(pad);
> @@ -650,11 +662,23 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
>  	if (stream != 0)
>  		return -EINVAL;
>  
> -	mutex_lock(&priv->lock);
> -
> -	ret = rcar_csi2_sd_info(priv, &nextsd);
> +	/* Get information about multiplexed link */
> +	ret = rcar_csi2_get_source_info(priv, &nextsd, &rpad, &fd);
>  	if (ret)
> -		goto out;
> +		return ret;
> +
> +	/* Get stream on multiplexed link */
> +	for (i = 0; i < fd.num_entries; i++)
> +		if (fd.entry[i].bus.csi2.channel == vc)
> +			rstream = fd.entry[i].stream;

Virtual channel does not equate to the stream. You'll need the data type,
too.

You should actually obtain this from the configured routes instead.

How does this work btw. if you have several streams on the same virtual
channel that only have different data types?

> +
> +	if (rstream < 0) {
> +		dev_err(priv->dev, "Could not find stream for vc %u\n", vc);
> +		return -EINVAL;
> +	}
> +
> +	/* Start or stop the requested stream */
> +	mutex_lock(&priv->lock);
>  
>  	for (i = 0; i < 4; i++)
>  		count += priv->stream_count[i];
> @@ -673,14 +697,14 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
>  	}
>  
>  	if (enable && priv->stream_count[vc] == 0) {
> -		ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
> +		ret = v4l2_subdev_call(nextsd, pad, s_stream, rpad, rstream, 1);
>  		if (ret) {
>  			rcar_csi2_stop(priv);
>  			pm_runtime_put(priv->dev);
>  			goto out;
>  		}
>  	} else if (!enable && priv->stream_count[vc] == 1) {
> -		ret = v4l2_subdev_call(nextsd, video, s_stream, 0);
> +		ret = v4l2_subdev_call(nextsd, pad, s_stream, rpad, rstream, 0);
>  	}
>  
>  	priv->stream_count[vc] += enable ? 1 : -1;
> -- 
> 2.15.1
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH/RFC v2 06/15] rcar-csi2: use frame description information when propagating .s_stream()

[Niklas Söderlund]

Hi Sakari,

Thanks for your comments.

On 2017-12-15 15:19:36 +0200, Sakari Ailus wrote:
> Hi Niklas,
> 
> On Thu, Dec 14, 2017 at 08:08:26PM +0100, Niklas Söderlund wrote:
> > Use the frame description from the remote subdevice of the rcar-csi2's
> > sink pad to get the remote pad and stream pad needed to propagate the
> > .s_stream() operation.
> > 
> > The CSI-2 virtual channel which should be acted upon can be determined
> > by looking at which of the rcar-csi2 source pad the .s_stream() was
> > called on. This is because the rcar-csi2 acts as a demultiplexer for the
> > CSI-2 link on the one sink pad and outputs each virtual channel on a
> > distinct and known source pad.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 58 ++++++++++++++++++++---------
> >  1 file changed, 41 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index e0f56cc3d25179a9..6b607b2e31e26063 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -614,20 +614,31 @@ static void rcar_csi2_stop(struct rcar_csi2 *priv)
> >  	rcar_csi2_reset(priv);
> >  }
> >  
> > -static int rcar_csi2_sd_info(struct rcar_csi2 *priv, struct v4l2_subdev **sd)
> > +static int rcar_csi2_get_source_info(struct rcar_csi2 *priv,
> > +				     struct v4l2_subdev **subdev,
> 
> I wonder if using struct media_pad for this would be cleaner.

I can give it a try and see how it works out, thanks for the suggestion.

> 
> > +				     unsigned int *pad,
> > +				     struct v4l2_mbus_frame_desc *fd)
> >  {
> > -	struct media_pad *pad;
> > +	struct media_pad *remote_pad;
> >  
> > -	pad = media_entity_remote_pad(&priv->pads[RCAR_CSI2_SINK]);
> > -	if (!pad) {
> > -		dev_err(priv->dev, "Could not find remote pad\n");
> > +	/* Get source subdevice and pad */
> > +	remote_pad = media_entity_remote_pad(&priv->pads[RCAR_CSI2_SINK]);
> > +	if (!remote_pad) {
> > +		dev_err(priv->dev, "Could not find remote source pad\n");
> >  		return -ENODEV;
> >  	}
> > +	*subdev = media_entity_to_v4l2_subdev(remote_pad->entity);
> > +	*pad = remote_pad->index;
> >  
> > -	*sd = media_entity_to_v4l2_subdev(pad->entity);
> > -	if (!*sd) {
> > -		dev_err(priv->dev, "Could not find remote subdevice\n");
> > -		return -ENODEV;
> > +	/* Get frame descriptor */
> > +	if (v4l2_subdev_call(*subdev, pad, get_frame_desc, *pad, fd)) {
> > +		dev_err(priv->dev, "Could not read frame desc\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> > +		dev_err(priv->dev, "Frame desc do not describe CSI-2 link");
> > +		return -EINVAL;
> 
> I think this should also work with drivers that do not support frame
> descriptors.
> 
> Alternatively support could be added for all drivers. In practice this
> would mean having a few bus specific implementations of get_frame_desc op
> that would dig the information from the frame format.
> 
> Perhaps the former option would make sense here, for now.

I will try to give it some thought when I rework this series. At the 
moment I'm not sure what is the best idea :-)

> 
> >  	}
> >  
> >  	return 0;
> > @@ -637,9 +648,10 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> >  			      unsigned int stream, int enable)
> >  {
> >  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +	struct v4l2_mbus_frame_desc fd;
> >  	struct v4l2_subdev *nextsd;
> > -	unsigned int i, count = 0;
> > -	int ret, vc;
> > +	unsigned int i, rpad, count = 0;
> > +	int ret, vc, rstream = -1;
> >  
> >  	/* Only allow stream control on source pads and valid vc */
> >  	vc = rcar_csi2_pad_to_vc(pad);
> > @@ -650,11 +662,23 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> >  	if (stream != 0)
> >  		return -EINVAL;
> >  
> > -	mutex_lock(&priv->lock);
> > -
> > -	ret = rcar_csi2_sd_info(priv, &nextsd);
> > +	/* Get information about multiplexed link */
> > +	ret = rcar_csi2_get_source_info(priv, &nextsd, &rpad, &fd);
> >  	if (ret)
> > -		goto out;
> > +		return ret;
> > +
> > +	/* Get stream on multiplexed link */
> > +	for (i = 0; i < fd.num_entries; i++)
> > +		if (fd.entry[i].bus.csi2.channel == vc)
> > +			rstream = fd.entry[i].stream;
> 
> Virtual channel does not equate to the stream. You'll need the data type,
> too.
> 
> You should actually obtain this from the configured routes instead.

You are correct, will fix.

> 
> How does this work btw. if you have several streams on the same virtual
> channel that only have different data types?

I have not been able to test this but I think the hardware should be 
able to handle it. From other part of this driver:

tmp = VCDT_SEL_VC(entry->bus.csi2.channel) | VCDT_VCDTN_EN |
		    VCDT_SEL_DTN_ON |
		    VCDT_SEL_DT(entry->bus.csi2.data_type);

So it looks like selection is based on both virtual channel and data 
type. Unfortunately I'm not aware of hardware setup I can use to verify 
this for the R-Car CSI-2. Maybe I can create a data type mismatch in the 
driver and verify that no stream is outputted, but I would not say that 
would prove it would work with to streams with same VC but different 
data types.

> 
> > +
> > +	if (rstream < 0) {
> > +		dev_err(priv->dev, "Could not find stream for vc %u\n", vc);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Start or stop the requested stream */
> > +	mutex_lock(&priv->lock);
> >  
> >  	for (i = 0; i < 4; i++)
> >  		count += priv->stream_count[i];
> > @@ -673,14 +697,14 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> >  	}
> >  
> >  	if (enable && priv->stream_count[vc] == 0) {
> > -		ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
> > +		ret = v4l2_subdev_call(nextsd, pad, s_stream, rpad, rstream, 1);
> >  		if (ret) {
> >  			rcar_csi2_stop(priv);
> >  			pm_runtime_put(priv->dev);
> >  			goto out;
> >  		}
> >  	} else if (!enable && priv->stream_count[vc] == 1) {
> > -		ret = v4l2_subdev_call(nextsd, video, s_stream, 0);
> > +		ret = v4l2_subdev_call(nextsd, pad, s_stream, rpad, rstream, 0);
> >  	}
> >  
> >  	priv->stream_count[vc] += enable ? 1 : -1;
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Sakari Ailus
> sakari.ailus@linux.intel.com

-- 
Regards,
Niklas Söderlund

[PATCH/RFC v2 07/15] rcar-csi2: use frame description information to configure CSI-2 bus

[Niklas Söderlund]

The driver now have access to frame descriptor information, use it. Only
enable the virtual channels which are described in the frame descriptor
and calculate the link based on all enabled streams.

With multiplexed stream support it's now possible to have different
formats on the different source pads. Make source formats independent
off each other and disallowing a format on the multiplexed sink.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 6b607b2e31e26063..2dd7d03d622d5510 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -296,24 +296,22 @@ static const struct phtw_testdin_data testdin_data_v3m_e3[] = {
 #define CSI0CLKFREQRANGE(n)		((n & 0x3f) << 16)
 
 struct rcar_csi2_format {
-	unsigned int code;
 	unsigned int datatype;
 	unsigned int bpp;
 };
 
 static const struct rcar_csi2_format rcar_csi2_formats[] = {
-	{ .code = MEDIA_BUS_FMT_RGB888_1X24,	.datatype = 0x24, .bpp = 24 },
-	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
-	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .bpp = 16 },
-	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .bpp = 16 },
+	{ .datatype = 0x1e, .bpp = 16 },
+	{ .datatype = 0x24, .bpp = 24 },
 };
 
-static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int code)
+static const struct rcar_csi2_format
+*rcar_csi2_datatype_to_fmt(unsigned int datatype)
 {
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
-		if (rcar_csi2_formats[i].code == code)
+		if (rcar_csi2_formats[i].datatype == datatype)
 			return rcar_csi2_formats + i;
 
 	return NULL;
@@ -355,7 +353,7 @@ struct rcar_csi2 {
 	struct v4l2_async_notifier notifier;
 	struct v4l2_async_subdev remote;
 
-	struct v4l2_mbus_framefmt mf;
+	struct v4l2_mbus_framefmt mf[4];
 
 	struct mutex lock;
 	int stream_count[4];
@@ -411,25 +409,14 @@ static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
 	return -ETIMEDOUT;
 }
 
-static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
+static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv,
+			       struct v4l2_subdev *source,
+			       struct v4l2_mbus_frame_desc *fd)
 {
-	struct media_pad *pad, *source_pad;
-	struct v4l2_subdev *source = NULL;
 	struct v4l2_ctrl *ctrl;
+	unsigned int i, bpp = 0;
 	u64 mbps;
 
-	/* Get remote subdevice */
-	pad = &priv->subdev.entity.pads[RCAR_CSI2_SINK];
-	source_pad = media_entity_remote_pad(pad);
-	if (!source_pad) {
-		dev_err(priv->dev, "Could not find remote source pad\n");
-		return -ENODEV;
-	}
-	source = media_entity_to_v4l2_subdev(source_pad->entity);
-
-	dev_dbg(priv->dev, "Using source %s pad: %u\n", source->name,
-		source_pad->index);
-
 	/* Read the pixel rate control from remote */
 	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
 	if (!ctrl) {
@@ -438,6 +425,21 @@ static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
 		return -EINVAL;
 	}
 
+	/* Calculate total bpp */
+	for (i = 0; i < fd->num_entries; i++) {
+		const struct rcar_csi2_format *format;
+
+		format = rcar_csi2_datatype_to_fmt(
+					fd->entry[i].bus.csi2.data_type);
+		if (!format) {
+			dev_err(priv->dev, "Unknown data type: %d\n",
+				fd->entry[i].bus.csi2.data_type);
+			return -EINVAL;
+		}
+
+		bpp += format->bpp;
+	}
+
 	/* Calculate the phypll */
 	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
 	do_div(mbps, priv->lanes * 1000000);
@@ -489,39 +491,33 @@ static int rcar_csi2_set_phtw(struct rcar_csi2 *priv, unsigned int mbps)
 	return 0;
 }
 
-static int rcar_csi2_start(struct rcar_csi2 *priv)
+static int rcar_csi2_start(struct rcar_csi2 *priv, struct v4l2_subdev *source,
+			   struct v4l2_mbus_frame_desc *fd)
 {
-	const struct rcar_csi2_format *format;
-	u32 phycnt, tmp;
-	u32 vcdt = 0, vcdt2 = 0;
+	u32 phycnt, vcdt = 0, vcdt2 = 0;
 	unsigned int i;
 	int mbps, ret;
 
-	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
-		priv->mf.width, priv->mf.height,
-		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
-
-	/* Code is validated in set_ftm */
-	format = rcar_csi2_code_to_fmt(priv->mf.code);
+	for (i = 0; i < fd->num_entries; i++) {
+		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
+		u32 tmp;
 
-	/*
-	 * Enable all Virtual Channels
-	 *
-	 * NOTE: It's not possible to get individual datatype for each
-	 *       source virtual channel. Once this is possible in V4L2
-	 *       it should be used here.
-	 */
-	for (i = 0; i < 4; i++) {
-		tmp = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
-			VCDT_SEL_DT(format->datatype);
+		tmp = VCDT_SEL_VC(entry->bus.csi2.channel) | VCDT_VCDTN_EN |
+			VCDT_SEL_DTN_ON |
+			VCDT_SEL_DT(entry->bus.csi2.data_type);
 
 		/* Store in correct reg and offset */
-		if (i < 2)
-			vcdt |= tmp << ((i % 2) * 16);
+		if (entry->bus.csi2.channel < 2)
+			vcdt |= tmp << ((entry->bus.csi2.channel % 2) * 16);
 		else
-			vcdt2 |= tmp << ((i % 2) * 16);
+			vcdt2 |= tmp << ((entry->bus.csi2.channel % 2) * 16);
+
+		dev_dbg(priv->dev, "VC%d datatype: 0x%x\n",
+			entry->bus.csi2.channel, entry->bus.csi2.data_type);
 	}
 
+	dev_dbg(priv->dev, "VCDT: 0x%08x VCDT2: 0x%08x\n", vcdt, vcdt2);
+
 	switch (priv->lanes) {
 	case 1:
 		phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
@@ -537,7 +533,7 @@ static int rcar_csi2_start(struct rcar_csi2 *priv)
 		return -EINVAL;
 	}
 
-	mbps = rcar_csi2_calc_mbps(priv, format->bpp);
+	mbps = rcar_csi2_calc_mbps(priv, source, fd);
 	if (mbps < 0)
 		return mbps;
 
@@ -686,7 +682,7 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
 	if (enable && count == 0) {
 		pm_runtime_get_sync(priv->dev);
 
-		ret = rcar_csi2_start(priv);
+		ret = rcar_csi2_start(priv, nextsd, &fd);
 		if (ret) {
 			pm_runtime_put(priv->dev);
 			goto out;
@@ -720,14 +716,16 @@ static int rcar_csi2_set_pad_format(struct v4l2_subdev *sd,
 {
 	struct rcar_csi2 *priv = sd_to_csi2(sd);
 	struct v4l2_mbus_framefmt *framefmt;
+	int vc;
 
-	if (!rcar_csi2_code_to_fmt(format->format.code))
-		return -EINVAL;
+	vc = rcar_csi2_pad_to_vc(format->pad);
+	if (vc < 0)
+		return vc;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		priv->mf = format->format;
+		priv->mf[vc] = format->format;
 	} else {
-		framefmt = v4l2_subdev_get_try_format(sd, cfg, 0);
+		framefmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
 		*framefmt = format->format;
 	}
 
@@ -739,11 +737,17 @@ static int rcar_csi2_get_pad_format(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_format *format)
 {
 	struct rcar_csi2 *priv = sd_to_csi2(sd);
+	int vc;
+
+	vc = rcar_csi2_pad_to_vc(format->pad);
+	if (vc < 0)
+		return vc;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		format->format = priv->mf;
+		format->format = priv->mf[vc];
 	else
-		format->format = *v4l2_subdev_get_try_format(sd, cfg, 0);
+		format->format = *v4l2_subdev_get_try_format(sd, cfg,
+							     format->pad);
 
 	return 0;
 }
-- 
2.15.1

Re: [PATCH/RFC v2 07/15] rcar-csi2: use frame description information to configure CSI-2 bus

[jacopo mondi]

Hi Niklas,
   thanks for the patch!

On Thu, Dec 14, 2017 at 08:08:27PM +0100, Niklas Söderlund wrote:
> The driver now have access to frame descriptor information, use it. Only
> enable the virtual channels which are described in the frame descriptor
> and calculate the link based on all enabled streams.
>
> With multiplexed stream support it's now possible to have different
> formats on the different source pads. Make source formats independent
> off each other and disallowing a format on the multiplexed sink.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 112 ++++++++++++++--------------
>  1 file changed, 58 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 6b607b2e31e26063..2dd7d03d622d5510 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -296,24 +296,22 @@ static const struct phtw_testdin_data testdin_data_v3m_e3[] = {
>  #define CSI0CLKFREQRANGE(n)		((n & 0x3f) << 16)
>
>  struct rcar_csi2_format {
> -	unsigned int code;
>  	unsigned int datatype;
>  	unsigned int bpp;
>  };
>
>  static const struct rcar_csi2_format rcar_csi2_formats[] = {
> -	{ .code = MEDIA_BUS_FMT_RGB888_1X24,	.datatype = 0x24, .bpp = 24 },
> -	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
> -	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .bpp = 16 },
> -	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .bpp = 16 },
> +	{ .datatype = 0x1e, .bpp = 16 },
> +	{ .datatype = 0x24, .bpp = 24 },
>  };
>
> -static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int code)
> +static const struct rcar_csi2_format
> +*rcar_csi2_datatype_to_fmt(unsigned int datatype)
>  {
>  	unsigned int i;
>
>  	for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> -		if (rcar_csi2_formats[i].code == code)
> +		if (rcar_csi2_formats[i].datatype == datatype)
>  			return rcar_csi2_formats + i;
>
>  	return NULL;
> @@ -355,7 +353,7 @@ struct rcar_csi2 {
>  	struct v4l2_async_notifier notifier;
>  	struct v4l2_async_subdev remote;
>
> -	struct v4l2_mbus_framefmt mf;
> +	struct v4l2_mbus_framefmt mf[4];
>
>  	struct mutex lock;
>  	int stream_count[4];
> @@ -411,25 +409,14 @@ static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
>  	return -ETIMEDOUT;
>  }
>
> -static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> +static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv,
> +			       struct v4l2_subdev *source,
> +			       struct v4l2_mbus_frame_desc *fd)
>  {
> -	struct media_pad *pad, *source_pad;
> -	struct v4l2_subdev *source = NULL;
>  	struct v4l2_ctrl *ctrl;
> +	unsigned int i, bpp = 0;
>  	u64 mbps;
>
> -	/* Get remote subdevice */
> -	pad = &priv->subdev.entity.pads[RCAR_CSI2_SINK];
> -	source_pad = media_entity_remote_pad(pad);
> -	if (!source_pad) {
> -		dev_err(priv->dev, "Could not find remote source pad\n");
> -		return -ENODEV;
> -	}
> -	source = media_entity_to_v4l2_subdev(source_pad->entity);
> -
> -	dev_dbg(priv->dev, "Using source %s pad: %u\n", source->name,
> -		source_pad->index);
> -
>  	/* Read the pixel rate control from remote */
>  	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
>  	if (!ctrl) {
> @@ -438,6 +425,21 @@ static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>  		return -EINVAL;
>  	}
>
> +	/* Calculate total bpp */
> +	for (i = 0; i < fd->num_entries; i++) {
> +		const struct rcar_csi2_format *format;
> +
> +		format = rcar_csi2_datatype_to_fmt(
> +					fd->entry[i].bus.csi2.data_type);
> +		if (!format) {
> +			dev_err(priv->dev, "Unknown data type: %d\n",
> +				fd->entry[i].bus.csi2.data_type);
> +			return -EINVAL;
> +		}
> +
> +		bpp += format->bpp;
> +	}
> +
>  	/* Calculate the phypll */
>  	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
>  	do_div(mbps, priv->lanes * 1000000);
> @@ -489,39 +491,33 @@ static int rcar_csi2_set_phtw(struct rcar_csi2 *priv, unsigned int mbps)
>  	return 0;
>  }
>
> -static int rcar_csi2_start(struct rcar_csi2 *priv)
> +static int rcar_csi2_start(struct rcar_csi2 *priv, struct v4l2_subdev *source,
> +			   struct v4l2_mbus_frame_desc *fd)

I'm not sure I got this right, but with the new s_stream pad
operation, and with rcar-csi2 endpoints connected to differenct VIN
instances, is it possible for "rcar_csi2_s_stream()" to be called on
the same rcar-csi2 instance from different VIN instances?

In that case you are calling "rcar_csi2_start()" the first time only from
"rcar_csi2_s_stream()":

	for (i = 0; i < 4; i++)
		count += priv->stream_count[i];

	if (enable && count == 0) {
		pm_runtime_get_sync(priv->dev);

		ret = rcar_csi2_start(priv, nextsd, &fd);

and the consequentially VCDT register never gets updated to accommodate
new routes.

Thanks
   j

>  {
> -	const struct rcar_csi2_format *format;
> -	u32 phycnt, tmp;
> -	u32 vcdt = 0, vcdt2 = 0;
> +	u32 phycnt, vcdt = 0, vcdt2 = 0;
>  	unsigned int i;
>  	int mbps, ret;
>
> -	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> -		priv->mf.width, priv->mf.height,
> -		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> -
> -	/* Code is validated in set_ftm */
> -	format = rcar_csi2_code_to_fmt(priv->mf.code);
> +	for (i = 0; i < fd->num_entries; i++) {
> +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> +		u32 tmp;
>
> -	/*
> -	 * Enable all Virtual Channels
> -	 *
> -	 * NOTE: It's not possible to get individual datatype for each
> -	 *       source virtual channel. Once this is possible in V4L2
> -	 *       it should be used here.
> -	 */
> -	for (i = 0; i < 4; i++) {
> -		tmp = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> -			VCDT_SEL_DT(format->datatype);
> +		tmp = VCDT_SEL_VC(entry->bus.csi2.channel) | VCDT_VCDTN_EN |
> +			VCDT_SEL_DTN_ON |
> +			VCDT_SEL_DT(entry->bus.csi2.data_type);
>
>  		/* Store in correct reg and offset */
> -		if (i < 2)
> -			vcdt |= tmp << ((i % 2) * 16);
> +		if (entry->bus.csi2.channel < 2)
> +			vcdt |= tmp << ((entry->bus.csi2.channel % 2) * 16);
>  		else
> -			vcdt2 |= tmp << ((i % 2) * 16);
> +			vcdt2 |= tmp << ((entry->bus.csi2.channel % 2) * 16);
> +
> +		dev_dbg(priv->dev, "VC%d datatype: 0x%x\n",
> +			entry->bus.csi2.channel, entry->bus.csi2.data_type);
>  	}
>
> +	dev_dbg(priv->dev, "VCDT: 0x%08x VCDT2: 0x%08x\n", vcdt, vcdt2);
> +
>  	switch (priv->lanes) {
>  	case 1:
>  		phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> @@ -537,7 +533,7 @@ static int rcar_csi2_start(struct rcar_csi2 *priv)
>  		return -EINVAL;
>  	}
>
> -	mbps = rcar_csi2_calc_mbps(priv, format->bpp);
> +	mbps = rcar_csi2_calc_mbps(priv, source, fd);
>  	if (mbps < 0)
>  		return mbps;
>
> @@ -686,7 +682,7 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
>  	if (enable && count == 0) {
>  		pm_runtime_get_sync(priv->dev);
>
> -		ret = rcar_csi2_start(priv);
> +		ret = rcar_csi2_start(priv, nextsd, &fd);
>  		if (ret) {
>  			pm_runtime_put(priv->dev);
>  			goto out;
> @@ -720,14 +716,16 @@ static int rcar_csi2_set_pad_format(struct v4l2_subdev *sd,
>  {
>  	struct rcar_csi2 *priv = sd_to_csi2(sd);
>  	struct v4l2_mbus_framefmt *framefmt;
> +	int vc;
>
> -	if (!rcar_csi2_code_to_fmt(format->format.code))
> -		return -EINVAL;
> +	vc = rcar_csi2_pad_to_vc(format->pad);
> +	if (vc < 0)
> +		return vc;
>
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		priv->mf = format->format;
> +		priv->mf[vc] = format->format;
>  	} else {
> -		framefmt = v4l2_subdev_get_try_format(sd, cfg, 0);
> +		framefmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
>  		*framefmt = format->format;
>  	}
>
> @@ -739,11 +737,17 @@ static int rcar_csi2_get_pad_format(struct v4l2_subdev *sd,
>  				    struct v4l2_subdev_format *format)
>  {
>  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +	int vc;
> +
> +	vc = rcar_csi2_pad_to_vc(format->pad);
> +	if (vc < 0)
> +		return vc;
>
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		format->format = priv->mf;
> +		format->format = priv->mf[vc];
>  	else
> -		format->format = *v4l2_subdev_get_try_format(sd, cfg, 0);
> +		format->format = *v4l2_subdev_get_try_format(sd, cfg,
> +							     format->pad);
>
>  	return 0;
>  }
> --
> 2.15.1
>

Re: [PATCH/RFC v2 07/15] rcar-csi2: use frame description information to configure CSI-2 bus

[Niklas Söderlund]

Hi Jacopo,

Thanks for your comments.

On 2017-12-15 15:15:31 +0100, jacopo mondi wrote:
> Hi Niklas,
>    thanks for the patch!
> 
> On Thu, Dec 14, 2017 at 08:08:27PM +0100, Niklas Söderlund wrote:
> > The driver now have access to frame descriptor information, use it. Only
> > enable the virtual channels which are described in the frame descriptor
> > and calculate the link based on all enabled streams.
> >
> > With multiplexed stream support it's now possible to have different
> > formats on the different source pads. Make source formats independent
> > off each other and disallowing a format on the multiplexed sink.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 112 ++++++++++++++--------------
> >  1 file changed, 58 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 6b607b2e31e26063..2dd7d03d622d5510 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -296,24 +296,22 @@ static const struct phtw_testdin_data testdin_data_v3m_e3[] = {
> >  #define CSI0CLKFREQRANGE(n)		((n & 0x3f) << 16)
> >
> >  struct rcar_csi2_format {
> > -	unsigned int code;
> >  	unsigned int datatype;
> >  	unsigned int bpp;
> >  };
> >
> >  static const struct rcar_csi2_format rcar_csi2_formats[] = {
> > -	{ .code = MEDIA_BUS_FMT_RGB888_1X24,	.datatype = 0x24, .bpp = 24 },
> > -	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
> > -	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .bpp = 16 },
> > -	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .bpp = 16 },
> > +	{ .datatype = 0x1e, .bpp = 16 },
> > +	{ .datatype = 0x24, .bpp = 24 },
> >  };
> >
> > -static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int code)
> > +static const struct rcar_csi2_format
> > +*rcar_csi2_datatype_to_fmt(unsigned int datatype)
> >  {
> >  	unsigned int i;
> >
> >  	for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> > -		if (rcar_csi2_formats[i].code == code)
> > +		if (rcar_csi2_formats[i].datatype == datatype)
> >  			return rcar_csi2_formats + i;
> >
> >  	return NULL;
> > @@ -355,7 +353,7 @@ struct rcar_csi2 {
> >  	struct v4l2_async_notifier notifier;
> >  	struct v4l2_async_subdev remote;
> >
> > -	struct v4l2_mbus_framefmt mf;
> > +	struct v4l2_mbus_framefmt mf[4];
> >
> >  	struct mutex lock;
> >  	int stream_count[4];
> > @@ -411,25 +409,14 @@ static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> >  	return -ETIMEDOUT;
> >  }
> >
> > -static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> > +static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv,
> > +			       struct v4l2_subdev *source,
> > +			       struct v4l2_mbus_frame_desc *fd)
> >  {
> > -	struct media_pad *pad, *source_pad;
> > -	struct v4l2_subdev *source = NULL;
> >  	struct v4l2_ctrl *ctrl;
> > +	unsigned int i, bpp = 0;
> >  	u64 mbps;
> >
> > -	/* Get remote subdevice */
> > -	pad = &priv->subdev.entity.pads[RCAR_CSI2_SINK];
> > -	source_pad = media_entity_remote_pad(pad);
> > -	if (!source_pad) {
> > -		dev_err(priv->dev, "Could not find remote source pad\n");
> > -		return -ENODEV;
> > -	}
> > -	source = media_entity_to_v4l2_subdev(source_pad->entity);
> > -
> > -	dev_dbg(priv->dev, "Using source %s pad: %u\n", source->name,
> > -		source_pad->index);
> > -
> >  	/* Read the pixel rate control from remote */
> >  	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> >  	if (!ctrl) {
> > @@ -438,6 +425,21 @@ static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >  		return -EINVAL;
> >  	}
> >
> > +	/* Calculate total bpp */
> > +	for (i = 0; i < fd->num_entries; i++) {
> > +		const struct rcar_csi2_format *format;
> > +
> > +		format = rcar_csi2_datatype_to_fmt(
> > +					fd->entry[i].bus.csi2.data_type);
> > +		if (!format) {
> > +			dev_err(priv->dev, "Unknown data type: %d\n",
> > +				fd->entry[i].bus.csi2.data_type);
> > +			return -EINVAL;
> > +		}
> > +
> > +		bpp += format->bpp;
> > +	}
> > +
> >  	/* Calculate the phypll */
> >  	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> >  	do_div(mbps, priv->lanes * 1000000);
> > @@ -489,39 +491,33 @@ static int rcar_csi2_set_phtw(struct rcar_csi2 *priv, unsigned int mbps)
> >  	return 0;
> >  }
> >
> > -static int rcar_csi2_start(struct rcar_csi2 *priv)
> > +static int rcar_csi2_start(struct rcar_csi2 *priv, struct v4l2_subdev *source,
> > +			   struct v4l2_mbus_frame_desc *fd)
> 
> I'm not sure I got this right, but with the new s_stream pad
> operation, and with rcar-csi2 endpoints connected to differenct VIN
> instances, is it possible for "rcar_csi2_s_stream()" to be called on
> the same rcar-csi2 instance from different VIN instances?

Yes, this is true even without this series. You can configure the same 
CSI-2 and virtual channel to two different VIN instances and view the 
same stream on both of them. You can also ofc start and stop both VIN 
instances independently of each other.

> 
> In that case you are calling "rcar_csi2_start()" the first time only from
> "rcar_csi2_s_stream()":
> 
> 	for (i = 0; i < 4; i++)
> 		count += priv->stream_count[i];
> 
> 	if (enable && count == 0) {
> 		pm_runtime_get_sync(priv->dev);
> 
> 		ret = rcar_csi2_start(priv, nextsd, &fd);
> 
> and the consequentially VCDT register never gets updated to accommodate
> new routes.

Yes, this is true. And for the use-case so far this is OK. All streams 
needs to be configured before the first stream is started. This is why 
you get those nasty -EPIPE errors if they are not. No format, link or 
route change is allowed while a stream is on going so that VCDT is not 
updated on each new stream but only on the first one is intentional.

But you bring up a good point, route changes are not disallowed in this 
set whit an ongoing stream and that should be fixed :-) Media links are 
already disallowed but somehow this constraint was lost to me when 
routes where added, thanks.

> 
> Thanks
>    j
> 
> >  {
> > -	const struct rcar_csi2_format *format;
> > -	u32 phycnt, tmp;
> > -	u32 vcdt = 0, vcdt2 = 0;
> > +	u32 phycnt, vcdt = 0, vcdt2 = 0;
> >  	unsigned int i;
> >  	int mbps, ret;
> >
> > -	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > -		priv->mf.width, priv->mf.height,
> > -		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > -
> > -	/* Code is validated in set_ftm */
> > -	format = rcar_csi2_code_to_fmt(priv->mf.code);
> > +	for (i = 0; i < fd->num_entries; i++) {
> > +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> > +		u32 tmp;
> >
> > -	/*
> > -	 * Enable all Virtual Channels
> > -	 *
> > -	 * NOTE: It's not possible to get individual datatype for each
> > -	 *       source virtual channel. Once this is possible in V4L2
> > -	 *       it should be used here.
> > -	 */
> > -	for (i = 0; i < 4; i++) {
> > -		tmp = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> > -			VCDT_SEL_DT(format->datatype);
> > +		tmp = VCDT_SEL_VC(entry->bus.csi2.channel) | VCDT_VCDTN_EN |
> > +			VCDT_SEL_DTN_ON |
> > +			VCDT_SEL_DT(entry->bus.csi2.data_type);
> >
> >  		/* Store in correct reg and offset */
> > -		if (i < 2)
> > -			vcdt |= tmp << ((i % 2) * 16);
> > +		if (entry->bus.csi2.channel < 2)
> > +			vcdt |= tmp << ((entry->bus.csi2.channel % 2) * 16);
> >  		else
> > -			vcdt2 |= tmp << ((i % 2) * 16);
> > +			vcdt2 |= tmp << ((entry->bus.csi2.channel % 2) * 16);
> > +
> > +		dev_dbg(priv->dev, "VC%d datatype: 0x%x\n",
> > +			entry->bus.csi2.channel, entry->bus.csi2.data_type);
> >  	}
> >
> > +	dev_dbg(priv->dev, "VCDT: 0x%08x VCDT2: 0x%08x\n", vcdt, vcdt2);
> > +
> >  	switch (priv->lanes) {
> >  	case 1:
> >  		phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> > @@ -537,7 +533,7 @@ static int rcar_csi2_start(struct rcar_csi2 *priv)
> >  		return -EINVAL;
> >  	}
> >
> > -	mbps = rcar_csi2_calc_mbps(priv, format->bpp);
> > +	mbps = rcar_csi2_calc_mbps(priv, source, fd);
> >  	if (mbps < 0)
> >  		return mbps;
> >
> > @@ -686,7 +682,7 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> >  	if (enable && count == 0) {
> >  		pm_runtime_get_sync(priv->dev);
> >
> > -		ret = rcar_csi2_start(priv);
> > +		ret = rcar_csi2_start(priv, nextsd, &fd);
> >  		if (ret) {
> >  			pm_runtime_put(priv->dev);
> >  			goto out;
> > @@ -720,14 +716,16 @@ static int rcar_csi2_set_pad_format(struct v4l2_subdev *sd,
> >  {
> >  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> >  	struct v4l2_mbus_framefmt *framefmt;
> > +	int vc;
> >
> > -	if (!rcar_csi2_code_to_fmt(format->format.code))
> > -		return -EINVAL;
> > +	vc = rcar_csi2_pad_to_vc(format->pad);
> > +	if (vc < 0)
> > +		return vc;
> >
> >  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > -		priv->mf = format->format;
> > +		priv->mf[vc] = format->format;
> >  	} else {
> > -		framefmt = v4l2_subdev_get_try_format(sd, cfg, 0);
> > +		framefmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
> >  		*framefmt = format->format;
> >  	}
> >
> > @@ -739,11 +737,17 @@ static int rcar_csi2_get_pad_format(struct v4l2_subdev *sd,
> >  				    struct v4l2_subdev_format *format)
> >  {
> >  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +	int vc;
> > +
> > +	vc = rcar_csi2_pad_to_vc(format->pad);
> > +	if (vc < 0)
> > +		return vc;
> >
> >  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > -		format->format = priv->mf;
> > +		format->format = priv->mf[vc];
> >  	else
> > -		format->format = *v4l2_subdev_get_try_format(sd, cfg, 0);
> > +		format->format = *v4l2_subdev_get_try_format(sd, cfg,
> > +							     format->pad);
> >
> >  	return 0;
> >  }
> > --
> > 2.15.1
> >

-- 
Regards,
Niklas Söderlund

[PATCH/RFC v2 08/15] rcar-csi2: add get_routing support

[Niklas Söderlund]

To support multiplexed streams the internal routing between the
rcar-csi2 sink pad and its source pads needs to be described.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 2dd7d03d622d5510..fd1133e72482fc19 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -334,6 +334,14 @@ static int rcar_csi2_pad_to_vc(unsigned int pad)
 	return pad - RCAR_CSI2_SOURCE_VC0;
 }
 
+static int rcar_csi2_vc_to_pad(unsigned int vc)
+{
+	if (vc > 3)
+		return -EINVAL;
+
+	return vc + RCAR_CSI2_SOURCE_VC0;
+}
+
 struct rcar_csi2_info {
 	const struct phypll_hsfreqrange *hsfreqrange;
 	const struct phtw_testdin_data *testdin_data;
@@ -752,9 +760,55 @@ static int rcar_csi2_get_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int rcar_csi2_get_routing(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_routing *routing)
+{
+	struct rcar_csi2 *priv = sd_to_csi2(sd);
+	struct v4l2_mbus_frame_desc fd;
+	struct v4l2_subdev_route *r = routing->routes;
+	struct v4l2_subdev *rsubdev;
+	unsigned int i, rpad;
+	int ret;
+
+	/* Get information about multiplexed link */
+	ret = rcar_csi2_get_source_info(priv, &rsubdev, &rpad, &fd);
+	if (ret)
+		return ret;
+
+	if (routing->num_routes < fd.num_entries) {
+		routing->num_routes = fd.num_entries;
+		return -ENOSPC;
+	}
+
+	routing->num_routes = fd.num_entries;
+
+	for (i = 0; i < fd.num_entries; i++) {
+		struct v4l2_mbus_frame_desc_entry *entry = &fd.entry[i];
+		int source_pad;
+
+		source_pad = rcar_csi2_vc_to_pad(entry->bus.csi2.channel);
+		if (source_pad < 0) {
+			dev_err(priv->dev, "Virtual Channel out of range: %u\n",
+				entry->bus.csi2.channel);
+			return -ENOSPC;
+		}
+
+		r->sink_pad = RCAR_CSI2_SINK;
+		r->sink_stream = entry->stream;
+		r->source_pad = source_pad;
+		r->source_stream = 0;
+		r->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE |
+			V4L2_SUBDEV_ROUTE_FL_IMMUTABLE;
+		r++;
+	}
+
+	return 0;
+}
+
 static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
 	.set_fmt = rcar_csi2_set_pad_format,
 	.get_fmt = rcar_csi2_get_pad_format,
+	.get_routing = rcar_csi2_get_routing,
 	.s_stream = rcar_csi2_s_stream,
 };
 
-- 
2.15.1

[PATCH/RFC v2 09/15] adv748x: csi2: add module param for virtual channel

[Niklas Söderlund]

The hardware can output on any of the 4 (0-3) Virtual Channels of the
CSI-2 bus. Add a module parameter each for TXA and TXB to allow the user
to specify which channel should be used.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 10 ++++++++++
 drivers/media/i2c/adv748x/adv748x-csi2.c |  2 +-
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index fd92c9e4b519d2c5..3cad52532ead2e34 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -31,6 +31,9 @@
 
 #include "adv748x.h"
 
+static unsigned int txavc;
+static unsigned int txbvc;
+
 /* -----------------------------------------------------------------------------
  * Register manipulation
  */
@@ -747,6 +750,7 @@ static int adv748x_probe(struct i2c_client *client,
 	}
 
 	/* Initialise TXA */
+	state->txa.vc = txavc;
 	ret = adv748x_csi2_init(state, &state->txa);
 	if (ret) {
 		adv_err(state, "Failed to probe TXA");
@@ -754,6 +758,7 @@ static int adv748x_probe(struct i2c_client *client,
 	}
 
 	/* Initialise TXB */
+	state->txb.vc = txbvc;
 	ret = adv748x_csi2_init(state, &state->txb);
 	if (ret) {
 		adv_err(state, "Failed to probe TXB");
@@ -824,6 +829,11 @@ static struct i2c_driver adv748x_driver = {
 
 module_i2c_driver(adv748x_driver);
 
+module_param(txavc, uint, 0644);
+MODULE_PARM_DESC(txavc, "Virtual Channel for TXA");
+module_param(txbvc, uint, 0644);
+MODULE_PARM_DESC(txbvc, "Virtual Channel for TXB");
+
 MODULE_AUTHOR("Kieran Bingham <kieran.bingham@ideasonboard.com>");
 MODULE_DESCRIPTION("ADV748X video decoder");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 820b44ed56a8679f..2a5dff8c571013bf 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -281,7 +281,7 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
 	}
 
 	/* Initialise the virtual channel */
-	adv748x_csi2_set_virtual_channel(tx, 0);
+	adv748x_csi2_set_virtual_channel(tx, tx->vc);
 
 	adv748x_subdev_init(&tx->sd, state, &adv748x_csi2_ops,
 			    MEDIA_ENT_F_UNKNOWN,
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 6789e2f3bc8c2b49..f6e40ee3924e8f12 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -92,6 +92,7 @@ enum adv748x_csi2_pads {
 
 struct adv748x_csi2 {
 	struct adv748x_state *state;
+	unsigned int vc;
 	struct v4l2_mbus_framefmt format;
 	unsigned int page;
 
-- 
2.15.1

Re: [PATCH/RFC v2 09/15] adv748x: csi2: add module param for virtual channel

[Kieran Bingham]

Hi Niklas,

On 14/12/17 19:08, Niklas Söderlund wrote:
> The hardware can output on any of the 4 (0-3) Virtual Channels of the
> CSI-2 bus. Add a module parameter each for TXA and TXB to allow the user
> to specify which channel should be used.

This patch only configures the channel at initialisation time, (which is a valid
thing to do here at the moment I think) - but will we expect to provide
functionality to change the virtual channel later ?

Do we need to communicate the virtual channel in use across the media pad links
somehow? (or does that already happen?)

Perhaps the commit message could be clear on the fact that this only sets the
channels initialisation value - and that modifying the module parameter after
module load will have no effect?

Regards

Kieran


> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 10 ++++++++++
>  drivers/media/i2c/adv748x/adv748x-csi2.c |  2 +-
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index fd92c9e4b519d2c5..3cad52532ead2e34 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -31,6 +31,9 @@
>  
>  #include "adv748x.h"
>  
> +static unsigned int txavc;
> +static unsigned int txbvc;
> +
>  /* -----------------------------------------------------------------------------
>   * Register manipulation
>   */
> @@ -747,6 +750,7 @@ static int adv748x_probe(struct i2c_client *client,
>  	}
>  
>  	/* Initialise TXA */
> +	state->txa.vc = txavc;
>  	ret = adv748x_csi2_init(state, &state->txa);
>  	if (ret) {
>  		adv_err(state, "Failed to probe TXA");
> @@ -754,6 +758,7 @@ static int adv748x_probe(struct i2c_client *client,
>  	}
>  
>  	/* Initialise TXB */
> +	state->txb.vc = txbvc;
>  	ret = adv748x_csi2_init(state, &state->txb);
>  	if (ret) {
>  		adv_err(state, "Failed to probe TXB");
> @@ -824,6 +829,11 @@ static struct i2c_driver adv748x_driver = {
>  
>  module_i2c_driver(adv748x_driver);
>  
> +module_param(txavc, uint, 0644);
> +MODULE_PARM_DESC(txavc, "Virtual Channel for TXA");
> +module_param(txbvc, uint, 0644);
> +MODULE_PARM_DESC(txbvc, "Virtual Channel for TXB");
> +
>  MODULE_AUTHOR("Kieran Bingham <kieran.bingham@ideasonboard.com>");
>  MODULE_DESCRIPTION("ADV748X video decoder");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 820b44ed56a8679f..2a5dff8c571013bf 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -281,7 +281,7 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
>  	}
>  
>  	/* Initialise the virtual channel */
> -	adv748x_csi2_set_virtual_channel(tx, 0);
> +	adv748x_csi2_set_virtual_channel(tx, tx->vc);
>  
>  	adv748x_subdev_init(&tx->sd, state, &adv748x_csi2_ops,
>  			    MEDIA_ENT_F_UNKNOWN,
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 6789e2f3bc8c2b49..f6e40ee3924e8f12 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -92,6 +92,7 @@ enum adv748x_csi2_pads {
>  
>  struct adv748x_csi2 {
>  	struct adv748x_state *state;
> +	unsigned int vc;
>  	struct v4l2_mbus_framefmt format;
>  	unsigned int page;
>  
> 

Re: [PATCH/RFC v2 09/15] adv748x: csi2: add module param for virtual channel

[Niklas Söderlund]

Hi Kieran,

Thanks for your comments.

On 2017-12-14 22:19:59 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 14/12/17 19:08, Niklas Söderlund wrote:
> > The hardware can output on any of the 4 (0-3) Virtual Channels of the
> > CSI-2 bus. Add a module parameter each for TXA and TXB to allow the user
> > to specify which channel should be used.
> 
> This patch only configures the channel at initialisation time, (which is a valid
> thing to do here at the moment I think) - but will we expect to provide
> functionality to change the virtual channel later ?

I had no plan to add such functionality. But I would be open to 
suggesters on where to add such a control. I thought about this when 
working with this patch-set and I can think of three other places to 
control this.

1. A debugfs file
2. A configfs file
3. Define 4 streams in the frame descriptor for the source pad, one for 
   each CSI-2 VC. Then use the new G/S_ROUTE ioctls to control which 
   stream of the source the sink is connected to.

I thought option 1 and 2 was not the correct place for such a control.  
And option 3 would make the control of the VC output depending on this 
patch-set and all its dependencies. And since my use-case for this was 
patch is to test this patch-set it seemed silly at the time :-) But the 
more I think of this might be the best way forward for this type of 
things, what do you think?

> 
> Do we need to communicate the virtual channel in use across the media pad links
> somehow? (or does that already happen?)

Yes, this is part of this patch-set and its dependencies. The frame 
descriptor contains information about stream to CSI-2 virtual channel 
(and data type) mapping. While the G/S_ROUTE operations contains the 
controls and information on which stream of a multiplexed pad is routed 
to which 'normal' pad.

> 
> Perhaps the commit message could be clear on the fact that this only sets the
> channels initialisation value - and that modifying the module parameter after
> module load will have no effect?

Is this not true for all module parameters, they only have an effect at 
module load time? Can you even modify them after module load?

> 
> Regards
> 
> Kieran
> 
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 10 ++++++++++
> >  drivers/media/i2c/adv748x/adv748x-csi2.c |  2 +-
> >  drivers/media/i2c/adv748x/adv748x.h      |  1 +
> >  3 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index fd92c9e4b519d2c5..3cad52532ead2e34 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -31,6 +31,9 @@
> >  
> >  #include "adv748x.h"
> >  
> > +static unsigned int txavc;
> > +static unsigned int txbvc;
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Register manipulation
> >   */
> > @@ -747,6 +750,7 @@ static int adv748x_probe(struct i2c_client *client,
> >  	}
> >  
> >  	/* Initialise TXA */
> > +	state->txa.vc = txavc;
> >  	ret = adv748x_csi2_init(state, &state->txa);
> >  	if (ret) {
> >  		adv_err(state, "Failed to probe TXA");
> > @@ -754,6 +758,7 @@ static int adv748x_probe(struct i2c_client *client,
> >  	}
> >  
> >  	/* Initialise TXB */
> > +	state->txb.vc = txbvc;
> >  	ret = adv748x_csi2_init(state, &state->txb);
> >  	if (ret) {
> >  		adv_err(state, "Failed to probe TXB");
> > @@ -824,6 +829,11 @@ static struct i2c_driver adv748x_driver = {
> >  
> >  module_i2c_driver(adv748x_driver);
> >  
> > +module_param(txavc, uint, 0644);
> > +MODULE_PARM_DESC(txavc, "Virtual Channel for TXA");
> > +module_param(txbvc, uint, 0644);
> > +MODULE_PARM_DESC(txbvc, "Virtual Channel for TXB");
> > +
> >  MODULE_AUTHOR("Kieran Bingham <kieran.bingham@ideasonboard.com>");
> >  MODULE_DESCRIPTION("ADV748X video decoder");
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 820b44ed56a8679f..2a5dff8c571013bf 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -281,7 +281,7 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
> >  	}
> >  
> >  	/* Initialise the virtual channel */
> > -	adv748x_csi2_set_virtual_channel(tx, 0);
> > +	adv748x_csi2_set_virtual_channel(tx, tx->vc);
> >  
> >  	adv748x_subdev_init(&tx->sd, state, &adv748x_csi2_ops,
> >  			    MEDIA_ENT_F_UNKNOWN,
> > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> > index 6789e2f3bc8c2b49..f6e40ee3924e8f12 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -92,6 +92,7 @@ enum adv748x_csi2_pads {
> >  
> >  struct adv748x_csi2 {
> >  	struct adv748x_state *state;
> > +	unsigned int vc;
> >  	struct v4l2_mbus_framefmt format;
> >  	unsigned int page;
> >  
> > 

-- 
Regards,
Niklas Söderlund

[PATCH/RFC v2 10/15] adv748x: csi2: add translation from pixelcode to CSI-2 datatype

[Niklas Söderlund]

This will be needed to fill out the frame descriptor information
correctly.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 2a5dff8c571013bf..a2a6d93077204731 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -18,6 +18,28 @@
 
 #include "adv748x.h"
 
+struct adv748x_csi2_format {
+	unsigned int code;
+	unsigned int datatype;
+};
+
+static const struct adv748x_csi2_format adv748x_csi2_formats[] = {
+	{ .code = MEDIA_BUS_FMT_RGB888_1X24,    .datatype = 0x24, },
+	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,     .datatype = 0x1e, },
+	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,      .datatype = 0x1e, },
+	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,    .datatype = 0x1e, },
+};
+
+static unsigned int adv748x_csi2_code_to_datatype(unsigned int code)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(adv748x_csi2_formats); i++)
+		if (adv748x_csi2_formats[i].code == code)
+			return adv748x_csi2_formats[i].datatype;
+	return 0;
+}
+
 static bool is_txa(struct adv748x_csi2 *tx)
 {
 	return tx == &tx->state->txa;
-- 
2.15.1

Re: [PATCH/RFC v2 10/15] adv748x: csi2: add translation from pixelcode to CSI-2 datatype

[Kieran Bingham]

Hi Niklas,

On 14/12/17 19:08, Niklas Söderlund wrote:
> This will be needed to fill out the frame descriptor information
> correctly.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 2a5dff8c571013bf..a2a6d93077204731 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -18,6 +18,28 @@
>  
>  #include "adv748x.h"
>  
> +struct adv748x_csi2_format {
> +	unsigned int code;
> +	unsigned int datatype;
> +};
> +
> +static const struct adv748x_csi2_format adv748x_csi2_formats[] = {
> +	{ .code = MEDIA_BUS_FMT_RGB888_1X24,    .datatype = 0x24, },
> +	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,     .datatype = 0x1e, },
> +	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,      .datatype = 0x1e, },
> +	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,    .datatype = 0x1e, },
> +};

Is the datatype mapping specific to the ADV748x here?
or are these generic/common CSI2 mappings?

What do those datatype magic numbers represent?

--
Kieran

> +
> +static unsigned int adv748x_csi2_code_to_datatype(unsigned int code)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(adv748x_csi2_formats); i++)
> +		if (adv748x_csi2_formats[i].code == code)
> +			return adv748x_csi2_formats[i].datatype;
> +	return 0;
> +}
> +
>  static bool is_txa(struct adv748x_csi2 *tx)
>  {
>  	return tx == &tx->state->txa;
> 

Re: [PATCH/RFC v2 10/15] adv748x: csi2: add translation from pixelcode to CSI-2 datatype

[jacopo mondi]

Hi Kieran,

On Thu, Dec 14, 2017 at 10:25:36PM +0000, Kieran Bingham wrote:
> Hi Niklas,
>
> On 14/12/17 19:08, Niklas Söderlund wrote:
> > This will be needed to fill out the frame descriptor information
> > correctly.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 2a5dff8c571013bf..a2a6d93077204731 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -18,6 +18,28 @@
> >
> >  #include "adv748x.h"
> >
> > +struct adv748x_csi2_format {
> > +	unsigned int code;
> > +	unsigned int datatype;
> > +};
> > +
> > +static const struct adv748x_csi2_format adv748x_csi2_formats[] = {
> > +	{ .code = MEDIA_BUS_FMT_RGB888_1X24,    .datatype = 0x24, },
> > +	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,     .datatype = 0x1e, },
> > +	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,      .datatype = 0x1e, },
> > +	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,    .datatype = 0x1e, },

YUV 422 10 bit is associated to data type 0x1d in CSI-2 specifications

> > +};
>
> Is the datatype mapping specific to the ADV748x here?
> or are these generic/common CSI2 mappings?
>
> What do those datatype magic numbers represent?

They are fixed mappings defined by CSI-2 specifications and they
should probably be generic to all drivers imho

>
> --
> Kieran
>
> > +
> > +static unsigned int adv748x_csi2_code_to_datatype(unsigned int code)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(adv748x_csi2_formats); i++)
> > +		if (adv748x_csi2_formats[i].code == code)
> > +			return adv748x_csi2_formats[i].datatype;
> > +	return 0;
> > +}
> > +
> >  static bool is_txa(struct adv748x_csi2 *tx)
> >  {
> >  	return tx == &tx->state->txa;
> >

[PATCH/RFC v2 11/15] adv748x: csi2: implement get_frame_desc

[Niklas Söderlund]

Provide CSI-2 bus information for the multiplexed source pad using the
frame descriptor.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index a2a6d93077204731..a43b251d0bc67a43 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -225,9 +225,37 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int adv748x_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				       struct v4l2_mbus_frame_desc *fd)
+{
+	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
+	struct v4l2_mbus_framefmt *mbusformat;
+
+	memset(fd, 0, sizeof(*fd));
+
+	if (pad != ADV748X_CSI2_SOURCE)
+		return -EINVAL;
+
+	mbusformat = adv748x_csi2_get_pad_format(sd, NULL, ADV748X_CSI2_SINK,
+						 V4L2_SUBDEV_FORMAT_ACTIVE);
+	if (!mbusformat)
+		return -EINVAL;
+
+	fd->entry->stream = 0;
+	fd->entry->bus.csi2.channel = tx->vc;
+	fd->entry->bus.csi2.data_type =
+		adv748x_csi2_code_to_datatype(mbusformat->code);
+
+	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+	fd->num_entries = 1;
+
+	return 0;
+}
+
 static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
 	.get_fmt = adv748x_csi2_get_format,
 	.set_fmt = adv748x_csi2_set_format,
+	.get_frame_desc = adv748x_csi2_get_frame_desc,
 };
 
 /* -----------------------------------------------------------------------------
-- 
2.15.1

[PATCH/RFC v2 12/15] adv748x: csi2: switch to pad and stream aware s_stream

[Niklas Söderlund]

Switch the driver to implement the pad and stream aware s_stream
operation. This is needed to enable to support to start and stop
individual streams on a multiplexed pad.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index a43b251d0bc67a43..39f993282dd3bb5c 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -128,22 +128,26 @@ static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
  * v4l2_subdev_video_ops
  */
 
-static int adv748x_csi2_s_stream(struct v4l2_subdev *sd, int enable)
+static int adv748x_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
+				 unsigned int stream, int enable)
 {
 	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
+	struct adv748x_state *state = tx->state;
 	struct v4l2_subdev *src;
 
+	if (pad != ADV748X_CSI2_SOURCE || stream != 0)
+		return -EINVAL;
+
 	src = adv748x_get_remote_sd(&tx->pads[ADV748X_CSI2_SINK]);
 	if (!src)
 		return -EPIPE;
 
+	adv_dbg(state, "%s: pad: %u stream: %u enable: %d\n", sd->name,
+		pad, stream, enable);
+
 	return v4l2_subdev_call(src, video, s_stream, enable);
 }
 
-static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = {
-	.s_stream = adv748x_csi2_s_stream,
-};
-
 /* -----------------------------------------------------------------------------
  * v4l2_subdev_pad_ops
  *
@@ -256,6 +260,7 @@ static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
 	.get_fmt = adv748x_csi2_get_format,
 	.set_fmt = adv748x_csi2_set_format,
 	.get_frame_desc = adv748x_csi2_get_frame_desc,
+	.s_stream = adv748x_csi2_s_stream,
 };
 
 /* -----------------------------------------------------------------------------
@@ -263,7 +268,6 @@ static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
  */
 
 static const struct v4l2_subdev_ops adv748x_csi2_ops = {
-	.video = &adv748x_csi2_video_ops,
 	.pad = &adv748x_csi2_pad_ops,
 };
 
-- 
2.15.1

Re: [PATCH/RFC v2 12/15] adv748x: csi2: switch to pad and stream aware s_stream

[Kieran Bingham]

Hi Niklas,

On 14/12/17 19:08, Niklas Söderlund wrote:
> Switch the driver to implement the pad and stream aware s_stream
> operation. This is needed to enable to support to start and stop
> individual streams on a multiplexed pad

"This is needed to enable support for starting and stopping individual streams
on a multiplexed pad."

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

Otherwise,

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index a43b251d0bc67a43..39f993282dd3bb5c 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -128,22 +128,26 @@ static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
>   * v4l2_subdev_video_ops
>   */
>  
> -static int adv748x_csi2_s_stream(struct v4l2_subdev *sd, int enable)
> +static int adv748x_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> +				 unsigned int stream, int enable)
>  {
>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> +	struct adv748x_state *state = tx->state;
>  	struct v4l2_subdev *src;
>  
> +	if (pad != ADV748X_CSI2_SOURCE || stream != 0)
> +		return -EINVAL;
> +
>  	src = adv748x_get_remote_sd(&tx->pads[ADV748X_CSI2_SINK]);
>  	if (!src)
>  		return -EPIPE;
>  
> +	adv_dbg(state, "%s: pad: %u stream: %u enable: %d\n", sd->name,
> +		pad, stream, enable);
> +
>  	return v4l2_subdev_call(src, video, s_stream, enable);
>  }
>  
> -static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = {
> -	.s_stream = adv748x_csi2_s_stream,
> -};
> -
>  /* -----------------------------------------------------------------------------
>   * v4l2_subdev_pad_ops
>   *
> @@ -256,6 +260,7 @@ static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
>  	.get_fmt = adv748x_csi2_get_format,
>  	.set_fmt = adv748x_csi2_set_format,
>  	.get_frame_desc = adv748x_csi2_get_frame_desc,
> +	.s_stream = adv748x_csi2_s_stream,
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -263,7 +268,6 @@ static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
>   */
>  
>  static const struct v4l2_subdev_ops adv748x_csi2_ops = {
> -	.video = &adv748x_csi2_video_ops,
>  	.pad = &adv748x_csi2_pad_ops,
>  };
>  
> 

[PATCH/RFC v2 13/15] adv748x: csi2: only allow formats on sink pads

[Niklas Söderlund]

The driver is now pad and stream aware, only allow to get/set format on
sink pads. Also record a different format for each sink pad since it's
no longer true that they are all the same

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 39f993282dd3bb5c..291b35bef49d41fb 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -176,6 +176,9 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
 	struct adv748x_state *state = tx->state;
 	struct v4l2_mbus_framefmt *mbusformat;
 
+	if (sdformat->pad != ADV748X_CSI2_SINK)
+		return -EINVAL;
+
 	mbusformat = adv748x_csi2_get_pad_format(sd, cfg, sdformat->pad,
 						 sdformat->which);
 	if (!mbusformat)
@@ -199,6 +202,9 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *mbusformat;
 	int ret = 0;
 
+	if (sdformat->pad != ADV748X_CSI2_SINK)
+		return -EINVAL;
+
 	mbusformat = adv748x_csi2_get_pad_format(sd, cfg, sdformat->pad,
 						 sdformat->which);
 	if (!mbusformat)
-- 
2.15.1

Re: [PATCH/RFC v2 13/15] adv748x: csi2: only allow formats on sink pads

[Kieran Bingham]

Hi Niklas,

On 14/12/17 19:08, Niklas Söderlund wrote:
> The driver is now pad and stream aware, only allow to get/set format on
> sink pads.

Ok - I can see the patch is doing this ...

> Also record a different format for each sink pad since it's
> no longer true that they are all the same

But I can't see how the patch is doing this ^ ?

What have I missed?

--
Kieran

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 39f993282dd3bb5c..291b35bef49d41fb 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -176,6 +176,9 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
>  	struct adv748x_state *state = tx->state;
>  	struct v4l2_mbus_framefmt *mbusformat;
>  
> +	if (sdformat->pad != ADV748X_CSI2_SINK)
> +		return -EINVAL;
> +
>  	mbusformat = adv748x_csi2_get_pad_format(sd, cfg, sdformat->pad,
>  						 sdformat->which);
>  	if (!mbusformat)
> @@ -199,6 +202,9 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  	struct v4l2_mbus_framefmt *mbusformat;
>  	int ret = 0;
>  
> +	if (sdformat->pad != ADV748X_CSI2_SINK)
> +		return -EINVAL;
> +
>  	mbusformat = adv748x_csi2_get_pad_format(sd, cfg, sdformat->pad,
>  						 sdformat->which);
>  	if (!mbusformat)
> 

Re: [PATCH/RFC v2 13/15] adv748x: csi2: only allow formats on sink pads

[Niklas Söderlund]

HI Kieran,

Thanks for your comments.

On 2017-12-14 23:16:08 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 14/12/17 19:08, Niklas Söderlund wrote:
> > The driver is now pad and stream aware, only allow to get/set format on
> > sink pads.
> 
> Ok - I can see the patch is doing this ...
> 
> > Also record a different format for each sink pad since it's
> > no longer true that they are all the same
> 
> But I can't see how the patch is doing this ^ ?
> 
> What have I missed?

I have missed moving this commit message to another patch when moving 
code around :-) Thanks for noticing. Will fix this for next version.

> 
> --
> Kieran
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 39f993282dd3bb5c..291b35bef49d41fb 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -176,6 +176,9 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
> >  	struct adv748x_state *state = tx->state;
> >  	struct v4l2_mbus_framefmt *mbusformat;
> >  
> > +	if (sdformat->pad != ADV748X_CSI2_SINK)
> > +		return -EINVAL;
> > +
> >  	mbusformat = adv748x_csi2_get_pad_format(sd, cfg, sdformat->pad,
> >  						 sdformat->which);
> >  	if (!mbusformat)
> > @@ -199,6 +202,9 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> >  	struct v4l2_mbus_framefmt *mbusformat;
> >  	int ret = 0;
> >  
> > +	if (sdformat->pad != ADV748X_CSI2_SINK)
> > +		return -EINVAL;
> > +
> >  	mbusformat = adv748x_csi2_get_pad_format(sd, cfg, sdformat->pad,
> >  						 sdformat->which);
> >  	if (!mbusformat)
> > 

-- 
Regards,
Niklas Söderlund

[PATCH/RFC v2 14/15] adv748x: csi2: add get_routing support

[Niklas Söderlund]

To support multiplexed streams the internal routing between the
adv748x sink pad and its source pad needs to be described.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 291b35bef49d41fb..dbefb53f5b8c414d 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -262,10 +262,32 @@ static int adv748x_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 	return 0;
 }
 
+static int adv748x_csi2_get_routing(struct v4l2_subdev *subdev,
+				    struct v4l2_subdev_routing *routing)
+{
+	struct v4l2_subdev_route *r = routing->routes;
+
+	if (routing->num_routes < 1) {
+		routing->num_routes = 1;
+		return -ENOSPC;
+	}
+
+	routing->num_routes = 1;
+
+	r->sink_pad = ADV748X_CSI2_SINK;
+	r->sink_stream = 0;
+	r->source_pad = ADV748X_CSI2_SOURCE;
+	r->source_stream = 0;
+	r->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_IMMUTABLE;
+
+	return 0;
+}
+
 static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
 	.get_fmt = adv748x_csi2_get_format,
 	.set_fmt = adv748x_csi2_set_format,
 	.get_frame_desc = adv748x_csi2_get_frame_desc,
+	.get_routing = adv748x_csi2_get_routing,
 	.s_stream = adv748x_csi2_s_stream,
 };
 
-- 
2.15.1

Re: [PATCH/RFC v2 14/15] adv748x: csi2: add get_routing support

[Kieran Bingham]

Hi Niklas,

On 14/12/17 19:08, Niklas Söderlund wrote:
> To support multiplexed streams the internal routing between the
> adv748x sink pad and its source pad needs to be described.

The adv748x has quite a few sink and source pads... I presume here you mean the
adv748x csi2 sink and source pad :D

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 291b35bef49d41fb..dbefb53f5b8c414d 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -262,10 +262,32 @@ static int adv748x_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>  	return 0;
>  }
>  
> +static int adv748x_csi2_get_routing(struct v4l2_subdev *subdev,
> +				    struct v4l2_subdev_routing *routing)
> +{
> +	struct v4l2_subdev_route *r = routing->routes;
> +
> +	if (routing->num_routes < 1) {
> +		routing->num_routes = 1;
> +		return -ENOSPC;
> +	}
> +
> +	routing->num_routes = 1;
> +
> +	r->sink_pad = ADV748X_CSI2_SINK;
> +	r->sink_stream = 0;
> +	r->source_pad = ADV748X_CSI2_SOURCE;
> +	r->source_stream = 0;
> +	r->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_IMMUTABLE;
> +
> +	return 0;
> +}
> +

So - I think this is fine - but it seems a lot of code to define a static
default route which describes a single link between it's sink pad - and its
source pad ...

I suspect this should/could be wrapped by some helpers in core for cases like
this, as it's the simple case - but as we don't currently have that I guess we
have to put this in here for now ?

Maybe we should have a helper to make this

return v4l2_subdev_single_route(subdev, routing,
                                ADV748X_CS2_SINK, 0,
                                ADV748X_CSI2_SOURCE, 0,
                  V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_IMMUTABLE);

Or maybe even define these static routes in a struct somehow?

>  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
>  	.get_fmt = adv748x_csi2_get_format,
>  	.set_fmt = adv748x_csi2_set_format,
>  	.get_frame_desc = adv748x_csi2_get_frame_desc,
> +	.get_routing = adv748x_csi2_get_routing,
>  	.s_stream = adv748x_csi2_s_stream,
>  };
>  
> 

Re: [PATCH/RFC v2 14/15] adv748x: csi2: add get_routing support

[Niklas Söderlund]

Hi Kieran,

Thanks for your comments,

On 2017-12-14 23:27:57 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 14/12/17 19:08, Niklas Söderlund wrote:
> > To support multiplexed streams the internal routing between the
> > adv748x sink pad and its source pad needs to be described.
> 
> The adv748x has quite a few sink and source pads... I presume here you mean the
> adv748x csi2 sink and source pad :D

Yes :-) Will fix for next version, thanks.

> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 291b35bef49d41fb..dbefb53f5b8c414d 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -262,10 +262,32 @@ static int adv748x_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> >  	return 0;
> >  }
> >  
> > +static int adv748x_csi2_get_routing(struct v4l2_subdev *subdev,
> > +				    struct v4l2_subdev_routing *routing)
> > +{
> > +	struct v4l2_subdev_route *r = routing->routes;
> > +
> > +	if (routing->num_routes < 1) {
> > +		routing->num_routes = 1;
> > +		return -ENOSPC;
> > +	}
> > +
> > +	routing->num_routes = 1;
> > +
> > +	r->sink_pad = ADV748X_CSI2_SINK;
> > +	r->sink_stream = 0;
> > +	r->source_pad = ADV748X_CSI2_SOURCE;
> > +	r->source_stream = 0;
> > +	r->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_IMMUTABLE;
> > +
> > +	return 0;
> > +}
> > +
> 
> So - I think this is fine - but it seems a lot of code to define a static
> default route which describes a single link between it's sink pad - and its
> source pad ...
> 
> I suspect this should/could be wrapped by some helpers in core for cases like
> this, as it's the simple case - but as we don't currently have that I guess we
> have to put this in here for now ?

Yes for now we need to fill in the information here.

> 
> Maybe we should have a helper to make this

I'm sure there could be v4l2 helpers for such a case. I don't even think 
you wound need to prime it with any information. If there is only one 
source and one sink I'm sure a helper function can figure it out :-)

> 
> return v4l2_subdev_single_route(subdev, routing,
>                                 ADV748X_CS2_SINK, 0,
>                                 ADV748X_CSI2_SOURCE, 0,
>                   V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_IMMUTABLE);
> 
> Or maybe even define these static routes in a struct somehow?

For more complex setups a struct could be used together with a helper 
function to decode it. But then again maybe it's easier to just define a 
const v4l2_subdev_route array and 'routing->routes = my_const_routes' ?

> 
> >  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
> >  	.get_fmt = adv748x_csi2_get_format,
> >  	.set_fmt = adv748x_csi2_set_format,
> >  	.get_frame_desc = adv748x_csi2_get_frame_desc,
> > +	.get_routing = adv748x_csi2_get_routing,
> >  	.s_stream = adv748x_csi2_s_stream,
> >  };
> >  
> > 

-- 
Regards,
Niklas Söderlund

[PATCH/RFC v2 15/15] adv748x: afe: add routing support

[Niklas Söderlund]

The adv748x afe have eight analog sink pads, currently one of them is
chosen to be the active route based on device tree configuration. Whit
the new routeing API it's possible to control which of the eight sink
pads are routed to the source pad.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-afe.c | 66 +++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
index 5188178588c9067d..5dda85c707f6efd7 100644
--- a/drivers/media/i2c/adv748x/adv748x-afe.c
+++ b/drivers/media/i2c/adv748x/adv748x-afe.c
@@ -43,6 +43,9 @@
 #define ADV748X_AFE_STD_PAL_SECAM			0xe
 #define ADV748X_AFE_STD_PAL_SECAM_PED			0xf
 
+#define ADV748X_AFE_ROUTES_MAX ((ADV748X_AFE_SINK_AIN7 - \
+				ADV748X_AFE_SINK_AIN0) + 1)
+
 static int adv748x_afe_read_ro_map(struct adv748x_state *state, u8 reg)
 {
 	int ret;
@@ -386,10 +389,73 @@ static int adv748x_afe_set_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+
+static int adv748x_afe_get_routing(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_routing *routing)
+{
+	struct adv748x_afe *afe = adv748x_sd_to_afe(sd);
+	struct v4l2_subdev_route *r = routing->routes;
+	unsigned int i;
+
+	/* There are one possible route from each sink */
+	if (routing->num_routes < ADV748X_AFE_ROUTES_MAX) {
+		routing->num_routes = ADV748X_AFE_ROUTES_MAX;
+		return -ENOSPC;
+	}
+
+	routing->num_routes = ADV748X_AFE_ROUTES_MAX;
+
+	for (i = ADV748X_AFE_SINK_AIN0; i <= ADV748X_AFE_SINK_AIN7; i++) {
+		r->sink_pad = i;
+		r->sink_stream = 0;
+		r->source_pad = ADV748X_AFE_SOURCE;
+		r->source_stream = 0;
+		r->flags = afe->input == i ? V4L2_SUBDEV_ROUTE_FL_ACTIVE : 0;
+		r++;
+	}
+
+	return 0;
+}
+
+static int adv748x_afe_set_routing(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_routing *routing)
+{
+	struct adv748x_afe *afe = adv748x_sd_to_afe(sd);
+	struct v4l2_subdev_route *r = routing->routes;
+	int input = -1;
+	unsigned int i;
+
+	if (routing->num_routes > ADV748X_AFE_ROUTES_MAX)
+		return -ENOSPC;
+
+	for (i = 0; i < routing->num_routes; i++) {
+		if (r->sink_pad > ADV748X_AFE_SINK_AIN7 ||
+		    r->sink_stream != 0 ||
+		    r->source_pad != ADV748X_AFE_SOURCE ||
+		    r->source_stream != 0)
+			return -EINVAL;
+
+		if (r->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE) {
+			if (input != -1)
+				return -EMLINK;
+
+			input = r->sink_pad;
+		}
+		r++;
+	}
+
+	if (input != -1)
+		afe->input = input;
+
+	return 0;
+}
+
 static const struct v4l2_subdev_pad_ops adv748x_afe_pad_ops = {
 	.enum_mbus_code = adv748x_afe_enum_mbus_code,
 	.set_fmt = adv748x_afe_set_format,
 	.get_fmt = adv748x_afe_get_format,
+	.get_routing = adv748x_afe_get_routing,
+	.set_routing = adv748x_afe_set_routing,
 };
 
 /* -----------------------------------------------------------------------------
-- 
2.15.1

Re: [PATCH/RFC v2 15/15] adv748x: afe: add routing support

[Kieran Bingham]

Hi Niklas,

On 14/12/17 19:08, Niklas Söderlund wrote:
> The adv748x afe have eight analog sink pads, currently one of them is

s/have/has/

> chosen to be the active route based on device tree configuration. Whit

s/Whit/With/

> the new routeing API it's possible to control which of the eight sink
> pads are routed to the source pad.

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

Aha, I had been wondering how we would handle this...

Other than the minor nits, this is otherwise looking good

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/adv748x/adv748x-afe.c | 66 +++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> index 5188178588c9067d..5dda85c707f6efd7 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -43,6 +43,9 @@
>  #define ADV748X_AFE_STD_PAL_SECAM			0xe
>  #define ADV748X_AFE_STD_PAL_SECAM_PED			0xf
>  
> +#define ADV748X_AFE_ROUTES_MAX ((ADV748X_AFE_SINK_AIN7 - \
> +				ADV748X_AFE_SINK_AIN0) + 1)
> +
>  static int adv748x_afe_read_ro_map(struct adv748x_state *state, u8 reg)
>  {
>  	int ret;
> @@ -386,10 +389,73 @@ static int adv748x_afe_set_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +

No need for that extra line..

> +static int adv748x_afe_get_routing(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_routing *routing)
> +{
> +	struct adv748x_afe *afe = adv748x_sd_to_afe(sd);
> +	struct v4l2_subdev_route *r = routing->routes;
> +	unsigned int i;
> +
> +	/* There are one possible route from each sink */

	There is one possible ...

> +	if (routing->num_routes < ADV748X_AFE_ROUTES_MAX) {
> +		routing->num_routes = ADV748X_AFE_ROUTES_MAX;
> +		return -ENOSPC;
> +	}
> +
> +	routing->num_routes = ADV748X_AFE_ROUTES_MAX;
> +
> +	for (i = ADV748X_AFE_SINK_AIN0; i <= ADV748X_AFE_SINK_AIN7; i++) {
> +		r->sink_pad = i;
> +		r->sink_stream = 0;
> +		r->source_pad = ADV748X_AFE_SOURCE;
> +		r->source_stream = 0;
> +		r->flags = afe->input == i ? V4L2_SUBDEV_ROUTE_FL_ACTIVE : 0;
> +		r++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int adv748x_afe_set_routing(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_routing *routing)
> +{
> +	struct adv748x_afe *afe = adv748x_sd_to_afe(sd);
> +	struct v4l2_subdev_route *r = routing->routes;
> +	int input = -1;
> +	unsigned int i;
> +
> +	if (routing->num_routes > ADV748X_AFE_ROUTES_MAX)
> +		return -ENOSPC;
> +
> +	for (i = 0; i < routing->num_routes; i++) {
> +		if (r->sink_pad > ADV748X_AFE_SINK_AIN7 ||
> +		    r->sink_stream != 0 ||
> +		    r->source_pad != ADV748X_AFE_SOURCE ||
> +		    r->source_stream != 0)
> +			return -EINVAL;
> +
> +		if (r->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE) {
> +			if (input != -1)
> +				return -EMLINK;
> +
> +			input = r->sink_pad;
> +		}
> +		r++;
> +	}
> +
> +	if (input != -1)
> +		afe->input = input;> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_pad_ops adv748x_afe_pad_ops = {
>  	.enum_mbus_code = adv748x_afe_enum_mbus_code,
>  	.set_fmt = adv748x_afe_set_format,
>  	.get_fmt = adv748x_afe_get_format,
> +	.get_routing = adv748x_afe_get_routing,
> +	.set_routing = adv748x_afe_set_routing,
>  };
>  
>  /* -----------------------------------------------------------------------------
> 

Re: [PATCH/RFC v2 15/15] adv748x: afe: add routing support

[Kieran Bingham]

One more ...

On 14/12/17 22:56, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 14/12/17 19:08, Niklas Söderlund wrote:
>> The adv748x afe have eight analog sink pads, currently one of them is
> 
> s/have/has/
> 
>> chosen to be the active route based on device tree configuration. Whit
> 
> s/Whit/With/
> 
>> the new routeing API it's possible to control which of the eight sink

While routeing is correctly spelt, it is used as routing everywhere else...

s/routeing/routing/

>> pads are routed to the source pad.
> 
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Aha, I had been wondering how we would handle this...
> 
> Other than the minor nits, this is otherwise looking good
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
>> ---
>>  drivers/media/i2c/adv748x/adv748x-afe.c | 66 +++++++++++++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>>
>> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
>> index 5188178588c9067d..5dda85c707f6efd7 100644
>> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
>> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
>> @@ -43,6 +43,9 @@
>>  #define ADV748X_AFE_STD_PAL_SECAM			0xe
>>  #define ADV748X_AFE_STD_PAL_SECAM_PED			0xf
>>  
>> +#define ADV748X_AFE_ROUTES_MAX ((ADV748X_AFE_SINK_AIN7 - \
>> +				ADV748X_AFE_SINK_AIN0) + 1)
>> +
>>  static int adv748x_afe_read_ro_map(struct adv748x_state *state, u8 reg)
>>  {
>>  	int ret;
>> @@ -386,10 +389,73 @@ static int adv748x_afe_set_format(struct v4l2_subdev *sd,
>>  	return 0;
>>  }
>>  
>> +
> 
> No need for that extra line..
> 
>> +static int adv748x_afe_get_routing(struct v4l2_subdev *sd,
>> +				   struct v4l2_subdev_routing *routing)
>> +{
>> +	struct adv748x_afe *afe = adv748x_sd_to_afe(sd);
>> +	struct v4l2_subdev_route *r = routing->routes;
>> +	unsigned int i;
>> +
>> +	/* There are one possible route from each sink */
> 
> 	There is one possible ...
> 
>> +	if (routing->num_routes < ADV748X_AFE_ROUTES_MAX) {
>> +		routing->num_routes = ADV748X_AFE_ROUTES_MAX;
>> +		return -ENOSPC;
>> +	}
>> +
>> +	routing->num_routes = ADV748X_AFE_ROUTES_MAX;
>> +
>> +	for (i = ADV748X_AFE_SINK_AIN0; i <= ADV748X_AFE_SINK_AIN7; i++) {
>> +		r->sink_pad = i;
>> +		r->sink_stream = 0;
>> +		r->source_pad = ADV748X_AFE_SOURCE;
>> +		r->source_stream = 0;
>> +		r->flags = afe->input == i ? V4L2_SUBDEV_ROUTE_FL_ACTIVE : 0;
>> +		r++;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int adv748x_afe_set_routing(struct v4l2_subdev *sd,
>> +				   struct v4l2_subdev_routing *routing)
>> +{
>> +	struct adv748x_afe *afe = adv748x_sd_to_afe(sd);
>> +	struct v4l2_subdev_route *r = routing->routes;
>> +	int input = -1;
>> +	unsigned int i;
>> +
>> +	if (routing->num_routes > ADV748X_AFE_ROUTES_MAX)
>> +		return -ENOSPC;
>> +
>> +	for (i = 0; i < routing->num_routes; i++) {
>> +		if (r->sink_pad > ADV748X_AFE_SINK_AIN7 ||
>> +		    r->sink_stream != 0 ||
>> +		    r->source_pad != ADV748X_AFE_SOURCE ||
>> +		    r->source_stream != 0)
>> +			return -EINVAL;
>> +
>> +		if (r->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE) {
>> +			if (input != -1)
>> +				return -EMLINK;
>> +
>> +			input = r->sink_pad;
>> +		}
>> +		r++;
>> +	}
>> +
>> +	if (input != -1)
>> +		afe->input = input;> +
>> +	return 0;
>> +}
>> +
>>  static const struct v4l2_subdev_pad_ops adv748x_afe_pad_ops = {
>>  	.enum_mbus_code = adv748x_afe_enum_mbus_code,
>>  	.set_fmt = adv748x_afe_set_format,
>>  	.get_fmt = adv748x_afe_get_format,
>> +	.get_routing = adv748x_afe_get_routing,
>> +	.set_routing = adv748x_afe_set_routing,
>>  };
>>  
>>  /* -----------------------------------------------------------------------------
>>