On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote:
> > On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
> > > On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
> > > > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> > > > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> > > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
> > > > > >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > > >>>> * @driver_private: pointer to the bridge driver's internal context
> > > > > >>>> */
> > > > > >>>> struct drm_bridge {
> > > > > >>>> - struct drm_device *dev;
> > > > > >>>> + struct device *dev;
> > > > > >>>
> > > > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > > > > >>> drm structures still call it ->dev. Also, can't we use struct device_node
> > > > > >>> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > > >>>
> > > > > >>
> > > > > >> I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > > >> seems reasonable to keep the device_node instead.
> > > > > >
> > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > > > drm_crtc drop the struct device and go directly to a struct
> > > > > > device_node. Since we don't really need the sturct device, the only
> > > > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > > > Should be all fairly simple to pull off with cocci.
> > > > > >
> > > > > > Thierry?
> > > > >
> > > > > Looking at the of_drm_find_panel function I actually wonder how that
> > > > > works - the drm_panel doesn't really need to stick around afaics.
> > > > > After all panel_list is global so some other driver can unload.
> > > > > Russell's of support for possible_crtcs code works differently since
> > > > > it only looks at per-drm_device lists.
> > > >
> > > > I don't understand. Panels are global resources that get registered by
> > > > some driver that isn't tied to the DRM device until attached. It can't
> > > > be in a per-DRM device list, because it's external to the device.
> > > >
> > > > And yes, they can go away when a driver is unloaded, though it's not the
> > > > typical use-case.
> > > >
> > > > > This bridge code here though suffers from the same. So to me this
> > > > > looks rather fishy.
> > > >
> > > > Well, this version of the DRM bridge support was written to be close to
> > > > DRM panel, so it isn't really surprising that it's similar =), but like
> > > > I said, I don't really understand what you think is wrong with it.
> > >
> > > You have a mutex to protect the global list of bridges/panels. But if you
> > > do a lookup you don't grab a reference count on the panel, so the moment
> > > you drop the list mutex the panel/bridge can go away.
> > >
> > > Yes usually you don't unload a driver on a soc but soc isn't everything
> > > and designing new stuff to not be hotunplug save isn't great either.
> >
> > Yeah, I certainly agree that adding proper reference counting would be a
> > good thing. I think perhaps we could just take a reference on the struct
> > device * to prevent it from disappearing.
> >
> > Although perhaps I misunderstand what you mean by "go away".
>
> I meant the drm_panel/bridge could go away any second, since nothing
> prevents a module unload of the panel/bridge driver. So in theory you
> could get the unregister call right after you've done the lookup. Which
> means the bridge/panel pointer you've just returned is freed memory.
Ah yes, I see now.
> I think we nee try_get_module for the code and kref on the actual data
> structures.
Agreed, that should do the trick. We'd probably need some sort of logic
to also make operations return something like -ENODEV when the
underlying device has disappeared. I think David had introduced
something similar for DRM device not so long ago?
> That makes the entire thing a bit non-trivial, which is why I think it
> would be better as some generic helper. Which then gets embedded or
> instantiated for specific cases, like dt&drm_panel or dt&drm_bridge.
> Or maybe even acpi&drm_bridge, who knows ;-)
I worry a little about type safety. How will this generic helper know
what registry to look in for? Or conversely, if all these resources are
added to a single registry how do you know that they're of the correct
type? Failing to ensure this could cause situations where you're asking
for a panel and get a bridge in return because you've wrongly wired it
up in device tree for example.
But perhaps if both the registry and the device parts are turned into
helpers we could still have a single core implementation and then
instantiate that for each type, something roughly like this:
struct registry {
struct list_head list;
struct mutex lock;
};
struct registry_record {
struct list_head list;
struct module *owner;
struct kref *ref;
struct device *dev;
};
int registry_add(struct registry *registry, struct registry_record *record)
{
...
try_module_get(record->owner);
...
}
struct registry_record *registry_find_by_of_node(struct registry *registry,
struct device_node *np)
{
...
kref_get(...);
...
}
That way it should be possible to embed these into other structures,
like so:
struct drm_panel {
struct registry_record record;
...
};
static struct registry drm_panels;
int drm_panel_add(struct drm_panel *panel)
{
return registry_add(&drm_panels, &panel->record);
}
struct drm_panel *of_drm_panel_find(struct device_node *np)
{
struct registry_record *record;
record = registry_find_by_of_node(&drm_panels, np);
return container_of(record, struct drm_panel, record);
}
Is that what you had in mind?
Thierry
Attachment:
pgp9Yl9BM9njs.pgp
Description: PGP signature
