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

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

[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(-)

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)
-- 
2.7.4

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

[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(-)

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)
-- 
2.7.4

✓ Fi.CI.BAT: success for drm/core: Fail atomic IOCTL with no CRTC state but with signaling.

[Patchwork]

== Series Details ==

Series: drm/core: Fail atomic IOCTL with no CRTC state but with signaling.
URL   : https://patchwork.freedesktop.org/series/25591/
State : success

== Summary ==

Series 25591v1 drm/core: Fail atomic IOCTL with no CRTC state but with signaling.
https://patchwork.freedesktop.org/api/1.0/series/25591/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:441s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:431s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:578s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:516s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:490s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:484s
fi-glk-2a        total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:597s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:429s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:412s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:414s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:506s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:468s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:468s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:575s
fi-kbl-r         total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time:574s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:462s
fi-skl-6700hq    total:278  pass:228  dwarn:2   dfail:0   fail:26  skip:22  time:400s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:468s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:478s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:432s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:533s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:401s

1a2964dddfe7becfbe0ed3b7cadffdbc526a1ab9 drm-tip: 2017y-06m-09d-21h-50m-40s UTC integration manifest
82f6b26 drm/core: Fail atomic IOCTL with no CRTC state but with signaling.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4930/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

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.

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

[Maarten Lankhorst]

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

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

[Andrey Grodzovsky]

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.

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

[Andrey Grodzovsky]

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.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Andrey Grodzovsky]

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)

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

[Harry Wentland]

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)
> 

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

[Harry Wentland]

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)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sean Paul]

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)
> > 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[Sean Paul]

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)
> > 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Andrey Grodzovsky]

[-- Attachment #1.1: Type: text/plain, Size: 3569 bytes --]



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)
>>>


[-- Attachment #1.2: Type: text/html, Size: 5797 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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


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

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

[Daniel Vetter]

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


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Andrey Grodzovsky]

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>
---
 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;
 }
 
-- 
2.7.4

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

[Andrey Grodzovsky]

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>
---
 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;
 }
 
-- 
2.7.4

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

[Harry Wentland]

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>

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;
>  }
>  
> 

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

[Harry Wentland]

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>

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

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

[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;
> >  }
> >  
> > 

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

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

[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;
> >  }
> >  
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Maarten Lankhorst]

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;

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

[Maarten Lankhorst]

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

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

[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.

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

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

[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.

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

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

[Maarten Lankhorst]

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

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

[Maarten Lankhorst]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/core: Fail atomic IOCTL with no CRTC state but with signaling. (rev2)

[Patchwork]

== Series Details ==

Series: drm/core: Fail atomic IOCTL with no CRTC state but with signaling. (rev2)
URL   : https://patchwork.freedesktop.org/series/25591/
State : success

== Summary ==

Series 25591v2 drm/core: Fail atomic IOCTL with no CRTC state but with signaling.
https://patchwork.freedesktop.org/api/1.0/series/25591/revisions/2/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100125
Test prime_busy:
        Subgroup basic-wait-after-default:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101515 +1
Test prime_self_import:
        Subgroup basic-llseek-bad:
                dmesg-warn -> PASS       (fi-skl-6700hq)
Test drv_module_reload:
        Subgroup basic-reload:
                dmesg-warn -> PASS       (fi-skl-6700hq)

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101515 https://bugs.freedesktop.org/show_bug.cgi?id=101515

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:441s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:427s
fi-bsw-n3050     total:278  pass:241  dwarn:1   dfail:0   fail:0   skip:36  time:534s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:507s
fi-byt-j1900     total:278  pass:252  dwarn:2   dfail:0   fail:0   skip:24  time:488s
fi-byt-n2820     total:278  pass:248  dwarn:2   dfail:0   fail:0   skip:28  time:482s
fi-glk-2a        total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:586s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:437s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:417s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:419s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:494s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:471s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:573s
fi-kbl-r         total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time:578s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:460s
fi-skl-6700hq    total:278  pass:220  dwarn:3   dfail:0   fail:30  skip:24  time:341s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:472s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:478s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:540s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:411s

0c75e3f237ff950c7ba824cbb5f93f40b4bb9c02 drm-tip: 2017y-06m-20d-16h-15m-17s UTC integration manifest
154688f drm/core: Fail atomic IOCTL with no CRTC state but with signaling.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5002/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx