From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH] drm/i915: read/write IOCTLs Date: Fri, 01 Apr 2011 08:32:09 +0100 Message-ID: <1bdc18$k2r37b@fmsmga002.fm.intel.com> References: <1301621509-23107-1-git-send-email-ben@bwidawsk.net> <20110401070636.GA23731@snipes.kumite> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 41F419E830 for ; Fri, 1 Apr 2011 00:32:12 -0700 (PDT) In-Reply-To: <20110401070636.GA23731@snipes.kumite> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, 1 Apr 2011 00:06:37 -0700, Ben Widawsky 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