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

[Hans Verkuil]

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