Message-ID: <00ba2245-0e48-4b21-bcd4-29dfb728e408@linux.dev>
Date:   Thu, 23 Nov 2023 13:05:28 +0800
From:   Sui Jingfeng <sui.jingfeng@...ux.dev>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Phong LE <ple@...libre.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        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

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.


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

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.

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.

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

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?


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