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

[Archit Taneja <architt@codeaurora.org>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel