diff mbox series

[v13,02/18] drm: bridge: panel: Support nodrm case for drmm_panel_bridge_add

Message ID 20230227113925.875425-3-jagan@amarulasolutions.com
State New
Headers
Series drm: Add Samsung MIPI DSIM bridge |

Commit Message

Jagan Teki Feb. 27, 2023, 11:39 a.m. UTC 
drmm_panel_bridge_add DRM-managed action helper is useful for the bridge
which automatically removes the bridge when drm pointer is cleaned.

Supporting the same on non-component bridges like host DSI bridge requires
a drm pointer which is indeed available only when a panel-bridge is found.

For these use cases, the caller would call the drmm_panel_bridge_add by
passing NULL to drm pointer.

So, assign the bridge->dev to drm pointer for those cases.

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 v13:
- new patch

Note: use case on 
"[PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper"

 drivers/gpu/drm/bridge/panel.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Maxime Ripard Feb. 27, 2023, 12:11 p.m. UTC | #1 
On Mon, Feb 27, 2023 at 05:09:09PM +0530, Jagan Teki wrote:
> drmm_panel_bridge_add DRM-managed action helper is useful for the bridge
> which automatically removes the bridge when drm pointer is cleaned.
> 
> Supporting the same on non-component bridges like host DSI bridge requires
> a drm pointer which is indeed available only when a panel-bridge is found.
> 
> For these use cases, the caller would call the drmm_panel_bridge_add by
> passing NULL to drm pointer.
> 
> So, assign the bridge->dev to drm pointer for those cases.
> 
> 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 v13:
> - new patch
> 
> Note: use case on 
> "[PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper"
> 
>  drivers/gpu/drm/bridge/panel.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index d4b112911a99..45a0c6671000 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -402,6 +402,13 @@ struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
>  	if (IS_ERR(bridge))
>  		return bridge;
>  
> +	/*
> +	 * For non-component bridges, like host DSI bridge the DRM pointer
> +	 * can be available only when a panel-bridge is found.
> +	 */
> +	if (!drm)
> +		drm = bridge->dev;
> +

Why can't the caller use bridge->dev?

Also, where did the devm_drm_of_dsi_get_bridge go? I thought you were
going to convert it to a drm-managed version?

Maxime
Jagan Teki Feb. 27, 2023, 12:28 p.m. UTC | #2 
On Mon, Feb 27, 2023 at 5:41 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Mon, Feb 27, 2023 at 05:09:09PM +0530, Jagan Teki wrote:
> > drmm_panel_bridge_add DRM-managed action helper is useful for the bridge
> > which automatically removes the bridge when drm pointer is cleaned.
> >
> > Supporting the same on non-component bridges like host DSI bridge requires
> > a drm pointer which is indeed available only when a panel-bridge is found.
> >
> > For these use cases, the caller would call the drmm_panel_bridge_add by
> > passing NULL to drm pointer.
> >
> > So, assign the bridge->dev to drm pointer for those cases.
> >
> > 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 v13:
> > - new patch
> >
> > Note: use case on
> > "[PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper"
> >
> >  drivers/gpu/drm/bridge/panel.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> > index d4b112911a99..45a0c6671000 100644
> > --- a/drivers/gpu/drm/bridge/panel.c
> > +++ b/drivers/gpu/drm/bridge/panel.c
> > @@ -402,6 +402,13 @@ struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
> >       if (IS_ERR(bridge))
> >               return bridge;
> >
> > +     /*
> > +      * For non-component bridges, like host DSI bridge the DRM pointer
> > +      * can be available only when a panel-bridge is found.
> > +      */
> > +     if (!drm)
> > +             drm = bridge->dev;
> > +
>
> Why can't the caller use bridge->dev?

The first step of drmm_panel_bridge_add is to find the panel-bridge,
so we can only get bridge->dev after this step. The caller doesn't
know anything about the panel bridge it just sends a panel pointer to
find the panel-bridge and then assigns bridge->dev to drm for DRM
action.

Please check this patch about the caller,
https://patchwork.kernel.org/project/dri-devel/patch/20230227113925.875425-5-jagan@amarulasolutions.com/

>
> Also, where did the devm_drm_of_dsi_get_bridge go? I thought you were
> going to convert it to a drm-managed version?

Look like your suggestion to can't use devm_drm_of_dsi_get_bridge and
call the DRM-action from the driver, that is the reason I have removed
this and done the same as your previous inputs.
https://lore.kernel.org/all/20230203110437.otzl2azscbujigv6@houat/

Jagan.
Jagan Teki Feb. 27, 2023, 1:39 p.m. UTC | #3 
On Mon, Feb 27, 2023 at 5:41 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Mon, Feb 27, 2023 at 05:09:09PM +0530, Jagan Teki wrote:
> > drmm_panel_bridge_add DRM-managed action helper is useful for the bridge
> > which automatically removes the bridge when drm pointer is cleaned.
> >
> > Supporting the same on non-component bridges like host DSI bridge requires
> > a drm pointer which is indeed available only when a panel-bridge is found.
> >
> > For these use cases, the caller would call the drmm_panel_bridge_add by
> > passing NULL to drm pointer.
> >
> > So, assign the bridge->dev to drm pointer for those cases.
> >
> > 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 v13:
> > - new patch
> >
> > Note: use case on
> > "[PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper"
> >
> >  drivers/gpu/drm/bridge/panel.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> > index d4b112911a99..45a0c6671000 100644
> > --- a/drivers/gpu/drm/bridge/panel.c
> > +++ b/drivers/gpu/drm/bridge/panel.c
> > @@ -402,6 +402,13 @@ struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
> >       if (IS_ERR(bridge))
> >               return bridge;
> >
> > +     /*
> > +      * For non-component bridges, like host DSI bridge the DRM pointer
> > +      * can be available only when a panel-bridge is found.
> > +      */
> > +     if (!drm)
> > +             drm = bridge->dev;
> > +
>
> Why can't the caller use bridge->dev?
>
> Also, where did the devm_drm_of_dsi_get_bridge go? I thought you were
> going to convert it to a drm-managed version?

I found another solution that supports DRM-managed action common
across dsi and normal bridges, can I send those patches alone by
excluding them from this series?

Please let me know.

Jagan.
Maxime Ripard Feb. 27, 2023, 5:14 p.m. UTC | #4 
On Mon, Feb 27, 2023 at 05:58:03PM +0530, Jagan Teki wrote:
> On Mon, Feb 27, 2023 at 5:41 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Mon, Feb 27, 2023 at 05:09:09PM +0530, Jagan Teki wrote:
> > > drmm_panel_bridge_add DRM-managed action helper is useful for the bridge
> > > which automatically removes the bridge when drm pointer is cleaned.
> > >
> > > Supporting the same on non-component bridges like host DSI bridge requires
> > > a drm pointer which is indeed available only when a panel-bridge is found.
> > >
> > > For these use cases, the caller would call the drmm_panel_bridge_add by
> > > passing NULL to drm pointer.
> > >
> > > So, assign the bridge->dev to drm pointer for those cases.
> > >
> > > 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 v13:
> > > - new patch
> > >
> > > Note: use case on
> > > "[PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper"
> > >
> > >  drivers/gpu/drm/bridge/panel.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> > > index d4b112911a99..45a0c6671000 100644
> > > --- a/drivers/gpu/drm/bridge/panel.c
> > > +++ b/drivers/gpu/drm/bridge/panel.c
> > > @@ -402,6 +402,13 @@ struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
> > >       if (IS_ERR(bridge))
> > >               return bridge;
> > >
> > > +     /*
> > > +      * For non-component bridges, like host DSI bridge the DRM pointer
> > > +      * can be available only when a panel-bridge is found.
> > > +      */
> > > +     if (!drm)
> > > +             drm = bridge->dev;
> > > +
> >
> > Why can't the caller use bridge->dev?
> 
> The first step of drmm_panel_bridge_add is to find the panel-bridge,
> so we can only get bridge->dev after this step. The caller doesn't
> know anything about the panel bridge it just sends a panel pointer to
> find the panel-bridge

Ah, yes, indeed. This is still a hack we don't need.

> then assigns bridge->dev to drm for DRM action
>
> Please check this patch about the caller,
> https://patchwork.kernel.org/project/dri-devel/patch/20230227113925.875425-5-jagan@amarulasolutions.com/

Because we've already been over this. You can't call that function from
DSI's attach. So you should change that to what I already pointed you
to, and then you'll have the drm device pointer available.

> > Also, where did the devm_drm_of_dsi_get_bridge go? I thought you were
> > going to convert it to a drm-managed version?
> 
> Look like your suggestion to can't use devm_drm_of_dsi_get_bridge and
> call the DRM-action from the driver, that is the reason I have removed
> this and done the same as your previous inputs.
> https://lore.kernel.org/all/20230203110437.otzl2azscbujigv6@houat/

You can't use devm. You can and should definitely use drmm.

Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index d4b112911a99..45a0c6671000 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -402,6 +402,13 @@  struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
 	if (IS_ERR(bridge))
 		return bridge;
 
+	/*
+	 * For non-component bridges, like host DSI bridge the DRM pointer
+	 * can be available only when a panel-bridge is found.
+	 */
+	if (!drm)
+		drm = bridge->dev;
+
 	ret = drmm_add_action_or_reset(drm, drmm_drm_panel_bridge_release,
 				       bridge);
 	if (ret)