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

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.

Cc: Maxime Ripard <mripard@kernel.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v11:
- none
Changes for v10:
- new patch

 drivers/gpu/drm/bridge/panel.c | 34 ++++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h       |  2 ++
 2 files changed, 36 insertions(+)
  

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.

Maxime
  

On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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?

Jagan.
  

Hi,

On Thu, Jan 26, 2023 at 8:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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.

Thanks,
Jagan.
  

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@amarulasolutions.com> wrote:
> >
> > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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

Maxime
  

On Thu, Jan 26, 2023 at 08:48:48PM +0530, Jagan Teki wrote:
> On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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?

It's not that we can, it's that the devm_panel_bridge_add is unsafe:
when the module is removed the device will go away and all the devm
resources freed, but the DRM device sticks around until the last
application with a fd open closes that fd.

Maxime
  

On Mon, Jan 30, 2023 at 6:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Thu, Jan 26, 2023 at 08:48:48PM +0530, Jagan Teki wrote:
> > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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?
>
> It's not that we can, it's that the devm_panel_bridge_add is unsafe:
> when the module is removed the device will go away and all the devm
> resources freed, but the DRM device sticks around until the last
> application with a fd open closes that fd.

Thanks for the details. I think this is the reason you have introduced
this DRM-managed action helper - drmm_of_get_bridge. Initially, i
thought of adding similar, but as you are aware it is not possible to
call it from the host attach.

Jagan.
  

On Mon, Jan 30, 2023 at 6:26 PM Maxime Ripard <maxime@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@amarulasolutions.com> wrote:
> > >
> > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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. 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?

Thanks,
Jagan.
  

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@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@amarulasolutions.com> wrote:
> > > >
> > > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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.

> 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?

Maxime
  

On Tue, Jan 31, 2023 at 6:16 PM Maxime Ripard <maxime@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@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@amarulasolutions.com> wrote:
> > > > >
> > > > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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

>
> > 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.

Thanks,
Jagan.
  

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@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@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@amarulasolutions.com> wrote:
> > > > > >
> > > > > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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
  

On Tue, Jan 31, 2023 at 7:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> 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@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@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@amarulasolutions.com> wrote:
> > > > > > >
> > > > > > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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.

Yes, what I'm saying here is we can have the option to use a DRM
device pointer so finding the panel or bridge by using a DRM-managed
action helper can be possible rather than host attach.

Something like this,

struct drm_bridge *drmm_of_dsi_get_bridge(struct drm_device *drm,
                                      struct device_node *np,
                                      u32 port, u32 endpoint)
{
        struct drm_bridge *bridge;
        struct drm_panel *panel;
        int ret;

        ret = drm_of_dsi_find_panel_or_bridge(np, port, endpoint,
                                          &panel, &bridge);
        if (ret)
                return ERR_PTR(ret);

        if (panel)
                bridge = drmm_panel_bridge_add(drm, panel);

        return bridge;
}

static int nwl_dsi_bridge_attach(struct drm_bridge *bridge, enum
drm_bridge_attach_flags flags)
{
        struct nwl_dsi *dsi = bridge_to_dsi(bridge);
        struct drm_bridge *bridge;
        int ret;

       bridge = drmm_of_dsi_get_bridge(bridge->dev, dsi->dev->of_node, 1, 0);
       if (IS_ERR(bridge))
           ret = PTR_ERR(dsi->out_bridge);

       return drm_bridge_attach(bridge->encoder, dsi->panel_bridge,
bridge, flags);
}

static const struct drm_bridge_funcs nwl_dsi_bridge_funcs = {
      .attach     = nwl_dsi_bridge_attach,
};

>
> > >
> > > > 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.

Ohh, look like I didn't get this in the first e-mail. Okay, now I got
it, thanks. On the other hand, this series recurring for more than a
year, so to merge things go quickly can you please suggest some
solution based on this discussion?

Thanks,
Jagan.
  

Hi Maxime,

On Mon, Jan 30, 2023 at 6:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Thu, Jan 26, 2023 at 08:48:48PM +0530, Jagan Teki wrote:
> > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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?
>
> It's not that we can, it's that the devm_panel_bridge_add is unsafe:
> when the module is removed the device will go away and all the devm
> resources freed, but the DRM device sticks around until the last
> application with a fd open closes that fd.

Would you please check this, Here I'm trying to do

1. find a panel or bridge
2. if panel add it as a panel bridge
3. add DRM-managed action with the help of bridge->dev after step 2.

Didn't test the behavior, just wanted to check whether it can be a
possibility to use bridge->dev as this dev is assigned with
encoder->dev during the bridge attach the chain. Please check and let
me know.

struct drm_bridge *devm_drm_of_dsi_get_bridge(struct device *dev,
                                              struct device_node *np,
                                              u32 port, u32 endpoint)
{
        struct drm_bridge *bridge;
        struct drm_panel *panel;
        int ret;

        ret = drm_of_dsi_find_panel_or_bridge(np, port, endpoint,
                                              &panel, &bridge);
        if (ret)
                return ERR_PTR(ret);

        if (panel)
                bridge = devm_drm_panel_bridge_add(dev, panel);

        if (IS_ERR(bridge))
                return bridge;

        ret = drmm_add_action_or_reset(bridge->dev,
drmm_drm_panel_bridge_release,
                                       bridge);
        if (ret)
                return ERR_PTR(ret);

        return bridge;
}

Thanks,
Jagan.
  

On Thu, Feb 02, 2023 at 10:22:42PM +0530, Jagan Teki wrote:
> Hi Maxime,
> 
> On Mon, Jan 30, 2023 at 6:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Thu, Jan 26, 2023 at 08:48:48PM +0530, Jagan Teki wrote:
> > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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?
> >
> > It's not that we can, it's that the devm_panel_bridge_add is unsafe:
> > when the module is removed the device will go away and all the devm
> > resources freed, but the DRM device sticks around until the last
> > application with a fd open closes that fd.
> 
> Would you please check this, Here I'm trying to do
> 
> 1. find a panel or bridge
> 2. if panel add it as a panel bridge
> 3. add DRM-managed action with the help of bridge->dev after step 2.

The logic is sound in your patch

> Didn't test the behavior, just wanted to check whether it can be a
> possibility to use bridge->dev as this dev is assigned with
> encoder->dev during the bridge attach the chain. Please check and let
> me know.
> 
> struct drm_bridge *devm_drm_of_dsi_get_bridge(struct device *dev,
>                                               struct device_node *np,
>                                               u32 port, u32 endpoint)
> {
>         struct drm_bridge *bridge;
>         struct drm_panel *panel;
>         int ret;
> 
>         ret = drm_of_dsi_find_panel_or_bridge(np, port, endpoint,
>                                               &panel, &bridge);
>         if (ret)
>                 return ERR_PTR(ret);
> 
>         if (panel)
>                 bridge = devm_drm_panel_bridge_add(dev, panel);
> 
>         if (IS_ERR(bridge))
>                 return bridge;
> 
>         ret = drmm_add_action_or_reset(bridge->dev,
> drmm_drm_panel_bridge_release,
>                                        bridge);
>         if (ret)
>                 return ERR_PTR(ret);
> 
>         return bridge;
> }

It's the implementation that isn't. You cannot use a devm hook to
register a KMS structure, so it's not that you should add a
drmm_add_action call, it's that you shouldn't call
devm_drm_panel_bridge_add in the first place.

So either you use drm_panel_bridge_add and a custom drmm action, or you
add a drmm_panel_bridge_add function and use it.

Maxime
  

On Fri, Feb 3, 2023 at 1:56 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Thu, Feb 02, 2023 at 10:22:42PM +0530, Jagan Teki wrote:
> > Hi Maxime,
> >
> > On Mon, Jan 30, 2023 at 6:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Thu, Jan 26, 2023 at 08:48:48PM +0530, Jagan Teki wrote:
> > > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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?
> > >
> > > It's not that we can, it's that the devm_panel_bridge_add is unsafe:
> > > when the module is removed the device will go away and all the devm
> > > resources freed, but the DRM device sticks around until the last
> > > application with a fd open closes that fd.
> >
> > Would you please check this, Here I'm trying to do
> >
> > 1. find a panel or bridge
> > 2. if panel add it as a panel bridge
> > 3. add DRM-managed action with the help of bridge->dev after step 2.
>
> The logic is sound in your patch
>
> > Didn't test the behavior, just wanted to check whether it can be a
> > possibility to use bridge->dev as this dev is assigned with
> > encoder->dev during the bridge attach the chain. Please check and let
> > me know.
> >
> > struct drm_bridge *devm_drm_of_dsi_get_bridge(struct device *dev,
> >                                               struct device_node *np,
> >                                               u32 port, u32 endpoint)
> > {
> >         struct drm_bridge *bridge;
> >         struct drm_panel *panel;
> >         int ret;
> >
> >         ret = drm_of_dsi_find_panel_or_bridge(np, port, endpoint,
> >                                               &panel, &bridge);
> >         if (ret)
> >                 return ERR_PTR(ret);
> >
> >         if (panel)
> >                 bridge = devm_drm_panel_bridge_add(dev, panel);
> >
> >         if (IS_ERR(bridge))
> >                 return bridge;
> >
> >         ret = drmm_add_action_or_reset(bridge->dev,
> > drmm_drm_panel_bridge_release,
> >                                        bridge);
> >         if (ret)
> >                 return ERR_PTR(ret);
> >
> >         return bridge;
> > }
>
> It's the implementation that isn't. You cannot use a devm hook to
> register a KMS structure, so it's not that you should add a
> drmm_add_action call, it's that you shouldn't call
> devm_drm_panel_bridge_add in the first place.

I think it is because the remove action helper uses
drm_panel_bridge_remove instead of devm hook.
>
> So either you use drm_panel_bridge_add and a custom drmm action, or you
> add a drmm_panel_bridge_add function and use it.

It is not possible to use this helper as it is expecting drm_device
point that would get only if we found panel_bridge, so combined calls
here can help.

Would you please check this updated implementation and let me know?

struct drm_bridge *drmm_panel_bridge_add_nodrm(struct drm_panel *panel)
{
        struct drm_bridge *bridge;
        int ret;

        bridge = drm_panel_bridge_add_typed(panel, panel->connector_type);
        if (IS_ERR(bridge))
                return bridge;

        ret = drmm_add_action_or_reset(bridge->dev,
drmm_drm_panel_bridge_release,
                                       bridge);
        if (ret)
                return ERR_PTR(ret);

        bridge->pre_enable_prev_first = panel->prepare_prev_first;

        return bridge;
}

struct drm_bridge *drm_of_dsi_get_bridge(struct device_node *np, u32
port, u32 endpoint)
{
        struct drm_bridge *bridge;
        struct drm_panel *panel;
        int ret;

        ret = drm_of_dsi_find_panel_or_bridge(np, port, endpoint,
                                              &panel, &bridge);
        if (ret)
                return ERR_PTR(ret);

        if (panel)
                bridge = drmm_panel_bridge_add_nodrm(panel);

        return bridge;
}

Thanks,
Jagan.
  

On Fri, Feb 03, 2023 at 04:13:49PM +0530, Jagan Teki wrote:
> On Fri, Feb 3, 2023 at 1:56 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Thu, Feb 02, 2023 at 10:22:42PM +0530, Jagan Teki wrote:
> > > Hi Maxime,
> > >
> > > On Mon, Jan 30, 2023 at 6:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > On Thu, Jan 26, 2023 at 08:48:48PM +0530, Jagan Teki wrote:
> > > > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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?
> > > >
> > > > It's not that we can, it's that the devm_panel_bridge_add is unsafe:
> > > > when the module is removed the device will go away and all the devm
> > > > resources freed, but the DRM device sticks around until the last
> > > > application with a fd open closes that fd.
> > >
> > > Would you please check this, Here I'm trying to do
> > >
> > > 1. find a panel or bridge
> > > 2. if panel add it as a panel bridge
> > > 3. add DRM-managed action with the help of bridge->dev after step 2.
> >
> > The logic is sound in your patch
> >
> > > Didn't test the behavior, just wanted to check whether it can be a
> > > possibility to use bridge->dev as this dev is assigned with
> > > encoder->dev during the bridge attach the chain. Please check and let
> > > me know.
> > >
> > > struct drm_bridge *devm_drm_of_dsi_get_bridge(struct device *dev,
> > >                                               struct device_node *np,
> > >                                               u32 port, u32 endpoint)
> > > {
> > >         struct drm_bridge *bridge;
> > >         struct drm_panel *panel;
> > >         int ret;
> > >
> > >         ret = drm_of_dsi_find_panel_or_bridge(np, port, endpoint,
> > >                                               &panel, &bridge);
> > >         if (ret)
> > >                 return ERR_PTR(ret);
> > >
> > >         if (panel)
> > >                 bridge = devm_drm_panel_bridge_add(dev, panel);
> > >
> > >         if (IS_ERR(bridge))
> > >                 return bridge;
> > >
> > >         ret = drmm_add_action_or_reset(bridge->dev,
> > > drmm_drm_panel_bridge_release,
> > >                                        bridge);
> > >         if (ret)
> > >                 return ERR_PTR(ret);
> > >
> > >         return bridge;
> > > }
> >
> > It's the implementation that isn't. You cannot use a devm hook to
> > register a KMS structure, so it's not that you should add a
> > drmm_add_action call, it's that you shouldn't call
> > devm_drm_panel_bridge_add in the first place.
> 
> I think it is because the remove action helper uses
> drm_panel_bridge_remove instead of devm hook.
> >
> > So either you use drm_panel_bridge_add and a custom drmm action, or you
> > add a drmm_panel_bridge_add function and use it.
> 
> It is not possible to use this helper as it is expecting drm_device

It's definitely possible, just change the prototype of the function to
take a drm_device pointer, just like any other drmm_* function.

Maxime
  

On Fri, Feb 3, 2023 at 4:19 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Fri, Feb 03, 2023 at 04:13:49PM +0530, Jagan Teki wrote:
> > On Fri, Feb 3, 2023 at 1:56 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Thu, Feb 02, 2023 at 10:22:42PM +0530, Jagan Teki wrote:
> > > > Hi Maxime,
> > > >
> > > > On Mon, Jan 30, 2023 at 6:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > >
> > > > > On Thu, Jan 26, 2023 at 08:48:48PM +0530, Jagan Teki wrote:
> > > > > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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?
> > > > >
> > > > > It's not that we can, it's that the devm_panel_bridge_add is unsafe:
> > > > > when the module is removed the device will go away and all the devm
> > > > > resources freed, but the DRM device sticks around until the last
> > > > > application with a fd open closes that fd.
> > > >
> > > > Would you please check this, Here I'm trying to do
> > > >
> > > > 1. find a panel or bridge
> > > > 2. if panel add it as a panel bridge
> > > > 3. add DRM-managed action with the help of bridge->dev after step 2.
> > >
> > > The logic is sound in your patch
> > >
> > > > Didn't test the behavior, just wanted to check whether it can be a
> > > > possibility to use bridge->dev as this dev is assigned with
> > > > encoder->dev during the bridge attach the chain. Please check and let
> > > > me know.
> > > >
> > > > struct drm_bridge *devm_drm_of_dsi_get_bridge(struct device *dev,
> > > >                                               struct device_node *np,
> > > >                                               u32 port, u32 endpoint)
> > > > {
> > > >         struct drm_bridge *bridge;
> > > >         struct drm_panel *panel;
> > > >         int ret;
> > > >
> > > >         ret = drm_of_dsi_find_panel_or_bridge(np, port, endpoint,
> > > >                                               &panel, &bridge);
> > > >         if (ret)
> > > >                 return ERR_PTR(ret);
> > > >
> > > >         if (panel)
> > > >                 bridge = devm_drm_panel_bridge_add(dev, panel);
> > > >
> > > >         if (IS_ERR(bridge))
> > > >                 return bridge;
> > > >
> > > >         ret = drmm_add_action_or_reset(bridge->dev,
> > > > drmm_drm_panel_bridge_release,
> > > >                                        bridge);
> > > >         if (ret)
> > > >                 return ERR_PTR(ret);
> > > >
> > > >         return bridge;
> > > > }
> > >
> > > It's the implementation that isn't. You cannot use a devm hook to
> > > register a KMS structure, so it's not that you should add a
> > > drmm_add_action call, it's that you shouldn't call
> > > devm_drm_panel_bridge_add in the first place.
> >
> > I think it is because the remove action helper uses
> > drm_panel_bridge_remove instead of devm hook.
> > >
> > > So either you use drm_panel_bridge_add and a custom drmm action, or you
> > > add a drmm_panel_bridge_add function and use it.
> >
> > It is not possible to use this helper as it is expecting drm_device
>
> It's definitely possible, just change the prototype of the function to
> take a drm_device pointer, just like any other drmm_* function.

But, in my case, I only get the drm_device pointer once I found the
bridge pointer of panel_bridge but the actual bridge finding for
panel_bridge is happening in the drmm_panel_bridge_add definition.
Doesn't it redundant if I find the panel_bridge and pass drm_device
and panel pointer for calling drmm_panel_bridge_add?

Jagan.
  

On Fri, Feb 03, 2023 at 04:28:30PM +0530, Jagan Teki wrote:
> On Fri, Feb 3, 2023 at 4:19 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Fri, Feb 03, 2023 at 04:13:49PM +0530, Jagan Teki wrote:
> > > On Fri, Feb 3, 2023 at 1:56 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > On Thu, Feb 02, 2023 at 10:22:42PM +0530, Jagan Teki wrote:
> > > > > Hi Maxime,
> > > > >
> > > > > On Mon, Jan 30, 2023 at 6:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > >
> > > > > > On Thu, Jan 26, 2023 at 08:48:48PM +0530, Jagan Teki wrote:
> > > > > > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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?
> > > > > >
> > > > > > It's not that we can, it's that the devm_panel_bridge_add is unsafe:
> > > > > > when the module is removed the device will go away and all the devm
> > > > > > resources freed, but the DRM device sticks around until the last
> > > > > > application with a fd open closes that fd.
> > > > >
> > > > > Would you please check this, Here I'm trying to do
> > > > >
> > > > > 1. find a panel or bridge
> > > > > 2. if panel add it as a panel bridge
> > > > > 3. add DRM-managed action with the help of bridge->dev after step 2.
> > > >
> > > > The logic is sound in your patch
> > > >
> > > > > Didn't test the behavior, just wanted to check whether it can be a
> > > > > possibility to use bridge->dev as this dev is assigned with
> > > > > encoder->dev during the bridge attach the chain. Please check and let
> > > > > me know.
> > > > >
> > > > > struct drm_bridge *devm_drm_of_dsi_get_bridge(struct device *dev,
> > > > >                                               struct device_node *np,
> > > > >                                               u32 port, u32 endpoint)
> > > > > {
> > > > >         struct drm_bridge *bridge;
> > > > >         struct drm_panel *panel;
> > > > >         int ret;
> > > > >
> > > > >         ret = drm_of_dsi_find_panel_or_bridge(np, port, endpoint,
> > > > >                                               &panel, &bridge);
> > > > >         if (ret)
> > > > >                 return ERR_PTR(ret);
> > > > >
> > > > >         if (panel)
> > > > >                 bridge = devm_drm_panel_bridge_add(dev, panel);
> > > > >
> > > > >         if (IS_ERR(bridge))
> > > > >                 return bridge;
> > > > >
> > > > >         ret = drmm_add_action_or_reset(bridge->dev,
> > > > > drmm_drm_panel_bridge_release,
> > > > >                                        bridge);
> > > > >         if (ret)
> > > > >                 return ERR_PTR(ret);
> > > > >
> > > > >         return bridge;
> > > > > }
> > > >
> > > > It's the implementation that isn't. You cannot use a devm hook to
> > > > register a KMS structure, so it's not that you should add a
> > > > drmm_add_action call, it's that you shouldn't call
> > > > devm_drm_panel_bridge_add in the first place.
> > >
> > > I think it is because the remove action helper uses
> > > drm_panel_bridge_remove instead of devm hook.
> > > >
> > > > So either you use drm_panel_bridge_add and a custom drmm action, or you
> > > > add a drmm_panel_bridge_add function and use it.
> > >
> > > It is not possible to use this helper as it is expecting drm_device
> >
> > It's definitely possible, just change the prototype of the function to
> > take a drm_device pointer, just like any other drmm_* function.
> 
> But, in my case, I only get the drm_device pointer once I found the
> bridge pointer of panel_bridge but the actual bridge finding for
> panel_bridge is happening in the drmm_panel_bridge_add definition.
> Doesn't it redundant if I find the panel_bridge and pass drm_device
> and panel pointer for calling drmm_panel_bridge_add?

We've already discussed this, but you can't use
devm_drm_of_dsi_get_bridge() from the MIPI-DSI host attach function. So
fix that first, and then you'll have access to the DRM device in your
driver.
  

On Fri, Feb 3, 2023 at 4:34 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Fri, Feb 03, 2023 at 04:28:30PM +0530, Jagan Teki wrote:
> > On Fri, Feb 3, 2023 at 4:19 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Fri, Feb 03, 2023 at 04:13:49PM +0530, Jagan Teki wrote:
> > > > On Fri, Feb 3, 2023 at 1:56 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > >
> > > > > On Thu, Feb 02, 2023 at 10:22:42PM +0530, Jagan Teki wrote:
> > > > > > Hi Maxime,
> > > > > >
> > > > > > On Mon, Jan 30, 2023 at 6:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > >
> > > > > > > On Thu, Jan 26, 2023 at 08:48:48PM +0530, Jagan Teki wrote:
> > > > > > > > On Thu, Jan 26, 2023 at 5:42 PM Maxime Ripard <maxime@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?
> > > > > > >
> > > > > > > It's not that we can, it's that the devm_panel_bridge_add is unsafe:
> > > > > > > when the module is removed the device will go away and all the devm
> > > > > > > resources freed, but the DRM device sticks around until the last
> > > > > > > application with a fd open closes that fd.
> > > > > >
> > > > > > Would you please check this, Here I'm trying to do
> > > > > >
> > > > > > 1. find a panel or bridge
> > > > > > 2. if panel add it as a panel bridge
> > > > > > 3. add DRM-managed action with the help of bridge->dev after step 2.
> > > > >
> > > > > The logic is sound in your patch
> > > > >
> > > > > > Didn't test the behavior, just wanted to check whether it can be a
> > > > > > possibility to use bridge->dev as this dev is assigned with
> > > > > > encoder->dev during the bridge attach the chain. Please check and let
> > > > > > me know.
> > > > > >
> > > > > > struct drm_bridge *devm_drm_of_dsi_get_bridge(struct device *dev,
> > > > > >                                               struct device_node *np,
> > > > > >                                               u32 port, u32 endpoint)
> > > > > > {
> > > > > >         struct drm_bridge *bridge;
> > > > > >         struct drm_panel *panel;
> > > > > >         int ret;
> > > > > >
> > > > > >         ret = drm_of_dsi_find_panel_or_bridge(np, port, endpoint,
> > > > > >                                               &panel, &bridge);
> > > > > >         if (ret)
> > > > > >                 return ERR_PTR(ret);
> > > > > >
> > > > > >         if (panel)
> > > > > >                 bridge = devm_drm_panel_bridge_add(dev, panel);
> > > > > >
> > > > > >         if (IS_ERR(bridge))
> > > > > >                 return bridge;
> > > > > >
> > > > > >         ret = drmm_add_action_or_reset(bridge->dev,
> > > > > > drmm_drm_panel_bridge_release,
> > > > > >                                        bridge);
> > > > > >         if (ret)
> > > > > >                 return ERR_PTR(ret);
> > > > > >
> > > > > >         return bridge;
> > > > > > }
> > > > >
> > > > > It's the implementation that isn't. You cannot use a devm hook to
> > > > > register a KMS structure, so it's not that you should add a
> > > > > drmm_add_action call, it's that you shouldn't call
> > > > > devm_drm_panel_bridge_add in the first place.
> > > >
> > > > I think it is because the remove action helper uses
> > > > drm_panel_bridge_remove instead of devm hook.
> > > > >
> > > > > So either you use drm_panel_bridge_add and a custom drmm action, or you
> > > > > add a drmm_panel_bridge_add function and use it.
> > > >
> > > > It is not possible to use this helper as it is expecting drm_device
> > >
> > > It's definitely possible, just change the prototype of the function to
> > > take a drm_device pointer, just like any other drmm_* function.
> >
> > But, in my case, I only get the drm_device pointer once I found the
> > bridge pointer of panel_bridge but the actual bridge finding for
> > panel_bridge is happening in the drmm_panel_bridge_add definition.
> > Doesn't it redundant if I find the panel_bridge and pass drm_device
> > and panel pointer for calling drmm_panel_bridge_add?
>
> We've already discussed this, but you can't use
> devm_drm_of_dsi_get_bridge() from the MIPI-DSI host attach function. So
> fix that first, and then you'll have access to the DRM device in your
> driver.

I have updated the new series with this change. Please have a look.

Thanks,
Jagan.