[2/2] drm/doc: Improve docs around connector (un)registration
diff mbox series

Message ID 20190917120936.7501-2-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • [1/2] drm/kms: Duct-tape for mode object lifetime checks
Related show

Commit Message

Daniel Vetter Sept. 17, 2019, 12:09 p.m. UTC
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(-)

Comments

Lyude Paul Sept. 17, 2019, 7:40 p.m. UTC | #1
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.
>   */
Daniel Vetter Sept. 18, 2019, 9:28 a.m. UTC | #2
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
>

Patch
diff mbox series

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.
  */