diff mbox

[v2,1/3] drm: Add callbacks for late registering

Message ID 1466501496-18852-2-git-send-email-benjamin.gaignard@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Benjamin Gaignard June 21, 2016, 9:31 a.m. UTC 
Like what has been done for connectors add callbacks on encoder,
crtc and plane to let driver do actions after drm device registration.

Correspondingly, add callbacks called before unregister drm device.

version 2:
add drm_modeset_register_all() and drm_modeset_unregister_all()
to centralize all calls

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/gpu/drm/drm_crtc.c | 122 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c  |   4 +-
 include/drm/drm_crtc.h     |  79 +++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+), 2 deletions(-)

Comments

Chris Wilson June 21, 2016, 10:31 a.m. UTC | #1 
On Tue, Jun 21, 2016 at 11:31:34AM +0200, Benjamin Gaignard wrote:
> Like what has been done for connectors add callbacks on encoder,
> crtc and plane to let driver do actions after drm device registration.
> 
> Correspondingly, add callbacks called before unregister drm device.
> 
> version 2:
> add drm_modeset_register_all() and drm_modeset_unregister_all()
> to centralize all calls
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
>  drivers/gpu/drm/drm_crtc.c | 122 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_drv.c  |   4 +-
>  include/drm/drm_crtc.h     |  79 +++++++++++++++++++++++++++++
>  3 files changed, 203 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e7c862b..b393feb 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -608,6 +608,31 @@ static unsigned int drm_num_crtcs(struct drm_device *dev)
>  	return num;
>  }
>  
> +static int drm_crtc_register_all(struct drm_device *dev)
> +{
> +	struct drm_crtc *crtc;
> +	int ret;
> +
> +	drm_for_each_crtc(crtc, dev) {
> +		if (crtc->funcs->late_register)
> +			ret = crtc->funcs->late_register(crtc);
> +		if (ret)

ret is uninitialised.

It is a good question (probably for another series) on what exactly to
do on registration failure? At the very least we need to unwind on the
error path, or we just ignore errors (other than a DRM_ERROR to
userspace explaining why one object is not available, but otherwise let
the driver load).

> +int drm_modeset_register_all(struct drm_device *dev)
> +{
> +	int ret;
> +
> +	ret = drm_plane_register_all(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_crtc_register_all(dev);
> +	if  (ret)
> +		return ret;
> +
> +	ret = drm_encoder_register_all(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_connector_register_all(dev);

Might as well continue on with 

ret = <>
if (ret)
	return ret;

for a consistent style. Along with the comment about how should we be
handling errors? If we report an error, everything up to that point
should be unwound.

> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_modeset_register_all);
> +
> +/**
> + * drm_modeset_unregister_all - do early unregistration
> + * @dev: drm device
> + *
> + * This function call early_unregister callbakc for all planes,
> + * crtcs, encoders and connectors
> + */
> +void drm_modeset_unregister_all(struct drm_device *dev)
> +{
> +	drm_plane_unregister_all(dev);
> +	drm_crtc_unregister_all(dev);
> +	drm_encoder_unregister_all(dev);
> +	drm_connector_unregister_all(dev);

Unregister should be in the opposite order.

> +}
> +EXPORT_SYMBOL(drm_modeset_unregister_all);

I think the plan is not to export these symbols,
-Chris
Benjamin Gaignard June 21, 2016, 11:09 a.m. UTC | #2 
2016-06-21 12:31 GMT+02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Jun 21, 2016 at 11:31:34AM +0200, Benjamin Gaignard wrote:
>> Like what has been done for connectors add callbacks on encoder,
>> crtc and plane to let driver do actions after drm device registration.
>>
>> Correspondingly, add callbacks called before unregister drm device.
>>
>> version 2:
>> add drm_modeset_register_all() and drm_modeset_unregister_all()
>> to centralize all calls
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 122 +++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_drv.c  |   4 +-
>>  include/drm/drm_crtc.h     |  79 +++++++++++++++++++++++++++++
>>  3 files changed, 203 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index e7c862b..b393feb 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -608,6 +608,31 @@ static unsigned int drm_num_crtcs(struct drm_device *dev)
>>       return num;
>>  }
>>
>> +static int drm_crtc_register_all(struct drm_device *dev)
>> +{
>> +     struct drm_crtc *crtc;
>> +     int ret;
>> +
>> +     drm_for_each_crtc(crtc, dev) {
>> +             if (crtc->funcs->late_register)
>> +                     ret = crtc->funcs->late_register(crtc);
>> +             if (ret)
>
> ret is uninitialised.

OK I fix that for v3

>
> It is a good question (probably for another series) on what exactly to
> do on registration failure? At the very least we need to unwind on the
> error path, or we just ignore errors (other than a DRM_ERROR to
> userspace explaining why one object is not available, but otherwise let
> the driver load).
>
>> +int drm_modeset_register_all(struct drm_device *dev)
>> +{
>> +     int ret;
>> +
>> +     ret = drm_plane_register_all(dev);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = drm_crtc_register_all(dev);
>> +     if  (ret)
>> +             return ret;
>> +
>> +     ret = drm_encoder_register_all(dev);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = drm_connector_register_all(dev);
>
> Might as well continue on with
>
> ret = <>
> if (ret)
>         return ret;
>
> for a consistent style. Along with the comment about how should we be
> handling errors? If we report an error, everything up to that point
> should be unwound.
>

OK I will add goto for each case.

>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(drm_modeset_register_all);
>> +
>> +/**
>> + * drm_modeset_unregister_all - do early unregistration
>> + * @dev: drm device
>> + *
>> + * This function call early_unregister callbakc for all planes,
>> + * crtcs, encoders and connectors
>> + */
>> +void drm_modeset_unregister_all(struct drm_device *dev)
>> +{
>> +     drm_plane_unregister_all(dev);
>> +     drm_crtc_unregister_all(dev);
>> +     drm_encoder_unregister_all(dev);
>> +     drm_connector_unregister_all(dev);
>
> Unregister should be in the opposite order.

OK for v3

>> +}
>> +EXPORT_SYMBOL(drm_modeset_unregister_all);
>
> I think the plan is not to export these symbols,
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e7c862b..b393feb 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -608,6 +608,31 @@  static unsigned int drm_num_crtcs(struct drm_device *dev)
 	return num;
 }
 
+static int drm_crtc_register_all(struct drm_device *dev)
+{
+	struct drm_crtc *crtc;
+	int ret;
+
+	drm_for_each_crtc(crtc, dev) {
+		if (crtc->funcs->late_register)
+			ret = crtc->funcs->late_register(crtc);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void drm_crtc_unregister_all(struct drm_device *dev)
+{
+	struct drm_crtc *crtc;
+
+	drm_for_each_crtc(crtc, dev) {
+		if (crtc->funcs->early_unregister)
+			crtc->funcs->early_unregister(crtc);
+	}
+}
+
 /**
  * drm_crtc_init_with_planes - Initialise a new CRTC object with
  *    specified primary and cursor planes.
@@ -1102,6 +1127,31 @@  void drm_connector_unregister_all(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_connector_unregister_all);
 
+static int drm_encoder_register_all(struct drm_device *dev)
+{
+	struct drm_encoder *encoder;
+	int ret;
+
+	drm_for_each_encoder(encoder, dev) {
+		if (encoder->funcs->late_register)
+			ret = encoder->funcs->late_register(encoder);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void drm_encoder_unregister_all(struct drm_device *dev)
+{
+	struct drm_encoder *encoder;
+
+	drm_for_each_encoder(encoder, dev) {
+		if (encoder->funcs->early_unregister)
+			encoder->funcs->early_unregister(encoder);
+	}
+}
+
 /**
  * drm_encoder_init - Init a preallocated encoder
  * @dev: drm device
@@ -1290,6 +1340,31 @@  int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 }
 EXPORT_SYMBOL(drm_universal_plane_init);
 
+static int drm_plane_register_all(struct drm_device *dev)
+{
+	struct drm_plane *plane;
+	int ret;
+
+	drm_for_each_plane(plane, dev) {
+		if (plane->funcs->late_register)
+			ret = plane->funcs->late_register(plane);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void drm_plane_unregister_all(struct drm_device *dev)
+{
+	struct drm_plane *plane;
+
+	drm_for_each_plane(plane, dev) {
+		if (plane->funcs->early_unregister)
+			plane->funcs->early_unregister(plane);
+	}
+}
+
 /**
  * drm_plane_init - Initialize a legacy plane
  * @dev: DRM device
@@ -1412,6 +1487,53 @@  void drm_plane_force_disable(struct drm_plane *plane)
 }
 EXPORT_SYMBOL(drm_plane_force_disable);
 
+/**
+ * drm_modeset_register_all - do late registration
+ * @dev: drm device
+ *
+ * This function call late_register callback for all planes,
+ * crcts, encoders and connectors
+ *
+ * Returns:
+ * Zero on success, erro code on failure
+ */
+int drm_modeset_register_all(struct drm_device *dev)
+{
+	int ret;
+
+	ret = drm_plane_register_all(dev);
+	if (ret)
+		return ret;
+
+	ret = drm_crtc_register_all(dev);
+	if  (ret)
+		return ret;
+
+	ret = drm_encoder_register_all(dev);
+	if (ret)
+		return ret;
+
+	ret = drm_connector_register_all(dev);
+	return ret;
+}
+EXPORT_SYMBOL(drm_modeset_register_all);
+
+/**
+ * drm_modeset_unregister_all - do early unregistration
+ * @dev: drm device
+ *
+ * This function call early_unregister callbakc for all planes,
+ * crtcs, encoders and connectors
+ */
+void drm_modeset_unregister_all(struct drm_device *dev)
+{
+	drm_plane_unregister_all(dev);
+	drm_crtc_unregister_all(dev);
+	drm_encoder_unregister_all(dev);
+	drm_connector_unregister_all(dev);
+}
+EXPORT_SYMBOL(drm_modeset_unregister_all);
+
 static int drm_mode_create_standard_properties(struct drm_device *dev)
 {
 	struct drm_property *prop;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index c7101c0..53eadca 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -688,7 +688,7 @@  int drm_dev_register(struct drm_device *dev, unsigned long flags)
 	}
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		drm_connector_register_all(dev);
+		drm_modeset_register_all(dev);
 
 	ret = 0;
 	goto out_unlock;
@@ -721,7 +721,7 @@  void drm_dev_unregister(struct drm_device *dev)
 	drm_lastclose(dev);
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		drm_connector_unregister_all(dev);
+		drm_modeset_unregister_all(dev);
 
 	if (dev->driver->unload)
 		dev->driver->unload(dev);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c273497..9b681f6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -704,6 +704,32 @@  struct drm_crtc_funcs {
 				   const struct drm_crtc_state *state,
 				   struct drm_property *property,
 				   uint64_t *val);
+
+	/**
+	 * @late_register:
+	 *
+	 * This optional hook can be used to register additional userspace
+	 * interfaces attached to the crtc like debugfs interfaces.
+	 * It is called late in the driver load sequence from drm_dev_register().
+	 * 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_crtc *crtc);
+
+	/**
+	 * @early_unregister:
+	 *
+	 * This optional hook should be used to unregister the additional
+	 * userspace interfaces attached to the crtc from
+	 * late_unregister(). It is called from drm_dev_unregister(),
+	 * early in the driver unload sequence to disable userspace access
+	 * before data structures are torndown.
+	 */
+	void (*early_unregister)(struct drm_crtc *crtc);
 };
 
 /**
@@ -1127,6 +1153,32 @@  struct drm_encoder_funcs {
 	 * hotplugged in DRM.
 	 */
 	void (*destroy)(struct drm_encoder *encoder);
+
+	/**
+	 * @late_register:
+	 *
+	 * This optional hook can be used to register additional userspace
+	 * interfaces attached to the encoder like debugfs interfaces.
+	 * It is called late in the driver load sequence from drm_dev_register().
+	 * 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_encoder *encoder);
+
+	/**
+	 * @early_unregister:
+	 *
+	 * This optional hook should be used to unregister the additional
+	 * userspace interfaces attached to the encoder from
+	 * late_unregister(). It is called from drm_dev_unregister(),
+	 * early in the driver unload sequence to disable userspace access
+	 * before data structures are torndown.
+	 */
+	void (*early_unregister)(struct drm_encoder *encoder);
 };
 
 #define DRM_CONNECTOR_MAX_ENCODER 3
@@ -1570,6 +1622,31 @@  struct drm_plane_funcs {
 				   const struct drm_plane_state *state,
 				   struct drm_property *property,
 				   uint64_t *val);
+	/**
+	 * @late_register:
+	 *
+	 * This optional hook can be used to register additional userspace
+	 * interfaces attached to the plane like debugfs interfaces.
+	 * It is called late in the driver load sequence from drm_dev_register().
+	 * 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_plane *plane);
+
+	/**
+	 * @early_unregister:
+	 *
+	 * This optional hook should be used to unregister the additional
+	 * userspace interfaces attached to the plane from
+	 * late_unregister(). It is called from drm_dev_unregister(),
+	 * early in the driver unload sequence to disable userspace access
+	 * before data structures are torndown.
+	 */
+	void (*early_unregister)(struct drm_plane *plane);
 };
 
 enum drm_plane_type {
@@ -2514,6 +2591,8 @@  static inline unsigned drm_connector_index(struct drm_connector *connector)
 /* helpers to {un}register all connectors from sysfs for device */
 extern int drm_connector_register_all(struct drm_device *dev);
 extern void drm_connector_unregister_all(struct drm_device *dev);
+extern int drm_modeset_register_all(struct drm_device *dev);
+extern void drm_modeset_unregister_all(struct drm_device *dev);
 
 extern int drm_bridge_add(struct drm_bridge *bridge);
 extern void drm_bridge_remove(struct drm_bridge *bridge);