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

Message ID 1416418691-24747-1-git-send-email-daniel.vetter@ffwll.ch
State Accepted
Headers show

Commit Message

Daniel Vetter Nov. 19, 2014, 5:38 p.m. UTC
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(-)

Comments

Rob Clark Nov. 19, 2014, 8:21 p.m. UTC | #1
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

Patch
diff mbox

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)