On Mon, 03 Apr 2023 08:23:45 -0700, Belgaumkar, Vinay wrote:
>
>
> On 3/31/2023 4:56 PM, Dixit, Ashutosh wrote:
> > On Mon, 27 Mar 2023 19:00:28 -0700, Vinay Belgaumkar wrote:
> > Hi Vinay,
> >
> >> +/*
> >> + * Too many intermediate components and steps before freq is adjusted
> >> + * Specially if workload is under execution, so let's wait 100 ms.
> >> + */
> >> +#define ACT_FREQ_LATENCY_US 100000
> >> +
> >> +static uint32_t get_freq(int dirfd, uint8_t id)
> >> +{
> >> + uint32_t val;
> >> +
> >> + igt_require(igt_sysfs_rps_scanf(dirfd, id, "%u", &val) == 1);
> > igt_assert?
> ok.
> >
> >> +static void test_freq_basic_api(int dirfd, int gt)
> >> +{
> >> + uint32_t rpn, rp0, rpe;
> >> +
> >> + /* Save frequencies */
> >> + rpn = get_freq(dirfd, RPS_RPn_FREQ_MHZ);
> >> + rp0 = get_freq(dirfd, RPS_RP0_FREQ_MHZ);
> >> + rpe = get_freq(dirfd, RPS_RP1_FREQ_MHZ);
> >> + igt_info("System min freq: %dMHz; max freq: %dMHz\n", rpn, rp0);
> >> +
> >> + /*
> >> + * Negative bound tests
> >> + * RPn is the floor
> >> + * RP0 is the ceiling
> >> + */
> >> + igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpn - 1) < 0);
> >> + igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rp0 + 1) < 0);
> >> + igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpn - 1) < 0);
> > Is this supposed to be RPS_MAX_FREQ_MHZ?
> We could do this check for max as well. But this is trying to see if min
> can be set to below rpn.
In that case this statement is the same as the first one (2 lines
above). Is that needed?
> >> + igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rp0 + 1) < 0);
> >> +
> > After addressing the above, this is:
> >
> > Reviewed-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> >
> > Also, before merging it would be good to see the results of the new
> > tests. So could you add a HAX patch adding the new tests to
> > fast-feedback.testlist and resend the series?
>
> Sure, will do. Thanks for the review.
>
> Vinay.
>
> >
> > Thanks.
> > --
> > Ashutosh
