[PATCH 0/3] [media] tc358743: Support for a wider range of inputs

[PATCH 0/3] [media] tc358743: Support for a wider range of inputs

[Dave Stevenson]

Three minor changes to the TC358743 HDMI to CSI2 bridge chip driver.
- Correct the clock mode reported via g_mbus_config to match that set.
- Increase the FIFO level to allow resolutions lower than 720P60 to work.
- Add settings for a new link frequency of 972Mbit/s. This allows
  1080P50 UYVY to work on two lanes (useful on the Raspberry Pi which
  only brings out two CSI2 lanes to the camera connector).

I'd like to extend the last one to dynamically calculate all the values
for an arbitrary link speed, but time hasn't allowed as yet.

Dave Stevenson (3):
  [media] tc358743: Correct clock mode reported in g_mbus_config
  [media] tc358743: Increase FIFO level to 300.
  [media] tc358743: Add support for 972Mbit/s link freq.

 drivers/media/i2c/tc358743.c | 62 +++++++++++++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH 1/3] [media] tc358743: Correct clock mode reported in g_mbus_config

[Dave Stevenson]

Support for non-continuous clock had previously been added via
device tree, but a comment and the value reported by g_mbus_config
still stated that it wasn't supported.
Remove the comment, and return the correct value in g_mbus_config.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index e6f5c36..6b0fd07 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1461,8 +1461,9 @@ static int tc358743_g_mbus_config(struct v4l2_subdev *sd,
 
 	cfg->type = V4L2_MBUS_CSI2;
 
-	/* Support for non-continuous CSI-2 clock is missing in the driver */
-	cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+	cfg->flags = state->bus.flags &
+			(V4L2_MBUS_CSI2_CONTINUOUS_CLOCK |
+			 V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK);
 
 	switch (state->csi_lanes_in_use) {
 	case 1:
-- 
2.7.4

[PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Dave Stevenson]

The existing fixed value of 16 worked for UYVY 720P60 over
2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
1080P60 needs 6 lanes at 594MHz).
It doesn't allow for lower resolutions to work as the FIFO
underflows.

Using a value of 300 works for all resolutions down to VGA60,
and the increase in frame delay is <4usecs for 1080P60 UYVY
(2.55usecs for RGB888).

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 6b0fd07..7632daf 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1782,8 +1782,14 @@ static int tc358743_probe_of(struct tc358743_state *state)
 	state->pdata.refclk_hz = clk_get_rate(refclk);
 	state->pdata.ddc5v_delay = DDC5V_DELAY_100_MS;
 	state->pdata.enable_hdcp = false;
-	/* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz. */
-	state->pdata.fifo_level = 16;
+	/*
+	 * A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz,
+	 * but is insufficient for lower resolutions.
+	 * A value of 300 allows for resolutions down to VGA60 (and possibly
+	 * lower) to work, whilst still leaving the delay for 1080P60
+	 * stilll below 4usecs.
+	 */
+	state->pdata.fifo_level = 300;
 	/*
 	 * The PLL input clock is obtained by dividing refclk by pll_prd.
 	 * It must be between 6 MHz and 40 MHz, lower frequency is better.
-- 
2.7.4

[PATCH 3/3] [media] tc358743: Add support for 972Mbit/s link freq.

[Dave Stevenson]

Adds register setups for running the CSI lanes at 972Mbit/s,
which allows 1080P50 UYVY down 2 lanes.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 47 +++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 7632daf..dcc100e 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1809,6 +1809,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
 	/*
 	 * The CSI bps per lane must be between 62.5 Mbps and 1 Gbps.
 	 * The default is 594 Mbps for 4-lane 1080p60 or 2-lane 720p60.
+	 * 972 Mbps allows 1080P50 UYVY over 2-lane.
 	 */
 	bps_pr_lane = 2 * endpoint->link_frequencies[0];
 	if (bps_pr_lane < 62500000U || bps_pr_lane > 1000000000U) {
@@ -1821,23 +1822,41 @@ static int tc358743_probe_of(struct tc358743_state *state)
 			       state->pdata.refclk_hz * state->pdata.pll_prd;
 
 	/*
-	 * FIXME: These timings are from REF_02 for 594 Mbps per lane (297 MHz
-	 * link frequency). In principle it should be possible to calculate
+	 * FIXME: These timings are from REF_02 for 594 or 972 Mbps per lane
+	 * (297 MHz or 495 MHz link frequency).
+	 * In principle it should be possible to calculate
 	 * them based on link frequency and resolution.
 	 */
-	if (bps_pr_lane != 594000000U)
+	switch (bps_pr_lane) {
+	default:
 		dev_warn(dev, "untested bps per lane: %u bps\n", bps_pr_lane);
-	state->pdata.lineinitcnt = 0xe80;
-	state->pdata.lptxtimecnt = 0x003;
-	/* tclk-preparecnt: 3, tclk-zerocnt: 20 */
-	state->pdata.tclk_headercnt = 0x1403;
-	state->pdata.tclk_trailcnt = 0x00;
-	/* ths-preparecnt: 3, ths-zerocnt: 1 */
-	state->pdata.ths_headercnt = 0x0103;
-	state->pdata.twakeup = 0x4882;
-	state->pdata.tclk_postcnt = 0x008;
-	state->pdata.ths_trailcnt = 0x2;
-	state->pdata.hstxvregcnt = 0;
+	case 594000000U:
+		state->pdata.lineinitcnt = 0xe80;
+		state->pdata.lptxtimecnt = 0x003;
+		/* tclk-preparecnt: 3, tclk-zerocnt: 20 */
+		state->pdata.tclk_headercnt = 0x1403;
+		state->pdata.tclk_trailcnt = 0x00;
+		/* ths-preparecnt: 3, ths-zerocnt: 1 */
+		state->pdata.ths_headercnt = 0x0103;
+		state->pdata.twakeup = 0x4882;
+		state->pdata.tclk_postcnt = 0x008;
+		state->pdata.ths_trailcnt = 0x2;
+		state->pdata.hstxvregcnt = 0;
+		break;
+	case 972000000U:
+		state->pdata.lineinitcnt = 0xFA0;
+		state->pdata.lptxtimecnt = 0x007;
+		/* tclk-preparecnt: 6, tclk-zerocnt: 40 */
+		state->pdata.tclk_headercnt = 0x2806;
+		state->pdata.tclk_trailcnt = 0x00;
+		/* ths-preparecnt: 3, ths-zerocnt: 1 */
+		state->pdata.ths_headercnt = 0x0806;
+		state->pdata.twakeup = 0x4268;
+		state->pdata.tclk_postcnt = 0x008;
+		state->pdata.ths_trailcnt = 0x5;
+		state->pdata.hstxvregcnt = 0;
+		break;
+	}
 
 	state->reset_gpio = devm_gpiod_get_optional(dev, "reset",
 						    GPIOD_OUT_LOW);
-- 
2.7.4

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Philipp Zabel]

Hi Dave,

On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
> The existing fixed value of 16 worked for UYVY 720P60 over
> 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
> 1080P60 needs 6 lanes at 594MHz).
> It doesn't allow for lower resolutions to work as the FIFO
> underflows.
> 
> Using a value of 300 works for all resolutions down to VGA60,
> and the increase in frame delay is <4usecs for 1080P60 UYVY
> (2.55usecs for RGB888).
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>

Can we increase this to 320? This would also allow
720p60 at 594 Mbps / 4 lanes, according to the xls.

regards
Philipp

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Mauro Carvalho Chehab]

Em Tue, 19 Sep 2017 17:24:45 +0200
Philipp Zabel <p.zabel@pengutronix.de> escreveu:

> Hi Dave,
> 
> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
> > The existing fixed value of 16 worked for UYVY 720P60 over
> > 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
> > 1080P60 needs 6 lanes at 594MHz).
> > It doesn't allow for lower resolutions to work as the FIFO
> > underflows.
> > 
> > Using a value of 300 works for all resolutions down to VGA60,
> > and the increase in frame delay is <4usecs for 1080P60 UYVY
> > (2.55usecs for RGB888).
> > 
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>  
> 
> Can we increase this to 320? This would also allow
> 720p60 at 594 Mbps / 4 lanes, according to the xls.

Hmm... if this is dependent on the resolution and frame rate, wouldn't
it be better to dynamically adjust it accordingly?

Regards,
Maur

Thanks,
Mauro

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Dave Stevenson]

Hi Mauro & Philipp

On 19 September 2017 at 17:49, Mauro Carvalho Chehab
<mchehab@s-opensource.com> wrote:
> Em Tue, 19 Sep 2017 17:24:45 +0200
> Philipp Zabel <p.zabel@pengutronix.de> escreveu:
>
>> Hi Dave,
>>
>> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
>> > The existing fixed value of 16 worked for UYVY 720P60 over
>> > 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
>> > 1080P60 needs 6 lanes at 594MHz).
>> > It doesn't allow for lower resolutions to work as the FIFO
>> > underflows.
>> >
>> > Using a value of 300 works for all resolutions down to VGA60,
>> > and the increase in frame delay is <4usecs for 1080P60 UYVY
>> > (2.55usecs for RGB888).
>> >
>> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>>
>> Can we increase this to 320? This would also allow
>> 720p60 at 594 Mbps / 4 lanes, according to the xls.

Unless I've missed something then the driver would currently request
only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
on a FIFO setting of 16.
How/why were you thinking we need to run all four lanes for 720p60
without other significant driver mods around lane config?

Once I've got a v3 done on the Unicam driver I'll bash through the
standard HDMI modes and check what value they need - I can see a big
spreadsheet coming on.
I'll ignore interlaced modes as I can't see any support for it in the
driver. Receiving the fields on different CSI-2 data types is
something I know the Unicam hardware won't handle nicely, and I
suspect it'd be an issue for many other platforms too.

> Hmm... if this is dependent on the resolution and frame rate, wouldn't
> it be better to dynamically adjust it accordingly?

It's setting up the FIFO matching the incoming HDMI data rate and
outgoing CSI rate. That means it's dependent on the incoming pixel
clock, blanking, colour format and resolution, and output CSI link
frequency, number of lanes, and colour format.
Whilst it could be set dynamically based on all those parameters, is
there a significant enough gain in doing so?

The value of 300 works for all cases I've tried, and referencing back
it is also the value that Hans said Cisco use via platform data on
their hardware [1]. Generally I'm seeing that values of 0-130 are
required, so 300 is giving a fair safety margin.

Second question is does anyone have a suitable relationship with
Toshiba to get permission to release details of these register
calculations? The datasheet and value spreadsheet are marked as
confidential, and probably under NDA in almost all cases. Whilst they
can't object to drivers containing values to make them work, they
might over releasing significant details.

Thanks,
  Dave

[1] https://www.spinics.net/lists/linux-media/msg116360.html

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Philipp Zabel]

Hi,

On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote:
> Hi Mauro & Philipp
> 
> On 19 September 2017 at 17:49, Mauro Carvalho Chehab
> <mchehab@s-opensource.com> wrote:
> > Em Tue, 19 Sep 2017 17:24:45 +0200
> > Philipp Zabel <p.zabel@pengutronix.de> escreveu:
> > 
> > > Hi Dave,
> > > 
> > > On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
> > > > The existing fixed value of 16 worked for UYVY 720P60 over
> > > > 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
> > > > 1080P60 needs 6 lanes at 594MHz).
> > > > It doesn't allow for lower resolutions to work as the FIFO
> > > > underflows.
> > > > 
> > > > Using a value of 300 works for all resolutions down to VGA60,
> > > > and the increase in frame delay is <4usecs for 1080P60 UYVY
> > > > (2.55usecs for RGB888).
> > > > 
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> > > 
> > > Can we increase this to 320? This would also allow
> > > 720p60 at 594 Mbps / 4 lanes, according to the xls.
> 
> Unless I've missed something then the driver would currently request
> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
> on a FIFO setting of 16.
> How/why were you thinking we need to run all four lanes for 720p60
> without other significant driver mods around lane config?

The driver currently silently changes the number of active lanes
depending on required data rate, with no way to communicate it to the
receiver.
The i.MX6 MIPI CSI-2 receiver driver can't cope with that, as it always
activates all four lanes that are configured in the device tree. I can
work around that with the following patch:

----------8<----------
Subject: [PATCH] [media] tc358743: do not dynamically reduce number of lanes

Dynamic lane number reduction does not work with receivers that
configure a fixed lane number according to the device tree settings.
To allow 720p60 at 594 Mbit/s on 4 lanes, increase the fifo_level
and tclk_trailcnt settings.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/i2c/tc358743.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 64f504542a819..70a9435928cdb 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -683,7 +683,7 @@ static void tc358743_set_csi(struct v4l2_subdev *sd)
 {
 	struct tc358743_state *state = to_state(sd);
 	struct tc358743_platform_data *pdata = &state->pdata;
-	unsigned lanes = tc358743_num_csi_lanes_needed(sd);
+	unsigned lanes = state->bus.num_data_lanes;
 
 	v4l2_dbg(3, debug, sd, "%s:\n", __func__);
 
@@ -1906,7 +1906,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
 	state->pdata.ddc5v_delay = DDC5V_DELAY_100_MS;
 	state->pdata.enable_hdcp = false;
 	/* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz. */
-	state->pdata.fifo_level = 16;
+	state->pdata.fifo_level = 320;
 	/*
 	 * The PLL input clock is obtained by dividing refclk by pll_prd.
 	 * It must be between 6 MHz and 40 MHz, lower frequency is better.
@@ -1948,7 +1948,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
 	state->pdata.lptxtimecnt = 0x003;
 	/* tclk-preparecnt: 3, tclk-zerocnt: 20 */
 	state->pdata.tclk_headercnt = 0x1403;
-	state->pdata.tclk_trailcnt = 0x00;
+	state->pdata.tclk_trailcnt = 0x01;
 	/* ths-preparecnt: 3, ths-zerocnt: 1 */
 	state->pdata.ths_headercnt = 0x0103;
 	state->pdata.twakeup = 0x4882;
-- 
2.11.0
---------->8----------

Just adding the same heuristic as tc358743_num_csi_lanes_needed in the
imx6-mipi-csi2 driver doesn't work, as the heuristic is specific to the
Toshiba chip. There are MIPI CSI-2 sensors that only support a fixed
number of lanes, for example.

I'd need a way to communicate the number of active MIPI CSI-2 lanes
between transmitting and receiving subdevice driver.

> Once I've got a v3 done on the Unicam driver I'll bash through the
> standard HDMI modes and check what value they need - I can see a big
> spreadsheet coming on.

Oh dear. Unless the point you make below can be resolved, I think we
have no other choice.

> I'll ignore interlaced modes as I can't see any support for it in the
> driver. Receiving the fields on different CSI-2 data types is
> something I know the Unicam hardware won't handle nicely, and I
> suspect it'd be an issue for many other platforms too.

Yes, let's pretend interlacing doesn't exist as long as possible.

> > Hmm... if this is dependent on the resolution and frame rate,
> > wouldn't
> > it be better to dynamically adjust it accordingly?
> 
> It's setting up the FIFO matching the incoming HDMI data rate and
> outgoing CSI rate. That means it's dependent on the incoming pixel
> clock, blanking, colour format and resolution, and output CSI link
> frequency, number of lanes, and colour format.
> Whilst it could be set dynamically based on all those parameters, is
> there a significant enough gain in doing so?

Ideally there would be no need to maintain a timing database with -
worst case - (number of link frequencies * number of HDMI modes) entries
in the driver ...

> The value of 300 works for all cases I've tried, and referencing back
> it is also the value that Hans said Cisco use via platform data on
> their hardware [1]. Generally I'm seeing that values of 0-130 are
> required, so 300 is giving a fair safety margin.

It does not work for 720p60 on 4 lanes at 594 Mbit/s, as the spreadsheet
warns, and testing shows.

> Second question is does anyone have a suitable relationship with
> Toshiba to get permission to release details of these register
> calculations? The datasheet and value spreadsheet are marked as
> confidential, and probably under NDA in almost all cases. Whilst they
> can't object to drivers containing values to make them work, they
> might over releasing significant details.

Unfortunately, I don't.

regards
Philipp

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Dave Stevenson]

On 20 September 2017 at 11:23, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi,
>
> On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote:
>> Hi Mauro & Philipp
>>
>> On 19 September 2017 at 17:49, Mauro Carvalho Chehab
>> <mchehab@s-opensource.com> wrote:
>> > Em Tue, 19 Sep 2017 17:24:45 +0200
>> > Philipp Zabel <p.zabel@pengutronix.de> escreveu:
>> >
>> > > Hi Dave,
>> > >
>> > > On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
>> > > > The existing fixed value of 16 worked for UYVY 720P60 over
>> > > > 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
>> > > > 1080P60 needs 6 lanes at 594MHz).
>> > > > It doesn't allow for lower resolutions to work as the FIFO
>> > > > underflows.
>> > > >
>> > > > Using a value of 300 works for all resolutions down to VGA60,
>> > > > and the increase in frame delay is <4usecs for 1080P60 UYVY
>> > > > (2.55usecs for RGB888).
>> > > >
>> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>> > >
>> > > Can we increase this to 320? This would also allow
>> > > 720p60 at 594 Mbps / 4 lanes, according to the xls.
>>
>> Unless I've missed something then the driver would currently request
>> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
>> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
>> on a FIFO setting of 16.
>> How/why were you thinking we need to run all four lanes for 720p60
>> without other significant driver mods around lane config?
>
> The driver currently silently changes the number of active lanes
> depending on required data rate, with no way to communicate it to the
> receiver.

It is communicated over the subdevice API - tc358743_g_mbus_config
reports back the appropriate number of lanes to the receiver
subdevice.
A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call
as you're starting streaming therefore gives you the correct
information. That's what I've just done for the BCM283x Unicam
driver[1], but admittedly I'm not using the media controller API which
i.MX6 is.

[1] http://www.spinics.net/lists/linux-media/msg121813.html, as part
of the unicam_start_streaming function.

> The i.MX6 MIPI CSI-2 receiver driver can't cope with that, as it always
> activates all four lanes that are configured in the device tree. I can
> work around that with the following patch:

It can't cope running at less than 4 lanes, or it can't cope with a change?

> ----------8<----------
> Subject: [PATCH] [media] tc358743: do not dynamically reduce number of lanes
>
> Dynamic lane number reduction does not work with receivers that
> configure a fixed lane number according to the device tree settings.
> To allow 720p60 at 594 Mbit/s on 4 lanes, increase the fifo_level
> and tclk_trailcnt settings.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/i2c/tc358743.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index 64f504542a819..70a9435928cdb 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -683,7 +683,7 @@ static void tc358743_set_csi(struct v4l2_subdev *sd)
>  {
>         struct tc358743_state *state = to_state(sd);
>         struct tc358743_platform_data *pdata = &state->pdata;
> -       unsigned lanes = tc358743_num_csi_lanes_needed(sd);
> +       unsigned lanes = state->bus.num_data_lanes;
>
>         v4l2_dbg(3, debug, sd, "%s:\n", __func__);
>
> @@ -1906,7 +1906,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
>         state->pdata.ddc5v_delay = DDC5V_DELAY_100_MS;
>         state->pdata.enable_hdcp = false;
>         /* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz. */
> -       state->pdata.fifo_level = 16;
> +       state->pdata.fifo_level = 320;
>         /*
>          * The PLL input clock is obtained by dividing refclk by pll_prd.
>          * It must be between 6 MHz and 40 MHz, lower frequency is better.
> @@ -1948,7 +1948,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
>         state->pdata.lptxtimecnt = 0x003;
>         /* tclk-preparecnt: 3, tclk-zerocnt: 20 */
>         state->pdata.tclk_headercnt = 0x1403;
> -       state->pdata.tclk_trailcnt = 0x00;
> +       state->pdata.tclk_trailcnt = 0x01;
>         /* ths-preparecnt: 3, ths-zerocnt: 1 */
>         state->pdata.ths_headercnt = 0x0103;
>         state->pdata.twakeup = 0x4882;
> --
> 2.11.0
> ---------->8----------
>
> Just adding the same heuristic as tc358743_num_csi_lanes_needed in the
> imx6-mipi-csi2 driver doesn't work, as the heuristic is specific to the
> Toshiba chip. There are MIPI CSI-2 sensors that only support a fixed
> number of lanes, for example.
>
> I'd need a way to communicate the number of active MIPI CSI-2 lanes
> between transmitting and receiving subdevice driver.
>
>> Once I've got a v3 done on the Unicam driver I'll bash through the
>> standard HDMI modes and check what value they need - I can see a big
>> spreadsheet coming on.
>
> Oh dear. Unless the point you make below can be resolved, I think we
> have no other choice.
>
>> I'll ignore interlaced modes as I can't see any support for it in the
>> driver. Receiving the fields on different CSI-2 data types is
>> something I know the Unicam hardware won't handle nicely, and I
>> suspect it'd be an issue for many other platforms too.
>
> Yes, let's pretend interlacing doesn't exist as long as possible.
>
>> > Hmm... if this is dependent on the resolution and frame rate,
>> > wouldn't
>> > it be better to dynamically adjust it accordingly?
>>
>> It's setting up the FIFO matching the incoming HDMI data rate and
>> outgoing CSI rate. That means it's dependent on the incoming pixel
>> clock, blanking, colour format and resolution, and output CSI link
>> frequency, number of lanes, and colour format.
>> Whilst it could be set dynamically based on all those parameters, is
>> there a significant enough gain in doing so?
>
> Ideally there would be no need to maintain a timing database with -
> worst case - (number of link frequencies * number of HDMI modes) entries
> in the driver ...
>
>> The value of 300 works for all cases I've tried, and referencing back
>> it is also the value that Hans said Cisco use via platform data on
>> their hardware [1]. Generally I'm seeing that values of 0-130 are
>> required, so 300 is giving a fair safety margin.
>
> It does not work for 720p60 on 4 lanes at 594 Mbit/s, as the spreadsheet
> warns, and testing shows.

If it doesn't work with 720p60, then I guess it has no hope with many
other resolutions.
It sounds like confirming whether g_mbus_config is a potential
solution for i.MX6 (sorry I'm not familiar enough with that code to do
my own quick search), but otherwise cranking it up to 320 is
reasonable, and I'll see what other numbers fall out of the
spreadsheet.

>> Second question is does anyone have a suitable relationship with
>> Toshiba to get permission to release details of these register
>> calculations? The datasheet and value spreadsheet are marked as
>> confidential, and probably under NDA in almost all cases. Whilst they
>> can't object to drivers containing values to make them work, they
>> might over releasing significant details.
>
> Unfortunately, I don't.

I'm guessing Cisco may be in the best position to try pushing that.
Certainly all our info is under NDA, and we don't really have a
significant working relationship with them.

  Dave

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Hans Verkuil]

On 09/20/17 13:00, Dave Stevenson wrote:
> On 20 September 2017 at 11:23, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> Hi,
>>
>> On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote:
>>> Hi Mauro & Philipp
>>>
>>> On 19 September 2017 at 17:49, Mauro Carvalho Chehab
>>> <mchehab@s-opensource.com> wrote:
>>>> Em Tue, 19 Sep 2017 17:24:45 +0200
>>>> Philipp Zabel <p.zabel@pengutronix.de> escreveu:
>>>>
>>>>> Hi Dave,
>>>>>
>>>>> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
>>>>>> The existing fixed value of 16 worked for UYVY 720P60 over
>>>>>> 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
>>>>>> 1080P60 needs 6 lanes at 594MHz).
>>>>>> It doesn't allow for lower resolutions to work as the FIFO
>>>>>> underflows.
>>>>>>
>>>>>> Using a value of 300 works for all resolutions down to VGA60,
>>>>>> and the increase in frame delay is <4usecs for 1080P60 UYVY
>>>>>> (2.55usecs for RGB888).
>>>>>>
>>>>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>>>>>
>>>>> Can we increase this to 320? This would also allow
>>>>> 720p60 at 594 Mbps / 4 lanes, according to the xls.
>>>
>>> Unless I've missed something then the driver would currently request
>>> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
>>> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
>>> on a FIFO setting of 16.
>>> How/why were you thinking we need to run all four lanes for 720p60
>>> without other significant driver mods around lane config?
>>
>> The driver currently silently changes the number of active lanes
>> depending on required data rate, with no way to communicate it to the
>> receiver.
> 
> It is communicated over the subdevice API - tc358743_g_mbus_config
> reports back the appropriate number of lanes to the receiver
> subdevice.
> A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call
> as you're starting streaming therefore gives you the correct
> information. That's what I've just done for the BCM283x Unicam
> driver[1], but admittedly I'm not using the media controller API which
> i.MX6 is.

Shouldn't this information come from the device tree? The g_mbus_config
op is close to being deprecated or even removed. There are currently only
two obscure V4L2 bridge drivers that call it. It dates from pre-DT times
I rather not see it used in a new bridge driver.

The problem is that contains data that belongs to the DT (hardware
capabilities). Things that can actually change dynamically should be
communicated via another op. We don't have that, so that should be created.

I've CC-ed Sakari, he is the specialist for such things.

> 
> [1] http://www.spinics.net/lists/linux-media/msg121813.html, as part
> of the unicam_start_streaming function.
> 
>> The i.MX6 MIPI CSI-2 receiver driver can't cope with that, as it always
>> activates all four lanes that are configured in the device tree. I can
>> work around that with the following patch:
> 
> It can't cope running at less than 4 lanes, or it can't cope with a change?
> 
>> ----------8<----------
>> Subject: [PATCH] [media] tc358743: do not dynamically reduce number of lanes
>>
>> Dynamic lane number reduction does not work with receivers that
>> configure a fixed lane number according to the device tree settings.
>> To allow 720p60 at 594 Mbit/s on 4 lanes, increase the fifo_level
>> and tclk_trailcnt settings.
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> ---
>>  drivers/media/i2c/tc358743.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
>> index 64f504542a819..70a9435928cdb 100644
>> --- a/drivers/media/i2c/tc358743.c
>> +++ b/drivers/media/i2c/tc358743.c
>> @@ -683,7 +683,7 @@ static void tc358743_set_csi(struct v4l2_subdev *sd)
>>  {
>>         struct tc358743_state *state = to_state(sd);
>>         struct tc358743_platform_data *pdata = &state->pdata;
>> -       unsigned lanes = tc358743_num_csi_lanes_needed(sd);
>> +       unsigned lanes = state->bus.num_data_lanes;
>>
>>         v4l2_dbg(3, debug, sd, "%s:\n", __func__);
>>
>> @@ -1906,7 +1906,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
>>         state->pdata.ddc5v_delay = DDC5V_DELAY_100_MS;
>>         state->pdata.enable_hdcp = false;
>>         /* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz. */
>> -       state->pdata.fifo_level = 16;
>> +       state->pdata.fifo_level = 320;
>>         /*
>>          * The PLL input clock is obtained by dividing refclk by pll_prd.
>>          * It must be between 6 MHz and 40 MHz, lower frequency is better.
>> @@ -1948,7 +1948,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
>>         state->pdata.lptxtimecnt = 0x003;
>>         /* tclk-preparecnt: 3, tclk-zerocnt: 20 */
>>         state->pdata.tclk_headercnt = 0x1403;
>> -       state->pdata.tclk_trailcnt = 0x00;
>> +       state->pdata.tclk_trailcnt = 0x01;
>>         /* ths-preparecnt: 3, ths-zerocnt: 1 */
>>         state->pdata.ths_headercnt = 0x0103;
>>         state->pdata.twakeup = 0x4882;
>> --
>> 2.11.0
>> ---------->8----------
>>
>> Just adding the same heuristic as tc358743_num_csi_lanes_needed in the
>> imx6-mipi-csi2 driver doesn't work, as the heuristic is specific to the
>> Toshiba chip. There are MIPI CSI-2 sensors that only support a fixed
>> number of lanes, for example.
>>
>> I'd need a way to communicate the number of active MIPI CSI-2 lanes
>> between transmitting and receiving subdevice driver.
>>
>>> Once I've got a v3 done on the Unicam driver I'll bash through the
>>> standard HDMI modes and check what value they need - I can see a big
>>> spreadsheet coming on.
>>
>> Oh dear. Unless the point you make below can be resolved, I think we
>> have no other choice.
>>
>>> I'll ignore interlaced modes as I can't see any support for it in the
>>> driver. Receiving the fields on different CSI-2 data types is
>>> something I know the Unicam hardware won't handle nicely, and I
>>> suspect it'd be an issue for many other platforms too.
>>
>> Yes, let's pretend interlacing doesn't exist as long as possible.
>>
>>>> Hmm... if this is dependent on the resolution and frame rate,
>>>> wouldn't
>>>> it be better to dynamically adjust it accordingly?
>>>
>>> It's setting up the FIFO matching the incoming HDMI data rate and
>>> outgoing CSI rate. That means it's dependent on the incoming pixel
>>> clock, blanking, colour format and resolution, and output CSI link
>>> frequency, number of lanes, and colour format.
>>> Whilst it could be set dynamically based on all those parameters, is
>>> there a significant enough gain in doing so?
>>
>> Ideally there would be no need to maintain a timing database with -
>> worst case - (number of link frequencies * number of HDMI modes) entries
>> in the driver ...
>>
>>> The value of 300 works for all cases I've tried, and referencing back
>>> it is also the value that Hans said Cisco use via platform data on
>>> their hardware [1]. Generally I'm seeing that values of 0-130 are
>>> required, so 300 is giving a fair safety margin.
>>
>> It does not work for 720p60 on 4 lanes at 594 Mbit/s, as the spreadsheet
>> warns, and testing shows.
> 
> If it doesn't work with 720p60, then I guess it has no hope with many
> other resolutions.
> It sounds like confirming whether g_mbus_config is a potential
> solution for i.MX6 (sorry I'm not familiar enough with that code to do
> my own quick search), but otherwise cranking it up to 320 is
> reasonable, and I'll see what other numbers fall out of the
> spreadsheet.
> 
>>> Second question is does anyone have a suitable relationship with
>>> Toshiba to get permission to release details of these register
>>> calculations? The datasheet and value spreadsheet are marked as
>>> confidential, and probably under NDA in almost all cases. Whilst they
>>> can't object to drivers containing values to make them work, they
>>> might over releasing significant details.
>>
>> Unfortunately, I don't.
> 
> I'm guessing Cisco may be in the best position to try pushing that.
> Certainly all our info is under NDA, and we don't really have a
> significant working relationship with them.

I think we tried at the time, but no luck.

Regards,

	Hans

> 
>   Dave
> 

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Dave Stevenson]

On 20 September 2017 at 12:24, Hans Verkuil <hansverk@cisco.com> wrote:
> On 09/20/17 13:00, Dave Stevenson wrote:
>> On 20 September 2017 at 11:23, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>>> Hi,
>>>
>>> On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote:
>>>> Hi Mauro & Philipp
>>>>
>>>> On 19 September 2017 at 17:49, Mauro Carvalho Chehab
>>>> <mchehab@s-opensource.com> wrote:
>>>>> Em Tue, 19 Sep 2017 17:24:45 +0200
>>>>> Philipp Zabel <p.zabel@pengutronix.de> escreveu:
>>>>>
>>>>>> Hi Dave,
>>>>>>
>>>>>> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
>>>>>>> The existing fixed value of 16 worked for UYVY 720P60 over
>>>>>>> 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
>>>>>>> 1080P60 needs 6 lanes at 594MHz).
>>>>>>> It doesn't allow for lower resolutions to work as the FIFO
>>>>>>> underflows.
>>>>>>>
>>>>>>> Using a value of 300 works for all resolutions down to VGA60,
>>>>>>> and the increase in frame delay is <4usecs for 1080P60 UYVY
>>>>>>> (2.55usecs for RGB888).
>>>>>>>
>>>>>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>>>>>>
>>>>>> Can we increase this to 320? This would also allow
>>>>>> 720p60 at 594 Mbps / 4 lanes, according to the xls.
>>>>
>>>> Unless I've missed something then the driver would currently request
>>>> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
>>>> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
>>>> on a FIFO setting of 16.
>>>> How/why were you thinking we need to run all four lanes for 720p60
>>>> without other significant driver mods around lane config?
>>>
>>> The driver currently silently changes the number of active lanes
>>> depending on required data rate, with no way to communicate it to the
>>> receiver.
>>
>> It is communicated over the subdevice API - tc358743_g_mbus_config
>> reports back the appropriate number of lanes to the receiver
>> subdevice.
>> A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call
>> as you're starting streaming therefore gives you the correct
>> information. That's what I've just done for the BCM283x Unicam
>> driver[1], but admittedly I'm not using the media controller API which
>> i.MX6 is.
>
> Shouldn't this information come from the device tree? The g_mbus_config
> op is close to being deprecated or even removed. There are currently only
> two obscure V4L2 bridge drivers that call it. It dates from pre-DT times
> I rather not see it used in a new bridge driver.
>
> The problem is that contains data that belongs to the DT (hardware
> capabilities). Things that can actually change dynamically should be
> communicated via another op. We don't have that, so that should be created.
>
> I've CC-ed Sakari, he is the specialist for such things.

You've reminded me that I asked that same question earlier in the
year, and Sakari had replied -
http://www.spinics.net/lists/linux-media/msg115550.html

Is it specifically device tree related? Just because the lanes are
physically there doesn't necessarily mean they have to be used.

A quick test with the spreadsheet appears to say that 1080p24 UYVY
over 4 lanes at 594Mbps needs a FIFO setting >=480 (the max is 511). I
would anticipate that to be one of the worst situations as we're
dealing with a FIFO underflow herewhen there is a significantly faster
CSI rate than HDMI.
It can't be supported with a 972Mbps link frequency over 4 lanes
(needs >=667), and 2 lanes needs a FIFO setting >=374.

I'll see what numbers fall out of the new spreadsheet for all standard
modes. If there are some modes that can't be supported over 4 lanes
then there is an absolute requirement for communicating the number of
lanes to use.

Seeing as Cisco have kit shipping with this chip and driver, can I ask
how they are managing the choice over number of lanes in use?


As for this patch it sounds like we need to crank the FIFO setting up
to the maximum of 511, and potentially a second patch that removes
g_mbus_config and only reads DT if that is the desired behaviour.

>>
>> [1] http://www.spinics.net/lists/linux-media/msg121813.html, as part
>> of the unicam_start_streaming function.
>>
>>> The i.MX6 MIPI CSI-2 receiver driver can't cope with that, as it always
>>> activates all four lanes that are configured in the device tree. I can
>>> work around that with the following patch:
>>
>> It can't cope running at less than 4 lanes, or it can't cope with a change?
>>
>>> ----------8<----------
>>> Subject: [PATCH] [media] tc358743: do not dynamically reduce number of lanes
>>>
>>> Dynamic lane number reduction does not work with receivers that
>>> configure a fixed lane number according to the device tree settings.
>>> To allow 720p60 at 594 Mbit/s on 4 lanes, increase the fifo_level
>>> and tclk_trailcnt settings.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>  drivers/media/i2c/tc358743.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
>>> index 64f504542a819..70a9435928cdb 100644
>>> --- a/drivers/media/i2c/tc358743.c
>>> +++ b/drivers/media/i2c/tc358743.c
>>> @@ -683,7 +683,7 @@ static void tc358743_set_csi(struct v4l2_subdev *sd)
>>>  {
>>>         struct tc358743_state *state = to_state(sd);
>>>         struct tc358743_platform_data *pdata = &state->pdata;
>>> -       unsigned lanes = tc358743_num_csi_lanes_needed(sd);
>>> +       unsigned lanes = state->bus.num_data_lanes;
>>>
>>>         v4l2_dbg(3, debug, sd, "%s:\n", __func__);
>>>
>>> @@ -1906,7 +1906,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
>>>         state->pdata.ddc5v_delay = DDC5V_DELAY_100_MS;
>>>         state->pdata.enable_hdcp = false;
>>>         /* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz. */
>>> -       state->pdata.fifo_level = 16;
>>> +       state->pdata.fifo_level = 320;
>>>         /*
>>>          * The PLL input clock is obtained by dividing refclk by pll_prd.
>>>          * It must be between 6 MHz and 40 MHz, lower frequency is better.
>>> @@ -1948,7 +1948,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
>>>         state->pdata.lptxtimecnt = 0x003;
>>>         /* tclk-preparecnt: 3, tclk-zerocnt: 20 */
>>>         state->pdata.tclk_headercnt = 0x1403;
>>> -       state->pdata.tclk_trailcnt = 0x00;
>>> +       state->pdata.tclk_trailcnt = 0x01;
>>>         /* ths-preparecnt: 3, ths-zerocnt: 1 */
>>>         state->pdata.ths_headercnt = 0x0103;
>>>         state->pdata.twakeup = 0x4882;
>>> --
>>> 2.11.0
>>> ---------->8----------
>>>
>>> Just adding the same heuristic as tc358743_num_csi_lanes_needed in the
>>> imx6-mipi-csi2 driver doesn't work, as the heuristic is specific to the
>>> Toshiba chip. There are MIPI CSI-2 sensors that only support a fixed
>>> number of lanes, for example.
>>>
>>> I'd need a way to communicate the number of active MIPI CSI-2 lanes
>>> between transmitting and receiving subdevice driver.
>>>
>>>> Once I've got a v3 done on the Unicam driver I'll bash through the
>>>> standard HDMI modes and check what value they need - I can see a big
>>>> spreadsheet coming on.
>>>
>>> Oh dear. Unless the point you make below can be resolved, I think we
>>> have no other choice.
>>>
>>>> I'll ignore interlaced modes as I can't see any support for it in the
>>>> driver. Receiving the fields on different CSI-2 data types is
>>>> something I know the Unicam hardware won't handle nicely, and I
>>>> suspect it'd be an issue for many other platforms too.
>>>
>>> Yes, let's pretend interlacing doesn't exist as long as possible.
>>>
>>>>> Hmm... if this is dependent on the resolution and frame rate,
>>>>> wouldn't
>>>>> it be better to dynamically adjust it accordingly?
>>>>
>>>> It's setting up the FIFO matching the incoming HDMI data rate and
>>>> outgoing CSI rate. That means it's dependent on the incoming pixel
>>>> clock, blanking, colour format and resolution, and output CSI link
>>>> frequency, number of lanes, and colour format.
>>>> Whilst it could be set dynamically based on all those parameters, is
>>>> there a significant enough gain in doing so?
>>>
>>> Ideally there would be no need to maintain a timing database with -
>>> worst case - (number of link frequencies * number of HDMI modes) entries
>>> in the driver ...
>>>
>>>> The value of 300 works for all cases I've tried, and referencing back
>>>> it is also the value that Hans said Cisco use via platform data on
>>>> their hardware [1]. Generally I'm seeing that values of 0-130 are
>>>> required, so 300 is giving a fair safety margin.
>>>
>>> It does not work for 720p60 on 4 lanes at 594 Mbit/s, as the spreadsheet
>>> warns, and testing shows.
>>
>> If it doesn't work with 720p60, then I guess it has no hope with many
>> other resolutions.
>> It sounds like confirming whether g_mbus_config is a potential
>> solution for i.MX6 (sorry I'm not familiar enough with that code to do
>> my own quick search), but otherwise cranking it up to 320 is
>> reasonable, and I'll see what other numbers fall out of the
>> spreadsheet.
>>
>>>> Second question is does anyone have a suitable relationship with
>>>> Toshiba to get permission to release details of these register
>>>> calculations? The datasheet and value spreadsheet are marked as
>>>> confidential, and probably under NDA in almost all cases. Whilst they
>>>> can't object to drivers containing values to make them work, they
>>>> might over releasing significant details.
>>>
>>> Unfortunately, I don't.
>>
>> I'm guessing Cisco may be in the best position to try pushing that.
>> Certainly all our info is under NDA, and we don't really have a
>> significant working relationship with them.
>
> I think we tried at the time, but no luck.

Thanks.
So unless someone else has a relationship with Toshiba then it has to
be some fixed value - no real option for calculating it dynamically.
(That also means my intent to support arbitrary link frequencies won't
fly as that would require publishing details of how to compute
lineinintcnt, lptxtimecnt, etc. Oh well.)

  Dave

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Philipp Zabel]

On Wed, 2017-09-20 at 13:24 +0200, Hans Verkuil wrote:
> On 09/20/17 13:00, Dave Stevenson wrote:
[...]
> > It is communicated over the subdevice API - tc358743_g_mbus_config
> > reports back the appropriate number of lanes to the receiver
> > subdevice.
> > A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call
> > as you're starting streaming therefore gives you the correct
> > information. That's what I've just done for the BCM283x Unicam
> > driver[1], but admittedly I'm not using the media controller API which
> > i.MX6 is.
> 
> Shouldn't this information come from the device tree? The g_mbus_config
> op is close to being deprecated or even removed. There are currently only
> two obscure V4L2 bridge drivers that call it. It dates from pre-DT times
> I rather not see it used in a new bridge driver.
> 
> The problem is that contains data that belongs to the DT (hardware
> capabilities). Things that can actually change dynamically should be
> communicated via another op. We don't have that, so that should be
> created.
> 
> I've CC-ed Sakari, he is the specialist for such things.

The total number of MIPI CSI-2 lanes (as well as their order) and the
list of allowed link frequencies are static and come from the device
tree.

But the possible combinations of link frequency and number of active
lanes out of those for a given resolution and format can vary depending
on both transmitter and receiver capabilities.

For example, if the DT specifies 4 lanes and both 148.5 MHz and 297 MHz
link frequencies, the Toshiba chip could send 720p60 YUYV via 4 lanes at
148.5 MHz, via 2 lanes at 297 MHz, or even via 4 lanes at 297 MHz, with
the longer FIFO delay.

> > 
> > [1] http://www.spinics.net/lists/linux-media/msg121813.html, as part
> > of the unicam_start_streaming function.
> > 
> > > The i.MX6 MIPI CSI-2 receiver driver can't cope with that, as it always
> > > activates all four lanes that are configured in the device tree. I can
> > > work around that with the following patch:
> > 
> > It can't cope running at less than 4 lanes, or it can't cope with a
> > change?

The hardware can cope with both, although I don't know if there are
receivers that can not.
In my case this is just about not knowing how many lanes to activate
(see below) and which link frequency to choose (currently fixed to the
first).

[...]
> > > [...] 300 is giving a fair safety margin.
> > > 
> > > It does not work for 720p60 on 4 lanes at 594 Mbit/s, as the spreadsheet
> > > warns, and testing shows.
> > 
> > If it doesn't work with 720p60, then I guess it has no hope with many
> > other resolutions.

That would have to be checked on a case by case basis, unfortunately.
I have a usecase that only supports 1080p50/60 and 720p50/60.

> > It sounds like confirming whether g_mbus_config is a potential
> > solution for i.MX6 (sorry I'm not familiar enough with that code to do
> > my own quick search), but otherwise cranking it up to 320 is
> > reasonable, and I'll see what other numbers fall out of the
> > spreadsheet.

It seems we need a replacement for g_mbus_config that only returns the
dynamic parameters.

regards
Philipp

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Hans Verkuil]

On 09/20/17 14:23, Dave Stevenson wrote:
> On 20 September 2017 at 12:24, Hans Verkuil <hansverk@cisco.com> wrote:
>> On 09/20/17 13:00, Dave Stevenson wrote:
>>> On 20 September 2017 at 11:23, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>>>> Hi,
>>>>
>>>> On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote:
>>>>> Hi Mauro & Philipp
>>>>>
>>>>> On 19 September 2017 at 17:49, Mauro Carvalho Chehab
>>>>> <mchehab@s-opensource.com> wrote:
>>>>>> Em Tue, 19 Sep 2017 17:24:45 +0200
>>>>>> Philipp Zabel <p.zabel@pengutronix.de> escreveu:
>>>>>>
>>>>>>> Hi Dave,
>>>>>>>
>>>>>>> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
>>>>>>>> The existing fixed value of 16 worked for UYVY 720P60 over
>>>>>>>> 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
>>>>>>>> 1080P60 needs 6 lanes at 594MHz).
>>>>>>>> It doesn't allow for lower resolutions to work as the FIFO
>>>>>>>> underflows.
>>>>>>>>
>>>>>>>> Using a value of 300 works for all resolutions down to VGA60,
>>>>>>>> and the increase in frame delay is <4usecs for 1080P60 UYVY
>>>>>>>> (2.55usecs for RGB888).
>>>>>>>>
>>>>>>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>>>>>>>
>>>>>>> Can we increase this to 320? This would also allow
>>>>>>> 720p60 at 594 Mbps / 4 lanes, according to the xls.
>>>>>
>>>>> Unless I've missed something then the driver would currently request
>>>>> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
>>>>> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
>>>>> on a FIFO setting of 16.
>>>>> How/why were you thinking we need to run all four lanes for 720p60
>>>>> without other significant driver mods around lane config?
>>>>
>>>> The driver currently silently changes the number of active lanes
>>>> depending on required data rate, with no way to communicate it to the
>>>> receiver.
>>>
>>> It is communicated over the subdevice API - tc358743_g_mbus_config
>>> reports back the appropriate number of lanes to the receiver
>>> subdevice.
>>> A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call
>>> as you're starting streaming therefore gives you the correct
>>> information. That's what I've just done for the BCM283x Unicam
>>> driver[1], but admittedly I'm not using the media controller API which
>>> i.MX6 is.
>>
>> Shouldn't this information come from the device tree? The g_mbus_config
>> op is close to being deprecated or even removed. There are currently only
>> two obscure V4L2 bridge drivers that call it. It dates from pre-DT times
>> I rather not see it used in a new bridge driver.
>>
>> The problem is that contains data that belongs to the DT (hardware
>> capabilities). Things that can actually change dynamically should be
>> communicated via another op. We don't have that, so that should be created.
>>
>> I've CC-ed Sakari, he is the specialist for such things.
> 
> You've reminded me that I asked that same question earlier in the
> year, and Sakari had replied -
> http://www.spinics.net/lists/linux-media/msg115550.html
> 
> Is it specifically device tree related? Just because the lanes are
> physically there doesn't necessarily mean they have to be used.

The DT should tell how many lanes are connected.

g/s_mbus_config was really doing the job that the device tree does today,
but it does so badly.

My recommendation is that you don't use it at all. Instead look at the
DT for the number of lanes.

*If* it becomes clear that you need to communicate the actual number of
lanes in use, then we need to make a new op or whatever.

> 
> A quick test with the spreadsheet appears to say that 1080p24 UYVY
> over 4 lanes at 594Mbps needs a FIFO setting >=480 (the max is 511). I
> would anticipate that to be one of the worst situations as we're
> dealing with a FIFO underflow herewhen there is a significantly faster
> CSI rate than HDMI.
> It can't be supported with a 972Mbps link frequency over 4 lanes
> (needs >=667), and 2 lanes needs a FIFO setting >=374.
> 
> I'll see what numbers fall out of the new spreadsheet for all standard
> modes. If there are some modes that can't be supported over 4 lanes
> then there is an absolute requirement for communicating the number of
> lanes to use.
> 
> Seeing as Cisco have kit shipping with this chip and driver, can I ask
> how they are managing the choice over number of lanes in use?

It's using g_mbus_config since it runs on an old pre-DT kernel.

I personally would be perfectly happy with a simple new op to just
communicate the number of lanes in use, but there may be more things
that should be passed on to the bridge driver. Sakari knows this better
than I do.

But g_mbus_config shouldn't be used here. No way you could have known
that, we really need to clarify that in v4l2-subdev.h.

Hmm, it DOES say that for s_mbus_config, but not for g_mbus_config.

Regards,

	Hans

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Sakari Ailus]

Hi Hans and others,

On Wed, Sep 20, 2017 at 01:24:02PM +0200, Hans Verkuil wrote:
> On 09/20/17 13:00, Dave Stevenson wrote:
> > On 20 September 2017 at 11:23, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >> Hi,
> >>
> >> On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote:
> >>> Hi Mauro & Philipp
> >>>
> >>> On 19 September 2017 at 17:49, Mauro Carvalho Chehab
> >>> <mchehab@s-opensource.com> wrote:
> >>>> Em Tue, 19 Sep 2017 17:24:45 +0200
> >>>> Philipp Zabel <p.zabel@pengutronix.de> escreveu:
> >>>>
> >>>>> Hi Dave,
> >>>>>
> >>>>> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
> >>>>>> The existing fixed value of 16 worked for UYVY 720P60 over
> >>>>>> 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
> >>>>>> 1080P60 needs 6 lanes at 594MHz).
> >>>>>> It doesn't allow for lower resolutions to work as the FIFO
> >>>>>> underflows.
> >>>>>>
> >>>>>> Using a value of 300 works for all resolutions down to VGA60,
> >>>>>> and the increase in frame delay is <4usecs for 1080P60 UYVY
> >>>>>> (2.55usecs for RGB888).
> >>>>>>
> >>>>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> >>>>>
> >>>>> Can we increase this to 320? This would also allow
> >>>>> 720p60 at 594 Mbps / 4 lanes, according to the xls.
> >>>
> >>> Unless I've missed something then the driver would currently request
> >>> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
> >>> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
> >>> on a FIFO setting of 16.
> >>> How/why were you thinking we need to run all four lanes for 720p60
> >>> without other significant driver mods around lane config?
> >>
> >> The driver currently silently changes the number of active lanes
> >> depending on required data rate, with no way to communicate it to the
> >> receiver.
> > 
> > It is communicated over the subdevice API - tc358743_g_mbus_config
> > reports back the appropriate number of lanes to the receiver
> > subdevice.
> > A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call
> > as you're starting streaming therefore gives you the correct
> > information. That's what I've just done for the BCM283x Unicam
> > driver[1], but admittedly I'm not using the media controller API which
> > i.MX6 is.
> 
> Shouldn't this information come from the device tree? The g_mbus_config
> op is close to being deprecated or even removed. There are currently only
> two obscure V4L2 bridge drivers that call it. It dates from pre-DT times
> I rather not see it used in a new bridge driver.
> 
> The problem is that contains data that belongs to the DT (hardware
> capabilities). Things that can actually change dynamically should be
> communicated via another op. We don't have that, so that should be created.

The DT tells how many lanes are connected in hardware, but up to now that's
also been the number of lanes actually used.

The g_mbus_config() is there, and I'd like to replace that with the more
generic frame descriptors, with CSI-2 virtual channel and data type
support. They're however not quite ready yet nor I've recently worked on
them.

I think using g_mbus_config() for the purpose right now is entirely
acceptable, we can rework that later on when adding support for frame
descriptors.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Hans Verkuil]

On 09/20/17 14:50, Sakari Ailus wrote:
> Hi Hans and others,
> 
> On Wed, Sep 20, 2017 at 01:24:02PM +0200, Hans Verkuil wrote:
>> On 09/20/17 13:00, Dave Stevenson wrote:
>>> On 20 September 2017 at 11:23, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>>>> Hi,
>>>>
>>>> On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote:
>>>>> Hi Mauro & Philipp
>>>>>
>>>>> On 19 September 2017 at 17:49, Mauro Carvalho Chehab
>>>>> <mchehab@s-opensource.com> wrote:
>>>>>> Em Tue, 19 Sep 2017 17:24:45 +0200
>>>>>> Philipp Zabel <p.zabel@pengutronix.de> escreveu:
>>>>>>
>>>>>>> Hi Dave,
>>>>>>>
>>>>>>> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
>>>>>>>> The existing fixed value of 16 worked for UYVY 720P60 over
>>>>>>>> 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
>>>>>>>> 1080P60 needs 6 lanes at 594MHz).
>>>>>>>> It doesn't allow for lower resolutions to work as the FIFO
>>>>>>>> underflows.
>>>>>>>>
>>>>>>>> Using a value of 300 works for all resolutions down to VGA60,
>>>>>>>> and the increase in frame delay is <4usecs for 1080P60 UYVY
>>>>>>>> (2.55usecs for RGB888).
>>>>>>>>
>>>>>>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>>>>>>>
>>>>>>> Can we increase this to 320? This would also allow
>>>>>>> 720p60 at 594 Mbps / 4 lanes, according to the xls.
>>>>>
>>>>> Unless I've missed something then the driver would currently request
>>>>> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
>>>>> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
>>>>> on a FIFO setting of 16.
>>>>> How/why were you thinking we need to run all four lanes for 720p60
>>>>> without other significant driver mods around lane config?
>>>>
>>>> The driver currently silently changes the number of active lanes
>>>> depending on required data rate, with no way to communicate it to the
>>>> receiver.
>>>
>>> It is communicated over the subdevice API - tc358743_g_mbus_config
>>> reports back the appropriate number of lanes to the receiver
>>> subdevice.
>>> A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call
>>> as you're starting streaming therefore gives you the correct
>>> information. That's what I've just done for the BCM283x Unicam
>>> driver[1], but admittedly I'm not using the media controller API which
>>> i.MX6 is.
>>
>> Shouldn't this information come from the device tree? The g_mbus_config
>> op is close to being deprecated or even removed. There are currently only
>> two obscure V4L2 bridge drivers that call it. It dates from pre-DT times
>> I rather not see it used in a new bridge driver.
>>
>> The problem is that contains data that belongs to the DT (hardware
>> capabilities). Things that can actually change dynamically should be
>> communicated via another op. We don't have that, so that should be created.
> 
> The DT tells how many lanes are connected in hardware, but up to now that's
> also been the number of lanes actually used.
> 
> The g_mbus_config() is there, and I'd like to replace that with the more
> generic frame descriptors, with CSI-2 virtual channel and data type
> support. They're however not quite ready yet nor I've recently worked on
> them.
> 
> I think using g_mbus_config() for the purpose right now is entirely
> acceptable, we can rework that later on when adding support for frame
> descriptors.
> 

I don't like it :-)

Currently g_mbus_config returns (and I quote from v4l2-mediabus.h): "How
many lanes the client can use". I.e. the capabilities of the HW.

If we are going to use this to communicate how many lines are currently
in use, then I would propose that we add a lane mask, i.e. something like
this:

/* Number of lanes in use, 0 == use all available lanes (default) */
#define V4L2_MBUS_CSI2_LANE_MASK                (3 << 10)

And add comments along the lines that this is a temporary fix.

I would feel a lot happier (or a lot less unhappy) if we'd do it this way.
Rather than re-interpreting bits that are not quite what they should be.

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.

Regards,

	Hans

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Sakari Ailus]

Hi Hans,

On Wed, Sep 20, 2017 at 03:12:03PM +0200, Hans Verkuil wrote:
> On 09/20/17 14:50, Sakari Ailus wrote:
> > Hi Hans and others,
> > 
> > On Wed, Sep 20, 2017 at 01:24:02PM +0200, Hans Verkuil wrote:
> >> On 09/20/17 13:00, Dave Stevenson wrote:
> >>> On 20 September 2017 at 11:23, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >>>> Hi,
> >>>>
> >>>> On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote:
> >>>>> Hi Mauro & Philipp
> >>>>>
> >>>>> On 19 September 2017 at 17:49, Mauro Carvalho Chehab
> >>>>> <mchehab@s-opensource.com> wrote:
> >>>>>> Em Tue, 19 Sep 2017 17:24:45 +0200
> >>>>>> Philipp Zabel <p.zabel@pengutronix.de> escreveu:
> >>>>>>
> >>>>>>> Hi Dave,
> >>>>>>>
> >>>>>>> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
> >>>>>>>> The existing fixed value of 16 worked for UYVY 720P60 over
> >>>>>>>> 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
> >>>>>>>> 1080P60 needs 6 lanes at 594MHz).
> >>>>>>>> It doesn't allow for lower resolutions to work as the FIFO
> >>>>>>>> underflows.
> >>>>>>>>
> >>>>>>>> Using a value of 300 works for all resolutions down to VGA60,
> >>>>>>>> and the increase in frame delay is <4usecs for 1080P60 UYVY
> >>>>>>>> (2.55usecs for RGB888).
> >>>>>>>>
> >>>>>>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> >>>>>>>
> >>>>>>> Can we increase this to 320? This would also allow
> >>>>>>> 720p60 at 594 Mbps / 4 lanes, according to the xls.
> >>>>>
> >>>>> Unless I've missed something then the driver would currently request
> >>>>> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
> >>>>> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
> >>>>> on a FIFO setting of 16.
> >>>>> How/why were you thinking we need to run all four lanes for 720p60
> >>>>> without other significant driver mods around lane config?
> >>>>
> >>>> The driver currently silently changes the number of active lanes
> >>>> depending on required data rate, with no way to communicate it to the
> >>>> receiver.
> >>>
> >>> It is communicated over the subdevice API - tc358743_g_mbus_config
> >>> reports back the appropriate number of lanes to the receiver
> >>> subdevice.
> >>> A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call
> >>> as you're starting streaming therefore gives you the correct
> >>> information. That's what I've just done for the BCM283x Unicam
> >>> driver[1], but admittedly I'm not using the media controller API which
> >>> i.MX6 is.
> >>
> >> Shouldn't this information come from the device tree? The g_mbus_config
> >> op is close to being deprecated or even removed. There are currently only
> >> two obscure V4L2 bridge drivers that call it. It dates from pre-DT times
> >> I rather not see it used in a new bridge driver.
> >>
> >> The problem is that contains data that belongs to the DT (hardware
> >> capabilities). Things that can actually change dynamically should be
> >> communicated via another op. We don't have that, so that should be created.
> > 
> > The DT tells how many lanes are connected in hardware, but up to now that's
> > also been the number of lanes actually used.
> > 
> > The g_mbus_config() is there, and I'd like to replace that with the more
> > generic frame descriptors, with CSI-2 virtual channel and data type
> > support. They're however not quite ready yet nor I've recently worked on
> > them.
> > 
> > I think using g_mbus_config() for the purpose right now is entirely
> > acceptable, we can rework that later on when adding support for frame
> > descriptors.
> > 
> 
> I don't like it :-)
> 
> Currently g_mbus_config returns (and I quote from v4l2-mediabus.h): "How
> many lanes the client can use". I.e. the capabilities of the HW.
> 
> If we are going to use this to communicate how many lines are currently
> in use, then I would propose that we add a lane mask, i.e. something like
> this:
> 
> /* Number of lanes in use, 0 == use all available lanes (default) */
> #define V4L2_MBUS_CSI2_LANE_MASK                (3 << 10)
> 
> And add comments along the lines that this is a temporary fix.
> 
> I would feel a lot happier (or a lot less unhappy) if we'd do it this way.
> Rather than re-interpreting bits that are not quite what they should be.
> 
> 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.

That would work for me as well.

There are very few users of the g_mbus_config API and I bet the current
users could get the configuration from DT as well: they're platform drivers
used on ARM. With the possible exception of SoC camera drives.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Philipp Zabel]

Hi Hans,

On Wed, 2017-09-20 at 15:12 +0200, Hans Verkuil wrote:
[...]
> I don't like it :-)
> 
> Currently g_mbus_config returns (and I quote from v4l2-mediabus.h): "How
> many lanes the client can use". I.e. the capabilities of the HW.
> 
> If we are going to use this to communicate how many lines are currently
> in use, then I would propose that we add a lane mask, i.e. something like
> this:
> 
> /* Number of lanes in use, 0 == use all available lanes (default) */
> #define V4L2_MBUS_CSI2_LANE_MASK                (3 << 10)
> 
> And add comments along the lines that this is a temporary fix.
> 
> I would feel a lot happier (or a lot less unhappy) if we'd do it this way.
> Rather than re-interpreting bits that are not quite what they should be.
> 
> 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.

I would like to try this.

Currently the driver sets the V4L2_MBUS_CSI2_[1-4]_LANE bits according
to csi_lanes_in_use, which is wrong as you say.

After moving the csi_lanes_in_use info into a new
V4L2_MBUS_CSI2_LANE_MASK bitfield, the V4L2_MBUS_CSI2_[1-4]_LANE bits
could be either set to zero or to the really connected lanes as
configured in the device tree (csi->bus.num_data_lanes) in the DT case.

What would the bits be set to in the pdata case, though? Should a lane
count setting be added to tc358743_platform_data with, defaulting to all
bits set?

regards
Philipp

Re: [PATCH 1/3] [media] tc358743: Correct clock mode reported in g_mbus_config

[Philipp Zabel]

On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
> Support for non-continuous clock had previously been added via
> device tree, but a comment and the value reported by g_mbus_config
> still stated that it wasn't supported.
> Remove the comment, and return the correct value in g_mbus_config.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> ---
>  drivers/media/i2c/tc358743.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/tc358743.c
> b/drivers/media/i2c/tc358743.c
> index e6f5c36..6b0fd07 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -1461,8 +1461,9 @@ static int tc358743_g_mbus_config(struct
> v4l2_subdev *sd,
>  
>  	cfg->type = V4L2_MBUS_CSI2;
>  
> -	/* Support for non-continuous CSI-2 clock is missing in the
> driver */
> -	cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
> +	cfg->flags = state->bus.flags &
> +			(V4L2_MBUS_CSI2_CONTINUOUS_CLOCK |
> +			 V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK);

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp