| Message ID | 20180702111613.8326-1-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
Hi, On 7/2/2018 4:57 PM, Maarten Lankhorst wrote: > 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? thanks for review. I don't have commit rights -Mahesh
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
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> <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <p>Hi,<br> </p> <br> <div class="moz-cite-prefix">On 8/2/2018 4:13 PM, Maarten Lankhorst wrote:<br> </div> <blockquote type="cite" cite="mid:8a80db90-7b62-0242-37a8-d46515bd024a@linux.intel.com"> <pre wrap="">Op 02-08-18 om 12:42 schreef Maarten Lankhorst: </pre> <blockquote type="cite"> <pre wrap="">Op 02-07-18 om 13:27 schreef Maarten Lankhorst: </pre> <blockquote type="cite"> <pre wrap="">Op 02-07-18 om 13:16 schreef Mahesh Kumar: </pre> <blockquote type="cite"> <pre wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:mahesh1.kumar@intel.com"><mahesh1.kumar@intel.com></a> --- 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 </pre> </blockquote> <pre wrap="">New behavior makes more sense. Reviewed-by: Maarten Lankhorst <a class="moz-txt-link-rfc2396E" href="mailto:maarten.lankhorst@linux.intel.com"><maarten.lankhorst@linux.intel.com></a> Do you have igt commit rights? </pre> </blockquote> <pre wrap="">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 </pre> </blockquote> <pre wrap="">Hm without the else, errno should be EINVAL in any case..</pre> </blockquote> agree, with this change<br> Reviewed-by: Mahesh Kumar <a class="moz-txt-link-rfc2396E" href="mailto:mahesh1.kumar@intel.com"><mahesh1.kumar@intel.com> </a> <blockquote type="cite" cite="mid:8a80db90-7b62-0242-37a8-d46515bd024a@linux.intel.com"> <pre wrap=""> </pre> </blockquote> <br> </body> </html>
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/
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
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(-)