| Message ID | 1433428266-1867-3-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 04, 2015 at 11:31:05AM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We may not be perfect, but if we don't even test, we will probably > only get worse over time. > > The function called makes sure we restore whatever was the original > FBC parameter when we exit the test, so this should not affect the > other tests. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > tests/kms_fbc_crc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c > index 37221ac..d964224 100644 > --- a/tests/kms_fbc_crc.c > +++ b/tests/kms_fbc_crc.c > @@ -559,10 +559,10 @@ igt_main > igt_assert_lt(0, fread(buf, 1, sizeof(buf), status)); > fclose(status); > buf[sizeof(buf) - 1] = '\0'; > - igt_require_f(!strstr(buf, "unsupported on this chipset") && > - !strstr(buf, "disabled per module param") && > - !strstr(buf, "disabled per chip default"), > - "FBC not supported/enabled\n"); > + igt_require_f(!strstr(buf, "unsupported on this chipset"), > + "FBC not supported\n"); > + > + igt_set_module_param_int("enable_fbc", 1); This is risky since on older chips fbc is known to pretty much insta-kill the box. Imo we should have a gen6+ here at least (that's roughly the point where the hw workarounds start to sound less scary). Trying to enable this on older platforms just isn't worth it imo. -Daniel > > data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096); > igt_assert(data.bufmgr); > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-06-15 9:11 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > On Thu, Jun 04, 2015 at 11:31:05AM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> We may not be perfect, but if we don't even test, we will probably >> only get worse over time. >> >> The function called makes sure we restore whatever was the original >> FBC parameter when we exit the test, so this should not affect the >> other tests. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> tests/kms_fbc_crc.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c >> index 37221ac..d964224 100644 >> --- a/tests/kms_fbc_crc.c >> +++ b/tests/kms_fbc_crc.c >> @@ -559,10 +559,10 @@ igt_main >> igt_assert_lt(0, fread(buf, 1, sizeof(buf), status)); >> fclose(status); >> buf[sizeof(buf) - 1] = '\0'; >> - igt_require_f(!strstr(buf, "unsupported on this chipset") && >> - !strstr(buf, "disabled per module param") && >> - !strstr(buf, "disabled per chip default"), >> - "FBC not supported/enabled\n"); >> + igt_require_f(!strstr(buf, "unsupported on this chipset"), >> + "FBC not supported\n"); >> + >> + igt_set_module_param_int("enable_fbc", 1); > > This is risky since on older chips fbc is known to pretty much insta-kill > the box. Imo we should have a gen6+ here at least (that's roughly the > point where the hw workarounds start to sound less scary). Well, we can try and then keep an eye on bugzilla, watching for the possible insta-kill bug reports, can't we? At least to me, this was not known as instal-kill since it's not documented anywhere and we don't have bug reports. Also, the move to the frontbuffer tracking infrastructure could maybe have solved the insta-kill problem. > > Trying to enable this on older platforms just isn't worth it imo. If we're not even going to try this, shouldn't we remove all the Kernel code support for FBC on these platforms? This is something I wanted to discuss at some point, but now you gave us a nice excuse :). Maybe it's better to just not have the code at all vs have a code that (maybe) instakills the machine and we're probably not going to fix anytime soon. I don't know, just an idea. Btw, those patches are already on IGT. Thomas seemed to be OK with the ideas, and after one week on the mailing list I pushed them. > -Daniel > > >> >> data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096); >> igt_assert(data.bufmgr); >> -- >> 2.1.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Mon, Jun 15, 2015 at 11:08:27AM -0300, Paulo Zanoni wrote: > 2015-06-15 9:11 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > > On Thu, Jun 04, 2015 at 11:31:05AM -0300, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> We may not be perfect, but if we don't even test, we will probably > >> only get worse over time. > >> > >> The function called makes sure we restore whatever was the original > >> FBC parameter when we exit the test, so this should not affect the > >> other tests. > >> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> --- > >> tests/kms_fbc_crc.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c > >> index 37221ac..d964224 100644 > >> --- a/tests/kms_fbc_crc.c > >> +++ b/tests/kms_fbc_crc.c > >> @@ -559,10 +559,10 @@ igt_main > >> igt_assert_lt(0, fread(buf, 1, sizeof(buf), status)); > >> fclose(status); > >> buf[sizeof(buf) - 1] = '\0'; > >> - igt_require_f(!strstr(buf, "unsupported on this chipset") && > >> - !strstr(buf, "disabled per module param") && > >> - !strstr(buf, "disabled per chip default"), > >> - "FBC not supported/enabled\n"); > >> + igt_require_f(!strstr(buf, "unsupported on this chipset"), > >> + "FBC not supported\n"); > >> + > >> + igt_set_module_param_int("enable_fbc", 1); > > > > This is risky since on older chips fbc is known to pretty much insta-kill > > the box. Imo we should have a gen6+ here at least (that's roughly the > > point where the hw workarounds start to sound less scary). > > Well, we can try and then keep an eye on bugzilla, watching for the > possible insta-kill bug reports, can't we? At least to me, this was > not known as instal-kill since it's not documented anywhere and we > don't have bug reports. Also, the move to the frontbuffer tracking > infrastructure could maybe have solved the insta-kill problem. Afaik fbc kills the chip on gen2 reliable when enabling, and iirc the same on gen3. Not sure about gen4, but on ilk the w/a list in bspec is scary enough to make grown-ups scream. Art recommended to just not bother, since some of the required w/a (which we don't implement) force the chip into an ill-defined state to provoke some kind of soft-reset. It's really all fairly horrible. > > Trying to enable this on older platforms just isn't worth it imo. > > If we're not even going to try this, shouldn't we remove all the > Kernel code support for FBC on these platforms? This is something I > wanted to discuss at some point, but now you gave us a nice excuse :). > Maybe it's better to just not have the code at all vs have a code that > (maybe) instakills the machine and we're probably not going to fix > anytime soon. I don't know, just an idea. Yeah it might be time to nuke the old fbc code entirely. > Btw, those patches are already on IGT. Thomas seemed to be OK with > the ideas, and after one week on the mailing list I pushed them. Yeah didn't notice that yet. I'll follow up with a quick patch just for paranoia.
diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c index 37221ac..d964224 100644 --- a/tests/kms_fbc_crc.c +++ b/tests/kms_fbc_crc.c @@ -559,10 +559,10 @@ igt_main igt_assert_lt(0, fread(buf, 1, sizeof(buf), status)); fclose(status); buf[sizeof(buf) - 1] = '\0'; - igt_require_f(!strstr(buf, "unsupported on this chipset") && - !strstr(buf, "disabled per module param") && - !strstr(buf, "disabled per chip default"), - "FBC not supported/enabled\n"); + igt_require_f(!strstr(buf, "unsupported on this chipset"), + "FBC not supported\n"); + + igt_set_module_param_int("enable_fbc", 1); data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096); igt_assert(data.bufmgr);