On Wed, Jan 4, 2017 at 2:08 PM, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > Hi Daniel, > > On Wednesday 04 Jan 2017 09:18:18 Daniel Vetter wrote: >> On Tue, Nov 29, 2016 at 10:57:07PM +0200, Laurent Pinchart wrote: >> > On Tuesday 29 Nov 2016 10:54:09 Daniel Vetter wrote: >> >> On Tue, Nov 29, 2016 at 11:04:36AM +0200, Laurent Pinchart wrote: >> >>> The LVDS encoder driver is a DRM bridge driver that supports the >> >>> parallel to LVDS encoders that don't require any configuration. The >> >>> driver thus doesn't interact with the device, but creates an LVDS >> >>> connector for the panel and exposes its size and timing based on >> >>> information retrieved from DT. >> >>> >> >>> Signed-off-by: Laurent Pinchart >> >>> <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> >> >> >> >> Since it's 100% dummy, why put LVDS into the name? This little thing >> >> here could be our generic "wrap drm_panel and attach it to a chain" >> >> helper. So what about calling this _The_ drm_panel_bridge, and also >> >> linking it into docs to feature it a bit more prominently. >> > >> > I'm not opposed to that, except that this driver should not be considered >> > as just a helper to link a panel. It should only be used to support a >> > real hardware bridge that requires no control. >> > >> >> I came up with this because I spotted some refactoring belows for >> >> building this helper, until I realized that this driver _is_ the helper I >> >> think we want ;-) Only thing missing is an exported function to >> >> instantiate a bridge with just a drm_panel as the parameter. And putting >> >> it into the drm_kms_helper.ko module. >> > >> > What would that be used for ? The bridge should be instantiated by this >> > bridge driver, bound to a bridge device instantiated from DT (or the >> > platform firmware of your choice). >> >> Atm every driver using drm_panel needs a bit of glue to bind it into it's >> display chain. With this code, and bridge chaining, you'd get that for >> free, and I think that would be rather useful. > > You would trade the bit of panel glue for a bit of bridge glue. Bridge > chaining could perhaps help slightly at runtime there, but at init time the > amount of glue would be similar. Hm, something like drm_bridge_panel_bridge_init(dev, panel) should be enough, or not? My idea is to use this for the case where the only thing in dt is the panel, with no real bridge chip. And I think we don't need anything beyond that one _init function, plus maybe some additional paramaters ... > I've noticed one piece of code that is LVDS-specific in this driver, and > that's connector creation. The connector type is hardcoded to LVDS. To make > the driver more generic, we would need a way to find the connector type at > runtime. I'm wondering whether I should do this now, or keep the driver LVDS- > specific until someone comes up with a similar need. Without several potential > users it's often hard to design a properly generic API. ... like whether the panel is dsi or lvds or soemthing else. Or maybe we should just use LVDS for everything, because it's a panel, and userspace shouldn't really care beyond that ;-) >> And from a software point of view I'd say if it quacks like a bridge, and >> walks like a bridge, it probably _is_ a bridge. Even if no one calls that, >> or if physical the only thing on the board at that spot are a bunch of dumb >> wires. > > I dream of moving all encoders to DRM bridge, and then unifying drm_bridge and > drm_panel with a common set of operations that could be invoked on both > objects. That way the core could chain bridges and panels quite easily. I plan > to experiment with this when moving the omapdrm driver to DRM bridge and DRM > panel (it currently includes a bunch of custom encoder and panel drivers). Agreed on that dream, auto-wrapping panels in dummy bridges would be a first step towards that. The other one is making drm_encoders entirely dummies, and I think we're 90% there already. End result isn't quite as clean as a complete rewrite, but probably good enough. And because uapi we can't get rid of drm_encoder anyway, and keeping drm_panel as the internal thing embedded into a drm_panel_bridge struct seems reasonable too. That way panel drivers can cope with a slightly simpler interface than full bridges. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
