Re: [PATCH v3 06/13] drm: bridge: Add LVDS encoder driver

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 4, 2017 at 2:08 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@ideasonboard.com>
>> >>
>> >> 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