Re: [PATCH v2 0/5] allow override of bus format in bridges

[Peter Rosin]

On 2018-04-09 14:51, Laurent Pinchart wrote:
> Hi Peter,
> 
> On Monday, 9 April 2018 10:04:05 EEST Peter Rosin wrote:
>> On 2018-04-04 14:35, Peter Rosin wrote:
>>> On 2018-04-04 11:07, Laurent Pinchart wrote:
>>>> On Wednesday, 4 April 2018 09:34:41 EEST Daniel Vetter wrote:
>>>>> On Wed, Apr 4, 2018 at 12:28 AM, Laurent Pinchart wrote:
>>>>>> On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote:
>>>>>>> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> [I got to v2 sooner than expected]
>>>>>>>>
>>>>>>>> I have an Atmel sama5d31 hooked up to an lvds encoder and then
>>>>>>>> on to an lvds panel. Which seems like something that has been
>>>>>>>> done one or two times before...
>>>>>>>>
>>>>>>>> The problem is that the bus_format of the SoC and the panel do
>>>>>>>> not agree. The SoC driver (atmel-hlcdc) can handle the
>>>>>>>> rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
>>>>>>>> wired for the rgb565 case. The lvds encoder supports rgb888 on
>>>>>>>> its input side with the LSB wires for each color simply pulled
>>>>>>>> down internally in the encoder in my case which means that the
>>>>>>>> rgb565 bus_format is the format that works best. And the panel
>>>>>>>> is expecting lvds (vesa-24), which is what the encoder outputs.
>>>>>>>>
>>>>>>>> The reason I "blame" the bus_format of the drm_connector is that
>>>>>>>> with the below DT snippet, things do not work *exactly* due to
>>>>>>>> that. At least, it starts to work if I hack the panel-lvds driver
>>>>>>>> to report the rgb565 bus_format instead of vesa-24.
>>>>>>>>
>>>>>>>>     panel: panel {
>>>>>>>>             compatible = "panel-lvds";
>>>>>>>>             
>>>>>>>>             width-mm = <304>;
>>>>>>>>             height-mm = <228;
>>>>>>>>             
>>>>>>>>             data-mapping = "vesa-24";
>>>>>>>>             
>>>>>>>>             panel-timing {
>>>>>>>>                     // 1024x768 @ 60Hz (typical)
>>>>>>>>                     clock-frequency = <52140000 65000000 71100000>;
>>>>>>>>                     hactive = <1024>;
>>>>>>>>                     vactive = <768>;
>>>>>>>>                     hfront-porch = <48 88 88>;
>>>>>>>>                     hback-porch = <96 168 168>;
>>>>>>>>                     hsync-len = <32 64 64>;
>>>>>>>>                     vfront-porch = <8 13 14>;
>>>>>>>>                     vback-porch = <8 13 14>;
>>>>>>>>                     vsync-len = <8 12 14>;
>>>>>>>>             };
>>>>>>>>             
>>>>>>>>             port {
>>>>>>>>                     panel_input: endpoint {
>>>>>>>>                             remote-endpoint = <&lvds_encoder_output>;
>>>>>>>>                     };
>>>>>>>>             };
>>>>>>>>     };
>>>>>>>>     
>>>>>>>>     lvds-encoder {
>>>>>>>>             compatible = "ti,ds90c185", "lvds-encoder";
>>>>>>>>             
>>>>>>>>             ports {
>>>>>>>>                     #address-cells = <1>;
>>>>>>>>                     #size-cells = <0>;
>>>>>>>>                     
>>>>>>>>                     port@0 {
>>>>>>>>                             reg = <0>;
>>>>>>>>                             
>>>>>>>>                             lvds_encoder_input: endpoint {
>>>>>>>>                                     remote-endpoint =
>>>>>>>>                                     <&hlcdc_output>;
>>>>>>>>                             };
>>>>>>>>                     };
>>>>>>>>                     
>>>>>>>>                     port@1 {
>>>>>>>>                             reg = <1>;
>>>>>>>>                             
>>>>>>>>                             lvds_encoder_output: endpoint {
>>>>>>>>                                     remote-endpoint = <&panel_input>;
>>>>>>>>                             };
>>>>>>>>                     };
>>>>>>>>             };
>>>>>>>>     };
>>>>>>>>
>>>>>>>> But, instead of perverting the panel-lvds driver with support
>>>>>>>> for a totally fake non-lvds bus_format, I intruduce an API that
>>>>>>>> allows display controller drivers to query the required bus_format of
>>>>>>>> any intermediate bridges, and match up with that instead of the
>>>>>>>> formats given by the drm_connector. I trigger this with this addition
>>>>>>>> to the lvds-encoder DT node:
>>>>>>>>             interface-pix-fmt = "rgb565";
>>>>>>>>
>>>>>>>> Naming is hard though, so I'm not sure if that's good?
>>>>>>>>
>>>>>>>> I threw in the first patch, since that is the actual lvds encoder
>>>>>>>> I have in this case.
>>>>>>>>
>>>>>>>> Suggestions welcome.
>>>>>>>
>>>>>>> Took a quick look, feels rather un-atomic. And there's beend
>>>>>>> discussing for other bridge related state that we might want to track
>>>>>>> (like the full adjusted_mode that might need to be adjusted at each
>>>>>>> stage in the chain). So here's my suggestions:
>>>>>>>
>>>>>>> - Add an optional per-bridge internal state struct using the support
>>>>>>>   in https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-> >>>>>   driver-private-state
>>>>>>>   
>>>>>>>   Yes it says "driver private", but since bridge is just helper stuff
>>>>>>>   that's all included. "driver private" == "not exposed as uapi" here.
>>>>>>>   Include all the usual convenience wrappers to get at the state for a
>>>>>>>   bridge.
>>>>>>>
>>>>>>> - Then stuff your bus_format into that new drm_bridge_state struct.
>>>>>>>
>>>>>>> - Add a new bridge callback atomic_check, which gets that bridge state
>>>>>>>   as parameter (similar to all the other atomic_check functions).
>>>>>>>
>>>>>>> This way we can even handle the bus_format dynamically, through the
>>>>>>> atomic framework your bridge's atomic_check callback can look at the
>>>>>>> entire atomic state (both up and down the chain if needed), it all
>>>>>>> neatly fits into atomic overall and it's much easier to extend.
>>>>>>
>>>>>> While I think we'll eventually need bridge states, I don't think that's
>>>>>> need yet. The bus formats reported by this patch series are static.
>>>>>> We're not talking about the currently configured format for a bridge,
>>>>>> but about the list of supported formats. This is similar to the
>>>>>> bus_formats field present in the drm_display_info structure. There is
>>>>>> thus in my opinion no need to interface this with atomic until we need
>>>>>> to track the current format (and I think that will indeed happen at
>>>>>> some point, but I don't think Peter needs this feature for now). That's
>>>>>> why I've told Peter that I would like a bridge API to report the
>>>>>> information and haven't requested a state-based implementation.
>>>>>
>>>>> If it's static, why a dynamic callback? Just add it to struct
>>>>> drm_bridge, require it's filled out before registering the bridge,
>>>>> done.
>>>>
>>>> If I remember correctly I mentioned both options in my initial proposal,
>>>> without a personal preference. A new field in struct drm_bridge would
>>>> certainly work for me.
>>>
>>> You did. Ok, so v3 coming up...
>>
>> Nope, v3 is not coming up. This feels like an impossible mission for me, as
>> this changes core DRM design, and it would just be too much of a time sink
>> for me. Besides, the final paragraph of the nice long "state of
>> bridges"-mail from Laurent, i.e.
>>
>> On 2018-04-04 15:07, Laurent Pinchart wrote:
>>> Finally, still regarding Peter's case, the decision to output RGB565
>>> instead of RGB888 (which the LVDS encoder expects) is due to PCB routing
>>> between the display controller and the LVDS encoder. This isn't a
>>> property of the LVDS encoder or the display controller, but of their
>>> hardware connection. This patch series uses a DT property in the LVDS
>>> encoder DT node to convey that information, but wouldn't it be equally
>>> correct (or incorrect :-)) to instead use a DT property in the display
>>> controller DT node ?
>>
>> hints at where I'm going. The reason is that I have a patch (that needs some
>> polish, will post soon) that makes the atmel-hlcdc driver use components in
>> order to hook it with the tda998x driver (an hdmi encoder), and there too I
>> need the atmel-hlcdc driver to use rgb565 output. And in that case there
>> are no bridges involved, so the proposed solution in this series has zero
>> hope of being adequate. It simply seems that forcing the output mode in the
>> atmel-hlcdc driver is what fixes the root cause.
>>
>> End result; the only thing that survives this series is the interesting
>> discussion and patch 1/5. But I will include that patch in the alternative
>> solution, so you can drop this series entirely...
> 
> I feel some disappointment here. I would like to make it very clear that your 
> work was appreciated. Not all efforts turn into patches that get merged 
> upstream, and some of the greatest work only results in ideas that are then 
> taken over by other developers. Even if this patch series ends up being 
> dropped, it served as a very useful experiment to get us closer to a good 
> solution to the problem. As such the time and efforts you have invested in it 
> are certainly not wasted and were very helpful.

No hard feelings from me, and maybe I'll revisit (not all that keen though)
if the output-mode override for the atmel-hlcdc driver is considered a
workaround in need of a proper solution. Not that I think that's actually
the case, but who knows...

Cheers,
Peter