Re: [igt-dev] [PATCH i-g-t 3/3] i915_pm_rpm: rpm resume by user forcewake

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit@intel.com>
> Sent: Wednesday, May 4, 2022 6:46 AM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> Cc: igt-dev@lists.freedesktop.org; Nilawar, Badal <badal.nilawar@intel.com>;
> Ewins, Jon <jon.ewins@intel.com>; Wilson, Chris P <chris.p.wilson@intel.com>
> Subject: Re: [PATCH i-g-t 3/3] i915_pm_rpm: rpm resume by user forcewake
> 
> On Thu, 21 Apr 2022 10:02:45 -0700, Anshuman Gupta wrote:
> >
> > Few gem rpm tests relies on enabling kms crtc in order to trigger rpm
> > resume but on headless platforms these tests skips.
> 
> skip
> 
> > Let it triggered the rpm resume by taking user forcewake.
> 
> Let it trigger rpm resume by taking user forcewake.
Thanks for review comments.
> 
> > +static void
> > +enable_one_screen_or_forcewake_and_wait(struct mode_set_data *data) {
> > +	bool headless;
> > +
> > +	/* Try to resume by enabling any type of display */
> > +	headless = !enable_one_screen_with_type(data, SCREEN_TYPE_ANY);
> > +
> > +	/*
> > +	 * Get User Forcewake to trigger rpm resume in case of headless
> > +	 * as well as no display being connected.
> > +	 */
> > +	if (headless && has_runtime_pm) {
> 
> I think we should remove 'has_runtime_pm' from here, it's confusing. E.g. what
> should we do in the '(headless && !has_runtime_pm)'
> case? There is already a 'igt_require(has_runtime_pm)' in
> setup_environment() triggered from igt_fixture.
Make sense I will do that change.
> 
> Different question: do we need to do this only in headless or can we do this
> unconditionally (i.e. get forcewake both with and without display)? If we can do
> this unconditionally the function becomes
> enable_one_screen_and_forcewake_and_wait() (s/or/and/).
I was conservative to add forcewake overhead to display test.
IMO let's only add this for non-display test.
enable_one_screen() does a modeset and that forces a resume, we need forcewake 
only when system is headless as it can't resume.
> 
> (Note to myself: with this change display tests will use
> enable_one_screen_and_wait() and skip when no display and tests which can
> run with or without display will use
> enable_one_screen_or_forcewake_and_wait(). For display tests we do need to
> call enable_one_screen_with_type() to initialize display).
Correct enable_one_screen_with_type() will do a modeset , display init will be there 
in setup_envirnment().

> 
> > +		data->fw_fd = igt_open_forcewake_handle(drm_fd);
> > +		igt_require(data->fw_fd > 0);
> > +	}
> > +	igt_assert(wait_for_active());
> > +}
> > +
> > +static void clear_forcewake(struct mode_set_data *data) {
> > +	if (data->fw_fd <= 0)
> > +		return;
> > +
> > +	data->fw_fd = close(data->fw_fd);
> > +	igt_assert_eq(data->fw_fd, 0);
> > +}
> > +
> > +static void
> > +disable_all_screens_or_clr_forcewake_and_wait(struct mode_set_data
> > +*data) {
> > +	clear_forcewake(data);
> > +	disable_all_screens(data);
> > +	igt_assert(wait_for_suspended());
> > +}
> > +
> 
> Change function name to disable_all_screens_and_clr_forcewake_and_wait()
> (s/or/and/) since we are doing both?
> 
> >  static drmModePropertyBlobPtr get_connector_edid(drmModeConnectorPtr
> connector,
> >						 int index)
> >  {
> > @@ -842,8 +879,10 @@ static void basic_subtest(void)
> >  {
> >	disable_all_screens_and_wait(&ms_data);
> >
> > -	if (ms_data.res)
> > -		enable_one_screen_and_wait(&ms_data);
> > +	if (ms_data.res) {
> 
> We shouldn't have this check now since we are now supporting headless?
Thanks for pointing this out,  I will remove this cond.
> 
> > +		enable_one_screen_or_forcewake_and_wait(&ms_data);
> > +		clear_forcewake(&ms_data);
> > +	}
> 
> /snip/
> 
> > @@ -2195,8 +2237,10 @@ igt_main_args("", long_options, help_str,
> opt_handler, NULL)
> >		pm_test_caching();
> >	}
> >
> > -	igt_fixture
> > +	igt_fixture {
> >		teardown_environment(false);
> > +		clear_forcewake(&ms_data);
> 
> Also looks like we do not need to install an exit_handler (which calls
> clear_forcewake()) since just closing the fd disables forcewake (and fd will be
> closed on process exit whether in error or not). So that part is ok.
> 
> Other tests seem display related but do we also need to change
> pm_test_tiling()?
This is display specific tilling , TileX, TileY. AFAIU tilling can not be tested without
connected display.
Only missing test I could think of module-reload test, there wcan have a test for headless .

Thanks,
Anshuman Gupta.