From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <igt-dev-bounces@lists.freedesktop.org>
Received: from mga06.intel.com (mga06b.intel.com [134.134.136.31])
 by gabe.freedesktop.org (Postfix) with ESMTPS id A5EC110FB7E
 for <igt-dev@lists.freedesktop.org>; Tue,  3 May 2022 21:17:04 +0000 (UTC)
Date: Tue, 03 May 2022 14:17:03 -0700
Message-ID: <877d72qptc.wl-ashutosh.dixit@intel.com>
From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
To: Anshuman Gupta <anshuman.gupta@intel.com>
In-Reply-To: <87bkwudnhf.wl-ashutosh.dixit@intel.com>
References: <20220421170245.11898-1-anshuman.gupta@intel.com>	<20220421170245.11898-3-anshuman.gupta@intel.com>	<87bkwudnhf.wl-ashutosh.dixit@intel.com>
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Subject: Re: [igt-dev] [PATCH i-g-t 2/3] test: i915_pm_rpm: conditional
 initialization of igt_display_t
List-Unsubscribe: <https://lists.freedesktop.org/mailman/options/igt-dev>,
 <mailto:igt-dev-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <https://lists.freedesktop.org/archives/igt-dev>
List-Post: <mailto:igt-dev@lists.freedesktop.org>
List-Help: <mailto:igt-dev-request@lists.freedesktop.org?subject=help>
List-Subscribe: <https://lists.freedesktop.org/mailman/listinfo/igt-dev>,
 <mailto:igt-dev-request@lists.freedesktop.org?subject=subscribe>
Cc: igt-dev@lists.freedesktop.org, badal.nilawar@intel.com
Errors-To: igt-dev-bounces@lists.freedesktop.org
Sender: "igt-dev" <igt-dev-bounces@lists.freedesktop.org>
List-ID: <igt-dev@lists.freedesktop.org>

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@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.