Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1675364 > unrolled thread

Re: [PATCH v2] drm/core: Fail atomic IOCTL with no CRTC state but with signaling.

Started byDaniel Vetter <daniel@ffwll.ch>
First post2017-06-27 09:40 +0200
Last post2017-06-28 12:30 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] drm/core: Fail atomic IOCTL with no CRTC state but  with signaling. Daniel Vetter <daniel@ffwll.ch> - 2017-06-27 09:40 +0200
    Re: [PATCH v2] drm/core: Fail atomic IOCTL with no CRTC state but  with signaling. Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2017-06-27 16:40 +0200
      Re: [PATCH v2] drm/core: Fail atomic IOCTL with no CRTC state but  with signaling. Daniel Vetter <daniel@ffwll.ch> - 2017-06-27 17:30 +0200
        Re: [PATCH v2] drm/core: Fail atomic IOCTL with no CRTC state but  with signaling. Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2017-06-28 12:30 +0200

#1675364 — Re: [PATCH v2] drm/core: Fail atomic IOCTL with no CRTC state but with signaling.

FromDaniel Vetter <daniel@ffwll.ch>
Date2017-06-27 09:40 +0200
SubjectRe: [PATCH v2] drm/core: Fail atomic IOCTL with no CRTC state but with signaling.
Message-ID<tWTgl-5HC-1@gated-at.bofh.it>
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;
> >  }
> >  
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [next] | [standalone]

#1675830

FromMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date2017-06-27 16:40 +0200
Message-ID<tWZOP-1NK-43@gated-at.bofh.it>
In reply to#1675364
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;

[toc] | [prev] | [next] | [standalone]

#1675905

FromDaniel Vetter <daniel@ffwll.ch>
Date2017-06-27 17:30 +0200
Message-ID<tX0Bd-2nz-53@gated-at.bofh.it>
In reply to#1675830
On Tue, Jun 27, 2017 at 04:29:44PM +0200, Maarten Lankhorst wrote:
> 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?

Ack on both the kernel patch and the igt patch, feel free to push them
both with:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

If you do, please add the Testcase: line to the kernel patch.

Thanks, Daniel

> 
> 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;
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1676524

FromMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date2017-06-28 12:30 +0200
Message-ID<tXiop-5z2-5@gated-at.bofh.it>
In reply to#1675905
Op 27-06-17 om 17:01 schreef Daniel Vetter:
> On Tue, Jun 27, 2017 at 04:29:44PM +0200, Maarten Lankhorst wrote:
>> 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?
> Ack on both the kernel patch and the igt patch, feel free to push them
> both with:
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> If you do, please add the Testcase: line to the kernel patch.
Oops, missed adding the testcase to commit message, but pushed.

I've pushed the IGT testcase as well, after fixing it up to make sure it works as intended.

~Maarten

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web