Re: [PATCH 3/4] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Sat, Sep 23, 2017 at 12:30 AM, Hans Verkuil  wrote:
> Hi Tim,
>
> On 23/09/17 00:24, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.
>

Hans,

Thanks for the quick review!

> I did a very quick high-level scan and found a few blockers:
>
> 1) You *must* implement the get/set_edid ops. I won't accept
>the driver without that. You can use v4l2-ctl to set/get the
>EDID (see v4l2-ctl --help-edid).

ok

When I add hooks for get_edid/set_edid I'm getting 'VIDIOC_S_EDID:
failed: Inappropriate ioctl for device'. v4l2-ctl doesn't operate
directly on subdevs so I'm operating on the imx-media-capture device
that the tda1997x is linked to via media-ctl. Am I missing something?

>
> 2) The dv_timings_cap and enum_dv_timings ops are missing: those
>must be implemented as well.

ok

The v4l2_dv_timings_cap contains a v4l2_bt_timings_cap and a type but
it looks like the only type currently available is
V4L2_DV_BT_656_1120. As the tda1997x can output raw RGB and raw YUV
which are not technically BT656/1120 what should I be setting for
those?

>
> 3) Drop the deprecated g_mbus_config op.

ok

>
> 4) Do a proper implementation of query_dv_timings. It appears you
>change the timings in the irq function: that's wrong. The sequence
>should be the following:
>
>a) the irq handler detects that timings have changed and sends
>   a V4L2_EVENT_SOURCE_CHANGE event to userspace.
>b) when userspace receives that event it will stop streaming,
>   call VIDIOC_QUERY_DV_TIMINGS and if new valid timings are
>   detected it will call VIDIOC_S_DV_TIMINGS, allocate the new
>   buffers and start streaming again.
>
>The driver shall never switch timings on its own, this must be
>directed from userspace. Different timings will require different
>configuration through the whole stack (other HW in the video pipeline,
>DMA engines, userspace memory allocations, etc, etc). Only userspace
>can do the reconfiguration.

ok, that makes a ton of sense. I was curious how userspace was
notified and handled this.

>
> General note: if you want to implement CEC and/or HDCP, contact me first.
> I can give pointers on how to do that.
>
> This is just a quick scan. I won't have time to do an in-depth review
> for the next two weeks. Ideally you'll have a v2 ready by then with the
> issues mentioned above fixed.

yes, I should have a v2 ready in a week or so after waiting for some
more feedback.

I do have some general questions perhaps you can answer for me.

The TDA1997x video output port can drive HS/VS/DE either directly from
the HDMI input data (which creates a HS/VS pulses) or it can use an
internal VHREF timing generator that generates VREF/HREF signals
(active during the entire frame/row). What sync signalling is
typically required by SoC's video input?

The IMX6 which I'm using needs HREF and VS so I've defaulted the sync
configuration to that and not allowed sync type configuration via
device-tree (although I did allow sync type configuration via a
platform data struct). I'm not clear if I need to allow all the sync
configuration possibilities to be defined in device-tree at this point
and haven't run across any other video decoders that expose this level
of flexibility via dts.

In order for HREF/VREF to be generated the TDA1997x has an internal
VHREF timing generator that works off of pixel counting. I'm currently
configuring this based off of hard-coded values (units of pixel clock)
for href_start, href_end, vref_f1_start, vref_f1_width, vref_f2_start,
vref_f2_width, fieldref_f1_start, fieldref_f2_start and field_polarity
that were provided in some vendor-specific sample code. I didn't see
anything that would let me pull these values from the list of possible
timings in the kernel, but perhaps I should try calculate these based
on struct v4l2_bt_timings fields instead?

>
> Did you run the v4l2-compliance utility to test this driver? For a v2
> please run it and add the output to the cover letter of the patch series.

I did run v4l2-compliance but I wasn't clear how to interpret the
results and filter out compliance issues between the v4l2-subdev and
the capture driver it's linked to.

I will post this info on the v2 cover page as requested but if
interested here's what it shows with my v0 RFC:

Here's my pipline configuration for a GW54xx IMX6Q where tda19971 is
on IPU1_CSI0 via 16bit data bus supporting YUV422 (single clock cycle)

# links
media-ctl --reset
media-ctl --links '"tda19971 2-0048":0 -> "ipu1_csi0_mux":1[1]'
media-ctl --links '"ipu1_csi0_mux":2 -> "ipu1_csi0":0[1]'
media-ctl --links '"ipu1_csi0":2 -> "ipu1_csi0 capture":0[1]'
# format
media-ctl --set-v4l2 "'tda19971 2-0048':0[fmt:UYVY8_1X16/1280x720]"
media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1280x720]"
media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1280x720]"

# capture 1 frame
v4l2-ctl -d4 --set-fmt-video=width=1280,height=720,pixelformat=UYVY
v4l2-ctl -d4 

Re: [PATCH 3/4] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

Hi Tim,

On 23/09/17 00:24, Tim Harvey wrote:
> Add support for the TDA1997x HDMI receivers.

I did a very quick high-level scan and found a few blockers:

1) You *must* implement the get/set_edid ops. I won't accept
   the driver without that. You can use v4l2-ctl to set/get the
   EDID (see v4l2-ctl --help-edid).

2) The dv_timings_cap and enum_dv_timings ops are missing: those
   must be implemented as well.

3) Drop the deprecated g_mbus_config op.

4) Do a proper implementation of query_dv_timings. It appears you
   change the timings in the irq function: that's wrong. The sequence
   should be the following:

   a) the irq handler detects that timings have changed and sends
  a V4L2_EVENT_SOURCE_CHANGE event to userspace.
   b) when userspace receives that event it will stop streaming,
  call VIDIOC_QUERY_DV_TIMINGS and if new valid timings are
  detected it will call VIDIOC_S_DV_TIMINGS, allocate the new
  buffers and start streaming again.

   The driver shall never switch timings on its own, this must be
   directed from userspace. Different timings will require different
   configuration through the whole stack (other HW in the video pipeline,
   DMA engines, userspace memory allocations, etc, etc). Only userspace
   can do the reconfiguration.

General note: if you want to implement CEC and/or HDCP, contact me first.
I can give pointers on how to do that.

This is just a quick scan. I won't have time to do an in-depth review
for the next two weeks. Ideally you'll have a v2 ready by then with the
issues mentioned above fixed.

Did you run the v4l2-compliance utility to test this driver? For a v2
please run it and add the output to the cover letter of the patch series.

You say "TDA19972 support (2 inputs)": I assume that that means that there
are 2 inputs, but only one is active at a time. Right?

Regards,

Hans

> 
> Signed-off-by: Tim Harvey 
> ---
>  drivers/media/i2c/Kconfig|9 +
>  drivers/media/i2c/Makefile   |1 +
>  drivers/media/i2c/tda1997x.c | 3065 
> ++
>  include/dt-bindings/media/tda1997x.h |   78 +
>  include/media/i2c/tda1997x.h |   53 +
>  5 files changed, 3206 insertions(+)
>  create mode 100644 drivers/media/i2c/tda1997x.c
>  create mode 100644 include/dt-bindings/media/tda1997x.h
>  create mode 100644 include/media/i2c/tda1997x.h

[PATCH 3/4] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

Add support for the TDA1997x HDMI receivers.

Signed-off-by: Tim Harvey 
---
 drivers/media/i2c/Kconfig|9 +
 drivers/media/i2c/Makefile   |1 +
 drivers/media/i2c/tda1997x.c | 3065 ++
 include/dt-bindings/media/tda1997x.h |   78 +
 include/media/i2c/tda1997x.h |   53 +
 5 files changed, 3206 insertions(+)
 create mode 100644 drivers/media/i2c/tda1997x.c
 create mode 100644 include/dt-bindings/media/tda1997x.h
 create mode 100644 include/media/i2c/tda1997x.h

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 9415389..c2b0400 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -56,6 +56,15 @@ config VIDEO_TDA9840
  To compile this driver as a module, choose M here: the
  module will be called tda9840.
 
+config VIDEO_TDA1997X
+   tristate "NXP TDA1997x HDMI receiver"
+   depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
+   ---help---
+ V4L2 subdevice driver for the NXP TDA1997x HDMI receivers.
+
+ To compile this driver as a module, choose M here: the
+ module will be called tda1997x.
+
 config VIDEO_TEA6415C
tristate "Philips TEA6415C audio processor"
depends on I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index c843c18..58f2b2e 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_VIDEO_TVAUDIO) += tvaudio.o
 obj-$(CONFIG_VIDEO_TDA7432) += tda7432.o
 obj-$(CONFIG_VIDEO_SAA6588) += saa6588.o
 obj-$(CONFIG_VIDEO_TDA9840) += tda9840.o
+obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
 obj-$(CONFIG_VIDEO_TEA6415C) += tea6415c.o
 obj-$(CONFIG_VIDEO_TEA6420) += tea6420.o
 obj-$(CONFIG_VIDEO_SAA7110) += saa7110.o
diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c
new file mode 100644
index 000..ca3fec8
--- /dev/null
+++ b/drivers/media/i2c/tda1997x.c
@@ -0,0 +1,3065 @@
+/*
+ * Copyright (C) 2017 Gateworks Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+/* debug level */
+static int debug;
+module_param(debug, int, 0644);
+MODULE_PARM_DESC(debug, "debug level (0-2)");
+
+/* Page 0x00 - General Control */
+#define REG_VERSION0x
+#define REG_INPUT_SEL  0x0001
+#define REG_SVC_MODE   0x0002
+#define REG_HPD_MAN_CTRL   0x0003
+#define REG_RT_MAN_CTRL0x0004
+#define REG_STANDBY_SOFT_RST   0x000A
+#define REG_HDMI_SOFT_RST  0x000B
+#define REG_HDMI_INFO_RST  0x000C
+#define REG_INT_FLG_CLR_TOP0x000E
+#define REG_INT_FLG_CLR_SUS0x000F
+#define REG_INT_FLG_CLR_DDC0x0010
+#define REG_INT_FLG_CLR_RATE   0x0011
+#define REG_INT_FLG_CLR_MODE   0x0012
+#define REG_INT_FLG_CLR_INFO   0x0013
+#define REG_INT_FLG_CLR_AUDIO  0x0014
+#define REG_INT_FLG_CLR_HDCP   0x0015
+#define REG_INT_FLG_CLR_AFE0x0016
+#define REG_INT_MASK_TOP   0x0017
+#define REG_INT_MASK_SUS   0x0018
+#define REG_INT_MASK_DDC   0x0019
+#define REG_INT_MASK_RATE  0x001A
+#define REG_INT_MASK_MODE  0x001B
+#define REG_INT_MASK_INFO  0x001C
+#define REG_INT_MASK_AUDIO 0x001D
+#define REG_INT_MASK_HDCP  0x001E
+#define REG_INT_MASK_AFE   0x001F
+#define REG_DETECT_5V  0x0020
+#define REG_SUS_STATUS 0x0021
+#define REG_V_PER  0x0022
+#define REG_H_PER  0x0025
+#define REG_HS_WIDTH   0x0027
+#define REG_FMT_H_TOT  0x0029
+#define REG_FMT_H_ACT  0x002b
+#define REG_FMT_H_FRONT0x002d
+#define REG_FMT_H_SYNC 0x002f
+#define REG_FMT_H_BACK 0x0031
+#define REG_FMT_V_TOT  0x0033
+#define REG_FMT_V_ACT  0x0035
+#define REG_FMT_V_FRONT_F1 0x0037
+#define REG_FMT_V_FRONT_F2 0x0038
+#define REG_FMT_V_SYNC 0x0039
+#define REG_FMT_V_BACK_F1  0x003a
+#define REG_FMT_V_BACK_F2  0x003b
+#define REG_FMT_DE_ACT 0x003c
+#define REG_RATE_CTRL  0x0040
+#define REG_CLK_MIN_RATE   0x0043
+#define REG_CLK_MAX_RATE   0x0046
+#define REG_CLK_A_STATUS   0x0049
+#define REG_CLK_A_RATE 0x004A
+#define REG_DRIFT_CLK_A_REG0x004D
+#define REG_CLK_B_STATUS   0x004E
+#define REG_CLK_B_RATE 0x004F
+#define REG_DRIFT_CLK_B_REG0x0052
+#define REG_HDCP_CTRL  0x0060
+#define REG_HDCP_KDS   0x0061
+#define REG_HDCP_BCAPS 0x0063
+#define REG_HDCP_KEY_CTRL  0x0064
+#define