Hi Laurent,
On 03.09.2020 11:59, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Thu, Sep 03, 2020 at 11:40:58AM +0200, Andrzej Hajda wrote:
>> On 26.07.2020 22:33, Sam Ravnborg wrote:
>>> Prepare the tc358764 bridge driver for use in a chained setup by
>>> replacing direct use of drm_panel with drm_panel_bridge support.
>>>
>>> The bridge panel will use the connector type reported by the panel,
>>> where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS.
>>>
>>> The tc358764 did not any additional info the the connector so the
>>> connector creation is passed to the bridge panel driver.
>>>
>>> v3:
>>> - Merge with patch to make connector creation optional to avoid
>>> creating two connectors (Laurent)
>>> - Pass connector creation to bridge panel, as this bridge driver
>>> did not add any extra info to the connector.
>>> - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.
>>>
>>> v2:
>>> - Use PTR_ERR_OR_ZERO() (kbuild test robot)
>>>
>>> Signed-off-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
>>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> Cc: kbuild test robot <lkp@xxxxxxxxx>
>>> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
>>> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>>> Cc: Jonas Karlman <jonas@xxxxxxxxx>
>>> Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
>>> ---
>>> drivers/gpu/drm/bridge/tc358764.c | 107 +++++-------------------------
>>> 1 file changed, 16 insertions(+), 91 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
>>> index a277739fab58..fdde4cfdc724 100644
>>> --- a/drivers/gpu/drm/bridge/tc358764.c
>>> +++ b/drivers/gpu/drm/bridge/tc358764.c
>>> @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = {
>>> struct tc358764 {
>>> struct device *dev;
>>> struct drm_bridge bridge;
>>> - struct drm_connector connector;
>>> struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
>>> struct gpio_desc *gpio_reset;
>>> - struct drm_panel *panel;
>>> + struct drm_bridge *panel_bridge;
>>> int error;
>>> };
>>>
>>> @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge)
>>> return container_of(bridge, struct tc358764, bridge);
>>> }
>>>
>>> -static inline
>>> -struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
>>> -{
>>> - return container_of(connector, struct tc358764, connector);
>>> -}
>>> -
>>> static int tc358764_init(struct tc358764 *ctx)
>>> {
>>> u32 v = 0;
>>> @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx)
>>> usleep_range(1000, 2000);
>>> }
>>>
>>> -static int tc358764_get_modes(struct drm_connector *connector)
>>> -{
>>> - struct tc358764 *ctx = connector_to_tc358764(connector);
>>> -
>>> - return drm_panel_get_modes(ctx->panel, connector);
>>> -}
>>> -
>>> -static const
>>> -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
>>> - .get_modes = tc358764_get_modes,
>>> -};
>>> -
>>> -static const struct drm_connector_funcs tc358764_connector_funcs = {
>>> - .fill_modes = drm_helper_probe_single_connector_modes,
>>> - .destroy = drm_connector_cleanup,
>>> - .reset = drm_atomic_helper_connector_reset,
>>> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>>> - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>> -};
>>> -
>>> -static void tc358764_disable(struct drm_bridge *bridge)
>>> -{
>>> - struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>> - int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
>>> -
>>> - if (ret < 0)
>>> - dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
>>> -}
>>> -
>>> static void tc358764_post_disable(struct drm_bridge *bridge)
>>> {
>>> struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>> int ret;
>>>
>>> - ret = drm_panel_unprepare(ctx->panel);
>>> - if (ret < 0)
>>> - dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
>>
>> Using this bridge_panel thing you reverse order of hw
>> initialization/de-initialization, this is incorrect.
>>
>> For example:
>>
>> - panel_unprepare should be called before tc35* turn off,
>>
>> - panel_prepare should be called after tc35* on.
>>
>> This is why I avoid the whole "bridge chaining" - it enforces ridiculous
>> order of initialization.
>>
>>
>>> tc358764_reset(ctx);
>>> usleep_range(10000, 15000);
>>> ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>>> @@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge)
>>> ret = tc358764_init(ctx);
>>> if (ret < 0)
>>> dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
>>> - ret = drm_panel_prepare(ctx->panel);
>>> - if (ret < 0)
>>> - dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
>>> -}
>>> -
>>> -static void tc358764_enable(struct drm_bridge *bridge)
>>> -{
>>> - struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>> - int ret = drm_panel_enable(ctx->panel);
>>> -
>>> - if (ret < 0)
>>> - dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
>>> }
>>>
>>> static int tc358764_attach(struct drm_bridge *bridge,
>>> enum drm_bridge_attach_flags flags)
>>> -{
>>> - struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>> - struct drm_device *drm = bridge->dev;
>>> - int ret;
>>> -
>>> - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>>> - DRM_ERROR("Fix bridge driver to make connector optional!");
>>> - return -EINVAL;
>>> - }
>>> -
>>> - ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
>>> - ret = drm_connector_init(drm, &ctx->connector,
>>> - &tc358764_connector_funcs,
>>> - DRM_MODE_CONNECTOR_LVDS);
>>> - if (ret) {
>>> - DRM_ERROR("Failed to initialize connector\n");
>>> - return ret;
>>> - }
>>> -
>>> - drm_connector_helper_add(&ctx->connector,
>>> - &tc358764_connector_helper_funcs);
>>> - drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
>>> - drm_panel_attach(ctx->panel, &ctx->connector);
>>> - ctx->connector.funcs->reset(&ctx->connector);
>>
>> I guess lack of calling .reset here is direct cause of WARN reported by
>> Marek.
>>
>>
>> Summarizing my findings:
>>
>> 1. drm_panel_bridge does not fit to this scenario - it relays on 'bridge
>> chaining" which has crazy assumption that order of hw initalization in
>> the display chain follows the same fixed order of calls for all hw.
> This would need to be addressed in the bridge core. I don't want to go
> back to manual chaining of operations, that opens the door to creating
> incompatibilities between bridges and display controllers. The pre/post
> enable/disable operations probably need to be better defined, and if a
> sink requires a smaller granularity, then new operations need to be
> added.
Bigger granularity of operations is source of incompatibilities. We have
already multiple issues with only two operations - pre_enable,
post_enable, developers are confused what put where, especially if they
can test bridge driver only with only fixed chain determined by the
platform they are working on.
Adding new operations will make things worse. On the other hand explicit
calling ops of downstream device has following advantages:
- we see explicitly how the bridge and its sink is initialized, it is
even easier to compare it with docs,
- in case the stream is split or duplicated to two or more sinks, and/or
eventually joined then later, it is much easier and straightforward to
program it with explicit ops. With bridge chaining it is impossible
without workarounds - all the cases with dual DSI etc.
Regards
Andrzej
>> 2. tc35* bridge allocates/deallocates connector dynamically - to safely
>> handle drivers load/unload, and to avoid multiple deferred probe issues
>> , drm_panel_bridge does not support it.
>>
>> This and previous patch violates both points.
>>
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static void tc358764_detach(struct drm_bridge *bridge)
>>> {
>>> struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>>
>>> - drm_panel_detach(ctx->panel);
>>> - ctx->panel = NULL;
>>> + return drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
>>> + bridge, flags);
>>> }
>>>
>>> static const struct drm_bridge_funcs tc358764_bridge_funcs = {
>>> - .disable = tc358764_disable,
>>> .post_disable = tc358764_post_disable,
>>> - .enable = tc358764_enable,
>>> .pre_enable = tc358764_pre_enable,
>>> .attach = tc358764_attach,
>>> - .detach = tc358764_detach,
>>> };
>>>
>>> static int tc358764_parse_dt(struct tc358764 *ctx)
>>> {
>>> + struct drm_bridge *panel_bridge;
>>> struct device *dev = ctx->dev;
>>> + struct drm_panel *panel;
>>> int ret;
>>>
>>> ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>>> @@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx)
>>> return PTR_ERR(ctx->gpio_reset);
>>> }
>>>
>>> - ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
>>> - NULL);
>>> - if (ret && ret != -EPROBE_DEFER)
>>> - dev_err(dev, "cannot find panel (%d)\n", ret);
>>> + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
>>> + if (ret)
>>> + return ret;
>>>
>>> - return ret;
>>> + panel_bridge = devm_drm_panel_bridge_add(dev, panel);
>>> +
>>> + if (IS_ERR(panel_bridge))
>>> + return PTR_ERR(panel_bridge);
>>> +
>>> + ctx->panel_bridge = panel_bridge;
>>> + return 0;
>>> }
>>>
>>> static int tc358764_configure_regulators(struct tc358764 *ctx)
>>> @@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi)
>>> return ret;
>>>
>>> ctx->bridge.funcs = &tc358764_bridge_funcs;
>>> + ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS;
>>> ctx->bridge.of_node = dev->of_node;
>>>
>>> drm_bridge_add(&ctx->bridge);
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
