[2/8] tests/kms_panel_fitting: check for i915 before checking version

Submitted by Ulrich Hecht on March 15, 2018, 2:45 p.m.

Details

Message ID 1521125144-28614-3-git-send-email-ulrich.hecht+renesas@gmail.com
State New
Headers show
Series "Non-Intel test suite fixes" ( rev: 1 ) in IGT (deprecated)

Not browsing as part of any series.

Commit Message

Ulrich Hecht March 15, 2018, 2:45 p.m.
Fixes false negatives on non-i915 platforms.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 tests/kms_panel_fitting.c | 1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

diff --git a/tests/kms_panel_fitting.c b/tests/kms_panel_fitting.c
index b3cee22..6d0be50 100644
--- a/tests/kms_panel_fitting.c
+++ b/tests/kms_panel_fitting.c
@@ -243,6 +243,7 @@  static void test_atomic_fastset(igt_display_t *display)
 		igt_set_module_param_int("fastboot", 1);
 
 	igt_require(display->is_atomic);
+	igt_require(is_i915_device(display->drm_fd));
 	igt_require(intel_gen(intel_get_drm_devid(display->drm_fd)) >= 5);
 
 	for_each_pipe_with_valid_output(display, pipe, output) {

Comments

On Thu, Mar 15, 2018 at 03:45:38PM +0100, Ulrich Hecht wrote:
> Fixes false negatives on non-i915 platforms.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  tests/kms_panel_fitting.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/kms_panel_fitting.c b/tests/kms_panel_fitting.c
> index b3cee22..6d0be50 100644
> --- a/tests/kms_panel_fitting.c
> +++ b/tests/kms_panel_fitting.c
> @@ -243,6 +243,7 @@ static void test_atomic_fastset(igt_display_t *display)
>  		igt_set_module_param_int("fastboot", 1);
>  
>  	igt_require(display->is_atomic);
> +	igt_require(is_i915_device(display->drm_fd));
>  	igt_require(intel_gen(intel_get_drm_devid(display->drm_fd)) >= 5);
>  
>  	for_each_pipe_with_valid_output(display, pipe, output) {
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Ulrich,

Thank you for the patch.

On Thursday, 15 March 2018 16:45:38 EET Ulrich Hecht wrote:
> Fixes false negatives on non-i915 platforms.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  tests/kms_panel_fitting.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/kms_panel_fitting.c b/tests/kms_panel_fitting.c
> index b3cee22..6d0be50 100644
> --- a/tests/kms_panel_fitting.c
> +++ b/tests/kms_panel_fitting.c
> @@ -243,6 +243,7 @@ static void test_atomic_fastset(igt_display_t *display)
>  		igt_set_module_param_int("fastboot", 1);
> 
>  	igt_require(display->is_atomic);
> +	igt_require(is_i915_device(display->drm_fd));
>  	igt_require(intel_gen(intel_get_drm_devid(display->drm_fd)) >= 5);

I'm fine with this patch as a quick fix, but what in this test is Intel-
specific ? Can't we replace the Intel generation check with a different 
feature check ?

>  	for_each_pipe_with_valid_output(display, pipe, output) {
On Tue, Mar 20, 2018 at 01:24:09PM +0200, Laurent Pinchart wrote:
> Hi Ulrich,
> 
> Thank you for the patch.
> 
> On Thursday, 15 March 2018 16:45:38 EET Ulrich Hecht wrote:
> > Fixes false negatives on non-i915 platforms.
> > 
> > Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> > ---
> >  tests/kms_panel_fitting.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tests/kms_panel_fitting.c b/tests/kms_panel_fitting.c
> > index b3cee22..6d0be50 100644
> > --- a/tests/kms_panel_fitting.c
> > +++ b/tests/kms_panel_fitting.c
> > @@ -243,6 +243,7 @@ static void test_atomic_fastset(igt_display_t *display)
> >  		igt_set_module_param_int("fastboot", 1);
> > 
> >  	igt_require(display->is_atomic);
> > +	igt_require(is_i915_device(display->drm_fd));
> >  	igt_require(intel_gen(intel_get_drm_devid(display->drm_fd)) >= 5);
> 
> I'm fine with this patch as a quick fix, but what in this test is Intel-
> specific ? Can't we replace the Intel generation check with a different 
> feature check ?

There's some checks in there that we can do certain panel fitter mode
changes without a modeset (throught ALLOW_MODESET for atomic commits).
That's 100% encoding intel hw constraints: Our hw can disable the panel
fitter without a modest (so going from upscaled -> native resolution), but
not any of the other changes (native -> upscaled or 2 different upscaled
versions).
-Daniel

> 
> >  	for_each_pipe_with_valid_output(display, pipe, output) {
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Daniel,

On Wednesday, 21 March 2018 10:34:33 EET Daniel Vetter wrote:
> On Tue, Mar 20, 2018 at 01:24:09PM +0200, Laurent Pinchart wrote:
> > Hi Ulrich,
> > 
> > Thank you for the patch.
> > 
> > On Thursday, 15 March 2018 16:45:38 EET Ulrich Hecht wrote:
> > > Fixes false negatives on non-i915 platforms.
> > > 
> > > Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> > > ---
> > > 
> > >  tests/kms_panel_fitting.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tests/kms_panel_fitting.c b/tests/kms_panel_fitting.c
> > > index b3cee22..6d0be50 100644
> > > --- a/tests/kms_panel_fitting.c
> > > +++ b/tests/kms_panel_fitting.c
> > > @@ -243,6 +243,7 @@ static void test_atomic_fastset(igt_display_t
> > > *display)
> > > 
> > >  		igt_set_module_param_int("fastboot", 1);
> > >  	
> > >  	igt_require(display->is_atomic);
> > > 
> > > +	igt_require(is_i915_device(display->drm_fd));
> > > 
> > >  	igt_require(intel_gen(intel_get_drm_devid(display->drm_fd)) >= 5);
> > 
> > I'm fine with this patch as a quick fix, but what in this test is Intel-
> > specific ? Can't we replace the Intel generation check with a different
> > feature check ?
> 
> There's some checks in there that we can do certain panel fitter mode
> changes without a modeset (throught ALLOW_MODESET for atomic commits).
> That's 100% encoding intel hw constraints: Our hw can disable the panel
> fitter without a modest (so going from upscaled -> native resolution), but
> not any of the other changes (native -> upscaled or 2 different upscaled
> versions).

OK, thank you for the information.

Would it make sense to rename the Intel-specific test files to start with i915 
(or any other Intel prefix) to make this clear ?

> > >  	for_each_pipe_with_valid_output(display, pipe, output) {
On Wed, Mar 21, 2018 at 10:52:19AM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Wednesday, 21 March 2018 10:34:33 EET Daniel Vetter wrote:
> > On Tue, Mar 20, 2018 at 01:24:09PM +0200, Laurent Pinchart wrote:
> > > Hi Ulrich,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Thursday, 15 March 2018 16:45:38 EET Ulrich Hecht wrote:
> > > > Fixes false negatives on non-i915 platforms.
> > > > 
> > > > Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> > > > ---
> > > > 
> > > >  tests/kms_panel_fitting.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/tests/kms_panel_fitting.c b/tests/kms_panel_fitting.c
> > > > index b3cee22..6d0be50 100644
> > > > --- a/tests/kms_panel_fitting.c
> > > > +++ b/tests/kms_panel_fitting.c
> > > > @@ -243,6 +243,7 @@ static void test_atomic_fastset(igt_display_t
> > > > *display)
> > > > 
> > > >  		igt_set_module_param_int("fastboot", 1);
> > > >  	
> > > >  	igt_require(display->is_atomic);
> > > > 
> > > > +	igt_require(is_i915_device(display->drm_fd));
> > > > 
> > > >  	igt_require(intel_gen(intel_get_drm_devid(display->drm_fd)) >= 5);
> > > 
> > > I'm fine with this patch as a quick fix, but what in this test is Intel-
> > > specific ? Can't we replace the Intel generation check with a different
> > > feature check ?
> > 
> > There's some checks in there that we can do certain panel fitter mode
> > changes without a modeset (throught ALLOW_MODESET for atomic commits).
> > That's 100% encoding intel hw constraints: Our hw can disable the panel
> > fitter without a modest (so going from upscaled -> native resolution), but
> > not any of the other changes (native -> upscaled or 2 different upscaled
> > versions).
> 
> OK, thank you for the information.
> 
> Would it make sense to rename the Intel-specific test files to start with i915 
> (or any other Intel prefix) to make this clear ?

We prefer to not rename test names too much, fairly disruptive to our CI.
We're working on some new infrastructure which should make this easier at
least.

But yeah if you bring a qualition, then we can make this happen. Need to
coordinate with igt maintainers on #intel-gfx (since we also need to pull
in CI folks, which hang out there too).
-Daniel

> 
> > > >  	for_each_pipe_with_valid_output(display, pipe, output) {
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>