[PATCH i-g-t] tests/kms_fbc_crc: Don't force fbc on old platforms

[PATCH i-g-t] tests/kms_fbc_crc: Don't force fbc on old platforms

[Daniel Vetter]

It's simply a bit too scary on pre-gen6 and imo not worth the bother
really until someone starts to implement all the hacks an w/a required
on these platforms. On later platforms the issues are just with
correctness and performance hence no risk for hanging machines.

Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 tests/kms_fbc_crc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
index d9642243465c..f2a86a690767 100644
--- a/tests/kms_fbc_crc.c
+++ b/tests/kms_fbc_crc.c
@@ -562,7 +562,8 @@ igt_main
 		igt_require_f(!strstr(buf, "unsupported on this chipset"),
 			      "FBC not supported\n");
 
-		igt_set_module_param_int("enable_fbc", 1);
+		if (intel_gen(data.devid) >= 6)
+			igt_set_module_param_int("enable_fbc", 1);
 
 		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

Re: [PATCH i-g-t] tests/kms_fbc_crc: Don't force fbc on old platforms

[Chris Wilson]

On Mon, Jun 15, 2015 at 05:14:17PM +0200, Daniel Vetter wrote:
> It's simply a bit too scary on pre-gen6 and imo not worth the bother
> really until someone starts to implement all the hacks an w/a required
> on these platforms. On later platforms the issues are just with
> correctness and performance hence no risk for hanging machines.
> 
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  tests/kms_fbc_crc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
> index d9642243465c..f2a86a690767 100644
> --- a/tests/kms_fbc_crc.c
> +++ b/tests/kms_fbc_crc.c
> @@ -562,7 +562,8 @@ igt_main
>  		igt_require_f(!strstr(buf, "unsupported on this chipset"),
>  			      "FBC not supported\n");
>  
> -		igt_set_module_param_int("enable_fbc", 1);
> +		if (intel_gen(data.devid) >= 6)
> +			igt_set_module_param_int("enable_fbc", 1);

That reminds me, using these module params causes lots of "dangerous
module option" spew. Which I think is justified! However, the
alternatives would be either a SETPARAM ioctl or debugfs.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/kms_fbc_crc: Don't force fbc on old platforms

[Paulo Zanoni]

2015-06-15 12:14 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> It's simply a bit too scary on pre-gen6 and imo not worth the bother
> really until someone starts to implement all the hacks an w/a required
> on these platforms. On later platforms the issues are just with
> correctness and performance hence no risk for hanging machines.
>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  tests/kms_fbc_crc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
> index d9642243465c..f2a86a690767 100644
> --- a/tests/kms_fbc_crc.c
> +++ b/tests/kms_fbc_crc.c
> @@ -562,7 +562,8 @@ igt_main
>                 igt_require_f(!strstr(buf, "unsupported on this chipset"),
>                               "FBC not supported\n");
>
> -               igt_set_module_param_int("enable_fbc", 1);
> +               if (intel_gen(data.devid) >= 6)
> +                       igt_set_module_param_int("enable_fbc", 1);

I would prefer the more explicit:

igt_require_f(intel_gen(data.devid) >= 6, "Too afraid to test this.");
igt_set_module_param_int("enable_fbc", 1);

We also need to patch kms_frontbuffer_tracking.c, but I can do this
after your patch if you want.

>
>                 data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
>                 igt_assert(data.bufmgr);
> --
> 2.1.4
>



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

Re: [PATCH i-g-t] tests/kms_fbc_crc: Don't force fbc on old platforms

[Daniel Vetter]

On Mon, Jun 15, 2015 at 12:52:40PM -0300, Paulo Zanoni wrote:
> 2015-06-15 12:14 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > It's simply a bit too scary on pre-gen6 and imo not worth the bother
> > really until someone starts to implement all the hacks an w/a required
> > on these platforms. On later platforms the issues are just with
> > correctness and performance hence no risk for hanging machines.
> >
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  tests/kms_fbc_crc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
> > index d9642243465c..f2a86a690767 100644
> > --- a/tests/kms_fbc_crc.c
> > +++ b/tests/kms_fbc_crc.c
> > @@ -562,7 +562,8 @@ igt_main
> >                 igt_require_f(!strstr(buf, "unsupported on this chipset"),
> >                               "FBC not supported\n");
> >
> > -               igt_set_module_param_int("enable_fbc", 1);
> > +               if (intel_gen(data.devid) >= 6)
> > +                       igt_set_module_param_int("enable_fbc", 1);
> 
> I would prefer the more explicit:
> 
> igt_require_f(intel_gen(data.devid) >= 6, "Too afraid to test this.");

The problem with this is that it'll result in an unconditional skip even
on brave souls trying to test fbc on pre-gen5. Hence the explicit control
flow.
-Daniel

> igt_set_module_param_int("enable_fbc", 1);
> 
> We also need to patch kms_frontbuffer_tracking.c, but I can do this
> after your patch if you want.
> 
> >
> >                 data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
> >                 igt_assert(data.bufmgr);
> > --
> > 2.1.4
> >
> 
> 
> 
> -- 
> Paulo Zanoni

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

Re: [PATCH i-g-t] tests/kms_fbc_crc: Don't force fbc on old platforms

[Daniel Vetter]

On Mon, Jun 15, 2015 at 04:18:22PM +0100, Chris Wilson wrote:
> On Mon, Jun 15, 2015 at 05:14:17PM +0200, Daniel Vetter wrote:
> > It's simply a bit too scary on pre-gen6 and imo not worth the bother
> > really until someone starts to implement all the hacks an w/a required
> > on these platforms. On later platforms the issues are just with
> > correctness and performance hence no risk for hanging machines.
> > 
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  tests/kms_fbc_crc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
> > index d9642243465c..f2a86a690767 100644
> > --- a/tests/kms_fbc_crc.c
> > +++ b/tests/kms_fbc_crc.c
> > @@ -562,7 +562,8 @@ igt_main
> >  		igt_require_f(!strstr(buf, "unsupported on this chipset"),
> >  			      "FBC not supported\n");
> >  
> > -		igt_set_module_param_int("enable_fbc", 1);
> > +		if (intel_gen(data.devid) >= 6)
> > +			igt_set_module_param_int("enable_fbc", 1);
> 
> That reminds me, using these module params causes lots of "dangerous
> module option" spew. Which I think is justified! However, the
> alternatives would be either a SETPARAM ioctl or debugfs.

Hm right. I guess Paulo's module option support should be changed to not
touch the module option if it already has the desired value. Otherwise the
testcases will always have dmesg-warn as their result state, even when the
support is enabled and the test works correctly. But we can't just look at
the current setting since -1 doesn't tell us whether fbc is enabled or
not. And adding debugfs knobs for all of these is exactly what Paulo's
tried to avoid here.

I guess we need to reconsider this all a bit more :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx