[RESEND PATCH v11 02/18] drm: bridge: panel: Add devm_drm_of_dsi_get_bridge helper

[Maxime Ripard]

On Tue, Jan 31, 2023 at 07:17:50PM +0530, Jagan Teki wrote:
> On Tue, Jan 31, 2023 at 6:16 PM Maxime Ripard <maxime at cerno.tech> wrote:
> >
> > On Mon, Jan 30, 2023 at 06:54:54PM +0530, Jagan Teki wrote:
> > > On Mon, Jan 30, 2023 at 6:26 PM Maxime Ripard <maxime at cerno.tech> wrote:
> > > >
> > > > On Fri, Jan 27, 2023 at 11:09:26PM +0530, Jagan Teki wrote:
> > > > > Hi,
> > > > >
> > > > > On Thu, Jan 26, 2023 at 8:48 PM Jagan Teki <jagan at amarulasolutions.com> wrote:
> > > > > >
> > > > > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime at cerno.tech> wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Mon, Jan 23, 2023 at 08:41:56PM +0530, Jagan Teki wrote:
> > > > > > > > Add devm OF helper to return the next DSI bridge in the chain.
> > > > > > > >
> > > > > > > > Unlike general bridge return helper devm_drm_of_get_bridge, this
> > > > > > > > helper uses the dsi specific panel_or_bridge helper to find the
> > > > > > > > next DSI device in the pipeline.
> > > > > > > >
> > > > > > > > Helper lookup a given child DSI node or a DT node's port and
> > > > > > > > endpoint number, find the connected node and return either
> > > > > > > > the associated struct drm_panel or drm_bridge device.
> > > > > > >
> > > > > > > I'm not sure that using a device managed helper is the right choice
> > > > > > > here. The bridge will stay longer than the backing device so it will
> > > > > > > create a use-after-free. You should probably use a DRM-managed action
> > > > > > > here instead.
> > > > > >
> > > > > > Thanks for the comments. If I understand correctly we can use
> > > > > > drmm_panel_bridge_add instead devm_drm_panel_bridge_add once we found
> > > > > > the panel or bridge - am I correct?
> > > > >
> > > > > Look like it is not possible to use DRM-managed action helper here as
> > > > > devm_drm_of_dsi_get_bridge is calling from the DSI host attach hook in
> > > > > which we cannot find drm_device pointer (as drm_device pointer is
> > > > > mandatory for using DRM-managed action).
> > > > > https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1545
> > > > >
> > > > > Please check and correct me if I mentioned any incorrect details.
> > > >
> > > > You shouldn't call that function from attach anyway:
> > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges
> > >
> > > True, If I remember we have bridges earlier to find the downstream
> > > bridge/panels from the bridge ops and attach the hook, if that is the
> > > case maybe we can use this DRM-managed action as we can get the
> > > drm_device pointer from the bridge pointer.
> >
> > I'm not quite sure what you mean. You shouldn't retrieve the bridge from
> > the attach hook but from the probe / bind ones. If that's not working
> > for you, this is a bug in the documentation we should address.
> 
> Something like this, afterward the design has updated to move the
> panel or bridge found to be in host attach.
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/bridge/nwl-dsi.c?h=v5.10#n911

What are you pointing to exactly, it's not a MIPI-DSI host attach,
that's a bridge attach, you have access to the DRM device there.

> >
> > > So, what is the best we can do here, add any TODO comment to follow up
> > > the same in the future or something else, please let me know?
> >
> > Make it work in a safe way, as described in the documentation?
> 
> Yeah, it is a clear deadlock. It is not possible to use DM-managed
> action helper in host attach as there is no drm_device pointer and we
> cannot move panel or bridge finding out of host attach as per design
> documentation. I'm thinking of go-ahead with adding TODO for adding
> the safe way with an existing patch. Please let me know.

I've been telling you for three mails now that it's not acceptable. So,
again, no, it's not acceptable. Do something else and follow the
documentation instead.

Maxime