On 06/12/2017 07:08 AM, Maarten Lankhorst wrote: > Op 09-06-17 om 23:30 schreef Andrey Grodzovsky: >> Problem: >> While running IGT kms_atomic_transition test suite i encountered >> a hang in drmHandleEvent immidietly follwoing an atomic_commit. >> 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. >> This point to a bug in IGT but also DRM should gracefully >> fail such scenario so no hang on user side will happen. >> >> Fix: >> Explicitly fail by failing atomic_commit early in >> drm_mode_atomic_commit where such problem can be identified. >> >> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com> >> --- >> drivers/gpu/drm/drm_atomic.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) > Patch itself looks sane, but I'm worried about failing with -EINVAL when the same configuration with TEST_ONLY would otherwise succeed on it. DRM_MODE_ATOMIC_TEST_ONLY flag is checked later, after this, so if this fails , test only will return EINVAL also. > Not sure whether we should fail or not, since sending 0 events could still be considered success. > > I don't mind either way, but definitely something that should be discussed before applying. Sending 0 events is no problem at all on it's own but if user provides DRM_MODE_PAGE_FLIP_EVENT in args then when it becomes a problem , doesn't it mean he expects to receive at least one event ? Thanks.
