| Message ID | 1464597519-16659-3-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 30, 2016 at 09:38:20AM +0100, Chris Wilson wrote: > If a driver wants to more precisely control its initialisation and in > particular, defer registering its interfaces with userspace until after > everything is setup, it also needs to defer registering the connectors. > As some devices need more work during registration, add a callback so > that drivers can do additional work if required for a connector. > > Correspondingly, we also require an unregister callback. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: dri-devel@lists.freedesktop.org tbh I'd call these hooks simply register/unregister. There shouldn't be any need for ordering every with interface registration/unregistartion, assuming drivers don't fumble things. Besides naming&kerneldoc (see below) lgtm as an idea. -Daniel > --- > drivers/gpu/drm/drm_crtc.c | 18 ++++++++++++++++-- > include/drm/drm_crtc.h | 25 +++++++++++++++++++++++++ > 2 files changed, 41 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index d2a6d958ca76..81641544ac3e 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -1036,13 +1036,24 @@ int drm_connector_register(struct drm_connector *connector) > > ret = drm_debugfs_connector_add(connector); > if (ret) { > - drm_sysfs_connector_remove(connector); > - return ret; > + goto err_sysfs; > + } > + > + if (connector->funcs->late_register) { > + ret = connector->funcs->late_register(connector); > + if (ret) > + goto err_debugfs; > } > > drm_mode_object_register(connector->dev, &connector->base); > > return 0; > + > +err_debugfs: > + drm_debugfs_connector_remove(connector); > +err_sysfs: > + drm_sysfs_connector_remove(connector); > + return ret; > } > EXPORT_SYMBOL(drm_connector_register); > > @@ -1054,6 +1065,9 @@ EXPORT_SYMBOL(drm_connector_register); > */ > void drm_connector_unregister(struct drm_connector *connector) > { > + if (connector->funcs->early_unregister) > + connector->funcs->early_unregister(connector); > + > drm_sysfs_connector_remove(connector); > drm_debugfs_connector_remove(connector); > } > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index d1559cd04e3d..77b775cff4e7 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -926,6 +926,31 @@ struct drm_connector_funcs { > uint64_t val); > > /** > + * @late_register: > + * > + * Register the connector with userspace, called from > + * drm_connector_register. This should be called after driver > + * load during its registration phase. All actions such as registering > + * with auxiliary devices (such as drm_dp_aux_register) should be done > + * during this callback. A simple guideline is that everything added > + * from this callback should be removed during early_unregister (and > + * vice versa). Slight bikeshedding of the kerneldoc: "This optional hook can be used to register additional userspace interfaces attached to the connector, like backlight control, i2c, dp aux or similar interfaces. It is called from drm_connector_register() when registering all the core drm connector interfaces. Everything added from this callback should be unregistered in the unregister() callback. "Returns: "0 on success or a negative failure code on error." Similar bikeshed with unregister, i.e. state that it is optional, and that is just to unregister additional interfaces. > + * > + */ > + int (*late_register)(struct drm_connector *connector); > + > + /** > + * @early_unregister: > + * > + * Unregister the connector with userspace, called from > + * drm_connector_unregister. This is called early in the driver Please add () to drm_connector_unregister() so you end up with a hyperlink. > + * unload sequence to disable userspace access before data > + * structures are torndown. A simple guideline is that this callback > + * should remove everything added during late_register (and vice versa). > + */ > + void (*early_unregister)(struct drm_connector *connector); > + > + /** > * @destroy: > * > * Clean up connector resources. This is called at driver unload time > -- > 2.8.1 >
On Wed, Jun 01, 2016 at 11:57:09AM +0200, Daniel Vetter wrote: > On Mon, May 30, 2016 at 09:38:20AM +0100, Chris Wilson wrote: > > If a driver wants to more precisely control its initialisation and in > > particular, defer registering its interfaces with userspace until after > > everything is setup, it also needs to defer registering the connectors. > > As some devices need more work during registration, add a callback so > > that drivers can do additional work if required for a connector. > > > > Correspondingly, we also require an unregister callback. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: dri-devel@lists.freedesktop.org > > tbh I'd call these hooks simply register/unregister. There shouldn't be > any need for ordering every with interface registration/unregistartion, > assuming drivers don't fumble things. Ah, calling it late_register had the dual purpose of avoiding the 'register' keyword. :| For consistency with resume's naming scheme, it should be register_late. Maybe register_userspace for greater verbage? Though I like the shorthand that we have register as meaning expose the internal object to third parties, including userspace. -Chris
On Wed, Jun 01, 2016 at 11:38:03AM +0100, Chris Wilson wrote: > On Wed, Jun 01, 2016 at 11:57:09AM +0200, Daniel Vetter wrote: > > On Mon, May 30, 2016 at 09:38:20AM +0100, Chris Wilson wrote: > > > If a driver wants to more precisely control its initialisation and in > > > particular, defer registering its interfaces with userspace until after > > > everything is setup, it also needs to defer registering the connectors. > > > As some devices need more work during registration, add a callback so > > > that drivers can do additional work if required for a connector. > > > > > > Correspondingly, we also require an unregister callback. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: dri-devel@lists.freedesktop.org > > > > tbh I'd call these hooks simply register/unregister. There shouldn't be > > any need for ordering every with interface registration/unregistartion, > > assuming drivers don't fumble things. > > Ah, calling it late_register had the dual purpose of avoiding the > 'register' keyword. :| > > For consistency with resume's naming scheme, it should be register_late. > Maybe register_userspace for greater verbage? Though I like the > shorthand that we have register as meaning expose the internal object to > third parties, including userspace. register_aux and unregister_aux, shorthand for auxiliary interfaces? Slight confusion with dp aux, but hey if that tricks folks into putting the dp aux register call in here, even better ;-) Agreed that register_userspace is both doubly the same and not quite the right thing (since it's also about internal pulication within the kernel). -Daniel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d2a6d958ca76..81641544ac3e 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1036,13 +1036,24 @@ int drm_connector_register(struct drm_connector *connector) ret = drm_debugfs_connector_add(connector); if (ret) { - drm_sysfs_connector_remove(connector); - return ret; + goto err_sysfs; + } + + if (connector->funcs->late_register) { + ret = connector->funcs->late_register(connector); + if (ret) + goto err_debugfs; } drm_mode_object_register(connector->dev, &connector->base); return 0; + +err_debugfs: + drm_debugfs_connector_remove(connector); +err_sysfs: + drm_sysfs_connector_remove(connector); + return ret; } EXPORT_SYMBOL(drm_connector_register); @@ -1054,6 +1065,9 @@ EXPORT_SYMBOL(drm_connector_register); */ void drm_connector_unregister(struct drm_connector *connector) { + if (connector->funcs->early_unregister) + connector->funcs->early_unregister(connector); + drm_sysfs_connector_remove(connector); drm_debugfs_connector_remove(connector); } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d1559cd04e3d..77b775cff4e7 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -926,6 +926,31 @@ struct drm_connector_funcs { uint64_t val); /** + * @late_register: + * + * Register the connector with userspace, called from + * drm_connector_register. This should be called after driver + * load during its registration phase. All actions such as registering + * with auxiliary devices (such as drm_dp_aux_register) should be done + * during this callback. A simple guideline is that everything added + * from this callback should be removed during early_unregister (and + * vice versa). + * + */ + int (*late_register)(struct drm_connector *connector); + + /** + * @early_unregister: + * + * Unregister the connector with userspace, called from + * drm_connector_unregister. This is called early in the driver + * unload sequence to disable userspace access before data + * structures are torndown. A simple guideline is that this callback + * should remove everything added during late_register (and vice versa). + */ + void (*early_unregister)(struct drm_connector *connector); + + /** * @destroy: * * Clean up connector resources. This is called at driver unload time
If a driver wants to more precisely control its initialisation and in particular, defer registering its interfaces with userspace until after everything is setup, it also needs to defer registering the connectors. As some devices need more work during registration, add a callback so that drivers can do additional work if required for a connector. Correspondingly, we also require an unregister callback. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/drm_crtc.c | 18 ++++++++++++++++-- include/drm/drm_crtc.h | 25 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-)