[Intel-gfx] [PATCH] drm/i915: read/write IOCTLs

[Chris Wilson]

On Fri, 1 Apr 2011 00:06:37 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> 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.

I think most of that can and should be done in userspace. Eventually we
should have the list of registers and be able to use the spec names and
have those translated into the correct offset for the chipset.

Has anyone succeeded in getting a machine-readable description of the
registers? Or am I just dreaming that we have real documentation
somewhere?

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

Slightly better in avoiding the kernel killer. ;-)

I'm just not happy about haphazard locking. Can we do simple and safe
locking and revisit it if a real use-case for brute-forcing the read/write
is found?

> > > +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.

There's an equivalent get_taint(), but we have no way of knowing if that
was us or not. We do need to taint the oops (as having the hardware change
behind our backs is evil personified). When were you thinking of using
dev_priv->user_tainted?

> > > +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?

It's not the performance I care about, but the atomicity. There seem to be
a growing abundance of messaging systems within the chip driven by
combinations of registers. I'd feel happier if we could send a message
without fouling or being fouled by the kernel.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre