[PATCH v2 0/6] v4l2-subdev: Introduce get_mbus_format pad op

[PATCH v2 0/6] v4l2-subdev: Introduce get_mbus_format pad op

[Jacopo Mondi]

v2 introduces two new patches that could be likely squashed in later to
deprecate the g_mbus_config() operation in documentation and expand the newly
introduced function documentation by popular demand.

Will report again the use cases I'm trying to address here:
------------------------------------------------------------------------------
Quoting:
https://patchwork.kernel.org/cover/10855919/
"The use case this series cover is the following one:
the Gen-3 R-Car boards include an ADV748x HDMI/CVBS to CSI-2 converter
connected to its CSI-2 receivers. The ADV748x chip has recently gained support
for routing both HDMI and analogue video streams through its 4 lanes TXA
transmitter, specifically to support the Ebisu board that has a single CSI-2
receiver, compared to all other Gen-3 board where the ADV748x TXes are connected
to different CSI-2 receivers, and where analogue video is streamed out from the
ADV748x single lane TXB transmitter.
To properly support transmission of analogue video through TXA, the number of
data lanes shall be dynamically reduced to 1, in order to comply with the MIPI
CSI-2 minimum clock frequency requirements"

During the discussion of the RFC, Dave reported another use case for media
bus parameter negotiation on his platform:
https://patchwork.kernel.org/patch/10855923/#22569149

Another possible use case is for parallel bus multiplexing, where multiple image
sensor share the parallel bus lines and they get activated alternatively through
an enable signal. While this might not be most clever design, it's often seen
in the wild, and this operation allow receivers to re-configure their bus
parameter in between streaming session.

Hyun is now using this series to configure GMSL devices.
------------------------------------------------------------------------------

v1->v2:
- Address Sakari's comment to use unsigned int in place of bools
- Add two new patches to address documentation
- Adjust rcar-csi2 patch as much as possible according to Niklas comments
- Add Niklas's tags

Jacopo Mondi (6):
  media: v4l2-subdv: Introduce get_mbus_config pad op
  media: v4l2-subdev: Deprecate g_mbus_config video op
  media: v4l2-subdev: Expand get_mbus_config doc
  media: i2c: adv748x: Adjust TXA data lanes number
  media: i2c: adv748x: Implement get_mbus_config
  media: rcar-csi2: Negotiate data lanes number

 drivers/media/i2c/adv748x/adv748x-core.c    | 31 ++++++--
 drivers/media/i2c/adv748x/adv748x-csi2.c    | 15 ++++
 drivers/media/i2c/adv748x/adv748x.h         |  1 +
 drivers/media/platform/rcar-vin/rcar-csi2.c | 53 ++++++++++++-
 include/media/v4l2-subdev.h                 | 82 ++++++++++++++++++++-
 5 files changed, 171 insertions(+), 11 deletions(-)

--
2.26.0

[PATCH v2 1/6] media: v4l2-subdv: Introduce get_mbus_config pad op

[Jacopo Mondi]

Introduce a new pad operation to allow retrieving the media bus
configuration on a subdevice pad.

The newly introduced operation reassembles the s/g_mbus_config video
operation, which have been on their way to be deprecated since a long
time.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 69 +++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a4848de59852..fc16af578471 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc {
 	unsigned short num_entries;
 };
 
+/**
+ * struct v4l2_mbus_parallel_config - parallel mbus configuration
+ * @hsync_active: hsync active state: 1 for high, 0 for low
+ * @vsync_active: vsync active state: 1 for high, 0 for low
+ * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling
+ * @data_active: data lines active state: 1 for high, 0 for low
+ */
+struct v4l2_mbus_parallel_config {
+	unsigned int hsync_active : 1;
+	unsigned int vsync_active : 1;
+	unsigned int pclk_rising : 1;
+	unsigned int data_active : 1;
+};
+
+/**
+ * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
+ * @data_lanes: number of data lanes in use
+ * @clock_noncontinuous: non continuous clock enable flag: 1 for non
+ *			 continuous clock, 0 for continuous clock.
+ */
+struct v4l2_mbus_csi2_dphy_config {
+	unsigned int data_lanes : 3;
+	unsigned int clock_noncontinuous : 1;
+};
+
+/**
+ * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
+ *
+ * TODO
+ */
+struct v4l2_mbus_csi2_cphy_config {
+	/* TODO */
+};
+
+/**
+ * struct v4l2_mbus_pad_config - media bus configuration
+ *
+ * Report the subdevice media bus information to inform the caller of the
+ * current bus configuration. The structure describes bus configuration
+ * parameters that might change in-between streaming sessions, in order to allow
+ * the caller to adjust its media bus configuration to match what is reported
+ * here.
+ *
+ * TODO: add '_pad_' to the name to distinguish this from the structure
+ * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
+ * video operations. Reuse the there defined enum v4l2_mbus_type to define
+ * the bus type.
+ *
+ * @type: mbus type. See &enum v4l2_mbus_type
+ * @parallel: parallel bus configuration parameters.
+ *	      See &struct v4l2_mbus_parallel_config
+ * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
+ *	       See &struct v4l2_mbus_csi2_dphy_config
+ * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
+ *	       See &struct v4l2_mbus_csi2_cphy_config
+ */
+struct v4l2_mbus_pad_config {
+	enum v4l2_mbus_type type;
+	union {
+		struct v4l2_mbus_parallel_config parallel;
+		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
+		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
+	};
+};
+
 /**
  * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
  *				  in video mode.
@@ -670,6 +735,8 @@ 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.
+ *
+ * @get_mbus_config: get the current mbus configuration
  */
 struct v4l2_subdev_pad_ops {
 	int (*init_cfg)(struct v4l2_subdev *sd,
@@ -710,6 +777,8 @@ struct v4l2_subdev_pad_ops {
 			      struct v4l2_mbus_frame_desc *fd);
 	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
 			      struct v4l2_mbus_frame_desc *fd);
+	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
+			       struct v4l2_mbus_pad_config *config);
 };
 
 /**
-- 
2.26.0

Re: [PATCH v2 1/6] media: v4l2-subdv: Introduce get_mbus_config pad op

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 15, 2020 at 12:49:58PM +0200, Jacopo Mondi wrote:
> Introduce a new pad operation to allow retrieving the media bus
> configuration on a subdevice pad.
> 
> The newly introduced operation reassembles the s/g_mbus_config video
> operation, which have been on their way to be deprecated since a long
> time.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 69 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a4848de59852..fc16af578471 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc {
>  	unsigned short num_entries;
>  };
>  
> +/**
> + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> + * @hsync_active: hsync active state: 1 for high, 0 for low
> + * @vsync_active: vsync active state: 1 for high, 0 for low
> + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling

Is this for the driving side or the sampling side ?

> + * @data_active: data lines active state: 1 for high, 0 for low

I wonder, is there any system with active low data lines ?

> + */
> +struct v4l2_mbus_parallel_config {

Is this intended to cover BT.656 too ? Either way I think it should be
documented.

> +	unsigned int hsync_active : 1;
> +	unsigned int vsync_active : 1;
> +	unsigned int pclk_rising : 1;
> +	unsigned int data_active : 1;

Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used
in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is
getting deprecated, it doesn't mean we can reuse the same flags if they
make sense. Otherwise we'll have to translate between
v4l2_fwnode_bus_parallel.flags and the fields here. Ideally
v4l2_fwnode_bus_parallel should be replaced with
v4l2_mbus_parallel_config (once we add additional fields).

> +};
> +
> +/**
> + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration

s/DPHY/D-PHY/ (same below)

> + * @data_lanes: number of data lanes in use
> + * @clock_noncontinuous: non continuous clock enable flag: 1 for non
> + *			 continuous clock, 0 for continuous clock.
> + */
> +struct v4l2_mbus_csi2_dphy_config {
> +	unsigned int data_lanes : 3;
> +	unsigned int clock_noncontinuous : 1;
> +};
> +
> +/**
> + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> + *
> + * TODO
> + */
> +struct v4l2_mbus_csi2_cphy_config {
> +	/* TODO */
> +};

How about leaving this one out for now as it's empty ?

> +
> +/**
> + * struct v4l2_mbus_pad_config - media bus configuration
> + *
> + * Report the subdevice media bus information to inform the caller of the
> + * current bus configuration. The structure describes bus configuration
> + * parameters that might change in-between streaming sessions, in order to allow
> + * the caller to adjust its media bus configuration to match what is reported
> + * here.

I'd focus here on what the structure contains rather than how it's used,
usage belongs to the documentation of the .get_mbus_config() operation.

I think the documentation should specify clearly that this is about the
physical bus configuration (as it excludes virtual channels on CSI-2 for
instance), and should also explain that this is about usage of the
physical bus, not just its hardware configuration on the PCB (as the
intent is to report the number of CSI-2 D-PHY data lanes actually used,
not the number of data lanes present on the board for instance).

> + *
> + * TODO: add '_pad_' to the name to distinguish this from the structure
> + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> + * the bus type.

What is this TODO about ? There's a '_pad_' in the name of this
structure.

> + *
> + * @type: mbus type. See &enum v4l2_mbus_type
> + * @parallel: parallel bus configuration parameters.
> + *	      See &struct v4l2_mbus_parallel_config
> + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> + *	       See &struct v4l2_mbus_csi2_dphy_config
> + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> + *	       See &struct v4l2_mbus_csi2_cphy_config
> + */
> +struct v4l2_mbus_pad_config {
> +	enum v4l2_mbus_type type;
> +	union {
> +		struct v4l2_mbus_parallel_config parallel;
> +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> +	};
> +};
> +
>  /**
>   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
>   *				  in video mode.
> @@ -670,6 +735,8 @@ 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.
> + *
> + * @get_mbus_config: get the current mbus configuration

I was going to say that this is a bit too short, but then saw patch 3/6
:-)

>   */
>  struct v4l2_subdev_pad_ops {
>  	int (*init_cfg)(struct v4l2_subdev *sd,
> @@ -710,6 +777,8 @@ struct v4l2_subdev_pad_ops {
>  			      struct v4l2_mbus_frame_desc *fd);
>  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
>  			      struct v4l2_mbus_frame_desc *fd);
> +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> +			       struct v4l2_mbus_pad_config *config);
>  };
>  
>  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/6] media: v4l2-subdv: Introduce get_mbus_config pad op

[Sakari Ailus]

Hi Laurent,

On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Wed, Apr 15, 2020 at 12:49:58PM +0200, Jacopo Mondi wrote:
> > Introduce a new pad operation to allow retrieving the media bus
> > configuration on a subdevice pad.
> > 
> > The newly introduced operation reassembles the s/g_mbus_config video
> > operation, which have been on their way to be deprecated since a long
> > time.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 69 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index a4848de59852..fc16af578471 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc {
> >  	unsigned short num_entries;
> >  };
> >  
> > +/**
> > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > + * @hsync_active: hsync active state: 1 for high, 0 for low
> > + * @vsync_active: vsync active state: 1 for high, 0 for low
> > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling
> 
> Is this for the driving side or the sampling side ?

Is there a difference? I'd expect the configuration needs to be the same on
both sides.

> 
> > + * @data_active: data lines active state: 1 for high, 0 for low
> 
> I wonder, is there any system with active low data lines ?
> 
> > + */
> > +struct v4l2_mbus_parallel_config {
> 
> Is this intended to cover BT.656 too ? Either way I think it should be
> documented.

I think we should replace what directly refers to Bt.601 with something
that refers to that, and not "parallel". Both are parallel after all. I
think the naming is in line with that, assuming this covers both.

> 
> > +	unsigned int hsync_active : 1;
> > +	unsigned int vsync_active : 1;
> > +	unsigned int pclk_rising : 1;
> > +	unsigned int data_active : 1;
> 
> Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used
> in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is

I'd think it's easier to work with fields in drivers than the flags. This
isn't uAPI so we don't need to think the ABI. The change could also be done
to struct v4l2_fwnode_bus_parallel.

> getting deprecated, it doesn't mean we can reuse the same flags if they
> make sense. Otherwise we'll have to translate between
> v4l2_fwnode_bus_parallel.flags and the fields here. Ideally
> v4l2_fwnode_bus_parallel should be replaced with
> v4l2_mbus_parallel_config (once we add additional fields).

...

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 1/6] media: v4l2-subdv: Introduce get_mbus_config pad op

[Jacopo Mondi]

Hi Sakari, Laurent,

On Wed, Apr 29, 2020 at 04:54:30PM +0300, Sakari Ailus wrote:
> Hi Laurent,
>
> On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Wed, Apr 15, 2020 at 12:49:58PM +0200, Jacopo Mondi wrote:
> > > Introduce a new pad operation to allow retrieving the media bus
> > > configuration on a subdevice pad.
> > >
> > > The newly introduced operation reassembles the s/g_mbus_config video
> > > operation, which have been on their way to be deprecated since a long
> > > time.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  include/media/v4l2-subdev.h | 69 +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 69 insertions(+)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index a4848de59852..fc16af578471 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc {
> > >  	unsigned short num_entries;
> > >  };
> > >
> > > +/**
> > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > + * @hsync_active: hsync active state: 1 for high, 0 for low
> > > + * @vsync_active: vsync active state: 1 for high, 0 for low
> > > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling
> >
> > Is this for the driving side or the sampling side ?
>
> Is there a difference? I'd expect the configuration needs to be the same on
> both sides.

I was puzzled as well by this question, mostly because I never found
anything like this in the existing documentation, but actually yes,
the driving side clocks out data on one edge, sampling side samples on
the opposite one ? Is this what you meant Laurent ?

To me this has always been about sampling side though, and the setting
should match on both endpoints of course.

>
> >
> > > + * @data_active: data lines active state: 1 for high, 0 for low
> >
> > I wonder, is there any system with active low data lines ?

As this is part of the standard DT properties for video interfaces, I
added it here.

> >
> > > + */
> > > +struct v4l2_mbus_parallel_config {
> >
> > Is this intended to cover BT.656 too ? Either way I think it should be
> > documented.
>
> I think we should replace what directly refers to Bt.601 with something
> that refers to that, and not "parallel". Both are parallel after all. I
> think the naming is in line with that, assuming this covers both.
>

Currently in v4l2-fwnode BT.656 is selected if no vertical/horizontal
synch and field flags are specified. This implies the following DT
properties are shared between BT.601 and BT.656:
- pclk-sample
- data-active
- slave-mode
- bus-width
- data-shift
- sync-on-green-active
- data-enable-active

with
- hsync-active
- vsync-active
- field-even-active
being BT.601 only.

We could do here do the same and mention which flags apply to 601
only, or more clearly split the config structure by keeping a generic
'parallel' flag structure, with a sub-structure which is 601 specific.
I'm not sure it's worth the extra layer of indirection though.


> >
> > > +	unsigned int hsync_active : 1;
> > > +	unsigned int vsync_active : 1;
> > > +	unsigned int pclk_rising : 1;
> > > +	unsigned int data_active : 1;
> >
> > Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used
> > in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is
>
> I'd think it's easier to work with fields in drivers than the flags. This

I find it easier and less error prone to work with fields as well,
provided the space occupation is the same as working with flags.

> isn't uAPI so we don't need to think the ABI. The change could also be done
> to struct v4l2_fwnode_bus_parallel.
>
> > getting deprecated, it doesn't mean we can reuse the same flags if they
> > make sense. Otherwise we'll have to translate between
> > v4l2_fwnode_bus_parallel.flags and the fields here. Ideally

Right, I agree this is not desirable. Every driver should inspect the
fwnode properties and translate to this new config when queryed
through get_mbus_format.

> > v4l2_fwnode_bus_parallel should be replaced with
> > v4l2_mbus_parallel_config (once we add additional fields).

I like this idea

We could indeed expand the .flags structure of v4l2_fwnode_bus_parallel

struct v4l2_fwnode_bus_parallel {
	unsigned int flags;
	unsigned char bus_width;
	unsigned char data_shift;
};

but then we should replace the whole structure.

All in all, working with the same set of flags is the less disruptive
change and would not require translations in drivers... I'm not a fan,
but currently seems the easiest way forward...

What do you think ?

>
> ...
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH v2 1/6] media: v4l2-subdv: Introduce get_mbus_config pad op

[Sakari Ailus]

Hi Jacopo,

On Mon, May 11, 2020 at 01:32:39PM +0200, Jacopo Mondi wrote:
> Hi Sakari, Laurent,
> 
> On Wed, Apr 29, 2020 at 04:54:30PM +0300, Sakari Ailus wrote:
> > Hi Laurent,
> >
> > On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> > >
> > > On Wed, Apr 15, 2020 at 12:49:58PM +0200, Jacopo Mondi wrote:
> > > > Introduce a new pad operation to allow retrieving the media bus
> > > > configuration on a subdevice pad.
> > > >
> > > > The newly introduced operation reassembles the s/g_mbus_config video
> > > > operation, which have been on their way to be deprecated since a long
> > > > time.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  include/media/v4l2-subdev.h | 69 +++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 69 insertions(+)
> > > >
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index a4848de59852..fc16af578471 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc {
> > > >  	unsigned short num_entries;
> > > >  };
> > > >
> > > > +/**
> > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > > + * @hsync_active: hsync active state: 1 for high, 0 for low
> > > > + * @vsync_active: vsync active state: 1 for high, 0 for low
> > > > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling
> > >
> > > Is this for the driving side or the sampling side ?
> >
> > Is there a difference? I'd expect the configuration needs to be the same on
> > both sides.
> 
> I was puzzled as well by this question, mostly because I never found
> anything like this in the existing documentation, but actually yes,
> the driving side clocks out data on one edge, sampling side samples on
> the opposite one ? Is this what you meant Laurent ?
> 
> To me this has always been about sampling side though, and the setting
> should match on both endpoints of course.
> 
> >
> > >
> > > > + * @data_active: data lines active state: 1 for high, 0 for low
> > >
> > > I wonder, is there any system with active low data lines ?
> 
> As this is part of the standard DT properties for video interfaces, I
> added it here.
> 
> > >
> > > > + */
> > > > +struct v4l2_mbus_parallel_config {
> > >
> > > Is this intended to cover BT.656 too ? Either way I think it should be
> > > documented.
> >
> > I think we should replace what directly refers to Bt.601 with something
> > that refers to that, and not "parallel". Both are parallel after all. I
> > think the naming is in line with that, assuming this covers both.
> >
> 
> Currently in v4l2-fwnode BT.656 is selected if no vertical/horizontal
> synch and field flags are specified. This implies the following DT
> properties are shared between BT.601 and BT.656:
> - pclk-sample
> - data-active
> - slave-mode
> - bus-width
> - data-shift
> - sync-on-green-active
> - data-enable-active
> 
> with
> - hsync-active
> - vsync-active
> - field-even-active
> being BT.601 only.
> 
> We could do here do the same and mention which flags apply to 601
> only, or more clearly split the config structure by keeping a generic
> 'parallel' flag structure, with a sub-structure which is 601 specific.
> I'm not sure it's worth the extra layer of indirection though.
> 
> 
> > >
> > > > +	unsigned int hsync_active : 1;
> > > > +	unsigned int vsync_active : 1;
> > > > +	unsigned int pclk_rising : 1;
> > > > +	unsigned int data_active : 1;
> > >
> > > Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used
> > > in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is
> >
> > I'd think it's easier to work with fields in drivers than the flags. This
> 
> I find it easier and less error prone to work with fields as well,
> provided the space occupation is the same as working with flags.
> 
> > isn't uAPI so we don't need to think the ABI. The change could also be done
> > to struct v4l2_fwnode_bus_parallel.
> >
> > > getting deprecated, it doesn't mean we can reuse the same flags if they
> > > make sense. Otherwise we'll have to translate between
> > > v4l2_fwnode_bus_parallel.flags and the fields here. Ideally
> 
> Right, I agree this is not desirable. Every driver should inspect the
> fwnode properties and translate to this new config when queryed
> through get_mbus_format.
> 
> > > v4l2_fwnode_bus_parallel should be replaced with
> > > v4l2_mbus_parallel_config (once we add additional fields).
> 
> I like this idea
> 
> We could indeed expand the .flags structure of v4l2_fwnode_bus_parallel
> 
> struct v4l2_fwnode_bus_parallel {
> 	unsigned int flags;
> 	unsigned char bus_width;
> 	unsigned char data_shift;
> };
> 
> but then we should replace the whole structure.
> 
> All in all, working with the same set of flags is the less disruptive
> change and would not require translations in drivers... I'm not a fan,
> but currently seems the easiest way forward...
> 
> What do you think ?

Could we use a struct instead, say:

struct v4l2_parallel_flags {
	unsigned int hsync_active:1;
	/* and so on */
};

And then you'd add this to struct v4l2_fwnode_bus_parallel as a field. No
defines would be needed this way, and it'd be slightly safer because you
get types checked by the compilter.

I don't have strong opinion either way. Both would work just fine.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 1/6] media: v4l2-subdv: Introduce get_mbus_config pad op

[Laurent Pinchart]

Hello,

On Mon, May 11, 2020 at 11:03:54PM +0300, Sakari Ailus wrote:
> On Mon, May 11, 2020 at 01:32:39PM +0200, Jacopo Mondi wrote:
> > On Wed, Apr 29, 2020 at 04:54:30PM +0300, Sakari Ailus wrote:
> > > On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote:
> > > > On Wed, Apr 15, 2020 at 12:49:58PM +0200, Jacopo Mondi wrote:
> > > > > Introduce a new pad operation to allow retrieving the media bus
> > > > > configuration on a subdevice pad.
> > > > >
> > > > > The newly introduced operation reassembles the s/g_mbus_config video
> > > > > operation, which have been on their way to be deprecated since a long
> > > > > time.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > ---
> > > > >  include/media/v4l2-subdev.h | 69 +++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 69 insertions(+)
> > > > >
> > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > index a4848de59852..fc16af578471 100644
> > > > > --- a/include/media/v4l2-subdev.h
> > > > > +++ b/include/media/v4l2-subdev.h
> > > > > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc {
> > > > >  	unsigned short num_entries;
> > > > >  };
> > > > >
> > > > > +/**
> > > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > > > + * @hsync_active: hsync active state: 1 for high, 0 for low
> > > > > + * @vsync_active: vsync active state: 1 for high, 0 for low
> > > > > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling
> > > >
> > > > Is this for the driving side or the sampling side ?
> > >
> > > Is there a difference? I'd expect the configuration needs to be the same on
> > > both sides.
> > 
> > I was puzzled as well by this question, mostly because I never found
> > anything like this in the existing documentation, but actually yes,
> > the driving side clocks out data on one edge, sampling side samples on
> > the opposite one ? Is this what you meant Laurent ?

Yes, that's what I meant, sorry for the confusion.

> > To me this has always been about sampling side though, and the setting
> > should match on both endpoints of course.

Can we make it explicit ? How about naming the field pclk_sample_edge,
and adding macros name *_RISING and *_FALLING ? See
include/drm/drm_connector.h for an example (DRM_BUS_FLAG_PIXDATA_*).

> > > > > + * @data_active: data lines active state: 1 for high, 0 for low
> > > >
> > > > I wonder, is there any system with active low data lines ?
> > 
> > As this is part of the standard DT properties for video interfaces, I
> > added it here.
> > 
> > > > > + */
> > > > > +struct v4l2_mbus_parallel_config {
> > > >
> > > > Is this intended to cover BT.656 too ? Either way I think it should be
> > > > documented.
> > >
> > > I think we should replace what directly refers to Bt.601 with something
> > > that refers to that, and not "parallel". Both are parallel after all. I
> > > think the naming is in line with that, assuming this covers both.
> > 
> > Currently in v4l2-fwnode BT.656 is selected if no vertical/horizontal
> > synch and field flags are specified. This implies the following DT
> > properties are shared between BT.601 and BT.656:
> > - pclk-sample
> > - data-active
> > - slave-mode
> > - bus-width
> > - data-shift
> > - sync-on-green-active

Isn't this a property of analog signals ?

> > - data-enable-active

Does BT.656 have a data enable signal ?

> > 
> > with
> > - hsync-active
> > - vsync-active
> > - field-even-active
> > being BT.601 only.
> > 
> > We could do here do the same and mention which flags apply to 601
> > only, or more clearly split the config structure by keeping a generic
> > 'parallel' flag structure, with a sub-structure which is 601 specific.
> > I'm not sure it's worth the extra layer of indirection though.

Possibly not, I would be fine with just documenting the structure and
fields in details.

> > > > > +	unsigned int hsync_active : 1;
> > > > > +	unsigned int vsync_active : 1;
> > > > > +	unsigned int pclk_rising : 1;
> > > > > +	unsigned int data_active : 1;
> > > >
> > > > Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used
> > > > in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is
> > >
> > > I'd think it's easier to work with fields in drivers than the flags. This
> > 
> > I find it easier and less error prone to work with fields as well,
> > provided the space occupation is the same as working with flags.

I'm probably influenced by DRM_BUS_FLAG_* here :-) Especially for the
signal polarity, being able to say

	->flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE;

in the driver the transmitter driver, and

	if (->flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE)
		...

in the receiver driver is very nice. I won't push for it though.

> > > isn't uAPI so we don't need to think the ABI. The change could also be done
> > > to struct v4l2_fwnode_bus_parallel.
> > >
> > > > getting deprecated, it doesn't mean we can reuse the same flags if they
> > > > make sense. Otherwise we'll have to translate between
> > > > v4l2_fwnode_bus_parallel.flags and the fields here. Ideally
> > 
> > Right, I agree this is not desirable. Every driver should inspect the
> > fwnode properties and translate to this new config when queryed
> > through get_mbus_format.

Do you mean with a helper function ?

> > > > v4l2_fwnode_bus_parallel should be replaced with
> > > > v4l2_mbus_parallel_config (once we add additional fields).
> > 
> > I like this idea
> > 
> > We could indeed expand the .flags structure of v4l2_fwnode_bus_parallel
> > 
> > struct v4l2_fwnode_bus_parallel {
> > 	unsigned int flags;
> > 	unsigned char bus_width;
> > 	unsigned char data_shift;
> > };
> > 
> > but then we should replace the whole structure.
> > 
> > All in all, working with the same set of flags is the less disruptive
> > change and would not require translations in drivers... I'm not a fan,
> > but currently seems the easiest way forward...
> > 
> > What do you think ?
> 
> Could we use a struct instead, say:
> 
> struct v4l2_parallel_flags {
> 	unsigned int hsync_active:1;
> 	/* and so on */
> };
> 
> And then you'd add this to struct v4l2_fwnode_bus_parallel as a field. No
> defines would be needed this way, and it'd be slightly safer because you
> get types checked by the compilter.
> 
> I don't have strong opinion either way. Both would work just fine.

That's fine with me. As I wrote above I think flags can increase
readability in some cases, but I won't insist.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/6] media: v4l2-subdv: Introduce get_mbus_config pad op

[Jacopo Mondi]

Hi Laurent, Sakari,

On Mon, May 11, 2020 at 11:21:26PM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Mon, May 11, 2020 at 11:03:54PM +0300, Sakari Ailus wrote:
> > On Mon, May 11, 2020 at 01:32:39PM +0200, Jacopo Mondi wrote:
> > > On Wed, Apr 29, 2020 at 04:54:30PM +0300, Sakari Ailus wrote:
> > > > On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote:
> > > > > On Wed, Apr 15, 2020 at 12:49:58PM +0200, Jacopo Mondi wrote:
> > > > > > Introduce a new pad operation to allow retrieving the media bus
> > > > > > configuration on a subdevice pad.
> > > > > >
> > > > > > The newly introduced operation reassembles the s/g_mbus_config video
> > > > > > operation, which have been on their way to be deprecated since a long
> > > > > > time.
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > ---
> > > > > >  include/media/v4l2-subdev.h | 69 +++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 69 insertions(+)
> > > > > >
> > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > > index a4848de59852..fc16af578471 100644
> > > > > > --- a/include/media/v4l2-subdev.h
> > > > > > +++ b/include/media/v4l2-subdev.h
> > > > > > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc {
> > > > > >  	unsigned short num_entries;
> > > > > >  };
> > > > > >
> > > > > > +/**
> > > > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > > > > + * @hsync_active: hsync active state: 1 for high, 0 for low
> > > > > > + * @vsync_active: vsync active state: 1 for high, 0 for low
> > > > > > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling
> > > > >
> > > > > Is this for the driving side or the sampling side ?
> > > >
> > > > Is there a difference? I'd expect the configuration needs to be the same on
> > > > both sides.
> > >
> > > I was puzzled as well by this question, mostly because I never found
> > > anything like this in the existing documentation, but actually yes,
> > > the driving side clocks out data on one edge, sampling side samples on
> > > the opposite one ? Is this what you meant Laurent ?
>
> Yes, that's what I meant, sorry for the confusion.
>
> > > To me this has always been about sampling side though, and the setting
> > > should match on both endpoints of course.
>
> Can we make it explicit ? How about naming the field pclk_sample_edge,
> and adding macros name *_RISING and *_FALLING ? See
> include/drm/drm_connector.h for an example (DRM_BUS_FLAG_PIXDATA_*).
>

Please note that if we want to reuse V4L2_MBUS_* flags, there are
already V4L2_MBUS_PCLK_SAMPLE_RISING and V4L2_MBUS_PCLK_SAMPLE_FALLING

And yes, there's "SAMPLE" in the name :)

> > > > > > + * @data_active: data lines active state: 1 for high, 0 for low
> > > > >
> > > > > I wonder, is there any system with active low data lines ?
> > >
> > > As this is part of the standard DT properties for video interfaces, I
> > > added it here.
> > >
> > > > > > + */
> > > > > > +struct v4l2_mbus_parallel_config {
> > > > >
> > > > > Is this intended to cover BT.656 too ? Either way I think it should be
> > > > > documented.
> > > >
> > > > I think we should replace what directly refers to Bt.601 with something
> > > > that refers to that, and not "parallel". Both are parallel after all. I
> > > > think the naming is in line with that, assuming this covers both.
> > >
> > > Currently in v4l2-fwnode BT.656 is selected if no vertical/horizontal
> > > synch and field flags are specified. This implies the following DT
> > > properties are shared between BT.601 and BT.656:
> > > - pclk-sample
> > > - data-active
> > > - slave-mode
> > > - bus-width
> > > - data-shift
> > > - sync-on-green-active
>
> Isn't this a property of analog signals ?
>
> > > - data-enable-active
>
> Does BT.656 have a data enable signal ?
>

I don't think so, but onle the following three items are listed as
"BT.601" in v4l2-mediabus.h.

I'll update it

> > >
> > > with
> > > - hsync-active
> > > - vsync-active
> > > - field-even-active
> > > being BT.601 only.
> > >
> > > We could do here do the same and mention which flags apply to 601
> > > only, or more clearly split the config structure by keeping a generic
> > > 'parallel' flag structure, with a sub-structure which is 601 specific.
> > > I'm not sure it's worth the extra layer of indirection though.
>
> Possibly not, I would be fine with just documenting the structure and
> fields in details.
>

I think it depends on the last point here below

> > > > > > +	unsigned int hsync_active : 1;
> > > > > > +	unsigned int vsync_active : 1;
> > > > > > +	unsigned int pclk_rising : 1;
> > > > > > +	unsigned int data_active : 1;
> > > > >
> > > > > Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used
> > > > > in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is
> > > >
> > > > I'd think it's easier to work with fields in drivers than the flags. This
> > >
> > > I find it easier and less error prone to work with fields as well,
> > > provided the space occupation is the same as working with flags.
>
> I'm probably influenced by DRM_BUS_FLAG_* here :-) Especially for the
> signal polarity, being able to say
>
> 	->flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE;
>
> in the driver the transmitter driver, and
>
> 	if (->flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE)
> 		...
>
> in the receiver driver is very nice. I won't push for it though.
>
> > > > isn't uAPI so we don't need to think the ABI. The change could also be done
> > > > to struct v4l2_fwnode_bus_parallel.
> > > >
> > > > > getting deprecated, it doesn't mean we can reuse the same flags if they
> > > > > make sense. Otherwise we'll have to translate between
> > > > > v4l2_fwnode_bus_parallel.flags and the fields here. Ideally
> > >
> > > Right, I agree this is not desirable. Every driver should inspect the
> > > fwnode properties and translate to this new config when queryed
> > > through get_mbus_format.
>
> Do you mean with a helper function ?
>
> > > > > v4l2_fwnode_bus_parallel should be replaced with
> > > > > v4l2_mbus_parallel_config (once we add additional fields).
> > >
> > > I like this idea
> > >
> > > We could indeed expand the .flags structure of v4l2_fwnode_bus_parallel
> > >
> > > struct v4l2_fwnode_bus_parallel {
> > > 	unsigned int flags;
> > > 	unsigned char bus_width;
> > > 	unsigned char data_shift;
> > > };
> > >
> > > but then we should replace the whole structure.
> > >
> > > All in all, working with the same set of flags is the less disruptive
> > > change and would not require translations in drivers... I'm not a fan,
> > > but currently seems the easiest way forward...
> > >
> > > What do you think ?
> >
> > Could we use a struct instead, say:
> >
> > struct v4l2_parallel_flags {
> > 	unsigned int hsync_active:1;
> > 	/* and so on */
> > };
> >
> > And then you'd add this to struct v4l2_fwnode_bus_parallel as a field. No
> > defines would be needed this way, and it'd be slightly safer because you
> > get types checked by the compilter.
> >
> > I don't have strong opinion either way. Both would work just fine.
>
> That's fine with me. As I wrote above I think flags can increase
> readability in some cases, but I won't insist.

I'm really debated. Fields are easier to work with for someone and safer.
Flags are easier to work with for some else, and we have a ton of code
that uses flags, including fwnode parsing, from which I expect most of
the information reported by get_mbus_config() come from.

As suggested by Laurent we could provide an helper to translate from
flags collected in "struct v4l2_fwnode_bus_parallel.flags" to
"struct v4l2_parallel_flags" but I'm not sure it is worth the
effort...

All in all, I prefer flags, but the fact flags are used basically
everywhere makes me lean towards re-using V4L2_MBUS_* everywhere I
could...

v3 on its way

Thanks
   j

>
> --
> Regards,
>
> Laurent Pinchart

[PATCH v2 2/6] media: v4l2-subdev: Deprecate g_mbus_config video op

[Jacopo Mondi]

Deprecate 'g_mbus_config' video operation in favor of the newly
introduced 'get_mbus_config' pad operation.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d1a5e9d1ea63..9bf14c41626d 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -466,7 +466,9 @@ struct v4l2_mbus_pad_config {
  *
  * @query_dv_timings: callback for VIDIOC_QUERY_DV_TIMINGS() ioctl handler code.
  *
- * @g_mbus_config: get supported mediabus configurations
+ * @g_mbus_config: get supported mediabus configurations. This operation is
+ *		   deprecated in favour of the get_mbus_config() pad operation
+ *		   and should not be used by new software.
  *
  * @s_mbus_config: set a certain mediabus configuration. This operation is added
  *	for compatibility with soc-camera drivers and should not be used by new
-- 
2.26.0

Re: [PATCH v2 2/6] media: v4l2-subdev: Deprecate g_mbus_config video op

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 15, 2020 at 12:49:59PM +0200, Jacopo Mondi wrote:
> Deprecate 'g_mbus_config' video operation in favor of the newly
> introduced 'get_mbus_config' pad operation.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

Not necessarily a blocker for this series, but it would be nice to
convert the handful of users of this API (you can leave soc-camera
untouched as it's on its way out of the kernel).

> ---
>  include/media/v4l2-subdev.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d1a5e9d1ea63..9bf14c41626d 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -466,7 +466,9 @@ struct v4l2_mbus_pad_config {
>   *
>   * @query_dv_timings: callback for VIDIOC_QUERY_DV_TIMINGS() ioctl handler code.
>   *
> - * @g_mbus_config: get supported mediabus configurations
> + * @g_mbus_config: get supported mediabus configurations. This operation is
> + *		   deprecated in favour of the get_mbus_config() pad operation
> + *		   and should not be used by new software.
>   *
>   * @s_mbus_config: set a certain mediabus configuration. This operation is added
>   *	for compatibility with soc-camera drivers and should not be used by new

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/6] media: v4l2-subdev: Deprecate g_mbus_config video op

[Jacopo Mondi]

Hi Laurent, Hans, Sakari

On Mon, Apr 20, 2020 at 04:48:46AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Apr 15, 2020 at 12:49:59PM +0200, Jacopo Mondi wrote:
> > Deprecate 'g_mbus_config' video operation in favor of the newly
> > introduced 'get_mbus_config' pad operation.
> >
> > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Not necessarily a blocker for this series, but it would be nice to
> convert the handful of users of this API (you can leave soc-camera
> untouched as it's on its way out of the kernel).

I'm trying to, but I'm having an hard time fixing the new operations
semantic, specifically on set_mbus_config, as if I have to deprecate
the existing users, a replacement for s_mbus_config is required.

In particular, the old API seems to work as follows:
1) Report all supported config options through g_mbus_config
2) Apply the requested ones (assumed to be in the set of configurable
ones) though s_mbus_config.

There is no mention of how to treat unsupported configuration
parameters, no mention of how handle configuration errors, if
s_mbus_config should behave like set_fmt (update the received
configuration to reflect the current status and don't fail if the
configuration cannot be applied).

The new operation has a different semantic (which still has to be
defined) particularly:
1) get_mbus_config() reports the -current- media bus configuration, not the
list of supported options that could be tweaked.
2) set_mbus_config() has the following behavior specified:
  - Do not fail if received config is unsupported, update it to
    reflect the current configuration
  - Apply changes to the supported configuration options, ignore the
    non supported one, but do not fail.

I'm still trying to understand if a new set_mbus_config() is actually
welcome or not, but apart from this, I think porting existing users to
the new API, even if it could be made to compile successfully will
change quite some behaviour, and honestly I'm not sure we want to go
that way, or keep both the existing operations and only deprecate the
existing one in the documentation.

For reference ov6650.c is the only mainline user of s_mbus_config I
can see as well pxa_camera is the only platform driver using
g_mbus_config. Porting both to the new API would require testing I
cannot perform probably, or would you be ok with just compile testing
it ?

Thanks
   j

>
> > ---
> >  include/media/v4l2-subdev.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index d1a5e9d1ea63..9bf14c41626d 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -466,7 +466,9 @@ struct v4l2_mbus_pad_config {
> >   *
> >   * @query_dv_timings: callback for VIDIOC_QUERY_DV_TIMINGS() ioctl handler code.
> >   *
> > - * @g_mbus_config: get supported mediabus configurations
> > + * @g_mbus_config: get supported mediabus configurations. This operation is
> > + *		   deprecated in favour of the get_mbus_config() pad operation
> > + *		   and should not be used by new software.
> >   *
> >   * @s_mbus_config: set a certain mediabus configuration. This operation is added
> >   *	for compatibility with soc-camera drivers and should not be used by new
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH v2 3/6] media: v4l2-subdev: Expand get_mbus_config doc

[Jacopo Mondi]

Expand documentation of the newly introduced get_mbus_config() pad
operation.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---

Providing this as separate patch to ease review/discussion.
Can be likely squashed in 1/6

---
 include/media/v4l2-subdev.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 9bf14c41626d..e95f44e778a6 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -737,7 +737,17 @@ 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.
  *
- * @get_mbus_config: get the current mbus configuration
+ * @get_mbus_config: get the current media bus configuration. This operation is
+ *		     intended to be used to synchronize the media bus
+ *		     configuration parameters between receivers and
+ *		     transmitters. The static bus configuration is usually
+ *		     received from the firmware interface, and updated
+ *		     dynamically using this operation to retrieve bus
+ *		     configuration parameters which could change at run-time.
+ *		     Callers should make sure they get the most up-to-date as
+ *		     possible configuration from the connected sub-device,
+ *		     likely calling this operation as close as possible to
+ *		     stream on time.
  */
 struct v4l2_subdev_pad_ops {
 	int (*init_cfg)(struct v4l2_subdev *sd,
--
2.26.0

Re: [PATCH v2 3/6] media: v4l2-subdev: Expand get_mbus_config doc

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 15, 2020 at 12:50:00PM +0200, Jacopo Mondi wrote:
> Expand documentation of the newly introduced get_mbus_config() pad
> operation.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> 
> Providing this as separate patch to ease review/discussion.
> Can be likely squashed in 1/6

Yes, I think it should be squashed.

> ---
>  include/media/v4l2-subdev.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 9bf14c41626d..e95f44e778a6 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -737,7 +737,17 @@ 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.
>   *
> - * @get_mbus_config: get the current mbus configuration
> + * @get_mbus_config: get the current media bus configuration. This operation is
> + *		     intended to be used to synchronize the media bus
> + *		     configuration parameters between receivers and
> + *		     transmitters. The static bus configuration is usually
> + *		     received from the firmware interface, and updated
> + *		     dynamically using this operation to retrieve bus
> + *		     configuration parameters which could change at run-time.
> + *		     Callers should make sure they get the most up-to-date as
> + *		     possible configuration from the connected sub-device,
> + *		     likely calling this operation as close as possible to
> + *		     stream on time.

Much better than a single line, but still quite imprecise :-S I think we
need to describe clearly when the implementer of this operation is
allowed to change the bus configuration for instance, and we need to
think about the locking model between the subdev and the caller.

The other option is to consider that most subdev operations are
currently under-documented and keep going with the flow :-) I will thus
not block this patch series due to the documentation.

>   */
>  struct v4l2_subdev_pad_ops {
>  	int (*init_cfg)(struct v4l2_subdev *sd,

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/6] media: i2c: adv748x: Adjust TXA data lanes number

[Jacopo Mondi]

When outputting SD-Core output through the TXA MIPI CSI-2 interface,
the number of enabled data lanes should be reduced in order to guarantee
the two video format produced by the SD-Core (480i and 576i) generate a
MIPI CSI-2 link clock frequency compatible with the MIPI D-PHY
specifications.

Limit the number of enabled data lanes to 2, which is guaranteed to
support 480i and 576i formats.

Cache the number of enabled data lanes to be able to report it through
the new get_mbus_config operation introduced in the following patches.

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 31 ++++++++++++++++++------
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 23e02ff27b17..1fe7f97c6d52 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -241,10 +241,10 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
 	int ret = 0;
 
 	/* Enable n-lane MIPI */
-	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0x80 | tx->active_lanes, &ret);
 
 	/* Set Auto DPHY Timing */
-	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0xa0 | tx->active_lanes, &ret);
 
 	/* ADI Required Write */
 	if (tx->src == &state->hdmi.sd) {
@@ -270,7 +270,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
 	usleep_range(2000, 2500);
 
 	/* Power-up CSI-TX */
-	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0x20 | tx->active_lanes, &ret);
 	usleep_range(1000, 1500);
 
 	/* ADI Required Writes */
@@ -292,7 +292,7 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
 	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
 
 	/* Enable n-lane MIPI */
-	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0x80 | tx->active_lanes, &ret);
 
 	/* i2c_mipi_pll_en - 1'b1 */
 	adv748x_write_check(state, page, 0xda, 0x01, &ret);
@@ -357,14 +357,29 @@ static int adv748x_link_setup(struct media_entity *entity,
 	if (state->afe.tx) {
 		/* AFE Requires TXA enabled, even when output to TXB */
 		io10 |= ADV748X_IO_10_CSI4_EN;
-		if (is_txa(tx))
+		if (is_txa(tx)) {
+			/*
+			 * Output from the SD-core (480i and 576i) from the TXA
+			 * interface requires reducing the number of enabled
+			 * data lanes in order to guarantee a valid link
+			 * frequency.
+			 */
+			tx->active_lanes = min(tx->num_lanes, 2U);
 			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
-		else
+		} else {
+			/* TXB has a single data lane, no need to adjust. */
 			io10 |= ADV748X_IO_10_CSI1_EN;
+		}
 	}
 
-	if (state->hdmi.tx)
+	if (state->hdmi.tx) {
+		/*
+		 * Restore the number of active lanes, in case we have gone
+		 * through an AFE->TXA streaming sessions.
+		 */
+		tx->active_lanes = tx->num_lanes;
 		io10 |= ADV748X_IO_10_CSI4_EN;
+	}
 
 	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
 }
@@ -596,6 +611,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
 		}
 
 		state->txa.num_lanes = num_lanes;
+		state->txa.active_lanes = num_lanes;
 		adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
 	}
 
@@ -607,6 +623,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
 		}
 
 		state->txb.num_lanes = num_lanes;
+		state->txb.active_lanes = num_lanes;
 		adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
 	}
 
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index fccb388ce179..1061f425ece5 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -79,6 +79,7 @@ struct adv748x_csi2 {
 	unsigned int page;
 	unsigned int port;
 	unsigned int num_lanes;
+	unsigned int active_lanes;
 
 	struct media_pad pads[ADV748X_CSI2_NR_PADS];
 	struct v4l2_ctrl_handler ctrl_hdl;
-- 
2.26.0

Re: [PATCH v2 4/6] media: i2c: adv748x: Adjust TXA data lanes number

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 15, 2020 at 12:50:01PM +0200, Jacopo Mondi wrote:
> When outputting SD-Core output through the TXA MIPI CSI-2 interface,
> the number of enabled data lanes should be reduced in order to guarantee
> the two video format produced by the SD-Core (480i and 576i) generate a
> MIPI CSI-2 link clock frequency compatible with the MIPI D-PHY
> specifications.
> 
> Limit the number of enabled data lanes to 2, which is guaranteed to
> support 480i and 576i formats.
> 
> Cache the number of enabled data lanes to be able to report it through
> the new get_mbus_config operation introduced in the following patches.

When this will be merged it won't be "following patches" anymore :-)

> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 31 ++++++++++++++++++------
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 23e02ff27b17..1fe7f97c6d52 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -241,10 +241,10 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
>  	int ret = 0;
>  
>  	/* Enable n-lane MIPI */
> -	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> +	adv748x_write_check(state, page, 0x00, 0x80 | tx->active_lanes, &ret);
>  
>  	/* Set Auto DPHY Timing */
> -	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
> +	adv748x_write_check(state, page, 0x00, 0xa0 | tx->active_lanes, &ret);
>  
>  	/* ADI Required Write */
>  	if (tx->src == &state->hdmi.sd) {
> @@ -270,7 +270,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
>  	usleep_range(2000, 2500);
>  
>  	/* Power-up CSI-TX */
> -	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
> +	adv748x_write_check(state, page, 0x00, 0x20 | tx->active_lanes, &ret);
>  	usleep_range(1000, 1500);
>  
>  	/* ADI Required Writes */
> @@ -292,7 +292,7 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
>  	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
>  
>  	/* Enable n-lane MIPI */
> -	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> +	adv748x_write_check(state, page, 0x00, 0x80 | tx->active_lanes, &ret);
>  
>  	/* i2c_mipi_pll_en - 1'b1 */
>  	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> @@ -357,14 +357,29 @@ static int adv748x_link_setup(struct media_entity *entity,
>  	if (state->afe.tx) {
>  		/* AFE Requires TXA enabled, even when output to TXB */
>  		io10 |= ADV748X_IO_10_CSI4_EN;
> -		if (is_txa(tx))
> +		if (is_txa(tx)) {
> +			/*
> +			 * Output from the SD-core (480i and 576i) from the TXA
> +			 * interface requires reducing the number of enabled
> +			 * data lanes in order to guarantee a valid link
> +			 * frequency.
> +			 */
> +			tx->active_lanes = min(tx->num_lanes, 2U);
>  			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> -		else
> +		} else {
> +			/* TXB has a single data lane, no need to adjust. */
>  			io10 |= ADV748X_IO_10_CSI1_EN;
> +		}
>  	}
>  
> -	if (state->hdmi.tx)
> +	if (state->hdmi.tx) {
> +		/*
> +		 * Restore the number of active lanes, in case we have gone
> +		 * through an AFE->TXA streaming sessions.
> +		 */
> +		tx->active_lanes = tx->num_lanes;
>  		io10 |= ADV748X_IO_10_CSI4_EN;
> +	}
>  
>  	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
>  }
> @@ -596,6 +611,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
>  		}
>  
>  		state->txa.num_lanes = num_lanes;
> +		state->txa.active_lanes = num_lanes;
>  		adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
>  	}
>  
> @@ -607,6 +623,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
>  		}
>  
>  		state->txb.num_lanes = num_lanes;
> +		state->txb.active_lanes = num_lanes;
>  		adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
>  	}
>  
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index fccb388ce179..1061f425ece5 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -79,6 +79,7 @@ struct adv748x_csi2 {
>  	unsigned int page;
>  	unsigned int port;
>  	unsigned int num_lanes;
> +	unsigned int active_lanes;
>  
>  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
>  	struct v4l2_ctrl_handler ctrl_hdl;

-- 
Regards,

Laurent Pinchart

[PATCH v2 5/6] media: i2c: adv748x: Implement get_mbus_config

[Jacopo Mondi]

Implement the newly introduced get_mbus_config operation to report the
number of currently used data lanes on the MIPI CSI-2 interface.

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

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 2091cda50935..f13563da3ff3 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -214,9 +214,24 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
+					struct v4l2_mbus_pad_config *config)
+{
+	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
+
+	if (pad != ADV748X_CSI2_SOURCE)
+		return -EINVAL;
+
+	config->type = V4L2_MBUS_CSI2_DPHY;
+	config->csi2_dphy.data_lanes = tx->active_lanes;
+
+	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_mbus_config = adv748x_csi2_get_mbus_config,
 };
 
 /* -----------------------------------------------------------------------------
-- 
2.26.0

Re: [PATCH v2 5/6] media: i2c: adv748x: Implement get_mbus_config

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 15, 2020 at 12:50:02PM +0200, Jacopo Mondi wrote:
> Implement the newly introduced get_mbus_config operation to report the
> number of currently used data lanes on the MIPI CSI-2 interface.
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 2091cda50935..f13563da3ff3 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -214,9 +214,24 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  	return ret;
>  }
>  
> +static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
> +					struct v4l2_mbus_pad_config *config)
> +{
> +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> +
> +	if (pad != ADV748X_CSI2_SOURCE)
> +		return -EINVAL;
> +
> +	config->type = V4L2_MBUS_CSI2_DPHY;
> +	config->csi2_dphy.data_lanes = tx->active_lanes;
> +
> +	return 0;

This looks fine, but raises a few questions whose answers need to be
documented in 1/6 I think.

- Is the caller expected to zero the config struct ?
- What are the allowed returned values for this operation, and under
  what circumstances ?

These are generic questions for the API. For this patch,

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

> +}
> +
>  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
>  	.get_fmt = adv748x_csi2_get_format,
>  	.set_fmt = adv748x_csi2_set_format,
> +	.get_mbus_config = adv748x_csi2_get_mbus_config,
>  };
>  
>  /* -----------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart

[PATCH v2 6/6] media: rcar-csi2: Negotiate data lanes number

[Jacopo Mondi]

Use the newly introduced get_mbus_config() subdevice pad operation to
retrieve the remote subdevice MIPI CSI-2 bus configuration and configure
the number of active data lanes accordingly.

In order to be able to call the remote subdevice operation cache the
index of the remote pad connected to the single CSI-2 input port.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---

Niklas I've not been able to fully address comments as -ENOIOCTL command
has to be handled separately as reported by email.

---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 53 +++++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb23a2e9..0061d5ff37e3 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -363,6 +363,7 @@ struct rcar_csi2 {
 	struct v4l2_async_notifier notifier;
 	struct v4l2_async_subdev asd;
 	struct v4l2_subdev *remote;
+	unsigned short remote_pad;

 	struct v4l2_mbus_framefmt mf;

@@ -371,6 +372,7 @@ struct rcar_csi2 {

 	unsigned short lanes;
 	unsigned char lane_swap[4];
+	unsigned short active_lanes;
 };

 static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
@@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)

 	/* Wait for the clock and data lanes to enter LP-11 state. */
 	for (timeout = 0; timeout <= 20; timeout++) {
-		const u32 lane_mask = (1 << priv->lanes) - 1;
+		const u32 lane_mask = (1 << priv->active_lanes) - 1;

 		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
 		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
@@ -471,11 +473,49 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
 	 * bps = link_freq * 2
 	 */
 	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
-	do_div(mbps, priv->lanes * 1000000);
+	do_div(mbps, priv->active_lanes * 1000000);

 	return mbps;
 }

+static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
+{
+	struct v4l2_mbus_pad_config mbus_config;
+	int ret;
+
+	priv->active_lanes = priv->lanes;
+
+	memset(&mbus_config, 0, sizeof(mbus_config));
+	ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config,
+			       priv->remote_pad, &mbus_config);
+	if (ret == -ENOIOCTLCMD) {
+		dev_dbg(priv->dev, "No remote mbus configuration available\n");
+		return 0;
+	}
+
+	if (ret) {
+		dev_err(priv->dev, "Failed to get remote mbus configuration\n");
+		return ret;
+	}
+
+	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
+		dev_err(priv->dev, "Unsupported mbus type %u\n",
+			mbus_config.type);
+		return -EINVAL;
+	}
+
+	if (mbus_config.csi2_dphy.data_lanes > priv->lanes) {
+		dev_err(priv->dev,
+			"Unsupported mbus config: too many data lanes %u\n",
+			mbus_config.csi2_dphy.data_lanes);
+		return -EINVAL;
+	}
+
+	priv->active_lanes = mbus_config.csi2_dphy.data_lanes;
+
+	return 0;
+}
+
 static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 {
 	const struct rcar_csi2_format *format;
@@ -490,6 +530,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	/* Code is validated in set_fmt. */
 	format = rcsi2_code_to_fmt(priv->mf.code);

+	/* Get the remote mbus config to get the number of enabled lanes. */
+	ret = rcsi2_config_active_lanes(priv);
+	if (ret)
+		return ret;
+
 	/*
 	 * Enable all supported CSI-2 channels with virtual channel and
 	 * data type matching.
@@ -522,7 +567,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	}

 	phycnt = PHYCNT_ENABLECLK;
-	phycnt |= (1 << priv->lanes) - 1;
+	phycnt |= (1 << priv->active_lanes) - 1;

 	mbps = rcsi2_calc_mbps(priv, format->bpp);
 	if (mbps < 0)
@@ -748,6 +793,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
 	}

 	priv->remote = subdev;
+	priv->remote_pad = pad;

 	dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad);

@@ -793,6 +839,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
 			priv->lanes);
 		return -EINVAL;
 	}
+	priv->active_lanes = priv->lanes;

 	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
 		priv->lane_swap[i] = i < priv->lanes ?
--
2.26.0

Re: [PATCH v2 6/6] media: rcar-csi2: Negotiate data lanes number

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 15, 2020 at 12:50:03PM +0200, Jacopo Mondi wrote:
> Use the newly introduced get_mbus_config() subdevice pad operation to
> retrieve the remote subdevice MIPI CSI-2 bus configuration and configure
> the number of active data lanes accordingly.
> 
> In order to be able to call the remote subdevice operation cache the
> index of the remote pad connected to the single CSI-2 input port.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> 
> Niklas I've not been able to fully address comments as -ENOIOCTL command
> has to be handled separately as reported by email.
> 
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 53 +++++++++++++++++++--
>  1 file changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb23a2e9..0061d5ff37e3 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -363,6 +363,7 @@ struct rcar_csi2 {
>  	struct v4l2_async_notifier notifier;
>  	struct v4l2_async_subdev asd;
>  	struct v4l2_subdev *remote;
> +	unsigned short remote_pad;

You can make this an unsigned int, it will be more efficient and won't
consume more memory.

> 
>  	struct v4l2_mbus_framefmt mf;
> 
> @@ -371,6 +372,7 @@ struct rcar_csi2 {
> 
>  	unsigned short lanes;
>  	unsigned char lane_swap[4];
> +	unsigned short active_lanes;
>  };
> 
>  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> 
>  	/* Wait for the clock and data lanes to enter LP-11 state. */
>  	for (timeout = 0; timeout <= 20; timeout++) {
> -		const u32 lane_mask = (1 << priv->lanes) - 1;
> +		const u32 lane_mask = (1 << priv->active_lanes) - 1;
> 
>  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
>  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> @@ -471,11 +473,49 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>  	 * bps = link_freq * 2
>  	 */
>  	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> -	do_div(mbps, priv->lanes * 1000000);
> +	do_div(mbps, priv->active_lanes * 1000000);
> 
>  	return mbps;
>  }
> 
> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
> +{
> +	struct v4l2_mbus_pad_config mbus_config;

 = { 0 };

here and you can remove the memset().

> +	int ret;
> +
> +	priv->active_lanes = priv->lanes;
> +
> +	memset(&mbus_config, 0, sizeof(mbus_config));
> +	ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config,
> +			       priv->remote_pad, &mbus_config);
> +	if (ret == -ENOIOCTLCMD) {
> +		dev_dbg(priv->dev, "No remote mbus configuration available\n");
> +		return 0;
> +	}
> +
> +	if (ret) {
> +		dev_err(priv->dev, "Failed to get remote mbus configuration\n");

In the unlikely case that the source implements the operation but only
supports it on other pads (the adv748x driver returns -EINVAL when
called on the sink pad for instance), is this really an error ? I think
it's related to the question I've asked in another patch, regarding what
error values are allowed for this operation and under what conditions.

> +		return ret;
> +	}
> +
> +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> +		dev_err(priv->dev, "Unsupported mbus type %u\n",
> +			mbus_config.type);
> +		return -EINVAL;
> +	}
> +
> +	if (mbus_config.csi2_dphy.data_lanes > priv->lanes) {
> +		dev_err(priv->dev,
> +			"Unsupported mbus config: too many data lanes %u\n",
> +			mbus_config.csi2_dphy.data_lanes);
> +		return -EINVAL;
> +	}
> +
> +	priv->active_lanes = mbus_config.csi2_dphy.data_lanes;
> +
> +	return 0;
> +}
> +
>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
>  	const struct rcar_csi2_format *format;
> @@ -490,6 +530,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	/* Code is validated in set_fmt. */
>  	format = rcsi2_code_to_fmt(priv->mf.code);
> 
> +	/* Get the remote mbus config to get the number of enabled lanes. */
> +	ret = rcsi2_config_active_lanes(priv);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Enable all supported CSI-2 channels with virtual channel and
>  	 * data type matching.
> @@ -522,7 +567,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	}
> 
>  	phycnt = PHYCNT_ENABLECLK;
> -	phycnt |= (1 << priv->lanes) - 1;
> +	phycnt |= (1 << priv->active_lanes) - 1;
> 
>  	mbps = rcsi2_calc_mbps(priv, format->bpp);
>  	if (mbps < 0)
> @@ -748,6 +793,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
>  	}
> 
>  	priv->remote = subdev;
> +	priv->remote_pad = pad;
> 
>  	dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad);
> 
> @@ -793,6 +839,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  			priv->lanes);
>  		return -EINVAL;
>  	}
> +	priv->active_lanes = priv->lanes;
> 
>  	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
>  		priv->lane_swap[i] = i < priv->lanes ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/6] v4l2-subdev: Introduce get_mbus_format pad op

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patches.

On Wed, Apr 15, 2020 at 12:49:57PM +0200, Jacopo Mondi wrote:
> v2 introduces two new patches that could be likely squashed in later to
> deprecate the g_mbus_config() operation in documentation and expand the newly
> introduced function documentation by popular demand.
> 
> Will report again the use cases I'm trying to address here:
> ------------------------------------------------------------------------------
> Quoting:
> https://patchwork.kernel.org/cover/10855919/
> "The use case this series cover is the following one:
> the Gen-3 R-Car boards include an ADV748x HDMI/CVBS to CSI-2 converter
> connected to its CSI-2 receivers. The ADV748x chip has recently gained support
> for routing both HDMI and analogue video streams through its 4 lanes TXA
> transmitter, specifically to support the Ebisu board that has a single CSI-2
> receiver, compared to all other Gen-3 board where the ADV748x TXes are connected
> to different CSI-2 receivers, and where analogue video is streamed out from the
> ADV748x single lane TXB transmitter.
> To properly support transmission of analogue video through TXA, the number of
> data lanes shall be dynamically reduced to 1, in order to comply with the MIPI
> CSI-2 minimum clock frequency requirements"
> 
> During the discussion of the RFC, Dave reported another use case for media
> bus parameter negotiation on his platform:
> https://patchwork.kernel.org/patch/10855923/#22569149
> 
> Another possible use case is for parallel bus multiplexing, where multiple image
> sensor share the parallel bus lines and they get activated alternatively through
> an enable signal. While this might not be most clever design, it's often seen
> in the wild, and this operation allow receivers to re-configure their bus
> parameter in between streaming session.

Isn't this already supported today, with the bus configuration for each
source specified in the corresponding endpoint (on the receiver side) in
DT ?

> Hyun is now using this series to configure GMSL devices.
> ------------------------------------------------------------------------------
> 
> v1->v2:
> - Address Sakari's comment to use unsigned int in place of bools
> - Add two new patches to address documentation
> - Adjust rcar-csi2 patch as much as possible according to Niklas comments
> - Add Niklas's tags
> 
> Jacopo Mondi (6):
>   media: v4l2-subdv: Introduce get_mbus_config pad op
>   media: v4l2-subdev: Deprecate g_mbus_config video op
>   media: v4l2-subdev: Expand get_mbus_config doc
>   media: i2c: adv748x: Adjust TXA data lanes number
>   media: i2c: adv748x: Implement get_mbus_config
>   media: rcar-csi2: Negotiate data lanes number
> 
>  drivers/media/i2c/adv748x/adv748x-core.c    | 31 ++++++--
>  drivers/media/i2c/adv748x/adv748x-csi2.c    | 15 ++++
>  drivers/media/i2c/adv748x/adv748x.h         |  1 +
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 53 ++++++++++++-
>  include/media/v4l2-subdev.h                 | 82 ++++++++++++++++++++-
>  5 files changed, 171 insertions(+), 11 deletions(-)
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/6] v4l2-subdev: Introduce get_mbus_format pad op

[Jacopo Mondi]

Hi Laurent,

On Mon, Apr 20, 2020 at 05:02:28AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patches.
>
> On Wed, Apr 15, 2020 at 12:49:57PM +0200, Jacopo Mondi wrote:
> > v2 introduces two new patches that could be likely squashed in later to
> > deprecate the g_mbus_config() operation in documentation and expand the newly
> > introduced function documentation by popular demand.
> >
> > Will report again the use cases I'm trying to address here:
> > ------------------------------------------------------------------------------
> > Quoting:
> > https://patchwork.kernel.org/cover/10855919/
> > "The use case this series cover is the following one:
> > the Gen-3 R-Car boards include an ADV748x HDMI/CVBS to CSI-2 converter
> > connected to its CSI-2 receivers. The ADV748x chip has recently gained support
> > for routing both HDMI and analogue video streams through its 4 lanes TXA
> > transmitter, specifically to support the Ebisu board that has a single CSI-2
> > receiver, compared to all other Gen-3 board where the ADV748x TXes are connected
> > to different CSI-2 receivers, and where analogue video is streamed out from the
> > ADV748x single lane TXB transmitter.
> > To properly support transmission of analogue video through TXA, the number of
> > data lanes shall be dynamically reduced to 1, in order to comply with the MIPI
> > CSI-2 minimum clock frequency requirements"
> >
> > During the discussion of the RFC, Dave reported another use case for media
> > bus parameter negotiation on his platform:
> > https://patchwork.kernel.org/patch/10855923/#22569149
> >
> > Another possible use case is for parallel bus multiplexing, where multiple image
> > sensor share the parallel bus lines and they get activated alternatively through
> > an enable signal. While this might not be most clever design, it's often seen
> > in the wild, and this operation allow receivers to re-configure their bus
> > parameter in between streaming session.
>
> Isn't this already supported today, with the bus configuration for each
> source specified in the corresponding endpoint (on the receiver side) in
> DT ?
>

I think you're right.. One should probably cache each endpoint configuration
parsed at probe time.. I was thinking at multiple connections to the
same endpoint, but seems it's not possible.. good, I'll drop this


> > Hyun is now using this series to configure GMSL devices.
> > ------------------------------------------------------------------------------
> >
> > v1->v2:
> > - Address Sakari's comment to use unsigned int in place of bools
> > - Add two new patches to address documentation
> > - Adjust rcar-csi2 patch as much as possible according to Niklas comments
> > - Add Niklas's tags
> >
> > Jacopo Mondi (6):
> >   media: v4l2-subdv: Introduce get_mbus_config pad op
> >   media: v4l2-subdev: Deprecate g_mbus_config video op
> >   media: v4l2-subdev: Expand get_mbus_config doc
> >   media: i2c: adv748x: Adjust TXA data lanes number
> >   media: i2c: adv748x: Implement get_mbus_config
> >   media: rcar-csi2: Negotiate data lanes number
> >
> >  drivers/media/i2c/adv748x/adv748x-core.c    | 31 ++++++--
> >  drivers/media/i2c/adv748x/adv748x-csi2.c    | 15 ++++
> >  drivers/media/i2c/adv748x/adv748x.h         |  1 +
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 53 ++++++++++++-
> >  include/media/v4l2-subdev.h                 | 82 ++++++++++++++++++++-
> >  5 files changed, 171 insertions(+), 11 deletions(-)
> >
>
> --
> Regards,
>
> Laurent Pinchart