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

From: Tim Harvey
Date: Wed Jan 31 2018 - 23:47:22 EST

Next message: Stephen Rothwell: "linux-next: Tree for Feb 1"
Previous message: Rajendra Nayak: "[PATCH] arm64: dts: msm8996: Fix wrong use of GIC_CPU_MASK_SIMPLE()"
In reply to: Hans Verkuil: "Re: [PATCH v6 4/6] media: i2c: Add TDA1997x HDMI receiver driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Tim Harvey - Principal Software Engineer
Gateworks Corporation - http://www.gateworks.com/
3026 S. Higuera St. San Luis Obispo CA 93401
805-781-2000

On Wed, Jan 31, 2018 at 5:22 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> 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@xxxxxxxxx> 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@ventana:~# media-ctl -e 'tda19971 2-0048'
>>> /dev/v4l-subdev1
>>> root@ventana:~# 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@ventana:~# v4l2-ctl -d /dev/v4l-subdev1 --get-fmt-video-out
>>> VIDIOC_G_FMT: failed: Inappropriate ioctl for device
>>> root@ventana:~# 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);
>

Hans,

You forgot to include v4l2-ctl-selection.cpp in your patch.

Regards,

Tim

Next message: Stephen Rothwell: "linux-next: Tree for Feb 1"
Previous message: Rajendra Nayak: "[PATCH] arm64: dts: msm8996: Fix wrong use of GIC_CPU_MASK_SIMPLE()"
In reply to: Hans Verkuil: "Re: [PATCH v6 4/6] media: i2c: Add TDA1997x HDMI receiver driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]