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

[Archit Taneja <architt@codeaurora.org>]

On 10/22/2016 12:13 AM, Russell King - ARM Linux wrote:
> On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote:
>>
>>
>> On 10/20/2016 02:45 PM, Russell King - ARM Linux wrote:
>>> On Thu, Oct 20, 2016 at 02:38:25PM +0530, Archit Taneja wrote:
>>>>
>>>>
>>>> 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.
>>>
>>> Please show _technically_ how this would work.  I want to see code or
>>> pseudo-code illustrating how a "foreign" DRM encoder could be used with
>>> either dw-hdmi or tda998x, because right now I can't see any way that
>>> could work.
>>
>> This is something we already do with the adv7511 bridge driver on msm,
>> rcar and arc (for 4.9) drivers.
>>
>> I've shared pseudo code on the kms driver and encoder chip's driver
>> side. I've also shared a diff that converts the tda998x driver to use
>> drm_bridge(uncompiled/untested).
>>
>> 1) Kms driver side:
>>
>> /*
>>  * Create an encoder instance. Depending on the hardware represented
>>  * by the KMS driver, the encoder can ops can either have some
>>  * functionality, or be nops. In the case of tilcdc, the encoder
>>  * funcs would be mostly nops.
>>  */
>> drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs);
>> drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs,
>> 		 type, NULL);
>
> How does the KMS driver know to create the encoder?

In the DT case, the Dove CRTC would see that it is connected
to a device node which represents either a block of HW within
the SoC, or an external chip like TDA998x. The KMS driver
would be aware of its capabilities (i.e, whether it can
encode RGB to HDMI itself or not).

In cases where it can't do the encoding itself, it should
create an encoder (with nop funcs) and try to find a bridge.
Once it finds a bridge, it links the newly created encoder
to the bridge, and attach it.

In cases where we have encoding capability within the SoC,
but also have another external encoder chip, it creates
its own encoder as is (with the ops to configure the HW)
and links itself with the external encoder bridge.

To summarize, there are two cases:

- The HW represented by the bridge driver is the only encoder in
the chain: In this case, we create a 'dummy' encoder in the KMS
driver, and link it to the bridge.

- The HW represented by the bridge driver isn't the first encoder
in the chain: In this case, the KMS driver creates its encoder
as is, and gets the drm_bridge from the bridge driver, and links
the two.

>
>> /*
>>  * Extract the bridge using DT node of the external encoder chip,
>>  * i.e. tda998x
>>  */
>>
>> bridge = of_drm_find_bridge(encoder_chip_dev->of_node);
>
> What if we're not using DT?  There seems to be no solution for that, so
> this currently breaks non-DT armada-drm.

Yeah, I agree. We haven't looked at non-DT cases yet. Will try to figure
out what to do for non-DT scenarios.

>
> In the case of hardware which is:
>
> Dove ===> TDA998x ===> display
>
> Who provides this encoder_chip_dev's of_node?  The Dove is the CRTC.
> The TDA998x is the bridge.  What's the encoder?  Do we need to make up
> a ficticious DT device for that.  That will raise rightful objections,
> because it means DT is no longer representative of the hardware, but
> is representing the hardware in an implementation specific way - the
> split of the encoder from bridge is entirely a software abstraction.

The encoder_chip_dev is supposed to be the TDA998x device here. The
Dove CRTC's output port would be directly connected to the input port
of TDA998x in DT. There would be no encoder node in DT. The DT bindings
wouldn't change. Sorry about the choice of name, it made it a bit confusing.

Thanks,
Archit

-- 
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