[PATCH v2 01/21] drm: Export drm_dev_init() for subclassing

[PATCH v2 01/21] drm: Export drm_dev_init() for subclassing

[Chris Wilson]

In order to allow drivers to pack their privates and drm_device into one
struct (e.g. for subclassing), export the initialisation routines for
struct drm_device.

v2: Missed return ret. That error path had only one job to do!

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_drv.c | 63 ++++++++++++++++++++++++++++++++++++-----------
 include/drm/drmP.h        |  3 +++
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index bff89226a344..f0553ccd4f71 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -549,11 +549,12 @@ static void drm_fs_inode_free(struct inode *inode)
 }
 
 /**
- * drm_dev_alloc - Allocate new DRM device
- * @driver: DRM driver to allocate device for
+ * drm_dev_init - Initialise new DRM device
+ * @dev: DRM device
+ * @driver: DRM driver
  * @parent: Parent device object
  *
- * Allocate and initialize a new DRM device. No device registration is done.
+ * Initialize a new DRM device. No device registration is done.
  * Call drm_dev_register() to advertice the device to user space and register it
  * with other core subsystems. This should be done last in the device
  * initialization sequence to make sure userspace can't access an inconsistent
@@ -565,18 +566,14 @@ static void drm_fs_inode_free(struct inode *inode)
  * Note that for purely virtual devices @parent can be NULL.
  *
  * RETURNS:
- * Pointer to new DRM device, or NULL if out of memory.
+ * 0 on success, or error code on failure.
  */
-struct drm_device *drm_dev_alloc(struct drm_driver *driver,
-				 struct device *parent)
+int drm_dev_init(struct drm_device *dev,
+		 struct drm_driver *driver,
+		 struct device *parent)
 {
-	struct drm_device *dev;
 	int ret;
 
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (!dev)
-		return NULL;
-
 	kref_init(&dev->ref);
 	dev->dev = parent;
 	dev->driver = driver;
@@ -638,7 +635,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 			goto err_setunique;
 	}
 
-	return dev;
+	return 0;
 
 err_setunique:
 	if (drm_core_check_feature(dev, DRIVER_GEM))
@@ -653,8 +650,46 @@ err_minors:
 	drm_fs_inode_free(dev->anon_inode);
 err_free:
 	mutex_destroy(&dev->master_mutex);
-	kfree(dev);
-	return NULL;
+	return ret;
+}
+EXPORT_SYMBOL(drm_dev_init);
+
+/**
+ * drm_dev_alloc - Allocate new DRM device
+ * @driver: DRM driver to allocate device for
+ * @parent: Parent device object
+ *
+ * Allocate and initialize a new DRM device. No device registration is done.
+ * Call drm_dev_register() to advertice the device to user space and register it
+ * with other core subsystems. This should be done last in the device
+ * initialization sequence to make sure userspace can't access an inconsistent
+ * state.
+ *
+ * The initial ref-count of the object is 1. Use drm_dev_ref() and
+ * drm_dev_unref() to take and drop further ref-counts.
+ *
+ * Note that for purely virtual devices @parent can be NULL.
+ *
+ * RETURNS:
+ * Pointer to new DRM device, or NULL if out of memory.
+ */
+struct drm_device *drm_dev_alloc(struct drm_driver *driver,
+				 struct device *parent)
+{
+	struct drm_device *dev;
+	int ret;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return NULL;
+
+	ret = drm_dev_init(dev, driver, parent);
+	if (ret) {
+		kfree(dev);
+		return NULL;
+	}
+
+	return dev;
 }
 EXPORT_SYMBOL(drm_dev_alloc);
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index c5d29505f937..12dc5f9cad89 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1075,6 +1075,9 @@ extern void drm_sysfs_hotplug_event(struct drm_device *dev);
 
 struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 				 struct device *parent);
+int drm_dev_init(struct drm_device *dev,
+		 struct drm_driver *driver,
+		 struct device *parent);
 void drm_dev_ref(struct drm_device *dev);
 void drm_dev_unref(struct drm_device *dev);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 01/21] drm: Export drm_dev_init() for subclassing

[Daniel Vetter]

On Mon, May 30, 2016 at 09:38:19AM +0100, Chris Wilson wrote:
> In order to allow drivers to pack their privates and drm_device into one
> struct (e.g. for subclassing), export the initialisation routines for
> struct drm_device.
> 
> v2: Missed return ret. That error path had only one job to do!
> 
> 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_drv.c | 63 ++++++++++++++++++++++++++++++++++++-----------
>  include/drm/drmP.h        |  3 +++
>  2 files changed, 52 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index bff89226a344..f0553ccd4f71 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -549,11 +549,12 @@ static void drm_fs_inode_free(struct inode *inode)
>  }
>  
>  /**
> - * drm_dev_alloc - Allocate new DRM device
> - * @driver: DRM driver to allocate device for
> + * drm_dev_init - Initialise new DRM device
> + * @dev: DRM device
> + * @driver: DRM driver
>   * @parent: Parent device object
>   *
> - * Allocate and initialize a new DRM device. No device registration is done.
> + * Initialize a new DRM device. No device registration is done.
>   * Call drm_dev_register() to advertice the device to user space and register it
>   * with other core subsystems. This should be done last in the device
>   * initialization sequence to make sure userspace can't access an inconsistent
> @@ -565,18 +566,14 @@ static void drm_fs_inode_free(struct inode *inode)
>   * Note that for purely virtual devices @parent can be NULL.

Maybe add "Drivers which don't want to allocate their own device structure
embedding &drm_device can instead just call drm_dev_alloc()."
>   *
>   * RETURNS:
> - * Pointer to new DRM device, or NULL if out of memory.
> + * 0 on success, or error code on failure.
>   */
> -struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> -				 struct device *parent)
> +int drm_dev_init(struct drm_device *dev,
> +		 struct drm_driver *driver,
> +		 struct device *parent)
>  {
> -	struct drm_device *dev;
>  	int ret;
>  
> -	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> -	if (!dev)
> -		return NULL;
> -
>  	kref_init(&dev->ref);
>  	dev->dev = parent;
>  	dev->driver = driver;
> @@ -638,7 +635,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
>  			goto err_setunique;
>  	}
>  
> -	return dev;
> +	return 0;
>  
>  err_setunique:
>  	if (drm_core_check_feature(dev, DRIVER_GEM))
> @@ -653,8 +650,46 @@ err_minors:
>  	drm_fs_inode_free(dev->anon_inode);
>  err_free:
>  	mutex_destroy(&dev->master_mutex);
> -	kfree(dev);
> -	return NULL;
> +	return ret;

for "goto err_minors" you need to set ret first, otherwise we'll fail and
return 0. Wasn't a problem because of the unconditional return NULL here.

> +}
> +EXPORT_SYMBOL(drm_dev_init);
> +
> +/**
> + * drm_dev_alloc - Allocate new DRM device
> + * @driver: DRM driver to allocate device for
> + * @parent: Parent device object
> + *
> + * Allocate and initialize a new DRM device. No device registration is done.
> + * Call drm_dev_register() to advertice the device to user space and register it
> + * with other core subsystems. This should be done last in the device
> + * initialization sequence to make sure userspace can't access an inconsistent
> + * state.
> + *
> + * The initial ref-count of the object is 1. Use drm_dev_ref() and
> + * drm_dev_unref() to take and drop further ref-counts.
> + *
> + * Note that for purely virtual devices @parent can be NULL.

Maybe we can also add "Drivers which want to subclass/embed struct
&drm_device should instead look at drm_dev_init()."

> + *
> + * RETURNS:
> + * Pointer to new DRM device, or NULL if out of memory.
> + */
> +struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> +				 struct device *parent)
> +{
> +	struct drm_device *dev;
> +	int ret;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return NULL;
> +
> +	ret = drm_dev_init(dev, driver, parent);
> +	if (ret) {
> +		kfree(dev);
> +		return NULL;
> +	}
> +
> +	return dev;
>  }
>  EXPORT_SYMBOL(drm_dev_alloc);
>  
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index c5d29505f937..12dc5f9cad89 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1075,6 +1075,9 @@ extern void drm_sysfs_hotplug_event(struct drm_device *dev);
>  
>  struct drm_device *drm_dev_alloc(struct drm_driver *driver,
>  				 struct device *parent);
> +int drm_dev_init(struct drm_device *dev,
> +		 struct drm_driver *driver,
> +		 struct device *parent);
>  void drm_dev_ref(struct drm_device *dev);
>  void drm_dev_unref(struct drm_device *dev);
>  int drm_dev_register(struct drm_device *dev, unsigned long flags);

With the one bug in the error handling fixed, and the kerneldoc augmented:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -- 
> 2.8.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 01/21] drm: Export drm_dev_init() for subclassing

[Daniel Vetter]

On Wed, Jun 01, 2016 at 12:01:44PM +0200, Daniel Vetter wrote:
> On Mon, May 30, 2016 at 09:38:19AM +0100, Chris Wilson wrote:
> > In order to allow drivers to pack their privates and drm_device into one
> > struct (e.g. for subclassing), export the initialisation routines for
> > struct drm_device.
> > 
> > v2: Missed return ret. That error path had only one job to do!
> > 
> > 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_drv.c | 63 ++++++++++++++++++++++++++++++++++++-----------
> >  include/drm/drmP.h        |  3 +++
> >  2 files changed, 52 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index bff89226a344..f0553ccd4f71 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -549,11 +549,12 @@ static void drm_fs_inode_free(struct inode *inode)
> >  }
> >  
> >  /**
> > - * drm_dev_alloc - Allocate new DRM device
> > - * @driver: DRM driver to allocate device for
> > + * drm_dev_init - Initialise new DRM device
> > + * @dev: DRM device
> > + * @driver: DRM driver
> >   * @parent: Parent device object
> >   *
> > - * Allocate and initialize a new DRM device. No device registration is done.
> > + * Initialize a new DRM device. No device registration is done.
> >   * Call drm_dev_register() to advertice the device to user space and register it
> >   * with other core subsystems. This should be done last in the device
> >   * initialization sequence to make sure userspace can't access an inconsistent
> > @@ -565,18 +566,14 @@ static void drm_fs_inode_free(struct inode *inode)
> >   * Note that for purely virtual devices @parent can be NULL.
> 
> Maybe add "Drivers which don't want to allocate their own device structure
> embedding &drm_device can instead just call drm_dev_alloc()."
> >   *
> >   * RETURNS:
> > - * Pointer to new DRM device, or NULL if out of memory.
> > + * 0 on success, or error code on failure.
> >   */
> > -struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> > -				 struct device *parent)
> > +int drm_dev_init(struct drm_device *dev,
> > +		 struct drm_driver *driver,
> > +		 struct device *parent)
> >  {
> > -	struct drm_device *dev;
> >  	int ret;
> >  
> > -	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > -	if (!dev)
> > -		return NULL;
> > -
> >  	kref_init(&dev->ref);
> >  	dev->dev = parent;
> >  	dev->driver = driver;
> > @@ -638,7 +635,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> >  			goto err_setunique;
> >  	}
> >  
> > -	return dev;
> > +	return 0;
> >  
> >  err_setunique:
> >  	if (drm_core_check_feature(dev, DRIVER_GEM))
> > @@ -653,8 +650,46 @@ err_minors:
> >  	drm_fs_inode_free(dev->anon_inode);
> >  err_free:
> >  	mutex_destroy(&dev->master_mutex);
> > -	kfree(dev);
> > -	return NULL;
> > +	return ret;
> 
> for "goto err_minors" you need to set ret first, otherwise we'll fail and
> return 0. Wasn't a problem because of the unconditional return NULL here.
> 
> > +}
> > +EXPORT_SYMBOL(drm_dev_init);
> > +
> > +/**
> > + * drm_dev_alloc - Allocate new DRM device
> > + * @driver: DRM driver to allocate device for
> > + * @parent: Parent device object
> > + *
> > + * Allocate and initialize a new DRM device. No device registration is done.
> > + * Call drm_dev_register() to advertice the device to user space and register it
> > + * with other core subsystems. This should be done last in the device
> > + * initialization sequence to make sure userspace can't access an inconsistent
> > + * state.
> > + *
> > + * The initial ref-count of the object is 1. Use drm_dev_ref() and
> > + * drm_dev_unref() to take and drop further ref-counts.
> > + *
> > + * Note that for purely virtual devices @parent can be NULL.
> 
> Maybe we can also add "Drivers which want to subclass/embed struct
> &drm_device should instead look at drm_dev_init()."
> 
> > + *
> > + * RETURNS:
> > + * Pointer to new DRM device, or NULL if out of memory.
> > + */
> > +struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> > +				 struct device *parent)
> > +{
> > +	struct drm_device *dev;
> > +	int ret;
> > +
> > +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > +	if (!dev)
> > +		return NULL;
> > +
> > +	ret = drm_dev_init(dev, driver, parent);
> > +	if (ret) {
> > +		kfree(dev);
> > +		return NULL;
> > +	}
> > +
> > +	return dev;
> >  }
> >  EXPORT_SYMBOL(drm_dev_alloc);
> >  
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index c5d29505f937..12dc5f9cad89 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -1075,6 +1075,9 @@ extern void drm_sysfs_hotplug_event(struct drm_device *dev);
> >  
> >  struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> >  				 struct device *parent);
> > +int drm_dev_init(struct drm_device *dev,
> > +		 struct drm_driver *driver,
> > +		 struct device *parent);
> >  void drm_dev_ref(struct drm_device *dev);
> >  void drm_dev_unref(struct drm_device *dev);
> >  int drm_dev_register(struct drm_device *dev, unsigned long flags);
> 
> With the one bug in the error handling fixed, and the kerneldoc augmented:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

r-b retracted, this is a bit incomplete since it doesn't handle
drm_dev_release. I think we also should add an optional ->device_free
callback and use that in drm_dev_release instead of kfree().

That would finally make the drm_device refcounting also somewhat useful,
since right now you have to release driver-private stuff before the device
is actually gone, which is buggy. Or just leak, which is also buggy ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 01/21] drm: Export drm_dev_init() for subclassing

[Chris Wilson]

On Wed, Jun 01, 2016 at 12:01:44PM +0200, Daniel Vetter wrote:
> With the one bug in the error handling fixed, and the kerneldoc augmented:

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index f0553ccd4f71..4f3d3bba08f7 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -565,6 +565,9 @@ static void drm_fs_inode_free(struct inode *inode)
  *
  * Note that for purely virtual devices @parent can be NULL.
  *
+ * Drivers that do not want to allocate their own device struct
+ * embedding struct &drm_device can call drm_dev_alloc() instead.
+ *
  * RETURNS:
  * 0 on success, or error code on failure.
  */
@@ -616,7 +619,8 @@ int drm_dev_init(struct drm_device *dev,
        if (ret)
                goto err_minors;
 
-       if (drm_ht_create(&dev->map_hash, 12))
+       ret = drm_ht_create(&dev->map_hash, 12);
+       if (ret)
                goto err_minors;
 
        drm_legacy_ctxbitmap_init(dev);
@@ -670,6 +674,9 @@ EXPORT_SYMBOL(drm_dev_init);
  *
  * Note that for purely virtual devices @parent can be NULL.
  *
+ * Drivers that wish to subclass or embed struct &drm_device into their
+ * own struct should look at using drm_dev_init() instead.
+ *
  * RETURNS:
  * Pointer to new DRM device, or NULL if out of memory.
  */

> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks, nice catch for err_minors.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 02/21] drm: Add a callback from connector registering

[Chris Wilson]

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(-)

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
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 02/21] drm: Add a callback from connector registering

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 02/21] drm: Add a callback from connector registering

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 02/21] drm: Add a callback from connector registering

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 03/21] drm: Make drm_connector_register() safe against multiple calls

[Chris Wilson]

Protect against drivers that may try to register the connector more
than once, or who try to unregister it multiple times.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_crtc.c | 9 +++++++++
 include/drm/drm_crtc.h     | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 81641544ac3e..8b9ee921a9e1 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1030,6 +1030,9 @@ int drm_connector_register(struct drm_connector *connector)
 {
 	int ret;
 
+	if (connector->registered)
+		return 0;
+
 	ret = drm_sysfs_connector_add(connector);
 	if (ret)
 		return ret;
@@ -1047,6 +1050,7 @@ int drm_connector_register(struct drm_connector *connector)
 
 	drm_mode_object_register(connector->dev, &connector->base);
 
+	connector->registered = true;
 	return 0;
 
 err_debugfs:
@@ -1065,11 +1069,16 @@ EXPORT_SYMBOL(drm_connector_register);
  */
 void drm_connector_unregister(struct drm_connector *connector)
 {
+	if (!connector->registered)
+		return;
+
 	if (connector->funcs->early_unregister)
 		connector->funcs->early_unregister(connector);
 
 	drm_sysfs_connector_remove(connector);
 	drm_debugfs_connector_remove(connector);
+
+	connector->registered = false;
 }
 EXPORT_SYMBOL(drm_connector_unregister);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 77b775cff4e7..35e47eea5ee1 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1212,6 +1212,7 @@ struct drm_connector {
 	bool interlace_allowed;
 	bool doublescan_allowed;
 	bool stereo_allowed;
+	bool registered;
 	struct list_head modes; /* list of modes on this connector */
 
 	enum drm_connector_status status;
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 03/21] drm: Make drm_connector_register() safe against multiple calls

[Daniel Vetter]

On Mon, May 30, 2016 at 09:38:21AM +0100, Chris Wilson wrote:
> Protect against drivers that may try to register the connector more
> than once, or who try to unregister it multiple times.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_crtc.c | 9 +++++++++
>  include/drm/drm_crtc.h     | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 81641544ac3e..8b9ee921a9e1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1030,6 +1030,9 @@ int drm_connector_register(struct drm_connector *connector)
>  {
>  	int ret;
>  
> +	if (connector->registered)
> +		return 0;
> +
>  	ret = drm_sysfs_connector_add(connector);
>  	if (ret)
>  		return ret;
> @@ -1047,6 +1050,7 @@ int drm_connector_register(struct drm_connector *connector)
>  
>  	drm_mode_object_register(connector->dev, &connector->base);
>  
> +	connector->registered = true;
>  	return 0;
>  
>  err_debugfs:
> @@ -1065,11 +1069,16 @@ EXPORT_SYMBOL(drm_connector_register);
>   */
>  void drm_connector_unregister(struct drm_connector *connector)
>  {
> +	if (!connector->registered)
> +		return;
> +
>  	if (connector->funcs->early_unregister)
>  		connector->funcs->early_unregister(connector);
>  
>  	drm_sysfs_connector_remove(connector);
>  	drm_debugfs_connector_remove(connector);
> +
> +	connector->registered = false;
>  }
>  EXPORT_SYMBOL(drm_connector_unregister);
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 77b775cff4e7..35e47eea5ee1 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1212,6 +1212,7 @@ struct drm_connector {
>  	bool interlace_allowed;
>  	bool doublescan_allowed;
>  	bool stereo_allowed;
> +	bool registered;

Needs kerneldoc for this on, with that fixed

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	struct list_head modes; /* list of modes on this connector */
>  
>  	enum drm_connector_status status;
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 04/21] drm: Automatically unregister the connector during cleanup

[Chris Wilson]

As we now can call drm_connector_unregister() multiple times, provide a
failsafe unregister for a connector when cleaning it up.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_crtc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 8b9ee921a9e1..ba6689ba0ad9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -984,6 +984,8 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct drm_display_mode *mode, *t;
 
+	drm_connector_unregister(connector);
+
 	if (connector->tile_group) {
 		drm_mode_put_tile_group(dev, connector->tile_group);
 		connector->tile_group = NULL;
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 04/21] drm: Automatically unregister the connector during cleanup

[Daniel Vetter]

On Mon, May 30, 2016 at 09:38:22AM +0100, Chris Wilson wrote:
> As we now can call drm_connector_unregister() multiple times, provide a
> failsafe unregister for a connector when cleaning it up.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: dri-devel@lists.freedesktop.org

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Maybe WARN_ON(connector->registered); or is that way too noisy?
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 8b9ee921a9e1..ba6689ba0ad9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -984,6 +984,8 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	struct drm_device *dev = connector->dev;
>  	struct drm_display_mode *mode, *t;
>  
> +	drm_connector_unregister(connector);
> +
>  	if (connector->tile_group) {
>  		drm_mode_put_tile_group(dev, connector->tile_group);
>  		connector->tile_group = NULL;
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 05/21] drm: Minimally initialise drm_dp_aux

[Chris Wilson]

When trying to split up the initialisation phase and the registration
phase, one immediate problem encountered is trying to use our own i2c
devices before registration with userspace. drm_dp_aux in particular
only offers an interface for setting up the device after we have exposed
the connector via sysfs. In order to break the chicken-and-egg problem,
export drm_dp_aux_init() to minimally prepare the i2c device for
internal use before drm_connector_register().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 18 +++++++++++++-----
 include/drm/drm_dp_helper.h     |  1 +
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index eeaf5a7c3aa7..e1900d386685 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -774,6 +774,17 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
 	.master_xfer = drm_dp_i2c_xfer,
 };
 
+void drm_dp_aux_init(struct drm_dp_aux *aux)
+{
+	mutex_init(&aux->hw_mutex);
+	rt_mutex_init(&aux->ddc.bus_lock);
+
+	aux->ddc.algo = &drm_dp_i2c_algo;
+	aux->ddc.algo_data = aux;
+	aux->ddc.retries = 3;
+}
+EXPORT_SYMBOL(drm_dp_aux_init);
+
 /**
  * drm_dp_aux_register() - initialise and register aux channel
  * @aux: DisplayPort AUX channel
@@ -784,11 +795,8 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
 {
 	int ret;
 
-	mutex_init(&aux->hw_mutex);
-
-	aux->ddc.algo = &drm_dp_i2c_algo;
-	aux->ddc.algo_data = aux;
-	aux->ddc.retries = 3;
+	if (!aux->ddc.algo)
+		drm_dp_aux_init(aux);
 
 	aux->ddc.class = I2C_CLASS_DDC;
 	aux->ddc.owner = THIS_MODULE;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 5a848e734422..4d85cf2874af 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -805,6 +805,7 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
 
+void drm_dp_aux_init(struct drm_dp_aux *aux);
 int drm_dp_aux_register(struct drm_dp_aux *aux);
 void drm_dp_aux_unregister(struct drm_dp_aux *aux);
 
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 05/21] drm: Minimally initialise drm_dp_aux

[Daniel Vetter]

On Mon, May 30, 2016 at 09:38:23AM +0100, Chris Wilson wrote:
> When trying to split up the initialisation phase and the registration
> phase, one immediate problem encountered is trying to use our own i2c
> devices before registration with userspace. drm_dp_aux in particular
> only offers an interface for setting up the device after we have exposed
> the connector via sysfs. In order to break the chicken-and-egg problem,
> export drm_dp_aux_init() to minimally prepare the i2c device for
> internal use before drm_connector_register().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 18 +++++++++++++-----
>  include/drm/drm_dp_helper.h     |  1 +
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index eeaf5a7c3aa7..e1900d386685 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -774,6 +774,17 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
>  	.master_xfer = drm_dp_i2c_xfer,
>  };
>  
> +void drm_dp_aux_init(struct drm_dp_aux *aux)

Needs kerneldoc. Pleas also cross-reference to drm_dp_aux_register. Didn't
read all the i2c code to make sure stuff really works with just that, but
makes sense.

All three core patches also need an ack from Dave.
-Daniel

> +{
> +	mutex_init(&aux->hw_mutex);
> +	rt_mutex_init(&aux->ddc.bus_lock);
> +
> +	aux->ddc.algo = &drm_dp_i2c_algo;
> +	aux->ddc.algo_data = aux;
> +	aux->ddc.retries = 3;
> +}
> +EXPORT_SYMBOL(drm_dp_aux_init);
> +
>  /**
>   * drm_dp_aux_register() - initialise and register aux channel
>   * @aux: DisplayPort AUX channel
> @@ -784,11 +795,8 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>  {
>  	int ret;
>  
> -	mutex_init(&aux->hw_mutex);
> -
> -	aux->ddc.algo = &drm_dp_i2c_algo;
> -	aux->ddc.algo_data = aux;
> -	aux->ddc.retries = 3;
> +	if (!aux->ddc.algo)
> +		drm_dp_aux_init(aux);
>  
>  	aux->ddc.class = I2C_CLASS_DDC;
>  	aux->ddc.owner = THIS_MODULE;
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 5a848e734422..4d85cf2874af 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -805,6 +805,7 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  
> +void drm_dp_aux_init(struct drm_dp_aux *aux);
>  int drm_dp_aux_register(struct drm_dp_aux *aux);
>  void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>  
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 05/21] drm: Minimally initialise drm_dp_aux

[Chris Wilson]

On Wed, Jun 01, 2016 at 11:49:03AM +0200, Daniel Vetter wrote:
> On Mon, May 30, 2016 at 09:38:23AM +0100, Chris Wilson wrote:
> > When trying to split up the initialisation phase and the registration
> > phase, one immediate problem encountered is trying to use our own i2c
> > devices before registration with userspace. drm_dp_aux in particular
> > only offers an interface for setting up the device after we have exposed
> > the connector via sysfs. In order to break the chicken-and-egg problem,
> > export drm_dp_aux_init() to minimally prepare the i2c device for
> > internal use before drm_connector_register().
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > Cc: dri-devel@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 18 +++++++++++++-----
> >  include/drm/drm_dp_helper.h     |  1 +
> >  2 files changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index eeaf5a7c3aa7..e1900d386685 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -774,6 +774,17 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
> >  	.master_xfer = drm_dp_i2c_xfer,
> >  };
> >  
> > +void drm_dp_aux_init(struct drm_dp_aux *aux)
> 
> Needs kerneldoc. Pleas also cross-reference to drm_dp_aux_register. Didn't
> read all the i2c code to make sure stuff really works with just that, but
> makes sense.

Already added the kerneldoc (mostly copy'pasted from
drm_dp_aux_register), but good job you didn't check the i2c code as it
changed in v4.7. However, it motivated a better patch as i2c now allows
for us to control the locking and so we can make the interaction between
i2c and the drm_dp_aux->hw_mutex (used for the DPCD bypass) much
clearer.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx