Re: [PATCH] drm/i915: add register read IOCTL

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 11, 2012 at 05:07:36PM -0700, Ben Widawsky wrote:
> The interface's immediate purpose is to do synchronous timestamp queries
> as required by GL_TIMESTAMP. The GPU has a register for reading the
> timestamp but because that would normally require root access, the
> IOCTL can provide this service.
> 
> Currently the implementation whitelists only the render ring timestamp
> register, because that is the only thing we need to expose at this time.
> 
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>,
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Just two minor things below.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_dma.c |  1 +
>  drivers/gpu/drm/i915/i915_drv.c | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  include/drm/i915_drm.h          |  8 ++++++++
>  5 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f64ef4b..5e20f11 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1851,6 +1851,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_AUTH|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
>  };
>  
>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e754cdf..ae52326 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1152,3 +1152,33 @@ __i915_write(16, w)
>  __i915_write(32, l)
>  __i915_write(64, q)
>  #undef __i915_write
> +
> +int i915_reg_read_ioctl(struct drm_device *dev,
> +			void *data, struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_reg_read *reg = data;
> +
> +	/* Whitelisted for now */
> +	if (reg->offset != RING_TIMESTAMP(RENDER_RING_BASE))
> +		return -ENXIO;

I think we should check for both reg offset _and_ size. Just to avoid
people reading 64bit for a 32bit reg to get at the secret stuff in the
next reg ;-) Or in case that the hw has strange semantics if you don't
read the right size (I've seen that). Imo just creating a little table
with { offset; size; } pairs would be good enough.

> +
> +	switch (reg->size) {
> +	case 8:
> +		reg->val = I915_READ64(reg->offset);
> +		break;
> +	case 4:
> +		reg->val = I915_READ(reg->offset);
> +		break;
> +	case 2:
> +		reg->val = I915_READ16(reg->offset);
> +		break;
> +	case 1:
> +		reg->val = I915_READ8(reg->offset);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 627fe35..2c0d840 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1529,6 +1529,8 @@ extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
>  extern int intel_enable_rc6(const struct drm_device *dev);
>  
>  extern bool i915_semaphore_is_enabled(struct drm_device *dev);
> +int i915_reg_read_ioctl(struct drm_device *dev, void *data,
> +			struct drm_file *file);
>  
>  /* overlay */
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cc82871..33e66cc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -449,6 +449,7 @@
>  #define RING_ACTHD(base)	((base)+0x74)
>  #define RING_NOPID(base)	((base)+0x94)
>  #define RING_IMR(base)		((base)+0xa8)
> +#define RING_TIMESTAMP(base)	((base)+0x358)
>  #define   TAIL_ADDR		0x001FFFF8
>  #define   HEAD_WRAP_COUNT	0xFFE00000
>  #define   HEAD_WRAP_ONE		0x00200000
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index 8cc7083..6538d8b 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -203,6 +203,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_I915_GEM_WAIT	0x2c
>  #define DRM_I915_GEM_CONTEXT_CREATE	0x2d
>  #define DRM_I915_GEM_CONTEXT_DESTROY	0x2e
> +#define DRM_I915_REG_READ	0x30
>  
>  #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)
> @@ -249,6 +250,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_GEM_WAIT		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_WAIT, struct drm_i915_gem_wait)
>  #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
>  #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
> +#define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
>  
>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>   * on the security mechanisms provided by hardware.
> @@ -918,4 +920,10 @@ struct drm_i915_gem_context_destroy {
>  	__u32 pad;
>  };
>  
> +struct drm_i915_reg_read {
> +	__u64 offset;
> +	__u32 size;
> +	__u64 val; /* Return value */
> +	__u32 pad;

The padding is at the wrong place ...

> +};
>  #endif				/* _I915_DRM_H_ */
> -- 
> 1.7.11.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48