On Mon, Jun 19, 2017 at 11:35:28AM -0400, Harry Wentland wrote:

On 2017-06-09 05:30 PM, Andrey Grodzovsky wrote:

Problem:
While running IGT kms_atomic_transition test suite i encountered
a hang in drmHandleEvent immidietly follwoing an atomic_commit.

s/immidietly/immediately/g
s/follwoing/following/g

After dumping the atomic state I relized that in this case there was
not even one CRTC attached to the state and only disabled
planes. This probably due to a commit which hadn't changed any property
which would require attaching crtc state. This means drmHandleEvent
will never wake up from read since without CRTC in atomic state
the event fd will not be singnaled.

s/singnaled/signaled/g

This point to a bug in IGT but also DRM should gracefully
fail  such scenario so no hang on user side will happen.

Can we create an IGT fix for this to make sure this won't happen?

Fix:
Explicitly fail by failing atomic_commit early in
drm_mode_atomic_commit where such problem can be identified.

The change seems reasonable to me but I would like to see some input
from someone who's more familiar with the usermode side of things.

I wonder if there's ever a case where it might be desirable to generate an event
from a commit without a crtc. I don't know if anyone has explicitly said that an
event can only be generated from state with crtc.

I usually don't mind letting userspace shoot itself in the foot, so keep that in
mind :)

Sean

/* can't test and expect an event at the same time. */
if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
		(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
			return -EINVAL;

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
---
 drivers/gpu/drm/drm_atomic.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a567310..32eae1c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1933,7 +1933,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	int i, ret;
+	int i, c = 0, ret;
 
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
 		return 0;
@@ -1994,8 +1994,17 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 
 			crtc_state->event->base.fence = fence;
 		}
+
+		c++;

Not sure if intentional, but I like it.

 	}
 
+	/*
+	 * Having this flag means user mode pends on event which will never
+	 * reach due to lack of at least one CRTC for signaling
+	 */
+	if (c == 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
+		return -EINVAL;
+
 	return 0;
 }
 
@@ -2179,6 +2188,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		drm_mode_object_unreference(obj);
 	}
 
+
+

Remove these extraneous newlines.

Harry

 	ret = prepare_crtc_signaling(dev, state, arg, file_priv, &fence_state,
 				     &num_fences);
 	if (ret)