Message-ID: <83695d60-5052-14ba-4f7b-23f153a05a85@xs4all.nl>
Date:   Wed, 31 Jan 2018 14:22:11 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Tim Harvey <tharvey@...eworks.com>
Cc:     linux-media <linux-media@...r.kernel.org>,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Shawn Guo <shawnguo@...nel.org>,
        Steve Longerbeam <slongerbeam@...il.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Hans Verkuil <hansverk@...co.com>,
        Mauro Carvalho Chehab <mchehab@...pensource.com>
Subject: Re: [PATCH v6 4/6] media: i2c: Add TDA1997x HDMI receiver driver

On 01/31/18 08:38, Hans Verkuil wrote:
> On 01/31/2018 05:51 AM, Tim Harvey wrote:
>> On Mon, Jan 29, 2018 at 4:00 AM, Hans Verkuil <hverkuil@...all.nl> wrote:
>>> On 01/25/2018 05:15 PM, Tim Harvey wrote:
>> <snip>
>>>>>
>>>>> Hmm. This receiver supports multiple output formats, but you advertise only one.
>>>>> That looks wrong. If nothing else, you should be able to switch between RGB and
>>>>> YUV 4:4:4 since they use the same port config.
>>>>>
>>>>> It's a common use-case that you want to switch between RGB and YUV depending on
>>>>> the source material (i.e. if you receive a desktop/graphics then RGB is best, if
>>>>> you receive video then YUV 4:2:2 or 4:2:0 is best).
>>>>>
>>>>> Hardcoding just one format won't do.
>>>>>
>>>>
>>>> I've been thinking about this a bit. I had hard-coded a single format
>>>> for now because I haven't had any good ideas on how to deal with the
>>>> fact that the port mappings would need to differ if you change from
>>>> the RGB888/YUV444 (I think these are referred to as 'planar' formats?)
>>>> to YUV422 (semi-planar) and BT656 formats. It is true though that the
>>>> 36bit (TDA19973) RGB888/YUV444 and 24bit (TDA19971/2) formats can both
>>>> be supported with the same port mappings / pinout.
>>>
>>> Regarding terminology:
>>>
>>> RGB and YUV are typically interleaved, i.e. the color components are
>>> (for two pixels) either RGBRGB for RGB888, YUVYUV for YUV444 or YUYV
>>> for YUV422.
>>>
>>> Planar formats are in practice only seen for YUV and will first output
>>> all Y samples, and then the UV samples. This requires that the hardware
>>> buffers the frame and that's not normally done by HDMI receivers.
>>>
>>> The DMA engine, however, is often able to split up the interleaved YUV
>>> samples that it receives and DMA them to separate buffers, thus turning
>>> an interleaved media bus format to a planar memory format.
>>>
>>> BT656 doesn't refer to how the samples are transferred, instead it
>>> refers to how the hsync and vsync are reported. The enum v4l2_mbus_type
>>> has various options, one of them being BT656.
>>>
>>> Which mbus type is used is board specific (and should come from the
>>> device tree). Whether to transmit RGB888, YUV444 or YUV422 (or possibly
>>> even YUV420) is dynamic and is up to userspace since it is use-case
>>> dependent.
>>>
>>> So you'll never switch between BT656 and CSI, but you can switch between
>>> BT656+RGB and BT656+YUV, or between CSI+RGB and CSI+YUV.
>>>
>>>>
>>>> For example the GW5400 has a TDA19971 mapped to IMX6 CSI_DATA[19:4]
>>>> (16bit) for YUV422. However if you want to use BT656 you have to shift
>>>> the TDA19971 port mappings to get the YCbCr pins mapped to
>>>> CSI_DATA[19:x] and those pin groups are at the bottom of the bus for
>>>> the RGB888/YUV444 format.
>>>
>>> As mentioned above, you wouldn't switch between mbus types.
>>>
>>>>
>>>> I suppose however that perhaps for the example above if I have a 16bit
>>>> width required to support YUV422 there would never be a useful case
>>>> for supporting 8-bit/10-bit/12-bit BT656 on the same board?
>>>
>>> You wouldn't switch between mbus types, but if the device tree configures
>>> BT.656 with a bus width of 24 bits, then the application might very well
>>> want to dynamically switch between 8, 10 and 12 bits per color component.
>>>
>>
>> Hans,
>>
>> I just submitted a v7 with multiple format support. Your point about
>> bus_type being specified by dt is exactly what I needed to help make
>> sense of the formats.
> 
> Ah, good. It took me some time as well before I realized that the confusion
> was in mixing up bus types and formats.
> 
>> That said, I'm unsure how to properly test the enum_mbus_code() pad op
>> function. How do you obtain a list of valid formats on a subdev?
>>
>> I tried the following:
>> root@...tana:~# media-ctl -e 'tda19971 2-0048'
>> /dev/v4l-subdev1
>> root@...tana:~# media-ctl --get-v4l2 '"tda19971 2-0048":0'
>>                 [fmt:UYVY8_2X8/1280x720 field:none colorspace:srgb]
>> ^^^^ calls get_format and returns the 1 and only format available for
>> my tda19971 with 16bit parallel bus
>> root@...tana:~# v4l2-ctl -d /dev/v4l-subdev1 --get-fmt-video-out
>> VIDIOC_G_FMT: failed: Inappropriate ioctl for device
>> root@...tana:~# v4l2-ctl -d /dev/v4l-subdev1 --list-formats-out
>> ioctl: VIDIOC_ENUM_FMT
>>
>> I'm thinking perhaps enumerating the list of possible formats is a
>> missing feature in media-ctl?
> 
> Yeah, it is. Surprising, really. Ditto for the SUBDEV_ENUM_FRAME_SIZE/INTERVAL
> ioctls.
> 
> I wonder whether this should be added to v4l2-ctl, media-ctl or both. I always
> felt that media-ctl is more about setting up the whole pipeline, whereas v4l2-ctl
> is specific to a V4L2 device.
> 
> Regards,
> 
> 	Hans
> 

Here is a quick patch adding support for enumerating mbus codes to v4l2-ctl.
You get some compile warnings, just ignore those.

An example using vimc, listing the code for pad 1:

$ ./v4l2-ctl -d /dev/v4l-subdev5 --list-subdev-mbus-codes=1
ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE
        0x00001013: BGR888_1X24
        0x0000100a: RGB888_1X24
        0x0000100d: ARGB8888_1X32

Regards,

	Hans

---------------- cut here ------------------------
diff --git a/utils/v4l2-ctl/Makefile.am b/utils/v4l2-ctl/Makefile.am
index 83fa49a3..b3ccfc8b 100644
--- a/utils/v4l2-ctl/Makefile.am
+++ b/utils/v4l2-ctl/Makefile.am
@@ -6,9 +6,16 @@ v4l2_ctl_SOURCES = v4l2-ctl.cpp v4l2-ctl.h v4l2-ctl-common.cpp v4l2-ctl-tuner.cp
 	v4l2-ctl-io.cpp v4l2-ctl-stds.cpp v4l2-ctl-vidcap.cpp v4l2-ctl-vidout.cpp \
 	v4l2-ctl-overlay.cpp v4l2-ctl-vbi.cpp v4l2-ctl-selection.cpp v4l2-ctl-misc.cpp \
 	v4l2-ctl-streaming.cpp v4l2-ctl-sdr.cpp v4l2-ctl-edid.cpp v4l2-ctl-modes.cpp \
-	v4l2-tpg-colors.c v4l2-tpg-core.c v4l-stream.c v4l2-ctl-meta.cpp
+	v4l2-ctl-subdev.cpp v4l2-tpg-colors.c v4l2-tpg-core.c v4l-stream.c v4l2-ctl-meta.cpp
 v4l2_ctl_CPPFLAGS = -I$(top_srcdir)/utils/common

+media-bus-format-names.h: ../../include/linux/media-bus-format.h
+	sed -e '/#define MEDIA_BUS_FMT/ ! d; s/.*FMT_//; /FIXED/ d; s/\t.*//; s/.*/{ \"&\", MEDIA_BUS_FMT_& },/;' \
+	< $< > $@
+
+BUILT_SOURCES = media-bus-format-names.h
+CLEANFILES = $(BUILT_SOURCES)
+
 if WITH_V4L2_CTL_LIBV4L
 v4l2_ctl_LDADD = ../../lib/libv4l2/libv4l2.la ../../lib/libv4lconvert/libv4lconvert.la -lrt -lpthread
 else
diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
index 46e621f7..2f521f5d 100644
--- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
+++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
@@ -91,6 +91,8 @@ void common_usage(void)
 #ifndef NO_LIBV4L2
 	       "  -w, --wrapper      use the libv4l2 wrapper library.\n"
 #endif
+	       "  --which-is-try     set the 'which' argument to V4L2_SUBDEV_FORMAT_TRY instead of\n"
+	       "                     V4L2_SUBDEV_FORMAT_ACTIVE (the default)\n"
 	       "  --list-devices     list all v4l devices\n"
 	       "  --log-status       log the board status in the kernel log [VIDIOC_LOG_STATUS]\n"
 	       "  --get-priority     query the current access priority [VIDIOC_G_PRIORITY]\n"
diff --git a/utils/v4l2-ctl/v4l2-ctl.cpp b/utils/v4l2-ctl/v4l2-ctl.cpp
index e02dc756..9dced5b1 100644
--- a/utils/v4l2-ctl/v4l2-ctl.cpp
+++ b/utils/v4l2-ctl/v4l2-ctl.cpp
@@ -83,6 +83,7 @@ static struct option long_options[] = {
 	{"help-vbi", no_argument, 0, OptHelpVbi},
 	{"help-sdr", no_argument, 0, OptHelpSdr},
 	{"help-meta", no_argument, 0, OptHelpMeta},
+	{"help-subdev", no_argument, 0, OptHelpSubDev},
 	{"help-selection", no_argument, 0, OptHelpSelection},
 	{"help-misc", no_argument, 0, OptHelpMisc},
 	{"help-streaming", no_argument, 0, OptHelpStreaming},
@@ -91,6 +92,7 @@ static struct option long_options[] = {
 #ifndef NO_LIBV4L2
 	{"wrapper", no_argument, 0, OptUseWrapper},
 #endif
+	{"which-is-try", no_argument, 0, OptWhichIsTry},
 	{"concise", no_argument, 0, OptConcise},
 	{"get-output", no_argument, 0, OptGetOutput},
 	{"set-output", required_argument, 0, OptSetOutput},
@@ -115,6 +117,7 @@ static struct option long_options[] = {
 	{"list-formats-sdr-out", no_argument, 0, OptListSdrOutFormats},
 	{"list-formats-out", no_argument, 0, OptListOutFormats},
 	{"list-formats-meta", no_argument, 0, OptListMetaFormats},
+	{"list-subdev-mbus-codes", optional_argument, 0, OptListSubDevMBusCodes},
 	{"list-fields-out", no_argument, 0, OptListOutFields},
 	{"clear-clips", no_argument, 0, OptClearClips},
 	{"clear-bitmap", no_argument, 0, OptClearBitmap},
@@ -1212,6 +1215,7 @@ int main(int argc, char **argv)
 	const char *wait_event_id = NULL;
 	__u32 poll_for_event = 0;	/* poll for this event */
 	const char *poll_event_id = NULL;
+	__u32 which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	unsigned secs = 0;
 	char short_options[26 * 2 * 3 + 1];
 	int idx = 0;
@@ -1274,6 +1278,9 @@ int main(int argc, char **argv)
 		case OptHelpMeta:
 			meta_usage();
 			return 0;
+		case OptHelpSubDev:
+			subdev_usage();
+			return 0;
 		case OptHelpSelection:
 			selection_usage();
 			return 0;
@@ -1317,6 +1324,9 @@ int main(int argc, char **argv)
 			if (poll_for_event == 0)
 				return 1;
 			break;
+		case OptWhichIsTry:
+			which = V4L2_SUBDEV_FORMAT_TRY;
+			break;
 		case OptSleep:
 			secs = strtoul(optarg, 0L, 0);
 			break;
@@ -1341,6 +1351,7 @@ int main(int argc, char **argv)
 			vbi_cmd(ch, optarg);
 			sdr_cmd(ch, optarg);
 			meta_cmd(ch, optarg);
+			subdev_cmd(ch, optarg);
 			selection_cmd(ch, optarg);
 			misc_cmd(ch, optarg);
 			streaming_cmd(ch, optarg);
@@ -1480,6 +1491,7 @@ int main(int argc, char **argv)
 	vbi_set(fd);
 	sdr_set(fd);
 	meta_set(fd);
+	subdev_set(fd, which);
 	selection_set(fd);
 	streaming_set(fd, out_fd);
 	misc_set(fd);
@@ -1497,6 +1509,7 @@ int main(int argc, char **argv)
 	vbi_get(fd);
 	sdr_get(fd);
 	meta_get(fd);
+	subdev_get(fd, which);
 	selection_get(fd);
 	misc_get(fd);
 	edid_get(fd);
@@ -1512,6 +1525,7 @@ int main(int argc, char **argv)
 	vbi_list(fd);
 	sdr_list(fd);
 	meta_list(fd);
+	subdev_list(fd, which);
 	streaming_list(fd, out_fd);

 	if (options[OptWaitForEvent]) {
diff --git a/utils/v4l2-ctl/v4l2-ctl.h b/utils/v4l2-ctl/v4l2-ctl.h
index 3b56d8a6..2ac29268 100644
--- a/utils/v4l2-ctl/v4l2-ctl.h
+++ b/utils/v4l2-ctl/v4l2-ctl.h
@@ -10,6 +10,7 @@
 #include <string>

 #include <linux/videodev2.h>
+#include <linux/v4l2-subdev.h>

 #ifndef NO_LIBV4L2
 #include <libv4l2.h>
@@ -58,6 +59,7 @@ enum Option {
 	OptGetVideoFormat = 'V',
 	OptSetVideoFormat = 'v',
 	OptUseWrapper = 'w',
+	OptWhichIsTry = 'W',

 	OptGetSlicedVbiOutFormat = 128,
 	OptGetOverlayFormat,
@@ -97,6 +99,7 @@ enum Option {
 	OptListSdrOutFormats,
 	OptListOutFormats,
 	OptListMetaFormats,
+	OptListSubDevMBusCodes,
 	OptListOutFields,
 	OptClearClips,
 	OptClearBitmap,
@@ -211,6 +214,7 @@ enum Option {
 	OptHelpVbi,
 	OptHelpSdr,
 	OptHelpMeta,
+	OptHelpSubDev,
 	OptHelpSelection,
 	OptHelpMisc,
 	OptHelpStreaming,
@@ -345,6 +349,13 @@ void meta_set(int fd);
 void meta_get(int fd);
 void meta_list(int fd);

+// v4l2-ctl-subdev.cpp
+void subdev_usage(void);
+void subdev_cmd(int ch, char *optarg);
+void subdev_set(int fd, __u32 which);
+void subdev_get(int fd, __u32 which);
+void subdev_list(int fd, __u32 which);
+
 // v4l2-ctl-selection.cpp
 void selection_usage(void);
 void selection_cmd(int ch, char *optarg);

Powered by blists - more mailing lists