[PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib
Neil Armstrong
neil.armstrong at linaro.org
Thu Nov 23 13:39:59 UTC 2023
On 23/11/2023 09:08, Dmitry Baryshkov wrote:
> On Thu, 23 Nov 2023 at 07:05, Sui Jingfeng <sui.jingfeng at linux.dev> wrote:
>>
>> Hi,
>>
>>
>> On 2023/11/16 19:19, Dmitry Baryshkov wrote:
>>> On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <sui.jingfeng at linux.dev> wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
>>>>> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <sui.jingfeng at linux.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 at linux.dev> wrote:
>>>>>>>> From: Sui Jingfeng <suijingfeng at loongson.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 at loongson.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"
>>>>>>>> };
>>>>>
>>>
>>>
>
>
>
More information about the dri-devel
mailing list