| Message ID | 20190315130825.9005-2-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: Add ICN6211 MIPI-DSI/RGB bridge | expand |
Hi Jakan, On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote: > Export drm_bridge_detach from drm bridge core so-that it > can use on respective interface or bridge driver while > detaching the bridge. I don't see why this change is required based on the commit log. The DRM bridge code clearly indicates that drm_bridge_attach should *not* be balanced with a drm_bridge_detach call in the driver, so this seems quite wrong. The DRM core itself should handle detaching the bridge, not the driver. Is there any reason why you need to do things differently for DSI? Cheers, Paul > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > drivers/gpu/drm/drm_bridge.c | 1 + > include/drm/drm_bridge.h | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 138b2711d389..569d4f345429 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -159,6 +159,7 @@ void drm_bridge_detach(struct drm_bridge *bridge) > > bridge->dev = NULL; > } > +EXPORT_SYMBOL(drm_bridge_detach); > > /** > * DOC: bridge callbacks > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 9da8c93f7976..4955e3e50fa4 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -301,6 +301,7 @@ void drm_bridge_remove(struct drm_bridge *bridge); > struct drm_bridge *of_drm_find_bridge(struct device_node *np); > int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > struct drm_bridge *previous); > +void drm_bridge_detach(struct drm_bridge *bridge); > > bool drm_bridge_mode_fixup(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > -- > 2.18.0.321.gffc6fa0e3 >
Hi Paul, On Fri, Mar 15, 2019 at 6:58 PM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > Hi Jakan, > > On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote: > > Export drm_bridge_detach from drm bridge core so-that it > > can use on respective interface or bridge driver while > > detaching the bridge. > > I don't see why this change is required based on the commit log. The > DRM bridge code clearly indicates that drm_bridge_attach should *not* > be balanced with a drm_bridge_detach call in the driver, so this seems > quite wrong. > > The DRM core itself should handle detaching the bridge, not the driver. > Is there any reason why you need to do things differently for DSI? Yes, you are correct the detach of bridge is being taking care via drm_encoder_cleanup. This patch exported explicitly, since we need to taken care bridge detach during unbind even exynos_drm_dsi in other patch seems using detach by explicitly pointing. so I think the better approach is to use drm_encoder_cleanup in unbind, what do you say? Jagan.
Hi, Le lundi 18 mars 2019 à 22:18 +0530, Jagan Teki a écrit : > Hi Paul, > > On Fri, Mar 15, 2019 at 6:58 PM Paul Kocialkowski > <paul.kocialkowski@bootlin.com> wrote: > > Hi Jakan, > > > > On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote: > > > Export drm_bridge_detach from drm bridge core so-that it > > > can use on respective interface or bridge driver while > > > detaching the bridge. > > > > I don't see why this change is required based on the commit log. The > > DRM bridge code clearly indicates that drm_bridge_attach should *not* > > be balanced with a drm_bridge_detach call in the driver, so this seems > > quite wrong. > > > > The DRM core itself should handle detaching the bridge, not the driver. > > Is there any reason why you need to do things differently for DSI? > > Yes, you are correct the detach of bridge is being taking care via > drm_encoder_cleanup. This patch exported explicitly, since we need to > taken care bridge detach during unbind even exynos_drm_dsi in other > patch seems using detach by explicitly pointing. I can see that from your patches, but you are not explaining why you need the change. And if the framework doesn't work for your case, you should certainly try and fix the framework instead of working around the issue. Anyway, you should probably look into using drm_panel_bridge_add, it might fix the underlying issue on its own. > so I think the better approach is to use drm_encoder_cleanup in > unbind, what do you say? I any case, you need to state what your problem is (in the commit log, and not even in subsequent discussions) so that we can have a chance to understand it and provide some feedback about what is an appropriate fix and what is not. We can't understand the fix if we can't understand the underlying issue. Cheers, Paul
On Mon, Mar 18, 2019 at 10:27 PM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > Hi, > > Le lundi 18 mars 2019 à 22:18 +0530, Jagan Teki a écrit : > > Hi Paul, > > > > On Fri, Mar 15, 2019 at 6:58 PM Paul Kocialkowski > > <paul.kocialkowski@bootlin.com> wrote: > > > Hi Jakan, > > > > > > On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote: > > > > Export drm_bridge_detach from drm bridge core so-that it > > > > can use on respective interface or bridge driver while > > > > detaching the bridge. > > > > > > I don't see why this change is required based on the commit log. The > > > DRM bridge code clearly indicates that drm_bridge_attach should *not* > > > be balanced with a drm_bridge_detach call in the driver, so this seems > > > quite wrong. > > > > > > The DRM core itself should handle detaching the bridge, not the driver. > > > Is there any reason why you need to do things differently for DSI? > > > > Yes, you are correct the detach of bridge is being taking care via > > drm_encoder_cleanup. This patch exported explicitly, since we need to > > taken care bridge detach during unbind even exynos_drm_dsi in other > > patch seems using detach by explicitly pointing. > > I can see that from your patches, but you are not explaining why you > need the change. And if the framework doesn't work for your case, you > should certainly try and fix the framework instead of working around > the issue. Please see below comments. > > Anyway, you should probably look into using drm_panel_bridge_add, it > might fix the underlying issue on its own. > > > so I think the better approach is to use drm_encoder_cleanup in > > unbind, what do you say? > > I any case, you need to state what your problem is (in the commit log, > and not even in subsequent discussions) so that we can have a chance to > understand it and provide some feedback about what is an appropriate > fix and what is not. We can't understand the fix if we can't understand > the underlying issue. True, if my intentions is trying to fix some issue, but as I said in previous mail since the other dsi (exynos_drm_dsi) driver is explicitly detaching I presume it require or missed the export, not thought about fix and neither I mentioned on the commit head. Anyway thanks for the review, will update the code accordingly in next version.
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 138b2711d389..569d4f345429 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -159,6 +159,7 @@ void drm_bridge_detach(struct drm_bridge *bridge) bridge->dev = NULL; } +EXPORT_SYMBOL(drm_bridge_detach); /** * DOC: bridge callbacks diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 9da8c93f7976..4955e3e50fa4 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -301,6 +301,7 @@ void drm_bridge_remove(struct drm_bridge *bridge); struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous); +void drm_bridge_detach(struct drm_bridge *bridge); bool drm_bridge_mode_fixup(struct drm_bridge *bridge, const struct drm_display_mode *mode,
Export drm_bridge_detach from drm bridge core so-that it can use on respective interface or bridge driver while detaching the bridge. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- drivers/gpu/drm/drm_bridge.c | 1 + include/drm/drm_bridge.h | 1 + 2 files changed, 2 insertions(+)