diff mbox

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

Message ID 3e638375aff788b24f988e452214649d6100a596.1505826082.git.dave.stevenson@raspberrypi.org (mailing list archive)
State Obsoleted, archived
Delegated to: Hans Verkuil
Headers

Commit Message

Dave Stevenson Sept. 19, 2017, 1:08 p.m. UTC 
  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(-)

Comments

Philipp Zabel Sept. 19, 2017, 3:24 p.m. UTC | #1 
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
Mauro Carvalho Chehab Sept. 19, 2017, 4:49 p.m. UTC | #2 
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
Dave Stevenson Sept. 20, 2017, 9:14 a.m. UTC | #3 
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
diff mbox

Patch

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.