Re: [PATCH v4 00/34] drm/exynos: Refactor parts of the exynos driver

[Andrzej Hajda <a.hajda@samsung.com>]

On 03/04/2014 02:57 PM, Inki Dae wrote:
> Hi Andrzej,
>
>
> 2014-03-04 20:03 GMT+09:00 Andrzej Hajda <a.hajda@samsung.com>:
>> Hi Inki,
>>
>> On 02/28/2014 03:28 AM, Inki Dae wrote:
>> (...)
>>> Andrzej Hajda from our team is already working on addressing remaining
>>> issues with Sean's series, including fixing removed parallel display
>>> support. Earlier this month he sent a series restoring support for DSI
>>> panels: http://thread.gmane.org/gmane.
>>>
>>>  Already merged to internal kernel for test.
>>>
>>>> linux.kernel.samsung-soc/27044 . Now he's also trying to make the design
>>>> more modular. He will give you more information on Monday, as he's on
>>>> holiday for next two days.
>> Thanks Tomasz for pointing it out. I try to properly restore parallel
>> panel support.
>> I have come to the conclusion that the most natural way is to implement
>> crtc/encoder/connector in FIMD, as the FIMD directly produces
>> parallel RGB stream (or I80 in command mode).
>> I think implementing crtc/encoder/connector in FIMD in case of DSI and
>> eDP panel
>> is also more natural. In such case I would convert DSIM, eDP and ptn3460
>> to drm_panel
>> framework similarly as I already did for TC358764 DSI/LVDS bridge.
> In case of some machines without MIPI-DSI like below,
>         FIMD------>Parallel Panel
>
> There wouldn't be proper place for implementing a connector and a
> encoder. I guess your conclusion results from such cases.
Yes, this case has inspired me to re-think about proper placement for
drm_encoder and drm_connector in case of different HW pipelines.

Problem is that we can have arbitrary number of hw components
in the pipeline and we have only three drm components:
drm_crtc, drm_encoder, drm_connector. I skip drm_bridge as it is
in fact optional extra encoder, so it can be helpful in specific cases
but in
general it just complicates things IMHO.

The solution I see is to represent all encoders/bridges/image processors
present in the pipeline as a single drm_encoder, which encodes from
format provided to the
first element and produces format provided by the last one. The last
element of the pipeline
can be treated as drm_connector.

Additionally it would be nice to make possible to create all drm components
from video pipeline description provided by device tree.
If we start such pipeline creation from FIMD node, if code finds there
is any outgoing
video link from FIMD node it should create drm_encoder/drm_connector pair,
and attach device pointed by this link to that pair. Next attached
device should attach
devices pointed by its outgoing links and so on till there are no out links.
This is my rough idea, what do you think about it?
 
>
> But how should we handle eDP if connector and encoder are implemented
> only in FIMD driver?
> eDP can be hot-plugged and the hot-plug function will be handled by
> interrupt handler of eDP driver. This would mean that the hardware
> resource to the hot-plug belongs to eDP ip, and also its own connector
> should be implemented in eDP driver to detect the connection.
drm_panel has access to its connector and drm_dev, so after
extending drm_panel_funcs with 'detect' callback it should work.
>> Here are different scenarios of HW pipelines:
>> 1. Parallel panel:
>>     FIMD ---> Parallel Panel
>> 2. DSI panel:
>>     FIMD ---> DSIM ---> DSI Panel
>> 3. Arndale:
>>     FIMD ---> DSIM ---> DSI/LVDS bridge ---> LVDS panel
>> 4. Snow:
>>     FIMD ---> eDP ---> eDP/LVDS bridge ---> LVDS panel
>> 5. In case mobile image enhancer(MIE) will be used pipeline can be
>> extended to:
>>     FIMD ---> MIE ---> DSIM ---> .....
>>     or
>>     FIMD ---> MIE ---> eDP ---> .....
>>
> Other scenarios,
>       FIMD----->LVDS bridge----->LVDS panel
>
> Actually, I had used above machine, Geminus.
>
>
> And in case of Exynos4 series, it's more complicated,
>
>       FIMD----->mDNIe------>FIMD-Lite------>Parallel Panel
>       FIMD----->mDNIe------>FIMD-Lite------>MIPI-DSI------>DSI Panel
>       ...
>
> However, mDNIe ip isn't opened in public and also maybe continue so I
> think Exynos drm driver doesn't need to cover such cases.
It is available on some cyanogen repos, so theoretically it is possible
someone can try to implement it in mainline :)
>
>> Pros of implementing all drm components (crtc,encoder,connector) in
>> FIMD, and using
>> drm_panel framework for panels, DSIM, eDP and external bridges:
>> - it is natural, FIMD produces already encoded stream, DSIM and eDP are
>> just optional
>>   converters,
>> - easy to extend, adding/removing image processors or bridges will
>> require only changing
>>   video paths in DTS (in addition to adding device nodes),
>> - consistency.
>>
>> What do you think about it?
>>
> I think it's a good idea but corner case still exists as mentioned
> earlier. How about implementing connector and encoder in panel driver
> in case of machines not using MIPI-DSI and eDP? And in case of
> machines using MIPI-DSI and eDP, connector and encoder are implemented
> in their drivers as now.
For now it is OK for me, but I prefer to put the encoder/connector into
fimd, as drm_panel framework assumes connector is already created
when panel is attached to the drm.
>
> In case of using parallel panel, we could declare super device node
> for Exynos drm like below,
>
>         exynos-drm {
>                 compatible = "samsung,exynos-drm";
>                 crtcs = <&fimd>;
>                 connectors = <&panel>;
>         };
>
> connectors could be Panel, eDP, HDMI, and MIPI-DSI.
OK, it will allow avoid deferred probing.


Regards
Andrzej