| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <121163c9-0d56-47ad-a12e-e67390fef2b4@linux.dev>
Date: Thu, 16 Nov 2023 18:12:55 +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 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).
>>
>>>> +
>>>> + 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