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 at anholt.net>
> Cc: Jacek Lawrynowicz <jacek.lawrynowicz at intel.com>,
> Signed-off-by: Ben Widawsky <ben at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48
