| Message ID | 20170921013731.4160-1-radhakrishna.sripada@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 21, 2017 at 01:37:31AM +0000, Radhakrishna Sripada wrote: > The substring to be matched is modified to reflect kernel code. Well, technically it is not a regression on psr_drrs because it has this bug since the beginning ;) Also this commit message doesn't explain the false positive case that DK pointed out. Also this doesn't explain that is safe to use the reverse logic of !yes because DRRS is only enabled for eDP. Otherwise we would have to parse specifically the eDP block. What brings me the question of why do we list all that useless information on debugfs? :) > > Fixes: 33355210a43e (igt/kms_psr_sink_crc: Add psr_drrs subtest) > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> Please also CC other folks that are currently working on DRRS.. > --- > tests/kms_psr_sink_crc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c > index 1c25f2c81a34..f023b12c0131 100644 > --- a/tests/kms_psr_sink_crc.c > +++ b/tests/kms_psr_sink_crc.c > @@ -290,7 +290,7 @@ static bool drrs_disabled(data_t *data) > > igt_debugfs_read(data->drm_fd, "i915_drrs_status", buf); > > - return strstr(buf, "DRRS Support: No\n"); > + return !strstr(buf, "DRRS Supported: Yes\n"); > } > > static void run_test(data_t *data) > -- > 2.9.3 >
On Thu, 2017-09-21 at 07:53 -0700, Rodrigo Vivi wrote: > On Thu, Sep 21, 2017 at 01:37:31AM +0000, Radhakrishna Sripada wrote: > > The substring to be matched is modified to reflect kernel code. > > Well, technically it is not a regression on psr_drrs because it has > this bug since the beginning ;) > > Also this commit message doesn't explain the false positive case > that DK pointed out. > > Also this doesn't explain that is safe to use the reverse logic > of !yes because DRRS is only enabled for eDP. Otherwise we would > have to parse specifically the eDP block. > > What brings me the question of why do we list all that useless > information on debugfs? :) > > > > > Fixes: 33355210a43e (igt/kms_psr_sink_crc: Add psr_drrs subtest) > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > Please also CC other folks that are currently working on DRRS.. > > > --- > > tests/kms_psr_sink_crc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c > > index 1c25f2c81a34..f023b12c0131 100644 > > --- a/tests/kms_psr_sink_crc.c > > +++ b/tests/kms_psr_sink_crc.c > > @@ -290,7 +290,7 @@ static bool drrs_disabled(data_t *data) > > > > igt_debugfs_read(data->drm_fd, "i915_drrs_status", buf); > > > > - return strstr(buf, "DRRS Support: No\n"); > > + return !strstr(buf, "DRRS Supported: Yes\n"); Neat and simple. Please send a patch to change the debugfs output that Rodrigo pointed out. > > } > > > > static void run_test(data_t *data) > > -- > > 2.9.3 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Vivi, Rodrigo > Sent: Thursday, September 21, 2017 7:54 AM > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Pandiyan, Dhinakaran > <dhinakaran.pandiyan@intel.com> > Subject: Re: [igt] igt/kms_psr_sink_crc: Fix regression in psr_drrs subtest > > On Thu, Sep 21, 2017 at 01:37:31AM +0000, Radhakrishna Sripada wrote: > > The substring to be matched is modified to reflect kernel code. > > Well, technically it is not a regression on psr_drrs because it has this bug since > the beginning ;) > > Also this commit message doesn't explain the false positive case that DK > pointed out. > > Also this doesn't explain that is safe to use the reverse logic of !yes because > DRRS is only enabled for eDP. Otherwise we would have to parse specifically > the eDP block. > I will update the commit message in the next version of the patch. > What brings me the question of why do we list all that useless information on > debugfs? :) I will send in a cleanup patch of the debugfs output. -Radhakrishna > > > > > Fixes: 33355210a43e (igt/kms_psr_sink_crc: Add psr_drrs subtest) > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > Please also CC other folks that are currently working on DRRS.. > > > --- > > tests/kms_psr_sink_crc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c index > > 1c25f2c81a34..f023b12c0131 100644 > > --- a/tests/kms_psr_sink_crc.c > > +++ b/tests/kms_psr_sink_crc.c > > @@ -290,7 +290,7 @@ static bool drrs_disabled(data_t *data) > > > > igt_debugfs_read(data->drm_fd, "i915_drrs_status", buf); > > > > - return strstr(buf, "DRRS Support: No\n"); > > + return !strstr(buf, "DRRS Supported: Yes\n"); > > } > > > > static void run_test(data_t *data) > > -- > > 2.9.3 > >
diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c index 1c25f2c81a34..f023b12c0131 100644 --- a/tests/kms_psr_sink_crc.c +++ b/tests/kms_psr_sink_crc.c @@ -290,7 +290,7 @@ static bool drrs_disabled(data_t *data) igt_debugfs_read(data->drm_fd, "i915_drrs_status", buf); - return strstr(buf, "DRRS Support: No\n"); + return !strstr(buf, "DRRS Supported: Yes\n"); } static void run_test(data_t *data)
The substring to be matched is modified to reflect kernel code. Fixes: 33355210a43e (igt/kms_psr_sink_crc: Add psr_drrs subtest) Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> --- tests/kms_psr_sink_crc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)