tests/kms_pipe_crc_basic: expect setting bad source to fail

Submitted by Maarten Lankhorst on Aug. 2, 2018, 10:42 a.m.

Details

Message ID 387a425a-8177-84e8-5ae9-e884c28ad3d7@linux.intel.com
State New
Headers show
Series "tests/kms_pipe_crc_basic: expect setting bad source to fail" ( rev: 2 ) in IGT (deprecated)

Not browsing as part of any series.

Commit Message

Maarten Lankhorst Aug. 2, 2018, 10:42 a.m.
Op 02-07-18 om 13:27 schreef Maarten Lankhorst:
> Op 02-07-18 om 13:16 schreef Mahesh Kumar:
>> Now crc-core framework verifies the source string passed by the user.
>> So setting bad-source will fail. Expect file write to fail in bad-source
>> subtest of kms_pipe_crc_basic.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>  tests/kms_pipe_crc_basic.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
>> index 235fdc38..2d4dfee8 100644
>> --- a/tests/kms_pipe_crc_basic.c
>> +++ b/tests/kms_pipe_crc_basic.c
>> @@ -48,8 +48,7 @@ static struct {
>>  
>>  static void test_bad_source(data_t *data)
>>  {
>> -	igt_assert(igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
>> -	igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
>> +	igt_assert(!igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
>>  }
>>  
>>  #define N_CRCS	3
> New behavior makes more sense.
>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Do you have igt commit rights?
>
Any objections if we change this to allow both behaviors?

Patch hide | download patch | download mbox

diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
index 235fdc386ba2..91909fa42f2f 100644
--- a/tests/kms_pipe_crc_basic.c
+++ b/tests/kms_pipe_crc_basic.c
@@ -48,8 +48,11 @@  static struct {
 
 static void test_bad_source(data_t *data)
 {
-	igt_assert(igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
-	igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
+	errno = 0;
+	if (igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"))
+		igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
+	else
+		igt_assert_eq(errno, EINVAL);
 }
 
 #define N_CRCS	3

Comments

Op 02-08-18 om 12:42 schreef Maarten Lankhorst:
> Op 02-07-18 om 13:27 schreef Maarten Lankhorst:
>> Op 02-07-18 om 13:16 schreef Mahesh Kumar:
>>> Now crc-core framework verifies the source string passed by the user.
>>> So setting bad-source will fail. Expect file write to fail in bad-source
>>> subtest of kms_pipe_crc_basic.
>>>
>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>> ---
>>>  tests/kms_pipe_crc_basic.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
>>> index 235fdc38..2d4dfee8 100644
>>> --- a/tests/kms_pipe_crc_basic.c
>>> +++ b/tests/kms_pipe_crc_basic.c
>>> @@ -48,8 +48,7 @@ static struct {
>>>  
>>>  static void test_bad_source(data_t *data)
>>>  {
>>> -	igt_assert(igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
>>> -	igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
>>> +	igt_assert(!igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
>>>  }
>>>  
>>>  #define N_CRCS	3
>> New behavior makes more sense.
>>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>
>> Do you have igt commit rights?
>>
> Any objections if we change this to allow both behaviors?
>
> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
> index 235fdc386ba2..91909fa42f2f 100644
> --- a/tests/kms_pipe_crc_basic.c
> +++ b/tests/kms_pipe_crc_basic.c
> @@ -48,8 +48,11 @@ static struct {
>  
>  static void test_bad_source(data_t *data)
>  {
> -	igt_assert(igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
> -	igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
> +	errno = 0;
> +	if (igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"))
> +		igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
> +	else
> +		igt_assert_eq(errno, EINVAL);
>  }
>  
>  #define N_CRCS	3
>
Hm without the else, errno should be EINVAL in any case..
Hi,


On 8/2/2018 4:13 PM, Maarten Lankhorst wrote:
> Op 02-08-18 om 12:42 schreef Maarten Lankhorst:
>> Op 02-07-18 om 13:27 schreef Maarten Lankhorst:
>>> Op 02-07-18 om 13:16 schreef Mahesh Kumar:
>>>> Now crc-core framework verifies the source string passed by the user.
>>>> So setting bad-source will fail. Expect file write to fail in bad-source
>>>> subtest of kms_pipe_crc_basic.
>>>>
>>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>>> ---
>>>>   tests/kms_pipe_crc_basic.c | 3 +--
>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
>>>> index 235fdc38..2d4dfee8 100644
>>>> --- a/tests/kms_pipe_crc_basic.c
>>>> +++ b/tests/kms_pipe_crc_basic.c
>>>> @@ -48,8 +48,7 @@ static struct {
>>>>   
>>>>   static void test_bad_source(data_t *data)
>>>>   {
>>>> -	igt_assert(igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
>>>> -	igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
>>>> +	igt_assert(!igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
>>>>   }
>>>>   
>>>>   #define N_CRCS	3
>>> New behavior makes more sense.
>>>
>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>
>>> Do you have igt commit rights?
>>>
>> Any objections if we change this to allow both behaviors?
>>
>> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
>> index 235fdc386ba2..91909fa42f2f 100644
>> --- a/tests/kms_pipe_crc_basic.c
>> +++ b/tests/kms_pipe_crc_basic.c
>> @@ -48,8 +48,11 @@ static struct {
>>   
>>   static void test_bad_source(data_t *data)
>>   {
>> -	igt_assert(igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
>> -	igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
>> +	errno = 0;
>> +	if (igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"))
>> +		igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
>> +	else
>> +		igt_assert_eq(errno, EINVAL);
>>   }
>>   
>>   #define N_CRCS	3
>>
> Hm without the else, errno should be EINVAL in any case..
agree, with this change
Reviewed-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Quoting Maarten Lankhorst (2018-08-02 11:42:32)
> Op 02-07-18 om 13:27 schreef Maarten Lankhorst:
> > Op 02-07-18 om 13:16 schreef Mahesh Kumar:
> >> Now crc-core framework verifies the source string passed by the user.
> >> So setting bad-source will fail. Expect file write to fail in bad-source
> >> subtest of kms_pipe_crc_basic.
> >>
> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> >> ---
> >>  tests/kms_pipe_crc_basic.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
> >> index 235fdc38..2d4dfee8 100644
> >> --- a/tests/kms_pipe_crc_basic.c
> >> +++ b/tests/kms_pipe_crc_basic.c
> >> @@ -48,8 +48,7 @@ static struct {
> >>  
> >>  static void test_bad_source(data_t *data)
> >>  {
> >> -    igt_assert(igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
> >> -    igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
> >> +    igt_assert(!igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
> >>  }
> >>  
> >>  #define N_CRCS      3
> > New behavior makes more sense.
> >
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >
> > Do you have igt commit rights?
> >
> Any objections if we change this to allow both behaviors?
> 
> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
> index 235fdc386ba2..91909fa42f2f 100644
> --- a/tests/kms_pipe_crc_basic.c
> +++ b/tests/kms_pipe_crc_basic.c
> @@ -48,8 +48,11 @@ static struct {
>  
>  static void test_bad_source(data_t *data)
>  {
> -       igt_assert(igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
> -       igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
> +       errno = 0;
> +       if (igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"))
> +               igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
> +       else
> +               igt_assert_eq(errno, EINVAL);

Current errno is EIO
https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4585/fi-bsw-n3050/igt@kms_pipe_crc_basic@bad-source.html
https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4585/fi-kbl-x1275/igt@kms_pipe_crc_basic@bad-source.html
https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4585/fi-kbl-guc/igt@kms_pipe_crc_basic@bad-source.html
-Chris
Op 02-08-18 om 15:03 schreef Chris Wilson:
> Quoting Maarten Lankhorst (2018-08-02 11:42:32)
>> Op 02-07-18 om 13:27 schreef Maarten Lankhorst:
>>> Op 02-07-18 om 13:16 schreef Mahesh Kumar:
>>>> Now crc-core framework verifies the source string passed by the user.
>>>> So setting bad-source will fail. Expect file write to fail in bad-source
>>>> subtest of kms_pipe_crc_basic.
>>>>
>>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>>> ---
>>>>  tests/kms_pipe_crc_basic.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
>>>> index 235fdc38..2d4dfee8 100644
>>>> --- a/tests/kms_pipe_crc_basic.c
>>>> +++ b/tests/kms_pipe_crc_basic.c
>>>> @@ -48,8 +48,7 @@ static struct {
>>>>  
>>>>  static void test_bad_source(data_t *data)
>>>>  {
>>>> -    igt_assert(igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
>>>> -    igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
>>>> +    igt_assert(!igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
>>>>  }
>>>>  
>>>>  #define N_CRCS      3
>>> New behavior makes more sense.
>>>
>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>
>>> Do you have igt commit rights?
>>>
>> Any objections if we change this to allow both behaviors?
>>
>> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
>> index 235fdc386ba2..91909fa42f2f 100644
>> --- a/tests/kms_pipe_crc_basic.c
>> +++ b/tests/kms_pipe_crc_basic.c
>> @@ -48,8 +48,11 @@ static struct {
>>  
>>  static void test_bad_source(data_t *data)
>>  {
>> -       igt_assert(igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"));
>> -       igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
>> +       errno = 0;
>> +       if (igt_sysfs_set(data->debugfs, "crtc-0/crc/control", "foo"))
>> +               igt_assert(openat(data->debugfs, "crtc-0/crc/data", O_WRONLY) == -1);
>> +       else
>> +               igt_assert_eq(errno, EINVAL);
> Current errno is EIO
> https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4585/fi-bsw-n3050/igt@kms_pipe_crc_basic@bad-source.html
> https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4585/fi-kbl-x1275/igt@kms_pipe_crc_basic@bad-source.html
> https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4585/fi-kbl-guc/igt@kms_pipe_crc_basic@bad-source.html
> -Chris

Ugh, my bad, messed up.

https://patchwork.freedesktop.org/patch/242410/