| Message ID | 1497043819-28591-1-git-send-email-Andrey.Grodzovsky@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. 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.
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@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.
Just a reminder. Thanks. On 06/09/2017 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. > 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@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++; > } > > + /* > + * 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); > } > > + > + > ret = prepare_crtc_signaling(dev, state, arg, file_priv, &fence_state, > &num_fences); > if (ret)
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. > 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) >
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 > > > 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) > >
On 06/19/2017 03:24 PM, Sean Paul wrote: > 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. For a generic event i agree, bit seems to me without active CRTC no way you can expect PAGE_FLIP_EVENT to fire. > > I usually don't mind letting userspace shoot itself in the foot, so keep that in > mind :) > > Sean Seems to me you still would try to avoid a bad configuration, returning error will help debugging for user who made a mistake. I also see something similar in drm_mode_atomic_ioctl around line 2122 - /* 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; Thanks, Andrey >>> 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) >>>
On Mon, Jun 19, 2017 at 04:11:35PM -0400, Andrey Grodzovsky wrote: > > > On 06/19/2017 03:24 PM, Sean Paul wrote: > > 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. > For a generic event i agree, bit seems to me without active CRTC no way you > can expect PAGE_FLIP_EVENT to fire. > > > > I usually don't mind letting userspace shoot itself in the foot, so keep that in > > mind :) > > > > Sean > > Seems to me you still would try to avoid a bad configuration, returning > error > will help debugging for user who made a mistake. I also see something > similar > in drm_mode_atomic_ioctl around line 2122 - > > /* 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; Asking for an event without anything changed is a bug imo. We need to fix the testcases, and even better would be to have an explicit testcase which does such a no-op atomic ioctl + event and checks that we get the -EINVAL. Same applies for asking for events when the crtc is off and stays off btw, but I think we catch that already somewhere. But again would be good to make sure we have an igt for it. Thanks, Daniel > > Thanks, > Andrey > > > > 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) > > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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++; } + /* + * 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); } + + ret = prepare_crtc_signaling(dev, state, arg, file_priv, &fence_state, &num_fences); if (ret)
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@amd.com> --- drivers/gpu/drm/drm_atomic.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)