Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hi Dmitry,

On Thu, 16 Jan 2025 12:56:25 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

[...]

> > Idea 3: 
> > 
> > The idea is that if the panel driver framework always creates a panel
> > bridge, it will never need to be created on the fly automagically by
> > its consumers, so the whole problem would disappear. It also would be
> > better modeling the hardware: still wrapping a panel with a drm_bridge
> > that does not exist in the hardware, but at least having it created by
> > the provider driver and not by the consumer driver which happens to
> > look for it.  
> 
> I think this is the best option up to now.

Thanks for sharing your opinion. However a few points are not clear to
me, see below.

> > This looks like a promising and simple idea, so I tried a quick
> > implementation:
> > 
> >  void drm_panel_init(struct drm_panel *panel, struct device *dev,
> >                     const struct drm_panel_funcs *funcs, int connector_type)
> >  {
> > +       struct drm_bridge *bridge;
> > +
> >         INIT_LIST_HEAD(&panel->list);
> >         INIT_LIST_HEAD(&panel->followers);
> >         mutex_init(&panel->follower_lock);
> >         panel->dev = dev;
> >         panel->funcs = funcs;
> >         panel->connector_type = connector_type;
> > +
> > +       bridge = devm_drm_panel_bridge_add(panel->dev, panel);
> > +       WARN_ON(!bridge);
> >  }
> > 
> > This is somewhat working but it requires more work because:
> > 
> >  * as it is, it creates a circular dependency between drm_panel and the
> >    panel bridge, and modular builds will fail:
> > 
> >      depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
> > 
> >  * The panel bridge implementation should be made private to the panel
> >    driver only (possibly helping to solve the previous issue?)  
> 
> Or merge drm_panel.c into bridge/panel.c.

Not sure here you mean exactly what you wrote, or the other way around.
It looks more correct to me that the panel core (drm_panel.c) starts
exposing a bridge now, and not that the panel bridge which is just one
of many bridge drivers starts handling all the panel-related stuff.

> The panel bridge already
> exports non-standard API, so it should be fine to extend / change that
> API. Likewise we might move drm_panel.c to drm_kms_helper.o, also
> resolving the loop.

Again I'm not following I'm afraid. It would seem more logical to me to
move things from the helper into drm.o as they become more necessary
for common cases, rather than moving things out to a helper module and
then force all panel drivers to depend on the helper module.

Apologies, I'm perhaps not following your reasoning here. :(

> There will be a need for a Kconfig update in both
> cases.
> 
> >  * Many drivers currently call devm_drm_panel_bridge_add() directly,
> >    they should probably call drm_of_get_bridge instead  
> 
> I'd say, make it a stub that calls drm_of_get_bridge() with a possible
> deprecation warning.

I get the idea, but it would need to be implemented differently because
drm_of_get_bridge() calls devm_drm_panel_bridge_add(). They would loop
into each other. I think we might simply add a check at the beginning
of drm_panel_bridge_add_typed(), as in (pseudocode):

drm_panel_bridge_add_typed(struct drm_panel *panel, ...)
{
    if (panel_has_a_panel_bridge(panel))
        return panel->panel_bridge.bridge;

    // existing code
}

But that reopens the same issue: drm_panel_bridge_add_typed() would not
always call drm_bridge_add(), so the caller doesn't know how to dispose
of the returned pointer.

I think idea 3 can only work if the code understands that the panel
bridge is created by the panel and they _never_ have to create it
themselves. So handling the transition for all drivers would be quite
painful.

> >  * drm_of_find_panel_or_bridge() should disappear: other drivers would
> >    just look for a bridge  
> 
> Please keep it for now.
> 
> Some of the drivers think that it returns literally panel-or-bridge (but
> not both). However if panel bridge is already created, it will return
> both. So those drivers need to be updated.

I really believe it never returns both. The *bridge is set only when
ret != 0 here [0], which can happen only if a panel was not found.
Despite the slightly intricate logic of that function, I don't see how
it could return both in its current implementation.

[0]
https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/gpu/drm/drm_of.c#L273

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com