[PATCH 1/6] drm/atomic: Ensure that drm_connector_index is stable

[PATCH 1/6] drm/atomic: Ensure that drm_connector_index is stable

[Daniel Vetter]

I've totally forgotten that with DP MST connectors can now be
hotplugged. And failed to adapt Rob's drm_atomic_state code (which
predates connector hotplugging) to the new realities.

The first step is to make sure that the connector indices used to
access the arrays of pointers are stable. The connection mutex gives
us enough guarantees for that, which means we won't unecessarily block
on concurrent modesets or background probing.

So add a locking WARN_ON and shuffle the code slightly to make sure we
always hold the right lock.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 8 ++++----
 drivers/gpu/drm/drm_crtc.c   | 5 +++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index ed22a719440f..90b2d1644bd7 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -295,15 +295,15 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
 	struct drm_mode_config *config = &connector->dev->mode_config;
 	struct drm_connector_state *connector_state;
 
+	ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
+	if (ret)
+		return ERR_PTR(ret);
+
 	index = drm_connector_index(connector);
 
 	if (state->connector_states[index])
 		return state->connector_states[index];
 
-	ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
-	if (ret)
-		return ERR_PTR(ret);
-
 	connector_state = connector->funcs->atomic_duplicate_state(connector);
 	if (!connector_state)
 		return ERR_PTR(-ENOMEM);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 56737e74b59d..5c878f172365 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -867,6 +867,8 @@ int drm_connector_init(struct drm_device *dev,
 
 	drm_connector_get_cmdline_mode(connector);
 
+	/* We should add connectors at the end to avoid upsetting the connector
+	 * index too much. */
 	list_add_tail(&connector->head, &dev->mode_config.connector_list);
 	dev->mode_config.num_connector++;
 
@@ -930,6 +932,9 @@ unsigned int drm_connector_index(struct drm_connector *connector)
 {
 	unsigned int index = 0;
 	struct drm_connector *tmp;
+	struct drm_mode_config *config = &connector->dev->mode_config;
+
+	WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
 
 	list_for_each_entry(tmp, &connector->dev->mode_config.connector_list, head) {
 		if (tmp == connector)
-- 
2.1.1

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

[PATCH 2/6] drm/atomic: Only destroy connector states with connection mutex held

[Daniel Vetter]

Otherwise the connector might have been unplugged and destroyed while
we didn't look. Yet another fallout from DP MST hotplugging that I
didn't consider.

To make sure we get this right add an appropriate WARN_ON to
drm_atomic_state_clear (obviously only when we actually have a state
to clear up). And reorder all the state_clear and backoff calls to
make it work out properly.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c        |  9 ++++++---
 drivers/gpu/drm/drm_atomic_helper.c | 14 +++++++-------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 90b2d1644bd7..67c1dc894bd9 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -112,21 +112,24 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
 void drm_atomic_state_clear(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
+	struct drm_mode_config *config = &dev->mode_config;
 	int i;
 
 	DRM_DEBUG_KMS("Clearing atomic state %p\n", state);
 
-	for (i = 0; i < dev->mode_config.num_connector; i++) {
+	for (i = 0; i < config->num_connector; i++) {
 		struct drm_connector *connector = state->connectors[i];
 
 		if (!connector)
 			continue;
 
+		WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
+
 		connector->funcs->atomic_destroy_state(connector,
 						       state->connector_states[i]);
 	}
 
-	for (i = 0; i < dev->mode_config.num_crtc; i++) {
+	for (i = 0; i < config->num_crtc; i++) {
 		struct drm_crtc *crtc = state->crtcs[i];
 
 		if (!crtc)
@@ -136,7 +139,7 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
 						  state->crtc_states[i]);
 	}
 
-	for (i = 0; i < dev->mode_config.num_total_plane; i++) {
+	for (i = 0; i < config->num_total_plane; i++) {
 		struct drm_plane *plane = state->planes[i];
 
 		if (!plane)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fad2b932cf72..0cd054615920 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1217,8 +1217,8 @@ fail:
 
 	return ret;
 backoff:
-	drm_atomic_legacy_backoff(state);
 	drm_atomic_state_clear(state);
+	drm_atomic_legacy_backoff(state);
 
 	/*
 	 * Someone might have exchanged the framebuffer while we dropped locks
@@ -1285,8 +1285,8 @@ fail:
 
 	return ret;
 backoff:
-	drm_atomic_legacy_backoff(state);
 	drm_atomic_state_clear(state);
+	drm_atomic_legacy_backoff(state);
 
 	/*
 	 * Someone might have exchanged the framebuffer while we dropped locks
@@ -1462,8 +1462,8 @@ fail:
 
 	return ret;
 backoff:
-	drm_atomic_legacy_backoff(state);
 	drm_atomic_state_clear(state);
+	drm_atomic_legacy_backoff(state);
 
 	/*
 	 * Someone might have exchanged the framebuffer while we dropped locks
@@ -1528,8 +1528,8 @@ fail:
 
 	return ret;
 backoff:
-	drm_atomic_legacy_backoff(state);
 	drm_atomic_state_clear(state);
+	drm_atomic_legacy_backoff(state);
 
 	goto retry;
 }
@@ -1587,8 +1587,8 @@ fail:
 
 	return ret;
 backoff:
-	drm_atomic_legacy_backoff(state);
 	drm_atomic_state_clear(state);
+	drm_atomic_legacy_backoff(state);
 
 	goto retry;
 }
@@ -1646,8 +1646,8 @@ fail:
 
 	return ret;
 backoff:
-	drm_atomic_legacy_backoff(state);
 	drm_atomic_state_clear(state);
+	drm_atomic_legacy_backoff(state);
 
 	goto retry;
 }
@@ -1725,8 +1725,8 @@ fail:
 
 	return ret;
 backoff:
-	drm_atomic_legacy_backoff(state);
 	drm_atomic_state_clear(state);
+	drm_atomic_legacy_backoff(state);
 
 	/*
 	 * Someone might have exchanged the framebuffer while we dropped locks
-- 
2.1.1

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

Re: [Intel-gfx] [PATCH 2/6] drm/atomic: Only destroy connector states with connection mutex held

[Rob Clark]

On Wed, Nov 19, 2014 at 12:38 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Otherwise the connector might have been unplugged and destroyed while
> we didn't look. Yet another fallout from DP MST hotplugging that I
> didn't consider.
>
> To make sure we get this right add an appropriate WARN_ON to
> drm_atomic_state_clear (obviously only when we actually have a state
> to clear up). And reorder all the state_clear and backoff calls to
> make it work out properly.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_atomic.c        |  9 ++++++---
>  drivers/gpu/drm/drm_atomic_helper.c | 14 +++++++-------
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 90b2d1644bd7..67c1dc894bd9 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -112,21 +112,24 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
>  void drm_atomic_state_clear(struct drm_atomic_state *state)
>  {
>         struct drm_device *dev = state->dev;
> +       struct drm_mode_config *config = &dev->mode_config;
>         int i;
>
>         DRM_DEBUG_KMS("Clearing atomic state %p\n", state);
>
> -       for (i = 0; i < dev->mode_config.num_connector; i++) {
> +       for (i = 0; i < config->num_connector; i++) {
>                 struct drm_connector *connector = state->connectors[i];
>
>                 if (!connector)
>                         continue;
>
> +               WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
> +
>                 connector->funcs->atomic_destroy_state(connector,
>                                                        state->connector_states[i]);
>         }
>
> -       for (i = 0; i < dev->mode_config.num_crtc; i++) {
> +       for (i = 0; i < config->num_crtc; i++) {
>                 struct drm_crtc *crtc = state->crtcs[i];
>
>                 if (!crtc)
> @@ -136,7 +139,7 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
>                                                   state->crtc_states[i]);
>         }
>
> -       for (i = 0; i < dev->mode_config.num_total_plane; i++) {
> +       for (i = 0; i < config->num_total_plane; i++) {
>                 struct drm_plane *plane = state->planes[i];
>
>                 if (!plane)
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index fad2b932cf72..0cd054615920 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1217,8 +1217,8 @@ fail:
>
>         return ret;
>  backoff:
> -       drm_atomic_legacy_backoff(state);
>         drm_atomic_state_clear(state);
> +       drm_atomic_legacy_backoff(state);
>
>         /*
>          * Someone might have exchanged the framebuffer while we dropped locks
> @@ -1285,8 +1285,8 @@ fail:
>
>         return ret;
>  backoff:
> -       drm_atomic_legacy_backoff(state);
>         drm_atomic_state_clear(state);
> +       drm_atomic_legacy_backoff(state);
>
>         /*
>          * Someone might have exchanged the framebuffer while we dropped locks
> @@ -1462,8 +1462,8 @@ fail:
>
>         return ret;
>  backoff:
> -       drm_atomic_legacy_backoff(state);
>         drm_atomic_state_clear(state);
> +       drm_atomic_legacy_backoff(state);
>
>         /*
>          * Someone might have exchanged the framebuffer while we dropped locks
> @@ -1528,8 +1528,8 @@ fail:
>
>         return ret;
>  backoff:
> -       drm_atomic_legacy_backoff(state);
>         drm_atomic_state_clear(state);
> +       drm_atomic_legacy_backoff(state);
>
>         goto retry;
>  }
> @@ -1587,8 +1587,8 @@ fail:
>
>         return ret;
>  backoff:
> -       drm_atomic_legacy_backoff(state);
>         drm_atomic_state_clear(state);
> +       drm_atomic_legacy_backoff(state);
>
>         goto retry;
>  }
> @@ -1646,8 +1646,8 @@ fail:
>
>         return ret;
>  backoff:
> -       drm_atomic_legacy_backoff(state);
>         drm_atomic_state_clear(state);
> +       drm_atomic_legacy_backoff(state);
>
>         goto retry;
>  }
> @@ -1725,8 +1725,8 @@ fail:
>
>         return ret;
>  backoff:
> -       drm_atomic_legacy_backoff(state);
>         drm_atomic_state_clear(state);
> +       drm_atomic_legacy_backoff(state);
>
>         /*
>          * Someone might have exchanged the framebuffer while we dropped locks
> --
> 2.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] drm/atomic: Don't overrun the connector array when hotplugging

[Daniel Vetter]

Yet another fallout from not considering DP MST hotplug. With the
previous patches we have stable indices, but it might still happen
that a connector gets added between when we allocate the array and
when we actually add a connector. Especially when we back off due to
ww mutex contention or similar issues.

So store the sizes of the arrays in struct drm_atomic_state and double
check them. We don't really care about races except that we want to
use a consistent value, so ACCESS_ONCE is all we need. And if we
indeed notice that we'd overrun the array then just give up and
restart the entire ioctl.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 26 +++++++++++++++++++++-----
 drivers/gpu/drm/drm_atomic_helper.c | 23 ++++++++---------------
 include/drm/drm_crtc.h              |  2 ++
 3 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 67c1dc894bd9..3624632084e2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -56,6 +56,8 @@ drm_atomic_state_alloc(struct drm_device *dev)
 	if (!state)
 		return NULL;
 
+	state->num_connector = ACCESS_ONCE(dev->mode_config.num_connector);
+
 	state->crtcs = kcalloc(dev->mode_config.num_crtc,
 			       sizeof(*state->crtcs), GFP_KERNEL);
 	if (!state->crtcs)
@@ -72,12 +74,12 @@ drm_atomic_state_alloc(struct drm_device *dev)
 				      sizeof(*state->plane_states), GFP_KERNEL);
 	if (!state->plane_states)
 		goto fail;
-	state->connectors = kcalloc(dev->mode_config.num_connector,
+	state->connectors = kcalloc(state->num_connector,
 				    sizeof(*state->connectors),
 				    GFP_KERNEL);
 	if (!state->connectors)
 		goto fail;
-	state->connector_states = kcalloc(dev->mode_config.num_connector,
+	state->connector_states = kcalloc(state->num_connector,
 					  sizeof(*state->connector_states),
 					  GFP_KERNEL);
 	if (!state->connector_states)
@@ -117,7 +119,7 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
 
 	DRM_DEBUG_KMS("Clearing atomic state %p\n", state);
 
-	for (i = 0; i < config->num_connector; i++) {
+	for (i = 0; i < state->num_connector; i++) {
 		struct drm_connector *connector = state->connectors[i];
 
 		if (!connector)
@@ -304,6 +306,21 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
 
 	index = drm_connector_index(connector);
 
+	/*
+	 * Construction of atomic state updates can race with a connector
+	 * hot-add which might overflow. In this case flip the table and just
+	 * restart the entire ioctl - no one is fast enough to livelock a cpu
+	 * with physical hotplug events anyway.
+	 *
+	 * Note that we only grab the indexes once we have the right lock to
+	 * prevent hotplug/unplugging of connectors. So removal is no problem,
+	 * at most the array is a bit too large.
+	 */
+	if (index >= state->num_connector) {
+		DRM_DEBUG_KMS("Hot-added connector would overflow state array, restarting\n");
+		return -EAGAIN;
+	}
+
 	if (state->connector_states[index])
 		return state->connector_states[index];
 
@@ -499,10 +516,9 @@ int
 drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
 			       struct drm_crtc *crtc)
 {
-	int nconnectors = state->dev->mode_config.num_connector;
 	int i, num_connected_connectors = 0;
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < state->num_connector; i++) {
 		struct drm_connector_state *conn_state;
 
 		conn_state = state->connector_states[i];
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0cd054615920..99095ef147ef 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -249,7 +249,6 @@ static int
 mode_fixup(struct drm_atomic_state *state)
 {
 	int ncrtcs = state->dev->mode_config.num_crtc;
-	int nconnectors = state->dev->mode_config.num_connector;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector_state *conn_state;
 	int i;
@@ -264,7 +263,7 @@ mode_fixup(struct drm_atomic_state *state)
 		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
 	}
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < state->num_connector; i++) {
 		struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
 
@@ -336,7 +335,6 @@ drm_atomic_helper_check_prepare(struct drm_device *dev,
 				struct drm_atomic_state *state)
 {
 	int ncrtcs = dev->mode_config.num_crtc;
-	int nconnectors = dev->mode_config.num_connector;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int i, ret;
@@ -361,7 +359,7 @@ drm_atomic_helper_check_prepare(struct drm_device *dev,
 		}
 	}
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < state->num_connector; i++) {
 		/*
 		 * This only sets crtc->mode_changed for routing changes,
 		 * drivers must set crtc->mode_changed themselves when connector
@@ -485,10 +483,9 @@ static void
 disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
 	int ncrtcs = old_state->dev->mode_config.num_crtc;
-	int nconnectors = old_state->dev->mode_config.num_connector;
 	int i;
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < old_state->num_connector; i++) {
 		struct drm_connector_state *old_conn_state;
 		struct drm_connector *connector;
 		struct drm_encoder_helper_funcs *funcs;
@@ -553,12 +550,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 static void
 set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
-	int nconnectors = dev->mode_config.num_connector;
 	int ncrtcs = old_state->dev->mode_config.num_crtc;
 	int i;
 
 	/* clear out existing links */
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < old_state->num_connector; i++) {
 		struct drm_connector *connector;
 
 		connector = old_state->connectors[i];
@@ -573,7 +569,7 @@ set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state)
 	}
 
 	/* set new links */
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < old_state->num_connector; i++) {
 		struct drm_connector *connector;
 
 		connector = old_state->connectors[i];
@@ -608,7 +604,6 @@ static void
 crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
 	int ncrtcs = old_state->dev->mode_config.num_crtc;
-	int nconnectors = old_state->dev->mode_config.num_connector;
 	int i;
 
 	for (i = 0; i < ncrtcs; i++) {
@@ -626,7 +621,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 			funcs->mode_set_nofb(crtc);
 	}
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < old_state->num_connector; i++) {
 		struct drm_connector *connector;
 		struct drm_crtc_state *new_crtc_state;
 		struct drm_encoder_helper_funcs *funcs;
@@ -687,7 +682,6 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
 					  struct drm_atomic_state *old_state)
 {
 	int ncrtcs = old_state->dev->mode_config.num_crtc;
-	int nconnectors = old_state->dev->mode_config.num_connector;
 	int i;
 
 	for (i = 0; i < ncrtcs; i++) {
@@ -706,7 +700,7 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
 			funcs->commit(crtc);
 	}
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < old_state->num_connector; i++) {
 		struct drm_connector *connector;
 		struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
@@ -1304,7 +1298,6 @@ static int update_output_state(struct drm_atomic_state *state,
 {
 	struct drm_device *dev = set->crtc->dev;
 	struct drm_connector_state *conn_state;
-	int nconnectors = state->dev->mode_config.num_connector;
 	int ncrtcs = state->dev->mode_config.num_crtc;
 	int ret, i, j;
 
@@ -1333,7 +1326,7 @@ static int update_output_state(struct drm_atomic_state *state,
 	}
 
 	/* Then recompute connector->crtc links and crtc enabling state. */
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < state->num_connector; i++) {
 		struct drm_connector *connector;
 
 		connector = state->connectors[i];
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 91c09520aad3..cdbae7bdac70 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -845,6 +845,7 @@ struct drm_bridge {
  * @plane_states: pointer to array of plane states pointers
  * @crtcs: pointer to array of CRTC pointers
  * @crtc_states: pointer to array of CRTC states pointers
+ * @num_connector: size of the @connectors and @connector_states arrays
  * @connectors: pointer to array of connector pointers
  * @connector_states: pointer to array of connector states pointers
  * @acquire_ctx: acquire context for this atomic modeset state update
@@ -856,6 +857,7 @@ struct drm_atomic_state {
 	struct drm_plane_state **plane_states;
 	struct drm_crtc **crtcs;
 	struct drm_crtc_state **crtc_states;
+	int num_connector;
 	struct drm_connector **connectors;
 	struct drm_connector_state **connector_states;
 
-- 
2.1.1

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

Re: [PATCH 3/6] drm/atomic: Don't overrun the connector array when hotplugging

[Rob Clark]

On Wed, Nov 19, 2014 at 12:38 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Yet another fallout from not considering DP MST hotplug. With the
> previous patches we have stable indices, but it might still happen
> that a connector gets added between when we allocate the array and
> when we actually add a connector. Especially when we back off due to
> ww mutex contention or similar issues.
>
> So store the sizes of the arrays in struct drm_atomic_state and double
> check them. We don't really care about races except that we want to
> use a consistent value, so ACCESS_ONCE is all we need. And if we
> indeed notice that we'd overrun the array then just give up and
> restart the entire ioctl.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_atomic.c        | 26 +++++++++++++++++++++-----
>  drivers/gpu/drm/drm_atomic_helper.c | 23 ++++++++---------------
>  include/drm/drm_crtc.h              |  2 ++
>  3 files changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 67c1dc894bd9..3624632084e2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -56,6 +56,8 @@ drm_atomic_state_alloc(struct drm_device *dev)
>         if (!state)
>                 return NULL;
>
> +       state->num_connector = ACCESS_ONCE(dev->mode_config.num_connector);
> +
>         state->crtcs = kcalloc(dev->mode_config.num_crtc,
>                                sizeof(*state->crtcs), GFP_KERNEL);
>         if (!state->crtcs)
> @@ -72,12 +74,12 @@ drm_atomic_state_alloc(struct drm_device *dev)
>                                       sizeof(*state->plane_states), GFP_KERNEL);
>         if (!state->plane_states)
>                 goto fail;
> -       state->connectors = kcalloc(dev->mode_config.num_connector,
> +       state->connectors = kcalloc(state->num_connector,
>                                     sizeof(*state->connectors),
>                                     GFP_KERNEL);
>         if (!state->connectors)
>                 goto fail;
> -       state->connector_states = kcalloc(dev->mode_config.num_connector,
> +       state->connector_states = kcalloc(state->num_connector,
>                                           sizeof(*state->connector_states),
>                                           GFP_KERNEL);
>         if (!state->connector_states)
> @@ -117,7 +119,7 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
>
>         DRM_DEBUG_KMS("Clearing atomic state %p\n", state);
>
> -       for (i = 0; i < config->num_connector; i++) {
> +       for (i = 0; i < state->num_connector; i++) {
>                 struct drm_connector *connector = state->connectors[i];
>
>                 if (!connector)
> @@ -304,6 +306,21 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
>
>         index = drm_connector_index(connector);
>
> +       /*
> +        * Construction of atomic state updates can race with a connector
> +        * hot-add which might overflow. In this case flip the table and just
> +        * restart the entire ioctl - no one is fast enough to livelock a cpu
> +        * with physical hotplug events anyway.
> +        *
> +        * Note that we only grab the indexes once we have the right lock to
> +        * prevent hotplug/unplugging of connectors. So removal is no problem,
> +        * at most the array is a bit too large.
> +        */
> +       if (index >= state->num_connector) {
> +               DRM_DEBUG_KMS("Hot-added connector would overflow state array, restarting\n");
> +               return -EAGAIN;
> +       }
> +
>         if (state->connector_states[index])
>                 return state->connector_states[index];
>
> @@ -499,10 +516,9 @@ int
>  drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
>                                struct drm_crtc *crtc)
>  {
> -       int nconnectors = state->dev->mode_config.num_connector;
>         int i, num_connected_connectors = 0;
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < state->num_connector; i++) {
>                 struct drm_connector_state *conn_state;
>
>                 conn_state = state->connector_states[i];
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 0cd054615920..99095ef147ef 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -249,7 +249,6 @@ static int
>  mode_fixup(struct drm_atomic_state *state)
>  {
>         int ncrtcs = state->dev->mode_config.num_crtc;
> -       int nconnectors = state->dev->mode_config.num_connector;
>         struct drm_crtc_state *crtc_state;
>         struct drm_connector_state *conn_state;
>         int i;
> @@ -264,7 +263,7 @@ mode_fixup(struct drm_atomic_state *state)
>                 drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
>         }
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < state->num_connector; i++) {
>                 struct drm_encoder_helper_funcs *funcs;
>                 struct drm_encoder *encoder;
>
> @@ -336,7 +335,6 @@ drm_atomic_helper_check_prepare(struct drm_device *dev,
>                                 struct drm_atomic_state *state)
>  {
>         int ncrtcs = dev->mode_config.num_crtc;
> -       int nconnectors = dev->mode_config.num_connector;
>         struct drm_crtc *crtc;
>         struct drm_crtc_state *crtc_state;
>         int i, ret;
> @@ -361,7 +359,7 @@ drm_atomic_helper_check_prepare(struct drm_device *dev,
>                 }
>         }
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < state->num_connector; i++) {
>                 /*
>                  * This only sets crtc->mode_changed for routing changes,
>                  * drivers must set crtc->mode_changed themselves when connector
> @@ -485,10 +483,9 @@ static void
>  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  {
>         int ncrtcs = old_state->dev->mode_config.num_crtc;
> -       int nconnectors = old_state->dev->mode_config.num_connector;
>         int i;
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < old_state->num_connector; i++) {
>                 struct drm_connector_state *old_conn_state;
>                 struct drm_connector *connector;
>                 struct drm_encoder_helper_funcs *funcs;
> @@ -553,12 +550,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  static void
>  set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state)
>  {
> -       int nconnectors = dev->mode_config.num_connector;
>         int ncrtcs = old_state->dev->mode_config.num_crtc;
>         int i;
>
>         /* clear out existing links */
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < old_state->num_connector; i++) {
>                 struct drm_connector *connector;
>
>                 connector = old_state->connectors[i];
> @@ -573,7 +569,7 @@ set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state)
>         }
>
>         /* set new links */
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < old_state->num_connector; i++) {
>                 struct drm_connector *connector;
>
>                 connector = old_state->connectors[i];
> @@ -608,7 +604,6 @@ static void
>  crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>  {
>         int ncrtcs = old_state->dev->mode_config.num_crtc;
> -       int nconnectors = old_state->dev->mode_config.num_connector;
>         int i;
>
>         for (i = 0; i < ncrtcs; i++) {
> @@ -626,7 +621,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>                         funcs->mode_set_nofb(crtc);
>         }
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < old_state->num_connector; i++) {
>                 struct drm_connector *connector;
>                 struct drm_crtc_state *new_crtc_state;
>                 struct drm_encoder_helper_funcs *funcs;
> @@ -687,7 +682,6 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
>                                           struct drm_atomic_state *old_state)
>  {
>         int ncrtcs = old_state->dev->mode_config.num_crtc;
> -       int nconnectors = old_state->dev->mode_config.num_connector;
>         int i;
>
>         for (i = 0; i < ncrtcs; i++) {
> @@ -706,7 +700,7 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
>                         funcs->commit(crtc);
>         }
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < old_state->num_connector; i++) {
>                 struct drm_connector *connector;
>                 struct drm_encoder_helper_funcs *funcs;
>                 struct drm_encoder *encoder;
> @@ -1304,7 +1298,6 @@ static int update_output_state(struct drm_atomic_state *state,
>  {
>         struct drm_device *dev = set->crtc->dev;
>         struct drm_connector_state *conn_state;
> -       int nconnectors = state->dev->mode_config.num_connector;
>         int ncrtcs = state->dev->mode_config.num_crtc;
>         int ret, i, j;
>
> @@ -1333,7 +1326,7 @@ static int update_output_state(struct drm_atomic_state *state,
>         }
>
>         /* Then recompute connector->crtc links and crtc enabling state. */
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < state->num_connector; i++) {
>                 struct drm_connector *connector;
>
>                 connector = state->connectors[i];
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 91c09520aad3..cdbae7bdac70 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -845,6 +845,7 @@ struct drm_bridge {
>   * @plane_states: pointer to array of plane states pointers
>   * @crtcs: pointer to array of CRTC pointers
>   * @crtc_states: pointer to array of CRTC states pointers
> + * @num_connector: size of the @connectors and @connector_states arrays
>   * @connectors: pointer to array of connector pointers
>   * @connector_states: pointer to array of connector states pointers
>   * @acquire_ctx: acquire context for this atomic modeset state update
> @@ -856,6 +857,7 @@ struct drm_atomic_state {
>         struct drm_plane_state **plane_states;
>         struct drm_crtc **crtcs;
>         struct drm_crtc_state **crtc_states;
> +       int num_connector;
>         struct drm_connector **connectors;
>         struct drm_connector_state **connector_states;
>
> --
> 2.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/atomic: Don't overrun the connector array when hotplugging

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1017 bytes --]

On Wed, Nov 19, 2014 at 06:38:08PM +0100, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
[...]
> @@ -304,6 +306,21 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  
>  	index = drm_connector_index(connector);
>  
> +	/*
> +	 * Construction of atomic state updates can race with a connector
> +	 * hot-add which might overflow. In this case flip the table and just
> +	 * restart the entire ioctl - no one is fast enough to livelock a cpu
> +	 * with physical hotplug events anyway.
> +	 *
> +	 * Note that we only grab the indexes once we have the right lock to
> +	 * prevent hotplug/unplugging of connectors. So removal is no problem,
> +	 * at most the array is a bit too large.
> +	 */
> +	if (index >= state->num_connector) {
> +		DRM_DEBUG_KMS("Hot-added connector would overflow state array, restarting\n");
> +		return -EAGAIN;

This function returns a pointer, so this needs to be ERR_PTR(-EAGAIN).

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 4/6] drm/crtc: Polish kerneldoc

[Daniel Vetter]

- Make it clear that it's a negative errno (more in line with
  everything else).
- Clean up the confusion around get_properties vs. getproperty ioctls:
  One reads per-obj property values, the other reads property
  metadata.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 79 ++++++++++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5c878f172365..8c550302a9ef 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1490,7 +1490,7 @@ EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
  * connectors.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_create_aspect_ratio_property(struct drm_device *dev)
 {
@@ -1674,7 +1674,7 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
  * the caller.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 static int drm_crtc_convert_umode(struct drm_display_mode *out,
 				  const struct drm_mode_modeinfo *in)
@@ -1717,7 +1717,7 @@ static int drm_crtc_convert_umode(struct drm_display_mode *out,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_getresources(struct drm_device *dev, void *data,
 			  struct drm_file *file_priv)
@@ -1905,7 +1905,7 @@ out:
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_getcrtc(struct drm_device *dev,
 		     void *data, struct drm_file *file_priv)
@@ -1966,7 +1966,7 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_getconnector(struct drm_device *dev, void *data,
 			  struct drm_file *file_priv)
@@ -2110,7 +2110,7 @@ out:
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_getencoder(struct drm_device *dev, void *data,
 			struct drm_file *file_priv)
@@ -2151,7 +2151,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_getplane_res(struct drm_device *dev, void *data,
 			  struct drm_file *file_priv)
@@ -2212,7 +2212,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_getplane(struct drm_device *dev, void *data,
 		      struct drm_file *file_priv)
@@ -2386,7 +2386,7 @@ static int setplane_internal(struct drm_plane *plane,
  * valid crtc).
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_setplane(struct drm_device *dev, void *data,
 		      struct drm_file *file_priv)
@@ -2461,7 +2461,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
  * interface. The only thing it adds is correct refcounting dance.
  * 
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_set_config_internal(struct drm_mode_set *set)
 {
@@ -2554,7 +2554,7 @@ EXPORT_SYMBOL(drm_crtc_check_viewport);
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		     struct drm_file *file_priv)
@@ -2717,7 +2717,7 @@ out:
  * userspace wants to make use of these capabilities.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 static int drm_mode_cursor_universal(struct drm_crtc *crtc,
 				     struct drm_mode_cursor2 *req,
@@ -2865,7 +2865,7 @@ out:
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_cursor_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
@@ -2892,7 +2892,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_cursor2_ioctl(struct drm_device *dev,
 			   void *data, struct drm_file *file_priv)
@@ -2956,7 +2956,7 @@ EXPORT_SYMBOL(drm_mode_legacy_fb_format);
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_addfb(struct drm_device *dev,
 		   void *data, struct drm_file *file_priv)
@@ -3161,7 +3161,7 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_addfb2(struct drm_device *dev,
 		    void *data, struct drm_file *file_priv)
@@ -3189,7 +3189,7 @@ int drm_mode_addfb2(struct drm_device *dev,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_rmfb(struct drm_device *dev,
 		   void *data, struct drm_file *file_priv)
@@ -3243,7 +3243,7 @@ fail_lookup:
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_getfb(struct drm_device *dev,
 		   void *data, struct drm_file *file_priv)
@@ -3304,7 +3304,7 @@ int drm_mode_getfb(struct drm_device *dev,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
 			   void *data, struct drm_file *file_priv)
@@ -3384,7 +3384,7 @@ out_err1:
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 void drm_fb_release(struct drm_file *priv)
 {
@@ -3818,17 +3818,20 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
 EXPORT_SYMBOL(drm_object_property_get_value);
 
 /**
- * drm_mode_getproperty_ioctl - get the current value of a connector's property
+ * drm_mode_getproperty_ioctl - get the property metadata
  * @dev: DRM device
  * @data: ioctl data
  * @file_priv: DRM file info
  *
- * This function retrieves the current value for an connectors's property.
+ * This function retrieves the metadata for a given property, like the different
+ * possible values for an enum property or the limits for a range property.
+ *
+ * Blob properties are special
  *
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_getproperty_ioctl(struct drm_device *dev,
 			       void *data, struct drm_file *file_priv)
@@ -3981,7 +3984,7 @@ static void drm_property_destroy_blob(struct drm_device *dev,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_getblob_ioctl(struct drm_device *dev,
 			   void *data, struct drm_file *file_priv)
@@ -4026,7 +4029,7 @@ done:
  * them more meaningful names.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_connector_set_path_property(struct drm_connector *connector,
 					 const char *path)
@@ -4056,7 +4059,7 @@ EXPORT_SYMBOL(drm_mode_connector_set_path_property);
  * connector's edid property.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_connector_update_edid_property(struct drm_connector *connector,
 					    const struct edid *edid)
@@ -4153,7 +4156,7 @@ static bool drm_property_change_is_valid(struct drm_property *property,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
 				       void *data, struct drm_file *file_priv)
@@ -4236,7 +4239,7 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
 
 /**
- * drm_mode_getproperty_ioctl - get the current value of a object's property
+ * drm_mode_obj_get_properties_ioctl - get the current value of a object's property
  * @dev: DRM device
  * @data: ioctl data
  * @file_priv: DRM file info
@@ -4248,7 +4251,7 @@ EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 				      struct drm_file *file_priv)
@@ -4320,7 +4323,7 @@ out:
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *file_priv)
@@ -4392,7 +4395,7 @@ out:
  * possible_clones and possible_crtcs bitmasks.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 				      struct drm_encoder *encoder)
@@ -4419,7 +4422,7 @@ EXPORT_SYMBOL(drm_mode_connector_attach_encoder);
  * fixed gamma table size.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 				 int gamma_size)
@@ -4448,7 +4451,7 @@ EXPORT_SYMBOL(drm_mode_crtc_set_gamma_size);
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_gamma_set_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
@@ -4520,7 +4523,7 @@ out:
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
@@ -4586,7 +4589,7 @@ out:
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
@@ -4752,7 +4755,7 @@ EXPORT_SYMBOL(drm_mode_config_reset);
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_create_dumb_ioctl(struct drm_device *dev,
 			       void *data, struct drm_file *file_priv)
@@ -4804,7 +4807,7 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
@@ -4831,7 +4834,7 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
  * Called by the user via ioctl.
  *
  * Returns:
- * Zero on success, errno on failure.
+ * Zero on success, negative errno on failure.
  */
 int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
 				void *data, struct drm_file *file_priv)
-- 
2.1.1

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

Re: [PATCH 4/6] drm/crtc: Polish kerneldoc

[Rob Clark]

On Wed, Nov 19, 2014 at 12:38 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> - Make it clear that it's a negative errno (more in line with
>   everything else).
> - Clean up the confusion around get_properties vs. getproperty ioctls:
>   One reads per-obj property values, the other reads property
>   metadata.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_crtc.c | 79 ++++++++++++++++++++++++----------------------
>  1 file changed, 41 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5c878f172365..8c550302a9ef 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1490,7 +1490,7 @@ EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>   * connectors.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev)
>  {
> @@ -1674,7 +1674,7 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
>   * the caller.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  static int drm_crtc_convert_umode(struct drm_display_mode *out,
>                                   const struct drm_mode_modeinfo *in)
> @@ -1717,7 +1717,7 @@ static int drm_crtc_convert_umode(struct drm_display_mode *out,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_getresources(struct drm_device *dev, void *data,
>                           struct drm_file *file_priv)
> @@ -1905,7 +1905,7 @@ out:
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_getcrtc(struct drm_device *dev,
>                      void *data, struct drm_file *file_priv)
> @@ -1966,7 +1966,7 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_getconnector(struct drm_device *dev, void *data,
>                           struct drm_file *file_priv)
> @@ -2110,7 +2110,7 @@ out:
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_getencoder(struct drm_device *dev, void *data,
>                         struct drm_file *file_priv)
> @@ -2151,7 +2151,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_getplane_res(struct drm_device *dev, void *data,
>                           struct drm_file *file_priv)
> @@ -2212,7 +2212,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_getplane(struct drm_device *dev, void *data,
>                       struct drm_file *file_priv)
> @@ -2386,7 +2386,7 @@ static int setplane_internal(struct drm_plane *plane,
>   * valid crtc).
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_setplane(struct drm_device *dev, void *data,
>                       struct drm_file *file_priv)
> @@ -2461,7 +2461,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>   * interface. The only thing it adds is correct refcounting dance.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_set_config_internal(struct drm_mode_set *set)
>  {
> @@ -2554,7 +2554,7 @@ EXPORT_SYMBOL(drm_crtc_check_viewport);
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_setcrtc(struct drm_device *dev, void *data,
>                      struct drm_file *file_priv)
> @@ -2717,7 +2717,7 @@ out:
>   * userspace wants to make use of these capabilities.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>                                      struct drm_mode_cursor2 *req,
> @@ -2865,7 +2865,7 @@ out:
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_cursor_ioctl(struct drm_device *dev,
>                           void *data, struct drm_file *file_priv)
> @@ -2892,7 +2892,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_cursor2_ioctl(struct drm_device *dev,
>                            void *data, struct drm_file *file_priv)
> @@ -2956,7 +2956,7 @@ EXPORT_SYMBOL(drm_mode_legacy_fb_format);
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_addfb(struct drm_device *dev,
>                    void *data, struct drm_file *file_priv)
> @@ -3161,7 +3161,7 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_addfb2(struct drm_device *dev,
>                     void *data, struct drm_file *file_priv)
> @@ -3189,7 +3189,7 @@ int drm_mode_addfb2(struct drm_device *dev,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_rmfb(struct drm_device *dev,
>                    void *data, struct drm_file *file_priv)
> @@ -3243,7 +3243,7 @@ fail_lookup:
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_getfb(struct drm_device *dev,
>                    void *data, struct drm_file *file_priv)
> @@ -3304,7 +3304,7 @@ int drm_mode_getfb(struct drm_device *dev,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
>                            void *data, struct drm_file *file_priv)
> @@ -3384,7 +3384,7 @@ out_err1:
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  void drm_fb_release(struct drm_file *priv)
>  {
> @@ -3818,17 +3818,20 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
>  EXPORT_SYMBOL(drm_object_property_get_value);
>
>  /**
> - * drm_mode_getproperty_ioctl - get the current value of a connector's property
> + * drm_mode_getproperty_ioctl - get the property metadata
>   * @dev: DRM device
>   * @data: ioctl data
>   * @file_priv: DRM file info
>   *
> - * This function retrieves the current value for an connectors's property.
> + * This function retrieves the metadata for a given property, like the different
> + * possible values for an enum property or the limits for a range property.
> + *
> + * Blob properties are special
>   *
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_getproperty_ioctl(struct drm_device *dev,
>                                void *data, struct drm_file *file_priv)
> @@ -3981,7 +3984,7 @@ static void drm_property_destroy_blob(struct drm_device *dev,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_getblob_ioctl(struct drm_device *dev,
>                            void *data, struct drm_file *file_priv)
> @@ -4026,7 +4029,7 @@ done:
>   * them more meaningful names.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_connector_set_path_property(struct drm_connector *connector,
>                                          const char *path)
> @@ -4056,7 +4059,7 @@ EXPORT_SYMBOL(drm_mode_connector_set_path_property);
>   * connector's edid property.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>                                             const struct edid *edid)
> @@ -4153,7 +4156,7 @@ static bool drm_property_change_is_valid(struct drm_property *property,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
>                                        void *data, struct drm_file *file_priv)
> @@ -4236,7 +4239,7 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>  EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
>
>  /**
> - * drm_mode_getproperty_ioctl - get the current value of a object's property
> + * drm_mode_obj_get_properties_ioctl - get the current value of a object's property
>   * @dev: DRM device
>   * @data: ioctl data
>   * @file_priv: DRM file info
> @@ -4248,7 +4251,7 @@ EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>                                       struct drm_file *file_priv)
> @@ -4320,7 +4323,7 @@ out:
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>                                     struct drm_file *file_priv)
> @@ -4392,7 +4395,7 @@ out:
>   * possible_clones and possible_crtcs bitmasks.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_connector_attach_encoder(struct drm_connector *connector,
>                                       struct drm_encoder *encoder)
> @@ -4419,7 +4422,7 @@ EXPORT_SYMBOL(drm_mode_connector_attach_encoder);
>   * fixed gamma table size.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>                                  int gamma_size)
> @@ -4448,7 +4451,7 @@ EXPORT_SYMBOL(drm_mode_crtc_set_gamma_size);
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_gamma_set_ioctl(struct drm_device *dev,
>                              void *data, struct drm_file *file_priv)
> @@ -4520,7 +4523,7 @@ out:
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>                              void *data, struct drm_file *file_priv)
> @@ -4586,7 +4589,7 @@ out:
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_page_flip_ioctl(struct drm_device *dev,
>                              void *data, struct drm_file *file_priv)
> @@ -4752,7 +4755,7 @@ EXPORT_SYMBOL(drm_mode_config_reset);
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>                                void *data, struct drm_file *file_priv)
> @@ -4804,7 +4807,7 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
>                              void *data, struct drm_file *file_priv)
> @@ -4831,7 +4834,7 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
>   * Called by the user via ioctl.
>   *
>   * Returns:
> - * Zero on success, errno on failure.
> + * Zero on success, negative errno on failure.
>   */
>  int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
>                                 void *data, struct drm_file *file_priv)
> --
> 2.1.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] drm: s/enum_blob_list/enum_list/ in drm_property

[Daniel Vetter]

I guess for hysterical raisins this was meant to be the way to read
blob properties. But that's done with the two-stage approach which
uses separate blob kms object and the special-purpose get_blob ioctl.

Shipping userspace seems to have never relied on this, and the kernel
also never put any blob thing onto that property. And nowadays it
would blow up, e.g. in drm_property_destroy. Also it makes no sense to
return values in an ioctl that only returns metadata about everything.

So let's ditch all the internal code for the blob list, rename the
list to be unambiguous and sprinkle comments all over the place to
explain this peculiar piece of api.

v2: Squash in fixup from Rob to remove now unused variables.

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c  | 53 +++++++++++++++------------------------------
 include/drm/drm_crtc.h      |  2 +-
 include/uapi/drm/drm_mode.h |  2 ++
 3 files changed, 20 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 8c550302a9ef..589a921d4313 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3457,7 +3457,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags,
 
 	property->flags = flags;
 	property->num_values = num_values;
-	INIT_LIST_HEAD(&property->enum_blob_list);
+	INIT_LIST_HEAD(&property->enum_list);
 
 	if (name) {
 		strncpy(property->name, name, DRM_PROP_NAME_LEN);
@@ -3679,8 +3679,8 @@ int drm_property_add_enum(struct drm_property *property, int index,
 			(value > 63))
 		return -EINVAL;
 
-	if (!list_empty(&property->enum_blob_list)) {
-		list_for_each_entry(prop_enum, &property->enum_blob_list, head) {
+	if (!list_empty(&property->enum_list)) {
+		list_for_each_entry(prop_enum, &property->enum_list, head) {
 			if (prop_enum->value == value) {
 				strncpy(prop_enum->name, name, DRM_PROP_NAME_LEN);
 				prop_enum->name[DRM_PROP_NAME_LEN-1] = '\0';
@@ -3698,7 +3698,7 @@ int drm_property_add_enum(struct drm_property *property, int index,
 	prop_enum->value = value;
 
 	property->values[index] = value;
-	list_add_tail(&prop_enum->head, &property->enum_blob_list);
+	list_add_tail(&prop_enum->head, &property->enum_list);
 	return 0;
 }
 EXPORT_SYMBOL(drm_property_add_enum);
@@ -3715,7 +3715,7 @@ void drm_property_destroy(struct drm_device *dev, struct drm_property *property)
 {
 	struct drm_property_enum *prop_enum, *pt;
 
-	list_for_each_entry_safe(prop_enum, pt, &property->enum_blob_list, head) {
+	list_for_each_entry_safe(prop_enum, pt, &property->enum_list, head) {
 		list_del(&prop_enum->head);
 		kfree(prop_enum);
 	}
@@ -3839,16 +3839,12 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
 	struct drm_mode_get_property *out_resp = data;
 	struct drm_property *property;
 	int enum_count = 0;
-	int blob_count = 0;
 	int value_count = 0;
 	int ret = 0, i;
 	int copied;
 	struct drm_property_enum *prop_enum;
 	struct drm_mode_property_enum __user *enum_ptr;
-	struct drm_property_blob *prop_blob;
-	uint32_t __user *blob_id_ptr;
 	uint64_t __user *values_ptr;
-	uint32_t __user *blob_length_ptr;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -3862,11 +3858,8 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
 
 	if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) ||
 			drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) {
-		list_for_each_entry(prop_enum, &property->enum_blob_list, head)
+		list_for_each_entry(prop_enum, &property->enum_list, head)
 			enum_count++;
-	} else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
-		list_for_each_entry(prop_blob, &property->enum_blob_list, head)
-			blob_count++;
 	}
 
 	value_count = property->num_values;
@@ -3891,7 +3884,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
 		if ((out_resp->count_enum_blobs >= enum_count) && enum_count) {
 			copied = 0;
 			enum_ptr = (struct drm_mode_property_enum __user *)(unsigned long)out_resp->enum_blob_ptr;
-			list_for_each_entry(prop_enum, &property->enum_blob_list, head) {
+			list_for_each_entry(prop_enum, &property->enum_list, head) {
 
 				if (copy_to_user(&enum_ptr[copied].value, &prop_enum->value, sizeof(uint64_t))) {
 					ret = -EFAULT;
@@ -3909,28 +3902,16 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
 		out_resp->count_enum_blobs = enum_count;
 	}
 
-	if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
-		if ((out_resp->count_enum_blobs >= blob_count) && blob_count) {
-			copied = 0;
-			blob_id_ptr = (uint32_t __user *)(unsigned long)out_resp->enum_blob_ptr;
-			blob_length_ptr = (uint32_t __user *)(unsigned long)out_resp->values_ptr;
-
-			list_for_each_entry(prop_blob, &property->enum_blob_list, head) {
-				if (put_user(prop_blob->base.id, blob_id_ptr + copied)) {
-					ret = -EFAULT;
-					goto done;
-				}
-
-				if (put_user(prop_blob->length, blob_length_ptr + copied)) {
-					ret = -EFAULT;
-					goto done;
-				}
-
-				copied++;
-			}
-		}
-		out_resp->count_enum_blobs = blob_count;
-	}
+	/*
+	 * NOTE: The idea seems to have been to use this to read all the blob
+	 * property values. But nothing ever added them to the corresponding
+	 * list, userspace always used the special-purpose get_blob ioctl to
+	 * read the value for a blob property. It also doesn't make a lot of
+	 * sense to return values here when everything else is just metadata for
+	 * the property itself.
+	 */
+	if (drm_property_type_is(property, DRM_MODE_PROP_BLOB))
+		out_resp->count_enum_blobs = 0;
 done:
 	drm_modeset_unlock_all(dev);
 	return ret;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index cdbae7bdac70..3773fe7bc737 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -216,7 +216,7 @@ struct drm_property {
 	uint64_t *values;
 	struct drm_device *dev;
 
-	struct list_head enum_blob_list;
+	struct list_head enum_list;
 };
 
 struct drm_crtc;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a0db2d4aa5f0..86574b0005ff 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -286,6 +286,8 @@ struct drm_mode_get_property {
 	char name[DRM_PROP_NAME_LEN];
 
 	__u32 count_values;
+	/* This is only used to count enum values, not blobs. The _blobs is
+	 * simply because of a historical reason, i.e. backwards compat. */
 	__u32 count_enum_blobs;
 };
 
-- 
2.1.1

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

Re: [PATCH 5/6] drm: s/enum_blob_list/enum_list/ in drm_property

[Rob Clark]

On Wed, Nov 19, 2014 at 12:38 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> I guess for hysterical raisins this was meant to be the way to read
> blob properties. But that's done with the two-stage approach which
> uses separate blob kms object and the special-purpose get_blob ioctl.
>
> Shipping userspace seems to have never relied on this, and the kernel
> also never put any blob thing onto that property. And nowadays it
> would blow up, e.g. in drm_property_destroy. Also it makes no sense to
> return values in an ioctl that only returns metadata about everything.
>
> So let's ditch all the internal code for the blob list, rename the
> list to be unambiguous and sprinkle comments all over the place to
> explain this peculiar piece of api.
>
> v2: Squash in fixup from Rob to remove now unused variables.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_crtc.c  | 53 +++++++++++++++------------------------------
>  include/drm/drm_crtc.h      |  2 +-
>  include/uapi/drm/drm_mode.h |  2 ++
>  3 files changed, 20 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 8c550302a9ef..589a921d4313 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3457,7 +3457,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags,
>
>         property->flags = flags;
>         property->num_values = num_values;
> -       INIT_LIST_HEAD(&property->enum_blob_list);
> +       INIT_LIST_HEAD(&property->enum_list);
>
>         if (name) {
>                 strncpy(property->name, name, DRM_PROP_NAME_LEN);
> @@ -3679,8 +3679,8 @@ int drm_property_add_enum(struct drm_property *property, int index,
>                         (value > 63))
>                 return -EINVAL;
>
> -       if (!list_empty(&property->enum_blob_list)) {
> -               list_for_each_entry(prop_enum, &property->enum_blob_list, head) {
> +       if (!list_empty(&property->enum_list)) {
> +               list_for_each_entry(prop_enum, &property->enum_list, head) {
>                         if (prop_enum->value == value) {
>                                 strncpy(prop_enum->name, name, DRM_PROP_NAME_LEN);
>                                 prop_enum->name[DRM_PROP_NAME_LEN-1] = '\0';
> @@ -3698,7 +3698,7 @@ int drm_property_add_enum(struct drm_property *property, int index,
>         prop_enum->value = value;
>
>         property->values[index] = value;
> -       list_add_tail(&prop_enum->head, &property->enum_blob_list);
> +       list_add_tail(&prop_enum->head, &property->enum_list);
>         return 0;
>  }
>  EXPORT_SYMBOL(drm_property_add_enum);
> @@ -3715,7 +3715,7 @@ void drm_property_destroy(struct drm_device *dev, struct drm_property *property)
>  {
>         struct drm_property_enum *prop_enum, *pt;
>
> -       list_for_each_entry_safe(prop_enum, pt, &property->enum_blob_list, head) {
> +       list_for_each_entry_safe(prop_enum, pt, &property->enum_list, head) {
>                 list_del(&prop_enum->head);
>                 kfree(prop_enum);
>         }
> @@ -3839,16 +3839,12 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>         struct drm_mode_get_property *out_resp = data;
>         struct drm_property *property;
>         int enum_count = 0;
> -       int blob_count = 0;
>         int value_count = 0;
>         int ret = 0, i;
>         int copied;
>         struct drm_property_enum *prop_enum;
>         struct drm_mode_property_enum __user *enum_ptr;
> -       struct drm_property_blob *prop_blob;
> -       uint32_t __user *blob_id_ptr;
>         uint64_t __user *values_ptr;
> -       uint32_t __user *blob_length_ptr;
>
>         if (!drm_core_check_feature(dev, DRIVER_MODESET))
>                 return -EINVAL;
> @@ -3862,11 +3858,8 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>
>         if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) ||
>                         drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) {
> -               list_for_each_entry(prop_enum, &property->enum_blob_list, head)
> +               list_for_each_entry(prop_enum, &property->enum_list, head)
>                         enum_count++;
> -       } else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
> -               list_for_each_entry(prop_blob, &property->enum_blob_list, head)
> -                       blob_count++;
>         }
>
>         value_count = property->num_values;
> @@ -3891,7 +3884,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>                 if ((out_resp->count_enum_blobs >= enum_count) && enum_count) {
>                         copied = 0;
>                         enum_ptr = (struct drm_mode_property_enum __user *)(unsigned long)out_resp->enum_blob_ptr;
> -                       list_for_each_entry(prop_enum, &property->enum_blob_list, head) {
> +                       list_for_each_entry(prop_enum, &property->enum_list, head) {
>
>                                 if (copy_to_user(&enum_ptr[copied].value, &prop_enum->value, sizeof(uint64_t))) {
>                                         ret = -EFAULT;
> @@ -3909,28 +3902,16 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>                 out_resp->count_enum_blobs = enum_count;
>         }
>
> -       if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
> -               if ((out_resp->count_enum_blobs >= blob_count) && blob_count) {
> -                       copied = 0;
> -                       blob_id_ptr = (uint32_t __user *)(unsigned long)out_resp->enum_blob_ptr;
> -                       blob_length_ptr = (uint32_t __user *)(unsigned long)out_resp->values_ptr;
> -
> -                       list_for_each_entry(prop_blob, &property->enum_blob_list, head) {
> -                               if (put_user(prop_blob->base.id, blob_id_ptr + copied)) {
> -                                       ret = -EFAULT;
> -                                       goto done;
> -                               }
> -
> -                               if (put_user(prop_blob->length, blob_length_ptr + copied)) {
> -                                       ret = -EFAULT;
> -                                       goto done;
> -                               }
> -
> -                               copied++;
> -                       }
> -               }
> -               out_resp->count_enum_blobs = blob_count;
> -       }
> +       /*
> +        * NOTE: The idea seems to have been to use this to read all the blob
> +        * property values. But nothing ever added them to the corresponding
> +        * list, userspace always used the special-purpose get_blob ioctl to
> +        * read the value for a blob property. It also doesn't make a lot of
> +        * sense to return values here when everything else is just metadata for
> +        * the property itself.
> +        */
> +       if (drm_property_type_is(property, DRM_MODE_PROP_BLOB))
> +               out_resp->count_enum_blobs = 0;
>  done:
>         drm_modeset_unlock_all(dev);
>         return ret;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index cdbae7bdac70..3773fe7bc737 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -216,7 +216,7 @@ struct drm_property {
>         uint64_t *values;
>         struct drm_device *dev;
>
> -       struct list_head enum_blob_list;
> +       struct list_head enum_list;
>  };
>
>  struct drm_crtc;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index a0db2d4aa5f0..86574b0005ff 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -286,6 +286,8 @@ struct drm_mode_get_property {
>         char name[DRM_PROP_NAME_LEN];
>
>         __u32 count_values;
> +       /* This is only used to count enum values, not blobs. The _blobs is
> +        * simply because of a historical reason, i.e. backwards compat. */
>         __u32 count_enum_blobs;
>  };
>
> --
> 2.1.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/6] drm/atomic_helper: Make it clear that commit_planes gets the old state

[Daniel Vetter]

Oversight from my kerneldoc cleanup when doing the original atomic
helper series - I've only applied this clarification to the modeset
related helpers, and not the plane update code. Remedy this asap.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 99095ef147ef..690360038dc1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -976,18 +976,18 @@ EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
 /**
  * drm_atomic_helper_commit_planes - commit plane state
  * @dev: DRM device
- * @state: atomic state
+ * @old_state: atomic state object with old state structures
  *
  * This function commits the new plane state using the plane and atomic helper
  * functions for planes and crtcs. It assumes that the atomic state has already
  * been pushed into the relevant object state pointers, since this step can no
  * longer fail.
  *
- * It still requires the global state object @state to know which planes and
+ * It still requires the global state object @old_state to know which planes and
  * crtcs need to be updated though.
  */
 void drm_atomic_helper_commit_planes(struct drm_device *dev,
-				     struct drm_atomic_state *state)
+				     struct drm_atomic_state *old_state)
 {
 	int nplanes = dev->mode_config.num_total_plane;
 	int ncrtcs = dev->mode_config.num_crtc;
@@ -995,7 +995,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 
 	for (i = 0; i < ncrtcs; i++) {
 		struct drm_crtc_helper_funcs *funcs;
-		struct drm_crtc *crtc = state->crtcs[i];
+		struct drm_crtc *crtc = old_state->crtcs[i];
 
 		if (!crtc)
 			continue;
@@ -1010,7 +1010,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 
 	for (i = 0; i < nplanes; i++) {
 		struct drm_plane_helper_funcs *funcs;
-		struct drm_plane *plane = state->planes[i];
+		struct drm_plane *plane = old_state->planes[i];
 
 		if (!plane)
 			continue;
@@ -1025,7 +1025,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 
 	for (i = 0; i < ncrtcs; i++) {
 		struct drm_crtc_helper_funcs *funcs;
-		struct drm_crtc *crtc = state->crtcs[i];
+		struct drm_crtc *crtc = old_state->crtcs[i];
 
 		if (!crtc)
 			continue;
-- 
2.1.1

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

Re: [PATCH 6/6] drm/atomic_helper: Make it clear that commit_planes gets the old state

[Rob Clark]

On Wed, Nov 19, 2014 at 12:38 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Oversight from my kerneldoc cleanup when doing the original atomic
> helper series - I've only applied this clarification to the modeset
> related helpers, and not the plane update code. Remedy this asap.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 99095ef147ef..690360038dc1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -976,18 +976,18 @@ EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
>  /**
>   * drm_atomic_helper_commit_planes - commit plane state
>   * @dev: DRM device
> - * @state: atomic state
> + * @old_state: atomic state object with old state structures
>   *
>   * This function commits the new plane state using the plane and atomic helper
>   * functions for planes and crtcs. It assumes that the atomic state has already
>   * been pushed into the relevant object state pointers, since this step can no
>   * longer fail.
>   *
> - * It still requires the global state object @state to know which planes and
> + * It still requires the global state object @old_state to know which planes and
>   * crtcs need to be updated though.
>   */
>  void drm_atomic_helper_commit_planes(struct drm_device *dev,
> -                                    struct drm_atomic_state *state)
> +                                    struct drm_atomic_state *old_state)
>  {
>         int nplanes = dev->mode_config.num_total_plane;
>         int ncrtcs = dev->mode_config.num_crtc;
> @@ -995,7 +995,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>
>         for (i = 0; i < ncrtcs; i++) {
>                 struct drm_crtc_helper_funcs *funcs;
> -               struct drm_crtc *crtc = state->crtcs[i];
> +               struct drm_crtc *crtc = old_state->crtcs[i];
>
>                 if (!crtc)
>                         continue;
> @@ -1010,7 +1010,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>
>         for (i = 0; i < nplanes; i++) {
>                 struct drm_plane_helper_funcs *funcs;
> -               struct drm_plane *plane = state->planes[i];
> +               struct drm_plane *plane = old_state->planes[i];
>
>                 if (!plane)
>                         continue;
> @@ -1025,7 +1025,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>
>         for (i = 0; i < ncrtcs; i++) {
>                 struct drm_crtc_helper_funcs *funcs;
> -               struct drm_crtc *crtc = state->crtcs[i];
> +               struct drm_crtc *crtc = old_state->crtcs[i];
>
>                 if (!crtc)
>                         continue;
> --
> 2.1.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm/atomic_helper: Make it clear that commit_planes gets the old state

[Daniel Vetter]

On Wed, Nov 19, 2014 at 03:29:30PM -0500, Rob Clark wrote:
> On Wed, Nov 19, 2014 at 12:38 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Oversight from my kerneldoc cleanup when doing the original atomic
> > helper series - I've only applied this clarification to the modeset
> > related helpers, and not the plane update code. Remedy this asap.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>

Thanks for the review. I've pulled them into my drm patch pile branch and
will send a pull request to Dave in a few days.

Cheers, 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
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/6] drm/atomic: Ensure that drm_connector_index is stable

[Rob Clark]

On Wed, Nov 19, 2014 at 12:38 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> I've totally forgotten that with DP MST connectors can now be
> hotplugged. And failed to adapt Rob's drm_atomic_state code (which
> predates connector hotplugging) to the new realities.
>
> The first step is to make sure that the connector indices used to
> access the arrays of pointers are stable. The connection mutex gives
> us enough guarantees for that, which means we won't unecessarily block
> on concurrent modesets or background probing.
>
> So add a locking WARN_ON and shuffle the code slightly to make sure we
> always hold the right lock.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_atomic.c | 8 ++++----
>  drivers/gpu/drm/drm_crtc.c   | 5 +++++
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index ed22a719440f..90b2d1644bd7 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -295,15 +295,15 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
>         struct drm_mode_config *config = &connector->dev->mode_config;
>         struct drm_connector_state *connector_state;
>
> +       ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
>         index = drm_connector_index(connector);
>
>         if (state->connector_states[index])
>                 return state->connector_states[index];
>
> -       ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
> -       if (ret)
> -               return ERR_PTR(ret);
> -
>         connector_state = connector->funcs->atomic_duplicate_state(connector);
>         if (!connector_state)
>                 return ERR_PTR(-ENOMEM);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 56737e74b59d..5c878f172365 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -867,6 +867,8 @@ int drm_connector_init(struct drm_device *dev,
>
>         drm_connector_get_cmdline_mode(connector);
>
> +       /* We should add connectors at the end to avoid upsetting the connector
> +        * index too much. */
>         list_add_tail(&connector->head, &dev->mode_config.connector_list);
>         dev->mode_config.num_connector++;
>
> @@ -930,6 +932,9 @@ unsigned int drm_connector_index(struct drm_connector *connector)
>  {
>         unsigned int index = 0;
>         struct drm_connector *tmp;
> +       struct drm_mode_config *config = &connector->dev->mode_config;
> +
> +       WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
>
>         list_for_each_entry(tmp, &connector->dev->mode_config.connector_list, head) {
>                 if (tmp == connector)
> --
> 2.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel