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

[Arkadiusz Hiler]

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.

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.

>  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:

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

-- 
Cheers,
Arek