Re: [igt-dev] [PATCH i-g-t] lib/i915: Add a helper lib to read mmio registers via ioctl

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

Thanks the reviewing Kamil

On Wed, 2023-08-16 at 16:27 +0200, Kamil Konieczny wrote:
> Hi Alan,
> 
> On 2023-08-15 at 10:36:59 -0700, Alan Previn wrote:
> > Add a helper lib function to call DRM_IOCTL_I915_REG_READ to
> > read whitelisted registers.
> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > ---
> >  lib/i915/i915_reg_read_ioctl.c | 34 ++++++++++++++++++++++++++++++++++
> ---------------- ^^^^^^^^^^^^^^
> 
> imho better name would be i915_mmio_reg
alan: i assume we'll go with the other suggestion to move into the ioctl_wrapper.*


> > +/**
> > + * i915_reg_read_ioctl:
> > + * @drmfd: device file descriptor
> > + * @offset: mmio register to read from
> > + * @value: ptr to fill with results from read
> > + *
> > + * This function uses DRM_IOCTL_I915_REG_READ to read the
> > + * register and so it can return NOACCES if i915 doesn't allow.
> > + *
> > + * Returns:
> > + * -errno
> > + */
> > +int i915_reg_read_ioctl(int drmfd, __u64 offset, __u64 *value)
> 
> There is convention in igt to have "__function" without error asserts
> and "function" failing when ioctl fails, so maybe better is:
> 
> int __i915_reg_read_ioctl(int drmfd, __u64 offset, __u64 *value)

alna: okay - good point - i guess i should relook at the callers to see
if we need both exports (one wrapping the other) ... or not depending
if we already have users.
> 
> > +{
> > +	struct drm_i915_reg_read reg_read = {0};
> > +	int ret = 0;
> > +
> > +	reg_read.offset = offset;
> > +	if (igt_ioctl(drmfd, DRM_IOCTL_I915_REG_READ, &reg_read))
> > +		ret = -errno;
> > +	else if (value)
> > +		*value = reg_read.val;
> > +
> > +	return ret;
> > +}
> > +
> > 
> > 
> > 
[alan:snip]

> >   *
> > @@ -1852,13 +1855,11 @@ static void gem_evict_pwrite_subtest(void)
> >  /* This also triggered WARNs on dmesg at some point. */
> >  static void reg_read_ioctl_subtest(void)
> >  {
> > -	struct drm_i915_reg_read rr = {
> > -		.offset = 0x2358, /* render ring timestamp */
> > -	};
> > +	__u64 val;
> >  
> >  	disable_all_screens_and_wait(&ms_data);
[alan:snip]

> >  
> > -	do_ioctl(drm_fd, DRM_IOCTL_I915_REG_READ, &rr);
> > +	i915_reg_read_ioctl(drm_fd, 0x2358, &val);
> ----------------------------------- ^^^^^^
> Maybe define it at i915_mmio_reg.h ?
> 
> #define MMIO_REG_RENDER_RING_TIMESTAMP 0x2358
> 
alan: yes - my bad - i should have done that up front.