<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p><br>
</p>
<br>
<div class="moz-cite-prefix">On 06/19/2017 03:24 PM, Sean Paul
wrote:<br>
</div>
<blockquote cite="mid:20170619192431.tbsntjjdkianx2kc@art_vandelay"
type="cite">
<pre wrap="">On Mon, Jun 19, 2017 at 11:35:28AM -0400, Harry Wentland wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On 2017-06-09 05:30 PM, Andrey Grodzovsky wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Problem:
While running IGT kms_atomic_transition test suite i encountered
a hang in drmHandleEvent immidietly follwoing an atomic_commit.
</pre>
</blockquote>
<pre wrap="">
s/immidietly/immediately/g
s/follwoing/following/g
</pre>
<blockquote type="cite">
<pre wrap="">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.
</pre>
</blockquote>
<pre wrap="">
s/singnaled/signaled/g
</pre>
<blockquote type="cite">
<pre wrap="">This point to a bug in IGT but also DRM should gracefully
fail such scenario so no hang on user side will happen.
</pre>
</blockquote>
<pre wrap="">
Can we create an IGT fix for this to make sure this won't happen?
</pre>
<blockquote type="cite">
<pre wrap="">Fix:
Explicitly fail by failing atomic_commit early in
drm_mode_atomic_commit where such problem can be identified.
</pre>
</blockquote>
<pre wrap="">
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.
</pre>
</blockquote>
<pre wrap="">
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.</pre>
</blockquote>
For a generic event i agree, bit seems to me without active CRTC no
way you <br>
can expect PAGE_FLIP_EVENT to fire.<br>
<blockquote cite="mid:20170619192431.tbsntjjdkianx2kc@art_vandelay"
type="cite">
<pre wrap="">
I usually don't mind letting userspace shoot itself in the foot, so keep that in
mind :)
Sean</pre>
</blockquote>
<br>
Seems to me you still would try to avoid a bad configuration,
returning error<br>
will help debugging for user who made a mistake. I also see
something similar<br>
in drm_mode_atomic_ioctl around line 2122 - <br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<pre wrap=""><span class="cm" style="box-sizing: inherit; color: slategray; font-style: italic;">/* can't test and expect an event at the same time. */</span>
<span class="cm" style="box-sizing: inherit; color: slategray; font-style: italic;">if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&</span>
<span class="cm" style="box-sizing: inherit; color: slategray; font-style: italic;"> (arg->flags & DRM_MODE_PAGE_FLIP_EVENT))</span>
<span class="cm" style="box-sizing: inherit; color: slategray; font-style: italic;"> return -EINVAL;</span><span class="p" style="box-sizing: inherit; color: rgb(102, 102, 102);"></span>
</pre>
<font size="-1">Thanks,</font><font size="+1"><font size="-1"><br>
Andrey</font> </font><br>
<blockquote cite="mid:20170619192431.tbsntjjdkianx2kc@art_vandelay"
type="cite">
<blockquote type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">Signed-off-by: Andrey Grodzovsky <a class="moz-txt-link-rfc2396E" href="mailto:Andrey.Grodzovsky@amd.com"><Andrey.Grodzovsky@amd.com></a>
---
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++;
</pre>
</blockquote>
<pre wrap="">
Not sure if intentional, but I like it.
</pre>
<blockquote type="cite">
<pre wrap=""> }
+ /*
+ * 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);
}
+
+
</pre>
</blockquote>
<pre wrap="">
Remove these extraneous newlines.
Harry
</pre>
<blockquote type="cite">
<pre wrap=""> ret = prepare_crtc_signaling(dev, state, arg, file_priv, &fence_state,
&num_fences);
if (ret)
</pre>
</blockquote>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>