From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> To: Harry Wentland <harry.wentland@amd.com>, Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, seanpaul@chromium.org Subject: Re: [PATCH v2] drm/core: Fail atomic IOCTL with no CRTC state but with signaling. Date: Tue, 27 Jun 2017 16:29:44 +0200 [thread overview] Message-ID: <2fa47671-6a2b-5a19-3072-e7f59b92cb4d@linux.intel.com> (raw) In-Reply-To: <20170627073740.wz5xix2ecmvh4rbn@phenom.ffwll.local> Op 27-06-17 om 09:37 schreef Daniel Vetter: > On Mon, Jun 26, 2017 at 03:44:07PM -0400, Harry Wentland wrote: >> On 2017-06-20 01:57 PM, Andrey Grodzovsky wrote: >>> Problem : While running IGT kms_atomic_transition test suite i encountered >>> a hang in drmHandleEvent immediately following 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 signaled. >>> >>> Fix: Protect against this issue by failing atomic_commit early in >>> drm_mode_atomic_commit where such probelm can be identified. >>> >>> v2: >>> Fix typos and extra newlines. >>> >>> Change-Id: I3ee28ffae35fd1e8bfe553146c44da53da02e6f8 >>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> >> Reviewed-by: Harry Wentland <harry.wentland@amd.com> > Stalling on this hoping for the igt patch. Does it exist already? > -Daniel > >> Harry >> >>> --- >>> drivers/gpu/drm/drm_atomic.c | 11 ++++++++++- >>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>> index a567310..48145bf 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++; >>> } >>> >>> + /* >>> + * 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; >>> } >>> >>> Just do it, and I'll commit this to igt? diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c index 6375fede7179..77429b3db384 100644 --- a/tests/kms_atomic.c +++ b/tests/kms_atomic.c @@ -1205,6 +1205,15 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old, crtc_commit_atomic(&crtc, plane, req, ATOMIC_RELAX_NONE, DRM_MODE_ATOMIC_TEST_ONLY); + /* + * TEST_ONLY cannot be combined with DRM_MODE_PAGE_FLIP_EVENT, + * but DRM_MODE_PAGE_FLIP_EVENT will always generate EINVAL + * without valid crtc, so test it here. + */ + crtc_commit_atomic_err(&crtc, plane, req, ATOMIC_RELAX_NONE, + DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_PAGE_FLIP_EVENT, + EINVAL); + /* Create a blob which is the wrong size to be a valid mode. */ do_or_die(drmModeCreatePropertyBlob(crtc.state->desc->fd, crtc.mode.data, @@ -1356,12 +1365,12 @@ static void atomic_invalid_params(struct kms_atomic_crtc_state *crtc, /* Valid pointers, but still should copy nothing. */ do_ioctl(desc->fd, DRM_IOCTL_MODE_ATOMIC, &ioc); - /* Nonsense flags. */ - ioc.flags = 0xdeadbeef; + /* Valid noop, but with event set should fail. */ + ioc.flags = DRM_MODE_PAGE_FLIP_EVENT; do_ioctl_err(desc->fd, DRM_IOCTL_MODE_ATOMIC, &ioc, EINVAL); - /* Specifically forbidden combination. */ - ioc.flags = DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_PAGE_FLIP_EVENT; + /* Nonsense flags. */ + ioc.flags = 0xdeadbeef; do_ioctl_err(desc->fd, DRM_IOCTL_MODE_ATOMIC, &ioc, EINVAL); ioc.flags = 0;
WARNING: multiple messages have this Message-ID (diff)
From: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> To: Harry Wentland <harry.wentland-5C7GfCeVMHo@public.gmane.org>, Andrey Grodzovsky <Andrey.Grodzovsky-5C7GfCeVMHo@public.gmane.org>, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org Subject: Re: [PATCH v2] drm/core: Fail atomic IOCTL with no CRTC state but with signaling. Date: Tue, 27 Jun 2017 16:29:44 +0200 [thread overview] Message-ID: <2fa47671-6a2b-5a19-3072-e7f59b92cb4d@linux.intel.com> (raw) In-Reply-To: <20170627073740.wz5xix2ecmvh4rbn-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org> Op 27-06-17 om 09:37 schreef Daniel Vetter: > On Mon, Jun 26, 2017 at 03:44:07PM -0400, Harry Wentland wrote: >> On 2017-06-20 01:57 PM, Andrey Grodzovsky wrote: >>> Problem : While running IGT kms_atomic_transition test suite i encountered >>> a hang in drmHandleEvent immediately following 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 signaled. >>> >>> Fix: Protect against this issue by failing atomic_commit early in >>> drm_mode_atomic_commit where such probelm can be identified. >>> >>> v2: >>> Fix typos and extra newlines. >>> >>> Change-Id: I3ee28ffae35fd1e8bfe553146c44da53da02e6f8 >>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> >> Reviewed-by: Harry Wentland <harry.wentland@amd.com> > Stalling on this hoping for the igt patch. Does it exist already? > -Daniel > >> Harry >> >>> --- >>> drivers/gpu/drm/drm_atomic.c | 11 ++++++++++- >>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>> index a567310..48145bf 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++; >>> } >>> >>> + /* >>> + * 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; >>> } >>> >>> Just do it, and I'll commit this to igt? diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c index 6375fede7179..77429b3db384 100644 --- a/tests/kms_atomic.c +++ b/tests/kms_atomic.c @@ -1205,6 +1205,15 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old, crtc_commit_atomic(&crtc, plane, req, ATOMIC_RELAX_NONE, DRM_MODE_ATOMIC_TEST_ONLY); + /* + * TEST_ONLY cannot be combined with DRM_MODE_PAGE_FLIP_EVENT, + * but DRM_MODE_PAGE_FLIP_EVENT will always generate EINVAL + * without valid crtc, so test it here. + */ + crtc_commit_atomic_err(&crtc, plane, req, ATOMIC_RELAX_NONE, + DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_PAGE_FLIP_EVENT, + EINVAL); + /* Create a blob which is the wrong size to be a valid mode. */ do_or_die(drmModeCreatePropertyBlob(crtc.state->desc->fd, crtc.mode.data, @@ -1356,12 +1365,12 @@ static void atomic_invalid_params(struct kms_atomic_crtc_state *crtc, /* Valid pointers, but still should copy nothing. */ do_ioctl(desc->fd, DRM_IOCTL_MODE_ATOMIC, &ioc); - /* Nonsense flags. */ - ioc.flags = 0xdeadbeef; + /* Valid noop, but with event set should fail. */ + ioc.flags = DRM_MODE_PAGE_FLIP_EVENT; do_ioctl_err(desc->fd, DRM_IOCTL_MODE_ATOMIC, &ioc, EINVAL); - /* Specifically forbidden combination. */ - ioc.flags = DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_PAGE_FLIP_EVENT; + /* Nonsense flags. */ + ioc.flags = 0xdeadbeef; do_ioctl_err(desc->fd, DRM_IOCTL_MODE_ATOMIC, &ioc, EINVAL); ioc.flags = 0; _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2017-06-27 14:30 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-06-09 21:30 [PATCH] drm/core: Fail atomic IOCTL with no CRTC state but with signaling Andrey Grodzovsky 2017-06-09 21:30 ` Andrey Grodzovsky 2017-06-09 22:47 ` ✓ Fi.CI.BAT: success for " Patchwork 2017-06-12 11:08 ` [PATCH] " Maarten Lankhorst 2017-06-12 11:08 ` Maarten Lankhorst 2017-06-12 14:12 ` Andrey Grodzovsky 2017-06-12 14:12 ` Andrey Grodzovsky 2017-06-15 20:46 ` Andrey Grodzovsky 2017-06-19 15:35 ` Harry Wentland 2017-06-19 15:35 ` Harry Wentland 2017-06-19 19:24 ` Sean Paul 2017-06-19 19:24 ` Sean Paul 2017-06-19 20:11 ` Andrey Grodzovsky 2017-06-20 9:29 ` [Intel-gfx] " Daniel Vetter 2017-06-20 9:29 ` Daniel Vetter 2017-06-20 17:57 ` [PATCH v2] " Andrey Grodzovsky 2017-06-20 17:57 ` Andrey Grodzovsky 2017-06-26 19:44 ` Harry Wentland 2017-06-26 19:44 ` Harry Wentland 2017-06-27 7:37 ` Daniel Vetter 2017-06-27 7:37 ` Daniel Vetter 2017-06-27 14:29 ` Maarten Lankhorst [this message] 2017-06-27 14:29 ` Maarten Lankhorst 2017-06-27 15:01 ` Daniel Vetter 2017-06-27 15:01 ` Daniel Vetter 2017-06-28 10:23 ` Maarten Lankhorst 2017-06-28 10:23 ` Maarten Lankhorst 2017-06-20 18:30 ` ✓ Fi.CI.BAT: success for drm/core: Fail atomic IOCTL with no CRTC state but with signaling. (rev2) Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2fa47671-6a2b-5a19-3072-e7f59b92cb4d@linux.intel.com \ --to=maarten.lankhorst@linux.intel.com \ --cc=Andrey.Grodzovsky@amd.com \ --cc=amd-gfx@lists.freedesktop.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=harry.wentland@amd.com \ --cc=intel-gfx@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=seanpaul@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.