| Message ID | 20230123151212.269082-3-jagan@amarulasolutions.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm: Add Samsung MIPI DSIM bridge | expand |
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 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 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.
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index e8aae3cdc73d..be281eb26356 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -499,4 +499,38 @@ struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, } EXPORT_SYMBOL(drmm_of_get_bridge); +/** + * devm_drm_of_dsi_get_bridge - Return next DSI bridge in the chain + * @dev: device to tie the bridge lifetime to + * @np: device tree node containing encoder output ports + * @port: port in the device tree node + * @endpoint: endpoint in the device tree node + * + * 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. Either @panel or @bridge must not be NULL. + * + * Returns a pointer to the bridge if successful, or an error pointer + * otherwise. + */ +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); + + return bridge; +} +EXPORT_SYMBOL(devm_drm_of_dsi_get_bridge); + #endif diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 42f86327b40a..ccb14e361d3f 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -931,6 +931,8 @@ struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node u32 port, u32 endpoint); struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, struct device_node *node, u32 port, u32 endpoint); +struct drm_bridge *devm_drm_of_dsi_get_bridge(struct device *dev, struct device_node *node, + u32 port, u32 endpoint); #else static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
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(+)