Message-ID: <22676a5b-cc2b-4cb9-b921-d1fb6c13a626@linaro.org>
Date:   Thu, 23 Nov 2023 14:39:59 +0100
From:   Neil Armstrong <neil.armstrong@...aro.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Sui Jingfeng <sui.jingfeng@...ux.dev>
Cc:     Phong LE <ple@...libre.com>, Maxime Ripard <mripard@...nel.org>,
        Sui Jingfeng <suijingfeng@...ngson.cn>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Thomas Zimmermann <tzimmermann@...e.de>
Subject: Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

On 23/11/2023 09:08, Dmitry Baryshkov wrote:
> On Thu, 23 Nov 2023 at 07:05, Sui Jingfeng <sui.jingfeng@...ux.dev> wrote:
>>
>> Hi,
>>
>>
>> On 2023/11/16 19:19, Dmitry Baryshkov wrote:
>>> On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <sui.jingfeng@...ux.dev> wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
>>>>> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <sui.jingfeng@...ux.dev> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Thanks a lot for reviewing!
>>>>>>
>>>>>>
>>>>>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>>>>>>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@...ux.dev> wrote:
>>>>>>>> From: Sui Jingfeng <suijingfeng@...ngson.cn>
>>>>>>>>
>>>>>>>> The it66121_create_bridge() and it66121_destroy_bridge() are added to
>>>>>>>> export the core functionalities. Create a connector manually by using
>>>>>>>> bridge connector helpers when link as a lib.
>>>>>>>>
>>>>>>>> Signed-off-by: Sui Jingfeng <suijingfeng@...ngson.cn>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
>>>>>>>>      include/drm/bridge/ite-it66121.h     |  17 ++++
>>>>>>>>      2 files changed, 113 insertions(+), 38 deletions(-)
>>>>>>>>      create mode 100644 include/drm/bridge/ite-it66121.h
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>>>> index 8971414a2a60..f5968b679c5d 100644
>>>>>>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>>>> @@ -22,6 +22,7 @@
>>>>>>>>
>>>>>>>>      #include <drm/drm_atomic_helper.h>
>>>>>>>>      #include <drm/drm_bridge.h>
>>>>>>>> +#include <drm/drm_bridge_connector.h>
>>>>>>>>      #include <drm/drm_edid.h>
>>>>>>>>      #include <drm/drm_modes.h>
>>>>>>>>      #include <drm/drm_print.h>
>>>>>>>> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>>>>>>>>                                      enum drm_bridge_attach_flags flags)
>>>>>>>>      {
>>>>>>>>             struct it66121_ctx *ctx = bridge_to_it66121(bridge);
>>>>>>>> +       struct drm_bridge *next_bridge = ctx->next_bridge;
>>>>>>>> +       struct drm_encoder *encoder = bridge->encoder;
>>>>>>>>             int ret;
>>>>>>>>
>>>>>>>> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>>>>>>>> -               return -EINVAL;
>>>>>>>> +       if (next_bridge) {
>>>>>>>> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>>>>>>>> +                       WARN_ON(1);
>>>>>>> Why? At least use WARN() instead
>>>>>> Originally I want to
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>> +                       flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
>>>>>>>> +               }
>>>>>>>> +               ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
>>>>>>>> +               if (ret)
>>>>>>>> +                       return ret;
>>>>>>>> +       } else {
>>>>>>>> +               struct drm_connector *connector;
>>>>>>>>
>>>>>>>> -       ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
>>>>>>>> -       if (ret)
>>>>>>>> -               return ret;
>>>>>>>> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>>>>>>>> +                       WARN_ON(1);
>>>>>>> No. It is perfectly fine to create attach a bridge with no next_bridge
>>>>>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>>>>>>>
>>>>>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
>>>>>> the bridge shall not create a drm_connector. So I think if a display
>>>>>> bridge driver don't have a next bridge attached (Currently, this is
>>>>>> told by the DT), it says that this is a non-DT environment. On such
>>>>>> a case, this display bridge driver(it66121.ko) should behavior like
>>>>>> a *agent*. Because the upstream port of it66121 is the DVO port of
>>>>>> the display controller, the downstream port of it66121 is the HDMI
>>>>>> connector. it66121 is on the middle. So I think the it66121.ko should
>>>>>> handle all of troubles on behalf of the display controller drivers.
>>>>> No. Don't make decisions for the other drivers. They might have different needs.
>>>> [...]
>>>>
>>>>
>>>>>> Therefore (when in non-DT use case), the display controller drivers
>>>>>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
>>>>>> Which is to hint that the it66121 should totally in charge of those
>>>>>> tasks (either by using bridge connector helper or create a connector
>>>>>> manually). I don't understand on such a case, why bother display
>>>>>> controller drivers anymore.
>>>>> This is the reason why we had introduced this flag. It allows the
>>>>> driver to customise the connector. It even allows the driver to
>>>>> implement a connector on its own, completely ignoring the
>>>>> drm_bridge_connector.
>>>>
>>>> I know what you said is right in the sense of the universe cases,
>>>> but I think the most frequent(majority) use case is that there is
>>>> only one display bridge on the middle. Therefore, I don't want to
>>>> movethe connector things into device driver if there is only one display
>>>> bridge(say it66121) in the middle. After all, there is no *direct
>>>> physical connection* from the perspective of the hardware. I means that
>>>> there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers
>>>> should not interact with anything related with the connector on a
>>>> perfect abstract on the software side. Especially for such a simple use
>>>> case. It probably make senses to make a  decision for themost frequently use case, please also note
>>>> that this patch didn't introduce any-restriction for the more advance
>>>> uses cases(multiple bridges in the middle).
>>> So, for the sake of not having the connector in the display driver,
>>> you want to add boilerplate code basically to each and every bridge
>>> driver. In the end, they should all behave in the same way.
>>>
>>> Moreover, there is no way this implementation can work without a
>>> warning if there are two bridges in a chain and the it66121 is the
>>> second (the last) one. The host can not specify the
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>>>>>>>> +                       WARN_ON(1);
>>>>>>> No. It is perfectly fine to create attach a bridge with no next_bridge
>>>>>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>>>>>>>
>>>>>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
>>>>>> the bridge shall not create a drm_connector. So I think if a display
>>>>>> bridge driver don't have a next bridge attached (Currently, this is
>>>>>> told by the DT), it says that this is a non-DT environment. On such
>>>>>> a case, this display bridge driver(it66121.ko) should behavior like
>>>>>> a *agent*. Because the upstream port of it66121 is the DVO port of
>>>>>> the display controller, the downstream port of it66121 is the HDMI
>>>>>> connector. it66121 is on the middle. So I think the it66121.ko should
>>>>>> handle all of troubles on behalf of the display controller drivers.
>>>>> No. Don't make decisions for the other drivers. They might have different needs.
>>>> [...]
>>>>
>>>>
>>>>>> Therefore (when in non-DT use case), the display controller drivers
>>>>>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
>>>>>> Which is to hint that the it66121 should totally in charge of those
>>>>>> tasks (either by using bridge connector helper or create a connector
>>>>>> manually). I don't understand on such a case, why bother display
>>>>>> controller drivers anymore.
>>>>> This is the reason why we had introduced this flag. It allows the
>>>>> driver to customise the connector. It even allows the driver to
>>>>> implement a connector on its own, completely ignoring the
>>>>> drm_bridge_connector.
>>>>
>>>> I know what you said is right in the sense of the universe cases,
>>>> but I think the most frequent(majority) use case is that there is
>>>> only one display bridge on the middle. Therefore, I don't want to
>>>> movethe connector things into device driver if there is only one display
>>>> bridge(say it66121) in the middle. After all, there is no *direct
>>>> physical connection* from the perspective of the hardware. I means that
>>>> there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers
>>>> should not interact with anything related with the connector on a
>>>> perfect abstract on the software side. Especially for such a simple use
>>>> case. It probably make senses to make a  decision for themost frequently use case, please also note
>>>> that this patch didn't introduce any-restriction for the more advance
>>>> uses cases(multiple bridges in the middle).
>>> So, for the sake of not having the connector in the display driver,
>>> you want to add boilerplate code basically to each and every bridge
>>> driver. In the end, they should all behave in the same way.
>>
>> No, I'm only intend to modify the one when there has a user emerged.
>> If we have the connector related code in the KMS display driver side,
>> then I think we don't honor the meaning of the word *bridge*. I was
>> told drm_bridge is a modern design, if we still need the DC side
>> worry about something do not have a physical connection, then it will
>> not be modern anymore, it is not a good bridge.
> 
> Next time the user emerges for another bridge. And then for another.
> This way the very similar code is copy-pasted over all bridge drivers.
> So instead it was decided to keep it in the display driver code.
> 
>>
>>
>>> Moreover, there is no way this implementation can work without a
>>> warning if there are two bridges in a chain and the it66121 is the
>>> second (the last) one.
>>
>> Yes and no!
>>
>> If one of them are transparent, then thisimplementation still can works. It is just that this will not be a good
>> abstract anymore.I do have seen such design on some notebook hardware.  But from my programming experiences,
>> using two bridges are typically a bad practice in engineering. As it tend
>> to increase the PCB board area and increase entire cost. As you need buy
>> one more TX encoder chip. Please also consider that the embedded world focus
>> on low cost and low power consume.
> 
> A typical pipeline for an embedded device can perfectly look like:
> - DSI host (drm_encoder)
> - DSI-HDMI or DSI-eDP bridge (drm_bridge)
> - hdmi-connector or panel-bridge (drm_bridge)
> - drm_bridge_connector.
> 
> Two drm_bridge instances.

Nowadays, we are moving the encoder code into bridge so we can have way
more than 2 bridges, and connector code has been moved to a bridge.

On Amlogic SoCs, the HDMI chain has at least 3 bridges, it can have up to 4
on DSI usecase if you plug a DSI to eDP bridge.

On iMX8 platform, they have multiple internal SoC bridges even before the HDMI or DSI bridge.

The model works very well across extremely heterogeneous embedded platforms.

> 
>>
>> I think, multiple display bridges case should be avoided for middle/low end
>> application. Or allow us to handle the two and/or more bridges cases in the
>> future. When there has at least one user emerged, we will introduce new
>> approach to handle then.
>>
>> Do you find any product level boards that using two external display bridge and
>> one of them is it66121? If we can not even find a user, we are not even have a
>> board to test if current design (state of art) works. Does it suffer from module
>> loading order problems? what if their i2c slave address is same? Does such a use
>> case will past the S3/S4 test? All of those concerns are imposed to every display
>> bridges programmer from the very beginning.
> 
> Please add a hdmi-connector device to your testing model. You don't
> have to use it, but it is a fully legit use case. And suddenly you
> have to drm_bridge instances in your chain.
> 
>>
>> I'm agree with the idea that drm bridges drivers involved toward to a direction
>> that support more complex design, but I think we should also leave a way for the
>> most frequent use case. Make it straight-forward as a canonical design.
> 
> Not having anything connector-related in the drm_bridge driver is a
> canonical design.
> 
>>
>>> The host can not specify the
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, it will cause a warning here. And
>>> it can not omit the flag (as otherwise the first bridge will create a
>>> connector, without consulting the second bridge).
>>
>> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flagare implement-defined,
> 
> No, they are not. Semantics are pretty simple: do not create the
> drm_connector instance. Pass the flag to the next bridge in the chain.
> 
>> for our case, I could just ignore it if their
>> don't have a signal(DT or ACPI) tell me that there are multiple bridges
>> in the chain. This depend on community's attitude.
> 
> Ignoring a flag is a bad idea.
> 
>>
>> For it66121 with a canonical design, the host should not need to specify this flag.
>> Because at the time of when the device driver(it66121.ko) get loaded, the it66121
>> driver could parse the DT by itself, and detect if there has a next bridge, is it a
>> connector or is it yet another display bridges. The DT speak everything about the
>> topology. The flag is there just for the KMS display controller driver to explicit
>> control, use it and make it more useful is the right way, is it?
> 
> No. We have been there (before the DRM_BRIDGE_ATTACH_NO_CONNECTOR was
> introduced), we have gone away from it.
> 
>>
>>
>>>>
>>>>>>>> +
>>>>>>>> +               connector = drm_bridge_connector_init(bridge->dev, encoder);
>>>>>>>> +               if (IS_ERR(connector))
>>>>>>>> +                       return PTR_ERR(connector);
>>>>>>>> +
>>>>>>>> +               drm_connector_attach_encoder(connector, encoder);
>>>>>>> This goes into your device driver.
>>>>>>>
>>>>>>>> +
>>>>>>>> +               ctx->connector = connector;
>>>>>>>> +       }
>>>>>>>>
>>>>>>>>             if (ctx->info->id == ID_IT66121) {
>>>>>>>>                     ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
>>>>>>>> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
>>>>>>>>             "vcn33", "vcn18", "vrf12"
>>>>>>>>      };
>>>>>
>>>
>>>
> 
> 
> 

Powered by blists - more mailing lists