[igt-dev] [PATCH i-g-t 2/3] test: i915_pm_rpm: conditional initialization of igt_display_t

[Dixit, Ashutosh]

On Thu, 21 Apr 2022 16:29:32 -0700, Dixit, Ashutosh wrote:
>
> On Thu, 21 Apr 2022 10:02:44 -0700, Anshuman Gupta wrote:
> >
> > Initialize igt_display_t display only in case of there are
> > valid drm kms resources.
> >
> > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> > ---
> >  tests/i915/i915_pm_rpm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> > index 2cc89eef2..7a0176b2f 100644
> > --- a/tests/i915/i915_pm_rpm.c
> > +++ b/tests/i915/i915_pm_rpm.c
> > @@ -406,9 +406,9 @@ static void init_mode_set_data(struct mode_set_data *data)
> >		}
> >
> >		kmstest_set_vt_graphics_mode();
> > +		igt_display_require(&data->display, drm_fd);
> >	}
> >
> > -	igt_display_require(&data->display, drm_fd);
>
> Is this needed? Isn't display_disabled set to false in setup_environment()
> for headless? Or is this the right thing to do? Maybe somebody from display
> can review? Thanks.

Ignore my previous comments, I just looked again and understand the code a
little better now.

So the change is fine, I think we are trying to avoid the skip in
igt_display_require() when there is no display.

But we need to move igt_display_fini() inside 'if (data->res)' in
fini_mode_set_data() too I think.

Also can we add a patch first which will change 'bool
setup_environment(bool display_disabled)' to 'bool setup_environment(bool
display_enabled)' and reverse the logic everywhere to get rid of the double
negative, there seems to be no reason for the double negative. Thanks.