Re: [PATCH] drm/i915: read/write IOCTLs

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Apr 01, 2011 at 07:36:56AM +0100, Chris Wilson wrote:
> Nice. The consensus is that this ioctl is required, just a few comments
> inline.

Nobody likes it, but it could pay off, especially if we end up with
weird non-mappable registers we wish to allow user space to read. I had
actually decided to add another field (something like a bus) to the
interface just in case.

> > +static inline int
> > +reg_in_rsvd_range(struct drm_device *dev, u32 reg)
> > +{
> > +	if (INTEL_INFO(dev)->gen < 6) {
> > +		if (REG_IN_RANGE(reg, 0x1000, 0x1fff) ||
> > +		    ((IS_BROADWATER(dev) || IS_CRESTLINE(dev)) ?
> > +			REG_IN_RANGE(reg, 0x4000, 0x4fff) : 0) ||
> > +		    REG_IN_RANGE(reg, 0x9000, 0x9fff) ||
> > +		    REG_IN_RANGE(reg, 0xb000, 0xffff) ||
> > +		    REG_IN_RANGE(reg, 0x14000, 0x2ffff) ||
> > +		    REG_IN_RANGE(reg, 0x40000, 0x5ffff))
> > +			return 1;
> > +
> > +		if (reg > 0x73fff)
> > +			return 1;
> > +	} else {
> > +		if (REG_IN_RANGE(reg, 0x1000, 0x1fff) ||
> > +		    REG_IN_RANGE(reg, 0x9000, 0x9fff) ||
> > +		    REG_IN_RANGE(reg, 0xb000, 0xffff) ||
> > +		    REG_IN_RANGE(reg, 0x15000, 0x21fff) ||
> > +		    REG_IN_RANGE(reg, 0x23000, 0x23fff) ||
> > +		    REG_IN_RANGE(reg, 0x25000, 0x2ffff) ||
> > +		    REG_IN_RANGE(reg, 0x40000, 0xfffff) ||
> > +		    REG_IN_RANGE(reg, 0x108000, 0x13ffff))
> > +			return 1;
> > +
> > +		if (reg > 0x17ffff)
> > +			return 1;
> > +	}
> 
> I think this would be more maintainable as a set of tables with the
> ranges. And a r/w flag could be stored in the table for the individual
> read/write blacklists.

That's fine with me. When I started it was much less complicated than by
the time I submitted the patch.

> > +	if (ret)
> > +		DRM_DEBUG_DRIVER("Couldn't acquire mutex, reading anyway\n");
> > +
> > +	args->value = (u32)i915_gt_read(dev_priv, args->offset);
> > +
> > +	mutex_unlock(&dev->struct_mutex);
> 
> Don't even think about unlocking an unlocked mutex. We either apply
> strict locking or we don't bother. I think we can assume that in the
> scenario where the mutex is blocked (from experience that is only from an
> OOPS within i915.ko) we can play with the registers to our hearts contents
> directly from userspace.

This was a bug in my patch... you mean?
ret = mutex_lock_interruptible();
if (ret)
	reg_read

if (!ret)
	mutex_unlock()

> > +static int
> > +i915_write_register_ioctl(struct drm_device *dev, void *data,
> > +			  struct drm_file *file_priv)
> > +{
> > +	struct drm_intel_read_reg *args = data;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret;
> > +
> > +	if (!reg_write_allowed(dev, args->offset))
> > +		return -EINVAL;
> > +
> > +	ret = i915_mutex_lock_interruptible(dev);
> > +	if (ret)
> > +		DRM_DEBUG_DRIVER("Couldn't acquire mutex, writing anyway\n");
> > +
> > +	DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
> > +	dev_priv->user_tainted = true;
> 
> Nice touch. Let's try add_taint(TAINT_USER) instead.

Cool! I didn't know about that interface. I'd like to be able to query
this at runtime as opposed to an OOPS... I'm too tired to look if it
supports that right now, but if it does, I'll switch.

> >  
> > +struct drm_intel_read_reg {
> > +	/* register offset to read */
> > +	__u32 offset;
> > +
> > +	/* register size, RFU */
> > +	__u8 size;
> > +
> > +	/* return value, high 4 bytes RFU */
> > +	__u64 value;
> > +};
> 
> Haha! I was going to ask for this to handle arbitrary register sizes.
> What I think is also necessary is to be able to read/write a block of
> registers in a single call - thinking of some of the more complex
> procedures we may want to try from userspace (the pcode messages for
> example) or for snapshotting a set of registers.

I brought this question up at our meeting on Thursday. The consensus to
was just have the 1 register at a time because the overhead of the
syscall was small enough to make it not matter. I think I prefer the
single read/write a little bit just because it keeps the kernel code
simpler, but I don't care enough to argue, and I most certainly don't
care enough to set up some kind of performance test for it. Anyone else
care enough to argue?

> 
> My dream would be a small domain specific language that would allow me to
> write a set of registers and poll for a result and then return that
> result. In the meantime I'd settle for passing an array of registers to be
> written and read in a single ioctl.
> 

GFX User mode driver? :)

> -- 
> Chris Wilson, Intel Open Source Technology Centre

Thanks for the feedback. I'll try to fix it up when I wake up.

Ben