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

[Mika Kuoppala]

Andi Shyti <andi at etezian.org> writes:

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

I must have been mixing this with freq bins. Sorry.
And in this level as Chris said, we dont need to care.

>
>> > +	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?
>

I meant that if you take samples between wrap
period.
-Mika

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