[PATCH] drm: Handle atomic state properly in kms getfoo ioctl

* [PATCH] drm: Handle atomic state properly in kms getfoo ioctl
@ 2014-11-25 22:50 Daniel Vetter
  2014-11-26 13:00 ` [PATCH] drm: Handle atomic state properly in kms getfoo shuang.he
  2014-11-26 15:52 ` [PATCH] drm: Handle atomic state properly in kms getfoo ioctl Sean Paul
  0 siblings, 2 replies; 4+ messages in thread
From: Daniel Vetter @ 2014-11-25 22:50 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

So the problem with async commit (especially async modeset commit) is
that the legacy pointers only get updated after the point of no
return, in the async part of the modeset sequence. At least as
implemented by the current helper functions. This is done in the
set_routing_links function in drm_atomic_helper.c.

Which also means that access isn't protected by locks but only
coordinated by synchronizing with async workers. No problem thus far,
until we lock at the getconnector/encoder ioctls.

So fix this up by adding special cases for atomic drivers: For those
we need to look at state objects. Unfortunately digging out the
correct encoder->crtc link is a bit of work, so wrap this up in a
helper function.

Moving the assignments of connector->encoder and encoder->crtc earlier
isn't a good idea because the point of the atomic helpers is that we
stage the state updates. That way the disable functions can still
inspect the links and rely upon them.

v2: Extract full encoder->crtc lookup into helper (Rob).

v3: Extract drm_connector_get_encoder too since - we need to always
return state->best_encoder when there is a state otherwise we might
return stale data if there's a pending async disable (and chase
unlocked pointers, too). Same issue with encoder_get_crtc but there
it's a bit more tricky to handle.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 49 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 589a921d4313..d51b1c1f6726 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1955,6 +1955,15 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
 	return true;
 }
 
+static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *connector)
+{
+	/* For atomic drivers only state objects are synchronously updated and
+	 * protected by modeset locks, so check those first. */
+	if (connector->state)
+		return connector->state->best_encoder;
+	return connector->encoder;
+}
+
 /**
  * drm_mode_getconnector - get connector configuration
  * @dev: drm device for the ioctl
@@ -1973,6 +1982,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 {
 	struct drm_mode_get_connector *out_resp = data;
 	struct drm_connector *connector;
+	struct drm_encoder *encoder;
 	struct drm_display_mode *mode;
 	int mode_count = 0;
 	int props_count = 0;
@@ -2028,8 +2038,10 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	out_resp->subpixel = connector->display_info.subpixel_order;
 	out_resp->connection = connector->status;
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-	if (connector->encoder)
-		out_resp->encoder_id = connector->encoder->base.id;
+
+	encoder = drm_connector_get_encoder(connector);
+	if (encoder)
+		out_resp->encoder_id = encoder->base.id;
 	else
 		out_resp->encoder_id = 0;
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
@@ -2099,6 +2111,33 @@ out:
 	return ret;
 }
 
+static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
+{
+	struct drm_connector *connector;
+	struct drm_device *dev = encoder->dev;
+	bool uses_atomic = false;
+
+	/* For atomic drivers only state objects are synchronously updated and
+	 * protected by modeset locks, so check those first. */
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		if (!connector->state)
+			continue;
+
+		uses_atomic = true;
+
+		if (connector->state->best_encoder != encoder)
+			continue;
+
+		return connector->state->crtc;
+	}
+
+	/* Don't return stale data (e.g. pending async disable). */
+	if (uses_atomic)
+		return NULL;
+
+	return encoder->crtc;
+}
+
 /**
  * drm_mode_getencoder - get encoder configuration
  * @dev: drm device for the ioctl
@@ -2117,6 +2156,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
 {
 	struct drm_mode_get_encoder *enc_resp = data;
 	struct drm_encoder *encoder;
+	struct drm_crtc *crtc;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -2126,7 +2166,10 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
 		return -ENOENT;
 
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-	if (encoder->crtc)
+	crtc = drm_encoder_get_crtc(encoder);
+	if (crtc)
+		enc_resp->crtc_id = crtc->base.id;
+	else if (encoder->crtc)
 		enc_resp->crtc_id = encoder->crtc->base.id;
 	else
 		enc_resp->crtc_id = 0;
-- 
2.1.1

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread