[PATCH 1/6] drm/fb: fix missing /** in kerneldoc comment.

[PATCH 1/6] drm/fb: fix missing /** in kerneldoc comment.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 include/drm/drm_crtc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 297e527..6279989 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2606,7 +2606,7 @@ static inline uint32_t drm_color_lut_extract(uint32_t user_input,
 	return clamp_val(val, 0, max);
 }
 
-/*
+/**
  * drm_framebuffer_reference - incr the fb refcnt
  * @fb: framebuffer
  *
-- 
2.5.5

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

[PATCH 2/6] drm/modes: add connector reference counting. (v2)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This uses the previous changes to add reference counts
to drm connector objects.

v2: move fbdev changes to their own patch.
add some kerneldoc

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++++++--
 drivers/gpu/drm/drm_crtc.c   | 28 ++++++++++++++++++++++++----
 include/drm/drm_crtc.h       | 32 +++++++++++++++++++++++++++++++-
 3 files changed, 72 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8ee1db8..9d5e3c8 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -154,6 +154,7 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 						       state->connector_states[i]);
 		state->connectors[i] = NULL;
 		state->connector_states[i] = NULL;
+		drm_connector_unreference(connector);
 	}
 
 	for (i = 0; i < config->num_crtc; i++) {
@@ -924,6 +925,7 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
 	if (!connector_state)
 		return ERR_PTR(-ENOMEM);
 
+	drm_connector_reference(connector);
 	state->connector_states[index] = connector_state;
 	state->connectors[index] = connector;
 	connector_state->state = state;
@@ -1614,12 +1616,19 @@ retry:
 		}
 
 		obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
-		if (!obj || !obj->properties) {
+		if (!obj) {
+			ret = -ENOENT;
+			goto out;
+		}
+
+		if (!obj->properties) {
+			drm_mode_object_unreference(obj);
 			ret = -ENOENT;
 			goto out;
 		}
 
 		if (get_user(count_props, count_props_ptr + copied_objs)) {
+			drm_mode_object_unreference(obj);
 			ret = -EFAULT;
 			goto out;
 		}
@@ -1632,12 +1641,14 @@ retry:
 			struct drm_property *prop;
 
 			if (get_user(prop_id, props_ptr + copied_props)) {
+				drm_mode_object_unreference(obj);
 				ret = -EFAULT;
 				goto out;
 			}
 
 			prop = drm_property_find(dev, prop_id);
 			if (!prop) {
+				drm_mode_object_unreference(obj);
 				ret = -ENOENT;
 				goto out;
 			}
@@ -1645,13 +1656,16 @@ retry:
 			if (copy_from_user(&prop_value,
 					   prop_values_ptr + copied_props,
 					   sizeof(prop_value))) {
+				drm_mode_object_unreference(obj);
 				ret = -EFAULT;
 				goto out;
 			}
 
 			ret = atomic_set_prop(state, obj, prop, prop_value);
-			if (ret)
+			if (ret) {
+				drm_mode_object_unreference(obj);
 				goto out;
+			}
 
 			copied_props++;
 		}
@@ -1662,6 +1676,7 @@ retry:
 			plane_mask |= (1 << drm_plane_index(plane));
 			plane->old_fb = plane->fb;
 		}
+		drm_mode_object_unreference(obj);
 	}
 
 	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4e5b015..a78e202 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -862,6 +862,16 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
 		      mode->interlace ?  " interlaced" : "");
 }
 
+static void drm_connector_free(struct kref *kref)
+{
+	struct drm_connector *connector =
+		container_of(kref, struct drm_connector, base.refcount);
+	struct drm_device *dev = connector->dev;
+
+	drm_mode_object_unregister(dev, &connector->base);
+	connector->funcs->destroy(connector);
+}
+
 /**
  * drm_connector_init - Init a preallocated connector
  * @dev: DRM device
@@ -887,7 +897,9 @@ int drm_connector_init(struct drm_device *dev,
 
 	drm_modeset_lock_all(dev);
 
-	ret = drm_mode_object_get_reg(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR, false, NULL);
+	ret = drm_mode_object_get_reg(dev, &connector->base,
+				      DRM_MODE_OBJECT_CONNECTOR,
+				      false, drm_connector_free);
 	if (ret)
 		goto out_unlock;
 
@@ -2147,7 +2159,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 
 	mutex_lock(&dev->mode_config.mutex);
 
-	connector = drm_connector_find(dev, out_resp->connector_id);
+	connector = drm_connector_lookup(dev, out_resp->connector_id);
 	if (!connector) {
 		ret = -ENOENT;
 		goto out_unlock;
@@ -2231,6 +2243,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 out:
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 
+	drm_connector_unreference(connector);
 out_unlock:
 	mutex_unlock(&dev->mode_config.mutex);
 
@@ -2875,13 +2888,14 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		}
 
 		for (i = 0; i < crtc_req->count_connectors; i++) {
+			connector_set[i] = NULL;
 			set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
 			if (get_user(out_id, &set_connectors_ptr[i])) {
 				ret = -EFAULT;
 				goto out;
 			}
 
-			connector = drm_connector_find(dev, out_id);
+			connector = drm_connector_lookup(dev, out_id);
 			if (!connector) {
 				DRM_DEBUG_KMS("Connector id %d unknown\n",
 						out_id);
@@ -2909,6 +2923,12 @@ out:
 	if (fb)
 		drm_framebuffer_unreference(fb);
 
+	if (connector_set) {
+		for (i = 0; i < crtc_req->count_connectors; i++) {
+			if (connector_set[i])
+				drm_connector_unreference(connector_set[i]);
+		}
+	}
 	kfree(connector_set);
 	drm_mode_destroy(dev, mode);
 	drm_modeset_unlock_all(dev);
@@ -4999,7 +5019,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 	property = obj_to_property(prop_obj);
 
 	if (!drm_property_change_valid_get(property, arg->value, &ref))
-		goto out;
+		goto out_unref;
 
 	switch (arg_obj->type) {
 	case DRM_MODE_OBJECT_CONNECTOR:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 6279989..68100b8 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2571,7 +2571,15 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
 	return mo ? obj_to_encoder(mo) : NULL;
 }
 
-static inline struct drm_connector *drm_connector_find(struct drm_device *dev,
+/**
+ * drm_connector_lookup - lookup connector object
+ * @dev: DRM device
+ * @id: connector object id
+ *
+ * This function looks up the connector object specified by id
+ * add takes a reference to it.
+ */
+static inline struct drm_connector *drm_connector_lookup(struct drm_device *dev,
 		uint32_t id)
 {
 	struct drm_mode_object *mo;
@@ -2639,6 +2647,28 @@ static inline uint32_t drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
 	return atomic_read(&fb->base.refcount.refcount);
 }
 
+/**
+ * drm_connector_reference - incr the connector refcnt
+ * @connector: connector
+ *
+ * This function increments the connector's refcount.
+ */
+static inline void drm_connector_reference(struct drm_connector *connector)
+{
+	drm_mode_object_reference(&connector->base);
+}
+
+/**
+ * drm_connector_unreference - unref a connector
+ * @connector: connector to unref
+ *
+ * This function decrements the connector's refcount and frees it if it drops to zero.
+ */
+static inline void drm_connector_unreference(struct drm_connector *connector)
+{
+	drm_mode_object_unreference(&connector->base);
+}
+
 /* Plane list iterator for legacy (overlay only) planes. */
 #define drm_for_each_legacy_plane(plane, dev) \
 	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) \
-- 
2.5.5

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

[PATCH 3/6] drm/fb_helper: add connector reference counting. (v2)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This takes a reference count when fbdev adds the connector,
and drops it when it removes the connector.

It also drops the now unneeded code to find connectors
and remove the from the modeset as they are reference counted.

v2: drop references when removing all connectors at end.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 38 +++++---------------------------------
 1 file changed, 5 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 855108e..19a7a71 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -153,40 +153,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
 	if (!fb_helper_connector)
 		return -ENOMEM;
 
+	drm_connector_reference(connector);
 	fb_helper_connector->connector = connector;
 	fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
 	return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
 
-static void remove_from_modeset(struct drm_mode_set *set,
-		struct drm_connector *connector)
-{
-	int i, j;
-
-	for (i = 0; i < set->num_connectors; i++) {
-		if (set->connectors[i] == connector)
-			break;
-	}
-
-	if (i == set->num_connectors)
-		return;
-
-	for (j = i + 1; j < set->num_connectors; j++) {
-		set->connectors[j - 1] = set->connectors[j];
-	}
-	set->num_connectors--;
-
-	/*
-	 * TODO maybe need to makes sure we set it back to !=NULL somewhere?
-	 */
-	if (set->num_connectors == 0) {
-		set->fb = NULL;
-		drm_mode_destroy(connector->dev, set->mode);
-		set->mode = NULL;
-	}
-}
-
 int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 				       struct drm_connector *connector)
 {
@@ -206,6 +179,7 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	if (i == fb_helper->connector_count)
 		return -EINVAL;
 	fb_helper_connector = fb_helper->connector_info[i];
+	drm_connector_unreference(fb_helper_connector->connector);
 
 	for (j = i + 1; j < fb_helper->connector_count; j++) {
 		fb_helper->connector_info[j - 1] = fb_helper->connector_info[j];
@@ -213,10 +187,6 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	fb_helper->connector_count--;
 	kfree(fb_helper_connector);
 
-	/* also cleanup dangling references to the connector: */
-	for (i = 0; i < fb_helper->crtc_count; i++)
-		remove_from_modeset(&fb_helper->crtc_info[i].mode_set, connector);
-
 	return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
@@ -626,8 +596,10 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 {
 	int i;
 
-	for (i = 0; i < helper->connector_count; i++)
+	for (i = 0; i < helper->connector_count; i++) {
+		drm_connector_unreference(helper->connector_info[i]->connector);
 		kfree(helper->connector_info[i]);
+	}
 	kfree(helper->connector_info);
 	for (i = 0; i < helper->crtc_count; i++) {
 		kfree(helper->crtc_info[i].mode_set.connectors);
-- 
2.5.5

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

Re: [Intel-gfx] [PATCH 3/6] drm/fb_helper: add connector reference counting. (v2)

[Daniel Vetter]

On Tue, May 03, 2016 at 02:28:23PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This takes a reference count when fbdev adds the connector,
> and drops it when it removes the connector.
> 
> It also drops the now unneeded code to find connectors
> and remove the from the modeset as they are reference counted.
> 
> v2: drop references when removing all connectors at end.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Yeah I think this looks good now.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 38 +++++---------------------------------
>  1 file changed, 5 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 855108e..19a7a71 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -153,40 +153,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
>  	if (!fb_helper_connector)
>  		return -ENOMEM;
>  
> +	drm_connector_reference(connector);
>  	fb_helper_connector->connector = connector;
>  	fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
>  
> -static void remove_from_modeset(struct drm_mode_set *set,
> -		struct drm_connector *connector)
> -{
> -	int i, j;
> -
> -	for (i = 0; i < set->num_connectors; i++) {
> -		if (set->connectors[i] == connector)
> -			break;
> -	}
> -
> -	if (i == set->num_connectors)
> -		return;
> -
> -	for (j = i + 1; j < set->num_connectors; j++) {
> -		set->connectors[j - 1] = set->connectors[j];
> -	}
> -	set->num_connectors--;
> -
> -	/*
> -	 * TODO maybe need to makes sure we set it back to !=NULL somewhere?
> -	 */
> -	if (set->num_connectors == 0) {
> -		set->fb = NULL;
> -		drm_mode_destroy(connector->dev, set->mode);
> -		set->mode = NULL;
> -	}
> -}
> -
>  int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  				       struct drm_connector *connector)
>  {
> @@ -206,6 +179,7 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  	if (i == fb_helper->connector_count)
>  		return -EINVAL;
>  	fb_helper_connector = fb_helper->connector_info[i];
> +	drm_connector_unreference(fb_helper_connector->connector);
>  
>  	for (j = i + 1; j < fb_helper->connector_count; j++) {
>  		fb_helper->connector_info[j - 1] = fb_helper->connector_info[j];
> @@ -213,10 +187,6 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  	fb_helper->connector_count--;
>  	kfree(fb_helper_connector);
>  
> -	/* also cleanup dangling references to the connector: */
> -	for (i = 0; i < fb_helper->crtc_count; i++)
> -		remove_from_modeset(&fb_helper->crtc_info[i].mode_set, connector);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
> @@ -626,8 +596,10 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
>  {
>  	int i;
>  
> -	for (i = 0; i < helper->connector_count; i++)
> +	for (i = 0; i < helper->connector_count; i++) {
> +		drm_connector_unreference(helper->connector_info[i]->connector);
>  		kfree(helper->connector_info[i]);
> +	}
>  	kfree(helper->connector_info);
>  	for (i = 0; i < helper->crtc_count; i++) {
>  		kfree(helper->crtc_info[i].mode_set.connectors);
> -- 
> 2.5.5
> 
> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/crtc: take references to connectors used in a modeset. (v2)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This just takes a reference on the connector when we set a mode
in the non-atomic paths.

v2: Follow Daniel Stone's suggestions on when to take/drop
references.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_crtc_helper.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 66ca313..f47a252 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -456,6 +456,9 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
 			 * between them is henceforth no longer available.
 			 */
 			connector->dpms = DRM_MODE_DPMS_OFF;
+
+			/* we keep a reference while the encoder is bound */
+			drm_connector_unreference(connector);
 		}
 	}
 
@@ -606,6 +609,11 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 		mode_changed = true;
 	}
 
+	/* take a reference on all connectors in set */
+	for (ro = 0; ro < set->num_connectors; ro++) {
+		drm_connector_reference(set->connectors[ro]);
+	}
+
 	/* a) traverse passed in connector list and get encoders for them */
 	count = 0;
 	drm_for_each_connector(connector, dev) {
@@ -724,6 +732,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 		}
 	}
 
+	/* after fail drop reference on all connectors in save set */
+	count = 0;
+	drm_for_each_connector(connector, dev) {
+		drm_connector_unreference(&save_connectors[count++]);
+	}
+
 	kfree(save_connectors);
 	kfree(save_encoders);
 	return 0;
@@ -740,6 +754,11 @@ fail:
 		*connector = save_connectors[count++];
 	}
 
+	/* after fail drop reference on all connectors in set */
+	for (ro = 0; ro < set->num_connectors; ro++) {
+		drm_connector_unreference(set->connectors[ro]);
+	}
+
 	/* Try to restore the config */
 	if (mode_changed &&
 	    !drm_crtc_helper_set_mode(save_set.crtc, save_set.mode, save_set.x,
-- 
2.5.5

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

Re: [PATCH 4/6] drm/crtc: take references to connectors used in a modeset. (v2)

[Daniel Vetter]

On Tue, May 03, 2016 at 02:28:24PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This just takes a reference on the connector when we set a mode
> in the non-atomic paths.
> 
> v2: Follow Daniel Stone's suggestions on when to take/drop
> references.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

I've already embarrassed myself once with a bonghits r-b, let's try this
again:

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

> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 66ca313..f47a252 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -456,6 +456,9 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
>  			 * between them is henceforth no longer available.
>  			 */
>  			connector->dpms = DRM_MODE_DPMS_OFF;
> +
> +			/* we keep a reference while the encoder is bound */
> +			drm_connector_unreference(connector);
>  		}
>  	}
>  
> @@ -606,6 +609,11 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
>  		mode_changed = true;
>  	}
>  
> +	/* take a reference on all connectors in set */
> +	for (ro = 0; ro < set->num_connectors; ro++) {
> +		drm_connector_reference(set->connectors[ro]);
> +	}
> +
>  	/* a) traverse passed in connector list and get encoders for them */
>  	count = 0;
>  	drm_for_each_connector(connector, dev) {
> @@ -724,6 +732,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
>  		}
>  	}
>  
> +	/* after fail drop reference on all connectors in save set */
> +	count = 0;
> +	drm_for_each_connector(connector, dev) {
> +		drm_connector_unreference(&save_connectors[count++]);
> +	}
> +
>  	kfree(save_connectors);
>  	kfree(save_encoders);
>  	return 0;
> @@ -740,6 +754,11 @@ fail:
>  		*connector = save_connectors[count++];
>  	}
>  
> +	/* after fail drop reference on all connectors in set */
> +	for (ro = 0; ro < set->num_connectors; ro++) {
> +		drm_connector_unreference(set->connectors[ro]);
> +	}
> +
>  	/* Try to restore the config */
>  	if (mode_changed &&
>  	    !drm_crtc_helper_set_mode(save_set.crtc, save_set.mode, save_set.x,
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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 5/6] drm/atomic: use connector references (v2)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Take a reference when setting a crtc on a connecter,
also take one when duplicating if a crtc is set,
and drop one on destroy if a crtc is set.

v2: take Daniel Stone's advice and simplify the
ref/unref dances, also take care of NULL as connector
to state reset.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_atomic.c        | 4 ++++
 drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9d5e3c8..91cae88 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1160,6 +1160,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 {
 	struct drm_crtc_state *crtc_state;
 
+	if (crtc)
+		drm_connector_reference(conn_state->connector);
 	if (conn_state->crtc && conn_state->crtc != crtc) {
 		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
 								conn_state->crtc);
@@ -1177,6 +1179,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 			1 << drm_connector_index(conn_state->connector);
 	}
 
+	if (conn_state->crtc)
+		drm_connector_unreference(conn_state->connector);
 	conn_state->crtc = crtc;
 
 	if (crtc)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d25abce..f3e2642 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2762,6 +2762,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
 					    struct drm_connector_state *state)
 {
 	memcpy(state, connector->state, sizeof(*state));
+	if (state->crtc)
+		drm_connector_reference(connector);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
 
@@ -2889,6 +2891,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
 	 * state will automatically do the right thing if code is ever added
 	 * to this function.
 	 */
+	if (connector && state->crtc) {
+		drm_connector_unreference(state->connector);
+	}
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
 
-- 
2.5.5

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

Re: [PATCH 5/6] drm/atomic: use connector references (v2)

[Daniel Vetter]

On Tue, May 03, 2016 at 02:28:25PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Take a reference when setting a crtc on a connecter,
> also take one when duplicating if a crtc is set,
> and drop one on destroy if a crtc is set.
> 
> v2: take Daniel Stone's advice and simplify the
> ref/unref dances, also take care of NULL as connector
> to state reset.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 4 ++++
>  drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9d5e3c8..91cae88 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1160,6 +1160,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  {
>  	struct drm_crtc_state *crtc_state;
>  
> +	if (crtc)
> +		drm_connector_reference(conn_state->connector);
>  	if (conn_state->crtc && conn_state->crtc != crtc) {
>  		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
>  								conn_state->crtc);
> @@ -1177,6 +1179,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  			1 << drm_connector_index(conn_state->connector);
>  	}
>  
> +	if (conn_state->crtc)
> +		drm_connector_unreference(conn_state->connector);
>  	conn_state->crtc = crtc;
>  
>  	if (crtc)
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d25abce..f3e2642 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2762,6 +2762,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
>  					    struct drm_connector_state *state)
>  {
>  	memcpy(state, connector->state, sizeof(*state));
> +	if (state->crtc)
> +		drm_connector_reference(connector);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>  
> @@ -2889,6 +2891,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>  	 * state will automatically do the right thing if code is ever added
>  	 * to this function.
>  	 */
> +	if (connector && state->crtc) {
> +		drm_connector_unreference(state->connector);
> +	}

Bikeshed: no need for {} and you don't need to check that connector is
NULL either. Tbh all the destroy_state helpers don't really need their
object argument, only the state. Cleaning that up is somewhere on my todo.

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

>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>  
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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 5/6] drm/atomic: use connector references (v2)

[Dave Airlie]

>>        */
>> +     if (connector && state->crtc) {
>> +             drm_connector_unreference(state->connector);
>> +     }
>
> Bikeshed: no need for {} and you don't need to check that connector is
> NULL either. Tbh all the destroy_state helpers don't really need their
> object argument, only the state. Cleaning that up is somewhere on my todo.

Yes you really do need the connector, lost a boot to an oops,

drm_atomic_state_default_clear does some funky stuff that can possibly go away
after this, but I'm not sure.

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

Re: [PATCH 5/6] drm/atomic: use connector references (v2)

[Daniel Vetter]

On Tue, May 3, 2016 at 12:09 PM, Dave Airlie <airlied@gmail.com> wrote:
>>>        */
>>> +     if (connector && state->crtc) {
>>> +             drm_connector_unreference(state->connector);
>>> +     }
>>
>> Bikeshed: no need for {} and you don't need to check that connector is
>> NULL either. Tbh all the destroy_state helpers don't really need their
>> object argument, only the state. Cleaning that up is somewhere on my todo.
>
> Yes you really do need the connector, lost a boot to an oops,
>
> drm_atomic_state_default_clear does some funky stuff that can possibly go away
> after this, but I'm not sure.

Surprising, didn't expect that. Do you still have the backtrace
somewhere? I really wonder what goes boom with that one ...

What we might need is check for state->connector (instead of
connector), but that being NULL for a valid state is also a bit a bug.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/atomic: use connector references (v2)

[Daniel Stone]

Hi,

On 3 May 2016 at 05:28, Dave Airlie <airlied@gmail.com> wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> Take a reference when setting a crtc on a connecter,
> also take one when duplicating if a crtc is set,
> and drop one on destroy if a crtc is set.
>
> v2: take Daniel Stone's advice and simplify the
> ref/unref dances, also take care of NULL as connector
> to state reset.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>

I expect we'll still end up with all kinds of comedy races here, but
on the grounds that it's unambiguously an improvement on what came
before, and I can't see anything obviously wrong, for patches 1-5:
Reviewed-by: Daniel Stone <daniels@collabora.com>

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

[PATCH 6/6] drm/i915/mst: use reference counted connectors. (v3)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Don't just free the connector when we get the destroy callback.

Drop a reference to it, and set it's mst_port to NULL so
no more mst work is done on it.

v2: core mst accepts NULLs fine. Cleanup EDID code properly.
v3: drop the extra reference we were taking.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 43 +++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_drv.h    |  2 +-
 2 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 94b4e83..b6bf7fd 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -109,7 +109,7 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
 
-	drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->port);
+	drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->connector->port);
 
 	ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
 	if (ret) {
@@ -134,10 +134,11 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder)
 	/* and this can also fail */
 	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
 
-	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->port);
+	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->connector->port);
 
 	intel_dp->active_mst_links--;
-	intel_mst->port = NULL;
+
+	intel_mst->connector = NULL;
 	if (intel_dp->active_mst_links == 0) {
 		intel_dig_port->base.post_disable(&intel_dig_port->base);
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
@@ -177,7 +178,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
 	found->encoder = encoder;
 
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
-	intel_mst->port = found->port;
+
+	intel_mst->connector = found;
 
 	if (intel_dp->active_mst_links == 0) {
 		intel_prepare_ddi_buffer(&intel_dig_port->base);
@@ -195,7 +197,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
 	}
 
 	ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
-				       intel_mst->port,
+				       intel_mst->connector->port,
 				       intel_crtc->config->pbn, &slots);
 	if (ret == false) {
 		DRM_ERROR("failed to allocate vcpi\n");
@@ -244,7 +246,7 @@ static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder *encoder,
 {
 	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
 	*pipe = intel_mst->pipe;
-	if (intel_mst->port)
+	if (intel_mst->connector)
 		return true;
 	return false;
 }
@@ -308,10 +310,11 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
 	struct edid *edid;
 	int ret;
 
-	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
-	if (!edid)
-		return 0;
+	if (!intel_dp) {
+		return intel_connector_update_modes(connector, NULL);
+	}
 
+	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
 	ret = intel_connector_update_modes(connector, edid);
 	kfree(edid);
 
@@ -324,6 +327,8 @@ intel_dp_mst_detect(struct drm_connector *connector, bool force)
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct intel_dp *intel_dp = intel_connector->mst_port;
 
+	if (!intel_dp)
+		return connector_status_disconnected;
 	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);
 }
 
@@ -389,6 +394,8 @@ static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *c
 	struct intel_dp *intel_dp = intel_connector->mst_port;
 	struct intel_crtc *crtc = to_intel_crtc(state->crtc);
 
+	if (!intel_dp)
+		return NULL;
 	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
 }
 
@@ -396,6 +403,8 @@ static struct drm_encoder *intel_mst_best_encoder(struct drm_connector *connecto
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct intel_dp *intel_dp = intel_connector->mst_port;
+	if (!intel_dp)
+		return NULL;
 	return &intel_dp->mst_encoders[0]->base.base;
 }
 
@@ -506,23 +515,11 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 
 	/* need to nuke the connector */
 	drm_modeset_lock_all(dev);
-	if (connector->state->crtc) {
-		struct drm_mode_set set;
-		int ret;
-
-		memset(&set, 0, sizeof(set));
-		set.crtc = connector->state->crtc,
-
-		ret = drm_atomic_helper_set_config(&set);
-
-		WARN(ret, "Disabling mst crtc failed with %i\n", ret);
-	}
-
 	intel_connector_remove_from_fbdev(intel_connector);
-	drm_connector_cleanup(connector);
+	intel_connector->mst_port = NULL;
 	drm_modeset_unlock_all(dev);
 
-	kfree(intel_connector);
+	drm_connector_unreference(&intel_connector->base);
 	DRM_DEBUG_KMS("\n");
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e0fcfa1..288135de 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -878,7 +878,7 @@ struct intel_dp_mst_encoder {
 	struct intel_encoder base;
 	enum pipe pipe;
 	struct intel_digital_port *primary;
-	void *port; /* store this opaque as its illegal to dereference it */
+	struct intel_connector *connector;
 };
 
 static inline enum dpio_channel
-- 
2.5.5

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

Re: [PATCH 1/6] drm/fb: fix missing /** in kerneldoc comment.

[Daniel Vetter]

On Tue, May 03, 2016 at 02:28:21PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

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

> ---
>  include/drm/drm_crtc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 297e527..6279989 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2606,7 +2606,7 @@ static inline uint32_t drm_color_lut_extract(uint32_t user_input,
>  	return clamp_val(val, 0, max);
>  }
>  
> -/*
> +/**
>   * drm_framebuffer_reference - incr the fb refcnt
>   * @fb: framebuffer
>   *
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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