Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
Hi Russell, Am Montag, den 27.10.2014, 23:57 + schrieb Russell King - ARM Linux: On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote: 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. I rather dropped the ball with imx-drm when things became difficult with asking Greg to pull my git tree - and as a result, I decided that I would no longer be helping with patch submission for imx-drm, nor trying to get it out of the staging tree anymore. In that case I'd like to take up the ball. In my opinion the driver is now in a state in which it could be moved to gpu/drm first and then fixed up there (all the blockers in drivers/staging/imx-drm/TODO are gone). That would also give us a single path for all imx-drm/ipu-v3 related patches again. My main motivation to put it into staging in the first place was that the device tree bindings had to be worked out, but now it only results in a coordination nightmare. regards Philipp -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 dtdrm_panel or dtdrm_bridge.
Or maybe even acpidrm_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
pgpUw0Hut_eXQ.pgp
Description: PGP signature
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 seanp...@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
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 seanp...@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
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Mon, Nov 3, 2014 at 1:31 PM, Thierry Reding thierry.red...@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 dtdrm_panel or dtdrm_bridge.
Or maybe even acpidrm_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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 dtdrm_panel or dtdrm_bridge.
Or maybe even acpidrm_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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Thu, Oct 30, 2014 at 10:09:28AM +, 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 seanp...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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.red...@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 ajayn...@gmail.com wrote: On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@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 bridgespanels 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 dan...@ffwll.ch wrote:
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 dan...@ffwll.ch wrote:
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 seanp...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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.red...@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 ajayn...@gmail.com wrote: On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 dan...@ffwll.ch wrote:
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@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
pgpIpdWtkOLY6.pgp
Description: PGP signature
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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.red...@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 ajayn...@gmail.com wrote: On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 dan...@ffwll.ch
wrote:
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@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 dtdrm_panel or dtdrm_bridge.
Or maybe even acpidrm_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;
};
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 seanp...@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
pgpi3cwpmOupr.pgp
Description: PGP signature
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
Hi Daniel and Sean,
Thanks for the comments!
On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul seanp...@chromium.org wrote:
On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter dan...@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 quickgrumpy 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...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Tue, Oct 28, 2014 at 1:41 AM, Sean Paul seanp...@chromium.org wrote:
On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar ajaykumar...@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...@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 000..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;
+
+
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Tue, Oct 28, 2014 at 1:41 AM, Sean Paul seanp...@chromium.org wrote: On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar ajaykumar...@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...@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] -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar ajayn...@gmail.com wrote:
Hi Daniel and Sean,
Thanks for the comments!
On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul seanp...@chromium.org wrote:
On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter dan...@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 quickgrumpy 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...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar ajayn...@gmail.com wrote:
Hi Daniel and Sean,
Thanks for the comments!
On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul seanp...@chromium.org wrote:
On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter dan...@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 quickgrumpy 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...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar ajayn...@gmail.com wrote:
On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar ajayn...@gmail.com wrote:
Hi Daniel and Sean,
Thanks for the comments!
On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul seanp...@chromium.org wrote:
On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter dan...@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 quickgrumpy 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...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Tue, Oct 28, 2014 at 10:19 AM, Daniel Vetter dan...@ffwll.ch wrote:
On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar ajayn...@gmail.com wrote:
On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar ajayn...@gmail.com wrote:
Hi Daniel and Sean,
Thanks for the comments!
On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul seanp...@chromium.org wrote:
On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter dan...@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 quickgrumpy 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...@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
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@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
pgpSvaKUtJAL7.pgp
Description: PGP signature
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@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
pgpwbadwzK4yQ.pgp
Description: PGP signature
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote: On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar ajayn...@gmail.com wrote: On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@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 pgpJWY8ji2fm8.pgp Description: PGP signature
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding thierry.red...@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 ajayn...@gmail.com wrote: On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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.red...@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 ajayn...@gmail.com wrote: On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@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 pgpWDDu8jRmCM.pgp Description: PGP signature
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
So don't ask why but I accidentally ended up in a branch looking at this
patch and didn't like it. So very quickgrumpy 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...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter dan...@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 quickgrumpy 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...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar ajaykumar...@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...@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 000..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 =
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
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.
I rather dropped the ball with imx-drm when things became difficult with
asking Greg to pull my git tree - and as a result, I decided that I would
no longer be helping with patch submission for imx-drm, nor trying to get
it out of the staging tree anymore.
I do still have the patch (dated from July) in my git tree which adds it
to imx-drm - see below. Rebased to 3.18-rc2 today.
I also have a patch (from April) which creates a generic OF DDC connector
helper, but that remains unfinished, in the sense that it lives beside
imx-drm, pulling the DDC specific code out of imx-hdmi and imx-tve, even
though there's nothing imx-drm specific about it.
8
From: Russell King rmk+ker...@arm.linux.org.uk
Subject: [PATCH] imx-drm: convert imx-drm to use the generic DRM OF helper
Use the generic DRM OF helper to locate the possible CRTCs for the
encoder, thereby shrinking the imx-drm driver some more.
Acked-by: Philipp Zabel p.za...@pengutronix.de
Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
drivers/staging/imx-drm/imx-drm-core.c | 72 ++
1 file changed, 13 insertions(+), 59 deletions(-)
diff --git a/drivers/staging/imx-drm/imx-drm-core.c
b/drivers/staging/imx-drm/imx-drm-core.c
index 9cb222e2996f..5e2c1f4b9165 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -24,6 +24,7 @@
#include drm/drm_crtc_helper.h
#include drm/drm_gem_cma_helper.h
#include drm/drm_fb_cma_helper.h
+#include drm/drm_of.h
#include imx-drm.h
@@ -47,7 +48,6 @@ struct imx_drm_crtc {
struct drm_crtc *crtc;
int pipe;
struct imx_drm_crtc_helper_funcsimx_drm_helper_funcs;
- struct device_node *port;
};
static int legacyfb_depth = 16;
@@ -366,9 +366,10 @@ int imx_drm_add_crtc(struct drm_device *drm, struct
drm_crtc *crtc,
imx_drm_crtc-imx_drm_helper_funcs = *imx_drm_helper_funcs;
imx_drm_crtc-pipe = imxdrm-pipes++;
- imx_drm_crtc-port = port;
imx_drm_crtc-crtc = crtc;
+ crtc-port = port;
+
imxdrm-crtc[imx_drm_crtc-pipe] = imx_drm_crtc;
*new_crtc = imx_drm_crtc;
@@ -409,34 +410,6 @@ int imx_drm_remove_crtc(struct imx_drm_crtc *imx_drm_crtc)
}
EXPORT_SYMBOL_GPL(imx_drm_remove_crtc);
-/*
- * Find the DRM CRTC possible mask for the connected endpoint.
- *
- * The encoder possible masks are defined by their position in the
- * mode_config crtc_list. This means that CRTCs must not be added
- * or removed once the DRM device has been fully initialised.
- */
-static uint32_t imx_drm_find_crtc_mask(struct imx_drm_device *imxdrm,
- struct device_node *endpoint)
-{
- struct device_node *port;
- unsigned i;
-
- port = of_graph_get_remote_port(endpoint);
- if (!port)
- return 0;
- of_node_put(port);
-
- for (i = 0; i MAX_CRTC; i++) {
- struct imx_drm_crtc *imx_drm_crtc = imxdrm-crtc[i];
-
- if (imx_drm_crtc imx_drm_crtc-port == port)
- return drm_crtc_mask(imx_drm_crtc-crtc);
- }
-
- return 0;
-}
-
static struct device_node *imx_drm_of_get_next_endpoint(
const struct device_node *parent, struct device_node *prev)
{
@@ -449,35 +422,16 @@ static struct device_node *imx_drm_of_get_next_endpoint(
int imx_drm_encoder_parse_of(struct drm_device *drm,
struct drm_encoder *encoder, struct device_node *np)
{
- struct imx_drm_device *imxdrm = drm-dev_private;
- struct device_node *ep = NULL;
- uint32_t crtc_mask = 0;
- int i;
+ uint32_t crtc_mask = drm_of_find_possible_crtcs(drm, np);
- for (i = 0; ; i++) {
- u32 mask;
-
- ep = imx_drm_of_get_next_endpoint(np, ep);
- if (!ep)
- break;
-
- mask = imx_drm_find_crtc_mask(imxdrm, ep);
-
- /*
-* If we failed to find the CRTC(s) which this encoder is
-* supposed to be connected to, it's because the CRTC has
-* not been registered yet. Defer probing, and hope that
-* the required CRTC is added later.
-*/
- if (mask == 0)
- return -EPROBE_DEFER;
-
- crtc_mask |= mask;
-
[PATCH V7 03/12] drm/bridge: Add helper functions for drm_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...@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
+ 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 000..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
