[igt-dev] [PATCH i-g-t 2/3] Add a new IGT test to validate DC3CO state

[Arkadiusz Hiler]

On Fri, Oct 04, 2019 at 04:09:08PM +0530, Jeevan B wrote:
> On 2019-10-03 at 17:50:04 +0300, Arkadiusz Hiler wrote:
> > On Thu, Oct 03, 2019 at 01:15:08PM +0530, Jeevan B wrote:
> > > Add a subtest for DC3CO video playback case
> > > to generate selective frame update and validate
> > > that system stays in DC3CO state during execution.
> > > 
> > > v2: Changed PSR2 idle check to sleep check and addressed
> > > cosmetic changes.
> > > 
> > > v3: Renamed a function and restructured code according
> > > to Anshuman’s comments.
> > > 
> > > v4: Cosmetic changes.
> > > 
> > > Signed-off-by: Jeevan B <jeevan.b at intel.com>
> > <SNIP>
> > > +static void setup_dc3co(data_t *data)
> > > +{
> > > +	igt_require(IS_TIGERLAKE(data->devid));
> > 
> > Is there a reason to make a check specific to TIGERLAKE here?
> > 
> > I think that better solution would be to add
> > igt_require_dc_counter(CHECK_DC3CO) that would look for "DC3CO count"
> > presence in i915_dmc_info.
> We are checking DC3CO count in read_dc_counter(), it will check DC state count if counter
> not available then the test will skip. 

Right. That sounds better.

Requiring DC3CO counter explicitly upfront would help with few things
though:

 1. you have check for DC3CO being available early in the test
 instead of having one burried deep within some function - if someone
 reads the test it is much more obvious what are the requirements and
 how the checks are done

 2. you don't get surprise skips form function that has read_ in the
 name, require_ makes it more explicit that it may skip

 3. you do that way ealirer in the process, before speding time on
 setting up FBs/KMS

> In run_videoplayback we are using read_dc_counter() to check DC3CO count. 
> 
> Here we added TGL check as we didn't want to check for any other
> platform. As DC3CO is only supported on TGL.
> If you want us to remove TGL check, we will remove it. 

Adding platorm checks makes maintainence much harder. If any other
future platform will support DC3CO, someone will have to come here and
expand this check.

If you check for DC3CO counter presence instead then you will get this
test running on all DC3CO supporting HW for free.

It's up to kernel to not falsely advertise counters for C-states it does
not support.

I hope this convinces you.

> > This way you have feature check instead of encoding what is supported by
> > which platforms in the tests - kernel should not advertise counters for
> > things it does not support.
> > 
> > > +static void run_videoplayback(data_t *data, int dc_flag)
> > > +{
> > > +	igt_plane_t *primary;
> > > +	uint32_t dc3co_prev_cnt;
> > > +	int i, delay;
> > > +
> > > +	primary = igt_output_get_plane_type(data->output,
> > > +					    DRM_PLANE_TYPE_PRIMARY);
> > > +
> > > +	igt_plane_set_fb(primary, NULL);
> > > +
> > > +	dc3co_prev_cnt = read_dc_counter(data->drm_fd, dc_flag);
> > 
> > I don't quite get why this is parametrized with dc_flag, when the name
> > clearly says that we are testing for DC3CO. Just use CHECK_DC3CO
> > directly.
> > 
> > > +static void test_dc3co_vpb_simulation(data_t *data, int dc_flag)
> > > +{
> > 
> > Same here with dc_flag, you pass it few levels down to functions that
> > are not generic and won't work with anything else than CHECK_DC3CO.
> > 
> we need this flag because read_dc_counter() and check_dc_counter() are used for
> each DC states. (DC5:DC6:DC3CO) 

You need that flag when you are calling check_dc_counter yes, but I
don't think you should make test_dc3co_vpb_siulation() parametrizable
with it.

Let me explain:

test_dc3co_vpb_simulation says that it test dc3co but yet it's
parametrized with dc_flag which is confusing.

The only valid way to call it is:
	test_dc3co_vpb_simulation(&data, CHECK_DC3CO);

If you try to do:
	test_dc3co_vpb_simulation(&data, CHECK_DC5);
or
	test_dc3co_vpb_simulation(&data, CHECK_DC6);

It will not work. So the parameter is completetly unecessary.


Same goes for the line:
	dc3co_prev_cnt = read_dc_counter(data->drm_fd, dc_flag);

The variable is even has dc3co in the name, so you can just pass
CHECK_DC3CO directly instead of passing it through dc_flag all the way
from the top.

	static void test_dc3co_vpb_simulation(data_t *data)
	{
		uint32_t dc5_cnt_before, dc5_cnt_after;

		setup_dc3co(data);
		setup_vpb(data);
		dc5_cnt_before = read_dc_counter(data->drm_fd, CHECK_DC5);
		check_dc3co_with_videoplayback_like_load(data);
		dc5_cnt_after = read_dc_counter(data->drm_fd, CHECK_DC5);
		igt_assert_f(dc5_cnt_after == dc5_cnt_before,
				"DC State moved to DC5\n");
		cleanup_dc3co(data);
	}

and

	static void check_dc3co_with_videoplayback_like_load(data_t *data)
	{
		igt_plane_t *primary;
		uint32_t dc3co_prev_cnt;
		int i, delay;

		primary = igt_output_get_plane_type(data->output,
						    DRM_PLANE_TYPE_PRIMARY);

		igt_plane_set_fb(primary, NULL);

		dc3co_prev_cnt = read_dc_counter(data->drm_fd, CHECK_DC3CO);
		/* Calculate delay to generate idle frame */
		delay = ((1000 * 1000) / data->mode->vrefresh);

		for (i = 0; i < VIDEO_FRAMES; i++) {
			if (i % 2 == 0) {
				igt_plane_set_fb(primary, &data->fb_rgb);
				igt_display_commit(&data->display);
			} else {
				igt_plane_set_fb(primary, &data->fb_rgr);
				igt_display_commit(&data->display);
			}

			usleep(delay);
			igt_assert(psr2_wait_sleep_entry(data->debugfs_fd));
		}
		check_dc_counter(data->drm_fd, CHECK_DC3CO, dc3co_prev_cnt);
	}

Note the suggested name change for run_videoplayback - it actually do
checks (igt_asserts) for dc3co with videoplayback-like load, so the name
here seems more fitting.

> > >  static void test_dc_state_psr(data_t *data, int dc_flag)
> > >  {
> > >  	uint32_t dc_counter_before_psr;
> > > @@ -288,6 +428,13 @@ int main(int argc, char *argv[])
> > >  			     "Can't open /dev/cpu/0/msr.\n");
> > >  	}
> > >  
> > > +	igt_describe("This test simulate videoplay back "
> > > +		     "in order to validate DC3CO state "
> > > +		     "while PSR2 is active and in SLEEP state");
> > > +	igt_subtest("dc3co-vpb-simulation") {
> > 
> > This does not quite help me to understand what and why we are checking
> > here.
> > 
> > As far as I understand we want to make sure that we reach DC3CO state
> > between frames of videoplayback-like workload (when we have some idle
> > frames but not enough of them to reach deeper C-state).
> > 
> > So, if my understanding is correct (and please correct me if I am wrong)
> > something like this would work better:
> > 
> Yes, your understanding is correct. I will do the necessary change accordingly.
> Shall i change the test name to dc3co-vpb what is your opinion?

Looks good, but I have no strong opinion on this one :-)

> > "Make sure that we reach DC3CO power-saving state state with PSR2 panel
> > when we have videoplayback-like display workload."
> > 
> > I would also like to see a normal C comment that explains DC3CO a bit,
> > especially how it differs from DC5 and DC6 and expecations/interactions
> > with idle frames.
> > 
> Sure, I will add it.

Thanks!

-- 
Cheers,
Arek