[Intel-gfx] [PATCH v3 1/2] drm/i915: read/write ioctls for userspace

Chris Wilson chris at chris-wilson.co.uk
Mon Apr 4 19:14:48 CEST 2011


Comments inline.

On Fri,  1 Apr 2011 16:54:48 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> Straight forward IOCTLs which allow userspace to read and write
> registers. This is needed because on newer generation products,
> registers may become inaccessible due to specific power modes.
> 
> To manage this, created a set of register ranges for the different
> products which can give fine grained control over what we permit
> userspace to read and write. This list was created by hand, so expect
> issues/bugs.
> 
> The fields in the calls allow register widths to be defined, although
> currently this is not used by any products. There is also a rsvd field
> which can be used for giving a list of register reads/writes to perform
> in the future.
> 
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |  189 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h |   21 +++++
>  include/drm/i915_drm.h          |   30 ++++++
>  3 files changed, 240 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 7273037..30cd576 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1846,6 +1846,174 @@ out_unlock:
>  }
>  EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
>  
> +static struct i915_register_range *
> +get_register_range(struct drm_device *dev, u32 offset, int mode)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_register_range *range = dev_priv->register_map.map;
> +	u8 align = dev_priv->register_map.alignment_mask;
> +	u32 range_count = dev_priv->register_map.length;
> +
> +	if (offset & dev_priv->register_map.alignment_mask)
> +		return NULL;
> +
> +	if (offset >= dev_priv->register_map.top)
> +		return NULL;
> +
> +	while(range_count--) {
Missingwhitespace.;-)
Rather than range_count/dev_priv->register_map.length, I would prefer a
sentinel.

> +		/*  list is assumed to be in order */
> +		if (offset < range->base)
> +			break;
> +
> +		if ( (offset >= range->base) &&
> +		     (offset + align) <= (range->base + range->size)) {
> +			/*  assume perms ascend in security (ie. rw is > ro) */
> +			if (mode > range->flags)
> +				return NULL;
> +			else
> +				return range;
> +		}
> +		range++;
> +	}
> +
> +	return NULL;
> +}
> +
> +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;
> +
> +	if (args->rsvd != 0)
> +		DRM_DEBUG("rsvd field should be zero\n");

No, don't even mention it, just ignore it.

> +
> +	if (get_register_range(dev, args->offset, I915_RANGE_RO) == NULL) {
> +		args->value = 0xffffffff;
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&dev->struct_mutex);

Will be happier with i915_mutex_lock_interruptible() just in case.

> +	args->value = (u32)i915_gt_read(dev_priv, args->offset);
And we need to start doing
intel_register_read_get(dev, args->offset);
switch (args->size) {
switch 8: args->value = ioread8(regs+args->offset); break;
...
}
intel_register_read_put(dev, args->offset);

where intel_register_read_get() looks something like

int intel_register_read_get(struct drm_device *dev,
                            int offset)
{
	switch (INTEL_INFO(dev)->gen) {
	case 6:
		if (offset < 0x40000)
			__gen6_gt_force_wake_get(dev);
		return 0;

	case 5:
	case 4:
 	case 3:
		return 0;
	}
}

> +	mutex_unlock(&dev->struct_mutex);
> +
> +	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;
> +	struct i915_register_range *range;
> +
> +	if (args->rsvd != 0)
> +		DRM_DEBUG("rsvd field should be zero\n");
> +
> +	range = get_register_range(dev, args->offset, I915_RANGE_RW);
> +	if (!range)
> +		return -EINVAL;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
> +	range->user_tainted = true;
> +	i915_gt_write(dev_priv, args->offset, (u32)args->value);
ret = intel_register_write_get()
if (ret == 0) {
	switch (args->size) {
		case 8: iowrite8(args->value, regs+args->offset); break;
	}
	intel_register_write_put()
}
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
> +}
> +
> +struct i915_register_range gen_bwcl_register_map[] = {
static const struct i915_register_range ...

Keep sparse quiet.

> +	{0x00000000, 0x00000fff, I915_RANGE_RW},
> +	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x00002000, 0x00000fff, I915_RANGE_RW},
> +	{0x00003000, 0x000001ff, I915_RANGE_RW},
> +	{0x00003200, 0x00000dff, I915_RANGE_RW},
> +	{0x00004000, 0x000003ff, I915_RANGE_RSVD},
> +	{0x00004400, 0x00000bff, I915_RANGE_RSVD},
> +	{0x00005000, 0x00000fff, I915_RANGE_RW},
> +	{0x00006000, 0x00000fff, I915_RANGE_RW},
> +	{0x00007000, 0x000003ff, I915_RANGE_RW},
> +	{0x00007400, 0x000014ff, I915_RANGE_RW},
> +	{0x00008900, 0x000006ff, I915_RANGE_RSVD},
> +	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x0000a000, 0x00000fff, I915_RANGE_RW},
> +	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
> +	{0x00010000, 0x00003fff, I915_RANGE_RW},
> +	{0x00014000, 0x0001bfff, I915_RANGE_RSVD},
> +	{0x00030000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
> +	{0x00060000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00070000, 0x00002fff, I915_RANGE_RW},
> +	{0x00073000, 0x00000fff, I915_RANGE_RW},
> +	{0x00074000, 0x0000bfff, I915_RANGE_RSVD}
> +};
> +
> +struct i915_register_range genx_register_map[] = {
> +	{0x00000000, 0x00000fff, I915_RANGE_RW},
> +	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x00002000, 0x00000fff, I915_RANGE_RW},
> +	{0x00003000, 0x000001ff, I915_RANGE_RW},
> +	{0x00003200, 0x00000dff, I915_RANGE_RW},
> +	{0x00004000, 0x000003ff, I915_RANGE_RW},
> +	{0x00004400, 0x00000bff, I915_RANGE_RW},
> +	{0x00005000, 0x00000fff, I915_RANGE_RW},
> +	{0x00006000, 0x00000fff, I915_RANGE_RW},
> +	{0x00007000, 0x000003ff, I915_RANGE_RW},
> +	{0x00007400, 0x000014ff, I915_RANGE_RW},
> +	{0x00008900, 0x000006ff, I915_RANGE_RSVD},
> +	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x0000a000, 0x00000fff, I915_RANGE_RW},
> +	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
> +	{0x00010000, 0x00003fff, I915_RANGE_RW},
> +	{0x00014000, 0x0001bfff, I915_RANGE_RSVD},
> +	{0x00030000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
> +	{0x00060000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00070000, 0x00002fff, I915_RANGE_RW},
> +	{0x00073000, 0x00000fff, I915_RANGE_RW},
> +	{0x00074000, 0x0000bfff, I915_RANGE_RSVD}
> +};
> +
> +struct i915_register_range gen6_gt_register_map[] = {
> +	{0x00000000, 0x00000fff, I915_RANGE_RW},
> +	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x00002000, 0x00000fff, I915_RANGE_RW},
> +	{0x00003000, 0x000001ff, I915_RANGE_RW},
> +	{0x00003200, 0x00000dff, I915_RANGE_RW},
> +	{0x00004000, 0x00000fff, I915_RANGE_RW},
> +	{0x00005000, 0x0000017f, I915_RANGE_RW},
> +	{0x00005180, 0x00000e7f, I915_RANGE_RW},
> +	{0x00006000, 0x00001fff, I915_RANGE_RW},
> +	{0x00008000, 0x000007ff, I915_RANGE_RW},
> +	{0x00008800, 0x000000ff, I915_RANGE_RSVD},
> +	{0x00008900, 0x000006ff, I915_RANGE_RW},
> +	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x0000a000, 0x00000fff, I915_RANGE_RW},
> +	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
> +	{0x00010000, 0x00001fff, I915_RANGE_RW},
> +	{0x00012000, 0x000003ff, I915_RANGE_RW},
> +	{0x00012400, 0x00000bff, I915_RANGE_RW},
> +	{0x00013000, 0x00000fff, I915_RANGE_RW},
> +	{0x00014000, 0x00000fff, I915_RANGE_RW},
> +	{0x00015000, 0x0000cfff, I915_RANGE_RW},
> +	{0x00022000, 0x00000fff, I915_RANGE_RW},
> +	{0x00023000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x00024000, 0x00000fff, I915_RANGE_RW},
> +	{0x00025000, 0x0000afff, I915_RANGE_RSVD},
> +	{0x00030000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
> +	{0x00060000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00070000, 0x00002fff, I915_RANGE_RW},
> +	{0x00073000, 0x00000fff, I915_RANGE_RW},
> +	{0x00074000, 0x0008bfff, I915_RANGE_RSVD},
> +	{0x00100000, 0x00007fff, I915_RANGE_RW},
> +	{0x00108000, 0x00037fff, I915_RANGE_RSVD},
> +	{0x00140000, 0x0003ffff, I915_RANGE_RW}
> +};
> +
>  /**
>   * Tells the intel_ips driver that the i915 driver is now loaded, if
>   * IPS got loaded first.
> @@ -2062,6 +2230,25 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	ips_ping_for_i915_load();
>  
> +	if (IS_GEN6(dev)) {
> +		dev_priv->register_map.map = gen6_gt_register_map;
> +		dev_priv->register_map.length =
> +			ARRAY_SIZE(gen6_gt_register_map);
> +		dev_priv->register_map.top = 0x180000;
> +	} else if (IS_BROADWATER(dev) || IS_CRESTLINE(dev)) {
> +		dev_priv->register_map.map = gen_bwcl_register_map;
> +		dev_priv->register_map.length =
> +			ARRAY_SIZE(gen_bwcl_register_map);
> +		dev_priv->register_map.top = 0x80000;
> +	} else {
> +		dev_priv->register_map.map = genx_register_map;

Wow, didn't expect the gen5 register map to match gen2.

> +		dev_priv->register_map.length =
> +			ARRAY_SIZE(genx_register_map);
> +		dev_priv->register_map.top = 0x80000;
> +	}
> +
> +	dev_priv->register_map.alignment_mask = 0x3;
> +
>  	return 0;
>  
>  out_gem_unload:
> @@ -2276,6 +2463,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..355f008 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -703,6 +703,14 @@ typedef struct drm_i915_private {
>  	struct intel_fbdev *fbdev;
>  
>  	struct drm_property *broadcast_rgb_property;
> +
> +	/* User visible register map */
> +	struct {
> +		struct i915_register_range *map;
> +		u32 length;
As noted we can remove this. I hope.

> +		u32 top;
> +		u8 alignment_mask;
> +	} register_map;
>  } drm_i915_private_t;
>  
>  struct drm_i915_gem_object {
> @@ -1385,4 +1393,17 @@ static inline void i915_gt_write(struct drm_i915_private *dev_priv,
>  		__gen6_gt_wait_for_fifo(dev_priv);
>  	I915_WRITE(reg, val);
>  }
> +
> +/* Register range interface */
> +struct i915_register_range {
> +	u32 base;
> +	u32 size;
> +	u8 flags;
> +	bool user_tainted;
Save a puppy, merge this into flags.

> +};
> +
> +#define I915_RANGE_RSVD	(0<<0)
> +#define I915_RANGE_RO	(1<<0)
> +#define I915_RANGE_RW	(1<<1)
Mind renaming this as I915_RANGE_READ, I915_RANGE_WRITE and use them as
distinct permission bits and then I915_RANGE_RW is (I915_RANGE_READ |
I915_RANGE_WRITE). Yes, there are the occasional write-only registers out
there.

> +
>  #endif
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index c4d6dbf..9398cb2 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,30 @@ struct drm_intel_overlay_attrs {
>  	__u32 gamma5;
>  };
>  
> +struct drm_intel_read_reg {
> +	/* register offset to read */
> +	__u32 offset;
> +
> +	/* register size, RFU */
> +	__u8 size;

Make this a _u32 for alignment's sake. Leave a note saying that are a
spare 26 bits if you want ;-)

> +
> +	/* return value, high 4 bytes RFU */
> +	__u64 value;
> +
> +	__u64 rsvd;
> +};
> +
> +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;
> +
> +	__u64 rsvd;
> +};
> +
>  #endif				/* _I915_DRM_H_ */
> -- 
> 1.7.3.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Chris Wilson, Intel Open Source Technology Centre



More information about the Intel-gfx mailing list