On 05/12/2014 06:00 PM, Sean Paul wrote: > On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda <a.hajda at samsung.com> wrote: >> On 05/09/2014 05:05 PM, Ajay kumar wrote: >>> On Fri, May 9, 2014 at 7:29 PM, Rob Clark <robdclark at gmail.com> wrote: >>>> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda at 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 at 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 :) > > Right, that was me. > > >> 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. >> > > Also detect(). > > >> >>> 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. >> > > It was originally implemented this way. Check out the original > drm_bridge patch and discussion here: > http://lists.freedesktop.org/archives/dri-devel/2013-August/043237.html. > > The tl;dr is: If you don't implement the connector as part of the > bridge, you need to hack detect_connection() in drm_crtc_helper to > either go to the existing connector (which, btw will be the incorrect > connector type), or the bridge if there's one connected. That, or the > existing connector needs to know about the bridge's existence and call > into it for detect() and get_modes(). In the second case, the existing > connector will probably also be the wrong connector type.
Yes, I understand it and my idea was to use drm_panel instead of drm_bridge, at least for bridges which provides connector related functions (get_modes, detect). It seems more straightforward to implement and it simplifies chaining. Regards Andrzej > > >>>>> 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 >>>>>>>> >>