[igt-dev] [PATCH i-g-t 1/3] lib/igt_sysfs: Generic verification of writable sysfs attributes

[Dixit, Ashutosh]

On Tue, 13 Dec 2022 22:51:39 -0800, Tauro, Riana wrote:
>
> Hi Ashutosh

Hi Riana,

> On 12/14/2022 8:39 AM, Ashutosh Dixit wrote:
> > Attempt to verify writable sysfs attributes in a generic way, without going
> > into the specifics of any attribute. The attribute is first written to and
> > then read back and it is verified that the read value matches the written
> > value to a tolerance. However, when we try to do this we run into the issue
> > that a sysfs attribute might have a behavior where the read value is
> > different from the written value for any reason. For example, attributes
> > such as power, voltage, frequency and time typically have a linear region
> > outside which they are clamped (the values saturate). Therefore for such
> > attributes read values match the written value only in the linear region
> > and when writing we don't know if we are writing to the linear or to the
> > clamped region.
> >
> > Therefore the verification implemented here takes the approach of sweeping
> > across the range of possible values of the attribute (this is done using
> > 'doubling' rather than linearly) and seeing where there are matches. There
> > should be at least one match for the verification to have succeeded.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > ---
> >   lib/igt_sysfs.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++
> >   lib/igt_sysfs.h |  21 +++++++++
> >   2 files changed, 140 insertions(+)
> >
> > diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> > index a913be4c8f2..c6cccf6fa1e 100644
> > --- a/lib/igt_sysfs.c
> > +++ b/lib/igt_sysfs.c
> > @@ -784,3 +784,122 @@ void fbcon_blink_enable(bool enable)
> >	write(fd, buffer, r + 1);
> >	close(fd);
> >   }
> > +
> > +struct rw_attr_internal {
> > +	/* we need an uint64_t array of max 64 points */
> > +	uint64_t points[64];
> > +	int num_points;
> > +};
> > +
> > +static bool rw_attr_equal_within_epsilon(uint64_t x, uint64_t ref, double tol)
> > +{
> > +	return (x <= (1.0 + tol) * ref) && (x >= (1.0 - tol) * ref);
> > +}
> > +
> > +/* Sweep the range of values for an attribute to identify matching reads/writes */
> > +static int rw_attr_sweep(igt_sysfs_rw_attr_t *rw)
> > +{
> > +	struct rw_attr_internal *pts = rw->rsvd;
> > +	uint64_t get, set = rw->start;
> > +	bool ret;
> > +
> > +	igt_debug("'%s': sweeping range of values\n", rw->attr);
> > +	while (1) {
> Can this be replaced with while(set < UINT64_MAX / 2)?

I think it is useful to run this with --debug and see the traces (to see
the set and get values). So the debug message below (done sweeping) is
convenient to see when the sweep finishes. So you are right but to retain
the 'done sweeping' debug message I prefer to leave as is.

> > +		if (set >= UINT64_MAX / 2) {
> > +			igt_debug("'%s': done sweeping\n", rw->attr);
> > +			break;
> > +		}
> If ret is 0 and num_of_points is > 0 does the loop have to continue?
> Can we break since we have reached the clamped max?

Yeah this is how it was in v1 - v3 of this series. But v4 has dropped the
concept of clamping. So now basically for any sysfs attr we just write
these values and read them back and if they are within epsilon we retain
these points and run a reverification on them. So v4 is basically a
generalization of clamping, there are now N points and out of those M
"match", and it is no longer needed that the M points be in between clamped
min and max. To retain clamping for these rw sysfs attributes was looking
awkward.

So because of the above reasons I think we should leave this too as is.

Let me know what you think.

Thanks.
--
Ashutosh

>
> > +
> > +		ret = igt_sysfs_set_u64(rw->dir, rw->attr, set);
> > +		get = igt_sysfs_get_u64(rw->dir, rw->attr);
> > +		igt_debug("'%s': ret %d set %lu get %lu\n", rw->attr, ret, set, get);
> > +		if (ret && rw_attr_equal_within_epsilon(get, set, rw->tol)) {
> > +			pts->points[pts->num_points++] = set;
> > +			igt_debug("'%s': include %lu\n", rw->attr, set);
> > +		}
> > +
> > +		set *= 2;
> > +	}
> > +
> > +	return pts->num_points ? 0 : -ENOENT;
> > +}
> > +
> > +/* Re-verify that reads following writes are equal within a tolerance */
> > +static int rw_attr_reverify_points(igt_sysfs_rw_attr_t *rw)
> > +{
> > +	struct rw_attr_internal *pts = rw->rsvd;
> > +	uint64_t get, set;
> > +	int i;
> > +
> > +	igt_debug("'%s': reverifying\n", rw->attr);
> > +	for (i = pts->num_points - 1; i >= 0; i--) {
> > +		set = pts->points[i];
> > +		if (!igt_sysfs_set_u64(rw->dir, rw->attr, set)) {
> > +			igt_debug("'%s': set %lu failed\n", rw->attr, set);
> > +			return -EIO;
> > +		}
> > +		get = igt_sysfs_get_u64(rw->dir, rw->attr);
> > +		if (!rw_attr_equal_within_epsilon(get, set, rw->tol)) {
> > +			igt_debug("'%s': mismatch set %lu get %lu\n", rw->attr, set, get);
> > +			return -EIO;
> > +		}
> > +	}
> > +	igt_debug("'%s': done reverifying\n", rw->attr);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * igt_sysfs_rw_attr_verify:
> > + * @rw: 'struct igt_sysfs_rw_attr' describing a rw sysfs attr
> > + *
> > + * This function attempts to verify writable sysfs attributes, that is the
> > + * attribute is first written to and then read back and it is verified that
> > + * the read value matches the written value to a tolerance. However, when
> > + * we try to do this we run into the issue that a sysfs attribute might
> > + * have a behavior where the read value is different from the written value
> > + * for any reason. For example, attributes such as power, voltage,
> > + * frequency and time typically have a linear region outside which they are
> > + * clamped (the values saturate). Therefore for such attributes read values
> > + * match the written value only in the linear region and when writing we
> > + * don't know if we are writing to the linear or to the clamped region.
> > + *
> > + * Therefore the verification implemented here takes the approach of
> > + * sweeping across the range of possible values of the attribute (this is
> > + * done using 'doubling' rather than linearly) and seeing where there are
> > + * matches. There should be at least one match (to a tolerance) for the
> > + * verification to have succeeded.
> > + */
> > +void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
> > +{
> > +	uint64_t prev, get;
> > +	struct stat st;
> > +	int ret;
> > +
> > +	igt_assert(!fstatat(rw->dir, rw->attr, &st, 0));
> > +	igt_assert(st.st_mode & 0222); /* writable */
> > +	igt_assert(rw->start);	/* cannot be 0 */
> > +
> > +	prev = igt_sysfs_get_u64(rw->dir, rw->attr);
> > +	igt_debug("'%s': prev %lu\n", rw->attr, prev);
> > +
> > +	rw->rsvd = calloc(1, sizeof(struct rw_attr_internal));
> > +	igt_assert(rw->rsvd);
> > +
> > +	ret = rw_attr_sweep(rw);
> > +	if (ret)
> > +		goto restore;
> > +
> > +	ret = rw_attr_reverify_points(rw);
> > +
> > +	/*
> > +	 * Restore previous value: we don't assert before this point so
> > +	 * that we can restore the attr before asserting
> > +	 */
> > +restore:
> > +	free(rw->rsvd);
> > +	igt_assert_eq(1, igt_sysfs_set_u64(rw->dir, rw->attr, prev));
> > +	get = igt_sysfs_get_u64(rw->dir, rw->attr);
> > +	igt_assert_eq(get, prev);
> > +	igt_assert(!ret);
> > +}
> > diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> > index 1c9791a1bdc..418c800e8c5 100644
> > --- a/lib/igt_sysfs.h
> > +++ b/lib/igt_sysfs.h
> > @@ -125,4 +125,25 @@ void bind_fbcon(bool enable);
> >   void kick_snd_hda_intel(void);
> >   void fbcon_blink_enable(bool enable);
> >   +/**
> > + * igt_sysfs_rw_attr:
> > + * @dir: file descriptor for parent directory
> > + * @attr: name of sysfs attribute
> > + * @start: start value for searching for matching reads/writes
> > + * @tol: tolerance to use to compare written and read values
> > + * @rsvd: reserved field used internally
> > + *
> > + * Structure used to describe the rw sysfs attribute to
> > + * igt_sysfs_rw_attr_verify
> > + */
> > +typedef struct igt_sysfs_rw_attr {
> > +	int dir;
> > +	char *attr;
> > +	uint64_t start;
> > +	double tol;
> > +	void *rsvd;
> > +} igt_sysfs_rw_attr_t;
> > +
> > +void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw);
> > +
> >   #endif /* __IGT_SYSFS_H__ */