Message ID | 1409149783-12416-4-git-send-email-ajaykumar.rs@samsung.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
So don't ask why but I accidentally ended up in a branch looking at this patch and didn't like it. So very quick&grumpy review. First, please make the patch subject more descriptive: I'd expect a helper function scaffolding like the various crtc/probe/dp ... helpers we already have. You instead add code to untangle the probe ordering. Please say so. More comments below. On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote: > A set of helper functions are defined in this patch to make > bridge driver probe independent of the drm flow. > > The bridge devices register themselves on a lookup table > when they get probed by calling "drm_bridge_add". > > The parent encoder driver waits till the bridge is available > in the lookup table(by calling "of_drm_find_bridge") and then > continues with its initialization. > > The encoder driver should also call "drm_bridge_attach" to pass > on the drm_device, encoder pointers to the bridge object. > > drm_bridge_attach inturn calls drm_bridge_init to register itself > with the drm core. Later, it calls "bridge->funcs->attach" so that > bridge can continue with other initializations. > > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> [snip] > @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > * @driver_private: pointer to the bridge driver's internal context > */ > struct drm_bridge { > - struct drm_device *dev; > + struct device *dev; Please don't rename the ->dev pointer into drm. Because _all_ the other drm structures still call it ->dev. Also, can't we use struct device_node here like we do in the of helpers Russell added? See 7e435aad38083 > + struct drm_device *drm; > + struct drm_encoder *encoder; This breaks bridge->bridge chaining (if we ever get there). It seems pretty much unused anyway except for an EBUSY check. Can't you use bridge->dev for that? > struct list_head head; > + struct list_head list; These lists need better names. I know that the "head" is really awful, especially since it's actually not the head of the list but just an element. > > struct drm_mode_object base; Aside: I've noticed all this trying to update the kerneldoc for struct drm_bridge, which just showed that this patch makes inconsistent changes. Trying to write kerneldoc is a really great way to come up with better interfaces imo. Cheers, Daniel > > @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector); > /* helper to unplug all connectors from sysfs for device */ > extern void drm_connector_unplug_all(struct drm_device *dev); > > +extern int drm_bridge_add(struct drm_bridge *bridge); > +extern void drm_bridge_remove(struct drm_bridge *bridge); > +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); > +extern int drm_bridge_attach(struct drm_bridge *bridge, > + struct drm_encoder *encoder); > extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge); > extern void drm_bridge_cleanup(struct drm_bridge *bridge); > > -- > 1.7.9.5 >
On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > So don't ask why but I accidentally ended up in a branch looking at this > patch and didn't like it. So very quick&grumpy review. > > First, please make the patch subject more descriptive: I'd expect a helper > function scaffolding like the various crtc/probe/dp ... helpers we already > have. You instead add code to untangle the probe ordering. Please say so. > > More comments below. > > On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote: >> A set of helper functions are defined in this patch to make >> bridge driver probe independent of the drm flow. >> >> The bridge devices register themselves on a lookup table >> when they get probed by calling "drm_bridge_add". >> >> The parent encoder driver waits till the bridge is available >> in the lookup table(by calling "of_drm_find_bridge") and then >> continues with its initialization. >> >> The encoder driver should also call "drm_bridge_attach" to pass >> on the drm_device, encoder pointers to the bridge object. >> >> drm_bridge_attach inturn calls drm_bridge_init to register itself >> with the drm core. Later, it calls "bridge->funcs->attach" so that >> bridge can continue with other initializations. >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> > > [snip] > >> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >> * @driver_private: pointer to the bridge driver's internal context >> */ >> struct drm_bridge { >> - struct drm_device *dev; >> + struct device *dev; > > Please don't rename the ->dev pointer into drm. Because _all_ the other > drm structures still call it ->dev. Also, can't we use struct device_node > here like we do in the of helpers Russell added? See 7e435aad38083 > I think this is modeled after the naming in drm_panel, FWIW. However, seems reasonable to keep the device_node instead. >> + struct drm_device *drm; >> + struct drm_encoder *encoder; > > This breaks bridge->bridge chaining (if we ever get there). It seems > pretty much unused anyway except for an EBUSY check. Can't you use > bridge->dev for that? > Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach and leave it up to the caller to establish the proper chain. >> struct list_head head; >> + struct list_head list; > > These lists need better names. I know that the "head" is really awful, > especially since it's actually not the head of the list but just an > element. I think we can just rip bridge_list out of mode_config if we're going to keep track of bridges elsewhere. So we can nuke "head" and keep "list". This also means that bridge->destroy() goes away, but that's probably Ok if everything converts to the standalone driver model where we have driver->remove() Sean >> >> struct drm_mode_object base; > > > Aside: I've noticed all this trying to update the kerneldoc for struct > drm_bridge, which just showed that this patch makes inconsistent changes. > Trying to write kerneldoc is a really great way to come up with better > interfaces imo. > > Cheers, Daniel > >> >> @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector); >> /* helper to unplug all connectors from sysfs for device */ >> extern void drm_connector_unplug_all(struct drm_device *dev); >> >> +extern int drm_bridge_add(struct drm_bridge *bridge); >> +extern void drm_bridge_remove(struct drm_bridge *bridge); >> +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); >> +extern int drm_bridge_attach(struct drm_bridge *bridge, >> + struct drm_encoder *encoder); >> extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge); >> extern void drm_bridge_cleanup(struct drm_bridge *bridge); >> >> -- >> 1.7.9.5 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote: > A set of helper functions are defined in this patch to make > bridge driver probe independent of the drm flow. > > The bridge devices register themselves on a lookup table > when they get probed by calling "drm_bridge_add". > > The parent encoder driver waits till the bridge is available > in the lookup table(by calling "of_drm_find_bridge") and then > continues with its initialization. > > The encoder driver should also call "drm_bridge_attach" to pass > on the drm_device, encoder pointers to the bridge object. > > drm_bridge_attach inturn calls drm_bridge_init to register itself > with the drm core. Later, it calls "bridge->funcs->attach" so that > bridge can continue with other initializations. > > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> > --- > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/bridge/Kconfig | 15 ++++- > drivers/gpu/drm/drm_bridge.c | 102 ++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_crtc.c | 4 +- > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 4 +- > include/drm/drm_crtc.h | 12 +++- > 6 files changed, 131 insertions(+), 7 deletions(-) > create mode 100644 drivers/gpu/drm/drm_bridge.c > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 4a55d59..bdbfb6f 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \ > drm-$(CONFIG_COMPAT) += drm_ioc32.o > drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o > drm-$(CONFIG_PCI) += ati_pcigart.o > +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o > drm-$(CONFIG_DRM_PANEL) += drm_panel.o > drm-$(CONFIG_OF) += drm_of.o > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 884923f..5a8e907 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -1,5 +1,16 @@ > -config DRM_PTN3460 > - tristate "PTN3460 DP/LVDS bridge" > +config DRM_BRIDGE I'm not convinced this adds any value, to be honest. In addition to whether or not it's useful, it seems like you'd need to stub the drm_bridge_* functions that are declared in drm_crtc.h or break them out into drm_bridge.h. Sean > + tristate > depends on DRM > select DRM_KMS_HELPER > + help > + Bridge registration and lookup framework. > + > +menu "bridge chips" > + depends on DRM_BRIDGE > + > +config DRM_PTN3460 > + tristate "PTN3460 DP/LVDS bridge" > + depends on DRM_BRIDGE > ---help--- > + > +endmenu > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > new file mode 100644 > index 0000000..b2d43fd > --- /dev/null > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -0,0 +1,102 @@ > +/* > + * Copyright (c) 2014 Samsung Electronics Co., Ltd > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sub license, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the > + * next paragraph) shall be included in all copies or substantial portions > + * of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + */ > + > +#include <linux/err.h> > +#include <linux/module.h> > + > +#include <drm/drm_crtc.h> > + > +#include "drm/drmP.h" > + > +static DEFINE_MUTEX(bridge_lock); > +static LIST_HEAD(bridge_list); > + > +int drm_bridge_add(struct drm_bridge *bridge) > +{ > + mutex_lock(&bridge_lock); > + list_add_tail(&bridge->list, &bridge_list); > + mutex_unlock(&bridge_lock); > + > + return 0; > +} > +EXPORT_SYMBOL(drm_bridge_add); > + > +void drm_bridge_remove(struct drm_bridge *bridge) > +{ > + mutex_lock(&bridge_lock); > + list_del_init(&bridge->list); > + mutex_unlock(&bridge_lock); > +} > +EXPORT_SYMBOL(drm_bridge_remove); > + > +int drm_bridge_attach(struct drm_bridge *bridge, > + struct drm_encoder *encoder) > +{ > + int ret; > + > + if (!bridge || !encoder) > + return -EINVAL; > + > + if (bridge->encoder) > + return -EBUSY; > + > + encoder->bridge = bridge; > + bridge->encoder = encoder; > + bridge->drm = encoder->dev; > + > + ret = drm_bridge_init(bridge->drm, bridge); > + if (ret) { > + DRM_ERROR("Failed to register bridge with drm\n"); > + return ret; > + } > + > + if (bridge->funcs->attach) > + return bridge->funcs->attach(bridge); > + > + return 0; > +} > +EXPORT_SYMBOL(drm_bridge_attach); > + > +#ifdef CONFIG_OF > +struct drm_bridge *of_drm_find_bridge(struct device_node *np) > +{ > + struct drm_bridge *bridge; > + > + mutex_lock(&bridge_lock); > + > + list_for_each_entry(bridge, &bridge_list, list) { > + if (bridge->dev->of_node == np) { > + mutex_unlock(&bridge_lock); > + return bridge; > + } > + } > + > + mutex_unlock(&bridge_lock); > + return NULL; > +} > +EXPORT_SYMBOL(of_drm_find_bridge); > +#endif > + > +MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>"); > +MODULE_DESCRIPTION("DRM bridge infrastructure"); > +MODULE_LICENSE("GPL and additional rights"); > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 25f5cfa..2fb22fa 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -1039,7 +1039,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge) > if (ret) > goto out; > > - bridge->dev = dev; > + bridge->drm = dev; > > list_add_tail(&bridge->head, &dev->mode_config.bridge_list); > dev->mode_config.num_bridge++; > @@ -1058,7 +1058,7 @@ EXPORT_SYMBOL(drm_bridge_init); > */ > void drm_bridge_cleanup(struct drm_bridge *bridge) > { > - struct drm_device *dev = bridge->dev; > + struct drm_device *dev = bridge->drm; > > drm_modeset_lock_all(dev); > drm_mode_object_put(dev, &bridge->base); > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > index 0309539..bc9e5ff 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > @@ -33,7 +33,7 @@ static void hdmi_bridge_destroy(struct drm_bridge *bridge) > > static void power_on(struct drm_bridge *bridge) > { > - struct drm_device *dev = bridge->dev; > + struct drm_device *dev = bridge->drm; > struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); > struct hdmi *hdmi = hdmi_bridge->hdmi; > const struct hdmi_platform_config *config = hdmi->config; > @@ -67,7 +67,7 @@ static void power_on(struct drm_bridge *bridge) > > static void power_off(struct drm_bridge *bridge) > { > - struct drm_device *dev = bridge->dev; > + struct drm_device *dev = bridge->drm; > struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); > struct hdmi *hdmi = hdmi_bridge->hdmi; > const struct hdmi_platform_config *config = hdmi->config; > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index f48a436..f6f426f 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -629,6 +629,7 @@ struct drm_plane { > > /** > * drm_bridge_funcs - drm_bridge control functions > + * @attach: Called during drm_bridge_attach > * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge > * @disable: Called right before encoder prepare, disables the bridge > * @post_disable: Called right after encoder prepare, for lockstepped disable > @@ -638,6 +639,7 @@ struct drm_plane { > * @destroy: make object go away > */ > struct drm_bridge_funcs { > + int (*attach)(struct drm_bridge *bridge); > bool (*mode_fixup)(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode); > @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > * @driver_private: pointer to the bridge driver's internal context > */ > struct drm_bridge { > - struct drm_device *dev; > + struct device *dev; > + struct drm_device *drm; > + struct drm_encoder *encoder; > struct list_head head; > + struct list_head list; > > struct drm_mode_object base; > > @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector); > /* helper to unplug all connectors from sysfs for device */ > extern void drm_connector_unplug_all(struct drm_device *dev); > > +extern int drm_bridge_add(struct drm_bridge *bridge); > +extern void drm_bridge_remove(struct drm_bridge *bridge); > +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); > +extern int drm_bridge_attach(struct drm_bridge *bridge, > + struct drm_encoder *encoder); > extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge); > extern void drm_bridge_cleanup(struct drm_bridge *bridge); > > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >>> * @driver_private: pointer to the bridge driver's internal context >>> */ >>> struct drm_bridge { >>> - struct drm_device *dev; >>> + struct device *dev; >> >> Please don't rename the ->dev pointer into drm. Because _all_ the other >> drm structures still call it ->dev. Also, can't we use struct device_node >> here like we do in the of helpers Russell added? See 7e435aad38083 >> > > I think this is modeled after the naming in drm_panel, FWIW. However, > seems reasonable to keep the device_node instead. Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with drm_crtc drop the struct device and go directly to a struct device_node. Since we don't really need the sturct device, the only thing we care about is the of_node. For added bonus wrap an #ifdef CONFIG_OF around all the various struct device_node in drm_foo.h. Should be all fairly simple to pull off with cocci. Thierry? -Daniel
On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >>>> * @driver_private: pointer to the bridge driver's internal context >>>> */ >>>> struct drm_bridge { >>>> - struct drm_device *dev; >>>> + struct device *dev; >>> >>> Please don't rename the ->dev pointer into drm. Because _all_ the other >>> drm structures still call it ->dev. Also, can't we use struct device_node >>> here like we do in the of helpers Russell added? See 7e435aad38083 >>> >> >> I think this is modeled after the naming in drm_panel, FWIW. However, >> seems reasonable to keep the device_node instead. > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > drm_crtc drop the struct device and go directly to a struct > device_node. Since we don't really need the sturct device, the only > thing we care about is the of_node. For added bonus wrap an #ifdef > CONFIG_OF around all the various struct device_node in drm_foo.h. > Should be all fairly simple to pull off with cocci. > > Thierry? Looking at the of_drm_find_panel function I actually wonder how that works - the drm_panel doesn't really need to stick around afaics. After all panel_list is global so some other driver can unload. Russell's of support for possible_crtcs code works differently since it only looks at per-drm_device lists. This bridge code here though suffers from the same. So to me this looks rather fishy. It doesn't help that we have drm_of.[hc] around but not all the of code is in there. Adding Russell too. -Daniel
Hi Daniel and Sean, Thanks for the comments! On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <seanpaul@chromium.org> wrote: > On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> So don't ask why but I accidentally ended up in a branch looking at this >> patch and didn't like it. So very quick&grumpy review. >> >> First, please make the patch subject more descriptive: I'd expect a helper >> function scaffolding like the various crtc/probe/dp ... helpers we already >> have. You instead add code to untangle the probe ordering. Please say so. Sure. I will reword it properly. >> More comments below. >> >> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote: >>> A set of helper functions are defined in this patch to make >>> bridge driver probe independent of the drm flow. >>> >>> The bridge devices register themselves on a lookup table >>> when they get probed by calling "drm_bridge_add". >>> >>> The parent encoder driver waits till the bridge is available >>> in the lookup table(by calling "of_drm_find_bridge") and then >>> continues with its initialization. >>> >>> The encoder driver should also call "drm_bridge_attach" to pass >>> on the drm_device, encoder pointers to the bridge object. >>> >>> drm_bridge_attach inturn calls drm_bridge_init to register itself >>> with the drm core. Later, it calls "bridge->funcs->attach" so that >>> bridge can continue with other initializations. >>> >>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >> >> [snip] >> >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >>> * @driver_private: pointer to the bridge driver's internal context >>> */ >>> struct drm_bridge { >>> - struct drm_device *dev; >>> + struct device *dev; >> >> Please don't rename the ->dev pointer into drm. Because _all_ the other >> drm structures still call it ->dev. Also, can't we use struct device_node >> here like we do in the of helpers Russell added? See 7e435aad38083 >> > > I think this is modeled after the naming in drm_panel, Right, The entire rework is based on how drm_panel framework is structured. > FWIW. However, > seems reasonable to keep the device_node instead. Yes, its visible that just device_node would be sufficient. This will save us from renaming drm_device as well. >>> + struct drm_device *drm; >>> + struct drm_encoder *encoder; >> >> This breaks bridge->bridge chaining (if we ever get there). It seems >> pretty much unused anyway except for an EBUSY check. Can't you use >> bridge->dev for that? >> > > Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach > and leave it up to the caller to establish the proper chain. Ok. I will use drm_device pointer directly instead of passing encoder pointer. >>> struct list_head head; >>> + struct list_head list; >> >> These lists need better names. I know that the "head" is really awful, >> especially since it's actually not the head of the list but just an >> element. > > I think we can just rip bridge_list out of mode_config if we're going > to keep track of bridges elsewhere. So we can nuke "head" and keep > "list". This also means that bridge->destroy() goes away, but that's > probably Ok if everything converts to the standalone driver model > where we have driver->remove() > > Sean Great! Thierry actually mentioned about this once, and we have the confirmation now. >>> >>> struct drm_mode_object base; >> >> >> Aside: I've noticed all this trying to update the kerneldoc for struct >> drm_bridge, which just showed that this patch makes inconsistent changes. >> Trying to write kerneldoc is a really great way to come up with better >> interfaces imo. >> >> Cheers, Daniel I din't get this actually. You want me to create Doc../drm_bridge.txt or something similar? Ajay
On Tue, Oct 28, 2014 at 1:41 AM, Sean Paul <seanpaul@chromium.org> wrote: > On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote: >> A set of helper functions are defined in this patch to make >> bridge driver probe independent of the drm flow. >> >> The bridge devices register themselves on a lookup table >> when they get probed by calling "drm_bridge_add". >> >> The parent encoder driver waits till the bridge is available >> in the lookup table(by calling "of_drm_find_bridge") and then >> continues with its initialization. >> >> The encoder driver should also call "drm_bridge_attach" to pass >> on the drm_device, encoder pointers to the bridge object. >> >> drm_bridge_attach inturn calls drm_bridge_init to register itself >> with the drm core. Later, it calls "bridge->funcs->attach" so that >> bridge can continue with other initializations. >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >> --- >> drivers/gpu/drm/Makefile | 1 + >> drivers/gpu/drm/bridge/Kconfig | 15 ++++- >> drivers/gpu/drm/drm_bridge.c | 102 ++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_crtc.c | 4 +- >> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 4 +- >> include/drm/drm_crtc.h | 12 +++- >> 6 files changed, 131 insertions(+), 7 deletions(-) >> create mode 100644 drivers/gpu/drm/drm_bridge.c >> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile >> index 4a55d59..bdbfb6f 100644 >> --- a/drivers/gpu/drm/Makefile >> +++ b/drivers/gpu/drm/Makefile >> @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \ >> drm-$(CONFIG_COMPAT) += drm_ioc32.o >> drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o >> drm-$(CONFIG_PCI) += ati_pcigart.o >> +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o >> drm-$(CONFIG_DRM_PANEL) += drm_panel.o >> drm-$(CONFIG_OF) += drm_of.o >> >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >> index 884923f..5a8e907 100644 >> --- a/drivers/gpu/drm/bridge/Kconfig >> +++ b/drivers/gpu/drm/bridge/Kconfig >> @@ -1,5 +1,16 @@ >> -config DRM_PTN3460 >> - tristate "PTN3460 DP/LVDS bridge" >> +config DRM_BRIDGE > > I'm not convinced this adds any value, to be honest. In addition to > whether or not it's useful, it seems like you'd need to stub the > drm_bridge_* functions that are declared in drm_crtc.h or break them > out into drm_bridge.h. > > Sean > >> + tristate >> depends on DRM >> select DRM_KMS_HELPER >> + help >> + Bridge registration and lookup framework. >> + >> +menu "bridge chips" >> + depends on DRM_BRIDGE >> + >> +config DRM_PTN3460 >> + tristate "PTN3460 DP/LVDS bridge" >> + depends on DRM_BRIDGE >> ---help--- >> + >> +endmenu >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >> new file mode 100644 >> index 0000000..b2d43fd >> --- /dev/null >> +++ b/drivers/gpu/drm/drm_bridge.c >> @@ -0,0 +1,102 @@ >> +/* >> + * Copyright (c) 2014 Samsung Electronics Co., Ltd >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the "Software"), >> + * to deal in the Software without restriction, including without limitation >> + * the rights to use, copy, modify, merge, publish, distribute, sub license, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including the >> + * next paragraph) shall be included in all copies or substantial portions >> + * of the Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >> + * DEALINGS IN THE SOFTWARE. >> + */ >> + >> +#include <linux/err.h> >> +#include <linux/module.h> >> + >> +#include <drm/drm_crtc.h> >> + >> +#include "drm/drmP.h" >> + >> +static DEFINE_MUTEX(bridge_lock); >> +static LIST_HEAD(bridge_list); >> + >> +int drm_bridge_add(struct drm_bridge *bridge) >> +{ >> + mutex_lock(&bridge_lock); >> + list_add_tail(&bridge->list, &bridge_list); >> + mutex_unlock(&bridge_lock); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(drm_bridge_add); >> + >> +void drm_bridge_remove(struct drm_bridge *bridge) >> +{ >> + mutex_lock(&bridge_lock); >> + list_del_init(&bridge->list); >> + mutex_unlock(&bridge_lock); >> +} >> +EXPORT_SYMBOL(drm_bridge_remove); >> + >> +int drm_bridge_attach(struct drm_bridge *bridge, >> + struct drm_encoder *encoder) >> +{ >> + int ret; >> + >> + if (!bridge || !encoder) >> + return -EINVAL; >> + >> + if (bridge->encoder) >> + return -EBUSY; >> + >> + encoder->bridge = bridge; >> + bridge->encoder = encoder; >> + bridge->drm = encoder->dev; >> + >> + ret = drm_bridge_init(bridge->drm, bridge); >> + if (ret) { >> + DRM_ERROR("Failed to register bridge with drm\n"); >> + return ret; >> + } >> + >> + if (bridge->funcs->attach) >> + return bridge->funcs->attach(bridge); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(drm_bridge_attach); >> + >> +#ifdef CONFIG_OF >> +struct drm_bridge *of_drm_find_bridge(struct device_node *np) >> +{ >> + struct drm_bridge *bridge; >> + >> + mutex_lock(&bridge_lock); >> + >> + list_for_each_entry(bridge, &bridge_list, list) { >> + if (bridge->dev->of_node == np) { >> + mutex_unlock(&bridge_lock); >> + return bridge; >> + } >> + } >> + >> + mutex_unlock(&bridge_lock); >> + return NULL; >> +} >> +EXPORT_SYMBOL(of_drm_find_bridge); >> +#endif >> + >> +MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>"); >> +MODULE_DESCRIPTION("DRM bridge infrastructure"); >> +MODULE_LICENSE("GPL and additional rights"); >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index 25f5cfa..2fb22fa 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -1039,7 +1039,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge) >> if (ret) >> goto out; >> >> - bridge->dev = dev; >> + bridge->drm = dev; >> >> list_add_tail(&bridge->head, &dev->mode_config.bridge_list); >> dev->mode_config.num_bridge++; >> @@ -1058,7 +1058,7 @@ EXPORT_SYMBOL(drm_bridge_init); >> */ >> void drm_bridge_cleanup(struct drm_bridge *bridge) >> { >> - struct drm_device *dev = bridge->dev; >> + struct drm_device *dev = bridge->drm; >> >> drm_modeset_lock_all(dev); >> drm_mode_object_put(dev, &bridge->base); >> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c >> index 0309539..bc9e5ff 100644 >> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c >> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c >> @@ -33,7 +33,7 @@ static void hdmi_bridge_destroy(struct drm_bridge *bridge) >> >> static void power_on(struct drm_bridge *bridge) >> { >> - struct drm_device *dev = bridge->dev; >> + struct drm_device *dev = bridge->drm; >> struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); >> struct hdmi *hdmi = hdmi_bridge->hdmi; >> const struct hdmi_platform_config *config = hdmi->config; >> @@ -67,7 +67,7 @@ static void power_on(struct drm_bridge *bridge) >> >> static void power_off(struct drm_bridge *bridge) >> { >> - struct drm_device *dev = bridge->dev; >> + struct drm_device *dev = bridge->drm; >> struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); >> struct hdmi *hdmi = hdmi_bridge->hdmi; >> const struct hdmi_platform_config *config = hdmi->config; >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index f48a436..f6f426f 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -629,6 +629,7 @@ struct drm_plane { >> >> /** >> * drm_bridge_funcs - drm_bridge control functions >> + * @attach: Called during drm_bridge_attach >> * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge >> * @disable: Called right before encoder prepare, disables the bridge >> * @post_disable: Called right after encoder prepare, for lockstepped disable >> @@ -638,6 +639,7 @@ struct drm_plane { >> * @destroy: make object go away >> */ >> struct drm_bridge_funcs { >> + int (*attach)(struct drm_bridge *bridge); >> bool (*mode_fixup)(struct drm_bridge *bridge, >> const struct drm_display_mode *mode, >> struct drm_display_mode *adjusted_mode); >> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >> * @driver_private: pointer to the bridge driver's internal context >> */ >> struct drm_bridge { >> - struct drm_device *dev; >> + struct device *dev; >> + struct drm_device *drm; >> + struct drm_encoder *encoder; >> struct list_head head; >> + struct list_head list; >> >> struct drm_mode_object base; >> >> @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector); >> /* helper to unplug all connectors from sysfs for device */ >> extern void drm_connector_unplug_all(struct drm_device *dev); >> >> +extern int drm_bridge_add(struct drm_bridge *bridge); >> +extern void drm_bridge_remove(struct drm_bridge *bridge); >> +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); >> +extern int drm_bridge_attach(struct drm_bridge *bridge, >> + struct drm_encoder *encoder); >> extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge); >> extern void drm_bridge_cleanup(struct drm_bridge *bridge); >> >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 28, 2014 at 1:41 AM, Sean Paul <seanpaul@chromium.org> wrote: > On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote: >> A set of helper functions are defined in this patch to make >> bridge driver probe independent of the drm flow. >> >> The bridge devices register themselves on a lookup table >> when they get probed by calling "drm_bridge_add". >> >> The parent encoder driver waits till the bridge is available >> in the lookup table(by calling "of_drm_find_bridge") and then >> continues with its initialization. >> >> The encoder driver should also call "drm_bridge_attach" to pass >> on the drm_device, encoder pointers to the bridge object. >> >> drm_bridge_attach inturn calls drm_bridge_init to register itself >> with the drm core. Later, it calls "bridge->funcs->attach" so that >> bridge can continue with other initializations. >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >> --- >> drivers/gpu/drm/Makefile | 1 + >> drivers/gpu/drm/bridge/Kconfig | 15 ++++- >> drivers/gpu/drm/drm_bridge.c | 102 ++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_crtc.c | 4 +- >> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 4 +- >> include/drm/drm_crtc.h | 12 +++- >> 6 files changed, 131 insertions(+), 7 deletions(-) >> create mode 100644 drivers/gpu/drm/drm_bridge.c >> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile >> index 4a55d59..bdbfb6f 100644 >> --- a/drivers/gpu/drm/Makefile >> +++ b/drivers/gpu/drm/Makefile >> @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \ >> drm-$(CONFIG_COMPAT) += drm_ioc32.o >> drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o >> drm-$(CONFIG_PCI) += ati_pcigart.o >> +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o >> drm-$(CONFIG_DRM_PANEL) += drm_panel.o >> drm-$(CONFIG_OF) += drm_of.o >> >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >> index 884923f..5a8e907 100644 >> --- a/drivers/gpu/drm/bridge/Kconfig >> +++ b/drivers/gpu/drm/bridge/Kconfig >> @@ -1,5 +1,16 @@ >> -config DRM_PTN3460 >> - tristate "PTN3460 DP/LVDS bridge" >> +config DRM_BRIDGE > > I'm not convinced this adds any value, to be honest. Hmm, then how to compile drm_bridge.c? > In addition to > whether or not it's useful, it seems like you'd need to stub the > drm_bridge_* functions that are declared in drm_crtc.h or break them > out into drm_bridge.h. Well, Thierry mentioned about this long back. Again, we have the confirmation now! Ajay [snip]
On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar <ajaynumb@gmail.com> wrote: > Hi Daniel and Sean, > > Thanks for the comments! > > On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <seanpaul@chromium.org> wrote: >> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> So don't ask why but I accidentally ended up in a branch looking at this >>> patch and didn't like it. So very quick&grumpy review. >>> >>> First, please make the patch subject more descriptive: I'd expect a helper >>> function scaffolding like the various crtc/probe/dp ... helpers we already >>> have. You instead add code to untangle the probe ordering. Please say so. > Sure. I will reword it properly. > >>> More comments below. >>> >>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote: >>>> A set of helper functions are defined in this patch to make >>>> bridge driver probe independent of the drm flow. >>>> >>>> The bridge devices register themselves on a lookup table >>>> when they get probed by calling "drm_bridge_add". >>>> >>>> The parent encoder driver waits till the bridge is available >>>> in the lookup table(by calling "of_drm_find_bridge") and then >>>> continues with its initialization. >>>> >>>> The encoder driver should also call "drm_bridge_attach" to pass >>>> on the drm_device, encoder pointers to the bridge object. >>>> >>>> drm_bridge_attach inturn calls drm_bridge_init to register itself >>>> with the drm core. Later, it calls "bridge->funcs->attach" so that >>>> bridge can continue with other initializations. >>>> >>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >>> >>> [snip] >>> >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >>>> * @driver_private: pointer to the bridge driver's internal context >>>> */ >>>> struct drm_bridge { >>>> - struct drm_device *dev; >>>> + struct device *dev; >>> >>> Please don't rename the ->dev pointer into drm. Because _all_ the other >>> drm structures still call it ->dev. Also, can't we use struct device_node >>> here like we do in the of helpers Russell added? See 7e435aad38083 >>> >> >> I think this is modeled after the naming in drm_panel, > Right, The entire rework is based on how drm_panel framework is structured. > >> FWIW. However, >> seems reasonable to keep the device_node instead. > Yes, its visible that just device_node would be sufficient. > This will save us from renaming drm_device as well. > >>>> + struct drm_device *drm; >>>> + struct drm_encoder *encoder; >>> >>> This breaks bridge->bridge chaining (if we ever get there). It seems >>> pretty much unused anyway except for an EBUSY check. Can't you use >>> bridge->dev for that? >>> >> >> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach >> and leave it up to the caller to establish the proper chain. > Ok. I will use drm_device pointer directly instead of passing encoder pointer. Hm, if you do this can you pls also update drm_panel accordingly? It shouldn't be a lot of fuzz and would make things around drm+dt more consistent. > >>>> struct list_head head; >>>> + struct list_head list; >>> >>> These lists need better names. I know that the "head" is really awful, >>> especially since it's actually not the head of the list but just an >>> element. >> >> I think we can just rip bridge_list out of mode_config if we're going >> to keep track of bridges elsewhere. So we can nuke "head" and keep >> "list". This also means that bridge->destroy() goes away, but that's >> probably Ok if everything converts to the standalone driver model >> where we have driver->remove() >> >> Sean > Great! Thierry actually mentioned about this once, and we have the > confirmation now. > >>>> >>>> struct drm_mode_object base; >>> >>> >>> Aside: I've noticed all this trying to update the kerneldoc for struct >>> drm_bridge, which just showed that this patch makes inconsistent changes. >>> Trying to write kerneldoc is a really great way to come up with better >>> interfaces imo. >>> >>> Cheers, Daniel > I din't get this actually. You want me to create Doc../drm_bridge.txt > or something similar? If you want to document drm_bridge then I recomment to sprinkle proper kerneldoc over drm_bridge.c and pull it all into the drm DocBook template. That way all the drm documentation is in one place. I've done that for drm_crtc.h in an unrelated patch series (but based upon a branch with your patch here included) and there's struct drm_bridge* in there. Hence why I've noticed. If you do kerneldoc/DocBook integration for drm_bridge it's probably best to also do it for drm_panel and use the opportunity to review/rework the interfaces a bit for consistency. E.g. move dt related stuff into drm_of.c and have that in a separate section in the docs. -Daniel
On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar <ajaynumb@gmail.com> wrote: >> Hi Daniel and Sean, >> >> Thanks for the comments! >> >> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <seanpaul@chromium.org> wrote: >>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> So don't ask why but I accidentally ended up in a branch looking at this >>>> patch and didn't like it. So very quick&grumpy review. >>>> >>>> First, please make the patch subject more descriptive: I'd expect a helper >>>> function scaffolding like the various crtc/probe/dp ... helpers we already >>>> have. You instead add code to untangle the probe ordering. Please say so. >> Sure. I will reword it properly. >> >>>> More comments below. >>>> >>>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote: >>>>> A set of helper functions are defined in this patch to make >>>>> bridge driver probe independent of the drm flow. >>>>> >>>>> The bridge devices register themselves on a lookup table >>>>> when they get probed by calling "drm_bridge_add". >>>>> >>>>> The parent encoder driver waits till the bridge is available >>>>> in the lookup table(by calling "of_drm_find_bridge") and then >>>>> continues with its initialization. >>>>> >>>>> The encoder driver should also call "drm_bridge_attach" to pass >>>>> on the drm_device, encoder pointers to the bridge object. >>>>> >>>>> drm_bridge_attach inturn calls drm_bridge_init to register itself >>>>> with the drm core. Later, it calls "bridge->funcs->attach" so that >>>>> bridge can continue with other initializations. >>>>> >>>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >>>> >>>> [snip] >>>> >>>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >>>>> * @driver_private: pointer to the bridge driver's internal context >>>>> */ >>>>> struct drm_bridge { >>>>> - struct drm_device *dev; >>>>> + struct device *dev; >>>> >>>> Please don't rename the ->dev pointer into drm. Because _all_ the other >>>> drm structures still call it ->dev. Also, can't we use struct device_node >>>> here like we do in the of helpers Russell added? See 7e435aad38083 >>>> >>> >>> I think this is modeled after the naming in drm_panel, >> Right, The entire rework is based on how drm_panel framework is structured. >> >>> FWIW. However, >>> seems reasonable to keep the device_node instead. >> Yes, its visible that just device_node would be sufficient. >> This will save us from renaming drm_device as well. >> >>>>> + struct drm_device *drm; >>>>> + struct drm_encoder *encoder; >>>> >>>> This breaks bridge->bridge chaining (if we ever get there). It seems >>>> pretty much unused anyway except for an EBUSY check. Can't you use >>>> bridge->dev for that? >>>> >>> >>> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach >>> and leave it up to the caller to establish the proper chain. >> Ok. I will use drm_device pointer directly instead of passing encoder pointer. > > Hm, if you do this can you pls also update drm_panel accordingly? It > shouldn't be a lot of fuzz and would make things around drm+dt more > consistent. Are you talking about using struct device_node instead of struct device? I guess you have misplaced the comment under the wrong section! >> >>>>> struct list_head head; >>>>> + struct list_head list; >>>> >>>> These lists need better names. I know that the "head" is really awful, >>>> especially since it's actually not the head of the list but just an >>>> element. >>> >>> I think we can just rip bridge_list out of mode_config if we're going >>> to keep track of bridges elsewhere. So we can nuke "head" and keep >>> "list". This also means that bridge->destroy() goes away, but that's >>> probably Ok if everything converts to the standalone driver model >>> where we have driver->remove() >>> >>> Sean >> Great! Thierry actually mentioned about this once, and we have the >> confirmation now. >> >>>>> >>>>> struct drm_mode_object base; >>>> >>>> >>>> Aside: I've noticed all this trying to update the kerneldoc for struct >>>> drm_bridge, which just showed that this patch makes inconsistent changes. >>>> Trying to write kerneldoc is a really great way to come up with better >>>> interfaces imo. >>>> >>>> Cheers, Daniel >> I din't get this actually. You want me to create Doc../drm_bridge.txt >> or something similar? > > If you want to document drm_bridge then I recomment to sprinkle proper > kerneldoc over drm_bridge.c and pull it all into the drm DocBook > template. That way all the drm documentation is in one place. I've > done that for drm_crtc.h in an unrelated patch series (but based upon > a branch with your patch here included) and there's struct drm_bridge* > in there. Hence why I've noticed. Can you send a link for that? And, is there any problem if the doc comes later? Ajay
On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <ajaynumb@gmail.com> wrote: > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar <ajaynumb@gmail.com> wrote: >>> Hi Daniel and Sean, >>> >>> Thanks for the comments! >>> >>> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <seanpaul@chromium.org> wrote: >>>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>>> So don't ask why but I accidentally ended up in a branch looking at this >>>>> patch and didn't like it. So very quick&grumpy review. >>>>> >>>>> First, please make the patch subject more descriptive: I'd expect a helper >>>>> function scaffolding like the various crtc/probe/dp ... helpers we already >>>>> have. You instead add code to untangle the probe ordering. Please say so. >>> Sure. I will reword it properly. >>> >>>>> More comments below. >>>>> >>>>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote: >>>>>> A set of helper functions are defined in this patch to make >>>>>> bridge driver probe independent of the drm flow. >>>>>> >>>>>> The bridge devices register themselves on a lookup table >>>>>> when they get probed by calling "drm_bridge_add". >>>>>> >>>>>> The parent encoder driver waits till the bridge is available >>>>>> in the lookup table(by calling "of_drm_find_bridge") and then >>>>>> continues with its initialization. >>>>>> >>>>>> The encoder driver should also call "drm_bridge_attach" to pass >>>>>> on the drm_device, encoder pointers to the bridge object. >>>>>> >>>>>> drm_bridge_attach inturn calls drm_bridge_init to register itself >>>>>> with the drm core. Later, it calls "bridge->funcs->attach" so that >>>>>> bridge can continue with other initializations. >>>>>> >>>>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >>>>> >>>>> [snip] >>>>> >>>>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >>>>>> * @driver_private: pointer to the bridge driver's internal context >>>>>> */ >>>>>> struct drm_bridge { >>>>>> - struct drm_device *dev; >>>>>> + struct device *dev; >>>>> >>>>> Please don't rename the ->dev pointer into drm. Because _all_ the other >>>>> drm structures still call it ->dev. Also, can't we use struct device_node >>>>> here like we do in the of helpers Russell added? See 7e435aad38083 >>>>> >>>> >>>> I think this is modeled after the naming in drm_panel, >>> Right, The entire rework is based on how drm_panel framework is structured. >>> >>>> FWIW. However, >>>> seems reasonable to keep the device_node instead. >>> Yes, its visible that just device_node would be sufficient. >>> This will save us from renaming drm_device as well. >>> >>>>>> + struct drm_device *drm; >>>>>> + struct drm_encoder *encoder; >>>>> >>>>> This breaks bridge->bridge chaining (if we ever get there). It seems >>>>> pretty much unused anyway except for an EBUSY check. Can't you use >>>>> bridge->dev for that? >>>>> >>>> >>>> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach >>>> and leave it up to the caller to establish the proper chain. >>> Ok. I will use drm_device pointer directly instead of passing encoder pointer. >> >> Hm, if you do this can you pls also update drm_panel accordingly? It >> shouldn't be a lot of fuzz and would make things around drm+dt more >> consistent. > Are you talking about using struct device_node instead of struct device? > I guess you have misplaced the comment under the wrong section! Yeah, that should have been one up ;-) >>>>>> struct list_head head; >>>>>> + struct list_head list; >>>>> >>>>> These lists need better names. I know that the "head" is really awful, >>>>> especially since it's actually not the head of the list but just an >>>>> element. >>>> >>>> I think we can just rip bridge_list out of mode_config if we're going >>>> to keep track of bridges elsewhere. So we can nuke "head" and keep >>>> "list". This also means that bridge->destroy() goes away, but that's >>>> probably Ok if everything converts to the standalone driver model >>>> where we have driver->remove() >>>> >>>> Sean >>> Great! Thierry actually mentioned about this once, and we have the >>> confirmation now. >>> >>>>>> >>>>>> struct drm_mode_object base; >>>>> >>>>> >>>>> Aside: I've noticed all this trying to update the kerneldoc for struct >>>>> drm_bridge, which just showed that this patch makes inconsistent changes. >>>>> Trying to write kerneldoc is a really great way to come up with better >>>>> interfaces imo. >>>>> >>>>> Cheers, Daniel >>> I din't get this actually. You want me to create Doc../drm_bridge.txt >>> or something similar? >> >> If you want to document drm_bridge then I recomment to sprinkle proper >> kerneldoc over drm_bridge.c and pull it all into the drm DocBook >> template. That way all the drm documentation is in one place. I've >> done that for drm_crtc.h in an unrelated patch series (but based upon >> a branch with your patch here included) and there's struct drm_bridge* >> in there. Hence why I've noticed. > Can you send a link for that? > And, is there any problem if the doc comes later? Since quite a while we've asked for the kerneldoc polish as part of each drm core patch series. It's just that drm_bridge/panel kinda have flown under the radar of the people usually asking for docs ;-) -Daniel
On Tue, Oct 28, 2014 at 10:19 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <ajaynumb@gmail.com> wrote: >> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar <ajaynumb@gmail.com> wrote: >>>> Hi Daniel and Sean, >>>> >>>> Thanks for the comments! >>>> >>>> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <seanpaul@chromium.org> wrote: >>>>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>>>> So don't ask why but I accidentally ended up in a branch looking at this >>>>>> patch and didn't like it. So very quick&grumpy review. >>>>>> >>>>>> First, please make the patch subject more descriptive: I'd expect a helper >>>>>> function scaffolding like the various crtc/probe/dp ... helpers we already >>>>>> have. You instead add code to untangle the probe ordering. Please say so. >>>> Sure. I will reword it properly. >>>> >>>>>> More comments below. >>>>>> >>>>>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote: >>>>>>> A set of helper functions are defined in this patch to make >>>>>>> bridge driver probe independent of the drm flow. >>>>>>> >>>>>>> The bridge devices register themselves on a lookup table >>>>>>> when they get probed by calling "drm_bridge_add". >>>>>>> >>>>>>> The parent encoder driver waits till the bridge is available >>>>>>> in the lookup table(by calling "of_drm_find_bridge") and then >>>>>>> continues with its initialization. >>>>>>> >>>>>>> The encoder driver should also call "drm_bridge_attach" to pass >>>>>>> on the drm_device, encoder pointers to the bridge object. >>>>>>> >>>>>>> drm_bridge_attach inturn calls drm_bridge_init to register itself >>>>>>> with the drm core. Later, it calls "bridge->funcs->attach" so that >>>>>>> bridge can continue with other initializations. >>>>>>> >>>>>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >>>>>> >>>>>> [snip] >>>>>> >>>>>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >>>>>>> * @driver_private: pointer to the bridge driver's internal context >>>>>>> */ >>>>>>> struct drm_bridge { >>>>>>> - struct drm_device *dev; >>>>>>> + struct device *dev; >>>>>> >>>>>> Please don't rename the ->dev pointer into drm. Because _all_ the other >>>>>> drm structures still call it ->dev. Also, can't we use struct device_node >>>>>> here like we do in the of helpers Russell added? See 7e435aad38083 >>>>>> >>>>> >>>>> I think this is modeled after the naming in drm_panel, >>>> Right, The entire rework is based on how drm_panel framework is structured. >>>> >>>>> FWIW. However, >>>>> seems reasonable to keep the device_node instead. >>>> Yes, its visible that just device_node would be sufficient. >>>> This will save us from renaming drm_device as well. >>>> >>>>>>> + struct drm_device *drm; >>>>>>> + struct drm_encoder *encoder; >>>>>> >>>>>> This breaks bridge->bridge chaining (if we ever get there). It seems >>>>>> pretty much unused anyway except for an EBUSY check. Can't you use >>>>>> bridge->dev for that? >>>>>> >>>>> >>>>> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach >>>>> and leave it up to the caller to establish the proper chain. >>>> Ok. I will use drm_device pointer directly instead of passing encoder pointer. >>> >>> Hm, if you do this can you pls also update drm_panel accordingly? It >>> shouldn't be a lot of fuzz and would make things around drm+dt more >>> consistent. >> Are you talking about using struct device_node instead of struct device? >> I guess you have misplaced the comment under the wrong section! > > Yeah, that should have been one up ;-) > >>>>>>> struct list_head head; >>>>>>> + struct list_head list; >>>>>> >>>>>> These lists need better names. I know that the "head" is really awful, >>>>>> especially since it's actually not the head of the list but just an >>>>>> element. >>>>> >>>>> I think we can just rip bridge_list out of mode_config if we're going >>>>> to keep track of bridges elsewhere. So we can nuke "head" and keep >>>>> "list". This also means that bridge->destroy() goes away, but that's >>>>> probably Ok if everything converts to the standalone driver model >>>>> where we have driver->remove() >>>>> >>>>> Sean >>>> Great! Thierry actually mentioned about this once, and we have the >>>> confirmation now. >>>> >>>>>>> >>>>>>> struct drm_mode_object base; >>>>>> >>>>>> >>>>>> Aside: I've noticed all this trying to update the kerneldoc for struct >>>>>> drm_bridge, which just showed that this patch makes inconsistent changes. >>>>>> Trying to write kerneldoc is a really great way to come up with better >>>>>> interfaces imo. >>>>>> >>>>>> Cheers, Daniel >>>> I din't get this actually. You want me to create Doc../drm_bridge.txt >>>> or something similar? >>> >>> If you want to document drm_bridge then I recomment to sprinkle proper >>> kerneldoc over drm_bridge.c and pull it all into the drm DocBook >>> template. That way all the drm documentation is in one place. I've >>> done that for drm_crtc.h in an unrelated patch series (but based upon >>> a branch with your patch here included) and there's struct drm_bridge* >>> in there. Hence why I've noticed. >> Can you send a link for that? >> And, is there any problem if the doc comes later? > > Since quite a while we've asked for the kerneldoc polish as part of > each drm core patch series. It's just that drm_bridge/panel kinda have > flown under the radar of the people usually asking for docs ;-) Heh, sorry about that. Sean > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote: > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > >>> * @driver_private: pointer to the bridge driver's internal context > >>> */ > >>> struct drm_bridge { > >>> - struct drm_device *dev; > >>> + struct device *dev; > >> > >> Please don't rename the ->dev pointer into drm. Because _all_ the other > >> drm structures still call it ->dev. Also, can't we use struct device_node > >> here like we do in the of helpers Russell added? See 7e435aad38083 > >> > > > > I think this is modeled after the naming in drm_panel, FWIW. However, > > seems reasonable to keep the device_node instead. > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > drm_crtc drop the struct device and go directly to a struct > device_node. Since we don't really need the sturct device, the only > thing we care about is the of_node. For added bonus wrap an #ifdef > CONFIG_OF around all the various struct device_node in drm_foo.h. > Should be all fairly simple to pull off with cocci. > > Thierry? The struct device * is in DRM panel because there's nothing device tree specific about the concept. Having a struct device_node * instead would indicate that it can only be used with a device tree, whereas the framework doesn't care the tiniest bit what type of device we have. While the trend clearly is to use more device tree, I don't think we should make it impossible for anybody else to use these frameworks. There are other advantages to keeping a struct device *, like having access to the proper device and its name. Also you get access to the device_node * via dev->of_node anyway. I don't see any advantage in switching to just a struct device_node *, only disadvantages. Thierry
On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote: > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > >>>> * @driver_private: pointer to the bridge driver's internal context > >>>> */ > >>>> struct drm_bridge { > >>>> - struct drm_device *dev; > >>>> + struct device *dev; > >>> > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other > >>> drm structures still call it ->dev. Also, can't we use struct device_node > >>> here like we do in the of helpers Russell added? See 7e435aad38083 > >>> > >> > >> I think this is modeled after the naming in drm_panel, FWIW. However, > >> seems reasonable to keep the device_node instead. > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > > drm_crtc drop the struct device and go directly to a struct > > device_node. Since we don't really need the sturct device, the only > > thing we care about is the of_node. For added bonus wrap an #ifdef > > CONFIG_OF around all the various struct device_node in drm_foo.h. > > Should be all fairly simple to pull off with cocci. > > > > Thierry? > > Looking at the of_drm_find_panel function I actually wonder how that > works - the drm_panel doesn't really need to stick around afaics. > After all panel_list is global so some other driver can unload. > Russell's of support for possible_crtcs code works differently since > it only looks at per-drm_device lists. I don't understand. Panels are global resources that get registered by some driver that isn't tied to the DRM device until attached. It can't be in a per-DRM device list, because it's external to the device. And yes, they can go away when a driver is unloaded, though it's not the typical use-case. > This bridge code here though suffers from the same. So to me this > looks rather fishy. Well, this version of the DRM bridge support was written to be close to DRM panel, so it isn't really surprising that it's similar =), but like I said, I don't really understand what you think is wrong with it. > It doesn't help that we have drm_of.[hc] around but not all the of > code is in there. Adding Russell too. DRM panel and DRM bridge aren't just OF helpers. They can be used with whatever type of device you want. Thierry
On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote: > On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <ajaynumb@gmail.com> wrote: > > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: [...] > >> Hm, if you do this can you pls also update drm_panel accordingly? It > >> shouldn't be a lot of fuzz and would make things around drm+dt more > >> consistent. > > Are you talking about using struct device_node instead of struct device? > > I guess you have misplaced the comment under the wrong section! > > Yeah, that should have been one up ;-) Like I said earlier, I don't think dropping struct device * in favour of struct device_node * is a good idea. > >> If you want to document drm_bridge then I recomment to sprinkle proper > >> kerneldoc over drm_bridge.c and pull it all into the drm DocBook > >> template. That way all the drm documentation is in one place. I've > >> done that for drm_crtc.h in an unrelated patch series (but based upon > >> a branch with your patch here included) and there's struct drm_bridge* > >> in there. Hence why I've noticed. > > Can you send a link for that? > > And, is there any problem if the doc comes later? > > Since quite a while we've asked for the kerneldoc polish as part of > each drm core patch series. It's just that drm_bridge/panel kinda have > flown under the radar of the people usually asking for docs ;-) FWIW, there's some kerneldoc in include/drm/drm_panel.h but I guess I could write up something more complete and integrate it into DocBook. Thierry
On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote: >> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <ajaynumb@gmail.com> wrote: >> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > [...] >> >> Hm, if you do this can you pls also update drm_panel accordingly? It >> >> shouldn't be a lot of fuzz and would make things around drm+dt more >> >> consistent. >> > Are you talking about using struct device_node instead of struct device? >> > I guess you have misplaced the comment under the wrong section! >> >> Yeah, that should have been one up ;-) > > Like I said earlier, I don't think dropping struct device * in favour of > struct device_node * is a good idea. I am not sure about drm_panel. But, I am not really doing anything with the struct device pointer in case of bridge. So, just wondering if it is really needed? Ajay
On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote: > On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote: > >> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <ajaynumb@gmail.com> wrote: > >> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > [...] > >> >> Hm, if you do this can you pls also update drm_panel accordingly? It > >> >> shouldn't be a lot of fuzz and would make things around drm+dt more > >> >> consistent. > >> > Are you talking about using struct device_node instead of struct device? > >> > I guess you have misplaced the comment under the wrong section! > >> > >> Yeah, that should have been one up ;-) > > > > Like I said earlier, I don't think dropping struct device * in favour of > > struct device_node * is a good idea. > I am not sure about drm_panel. > But, I am not really doing anything with the struct device pointer in > case of bridge. > So, just wondering if it is really needed? I think it's useful to have it just to send the right message. DRM panel and DRM bridge aren't specific to device tree. They are completely generic and can work with any type of device, whether it was instantiated from the device tree or some other infrastructure. Dropping struct device * will make it effectively useless on anything but DT. I don't think we should strive for that, even if only DT-enabled platforms currently use them. Thierry
On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote: > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote: > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > > >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > > >>>> * @driver_private: pointer to the bridge driver's internal context > > >>>> */ > > >>>> struct drm_bridge { > > >>>> - struct drm_device *dev; > > >>>> + struct device *dev; > > >>> > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other > > >>> drm structures still call it ->dev. Also, can't we use struct device_node > > >>> here like we do in the of helpers Russell added? See 7e435aad38083 > > >>> > > >> > > >> I think this is modeled after the naming in drm_panel, FWIW. However, > > >> seems reasonable to keep the device_node instead. > > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > > > drm_crtc drop the struct device and go directly to a struct > > > device_node. Since we don't really need the sturct device, the only > > > thing we care about is the of_node. For added bonus wrap an #ifdef > > > CONFIG_OF around all the various struct device_node in drm_foo.h. > > > Should be all fairly simple to pull off with cocci. > > > > > > Thierry? > > > > Looking at the of_drm_find_panel function I actually wonder how that > > works - the drm_panel doesn't really need to stick around afaics. > > After all panel_list is global so some other driver can unload. > > Russell's of support for possible_crtcs code works differently since > > it only looks at per-drm_device lists. > > I don't understand. Panels are global resources that get registered by > some driver that isn't tied to the DRM device until attached. It can't > be in a per-DRM device list, because it's external to the device. > > And yes, they can go away when a driver is unloaded, though it's not the > typical use-case. > > > This bridge code here though suffers from the same. So to me this > > looks rather fishy. > > Well, this version of the DRM bridge support was written to be close to > DRM panel, so it isn't really surprising that it's similar =), but like > I said, I don't really understand what you think is wrong with it. You have a mutex to protect the global list of bridges/panels. But if you do a lookup you don't grab a reference count on the panel, so the moment you drop the list mutex the panel/bridge can go away. Yes usually you don't unload a driver on a soc but soc isn't everything and designing new stuff to not be hotunplug save isn't great either. -Daniel
On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote: > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote: > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > > >>> * @driver_private: pointer to the bridge driver's internal context > > >>> */ > > >>> struct drm_bridge { > > >>> - struct drm_device *dev; > > >>> + struct device *dev; > > >> > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other > > >> drm structures still call it ->dev. Also, can't we use struct device_node > > >> here like we do in the of helpers Russell added? See 7e435aad38083 > > >> > > > > > > I think this is modeled after the naming in drm_panel, FWIW. However, > > > seems reasonable to keep the device_node instead. > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > > drm_crtc drop the struct device and go directly to a struct > > device_node. Since we don't really need the sturct device, the only > > thing we care about is the of_node. For added bonus wrap an #ifdef > > CONFIG_OF around all the various struct device_node in drm_foo.h. > > Should be all fairly simple to pull off with cocci. > > > > Thierry? > > The struct device * is in DRM panel because there's nothing device tree > specific about the concept. Having a struct device_node * instead would > indicate that it can only be used with a device tree, whereas the > framework doesn't care the tiniest bit what type of device we have. > > While the trend clearly is to use more device tree, I don't think we > should make it impossible for anybody else to use these frameworks. > > There are other advantages to keeping a struct device *, like having > access to the proper device and its name. Also you get access to the > device_node * via dev->of_node anyway. I don't see any advantage in > switching to just a struct device_node *, only disadvantages. Well the idea is to make the lookup key specific, and conditional on #CONFIG_OF. If there's going to be another neat way to enumerate platform devices then I think we should add that, too. Or maybe we should have a void *platform_data or so. The reason I really don't want a struct device * in core drm structures is that two releases down the road people will have found tons of really great ways to abuse them and re-create a midlayer. DRM core really should only care about the sw objects and not be hw specific at all. Heck there's not even an requirement to have any piece of actual hw, you could write a completely fake drm driver (for e.g. testing like the new v4l driver). Tbh I wonder a bit why we even have this registery embedded into the core drm objects. Essentially the only thing you're doing is a list that maps some platform specific key onto some subsystem specific driver structure or fails the lookup. So instead of putting all these low-level details into drm core structures can't we just have a generic hashtable/list for this, plus some static inline helpers that cast the void * you get into the one you want? I also get the feeling that this really should be in the driver core (like the component helpers), and that we should think a lot harder about lifetimes and refcounting (see my other reply on that). -Daniel
On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote: > On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote: > > On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding > > <thierry.reding@gmail.com> wrote: > > > On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote: > > >> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <ajaynumb@gmail.com> wrote: > > >> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > [...] > > >> >> Hm, if you do this can you pls also update drm_panel accordingly? It > > >> >> shouldn't be a lot of fuzz and would make things around drm+dt more > > >> >> consistent. > > >> > Are you talking about using struct device_node instead of struct device? > > >> > I guess you have misplaced the comment under the wrong section! > > >> > > >> Yeah, that should have been one up ;-) > > > > > > Like I said earlier, I don't think dropping struct device * in favour of > > > struct device_node * is a good idea. > > I am not sure about drm_panel. > > But, I am not really doing anything with the struct device pointer in > > case of bridge. > > So, just wondering if it is really needed? > > I think it's useful to have it just to send the right message. DRM panel > and DRM bridge aren't specific to device tree. They are completely > generic and can work with any type of device, whether it was > instantiated from the device tree or some other infrastructure. Dropping > struct device * will make it effectively useless on anything but DT. I > don't think we should strive for that, even if only DT-enabled platforms > currently use them. See my other reply, but I now think we should put neither into drm structures. This "find me the driver structures for this device" problem looks really generic, and it has nothing to do with the drm structures and concepts like bridges/panels at all. It shouldn't be in there at all. Adding it looks very much like reintroducing the drm midlayer that we just finally made obsolete, just not at the top-level (struct drm_device) but at a bunch of leaf nodes. I expect all the same issues though. And I'm definitely not looking to de-midlayer more stuff that we're just adding. Imo this should be solved as a separate helper thing, maybe in the driver core akin to the component helpers from Russell. -Daniel
On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote: > On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote: > > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote: > > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > > > >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > > > >>>> * @driver_private: pointer to the bridge driver's internal context > > > >>>> */ > > > >>>> struct drm_bridge { > > > >>>> - struct drm_device *dev; > > > >>>> + struct device *dev; > > > >>> > > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other > > > >>> drm structures still call it ->dev. Also, can't we use struct device_node > > > >>> here like we do in the of helpers Russell added? See 7e435aad38083 > > > >>> > > > >> > > > >> I think this is modeled after the naming in drm_panel, FWIW. However, > > > >> seems reasonable to keep the device_node instead. > > > > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > > > > drm_crtc drop the struct device and go directly to a struct > > > > device_node. Since we don't really need the sturct device, the only > > > > thing we care about is the of_node. For added bonus wrap an #ifdef > > > > CONFIG_OF around all the various struct device_node in drm_foo.h. > > > > Should be all fairly simple to pull off with cocci. > > > > > > > > Thierry? > > > > > > Looking at the of_drm_find_panel function I actually wonder how that > > > works - the drm_panel doesn't really need to stick around afaics. > > > After all panel_list is global so some other driver can unload. > > > Russell's of support for possible_crtcs code works differently since > > > it only looks at per-drm_device lists. > > > > I don't understand. Panels are global resources that get registered by > > some driver that isn't tied to the DRM device until attached. It can't > > be in a per-DRM device list, because it's external to the device. > > > > And yes, they can go away when a driver is unloaded, though it's not the > > typical use-case. > > > > > This bridge code here though suffers from the same. So to me this > > > looks rather fishy. > > > > Well, this version of the DRM bridge support was written to be close to > > DRM panel, so it isn't really surprising that it's similar =), but like > > I said, I don't really understand what you think is wrong with it. > > You have a mutex to protect the global list of bridges/panels. But if you > do a lookup you don't grab a reference count on the panel, so the moment > you drop the list mutex the panel/bridge can go away. > > Yes usually you don't unload a driver on a soc but soc isn't everything > and designing new stuff to not be hotunplug save isn't great either. Yeah, I certainly agree that adding proper reference counting would be a good thing. I think perhaps we could just take a reference on the struct device * to prevent it from disappearing. Although perhaps I misunderstand what you mean by "go away". Thierry
On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote: > On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote: > > On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote: > > > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote: > > > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > > > > >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > > > > >>>> * @driver_private: pointer to the bridge driver's internal context > > > > >>>> */ > > > > >>>> struct drm_bridge { > > > > >>>> - struct drm_device *dev; > > > > >>>> + struct device *dev; > > > > >>> > > > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other > > > > >>> drm structures still call it ->dev. Also, can't we use struct device_node > > > > >>> here like we do in the of helpers Russell added? See 7e435aad38083 > > > > >>> > > > > >> > > > > >> I think this is modeled after the naming in drm_panel, FWIW. However, > > > > >> seems reasonable to keep the device_node instead. > > > > > > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > > > > > drm_crtc drop the struct device and go directly to a struct > > > > > device_node. Since we don't really need the sturct device, the only > > > > > thing we care about is the of_node. For added bonus wrap an #ifdef > > > > > CONFIG_OF around all the various struct device_node in drm_foo.h. > > > > > Should be all fairly simple to pull off with cocci. > > > > > > > > > > Thierry? > > > > > > > > Looking at the of_drm_find_panel function I actually wonder how that > > > > works - the drm_panel doesn't really need to stick around afaics. > > > > After all panel_list is global so some other driver can unload. > > > > Russell's of support for possible_crtcs code works differently since > > > > it only looks at per-drm_device lists. > > > > > > I don't understand. Panels are global resources that get registered by > > > some driver that isn't tied to the DRM device until attached. It can't > > > be in a per-DRM device list, because it's external to the device. > > > > > > And yes, they can go away when a driver is unloaded, though it's not the > > > typical use-case. > > > > > > > This bridge code here though suffers from the same. So to me this > > > > looks rather fishy. > > > > > > Well, this version of the DRM bridge support was written to be close to > > > DRM panel, so it isn't really surprising that it's similar =), but like > > > I said, I don't really understand what you think is wrong with it. > > > > You have a mutex to protect the global list of bridges/panels. But if you > > do a lookup you don't grab a reference count on the panel, so the moment > > you drop the list mutex the panel/bridge can go away. > > > > Yes usually you don't unload a driver on a soc but soc isn't everything > > and designing new stuff to not be hotunplug save isn't great either. > > Yeah, I certainly agree that adding proper reference counting would be a > good thing. I think perhaps we could just take a reference on the struct > device * to prevent it from disappearing. > > Although perhaps I misunderstand what you mean by "go away". I meant the drm_panel/bridge could go away any second, since nothing prevents a module unload of the panel/bridge driver. So in theory you could get the unregister call right after you've done the lookup. Which means the bridge/panel pointer you've just returned is freed memory. I think we nee try_get_module for the code and kref on the actual data structures. That makes the entire thing a bit non-trivial, which is why I think it would be better as some generic helper. Which then gets embedded or instantiated for specific cases, like dt&drm_panel or dt&drm_bridge. Or maybe even acpi&drm_bridge, who knows ;-) -Daniel
On 10/29/2014 08:58 AM, Daniel Vetter wrote: > On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote: >> On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote: >>> On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding >>> <thierry.reding@gmail.com> wrote: >>>> On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote: >>>>> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <ajaynumb@gmail.com> wrote: >>>>>> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> [...] >>>>>>> Hm, if you do this can you pls also update drm_panel accordingly? It >>>>>>> shouldn't be a lot of fuzz and would make things around drm+dt more >>>>>>> consistent. >>>>>> Are you talking about using struct device_node instead of struct device? >>>>>> I guess you have misplaced the comment under the wrong section! >>>>> >>>>> Yeah, that should have been one up ;-) >>>> >>>> Like I said earlier, I don't think dropping struct device * in favour of >>>> struct device_node * is a good idea. >>> I am not sure about drm_panel. >>> But, I am not really doing anything with the struct device pointer in >>> case of bridge. >>> So, just wondering if it is really needed? >> >> I think it's useful to have it just to send the right message. DRM panel >> and DRM bridge aren't specific to device tree. They are completely >> generic and can work with any type of device, whether it was >> instantiated from the device tree or some other infrastructure. Dropping >> struct device * will make it effectively useless on anything but DT. I >> don't think we should strive for that, even if only DT-enabled platforms >> currently use them. > > See my other reply, but I now think we should put neither into drm > structures. This "find me the driver structures for this device" problem > looks really generic, and it has nothing to do with the drm structures and > concepts like bridges/panels at all. It shouldn't be in there at all. > > Adding it looks very much like reintroducing the drm midlayer that we just > finally made obsolete, just not at the top-level (struct drm_device) but > at a bunch of leaf nodes. I expect all the same issues though. And I'm > definitely not looking to de-midlayer more stuff that we're just adding. > > Imo this should be solved as a separate helper thing, maybe in the driver > core akin to the component helpers from Russell. > -Daniel > As I understand you want something generic to look for panels, bridges, whatever and, like components, it should allow to safe hot plug/unplug. I have proposed such thing few months ago [1]. Have you looked at it? [1]: https://lkml.org/lkml/2014/4/30/345 Regards Andrzej
On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote: > On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote: > > On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote: > > > On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote: > > > > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote: > > > > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > > > > > >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > > > > > >>>> * @driver_private: pointer to the bridge driver's internal context > > > > > >>>> */ > > > > > >>>> struct drm_bridge { > > > > > >>>> - struct drm_device *dev; > > > > > >>>> + struct device *dev; > > > > > >>> > > > > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other > > > > > >>> drm structures still call it ->dev. Also, can't we use struct device_node > > > > > >>> here like we do in the of helpers Russell added? See 7e435aad38083 > > > > > >>> > > > > > >> > > > > > >> I think this is modeled after the naming in drm_panel, FWIW. However, > > > > > >> seems reasonable to keep the device_node instead. > > > > > > > > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > > > > > > drm_crtc drop the struct device and go directly to a struct > > > > > > device_node. Since we don't really need the sturct device, the only > > > > > > thing we care about is the of_node. For added bonus wrap an #ifdef > > > > > > CONFIG_OF around all the various struct device_node in drm_foo.h. > > > > > > Should be all fairly simple to pull off with cocci. > > > > > > > > > > > > Thierry? > > > > > > > > > > Looking at the of_drm_find_panel function I actually wonder how that > > > > > works - the drm_panel doesn't really need to stick around afaics. > > > > > After all panel_list is global so some other driver can unload. > > > > > Russell's of support for possible_crtcs code works differently since > > > > > it only looks at per-drm_device lists. > > > > > > > > I don't understand. Panels are global resources that get registered by > > > > some driver that isn't tied to the DRM device until attached. It can't > > > > be in a per-DRM device list, because it's external to the device. > > > > > > > > And yes, they can go away when a driver is unloaded, though it's not the > > > > typical use-case. > > > > > > > > > This bridge code here though suffers from the same. So to me this > > > > > looks rather fishy. > > > > > > > > Well, this version of the DRM bridge support was written to be close to > > > > DRM panel, so it isn't really surprising that it's similar =), but like > > > > I said, I don't really understand what you think is wrong with it. > > > > > > You have a mutex to protect the global list of bridges/panels. But if you > > > do a lookup you don't grab a reference count on the panel, so the moment > > > you drop the list mutex the panel/bridge can go away. > > > > > > Yes usually you don't unload a driver on a soc but soc isn't everything > > > and designing new stuff to not be hotunplug save isn't great either. > > > > Yeah, I certainly agree that adding proper reference counting would be a > > good thing. I think perhaps we could just take a reference on the struct > > device * to prevent it from disappearing. > > > > Although perhaps I misunderstand what you mean by "go away". > > I meant the drm_panel/bridge could go away any second, since nothing > prevents a module unload of the panel/bridge driver. So in theory you > could get the unregister call right after you've done the lookup. Which > means the bridge/panel pointer you've just returned is freed memory. Ah yes, I see now. > I think we nee try_get_module for the code and kref on the actual data > structures. Agreed, that should do the trick. We'd probably need some sort of logic to also make operations return something like -ENODEV when the underlying device has disappeared. I think David had introduced something similar for DRM device not so long ago? > That makes the entire thing a bit non-trivial, which is why I think it > would be better as some generic helper. Which then gets embedded or > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge. > Or maybe even acpi&drm_bridge, who knows ;-) I worry a little about type safety. How will this generic helper know what registry to look in for? Or conversely, if all these resources are added to a single registry how do you know that they're of the correct type? Failing to ensure this could cause situations where you're asking for a panel and get a bridge in return because you've wrongly wired it up in device tree for example. But perhaps if both the registry and the device parts are turned into helpers we could still have a single core implementation and then instantiate that for each type, something roughly like this: struct registry { struct list_head list; struct mutex lock; }; struct registry_record { struct list_head list; struct module *owner; struct kref *ref; struct device *dev; }; int registry_add(struct registry *registry, struct registry_record *record) { ... try_module_get(record->owner); ... } struct registry_record *registry_find_by_of_node(struct registry *registry, struct device_node *np) { ... kref_get(...); ... } That way it should be possible to embed these into other structures, like so: struct drm_panel { struct registry_record record; ... }; static struct registry drm_panels; int drm_panel_add(struct drm_panel *panel) { return registry_add(&drm_panels, &panel->record); } struct drm_panel *of_drm_panel_find(struct device_node *np) { struct registry_record *record; record = registry_find_by_of_node(&drm_panels, np); return container_of(record, struct drm_panel, record); } Is that what you had in mind? Thierry
On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote: > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote: > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote: > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > > > >>> * @driver_private: pointer to the bridge driver's internal context > > > >>> */ > > > >>> struct drm_bridge { > > > >>> - struct drm_device *dev; > > > >>> + struct device *dev; > > > >> > > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other > > > >> drm structures still call it ->dev. Also, can't we use struct device_node > > > >> here like we do in the of helpers Russell added? See 7e435aad38083 > > > >> > > > > > > > > I think this is modeled after the naming in drm_panel, FWIW. However, > > > > seems reasonable to keep the device_node instead. > > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > > > drm_crtc drop the struct device and go directly to a struct > > > device_node. Since we don't really need the sturct device, the only > > > thing we care about is the of_node. For added bonus wrap an #ifdef > > > CONFIG_OF around all the various struct device_node in drm_foo.h. > > > Should be all fairly simple to pull off with cocci. > > > > > > Thierry? > > > > The struct device * is in DRM panel because there's nothing device tree > > specific about the concept. Having a struct device_node * instead would > > indicate that it can only be used with a device tree, whereas the > > framework doesn't care the tiniest bit what type of device we have. > > > > While the trend clearly is to use more device tree, I don't think we > > should make it impossible for anybody else to use these frameworks. > > > > There are other advantages to keeping a struct device *, like having > > access to the proper device and its name. Also you get access to the > > device_node * via dev->of_node anyway. I don't see any advantage in > > switching to just a struct device_node *, only disadvantages. > > Well the idea is to make the lookup key specific, and conditional on > #CONFIG_OF. If there's going to be another neat way to enumerate platform > devices then I think we should add that, too. Or maybe we should have a > void *platform_data or so. > > The reason I really don't want a struct device * in core drm structures is > that two releases down the road people will have found tons of really > great ways to abuse them and re-create a midlayer. DRM core really should > only care about the sw objects and not be hw specific at all. Heck there's > not even an requirement to have any piece of actual hw, you could write a > completely fake drm driver (for e.g. testing like the new v4l driver). > > Tbh I wonder a bit why we even have this registery embedded into the core > drm objects. Essentially the only thing you're doing is a list that maps > some platform specific key onto some subsystem specific driver structure > or fails the lookup. So instead of putting all these low-level details > into drm core structures can't we just have a generic hashtable/list for > this, plus some static inline helpers that cast the void * you get into > the one you want? > > I also get the feeling that this really should be in the driver core (like > the component helpers), and that we should think a lot harder about > lifetimes and refcounting (see my other reply on that). Yes, that sounds very useful indeed. Also see my reply to yours. =) Thierry
On 10/29/2014 10:14 AM, Thierry Reding wrote: > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote: >> On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote: >>> On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote: >>>> On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote: >>>>> On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote: >>>>>> On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>>>>> On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: >>>>>>>>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >>>>>>>>>> * @driver_private: pointer to the bridge driver's internal context >>>>>>>>>> */ >>>>>>>>>> struct drm_bridge { >>>>>>>>>> - struct drm_device *dev; >>>>>>>>>> + struct device *dev; >>>>>>>>> >>>>>>>>> Please don't rename the ->dev pointer into drm. Because _all_ the other >>>>>>>>> drm structures still call it ->dev. Also, can't we use struct device_node >>>>>>>>> here like we do in the of helpers Russell added? See 7e435aad38083 >>>>>>>>> >>>>>>>> >>>>>>>> I think this is modeled after the naming in drm_panel, FWIW. However, >>>>>>>> seems reasonable to keep the device_node instead. >>>>>>> >>>>>>> Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with >>>>>>> drm_crtc drop the struct device and go directly to a struct >>>>>>> device_node. Since we don't really need the sturct device, the only >>>>>>> thing we care about is the of_node. For added bonus wrap an #ifdef >>>>>>> CONFIG_OF around all the various struct device_node in drm_foo.h. >>>>>>> Should be all fairly simple to pull off with cocci. >>>>>>> >>>>>>> Thierry? >>>>>> >>>>>> Looking at the of_drm_find_panel function I actually wonder how that >>>>>> works - the drm_panel doesn't really need to stick around afaics. >>>>>> After all panel_list is global so some other driver can unload. >>>>>> Russell's of support for possible_crtcs code works differently since >>>>>> it only looks at per-drm_device lists. >>>>> >>>>> I don't understand. Panels are global resources that get registered by >>>>> some driver that isn't tied to the DRM device until attached. It can't >>>>> be in a per-DRM device list, because it's external to the device. >>>>> >>>>> And yes, they can go away when a driver is unloaded, though it's not the >>>>> typical use-case. >>>>> >>>>>> This bridge code here though suffers from the same. So to me this >>>>>> looks rather fishy. >>>>> >>>>> Well, this version of the DRM bridge support was written to be close to >>>>> DRM panel, so it isn't really surprising that it's similar =), but like >>>>> I said, I don't really understand what you think is wrong with it. >>>> >>>> You have a mutex to protect the global list of bridges/panels. But if you >>>> do a lookup you don't grab a reference count on the panel, so the moment >>>> you drop the list mutex the panel/bridge can go away. >>>> >>>> Yes usually you don't unload a driver on a soc but soc isn't everything >>>> and designing new stuff to not be hotunplug save isn't great either. >>> >>> Yeah, I certainly agree that adding proper reference counting would be a >>> good thing. I think perhaps we could just take a reference on the struct >>> device * to prevent it from disappearing. >>> >>> Although perhaps I misunderstand what you mean by "go away". >> >> I meant the drm_panel/bridge could go away any second, since nothing >> prevents a module unload of the panel/bridge driver. So in theory you >> could get the unregister call right after you've done the lookup. Which >> means the bridge/panel pointer you've just returned is freed memory. > > Ah yes, I see now. > >> I think we nee try_get_module for the code and kref on the actual data >> structures. > > Agreed, that should do the trick. We'd probably need some sort of logic > to also make operations return something like -ENODEV when the > underlying device has disappeared. I think David had introduced > something similar for DRM device not so long ago? If the underlying device disappears it would be good to receive notification anyway to trigger DRM HPD event. And if we have the notification, we can release references to the device smoothly. We do not need to play tricky games with krefs, zombie data and module refcounting. On the other side component framework uses notification callbacks bind/unbind for master and components to smoothly attach/release devices, why should it be done differently in this case. Again, look at interface_tracker [1] it does exactly what you need. [1]: https://lkml.org/lkml/2014/4/30/345 Regards Andrzej
On Thu, Oct 30, 2014 at 11:01:02AM +0100, Andrzej Hajda wrote: > On 10/29/2014 10:14 AM, Thierry Reding wrote: > > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote: > >> I think we nee try_get_module for the code and kref on the actual data > >> structures. > > > > Agreed, that should do the trick. We'd probably need some sort of logic > > to also make operations return something like -ENODEV when the > > underlying device has disappeared. I think David had introduced > > something similar for DRM device not so long ago? > > If the underlying device disappears it would be good to receive > notification anyway to trigger DRM HPD event. And if we have the > notification, we can release references to the device smoothly. We do > not need to play tricky games with krefs, zombie data and module > refcounting. Any solution which thinks it needs to lock modules into the core is fundamentally broken. It totally misses the point. While you can lock a module into the core using try_get_module(), that doesn't stop the device itself being unbound from a driver. Soo many people have fallen into that trap. They write their device driver, along with some kind of framework which they make use try_get_module(). They think its safe. When you then echo the device name into the driver's unbind sysfs file, all hell breaks loose and the kernel oopses. try_get_module is /totally/ useless for ensuring that things stick around. The reality is that you can't make devices stick around. Once that remove callback from the driver layer is called, that's it, the device _is_ going away whether you like it or not. You can't stop it. It's no good returning -EBUSY, because the remove return code is ignored. What's more scarey is when you consider that in a real hotplug situation, when the remove callback is called, the device has /already/ gone. So please, stop thinking that try_get_module() has some magic solution. Any "solution" to device lifetimes using try_get_module() totally misses the problem, and is just mere obfuscation and actually a bug in itself.
On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote: > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote: > > That makes the entire thing a bit non-trivial, which is why I think it > > would be better as some generic helper. Which then gets embedded or > > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge. > > Or maybe even acpi&drm_bridge, who knows ;-) > > I worry a little about type safety. How will this generic helper know > what registry to look in for? Or conversely, if all these resources are > added to a single registry how do you know that they're of the correct > type? Failing to ensure this could cause situations where you're asking > for a panel and get a bridge in return because you've wrongly wired it > up in device tree for example. > > But perhaps if both the registry and the device parts are turned into > helpers we could still have a single core implementation and then > instantiate that for each type, something roughly like this: > > struct registry { > struct list_head list; > struct mutex lock; > }; > > struct registry_record { > struct list_head list; > struct module *owner; > struct kref *ref; > > struct device *dev; > }; > > int registry_add(struct registry *registry, struct registry_record *record) > { > ... > try_module_get(record->owner); > ... > } > > struct registry_record *registry_find_by_of_node(struct registry *registry, > struct device_node *np) > { > ... > kref_get(...); > ... > } > > That way it should be possible to embed these into other structures, > like so: > > struct drm_panel { > struct registry_record record; > ... > }; > > static struct registry drm_panels; > > int drm_panel_add(struct drm_panel *panel) > { > return registry_add(&drm_panels, &panel->record); > } > > struct drm_panel *of_drm_panel_find(struct device_node *np) > { > struct registry_record *record; > > record = registry_find_by_of_node(&drm_panels, np); > > return container_of(record, struct drm_panel, record); > } > > Is that what you had in mind? Yeah I've thought that we should instantiate using macros even, so that we have per-type registries. So you'd smash the usual set of DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take a (name, key, value) tripled. For the example here(of_drm_panel, struct device_node *, struct drm_panel *) or similar. I might be hand-waving over a few too many details though ;-) Cheers, Daniel
On Thu, Oct 30, 2014 at 10:09:28AM +0000, Russell King - ARM Linux wrote: > On Thu, Oct 30, 2014 at 11:01:02AM +0100, Andrzej Hajda wrote: > > On 10/29/2014 10:14 AM, Thierry Reding wrote: > > > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote: > > >> I think we nee try_get_module for the code and kref on the actual data > > >> structures. > > > > > > Agreed, that should do the trick. We'd probably need some sort of logic > > > to also make operations return something like -ENODEV when the > > > underlying device has disappeared. I think David had introduced > > > something similar for DRM device not so long ago? > > > > If the underlying device disappears it would be good to receive > > notification anyway to trigger DRM HPD event. And if we have the > > notification, we can release references to the device smoothly. We do > > not need to play tricky games with krefs, zombie data and module > > refcounting. > > Any solution which thinks it needs to lock modules into the core is > fundamentally broken. It totally misses the point. > > While you can lock a module into the core using try_get_module(), that > doesn't stop the device itself being unbound from a driver. Soo many > people have fallen into that trap. They write their device driver, > along with some kind of framework which they make use try_get_module(). > They think its safe. When you then echo the device name into the > driver's unbind sysfs file, all hell breaks loose and the kernel oopses. > > try_get_module is /totally/ useless for ensuring that things stick around. > > The reality is that you can't make devices stick around. Once that > remove callback from the driver layer is called, that's it, the device > _is_ going away whether you like it or not. You can't stop it. It's > no good returning -EBUSY, because the remove return code is ignored. > > What's more scarey is when you consider that in a real hotplug > situation, when the remove callback is called, the device has > /already/ gone. > > So please, stop thinking that try_get_module() has some magic solution. > Any "solution" to device lifetimes using try_get_module() totally misses > the problem, and is just mere obfuscation and actually a bug in itself. We need proper refcounting ofc, but we also need to make sure that as long as the thing is around the text section for the final unref (at least that) doesn't go poof. I'd prefer if the framework takes care of that detail and drivers could just supply a THIS_MODULE at the right place. But fully agree on your overall point that try_get_module alone is pure snake oil. -Daniel
On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote: > On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote: > > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote: > > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote: > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > > > > >>> * @driver_private: pointer to the bridge driver's internal context > > > > >>> */ > > > > >>> struct drm_bridge { > > > > >>> - struct drm_device *dev; > > > > >>> + struct device *dev; > > > > >> > > > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other > > > > >> drm structures still call it ->dev. Also, can't we use struct device_node > > > > >> here like we do in the of helpers Russell added? See 7e435aad38083 > > > > >> > > > > > > > > > > I think this is modeled after the naming in drm_panel, FWIW. However, > > > > > seems reasonable to keep the device_node instead. > > > > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > > > > drm_crtc drop the struct device and go directly to a struct > > > > device_node. Since we don't really need the sturct device, the only > > > > thing we care about is the of_node. For added bonus wrap an #ifdef > > > > CONFIG_OF around all the various struct device_node in drm_foo.h. > > > > Should be all fairly simple to pull off with cocci. > > > > > > > > Thierry? > > > > > > The struct device * is in DRM panel because there's nothing device tree > > > specific about the concept. Having a struct device_node * instead would > > > indicate that it can only be used with a device tree, whereas the > > > framework doesn't care the tiniest bit what type of device we have. > > > > > > While the trend clearly is to use more device tree, I don't think we > > > should make it impossible for anybody else to use these frameworks. > > > > > > There are other advantages to keeping a struct device *, like having > > > access to the proper device and its name. Also you get access to the > > > device_node * via dev->of_node anyway. I don't see any advantage in > > > switching to just a struct device_node *, only disadvantages. > > > > Well the idea is to make the lookup key specific, and conditional on > > #CONFIG_OF. If there's going to be another neat way to enumerate platform > > devices then I think we should add that, too. Or maybe we should have a > > void *platform_data or so. > > > > The reason I really don't want a struct device * in core drm structures is > > that two releases down the road people will have found tons of really > > great ways to abuse them and re-create a midlayer. DRM core really should > > only care about the sw objects and not be hw specific at all. Heck there's > > not even an requirement to have any piece of actual hw, you could write a > > completely fake drm driver (for e.g. testing like the new v4l driver). > > > > Tbh I wonder a bit why we even have this registery embedded into the core > > drm objects. Essentially the only thing you're doing is a list that maps > > some platform specific key onto some subsystem specific driver structure > > or fails the lookup. So instead of putting all these low-level details > > into drm core structures can't we just have a generic hashtable/list for > > this, plus some static inline helpers that cast the void * you get into > > the one you want? > > > > I also get the feeling that this really should be in the driver core (like > > the component helpers), and that we should think a lot harder about > > lifetimes and refcounting (see my other reply on that). > > Yes, that sounds very useful indeed. Also see my reply to yours. =) Just replying here with some of the irc discussions we've had. Since drm_bridge/panel isn't a core drm interface object exposed to userspace it's less critical. I still think that wasting a few brain cycles to have a clear separation between the abstract interface object and how to bind and unbind the pieces together is worthwhile, even though the cost when getting it wrong is much less severe than in the case of a mandatory piece of core infrastructure. I think in general the recent armsoc motivated drm infrastructure lacks a bit in attention to these details. E.g. the cma helpers are built into the drm.ko module, but clearly are auxilliary library code. So they should be pulled out and the headers clean up, so that we have a clear separation between core and helpers. Otherwise someone will sooner or later screw up and insert a helper depency into the core, and then we've started with the midlayer mess. Same goes with drm_bridge/panel, which didn't even bother to have separate headers from the core modeset header drm_crtc.h. So would be great if someone could invest a bit of time into cleaning this up. Writing proper api docs also helps a lot with achieving a clean and sensible split ;-) -Daniel
On Wed, Oct 29, 2014 at 10:09:04AM +0100, Andrzej Hajda wrote: > On 10/29/2014 08:58 AM, Daniel Vetter wrote: > > On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote: > >> On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote: > >>> On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding > >>> <thierry.reding@gmail.com> wrote: > >>>> On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote: > >>>>> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <ajaynumb@gmail.com> wrote: > >>>>>> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > >>>> [...] > >>>>>>> Hm, if you do this can you pls also update drm_panel accordingly? It > >>>>>>> shouldn't be a lot of fuzz and would make things around drm+dt more > >>>>>>> consistent. > >>>>>> Are you talking about using struct device_node instead of struct device? > >>>>>> I guess you have misplaced the comment under the wrong section! > >>>>> > >>>>> Yeah, that should have been one up ;-) > >>>> > >>>> Like I said earlier, I don't think dropping struct device * in favour of > >>>> struct device_node * is a good idea. > >>> I am not sure about drm_panel. > >>> But, I am not really doing anything with the struct device pointer in > >>> case of bridge. > >>> So, just wondering if it is really needed? > >> > >> I think it's useful to have it just to send the right message. DRM panel > >> and DRM bridge aren't specific to device tree. They are completely > >> generic and can work with any type of device, whether it was > >> instantiated from the device tree or some other infrastructure. Dropping > >> struct device * will make it effectively useless on anything but DT. I > >> don't think we should strive for that, even if only DT-enabled platforms > >> currently use them. > > > > See my other reply, but I now think we should put neither into drm > > structures. This "find me the driver structures for this device" problem > > looks really generic, and it has nothing to do with the drm structures and > > concepts like bridges/panels at all. It shouldn't be in there at all. > > > > Adding it looks very much like reintroducing the drm midlayer that we just > > finally made obsolete, just not at the top-level (struct drm_device) but > > at a bunch of leaf nodes. I expect all the same issues though. And I'm > > definitely not looking to de-midlayer more stuff that we're just adding. > > > > Imo this should be solved as a separate helper thing, maybe in the driver > > core akin to the component helpers from Russell. > > -Daniel > > > > As I understand you want something generic to look for panels, bridges, > whatever and, like components, it should allow to safe hot plug/unplug. > I have proposed such thing few months ago [1]. Have you looked at it? > > [1]: https://lkml.org/lkml/2014/4/30/345 Yeah I think I've looked but wasn't convinced. I do agree though that we should definitely aim for something in the driver core. Since if Greg doesn't like it it's not suddenly better if we just hide it in the drm subsystem. This really smells like a generic issue - after all we already have a two implementations with bridges&panels already. So maybe we need to augment the component framework with the possibility to add additional devices later on at runtime, or something similar. Not really sure since I don't have actual practice with these issues since i915 doesn't (yet) have these problems. -Daniel
On Fri, Oct 31, 2014 at 04:51:43PM +0100, Daniel Vetter wrote: > On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote: > > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote: > > > That makes the entire thing a bit non-trivial, which is why I think it > > > would be better as some generic helper. Which then gets embedded or > > > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge. > > > Or maybe even acpi&drm_bridge, who knows ;-) > > > > I worry a little about type safety. How will this generic helper know > > what registry to look in for? Or conversely, if all these resources are > > added to a single registry how do you know that they're of the correct > > type? Failing to ensure this could cause situations where you're asking > > for a panel and get a bridge in return because you've wrongly wired it > > up in device tree for example. > > > > But perhaps if both the registry and the device parts are turned into > > helpers we could still have a single core implementation and then > > instantiate that for each type, something roughly like this: > > > > struct registry { > > struct list_head list; > > struct mutex lock; > > }; > > > > struct registry_record { > > struct list_head list; > > struct module *owner; > > struct kref *ref; > > > > struct device *dev; > > }; > > > > int registry_add(struct registry *registry, struct registry_record *record) > > { > > ... > > try_module_get(record->owner); > > ... > > } > > > > struct registry_record *registry_find_by_of_node(struct registry *registry, > > struct device_node *np) > > { > > ... > > kref_get(...); > > ... > > } > > > > That way it should be possible to embed these into other structures, > > like so: > > > > struct drm_panel { > > struct registry_record record; > > ... > > }; > > > > static struct registry drm_panels; > > > > int drm_panel_add(struct drm_panel *panel) > > { > > return registry_add(&drm_panels, &panel->record); > > } > > > > struct drm_panel *of_drm_panel_find(struct device_node *np) > > { > > struct registry_record *record; > > > > record = registry_find_by_of_node(&drm_panels, np); > > > > return container_of(record, struct drm_panel, record); > > } > > > > Is that what you had in mind? > > Yeah I've thought that we should instantiate using macros even, so that we > have per-type registries. So you'd smash the usual set of > DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take > a (name, key, value) tripled. For the example here(of_drm_panel, struct > device_node *, struct drm_panel *) or similar. I might be hand-waving over > a few too many details though ;-) Okay, I'll take a stab at this and see if I can convert DRM panel to it. Thierry
On Fri, Oct 31, 2014 at 04:59:40PM +0100, Daniel Vetter wrote: > On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote: > > On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote: > > > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote: > > > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote: > > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > > > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > > > > > >>> * @driver_private: pointer to the bridge driver's internal context > > > > > >>> */ > > > > > >>> struct drm_bridge { > > > > > >>> - struct drm_device *dev; > > > > > >>> + struct device *dev; > > > > > >> > > > > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other > > > > > >> drm structures still call it ->dev. Also, can't we use struct device_node > > > > > >> here like we do in the of helpers Russell added? See 7e435aad38083 > > > > > >> > > > > > > > > > > > > I think this is modeled after the naming in drm_panel, FWIW. However, > > > > > > seems reasonable to keep the device_node instead. > > > > > > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > > > > > drm_crtc drop the struct device and go directly to a struct > > > > > device_node. Since we don't really need the sturct device, the only > > > > > thing we care about is the of_node. For added bonus wrap an #ifdef > > > > > CONFIG_OF around all the various struct device_node in drm_foo.h. > > > > > Should be all fairly simple to pull off with cocci. > > > > > > > > > > Thierry? > > > > > > > > The struct device * is in DRM panel because there's nothing device tree > > > > specific about the concept. Having a struct device_node * instead would > > > > indicate that it can only be used with a device tree, whereas the > > > > framework doesn't care the tiniest bit what type of device we have. > > > > > > > > While the trend clearly is to use more device tree, I don't think we > > > > should make it impossible for anybody else to use these frameworks. > > > > > > > > There are other advantages to keeping a struct device *, like having > > > > access to the proper device and its name. Also you get access to the > > > > device_node * via dev->of_node anyway. I don't see any advantage in > > > > switching to just a struct device_node *, only disadvantages. > > > > > > Well the idea is to make the lookup key specific, and conditional on > > > #CONFIG_OF. If there's going to be another neat way to enumerate platform > > > devices then I think we should add that, too. Or maybe we should have a > > > void *platform_data or so. > > > > > > The reason I really don't want a struct device * in core drm structures is > > > that two releases down the road people will have found tons of really > > > great ways to abuse them and re-create a midlayer. DRM core really should > > > only care about the sw objects and not be hw specific at all. Heck there's > > > not even an requirement to have any piece of actual hw, you could write a > > > completely fake drm driver (for e.g. testing like the new v4l driver). > > > > > > Tbh I wonder a bit why we even have this registery embedded into the core > > > drm objects. Essentially the only thing you're doing is a list that maps > > > some platform specific key onto some subsystem specific driver structure > > > or fails the lookup. So instead of putting all these low-level details > > > into drm core structures can't we just have a generic hashtable/list for > > > this, plus some static inline helpers that cast the void * you get into > > > the one you want? > > > > > > I also get the feeling that this really should be in the driver core (like > > > the component helpers), and that we should think a lot harder about > > > lifetimes and refcounting (see my other reply on that). > > > > Yes, that sounds very useful indeed. Also see my reply to yours. =) > > Just replying here with some of the irc discussions we've had. Since > drm_bridge/panel isn't a core drm interface object exposed to userspace > it's less critical. I still think that wasting a few brain cycles to have > a clear separation between the abstract interface object and how to bind > and unbind the pieces together is worthwhile, even though the cost when > getting it wrong is much less severe than in the case of a mandatory piece > of core infrastructure. > > I think in general the recent armsoc motivated drm infrastructure lacks a > bit in attention to these details. E.g. the cma helpers are built into the > drm.ko module, but clearly are auxilliary library code. So they should be > pulled out and the headers clean up, so that we have a clear separation > between core and helpers. Otherwise someone will sooner or later screw up > and insert a helper depency into the core, and then we've started with the > midlayer mess. Same goes with drm_bridge/panel, which didn't even bother > to have separate headers from the core modeset header drm_crtc.h. DRM panel does have a separate header. It's still built into the core DRM module, but using a separate drm-$(CONFIG_DRM_PANEL) += drm_panel.o entry in the makefile. At the time it didn't seem worth to add a completely separate module given the size of the code and the overhead associated with having a separate module. Do you still want me to split it off into a separate module to clarify that it isn't part of the core? > So would be great if someone could invest a bit of time into cleaning this > up. Writing proper api docs also helps a lot with achieving a clean and > sensible split ;-) There's a bit of API documentation for panels, but I'll see if I can find some time to enhance it. Thierry
On Mon, Nov 03, 2014 at 09:06:04AM +0100, Thierry Reding wrote: > On Fri, Oct 31, 2014 at 04:59:40PM +0100, Daniel Vetter wrote: > > On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote: > > > On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote: > > > > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote: > > > > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote: > > > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote: > > > > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > > > > > > >>> * @driver_private: pointer to the bridge driver's internal context > > > > > > >>> */ > > > > > > >>> struct drm_bridge { > > > > > > >>> - struct drm_device *dev; > > > > > > >>> + struct device *dev; > > > > > > >> > > > > > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other > > > > > > >> drm structures still call it ->dev. Also, can't we use struct device_node > > > > > > >> here like we do in the of helpers Russell added? See 7e435aad38083 > > > > > > >> > > > > > > > > > > > > > > I think this is modeled after the naming in drm_panel, FWIW. However, > > > > > > > seems reasonable to keep the device_node instead. > > > > > > > > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with > > > > > > drm_crtc drop the struct device and go directly to a struct > > > > > > device_node. Since we don't really need the sturct device, the only > > > > > > thing we care about is the of_node. For added bonus wrap an #ifdef > > > > > > CONFIG_OF around all the various struct device_node in drm_foo.h. > > > > > > Should be all fairly simple to pull off with cocci. > > > > > > > > > > > > Thierry? > > > > > > > > > > The struct device * is in DRM panel because there's nothing device tree > > > > > specific about the concept. Having a struct device_node * instead would > > > > > indicate that it can only be used with a device tree, whereas the > > > > > framework doesn't care the tiniest bit what type of device we have. > > > > > > > > > > While the trend clearly is to use more device tree, I don't think we > > > > > should make it impossible for anybody else to use these frameworks. > > > > > > > > > > There are other advantages to keeping a struct device *, like having > > > > > access to the proper device and its name. Also you get access to the > > > > > device_node * via dev->of_node anyway. I don't see any advantage in > > > > > switching to just a struct device_node *, only disadvantages. > > > > > > > > Well the idea is to make the lookup key specific, and conditional on > > > > #CONFIG_OF. If there's going to be another neat way to enumerate platform > > > > devices then I think we should add that, too. Or maybe we should have a > > > > void *platform_data or so. > > > > > > > > The reason I really don't want a struct device * in core drm structures is > > > > that two releases down the road people will have found tons of really > > > > great ways to abuse them and re-create a midlayer. DRM core really should > > > > only care about the sw objects and not be hw specific at all. Heck there's > > > > not even an requirement to have any piece of actual hw, you could write a > > > > completely fake drm driver (for e.g. testing like the new v4l driver). > > > > > > > > Tbh I wonder a bit why we even have this registery embedded into the core > > > > drm objects. Essentially the only thing you're doing is a list that maps > > > > some platform specific key onto some subsystem specific driver structure > > > > or fails the lookup. So instead of putting all these low-level details > > > > into drm core structures can't we just have a generic hashtable/list for > > > > this, plus some static inline helpers that cast the void * you get into > > > > the one you want? > > > > > > > > I also get the feeling that this really should be in the driver core (like > > > > the component helpers), and that we should think a lot harder about > > > > lifetimes and refcounting (see my other reply on that). > > > > > > Yes, that sounds very useful indeed. Also see my reply to yours. =) > > > > Just replying here with some of the irc discussions we've had. Since > > drm_bridge/panel isn't a core drm interface object exposed to userspace > > it's less critical. I still think that wasting a few brain cycles to have > > a clear separation between the abstract interface object and how to bind > > and unbind the pieces together is worthwhile, even though the cost when > > getting it wrong is much less severe than in the case of a mandatory piece > > of core infrastructure. > > > > I think in general the recent armsoc motivated drm infrastructure lacks a > > bit in attention to these details. E.g. the cma helpers are built into the > > drm.ko module, but clearly are auxilliary library code. So they should be > > pulled out and the headers clean up, so that we have a clear separation > > between core and helpers. Otherwise someone will sooner or later screw up > > and insert a helper depency into the core, and then we've started with the > > midlayer mess. Same goes with drm_bridge/panel, which didn't even bother > > to have separate headers from the core modeset header drm_crtc.h. > > DRM panel does have a separate header. It's still built into the core > DRM module, but using a separate drm-$(CONFIG_DRM_PANEL) += drm_panel.o > entry in the makefile. At the time it didn't seem worth to add a > completely separate module given the size of the code and the overhead > associated with having a separate module. > > Do you still want me to split it off into a separate module to clarify > that it isn't part of the core? Oh, it doesn't need to be a complete standalone module, smashing it into drm_kms_helper is imo totally ok. It's just to make really sure that helpers are helpers and there's never a depency from drm.ko to any optional helper code. Having a separate module for all the helper code helps a lot in ensure that. Also, everything a helper can do, a driver should be able to do too. Again a separate helper ensures that you haven't missed any EXPORT_SYMBOL. Which should then be a good reminder to update the kerneldoc ;-) > > So would be great if someone could invest a bit of time into cleaning this > > up. Writing proper api docs also helps a lot with achieving a clean and > > sensible split ;-) > > There's a bit of API documentation for panels, but I'll see if I can > find some time to enhance it. Imo pulling into the DocBook template is also important, since if you do that the 0-day tester will complain if the kerneldoc gets out of sync. Which does increases the changes of it staying up-to-date a lot. Cheers, Daniel
On Mon, Nov 3, 2014 at 1:31 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Oct 31, 2014 at 04:51:43PM +0100, Daniel Vetter wrote: >> On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote: >> > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote: >> > > That makes the entire thing a bit non-trivial, which is why I think it >> > > would be better as some generic helper. Which then gets embedded or >> > > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge. >> > > Or maybe even acpi&drm_bridge, who knows ;-) >> > >> > I worry a little about type safety. How will this generic helper know >> > what registry to look in for? Or conversely, if all these resources are >> > added to a single registry how do you know that they're of the correct >> > type? Failing to ensure this could cause situations where you're asking >> > for a panel and get a bridge in return because you've wrongly wired it >> > up in device tree for example. >> > >> > But perhaps if both the registry and the device parts are turned into >> > helpers we could still have a single core implementation and then >> > instantiate that for each type, something roughly like this: >> > >> > struct registry { >> > struct list_head list; >> > struct mutex lock; >> > }; >> > >> > struct registry_record { >> > struct list_head list; >> > struct module *owner; >> > struct kref *ref; >> > >> > struct device *dev; >> > }; >> > >> > int registry_add(struct registry *registry, struct registry_record *record) >> > { >> > ... >> > try_module_get(record->owner); >> > ... >> > } >> > >> > struct registry_record *registry_find_by_of_node(struct registry *registry, >> > struct device_node *np) >> > { >> > ... >> > kref_get(...); >> > ... >> > } >> > >> > That way it should be possible to embed these into other structures, >> > like so: >> > >> > struct drm_panel { >> > struct registry_record record; >> > ... >> > }; >> > >> > static struct registry drm_panels; >> > >> > int drm_panel_add(struct drm_panel *panel) >> > { >> > return registry_add(&drm_panels, &panel->record); >> > } >> > >> > struct drm_panel *of_drm_panel_find(struct device_node *np) >> > { >> > struct registry_record *record; >> > >> > record = registry_find_by_of_node(&drm_panels, np); >> > >> > return container_of(record, struct drm_panel, record); >> > } >> > >> > Is that what you had in mind? >> >> Yeah I've thought that we should instantiate using macros even, so that we >> have per-type registries. So you'd smash the usual set of >> DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take >> a (name, key, value) tripled. For the example here(of_drm_panel, struct >> device_node *, struct drm_panel *) or similar. I might be hand-waving over >> a few too many details though ;-) > > Okay, I'll take a stab at this and see if I can convert DRM panel to it. It would be great if you can do this soon. I would anyhow need a reference for converting bridge framework as per Daniel's requirement :) Ajay
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 4a55d59..bdbfb6f 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \ drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o drm-$(CONFIG_PCI) += ati_pcigart.o +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o drm-$(CONFIG_DRM_PANEL) += drm_panel.o drm-$(CONFIG_OF) += drm_of.o diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 884923f..5a8e907 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -1,5 +1,16 @@ -config DRM_PTN3460 - tristate "PTN3460 DP/LVDS bridge" +config DRM_BRIDGE + tristate depends on DRM select DRM_KMS_HELPER + help + Bridge registration and lookup framework. + +menu "bridge chips" + depends on DRM_BRIDGE + +config DRM_PTN3460 + tristate "PTN3460 DP/LVDS bridge" + depends on DRM_BRIDGE ---help--- + +endmenu diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c new file mode 100644 index 0000000..b2d43fd --- /dev/null +++ b/drivers/gpu/drm/drm_bridge.c @@ -0,0 +1,102 @@ +/* + * Copyright (c) 2014 Samsung Electronics Co., Ltd + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sub license, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the + * next paragraph) shall be included in all copies or substantial portions + * of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + */ + +#include <linux/err.h> +#include <linux/module.h> + +#include <drm/drm_crtc.h> + +#include "drm/drmP.h" + +static DEFINE_MUTEX(bridge_lock); +static LIST_HEAD(bridge_list); + +int drm_bridge_add(struct drm_bridge *bridge) +{ + mutex_lock(&bridge_lock); + list_add_tail(&bridge->list, &bridge_list); + mutex_unlock(&bridge_lock); + + return 0; +} +EXPORT_SYMBOL(drm_bridge_add); + +void drm_bridge_remove(struct drm_bridge *bridge) +{ + mutex_lock(&bridge_lock); + list_del_init(&bridge->list); + mutex_unlock(&bridge_lock); +} +EXPORT_SYMBOL(drm_bridge_remove); + +int drm_bridge_attach(struct drm_bridge *bridge, + struct drm_encoder *encoder) +{ + int ret; + + if (!bridge || !encoder) + return -EINVAL; + + if (bridge->encoder) + return -EBUSY; + + encoder->bridge = bridge; + bridge->encoder = encoder; + bridge->drm = encoder->dev; + + ret = drm_bridge_init(bridge->drm, bridge); + if (ret) { + DRM_ERROR("Failed to register bridge with drm\n"); + return ret; + } + + if (bridge->funcs->attach) + return bridge->funcs->attach(bridge); + + return 0; +} +EXPORT_SYMBOL(drm_bridge_attach); + +#ifdef CONFIG_OF +struct drm_bridge *of_drm_find_bridge(struct device_node *np) +{ + struct drm_bridge *bridge; + + mutex_lock(&bridge_lock); + + list_for_each_entry(bridge, &bridge_list, list) { + if (bridge->dev->of_node == np) { + mutex_unlock(&bridge_lock); + return bridge; + } + } + + mutex_unlock(&bridge_lock); + return NULL; +} +EXPORT_SYMBOL(of_drm_find_bridge); +#endif + +MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>"); +MODULE_DESCRIPTION("DRM bridge infrastructure"); +MODULE_LICENSE("GPL and additional rights"); diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 25f5cfa..2fb22fa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1039,7 +1039,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge) if (ret) goto out; - bridge->dev = dev; + bridge->drm = dev; list_add_tail(&bridge->head, &dev->mode_config.bridge_list); dev->mode_config.num_bridge++; @@ -1058,7 +1058,7 @@ EXPORT_SYMBOL(drm_bridge_init); */ void drm_bridge_cleanup(struct drm_bridge *bridge) { - struct drm_device *dev = bridge->dev; + struct drm_device *dev = bridge->drm; drm_modeset_lock_all(dev); drm_mode_object_put(dev, &bridge->base); diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c index 0309539..bc9e5ff 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c @@ -33,7 +33,7 @@ static void hdmi_bridge_destroy(struct drm_bridge *bridge) static void power_on(struct drm_bridge *bridge) { - struct drm_device *dev = bridge->dev; + struct drm_device *dev = bridge->drm; struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); struct hdmi *hdmi = hdmi_bridge->hdmi; const struct hdmi_platform_config *config = hdmi->config; @@ -67,7 +67,7 @@ static void power_on(struct drm_bridge *bridge) static void power_off(struct drm_bridge *bridge) { - struct drm_device *dev = bridge->dev; + struct drm_device *dev = bridge->drm; struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); struct hdmi *hdmi = hdmi_bridge->hdmi; const struct hdmi_platform_config *config = hdmi->config; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index f48a436..f6f426f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -629,6 +629,7 @@ struct drm_plane { /** * drm_bridge_funcs - drm_bridge control functions + * @attach: Called during drm_bridge_attach * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge * @disable: Called right before encoder prepare, disables the bridge * @post_disable: Called right after encoder prepare, for lockstepped disable @@ -638,6 +639,7 @@ struct drm_plane { * @destroy: make object go away */ struct drm_bridge_funcs { + int (*attach)(struct drm_bridge *bridge); bool (*mode_fixup)(struct drm_bridge *bridge, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); @@ -660,8 +662,11 @@ struct drm_bridge_funcs { * @driver_private: pointer to the bridge driver's internal context */ struct drm_bridge { - struct drm_device *dev; + struct device *dev; + struct drm_device *drm; + struct drm_encoder *encoder; struct list_head head; + struct list_head list; struct drm_mode_object base; @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector); /* helper to unplug all connectors from sysfs for device */ extern void drm_connector_unplug_all(struct drm_device *dev); +extern int drm_bridge_add(struct drm_bridge *bridge); +extern void drm_bridge_remove(struct drm_bridge *bridge); +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); +extern int drm_bridge_attach(struct drm_bridge *bridge, + struct drm_encoder *encoder); extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge); extern void drm_bridge_cleanup(struct drm_bridge *bridge);
A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling "drm_bridge_add". The parent encoder driver waits till the bridge is available in the lookup table(by calling "of_drm_find_bridge") and then continues with its initialization. The encoder driver should also call "drm_bridge_attach" to pass on the drm_device, encoder pointers to the bridge object. drm_bridge_attach inturn calls drm_bridge_init to register itself with the drm core. Later, it calls "bridge->funcs->attach" so that bridge can continue with other initializations. Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> --- drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/bridge/Kconfig | 15 ++++- drivers/gpu/drm/drm_bridge.c | 102 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_crtc.c | 4 +- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 4 +- include/drm/drm_crtc.h | 12 +++- 6 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 drivers/gpu/drm/drm_bridge.c