Re: [RFC V2 0/3] drm/bridge: panel and chaining

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

On 05/09/2014 05:05 PM, Ajay kumar wrote:
> On Fri, May 9, 2014 at 7:29 PM, Rob Clark <robdclark@gmail.com> wrote:
>> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> On 05/08/2014 08:24 PM, Rob Clark wrote:
>>>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>>>
>>>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>>>
>>>>>> Still need to make use of standard list calls and figure out proper way
>>>>>> of deleting the bridge chain. So, this is just a rough version.
>>>>> As I understand this patchset tries to solve two things:
>>>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>>>> Crtc -> Encoder -> Bridge -> Panel
>>>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>>>
>>>>> It is done using Russian doll schema, ops from the bridge calls the same
>>>>> ops from the next bridge and the next bridge ops can do the same.
>>>>>
>>>>> This schema means that all the bridges including the last one are seen
>>>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>>>> this particular case, the first bridge (ptn3460) implements connector
>>>>> so it is hard to guess what is the location of the 2nd bridge in video
>>>>> stream chain, sometimes it is after the connector, sometimes before.
>>>>> All this is quite confusing.
>>>>>
>>>>> But if you look at the bridge from upstream video interface point of
>>>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>>>> video input side acts as a panel. On the output side it expects a panel,
>>>>> lvds panel in this case.
>>>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>>>> best name.. I wouldn't object to changing it.
>>>>
>>>> But my thinking was to leave in drm_panel_funcs things that are just
>>>> needed by the connector (get_modes().. and maybe some day we need
>>>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>>>> could (if needed) implement both interfaces.
>>>>
>>>> That is basically the same as what you are proposing, but without
>>>> renaming bridge to panel ;-)
>>> Good to hear that. However there are points which are not clear for me.
>>> But first lets clarify names, I will use panel and bridge words
>>> to describe the hardware, and drm_panel, drm_bridge to describe the
>>> software interfaces.
>>>
>>> What bothers me:
>>> 1. You want to leave connector related callbacks in drm_panel and
>>> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
>>> must implement connector internally because of this limitation. I guess
>>> it is quite typical bridge. This problem does not exists in case
>>> of using drm_panel for ptn3460.
>>>
>>> 2. drm_bridge is attached to the encoder, this and the callback order
>>> suggests the video data flow should be as below:
>>> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>>>
>>> ptn3460 implements drm_bridge and drm_connector so it suggests its
>>> drm_bridge should be the last one, so there should be no place to add
>>> lvds panel implemented as a drm_bridge after it, as it is done in this
>>> patchset.
>>>
>>> Additionally it clearly shows that there should be two categories of
>>> drm_bridges - non-terminal and terminal.
>>>
>>> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
>>> its components are up. It simplifies synchronization but is quite
>>> fragile - the whole drm will be down due to error in some of its components.
>>> For this reason I prefer drm_panel as it is not real drm component
>>> it can be attached/detached to/from drm_connector anytime. I am not
>>> really sure but drm_bridge does not allow for that.
>> So I do think we need to stick to this all-or-nothing approach for
>> anything that is visible to userspace
>> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
>> to "hotplug" those so I don't see a real smooth upgrade path to add
>> that in a backwards compatible way that won't cause problems with old
>> userspace.
>>
>> But, that said, we have more flexibility with things not visible to
>> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
>> allow things to be completely dynamic (we already have some hard
>> enough locking fun).  But proposals/rfcs/etc welcome.
>>
>> I guess I'm not completely familiar w/ ptn3460, but the fact that it
>> needs to implement drm_connector makes me a bit suspicious.  Seems
>> like a symptom of missing things in drm_panel_funcs.  It would be
>> better to always create the connector statically, and just have
>> _detect() -> disconnected if panel==NULL.

ptn3460 has been implemented using drm_bridge and drm_connector, not by
me, to be clear :)
And to make it more clear from what I see ptn3460 exposes following ops:
- pre_enable (via drm_bridge).
- disable (via drm_bridge),
- get_modes (via drm_connector).
Other ops are exposed just to fulfill requirements of drm frameworks, I
guess.


> This is something which only Sean can answer!
> I guess he implemented ptn3460 as connector thinking that bridge would
> be the last
> entity in the video pipeline. If that's a real problem, we can still
> move out the
> connector part.
>
> Regards,
> Ajay

The question is how it can be implemented using only drm_bridge.

>>> Real life example to show importance of it: I have a phone with MIPI-DSI
>>> panel and HDMI. Due to initialization issues HDMI bridge driver
>>> sometimes fails during probe and the drmdev do not start. Of course this
>>> is development stage so I have serial console I can diagnose the
>>> problem, disable HDMI, fix the problem, etc...
>>> But what happens in case of end-user. He will see black screen - bricked
>>> phone. In case the bridge will be implemented using drm_panel
>>> he will have working phone with broken HDMI, much better.
>> well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)

It can break also during phone utilization.

>>
>> I suppose if bridge/panel where loaded dynamically (or at least after
>> drm device and drm_{connector,encoder,etc} are created, it would help
>> a bit here.  I'd kinda hope that isn't the only benefit/reason to make
>> things more dynamic.  Especially if we allow bridges/panels to be
>> unloaded.. (just loading them dynamically doesn't seem as scary from
>> locking perspective)
>>
>>> 4. And the last thing, it is more about the concept/design. drm_bridge,
>>> drm_hw_block suggests that those interfaces describes the whole device:
>>> bridge, panel, whatever.
>> hmm, I don't think this is the case.  I can easily see things like:
>>
>>   struct foo_panel {
>>     struct drm_panel base;
>>     struct drm_bridge bridge;
>>     ...
>>   }
>>
>> where a panel implementation implements both panel and bridge.  In
>> fact that is kinda what I was encouraging.

I guess it can work, but I see it sub-optimal. In general, looking on
the hardware
the same video data goes to the panel and to the bridge (if they are of
the same type of course),
I do not know why it couldn't be mapped to software interfaces. For
example drm_sink, as I described
previously (now it is cited below).

Regards
Andrzej

>>
>> BR,
>> -R
>>
>>> In my approach I have an interface
>>> to describe only one video input port of the device. And drm_panel is
>>> in fact misleading name, drm_sink may be better. So real panel
>>> would implement drm_sink interface. Bridge would implement drm_sink
>>> interface and it will request other drm_sink interface, to interact with
>>> the panel which is after it.
>>> This approach seems to me more flexible. Beside things I have described
>>> above it will allow to implement also more complicated devices, dsi
>>> hubs, video mixers, etc.
>>>
>>>
>>> Regards
>>> Andrzej
>>>
>>>> BR,
>>>> -R
>>>>
>>>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>>>> another drm_panel. With this approach everything fits much better.
>>>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>>>> to implement connector in the bridge and you have a driver following
>>>>> linux driver model. And no single bit changed in drm core.
>>>>>
>>>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>>>> problems with drm_bridges I have decide to attract attention to much
>>>>> more cleaner solution.
>>>>>
>>>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>>>
>>>>> Regards
>>>>> Andrzej
>>>>>
>>>>>
>>>>>> Ajay Kumar (3):
>>>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>>>
>>>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>>>