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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
