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 <[email protected]> wrote:
>> From: Sui Jingfeng <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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.
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.
>> +
>> + 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"
>> };
>
On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <[email protected]> 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 <[email protected]> wrote:
> >> From: Sui Jingfeng <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >> 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.
>
>
> >> +
> >> + 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"
> >> };
> >
--
With best wishes
Dmitry
Hi,
On 2023/11/16 17:30, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <[email protected]> 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 <[email protected]> wrote:
>>>> From: Sui Jingfeng <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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"
>>>> };
>
>
On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
>
> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
> > On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <[email protected]> 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 <[email protected]> wrote:
> >>>> From: Sui Jingfeng <[email protected]>
> >>>>
> >>>> 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 <[email protected]>
> >>>> ---
> >>>> 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.
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 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).
>
>
> >>
> >>>> +
> >>>> + 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"
> >>>> };
> >
> >
--
With best wishes
Dmitry
Hi,
On 2023/11/16 19:19, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <[email protected]> wrote:
>> Hi,
>>
>>
>> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
>>> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <[email protected]> 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 <[email protected]> wrote:
>>>>>> From: Sui Jingfeng <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> ---
>>>>>> 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"
>>>>>> };
>>>
>
>
On Thu, 23 Nov 2023 at 07:05, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
>
> On 2023/11/16 19:19, Dmitry Baryshkov wrote:
> > On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <[email protected]> wrote:
> >> Hi,
> >>
> >>
> >> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
> >>> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <[email protected]> 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 <[email protected]> wrote:
> >>>>>> From: Sui Jingfeng <[email protected]>
> >>>>>>
> >>>>>> 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 <[email protected]>
> >>>>>> ---
> >>>>>> 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.
>
> 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"
> >>>>>> };
> >>>
> >
> >
--
With best wishes
Dmitry
On 23/11/2023 09:08, Dmitry Baryshkov wrote:
> On Thu, 23 Nov 2023 at 07:05, Sui Jingfeng <[email protected]> wrote:
>>
>> Hi,
>>
>>
>> On 2023/11/16 19:19, Dmitry Baryshkov wrote:
>>> On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <[email protected]> wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
>>>>> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <[email protected]> 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 <[email protected]> wrote:
>>>>>>>> From: Sui Jingfeng <[email protected]>
>>>>>>>>
>>>>>>>> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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"
>>>>>>>> };
>>>>>
>>>
>>>
>
>
>
Hi,
On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flag are 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.
Then, my problem is that do we allow create a drm_connector instance in drm bridge
driver nowadays?
On Thu, 23 Nov 2023 at 17:41, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
>
> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
> >> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flag are 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.
>
>
> Then, my problem is that do we allow create a drm_connector instance in drm bridge
> driver nowadays?
Yes, if there is no DRM_BRIDGE_ATTACH_NO_CONNECTOR. But that's deprecated IMO.
--
With best wishes
Dmitry
Hi,
On 2023/11/23 21:39, Neil Armstrong wrote:
> On 23/11/2023 09:08, Dmitry Baryshkov wrote:
>> On Thu, 23 Nov 2023 at 07:05, Sui Jingfeng <[email protected]>
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 2023/11/16 19:19, Dmitry Baryshkov wrote:
>>>> On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <[email protected]>
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
>>>>>> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng
>>>>>> <[email protected]> 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
>>>>>>>> <[email protected]> wrote:
>>>>>>>>> From: Sui Jingfeng <[email protected]>
>>>>>>>>>
>>>>>>>>> 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 <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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.
>
But you don't mention the prerequisite, there have difference between
display bridge devices *inside* of the SoC and display bridge devices
*outside* of the SoC. IT66121 is a onboard device. Suppose you are the
hardware vendor ITE, you definitely want your chip can be used on any
platform. The model probably works well on ARM, but on X86 platform
there no DT support, the abstract for the onboard device becomes pale.
>>
>>>
>>> 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"
>>>>>>>>> };
>>>>>>
>>>>
>>>>
>>
>>
>>
>
Hi,
On 2023/11/24 00:06, Dmitry Baryshkov wrote:
> On Thu, 23 Nov 2023 at 17:41, Sui Jingfeng <[email protected]> wrote:
>> Hi,
>>
>>
>> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>>>> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flag are 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.
>>
>> Then, my problem is that do we allow create a drm_connector instance in drm bridge
>> driver nowadays?
> Yes, if there is no DRM_BRIDGE_ATTACH_NO_CONNECTOR. But that's deprecated IMO.
Then can you read the code in bridge/ti-tfp410.c please?
The tfp410_attach() function will create drm_connector instance manually
if the conditional (flags == 0) is true.
Hi,
On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>>> 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.
Can you also read the code in the bridge/lontium-lt8912.c please?
when flags == 0 is true, the lt8912 driver will just create
a drm_connector instance in the drm bridge drivers. The behavior
is similar with this patch in the perspective of spirit.
And the most important thing is that no matter what the flag the upstream
port is passed, lt8912 just always pass the DRM_BRIDGE_ATTACH_NO_CONNECTOR
flags to the next bridge. Does this count as a kind of ignore? or
This is to say that both the lt8912 and the tfp410 drm bridge drivers are
allowing create a drm_connector manually in drm bridge drivers. They didn't
being asked to move the drm_connector related code to display controller
driver. I don't know why I can't follow this way?
Do you really read the code before do comments or I failed to understand something?
On Thu, 23 Nov 2023 at 19:04, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
>
> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
> >>> 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.
>
>
> Can you also read the code in the bridge/lontium-lt8912.c please?
> when flags == 0 is true, the lt8912 driver will just create
> a drm_connector instance in the drm bridge drivers. The behavior
> is similar with this patch in the perspective of spirit.
>
> And the most important thing is that no matter what the flag the upstream
> port is passed, lt8912 just always pass the DRM_BRIDGE_ATTACH_NO_CONNECTOR
> flags to the next bridge. Does this count as a kind of ignore? or
>
> This is to say that both the lt8912 and the tfp410 drm bridge drivers are
> allowing create a drm_connector manually in drm bridge drivers. They didn't
> being asked to move the drm_connector related code to display controller
> driver. I don't know why I can't follow this way?
This is called 'legacy'.
>
> Do you really read the code before do comments or I failed to understand something?
--
With best wishes
Dmitry
Hi,
On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>> 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.
What you said is just for the more complex uses case. I can't agree, sorry.
By choosing the word "canonical design", I means that the most frequently used
cases in practice are the canonical design, 95+% motherboards I have seen has
only one *onboard* display bridges chip. For my driver, I abstract the internal
(inside of the chip) encoder as drm_encoder and abstract the external TX chip as
drm_bridge, this design still works very well.
Originally, I means that this is a concept of the hardware design.
You are wrong even through in the software design context, the
transparent simple drm bridge drivers(simple-bridge.c) also *allow*
to create drm connector manually. I don't think I need to emulate
more example, please read the code by youself.
Canonical or not canonical is not a question to argue, if other programmers
are allowed to do such kind of abstraction, I should also allowed too. Thanks.
On Fri, Nov 24, 2023 at 01:52:26AM +0800, Sui Jingfeng wrote:
> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
> > > 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.
>
> What you said is just for the more complex uses case. I can't agree, sorry.
>
> By choosing the word "canonical design", I means that the most frequently used
> cases in practice are the canonical design, 95+% motherboards I have seen has
> only one *onboard* display bridges chip. For my driver, I abstract the internal
> (inside of the chip) encoder as drm_encoder and abstract the external TX chip as
> drm_bridge, this design still works very well.
>
>
> Originally, I means that this is a concept of the hardware design.
> You are wrong even through in the software design context, the
> transparent simple drm bridge drivers(simple-bridge.c) also *allow*
> to create drm connector manually. I don't think I need to emulate
> more example, please read the code by youself.
Ok. That's it. We've been patient long enough. You have been given a
review and a list of things to fix for your driver to be merged. Whether
you follow them or not is your decision.
We won't tolerate insulting comments though.
Maxime
Hi,
On 2023/11/24 15:38, Maxime Ripard wrote:
> On Fri, Nov 24, 2023 at 01:52:26AM +0800, Sui Jingfeng wrote:
>> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>>>> 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.
>> What you said is just for the more complex uses case. I can't agree, sorry.
>>
>> By choosing the word "canonical design", I means that the most frequently used
>> cases in practice are the canonical design, 95+% motherboards I have seen has
>> only one *onboard* display bridges chip. For my driver, I abstract the internal
>> (inside of the chip) encoder as drm_encoder and abstract the external TX chip as
>> drm_bridge, this design still works very well.
>>
>>
>> Originally, I means that this is a concept of the hardware design.
>> You are wrong even through in the software design context, the
>> transparent simple drm bridge drivers(simple-bridge.c) also *allow*
>> to create drm connector manually. I don't think I need to emulate
>> more example, please read the code by youself.
'emulate' -> 'enumerate'
> Ok. That's it. We've been patient long enough. You have been given a
> review and a list of things to fix for your driver to be merged.
This series is not relevant to my driver, can we please *limit*
the discussion to this series?
> Whether
> you follow them or not is your decision.
I'm not saying that I will not follow, just to make sure what's solution is you want.
I need discussion to figure out.
> We won't tolerate insulting comments though.
There is *no* insulting, please don't misunderstanding before *sufficient* communication, OK?
Originally, I thought Dmitry may ignore(or overlook) what is the current status.
> Maxime
On Fri, Nov 24, 2023 at 03:51:00PM +0800, Sui Jingfeng wrote:
> Hi,
>
> On 2023/11/24 15:38, Maxime Ripard wrote:
> > On Fri, Nov 24, 2023 at 01:52:26AM +0800, Sui Jingfeng wrote:
> > > On 2023/11/23 16:08, Dmitry Baryshkov wrote:
> > > > > 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.
> > > What you said is just for the more complex uses case. I can't agree, sorry.
> > >
> > > By choosing the word "canonical design", I means that the most frequently used
> > > cases in practice are the canonical design, 95+% motherboards I have seen has
> > > only one *onboard* display bridges chip. For my driver, I abstract the internal
> > > (inside of the chip) encoder as drm_encoder and abstract the external TX chip as
> > > drm_bridge, this design still works very well.
> > >
> > >
> > > Originally, I means that this is a concept of the hardware design.
> > > You are wrong even through in the software design context, the
> > > transparent simple drm bridge drivers(simple-bridge.c) also *allow*
> > > to create drm connector manually. I don't think I need to emulate
> > > more example, please read the code by youself.
>
> 'emulate' -> 'enumerate'
>
> > Ok. That's it. We've been patient long enough. You have been given a
> > review and a list of things to fix for your driver to be merged.
>
> This series is not relevant to my driver, can we please *limit* the
> discussion to this series?
Right, I conflated the two, I meant this series, or the general goal to
enable that bridge with your driver. The rest of the driver is of course
unaffected.
> > Whether you follow them or not is your decision.
>
> I'm not saying that I will not follow, just to make sure what's
> solution is you want. I need discussion to figure out.
You had direct, repeated, feedback on that already by a maintainer and
one of the most experienced dev and reviewer on bridges. If you need
more guidance, you can definitely ask questions, but asking questions
and telling them they are wrong is very different.
> > We won't tolerate insulting comments though.
>
> There is *no* insulting, please don't misunderstanding before
> *sufficient* communication, OK? Originally, I thought Dmitry may
> ignore(or overlook) what is the current status.
Saying to someone maintaining and/or reviewing that code for years now
that they are wrong and should go read the code is insulting.
Maxime
Hi,
On 2023/11/24 16:13, Maxime Ripard wrote:
> On Fri, Nov 24, 2023 at 03:51:00PM +0800, Sui Jingfeng wrote:
>> Hi,
>>
>> On 2023/11/24 15:38, Maxime Ripard wrote:
>>> On Fri, Nov 24, 2023 at 01:52:26AM +0800, Sui Jingfeng wrote:
>>>> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>>>>>> 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.
>>>> What you said is just for the more complex uses case. I can't agree, sorry.
>>>>
>>>> By choosing the word "canonical design", I means that the most frequently used
>>>> cases in practice are the canonical design, 95+% motherboards I have seen has
>>>> only one *onboard* display bridges chip. For my driver, I abstract the internal
>>>> (inside of the chip) encoder as drm_encoder and abstract the external TX chip as
>>>> drm_bridge, this design still works very well.
>>>>
>>>>
>>>> Originally, I means that this is a concept of the hardware design.
>>>> You are wrong even through in the software design context, the
>>>> transparent simple drm bridge drivers(simple-bridge.c) also *allow*
>>>> to create drm connector manually. I don't think I need to emulate
>>>> more example, please read the code by youself.
>> 'emulate' -> 'enumerate'
>>
>>> Ok. That's it. We've been patient long enough. You have been given a
>>> review and a list of things to fix for your driver to be merged.
>> This series is not relevant to my driver, can we please *limit* the
>> discussion to this series?
> Right, I conflated the two, I meant this series, or the general goal to
> enable that bridge with your driver. The rest of the driver is of course
> unaffected.
>
>>> Whether you follow them or not is your decision.
>> I'm not saying that I will not follow, just to make sure what's
>> solution is you want. I need discussion to figure out.
> You had direct, repeated, feedback on that already by a maintainer and
> one of the most experienced dev and reviewer on bridges. If you need
> more guidance, you can definitely ask questions, but asking questions
> and telling them they are wrong is very different.
>
>>> We won't tolerate insulting comments though.
>> There is *no* insulting, please don't misunderstanding before
>> *sufficient* communication, OK? Originally, I thought Dmitry may
>> ignore(or overlook) what is the current status.
> Saying to someone maintaining and/or reviewing that code for years now
> that they are wrong and should go read the code is insulting.
OK, I will remind my written words in the future.
I will back to investigate for a period of time.
Thanks a lot for reviewing.
> Maxime
On 2023/11/24 16:13, Maxime Ripard wrote:
> On Fri, Nov 24, 2023 at 03:51:00PM +0800, Sui Jingfeng wrote:
>> Hi,
>>
>> On 2023/11/24 15:38, Maxime Ripard wrote:
>>> On Fri, Nov 24, 2023 at 01:52:26AM +0800, Sui Jingfeng wrote:
>>>> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>>>>>> 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.
>>>> What you said is just for the more complex uses case. I can't agree, sorry.
>>>>
>>>> By choosing the word "canonical design", I means that the most frequently used
>>>> cases in practice are the canonical design, 95+% motherboards I have seen has
>>>> only one *onboard* display bridges chip. For my driver, I abstract the internal
>>>> (inside of the chip) encoder as drm_encoder and abstract the external TX chip as
>>>> drm_bridge, this design still works very well.
>>>>
>>>>
>>>> Originally, I means that this is a concept of the hardware design.
>>>> You are wrong even through in the software design context, the
>>>> transparent simple drm bridge drivers(simple-bridge.c) also *allow*
>>>> to create drm connector manually. I don't think I need to emulate
>>>> more example, please read the code by youself.
>> 'emulate' -> 'enumerate'
>>
>>> Ok. That's it. We've been patient long enough. You have been given a
>>> review and a list of things to fix for your driver to be merged.
>> This series is not relevant to my driver, can we please *limit* the
>> discussion to this series?
> Right, I conflated the two, I meant this series, or the general goal to
> enable that bridge with your driver. The rest of the driver is of course
> unaffected.
>
>>> Whether you follow them or not is your decision.
>> I'm not saying that I will not follow, just to make sure what's
>> solution is you want. I need discussion to figure out.
> You had direct, repeated, feedback on that already by a maintainer and
> one of the most experienced dev and reviewer on bridges. If you need
> more guidance, you can definitely ask questions, but asking questions
> and telling them they are wrong is very different.
>
>>> We won't tolerate insulting comments though.
>> There is *no* insulting, please don't misunderstanding before
>> *sufficient* communication, OK? Originally, I thought Dmitry may
>> ignore(or overlook) what is the current status.
> Saying to someone maintaining and/or reviewing that code for years now
> that they are wrong and should go read the code is insulting.
Back to that time, I'm focus on kindly technique debating, this is a kind
of defensive reply for the patch. This is a kind of remind in case of ignores.
Probably a bit of negative, but please don't misunderstanding as insult anyway.
Dmitry, really thanks a lot for the instructs, I have learned a lot while
talking with you. I will back to try mentioned method.