Re: [PATCH] tc358743: fix connected/active CSI-2 lane reporting

[Hans Verkuil]

On 09/21/17 14:35, Philipp Zabel wrote:
> Hi Ian,
> 
> On Thu, 2017-09-21 at 12:06 +0100, Ian Arkver wrote:
> [...]
>>> My understanding of Hans' comment:
>>> "I'd also add a comment that all other flags must be 0 if the device 
>>> tree is used. This to avoid mixing the two."
>>>
>>> is that all the above should only happen if (!!state->pdata).
>>
>> Except that state->pdata is a copy of the pdata, not a pointer, but you 
>> know what I mean. Some other check for DT needed here.
> 
> Yes, I'll change this to zero all V4L2_MBUS_CSI2_[1-4]_LANE in the DT
> case. I suppose the V4L2_MBUS_CSI2_CONTINUOUS_CLOCK and
> V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK bits should be zeroed as well, then?

Yes. Just zero all bits except those in the V4L2_MBUS_CSI2_LANE_MASK. And
changing that to (0xf << 10) makes sense.

Regards,

Hans

> 
>>> I don't know if this would break any existing DT-using bridge drivers.
> 
> The only current users of g_mbus_config are the pxa_camera and
> sh_mobile_ceu_camera soc_camera drivers. Neither supports MIPI CSI-2, as
> far as I can tell.
> 
> regards
> Philipp
> 

Re: [PATCH] tc358743: fix connected/active CSI-2 lane reporting

[Philipp Zabel]

Hi Ian,

On Thu, 2017-09-21 at 12:06 +0100, Ian Arkver wrote:
[...]
> > My understanding of Hans' comment:
> > "I'd also add a comment that all other flags must be 0 if the device 
> > tree is used. This to avoid mixing the two."
> > 
> > is that all the above should only happen if (!!state->pdata).
> 
> Except that state->pdata is a copy of the pdata, not a pointer, but you 
> know what I mean. Some other check for DT needed here.

Yes, I'll change this to zero all V4L2_MBUS_CSI2_[1-4]_LANE in the DT
case. I suppose the V4L2_MBUS_CSI2_CONTINUOUS_CLOCK and
V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK bits should be zeroed as well, then?

> > I don't know if this would break any existing DT-using bridge drivers.

The only current users of g_mbus_config are the pxa_camera and
sh_mobile_ceu_camera soc_camera drivers. Neither supports MIPI CSI-2, as
far as I can tell.

regards
Philipp

Re: [PATCH] tc358743: fix connected/active CSI-2 lane reporting

[Philipp Zabel]

On Thu, 2017-09-21 at 12:41 +0100, Dave Stevenson wrote:
> Hi Philipp
> 
> On 21 September 2017 at 11:24, Philipp Zabel 
> wrote:
[...]
> > +   if (state->csi_lanes_in_use > 4)
> 
> One could suggest
> if (state->csi_lanes_in_use > state->bus.num_data_lanes)
> here. Needing to use more lanes than are configured is surely an
> error, although that may be detectable at the other end. See below
> too.

True. The OF parser could be improved to make sure that
num_data_lanes <= 4, and also that all lanes are in the correct order.

[...]
> > +/*
> > + * Number of lanes in use, 0 == use all available lanes (default)
> > + *
> > + * This is a temporary fix for devices that need to reduce the number of 
> > active
> > + * lanes for certain modes, until g_mbus_config() can be replaced with a 
> > better
> > + * solution.
> > + */
> > +#define V4L2_MBUS_CSI2_LANE_MASK(3 << 10)
> 
> I know this was Hans' suggested define, but are we saying 4 lanes is
> not a valid value? If it is then the mask needs to be at least (7 <<
> 10).

0 must map to "all lanes" for backwards compatibility, but I see no
reason why we shouldn't add another bit here to support reporting 4
lanes explicitly.

> 4 lanes is not necessarily "all available lanes".

Correct.

> - There are now CSI2 devices supporting up to 8 lanes (although
> V4L2_FWNODE_CSI2_MAX_DATA_LANES limits you to 4 at the moment).

So how about we just add two bits, then?

#define V4L2_MBUS_CSI2_LANE_MASK(0xf << 10)

> - Or you could have 2 lanes configured in DT and ask TC358743 for (eg)
> 1080P60 UYVY at 594Mbps (needs 4 lanes) which passes the current logic
> in the TC358743 driver and would return 0, when it is actually going
> to use 4 lanes. That could be classed as a driver bug though.

Right, the driver shouldn't try to start streaming at all if it knows
that the available CSI-2 bandwidth will be exceeded.

> My view is that if a driver is going to report how many lanes to use
> then it should always report it explicitly. The default 0 value should
> only be used for devices that will never change it from the DT
> settings. Perhaps others disagree
> 
> Otherwise the patch works for me.

I'm fine with changing this as you suggest.

regards
Philipp

Re: [PATCH] tc358743: fix connected/active CSI-2 lane reporting

[Dave Stevenson]

Hi Philipp

On 21 September 2017 at 11:24, Philipp Zabel  wrote:
> g_mbus_config was supposed to indicate all supported lane numbers, not
> only the number of those currently in active use. Since the tc358743
> can dynamically reduce the number of active lanes if the required
> bandwidth allows for it, report all lane numbers up to the connected
> number of lanes as supported.
> To allow communicating the number of currently active lanes, add a new
> bitfield to the v4l2_mbus_config flags. This is a temporary fix, until
> a better solution is found.
>
> Signed-off-by: Philipp Zabel 
> ---
>  drivers/media/i2c/tc358743.c  | 22 --
>  include/media/v4l2-mediabus.h |  8 
>  2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index e6f5c363ccab5..e2a9e6a18a49d 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -1464,21 +1464,22 @@ static int tc358743_g_mbus_config(struct v4l2_subdev 
> *sd,
> /* Support for non-continuous CSI-2 clock is missing in the driver */
> cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
>
> -   switch (state->csi_lanes_in_use) {
> -   case 1:
> +   if (state->bus.num_data_lanes > 0)
> cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
> -   break;
> -   case 2:
> +   if (state->bus.num_data_lanes > 1)
> cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
> -   break;
> -   case 3:
> +   if (state->bus.num_data_lanes > 2)
> cfg->flags |= V4L2_MBUS_CSI2_3_LANE;
> -   break;
> -   case 4:
> +   if (state->bus.num_data_lanes > 3)
> cfg->flags |= V4L2_MBUS_CSI2_4_LANE;
> -   break;
> -   default:
> +
> +   if (state->csi_lanes_in_use > 4)

One could suggest
if (state->csi_lanes_in_use > state->bus.num_data_lanes)
here. Needing to use more lanes than are configured is surely an
error, although that may be detectable at the other end. See below
too.

> return -EINVAL;
> +
> +   if (state->csi_lanes_in_use < state->bus.num_data_lanes) {
> +   const u32 mask = V4L2_MBUS_CSI2_LANE_MASK;
> +
> +   cfg->flags |= (state->csi_lanes_in_use << __ffs(mask)) & mask;
> }
>
> return 0;
> @@ -1885,6 +1886,7 @@ static int tc358743_probe(struct i2c_client *client,
> if (pdata) {
> state->pdata = *pdata;
> state->bus.flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
> +   state->bus.num_data_lanes = 4;
> } else {
> err = tc358743_probe_of(state);
> if (err == -ENODEV)
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 93f8afcb7a220..3467d97be5f5b 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -63,6 +63,14 @@
>  V4L2_MBUS_CSI2_3_LANE | 
> V4L2_MBUS_CSI2_4_LANE)
>  #define V4L2_MBUS_CSI2_CHANNELS(V4L2_MBUS_CSI2_CHANNEL_0 | 
> V4L2_MBUS_CSI2_CHANNEL_1 | \
>  V4L2_MBUS_CSI2_CHANNEL_2 | 
> V4L2_MBUS_CSI2_CHANNEL_3)
> +/*
> + * Number of lanes in use, 0 == use all available lanes (default)
> + *
> + * This is a temporary fix for devices that need to reduce the number of 
> active
> + * lanes for certain modes, until g_mbus_config() can be replaced with a 
> better
> + * solution.
> + */
> +#define V4L2_MBUS_CSI2_LANE_MASK(3 << 10)

I know this was Hans' suggested define, but are we saying 4 lanes is
not a valid value? If it is then the mask needs to be at least (7 <<
10).

4 lanes is not necessarily "all available lanes".
- There are now CSI2 devices supporting up to 8 lanes (although
V4L2_FWNODE_CSI2_MAX_DATA_LANES limits you to 4 at the moment).
- Or you could have 2 lanes configured in DT and ask TC358743 for (eg)
1080P60 UYVY at 594Mbps (needs 4 lanes) which passes the current logic
in the TC358743 driver and would return 0, when it is actually going
to use 4 lanes. That could be classed as a driver bug though.

My view is that if a driver is going to report how many lanes to use
then it should always report it explicitly. The default 0 value should
only be used for devices that will never change it from the DT
settings. Perhaps others disagree

Otherwise the patch works for me.

  Dave.

>  /**
>   * enum v4l2_mbus_type - media bus type
> --
> 2.11.0
>

Re: [PATCH] tc358743: fix connected/active CSI-2 lane reporting

[Ian Arkver]

On 21/09/17 12:04, Ian Arkver wrote:

Hi Philipp

On 21/09/17 11:24, Philipp Zabel wrote:

g_mbus_config was supposed to indicate all supported lane numbers, not
only the number of those currently in active use. Since the tc358743
can dynamically reduce the number of active lanes if the required
bandwidth allows for it, report all lane numbers up to the connected
number of lanes as supported.
To allow communicating the number of currently active lanes, add a new
bitfield to the v4l2_mbus_config flags. This is a temporary fix, until
a better solution is found.

Signed-off-by: Philipp Zabel 
---
  drivers/media/i2c/tc358743.c  | 22 --
  include/media/v4l2-mediabus.h |  8 
  2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index e6f5c363ccab5..e2a9e6a18a49d 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1464,21 +1464,22 @@ static int tc358743_g_mbus_config(struct 
v4l2_subdev *sd,
  /* Support for non-continuous CSI-2 clock is missing in the 
driver */

  cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
-    switch (state->csi_lanes_in_use) {
-    case 1:
+    if (state->bus.num_data_lanes > 0)
  cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
-    break;
-    case 2:
+    if (state->bus.num_data_lanes > 1)
  cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
-    break;
-    case 3:
+    if (state->bus.num_data_lanes > 2)
  cfg->flags |= V4L2_MBUS_CSI2_3_LANE;
-    break;
-    case 4:
+    if (state->bus.num_data_lanes > 3)
  cfg->flags |= V4L2_MBUS_CSI2_4_LANE;
-    break;
-    default:
+
+    if (state->csi_lanes_in_use > 4)
  return -EINVAL;
+


My understanding of Hans' comment:
"I'd also add a comment that all other flags must be 0 if the device 
tree is used. This to avoid mixing the two."


is that all the above should only happen if (!!state->pdata).


Except that state->pdata is a copy of the pdata, not a pointer, but you 
know what I mean. Some other check for DT needed here.



I don't know if this would break any existing DT-using bridge drivers.

Regards,
Ian

[snip]

Re: [PATCH] tc358743: fix connected/active CSI-2 lane reporting

[Ian Arkver]

Hi Philipp

On 21/09/17 11:24, Philipp Zabel wrote:

g_mbus_config was supposed to indicate all supported lane numbers, not
only the number of those currently in active use. Since the tc358743
can dynamically reduce the number of active lanes if the required
bandwidth allows for it, report all lane numbers up to the connected
number of lanes as supported.
To allow communicating the number of currently active lanes, add a new
bitfield to the v4l2_mbus_config flags. This is a temporary fix, until
a better solution is found.

Signed-off-by: Philipp Zabel 
---
  drivers/media/i2c/tc358743.c  | 22 --
  include/media/v4l2-mediabus.h |  8 
  2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index e6f5c363ccab5..e2a9e6a18a49d 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1464,21 +1464,22 @@ static int tc358743_g_mbus_config(struct v4l2_subdev 
*sd,
/* Support for non-continuous CSI-2 clock is missing in the driver */
cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
  
-	switch (state->csi_lanes_in_use) {

-   case 1:
+   if (state->bus.num_data_lanes > 0)
cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
-   break;
-   case 2:
+   if (state->bus.num_data_lanes > 1)
cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
-   break;
-   case 3:
+   if (state->bus.num_data_lanes > 2)
cfg->flags |= V4L2_MBUS_CSI2_3_LANE;
-   break;
-   case 4:
+   if (state->bus.num_data_lanes > 3)
cfg->flags |= V4L2_MBUS_CSI2_4_LANE;
-   break;
-   default:
+
+   if (state->csi_lanes_in_use > 4)
return -EINVAL;
+


My understanding of Hans' comment:
"I'd also add a comment that all other flags must be 0 if the device 
tree is used. This to avoid mixing the two."


is that all the above should only happen if (!!state->pdata).

I don't know if this would break any existing DT-using bridge drivers.

Regards,
Ian


+   if (state->csi_lanes_in_use < state->bus.num_data_lanes) {
+   const u32 mask = V4L2_MBUS_CSI2_LANE_MASK;
+
+   cfg->flags |= (state->csi_lanes_in_use << __ffs(mask)) & mask;
}
  
  	return 0;

@@ -1885,6 +1886,7 @@ static int tc358743_probe(struct i2c_client *client,
if (pdata) {
state->pdata = *pdata;
state->bus.flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+   state->bus.num_data_lanes = 4;
} else {
err = tc358743_probe_of(state);
if (err == -ENODEV)
diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 93f8afcb7a220..3467d97be5f5b 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -63,6 +63,14 @@
 V4L2_MBUS_CSI2_3_LANE | 
V4L2_MBUS_CSI2_4_LANE)
  #define V4L2_MBUS_CSI2_CHANNELS   (V4L2_MBUS_CSI2_CHANNEL_0 | 
V4L2_MBUS_CSI2_CHANNEL_1 | \
 V4L2_MBUS_CSI2_CHANNEL_2 | 
V4L2_MBUS_CSI2_CHANNEL_3)
+/*
+ * Number of lanes in use, 0 == use all available lanes (default)
+ *
+ * This is a temporary fix for devices that need to reduce the number of active
+ * lanes for certain modes, until g_mbus_config() can be replaced with a better
+ * solution.
+ */
+#define V4L2_MBUS_CSI2_LANE_MASK(3 << 10)
  
  /**

   * enum v4l2_mbus_type - media bus type