[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call

[Laurent Pinchart]

Hi Russell,

On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
> > On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
> >> In any case, I don't agree with converting it to a DRM bridge - that
> >> will mean that we have to split the driver into two pieces, the bridge
> >> part handling the mode set specifics, and a connector/encoder part which
> >> handles the detection/edid stuff.
> >> 
> >> We might as well keep the whole thing as the classical connector/encoder
> >> rather than introducing this additional layer of complexity.
> > 
> > We have different ways to instantiate external HDMI encoders, and that's
> > not good. I believe everybody agrees that drm encoder slave has to go, so
> > that's already one problem solved (or rather solvable, it still requires
> > someone to do the work). We will then be left with two different methods,
> > drm bridge and non-bridge component-based instantiation. We need to
> > somehow merge the two, and I'm open to discussions on how the end result
> > should look like.
>
> I think you're idea really doesn't work - and I think your idea that
> component-based is somehow separate from other methods is wrong.
> 
> Look at iMX for example - even converting all the code that could be
> a bridge to be a bridge will not get rid of its use of the component
> instantiation, because you still have other components that need to
> come from elsewhere.  The same is true of armada as well.

Don't get me wrong, I'm certainly not against the component framework itself. 
A way to bind multiple independent devices together is needed. We have a 
similar framework in V4L2 called v4l2-async, and I'd like to see it moved to 
the component framework at some point to unify code. Some changes to the 
component framework might be needed to support needs of V4L2 that are 
currently not applicable to DRM/KMS, but there's nothing major there.

> Moreover, as I've already said, converting tda998x to a DRM bridge
> will not get rid of the encoder/connector part, because it _is_ a
> connector in the DRM sense - it provides the detection and EDID
> reading.
> 
> So, what would we achieve by splitting the driver into a DRM bridge
> and DRM encoder/connector?

Please note that DRM bridge doesn't split the DRM connector out of the bridge, 
bridges instantiate drm_connector objects. In that sense they don't differ 
much from the model implemented by the tda998x driver.

I however believe that connectors should be split out DRM bridge drivers, for 
the simple reason that bridges can be chained. The output of a bridge isn't 
guaranteed to be a connector but can be another bridge. We managed not to have 
to deal with that in a generic way so far in mainline, but we'll run into the 
problem one of these days, and a solution will be needed. There's no rush 
right now, and this is unrelated to converting tda998x to DRM bridge.

> We would still need the component helper to manage the binding of
> the connector part, which would also then have to register a DRM
> bridge in addition to a DRM encoder and providing the DRM connector
> as we can't have two device drivers bound to the same i2c device.
> What we get is more complexity in the driver.

DRM bridges indeed don't create encoders. That task is left to the display 
driver. The reason is the same as above: bridges can be chained (including 
with an internal encoder that is not modelled as a bridge, and that's a case 
we have today), while the KMS model exposes a single encoder to userspace. 
Exposing DRM encoder objects as part of the KMS UABI was probably a mistake. 
Better solutions would have been to expose no encoder at all or all encoders 
in the chain. We are however stuck with this model as we can't break the UABI. 
For that reason a DRM encoder object doesn't represent an encoder but a chain 
of encoders. As a DRM bridge models a single encoder, the DRM encoder object 
must be created at a higher level, in the display driver.

> We can see this with what happened to the DW-HDMI driver - that still
> needs the component helper, and it provides a DRM bridge, DRM encoder
> and DRM connector parts.

To be precise, the DW-HDMI driver core doesn't create a DRM encoder, it's the 
glue layers implemented as part of the Rockchip and iMX display drivers that 
do. Nonetheless, that's a mistake, the encoder should be created by the 
display drivers.

> The only reason it made sense to split the DW-HDMI driver was due to the
> differences between the Rockchip and Freescale implementations.  Such
> differences do not exist for TDA998x. So even here, your idea that "DRM
> bridge" vs "non-DRM bridge component based" doesn't work - we have "DRM
> bridge component based" because of the problem that I'm illustrating here.
> 
> So, again, I ask: what's the point of needlessly splitting the code
> between an encoder/connector and a bridge?

We need to standardize on one model. I don't care much about how the end 
result is named, as long as it fulfils the task at hand. For the reasons 
explained above, it should not create a DRM encoder or DRM connector. A step 
by step approach is obviously needed to get there, one option being moving all 
external encoders to DRM bridge as a first step without instantiating the DRM 
encoder in the bridge driver, and then moving connector instantiation out as a 
second step.

The current situation is a huge mess, we simply can't pretend to ignore the 
problem.

> You seem to be forcing the DRM bridge model onto drivers with no
> regard for its appropriateness for those drivers.  If it pushes
> more complexity into drivers, the model is wrong.

I expect several (many? most?) display drivers to handle bridges, encoders and 
connectors in the same way, so we should obviously share common code in the 
form of helper functions to keep display drivers simple.

-- 
Regards,

Laurent Pinchart