On 05/12/2014 02:45 PM, Rob Clark wrote:
On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda a.hajda@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@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:
- 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.
- 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.
- 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 :)
sure, and afaiu it was adapted from a pre-bridge implementation on chromeos tree. So between that, and the fact that bridge and panel are relatively new, it is not unexpected that some evolution/refactoring will happen as we go.
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).
sure, this is why I'm leaning towards saying that drm_panel_funcs should be anything a connector needs that a bridge does not need (to avoid putting fxn ptrs in drm_bridge_funcs which don't make sense for a pure bridge)
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.
I'm not entirely sure I understand why. I think you would want to have a ptn3460 bridge (pure bridge) + chaining + foo_panel which has it's bridge interface chained up to ptn3460 and a panel interface passed to the connector.
(At some point, maybe it makes sense to have a generic drm_panel_connector which drivers can re-use to avoid duplicating the connector code, but that is an implementation detail.)
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)
- 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).
I'm not entirely sure why letting a panel implement multiple different interfaces (where needed) is suboptimal. It seems more sub-optimal to put panel related fxns which are only applicable to panels in drm_bridge_funcs.
foo implemented using drm_panel and drm_bridge seems to me less optimal than foo implemented with the same functionality but using drm_panel only.
Well, my initial reaction when you start talking about drm_src and drm_sinks is that this can quickly get over-designed. I'm not trying to turn kms into v4l2 unless there is a good reason. But maybe I'm assuming too much about what you are proposing.
OK, lesson learned: avoid using word sink :)
Regards Andrzej
BR, -R
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 >>