On 10/20/2016 01:50 PM, Laurent Pinchart wrote: > 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.
One more thing is that the TDA998x in its current form won't work with KMS drivers that create their own drm_encoder objects to represent their hardware's mipi DPI/RGB interfaces. For such drivers, we would want the TDA998x to tie itself to the existing encoder created by the KMS driver, rather than creating its own. Thanks, Archit > >> 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. > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
