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

[Chris Wilson <chris@chris-wilson.co.uk>]

Nice. The consensus is that this ioctl is required, just a few comments
inline.

On Thu, 31 Mar 2011 18:31:49 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> With the invention of forcewake (DevGT) , userspace tools to diagnose
> problems are no longer reliable. This will provide userspace a mechanism
> to read registers through an IOCTL, as well as root permission to write
> to registers.
> 
> The code tries to be smart about which registers can be read and
> written, as hardware behavior may become erratic if writing to reserved
> ranges.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |  129 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h |    2 +
>  include/drm/i915_drm.h          |   27 ++++++++
>  3 files changed, 158 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 7273037..8b5165d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1846,6 +1846,133 @@ out_unlock:
>  }
>  EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
>  
> +
> +/* If we assume registers are dword aligned, second check is redundant */
> +#define REG_IN_RANGE(reg, begin, end) \
> +	((reg <= end && reg >= begin) || \
> +	((reg+3) <= end && (reg+3) >= begin))
> +
> +/*
> + * This is here to avoid hanging the GPU from userspace.
> + * The ranges are defined the in "Register Address Map" section of the
> + * documents.
> + */
> +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.

> +
> +	return 0;
> +}
> +
> +
> +#define REG_IN_READ_BLACKLIST(reg) 0
> +#define REG_IN_WRITE_BLACKLIST(reg) 0
> +
> +static inline int
> +reg_read_allowed(struct drm_device *dev, u32 offset)
> +{
> +	if (offset & 0x3)
> +		return 0;
> +
> +	if (reg_in_rsvd_range(dev, offset))
> +		return 0;
> +
> +	if (REG_IN_READ_BLACKLIST(offset))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static inline int
> +reg_write_allowed(struct drm_device *dev, u32 offset)
> +{
> +	if (offset & 0x3)
> +		return 0;
> +
> +	if (reg_in_rsvd_range(dev, offset))
> +		return 0;
> +
> +	if (REG_IN_WRITE_BLACKLIST(offset))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static int
> +i915_read_register_ioctl(struct drm_device *dev, void *data,
> +			 struct drm_file *file_priv)
> +{
> +	struct drm_intel_write_reg *args = data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +
> +	if (!reg_read_allowed(dev, args->offset)) {
> +		args->value = 0xffffffff;
> +		return -EINVAL;
> +	}
> +
> +	ret = i915_mutex_lock_interruptible(dev);

I don't think you want i915_mutex_lock_interuptible() here just an
ordinary mutex_lock_interruptible() - we don't need or desire to check if
the GPU is wedged.

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

> +
> +	return 0;
> +}
> +
> +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.

> +
> +	i915_gt_write(dev_priv, args->offset, (u32)args->value);
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
> +}
> +
>  /**
>   * Tells the intel_ips driver that the i915 driver is now loaded, if
>   * IPS got loaded first.
> @@ -2276,6 +2403,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_OVERLAY_PUT_IMAGE, intel_overlay_put_image, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(I915_READ_REGISTER, i915_read_register_ioctl, DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(I915_WRITE_REGISTER, i915_write_register_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
>  };
>  
>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5004724..4c66734 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -703,6 +703,8 @@ typedef struct drm_i915_private {
>  	struct intel_fbdev *fbdev;
>  
>  	struct drm_property *broadcast_rgb_property;
> +
> +	bool user_tainted;
>  } drm_i915_private_t;
>  
>  struct drm_i915_gem_object {
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index c4d6dbf..68eeaa3 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -198,6 +198,8 @@ typedef struct _drm_i915_sarea {
>  #define DRM_I915_OVERLAY_PUT_IMAGE	0x27
>  #define DRM_I915_OVERLAY_ATTRS	0x28
>  #define DRM_I915_GEM_EXECBUFFER2	0x29
> +#define DRM_I915_READ_REGISTER	0x2a
> +#define DRM_I915_WRITE_REGISTER	0x2b
>  
>  #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
>  #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> @@ -239,6 +241,8 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_GEM_MADVISE	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MADVISE, struct drm_i915_gem_madvise)
>  #define DRM_IOCTL_I915_OVERLAY_PUT_IMAGE	DRM_IOW(DRM_COMMAND_BASE + DRM_IOCTL_I915_OVERLAY_ATTRS, struct drm_intel_overlay_put_image)
>  #define DRM_IOCTL_I915_OVERLAY_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
> +#define DRM_IOCTL_I915_READ_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_READ_REGISTER, struct drm_intel_read_reg)
> +#define DRM_IOCTL_I915_WRITE_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_WRITE_REGISTER, struct drm_intel_write_reg)
>  
>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>   * on the security mechanisms provided by hardware.
> @@ -844,4 +848,27 @@ struct drm_intel_overlay_attrs {
>  	__u32 gamma5;
>  };
>  
> +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.

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.

> +
> +struct drm_intel_write_reg {
> +	/* register offset to read */
> +	__u32 offset;
> +
> +	/* register size,  RFU */
> +	__u8 size;
> +
> +	/* value to write, high 4 bytes RFU */
> +	__u64 value;
> +
> +};
> +
>  #endif				/* _I915_DRM_H_ */
> -- 
> 1.7.3.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Chris Wilson, Intel Open Source Technology Centre