Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc

[Peter Rosin]

On 2018-04-20 13:38, jacopo mondi wrote:
> Hi Peter,
> 
> On Fri, Apr 20, 2018 at 01:05:21PM +0200, Peter Rosin wrote:
>> On 2018-04-20 12:18, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote:
>>>> Hi Peter,
>>>>     I've been a bit a pain in the arse for you recently, but please
>>>> bear with me a bit more, and sorry for jumping late on the band wagon.
>>>>
>>>> On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote:
>>>>> Hi!
>>>>>
>>>>> I naively thought that since there was support for both nxp,tda19988 (in
>>>>> the tda998x driver) and the atmel-hlcdc, things would be a smooth ride.
>>>>> But it wasn't, so I started looking around and realized I had to fix
>>>>> things.
>>>>>
>>>>> In v1 and v2 I fixed things by making the atmel-hlcdc driver a master
>>>>> component, but now in v3 I fix things by making the tda998x driver
>>>>> a bridge instead. This was after a suggestion from Boris Brezillion
>>
>> That should be Brezillon, sorry for being sloppy with the spelling.
>>
>>>>> (the atmel-hlcdc maintainer), so there was some risk of bias ... but
>>>>> after comparing what was needed, I too find the bridge approach better.
>>>>>
>>>>> In addition to the above, our PCB interface between the SAMA5D3 and the
>>>>> HDMI encoder is only using 16 bits, and this has to be described
>>>>> somewhere, or the atmel-hlcdc driver have no chance of selecting the
>>>>> correct output mode. Since I have similar problems with a ds90c185 lvds
>>>>> encoder I added patches to override the atmel-hlcdc output format via
>>>>> DT properties compatible with the media video-interface binding and
>>>>> things start to play together.
>>>>>
>>>>> Since this series superseeds the bridge series [1], I have included the
>>>>> leftover bindings patch for the ti,ds90c185 here.
>>>>
>>>> I feel like this series would look better if it would make use of the
>>>> proposed bridge media bus format support I have recently sent out [1]
>>>> (and which was not there when you first sent v1).
>>>>
>>>> I understand your fundamental problem here is that the bus format
>>>> that should be reported by your bridge is different from the ones
>>>> actually supported by the TDA19988 chip, as the wirings ground some
>>>> of the input pins.
>>>>
>>>> Although this is defintely something that could be described in the
>>>> bridge's own OF node with the 'bus_width' property, and what I would do,
>>>> now that you have made a bridge out from the tda19988 driver, is:
>>>>
>>>> 1) Set the bridge accepted input bus_format parsing its pin
>>>> configuration, or default it if that's not implemented yet.
>>>> This will likely be rgb888. You can do that using the trivial
>>>> support for bridge input image formats implemented by my series.
>>>> 2) Specify in the bridge endpoint a 'bus_width' of <16>
>>>> 3) In your atmel-hlcd get both the image format of the bridge (rgb888)
>>>> and parse the remote endpoint property 'bus_width' and get the <16>
>>>> value back.
>>>
>>> Parsing properties of remote nodes should be avoided as much as possible, as
>>> they need to be interpreted in the context of the DT bindings related to the
>>> compatible string applicable to that node. I'd rather have the bus_width
>>> property in the local endpoint node.
>>
>> In addition to that, my view of this binding
>>
>> 	endpoint {
>> 		bus-type = <0>;
> 
> bus-type is used by v4l2_fwnode helpers to decide which type of bus
> they're dealing with iirc. Here it seems to me it has no added
> value, also because in your bindings description "it shall be 0" and
> it is not parsed anywhere in you code, but no big deal....

bus-type is indeed parsed and verified to be zero which means auto-detect
according to the video-interfaces binding. An auto-detect bus-type with
a given bus-width means a parallel interface IIUC.

>From patch 3/7:

	if (of_property_read_u32(node, "bus-type", &bus_type))
		return 0;
	if (bus_type != 0)
		return -EINVAL;


>> 		bus-widht = <16>;
>> 	};
>>
>> is that it always means rgb565. See further below.
>>
>>>> 4) Set the correct format in the atmel hlcd driver to accommodate a
>>>> bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565,
>>>> or are there other possible combinations I am missing?)
>>>>
>>>> I would consider this better mostly because in this series you are
>>>> creating a semantic for the whole DRM subsystem on the 'bus_width'
>>>> property, where numerical values as '12', '16' etc are arbitrary tied
>>>> to the selection of a media bus format. At least you should use a
>>>> common set of defines [1] between the device tree and the driver,
>>>> but this looks anyway fragile imho.
>>>
>>> This I agree with though. Combining the remote bus format with the local bus
>>> width should fix the problem without having to parse remote properties.
>>
>> My thinking was that the binding with bus-type = <0> and bus-width = <bpp>
>> would mean a parallel bus (type 0 means auto-detect and with a bus-width that
>> auto-detect means a parallel bus) and the most natural/common interpretation
>> of that bus-width. For bus widths of 12, 16, 18, 24, 30 etc I think that
>> would be rgb444, rgb565, rgb666, rgb888, rgb101010 (or, I'm first so I get
>> to define the default). If you have some other interpretation of a bus with
>> that width, you'd need to extend the video-interface binding with some way
>> of saying what you need, perhaps using some kind of data mapping or something
>> to say e.g. bgr666. And you'd need some kind of indicator if you have YUV
>> signals instead of RGB, and LVDS isn't a completely parallel bus, so you'd
>> need something for that. Etc.
> 
> The fundamental issue here is that you're tying the bus with to an
> image format. Why <16> can't be, say, MEDIA_BUS_FMT_YVYU8_1X16? it
> spans to 16 bits, doesn't it? And I'm sorry but the 'I'm first so I
> get to define defaults' argument doesn't apply here.

I never said that <16> could not be something YUV. I said that <16> without
any further information is RGB. But you are right, the fundamental issue
is that I want to specify the full format in the endpoint node, while you
do not for some reason I don't understand. And maybe saying that "<16>
without more info is RGB" is too presumptuous, but then I think that the
right fix is adding something clearly indicating RGB is the way to go, so
that there is a way for endpoints to specify RGB565 (or whatever is needed).

> So, it is my opinion you need to expose an image format somewhere. And
> it is my opinion again, which can be very wrong ofc, that this place
> is the bridge driver.

I don't see why the input side is better than the output side when it
comes to specifying these details? The input side is as likely to have
different options as the output side.

> You need to adjust the output to accommodate wirings? You can use the
> bus_width on the hlcd side, as Laurent suggested, but the bus width
> does not tie to any image format at all, even more if you're making
> that decision in a drm-wide function.
> 
>>
>> Because the word from Rob was that there should be one common binding that
>> describes video interfaces. I started an implementation that interprets that
>> binding in a drm context in
>> [PATCH 3/7] drm: of: introduce drm_of_media_bus_fmt
> 
> As usual, one should try to reuse as much as possible of the existing
> bindings. That's a totally different thing compared to assign to a bus
> width property an associated image format for the whole DRM subsystem
> 
> ot: those properties should be moved outside of
> media/video_interfaces.txt sooner or later, to a more generic place...
> 
>>
>> With that view, any input format specification of the bridge is not helpful
>> for me since what the bridge specifies (without help) is going to be wrong
> 
> No it's not useless. I can have an encoder that can provide both YUV
> and RGB formats. If your bridge accepts RGB I want to know that, and
> the bus_width is unrelated imho.

I didn't say useless *period*, I said not helpful *for me*. What I mean is
that since I have an encoder that can only output RGB formats, asking the
bridge if it expects RGB is rather useless. It adds nothing whatsoever.

Cheers,
Peter

>> anyway. End result, I need to specify the format manually on either the
>> bridge or the atmel-hlcdc side, and I happen to think that the correct side
>> is with the atmel-hlcdc, because that is where my issue originates. In short,
>> the "drm: bridge: Add support for static image formats" series is unrelated
>> as far as I can tell.
> 
> I may be biased on this, so I let other to judge and provide more
> suggestions.
> 
> Thanks
>    j
>