Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

[-- Attachment #1: Type: text/plain, Size: 5642 bytes --]

Hi Thierry,

On Monday 22 September 2014 09:40:38 Thierry Reding wrote:
> On Wed, Sep 17, 2014 at 12:27:13PM +0300, Laurent Pinchart wrote:
> > On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote:
> > > On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote:
> > > > Hi Ajay,
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > I think we're moving in the right direction, but we're not there yet.
> > > > 
> > > > On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote:
> > > >> This patch tries to seperate drm_bridge implementation
> > > >> into 2 parts, a drm part and a non_drm part.
> > > >> 
> > > >> A set of helper functions are defined in this patch to make
> > > >> bridge driver probe independent of the drm flow.
> > > >> 
> > > >> The bridge devices register themselves on a lookup table
> > > >> when they get probed by calling "drm_bridge_add_for_lookup".
> > > >> 
> > > >> The parent encoder driver waits till the bridge is available in the
> > > >> lookup table(by calling "of_drm_find_bridge") and then continues with
> > > >> its initialization.
> > > > 
> > > > Before the introduction of the component framework I would have said
> > > > this is the way to go. Now, I think bridges should register themselves
> > > > as components, and the DRM master driver should use the component
> > > > framework to get a reference to the bridges it needs.
> > > 
> > > Well, I have modified the bridge framework exactly the way Thierry
> > > wanted it to be, I mean the same way the current panel framework is.
> > > And, I don't think there is a problem with that.
> > > What problem are you facing with current bridge implementation?
> > > What is the advantage of using the component framework to register
> > > bridges?
> > 
> > There are several advantages.
> > 
> > - The component framework has been designed with this exact problem in
> > mind, piecing multiple components into a display device.
> 
> No. Component framework was designed with multi-device drivers in mind.
> That is, drivers that need to combine two or more platform devices into
> a single logical device. Typically that includes display controllers and
> encoders (in various looks) for DRM.

I disagree. AFAIK the component framework was designed to easily combine 
multiple devices into a single logical device, regardless of which bus each 
device is connected to. That's what makes the component framework useful : it 
allows master drivers to build logical devices from heterogeneous components 
without having to use one API per bus and/or component type. If the only goal 
had been to combine platform devices on an SoC, simpler device-specific 
solutions would likely have been used instead.

> Panels and bridges are in my opinion different because they are outside
> of the DRM driver. They aren't part of the device complex that an SoC
> provides. They represent hardware that is external to the SoC and the
> DRM driver and can be shared across SoCs.

They represent hardware external to the SoC, but internal to the logical DRM 
device.

> Forcing panels and bridges to register as components will require all
> drivers to implement master/component support solely for accessing this
> external hardware.
> 
> What you're suggesting is like saying that clocks or regulators should
> register as components so that their users can get them that way. In
> fact by that argument everything that's referenced by phandle would need
> to register as component (PHYs, LEDs, GPIOs, I2C controllers, ...).

No, that's very different. The device you list are clearly external resources, 
while the bridges and panels are components part of a logical display device.

> > This patch set introduces yet another framework, without any compelling
> > reason as far as I can see. Today DRM drivers already need to use three
> > different frameworks (component, I2C slave encoder and panel), and we're
> > adding a fourth oneto make the mess even messier.
> 
> Panel and bridge aren't really frameworks. Rather they are a simple
> registry to allow drivers to register panels and bridges and display
> drivers to look them up.

Regardless of how you call them, we have three interfaces.

> > This is really a headlong rush, we need to stop and fix the  design
> > mistakes.
>
> Can you point out specific design mistakes? I don't see any, but I'm
> obviously biased.

The slave encoder / bridge split is what I consider a design mistake. Those 
two interfaces serve the same purpose, they should really be merged.

> > - The component framework solves the probe ordering problem. Bridges can
> > use deferred probing, but when a bridge requires a resources (such as a
> > clock for instance) provided by the display controller, this will break.
> 
> Panel and bridges can support deferred probing without the component
> framework just fine.

Not if the bridge requires a clock provided by the display controller, in 
which case there's a dependency loop.

> > > Without this patchset, you cannot bring an X server based display on
> > > snow and peach_pit. Also, day by day the number of platforms using
> > > drm_bridge is increasing.
> > 
> > That's exactly why I'd like to use the component framework now, as the
> > conversion will become more complex as time goes by.
> 
> No it won't. If we ever do decide that component framework is a better
> fit then the conversion may be more work but it would still be largely
> mechanical.

Are you volunteering to perform the conversion ? :-)

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]