[PATCH v3 01/33] drm: Export drm_dev_init() for subclassing

[PATCH v3 01/33] 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!
v3: Cross-referencing drm_dev_init/drm_dev_alloc in kerneldoc, fix
missed error code for goto err_minors.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_drv.c | 72 +++++++++++++++++++++++++++++++++++++----------
 include/drm/drmP.h        |  3 ++
 2 files changed, 60 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index bff89226a344..81fccc26e319 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
@@ -564,19 +565,18 @@ 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:
- * 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;
@@ -619,7 +619,8 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 	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);
@@ -638,7 +639,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 +654,49 @@ 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.
+ *
+ * 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.
+ */
+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 9e5eefd6f733..c49fddb0708c 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1076,6 +1076,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

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

[PATCH v3 02/33] 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: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 18 ++++++++++++++++--
 include/drm/drm_crtc.h     | 28 ++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a9cabef98f6b..68b2385ee505 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1018,13 +1018,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);
 
@@ -1036,6 +1047,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 e690021ce4cc..e217c5d46010 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -929,6 +929,34 @@ struct drm_connector_funcs {
 			     uint64_t val);
 
 	/**
+	 * @late_register:
+	 *
+	 * This optional hook can be used to register additional userspace
+	 * interfaces attached to the connector, light backlight control, i2c,
+	 * DP aux or similar interfaces. It is called late in the driver load
+	 * sequence from drm_connector_register() when registering all the
+	 * core drm connector interfaces. Everything added from this callback
+	 * should be unregistered in the early_unregister callback.
+	 *
+	 * Returns:
+	 *
+	 * 0 on success, or a negative error code on failure.
+	 *
+	 */
+	int (*late_register)(struct drm_connector *connector);
+
+	/**
+	 * @early_unregister:
+	 *
+	 * This optional hook should be used to unregister the additional
+	 * userspace interfaces attached to the connector from
+	 * late_unregister(). It is called from drm_connector_unregister(),
+	 * early in the driver unload sequence to disable userspace access
+	 * before data structures are torndown.
+	 */
+	void (*early_unregister)(struct drm_connector *connector);
+
+	/**
 	 * @destroy:
 	 *
 	 * Clean up connector resources. This is called at driver unload time
-- 
2.8.1

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

[PATCH v3 03/33] 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: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 9 +++++++++
 include/drm/drm_crtc.h     | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 68b2385ee505..e01c0fcf26cc 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1012,6 +1012,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;
@@ -1029,6 +1032,7 @@ int drm_connector_register(struct drm_connector *connector)
 
 	drm_mode_object_register(connector->dev, &connector->base);
 
+	connector->registered = true;
 	return 0;
 
 err_debugfs:
@@ -1047,11 +1051,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 e217c5d46010..8ca8e940a392 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1165,6 +1165,7 @@ struct drm_encoder {
  * @interlace_allowed: can this connector handle interlaced modes?
  * @doublescan_allowed: can this connector handle doublescan?
  * @stereo_allowed: can this connector handle stereo modes?
+ * @registered: is this connector exposed (registered) with userspace?
  * @modes: modes available on this connector (from fill_modes() + user)
  * @status: one of the drm_connector_status enums (connected, not, or unknown)
  * @probed_modes: list of modes derived directly from the display
@@ -1222,6 +1223,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

[PATCH v3 04/33] 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.

v2: Add a WARN to catch any connectors that are still visible to
userspace when we come to destoy them.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e01c0fcf26cc..c15cb43be78e 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -966,6 +966,12 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct drm_display_mode *mode, *t;
 
+	/* The connector should have been removed from userspace long before
+	 * it is finally destroyed.
+	 */
+	if (WARN_ON(connector->registered))
+		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

[PATCH v3 05/33] drm: Pass the drm_dp_aux->hw_mutex to i2c for its locking

[Chris Wilson]

Rather than have both drm_dp_aux lock within its transfer, and i2c to
lock around the transfer, use the same lock by filling in the locking
callbacks that i2c wants to use. We require our own hw_mutex as we
bypass i2c_transfer for drm_dp_dpcd_access().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index eeaf5a7c3aa7..4b088afa21b2 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -708,8 +708,6 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 
 	memset(&msg, 0, sizeof(msg));
 
-	mutex_lock(&aux->hw_mutex);
-
 	for (i = 0; i < num; i++) {
 		msg.address = msgs[i].addr;
 		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
@@ -764,8 +762,6 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 	msg.size = 0;
 	(void)drm_dp_i2c_do_msg(aux, &msg);
 
-	mutex_unlock(&aux->hw_mutex);
-
 	return err;
 }
 
@@ -774,6 +770,26 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
 	.master_xfer = drm_dp_i2c_xfer,
 };
 
+static struct drm_dp_aux *i2c_to_aux(struct i2c_adapter *i2c)
+{
+	return container_of(i2c, struct drm_dp_aux, ddc);
+}
+
+static void lock_bus(struct i2c_adapter *i2c, unsigned int flags)
+{
+	mutex_lock(&i2c_to_aux(i2c)->hw_mutex);
+}
+
+static int trylock_bus(struct i2c_adapter *i2c, unsigned int flags)
+{
+	return mutex_trylock(&i2c_to_aux(i2c)->hw_mutex);
+}
+
+static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags)
+{
+	mutex_unlock(&i2c_to_aux(i2c)->hw_mutex);
+}
+
 /**
  * drm_dp_aux_register() - initialise and register aux channel
  * @aux: DisplayPort AUX channel
@@ -790,6 +806,10 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
 	aux->ddc.algo_data = aux;
 	aux->ddc.retries = 3;
 
+	aux->ddc.lock_bus = lock_bus;
+	aux->ddc.trylock_bus = trylock_bus;
+	aux->ddc.unlock_bus = unlock_bus;
+
 	aux->ddc.class = I2C_CLASS_DDC;
 	aux->ddc.owner = THIS_MODULE;
 	aux->ddc.dev.parent = aux->dev;
-- 
2.8.1

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

Re: [PATCH v3 05/33] drm: Pass the drm_dp_aux->hw_mutex to i2c for its locking

[Ville Syrjälä]

On Fri, Jun 03, 2016 at 03:36:48PM +0100, Chris Wilson wrote:
> Rather than have both drm_dp_aux lock within its transfer, and i2c to
> lock around the transfer, use the same lock by filling in the locking
> callbacks that i2c wants to use. We require our own hw_mutex as we
> bypass i2c_transfer for drm_dp_dpcd_access().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index eeaf5a7c3aa7..4b088afa21b2 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -708,8 +708,6 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  
>  	memset(&msg, 0, sizeof(msg));
>  
> -	mutex_lock(&aux->hw_mutex);
> -
>  	for (i = 0; i < num; i++) {
>  		msg.address = msgs[i].addr;
>  		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
> @@ -764,8 +762,6 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  	msg.size = 0;
>  	(void)drm_dp_i2c_do_msg(aux, &msg);
>  
> -	mutex_unlock(&aux->hw_mutex);
> -
>  	return err;
>  }

AFAICS the only functional difference will be that we'll hold the lock
across the retries performed by the core. Looks like we set the retry
count to 3 for whatever reason, but I'm not seeing that we'd ever
actually return -EAGAIN so I guess the core will neever retry.
Oh, actually if we ever end up in the trylock path
(in_atomic||irqs_disabled) then the core would retry. But I don't think
we should ever do that.

Patch seems OK to me:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
> @@ -774,6 +770,26 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
>  	.master_xfer = drm_dp_i2c_xfer,
>  };
>  
> +static struct drm_dp_aux *i2c_to_aux(struct i2c_adapter *i2c)
> +{
> +	return container_of(i2c, struct drm_dp_aux, ddc);
> +}
> +
> +static void lock_bus(struct i2c_adapter *i2c, unsigned int flags)
> +{
> +	mutex_lock(&i2c_to_aux(i2c)->hw_mutex);
> +}
> +
> +static int trylock_bus(struct i2c_adapter *i2c, unsigned int flags)
> +{
> +	return mutex_trylock(&i2c_to_aux(i2c)->hw_mutex);
> +}
> +
> +static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags)
> +{
> +	mutex_unlock(&i2c_to_aux(i2c)->hw_mutex);
> +}
> +
>  /**
>   * drm_dp_aux_register() - initialise and register aux channel
>   * @aux: DisplayPort AUX channel
> @@ -790,6 +806,10 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>  	aux->ddc.algo_data = aux;
>  	aux->ddc.retries = 3;
>  
> +	aux->ddc.lock_bus = lock_bus;
> +	aux->ddc.trylock_bus = trylock_bus;
> +	aux->ddc.unlock_bus = unlock_bus;
> +
>  	aux->ddc.class = I2C_CLASS_DDC;
>  	aux->ddc.owner = THIS_MODULE;
>  	aux->ddc.dev.parent = aux->dev;
> -- 
> 2.8.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 06/33] 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 (to read EDID during device
discovery). 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: Dave Airlie <airlied@redhat.com>
Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 26 +++++++++++++++++++++-----
 include/drm/drm_dp_helper.h     |  1 +
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 4b088afa21b2..9b4ec65e1de6 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -791,15 +791,16 @@ static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags)
 }
 
 /**
- * drm_dp_aux_register() - initialise and register aux channel
+ * drm_dp_aux_init() - minimally initialise an aux channel
  * @aux: DisplayPort AUX channel
  *
- * Returns 0 on success or a negative error code on failure.
+ * If you need to use the drm_dp_aux's i2c adapter prior to registering it
+ * with the outside world, call drm_dp_aux_init() first. You must still
+ * call drm_dp_aux_register() once the connector has been registered to
+ * allow userspace access to the auxiliary DP channel.
  */
-int drm_dp_aux_register(struct drm_dp_aux *aux)
+void drm_dp_aux_init(struct drm_dp_aux *aux)
 {
-	int ret;
-
 	mutex_init(&aux->hw_mutex);
 
 	aux->ddc.algo = &drm_dp_i2c_algo;
@@ -809,6 +810,21 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
 	aux->ddc.lock_bus = lock_bus;
 	aux->ddc.trylock_bus = trylock_bus;
 	aux->ddc.unlock_bus = unlock_bus;
+}
+EXPORT_SYMBOL(drm_dp_aux_init);
+
+/**
+ * drm_dp_aux_register() - initialise and register aux channel
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_aux_register(struct drm_dp_aux *aux)
+{
+	int ret;
+
+	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

Re: [PATCH v3 06/33] drm: Minimally initialise drm_dp_aux

[Ville Syrjälä]

On Fri, Jun 03, 2016 at 03:36:49PM +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 (to read EDID during device
> discovery). 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: Dave Airlie <airlied@redhat.com>
> Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 26 +++++++++++++++++++++-----
>  include/drm/drm_dp_helper.h     |  1 +
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 4b088afa21b2..9b4ec65e1de6 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -791,15 +791,16 @@ static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags)
>  }
>  
>  /**
> - * drm_dp_aux_register() - initialise and register aux channel
> + * drm_dp_aux_init() - minimally initialise an aux channel
>   * @aux: DisplayPort AUX channel
>   *
> - * Returns 0 on success or a negative error code on failure.
> + * If you need to use the drm_dp_aux's i2c adapter prior to registering it
> + * with the outside world, call drm_dp_aux_init() first. You must still
> + * call drm_dp_aux_register() once the connector has been registered to
> + * allow userspace access to the auxiliary DP channel.
>   */
> -int drm_dp_aux_register(struct drm_dp_aux *aux)
> +void drm_dp_aux_init(struct drm_dp_aux *aux)
>  {
> -	int ret;
> -
>  	mutex_init(&aux->hw_mutex);
>  
>  	aux->ddc.algo = &drm_dp_i2c_algo;
> @@ -809,6 +810,21 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>  	aux->ddc.lock_bus = lock_bus;
>  	aux->ddc.trylock_bus = trylock_bus;
>  	aux->ddc.unlock_bus = unlock_bus;
> +}
> +EXPORT_SYMBOL(drm_dp_aux_init);

This doesn't feel very safe to me. To me it looks like the i2c core
wasn't designed to have the adapter be used before i2c_add_adapter()
is called. I guess it might work in this case since you provide your
own lock vfuncs.

I think someone should fix the i2c core to split i2c_add_adapter()
& co. into init and register phases. Cc:ing i2c folks...

> +
> +/**
> + * drm_dp_aux_register() - initialise and register aux channel
> + * @aux: DisplayPort AUX channel
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_aux_register(struct drm_dp_aux *aux)
> +{
> +	int ret;
> +
> +	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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 06/33] drm: Minimally initialise drm_dp_aux

[Chris Wilson]

On Fri, Jun 03, 2016 at 05:59:11PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 03, 2016 at 03:36:49PM +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 (to read EDID during device
> > discovery). 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: Dave Airlie <airlied@redhat.com>
> > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: dri-devel@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 26 +++++++++++++++++++++-----
> >  include/drm/drm_dp_helper.h     |  1 +
> >  2 files changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 4b088afa21b2..9b4ec65e1de6 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -791,15 +791,16 @@ static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags)
> >  }
> >  
> >  /**
> > - * drm_dp_aux_register() - initialise and register aux channel
> > + * drm_dp_aux_init() - minimally initialise an aux channel
> >   * @aux: DisplayPort AUX channel
> >   *
> > - * Returns 0 on success or a negative error code on failure.
> > + * If you need to use the drm_dp_aux's i2c adapter prior to registering it
> > + * with the outside world, call drm_dp_aux_init() first. You must still
> > + * call drm_dp_aux_register() once the connector has been registered to
> > + * allow userspace access to the auxiliary DP channel.
> >   */
> > -int drm_dp_aux_register(struct drm_dp_aux *aux)
> > +void drm_dp_aux_init(struct drm_dp_aux *aux)
> >  {
> > -	int ret;
> > -
> >  	mutex_init(&aux->hw_mutex);
> >  
> >  	aux->ddc.algo = &drm_dp_i2c_algo;
> > @@ -809,6 +810,21 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
> >  	aux->ddc.lock_bus = lock_bus;
> >  	aux->ddc.trylock_bus = trylock_bus;
> >  	aux->ddc.unlock_bus = unlock_bus;
> > +}
> > +EXPORT_SYMBOL(drm_dp_aux_init);
> 
> This doesn't feel very safe to me. To me it looks like the i2c core
> wasn't designed to have the adapter be used before i2c_add_adapter()
> is called. I guess it might work in this case since you provide your
> own lock vfuncs.
> 
> I think someone should fix the i2c core to split i2c_add_adapter()
> & co. into init and register phases. Cc:ing i2c folks...

As you've seen, I sent the patches to split i2c_add_adapter() to allow for
us to call i2c_init_adapter() here instead. It still requires the same
basic review that this init (the same as above) is sufficient for using
i2c_transfer().

I would like to get the regression fix completed without much futher
ado - in particular, not depending upon landing an external patch.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 06/33] drm: Minimally initialise drm_dp_aux

[Ville Syrjälä]

On Thu, Jun 09, 2016 at 09:57:24PM +0100, Chris Wilson wrote:
> On Fri, Jun 03, 2016 at 05:59:11PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 03, 2016 at 03:36:49PM +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 (to read EDID during device
> > > discovery). 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: Dave Airlie <airlied@redhat.com>
> > > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > ---
> > >  drivers/gpu/drm/drm_dp_helper.c | 26 +++++++++++++++++++++-----
> > >  include/drm/drm_dp_helper.h     |  1 +
> > >  2 files changed, 22 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > index 4b088afa21b2..9b4ec65e1de6 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -791,15 +791,16 @@ static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags)
> > >  }
> > >  
> > >  /**
> > > - * drm_dp_aux_register() - initialise and register aux channel
> > > + * drm_dp_aux_init() - minimally initialise an aux channel
> > >   * @aux: DisplayPort AUX channel
> > >   *
> > > - * Returns 0 on success or a negative error code on failure.
> > > + * If you need to use the drm_dp_aux's i2c adapter prior to registering it
> > > + * with the outside world, call drm_dp_aux_init() first. You must still
> > > + * call drm_dp_aux_register() once the connector has been registered to
> > > + * allow userspace access to the auxiliary DP channel.
> > >   */
> > > -int drm_dp_aux_register(struct drm_dp_aux *aux)
> > > +void drm_dp_aux_init(struct drm_dp_aux *aux)
> > >  {
> > > -	int ret;
> > > -
> > >  	mutex_init(&aux->hw_mutex);
> > >  
> > >  	aux->ddc.algo = &drm_dp_i2c_algo;
> > > @@ -809,6 +810,21 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
> > >  	aux->ddc.lock_bus = lock_bus;
> > >  	aux->ddc.trylock_bus = trylock_bus;
> > >  	aux->ddc.unlock_bus = unlock_bus;
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_aux_init);
> > 
> > This doesn't feel very safe to me. To me it looks like the i2c core
> > wasn't designed to have the adapter be used before i2c_add_adapter()
> > is called. I guess it might work in this case since you provide your
> > own lock vfuncs.
> > 
> > I think someone should fix the i2c core to split i2c_add_adapter()
> > & co. into init and register phases. Cc:ing i2c folks...
> 
> As you've seen, I sent the patches to split i2c_add_adapter() to allow for
> us to call i2c_init_adapter() here instead. It still requires the same
> basic review that this init (the same as above) is sufficient for using
> i2c_transfer().
> 
> I would like to get the regression fix completed without much futher
> ado - in particular, not depending upon landing an external patch.

What regression is this?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 06/33] drm: Minimally initialise drm_dp_aux

[Chris Wilson]

On Fri, Jun 10, 2016 at 01:26:24PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 09, 2016 at 09:57:24PM +0100, Chris Wilson wrote:
> > On Fri, Jun 03, 2016 at 05:59:11PM +0300, Ville Syrjälä wrote:
> > > On Fri, Jun 03, 2016 at 03:36:49PM +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 (to read EDID during device
> > > > discovery). 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: Dave Airlie <airlied@redhat.com>
> > > > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > ---
> > > >  drivers/gpu/drm/drm_dp_helper.c | 26 +++++++++++++++++++++-----
> > > >  include/drm/drm_dp_helper.h     |  1 +
> > > >  2 files changed, 22 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > > index 4b088afa21b2..9b4ec65e1de6 100644
> > > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > > @@ -791,15 +791,16 @@ static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags)
> > > >  }
> > > >  
> > > >  /**
> > > > - * drm_dp_aux_register() - initialise and register aux channel
> > > > + * drm_dp_aux_init() - minimally initialise an aux channel
> > > >   * @aux: DisplayPort AUX channel
> > > >   *
> > > > - * Returns 0 on success or a negative error code on failure.
> > > > + * If you need to use the drm_dp_aux's i2c adapter prior to registering it
> > > > + * with the outside world, call drm_dp_aux_init() first. You must still
> > > > + * call drm_dp_aux_register() once the connector has been registered to
> > > > + * allow userspace access to the auxiliary DP channel.
> > > >   */
> > > > -int drm_dp_aux_register(struct drm_dp_aux *aux)
> > > > +void drm_dp_aux_init(struct drm_dp_aux *aux)
> > > >  {
> > > > -	int ret;
> > > > -
> > > >  	mutex_init(&aux->hw_mutex);
> > > >  
> > > >  	aux->ddc.algo = &drm_dp_i2c_algo;
> > > > @@ -809,6 +810,21 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
> > > >  	aux->ddc.lock_bus = lock_bus;
> > > >  	aux->ddc.trylock_bus = trylock_bus;
> > > >  	aux->ddc.unlock_bus = unlock_bus;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_dp_aux_init);
> > > 
> > > This doesn't feel very safe to me. To me it looks like the i2c core
> > > wasn't designed to have the adapter be used before i2c_add_adapter()
> > > is called. I guess it might work in this case since you provide your
> > > own lock vfuncs.
> > > 
> > > I think someone should fix the i2c core to split i2c_add_adapter()
> > > & co. into init and register phases. Cc:ing i2c folks...
> > 
> > As you've seen, I sent the patches to split i2c_add_adapter() to allow for
> > us to call i2c_init_adapter() here instead. It still requires the same
> > basic review that this init (the same as above) is sufficient for using
> > i2c_transfer().
> > 
> > I would like to get the regression fix completed without much futher
> > ado - in particular, not depending upon landing an external patch.
> 
> What regression is this?

It all started from the unclaimed mmio access across suspend, which
stems from never having touched a context before suspend. That in turn
meant touching the delayed rps initialisation which ended up with Daniel
saying that the entire init chain needs to be unravelled. Snowball.
-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