[Intel-gfx] [PATCH] drm/i915/selftests: add basic selftests for rc6

[Andi Shyti]

Hi Mika,

> > +static bool test_rc6(struct intel_rc6 *rc6, bool enabled)
> > +{
> > +	struct intel_uncore *uncore = rc6_to_uncore(rc6);
> > +	intel_wakeref_t wakeref;
> > +	u32 ec1, ec2;
> > +	u32 interval;
> > +
> > +	wakeref = intel_runtime_pm_get(uncore->rpm);
> > +
> > +	interval = intel_uncore_read(uncore, GEN6_RC_EVALUATION_INTERVAL);
> > +
> > +	/*
> > +	 * the interval is stored in steps of 1.28us
> > +	 */
> > +	interval = div_u64(mul_u32_u32(interval, 128),
> > +			   100 * 1000); /* => miliseconds */
> > +
> 
> s/miliseconds/milliseconds.

thanks!

> I have a faint memory that the interval was not always 1.28us
> but gen dependant.

1.28 is the incremental step and I haven't seen any different
value in the docs. Have you?

> > +	pr_info("interval:%x [%dms], threshold:%x, rc6:%x, enabled?:%s\n",
> > +		intel_uncore_read(uncore, GEN6_RC_EVALUATION_INTERVAL),
> > +		interval,
> > +		intel_uncore_read(uncore, GEN6_RC6_THRESHOLD),
> > +	       	ec2 - ec1,
> > +	       	yesno(enabled));
> > +
> > +	intel_runtime_pm_put(uncore->rpm, wakeref);
> > +
> > +	return enabled != (ec1 >= ec2);
> 
> Wrap?

actually here I forgot a couple of things that went forgotten in
my git repo.

Anyway, do you mean with "wrap" to add parenthesis?

> > +	intel_rc6_unpark(rc6);
> > +
> > +	/* interval < threshold */
> > +	if (!test_rc6(rc6, false)) {
> 
> consider removing the assertion of 'activeness' in parameter
> and just if (!rc6_active(rc6)). Or am I missing something in here?

yes, you are right, it's misleading. I will make it more clear.

The basic idea is:

 1. disable rc6
 2. check whether it's disabled test_rc6(rc6, false)

or

 1. enable rc6
 2. check if it's enabled test_rc6(rc6, true)

Chris was skeptical about the naming as well.

Thanks!

Andi