| Message ID | 20190917120936.7501-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/kms: Duct-tape for mode object lifetime checks | expand |
Reviewed-by: Lyude Paul <lyude@redhat.com> On Tue, 2019-09-17 at 14:09 +0200, Daniel Vetter wrote: > Current code is quite a mess unfortunately, so also add a todo.rst > entry to maybe fix it up eventually. > > Cc: Michel Dänzer <michel@daenzer.net> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > Documentation/gpu/todo.rst | 12 ++++++++++++ > drivers/gpu/drm/drm_connector.c | 10 ++++++++-- > drivers/gpu/drm/drm_dp_helper.c | 8 ++++++++ > 3 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 32787acff0a8..8dc147c93c9c 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -284,6 +284,18 @@ drm_fb_helper tasks > removed: drm_fb_helper_single_add_all_connectors(), > drm_fb_helper_add_one_connector() and > drm_fb_helper_remove_one_connector(). > > +connector register/unregister fixes > +----------------------------------- > + > +- For most connectors it's a no-op to call > drm_connector_register/unregister > + directly from driver code, drm_dev_register/unregister take care of this > + already. We can remove all of them. > + > +- For dp drivers it's a bit more a mess, since we need the connector to be > + registered when calling drm_dp_aux_register. Fix this by instead calling > + drm_dp_aux_init, and moving the actual registering into a late_register > + callback as recommended in the kerneldoc. > + > Core refactorings > ================= > > diff --git a/drivers/gpu/drm/drm_connector.c > b/drivers/gpu/drm/drm_connector.c > index 4c766624b20d..cd5048ceab1d 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -480,7 +480,10 @@ EXPORT_SYMBOL(drm_connector_cleanup); > * drm_connector_register - register a connector > * @connector: the connector to register > * > - * Register userspace interfaces for a connector > + * Register userspace interfaces for a connector. Only call this for > connectors > + * which can be hotplugged after drm_dev_register() has been called > already, > + * e.g. DP MST connectors. All other connectors will be registered > automatically > + * when calling drm_dev_register(). > * > * Returns: > * Zero on success, error code on failure. > @@ -526,7 +529,10 @@ EXPORT_SYMBOL(drm_connector_register); > * drm_connector_unregister - unregister a connector > * @connector: the connector to unregister > * > - * Unregister userspace interfaces for a connector > + * Unregister userspace interfaces for a connector. Only call this for > + * connectors which have registered explicitly by calling > drm_dev_register(), > + * since connectors are unregistered automatically when > drm_dev_unregister() is > + * called. > */ > void drm_connector_unregister(struct drm_connector *connector) > { > diff --git a/drivers/gpu/drm/drm_dp_helper.c > b/drivers/gpu/drm/drm_dp_helper.c > index ffc68d305afe..f373798d82f6 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -1109,6 +1109,14 @@ EXPORT_SYMBOL(drm_dp_aux_init); > * @aux: DisplayPort AUX channel > * > * Automatically calls drm_dp_aux_init() if this hasn't been done yet. > + * This should only be called when the underlying &struct drm_connector is > + * initialiazed already. Therefore the best place to call this is from > + * &drm_connector_funcs.late_register. Not that drivers which don't follow > this > + * will Oops when CONFIG_DRM_DP_AUX_CHARDEV is enabled. > + * > + * Drivers which need to use the aux channel before that point (e.g. at > driver > + * load time, before drm_dev_register() has been called) need to call > + * drm_dp_aux_init(). > * > * Returns 0 on success or a negative error code on failure. > */
On Tue, Sep 17, 2019 at 03:40:29PM -0400, Lyude Paul wrote: > Reviewed-by: Lyude Paul <lyude@redhat.com> Thanks for taking a look, pushed to drm-misc-next. -Daniel > > On Tue, 2019-09-17 at 14:09 +0200, Daniel Vetter wrote: > > Current code is quite a mess unfortunately, so also add a todo.rst > > entry to maybe fix it up eventually. > > > > Cc: Michel Dänzer <michel@daenzer.net> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > Documentation/gpu/todo.rst | 12 ++++++++++++ > > drivers/gpu/drm/drm_connector.c | 10 ++++++++-- > > drivers/gpu/drm/drm_dp_helper.c | 8 ++++++++ > > 3 files changed, 28 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > > index 32787acff0a8..8dc147c93c9c 100644 > > --- a/Documentation/gpu/todo.rst > > +++ b/Documentation/gpu/todo.rst > > @@ -284,6 +284,18 @@ drm_fb_helper tasks > > removed: drm_fb_helper_single_add_all_connectors(), > > drm_fb_helper_add_one_connector() and > > drm_fb_helper_remove_one_connector(). > > > > +connector register/unregister fixes > > +----------------------------------- > > + > > +- For most connectors it's a no-op to call > > drm_connector_register/unregister > > + directly from driver code, drm_dev_register/unregister take care of this > > + already. We can remove all of them. > > + > > +- For dp drivers it's a bit more a mess, since we need the connector to be > > + registered when calling drm_dp_aux_register. Fix this by instead calling > > + drm_dp_aux_init, and moving the actual registering into a late_register > > + callback as recommended in the kerneldoc. > > + > > Core refactorings > > ================= > > > > diff --git a/drivers/gpu/drm/drm_connector.c > > b/drivers/gpu/drm/drm_connector.c > > index 4c766624b20d..cd5048ceab1d 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -480,7 +480,10 @@ EXPORT_SYMBOL(drm_connector_cleanup); > > * drm_connector_register - register a connector > > * @connector: the connector to register > > * > > - * Register userspace interfaces for a connector > > + * Register userspace interfaces for a connector. Only call this for > > connectors > > + * which can be hotplugged after drm_dev_register() has been called > > already, > > + * e.g. DP MST connectors. All other connectors will be registered > > automatically > > + * when calling drm_dev_register(). > > * > > * Returns: > > * Zero on success, error code on failure. > > @@ -526,7 +529,10 @@ EXPORT_SYMBOL(drm_connector_register); > > * drm_connector_unregister - unregister a connector > > * @connector: the connector to unregister > > * > > - * Unregister userspace interfaces for a connector > > + * Unregister userspace interfaces for a connector. Only call this for > > + * connectors which have registered explicitly by calling > > drm_dev_register(), > > + * since connectors are unregistered automatically when > > drm_dev_unregister() is > > + * called. > > */ > > void drm_connector_unregister(struct drm_connector *connector) > > { > > diff --git a/drivers/gpu/drm/drm_dp_helper.c > > b/drivers/gpu/drm/drm_dp_helper.c > > index ffc68d305afe..f373798d82f6 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -1109,6 +1109,14 @@ EXPORT_SYMBOL(drm_dp_aux_init); > > * @aux: DisplayPort AUX channel > > * > > * Automatically calls drm_dp_aux_init() if this hasn't been done yet. > > + * This should only be called when the underlying &struct drm_connector is > > + * initialiazed already. Therefore the best place to call this is from > > + * &drm_connector_funcs.late_register. Not that drivers which don't follow > > this > > + * will Oops when CONFIG_DRM_DP_AUX_CHARDEV is enabled. > > + * > > + * Drivers which need to use the aux channel before that point (e.g. at > > driver > > + * load time, before drm_dev_register() has been called) need to call > > + * drm_dp_aux_init(). > > * > > * Returns 0 on success or a negative error code on failure. > > */ > -- > Cheers, > Lyude Paul >
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 32787acff0a8..8dc147c93c9c 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -284,6 +284,18 @@ drm_fb_helper tasks removed: drm_fb_helper_single_add_all_connectors(), drm_fb_helper_add_one_connector() and drm_fb_helper_remove_one_connector(). +connector register/unregister fixes +----------------------------------- + +- For most connectors it's a no-op to call drm_connector_register/unregister + directly from driver code, drm_dev_register/unregister take care of this + already. We can remove all of them. + +- For dp drivers it's a bit more a mess, since we need the connector to be + registered when calling drm_dp_aux_register. Fix this by instead calling + drm_dp_aux_init, and moving the actual registering into a late_register + callback as recommended in the kerneldoc. + Core refactorings ================= diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 4c766624b20d..cd5048ceab1d 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -480,7 +480,10 @@ EXPORT_SYMBOL(drm_connector_cleanup); * drm_connector_register - register a connector * @connector: the connector to register * - * Register userspace interfaces for a connector + * Register userspace interfaces for a connector. Only call this for connectors + * which can be hotplugged after drm_dev_register() has been called already, + * e.g. DP MST connectors. All other connectors will be registered automatically + * when calling drm_dev_register(). * * Returns: * Zero on success, error code on failure. @@ -526,7 +529,10 @@ EXPORT_SYMBOL(drm_connector_register); * drm_connector_unregister - unregister a connector * @connector: the connector to unregister * - * Unregister userspace interfaces for a connector + * Unregister userspace interfaces for a connector. Only call this for + * connectors which have registered explicitly by calling drm_dev_register(), + * since connectors are unregistered automatically when drm_dev_unregister() is + * called. */ void drm_connector_unregister(struct drm_connector *connector) { diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index ffc68d305afe..f373798d82f6 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -1109,6 +1109,14 @@ EXPORT_SYMBOL(drm_dp_aux_init); * @aux: DisplayPort AUX channel * * Automatically calls drm_dp_aux_init() if this hasn't been done yet. + * This should only be called when the underlying &struct drm_connector is + * initialiazed already. Therefore the best place to call this is from + * &drm_connector_funcs.late_register. Not that drivers which don't follow this + * will Oops when CONFIG_DRM_DP_AUX_CHARDEV is enabled. + * + * Drivers which need to use the aux channel before that point (e.g. at driver + * load time, before drm_dev_register() has been called) need to call + * drm_dp_aux_init(). * * Returns 0 on success or a negative error code on failure. */
Current code is quite a mess unfortunately, so also add a todo.rst entry to maybe fix it up eventually. Cc: Michel Dänzer <michel@daenzer.net> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- Documentation/gpu/todo.rst | 12 ++++++++++++ drivers/gpu/drm/drm_connector.c | 10 ++++++++-- drivers/gpu/drm/drm_dp_helper.c | 8 ++++++++ 3 files changed, 28 insertions(+), 2 deletions(-)