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

[Jeevan B <jeevan.b@intel.com>]

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

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. 
> 
> 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) 
> >  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?
> "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.
> -- 
> Cheers,
> Arek
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev