Re: [PATCH v3 2/2] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

17.04.2020 00:39, Laurent Pinchart пишет:
> Hi Dmitry,
> 
> On Fri, Apr 17, 2020 at 12:15:33AM +0300, Dmitry Osipenko wrote:
>> 16.04.2020 23:50, Laurent Pinchart пишет:
>>> On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote:
>>>> 16.04.2020 21:52, Dmitry Osipenko пишет:
>>>> ...
>>>>>> May I also recommend switching to the DRM panel bridge helper ? It will
>>>>>> simplify the code.
>>>>>
>>>>> Could you please clarify what is the "DRM panel bridge helper"?
>>>>>
>>>>> I think we won't need any additional helpers after switching to the
>>>>> bridge connector helper, no?
>>>>
>>>> Actually, I now see that the panel needs to be manually attached to the
>>>> connector.
>>>
>>> The DRM panel bridge helper creates a bridge from a panel (with
>>> devm_drm_panel_bridge_add()). You can then attach that bridge to the
>>> chain, like any other bridge, and the enable/disable operations will be
>>> called automatically without any need to call the panel enable/disable
>>> manually as done currently.
>>>
>>>> Still it's not apparent to me how to get panel out of the bridge. It
>>>> looks like there is no such "panel helper" for the bridge API or I just
>>>> can't find it.
>>>
>>> You don't need to get a panel out of the bridge. You should get the
>>> bridge as done today,
>>
>> You mean "get the panel", correct?
> 
> Yes, sorry.
> 
>>> and wrap it in a bridge with
>>> devm_drm_panel_bridge_add().
>>>
>>
>> The lvds-codec already wraps panel into the bridge using
>> devm_drm_panel_bridge_add() and chains it for us, please see
>> lvds_codec_probe() / lvds_codec_attach().
>>
>> Does it mean that lvds-codec is not supporting the new model?
> 
> No, that *is* the new model :-) As I think I've commented during review,
> when you have an LVDS encoder and a panel, you don't need to handle the
> panel at all. When you have an RGB panel connected directly to the RGB
> output, you need to wrap it in a bridge, exactly the same way as
> lvds-codec does for its panel.
> 
>> Everything works nicely using the old model, where bridge creates
>> connector and attaches panel to it for us.
>>
>> [I'm still not sure what is the point to use the new model in a case of
>> a simple chain of bridges]
>>
>> Using the new model, the connector isn't created by the bridge, so I
>> need to duplicate that creation effort in the driver. Once the bridge
>> connector is manually created, I need to attach panel to this connector,
>> but panel is reachable only via the remote bridge (which wraps the panel).
>>
>> driver connector -> LVDS bridge -> panel bridge -> panel
> 
> With the new model,
> 
> 1. The display driver and the bridge drivers need to get hold of the
>    bridge directly connected to their output (for instance with
>    of_drm_find_panel()). If the output is connected to a panel, they
>    need to wrap that panel in a bridge (with
>    devm_drm_panel_bridge_add()). I plan, in the future, to make creation
>    of panel bridges automatic, so drivers won't have to care.
> 
> 2. The display driver needs to create a dummy drm_encoder for each of
>    its outputs (for instance with drm_simple_encoder_init()).
> 
> 3. The display driver needs to create a drm_connector for each of its
>    outputs, and implement connector operations by delegating them to the
>    bridges in the pipeline. Unless there's a good reason not to do so,
>    this should be done with drm_bridge_connector_init().
> 
> That's it. Every driver then focusses on its own needs, bridge drivers
> handle only the device they're associated with, and the DRM core and DRM
> bridge connector helper will handle all the rest.
> 

Thank you very much for the clarification again! :)

Now I realized what was the missing part.. in my case display panel is
mounted upside-down and display output needs to be rotated by 180°. I
have a local patch (hack) that adds orientation-property support to the
panel-lvds driver and previously the panel's orientation was assigned to
the connector using drm_panel_attach(), but now this attachment doesn't
happen and I found that panel_lvds_get_modes() missed
drm_connector_set_panel_orientation() in my patch. Everything is okay
once the panel_lvds_get_modes() is corrected. I'll prepare the v4.