| Message ID | 1342051656-32481-1-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
On Thu, 12 Jul 2012 09:58:43 +0200, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Jul 11, 2012 at 05:07:36PM -0700, Ben Widawsky wrote: > 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. Do you want to allow people to read a subregister? That's the only reason I see to have userspace pass in a size, and extracting a field from a register can be trivially done in userspace (and will be done as part of the normal process of extracting a value from the result). Reading 8,16,32,64 bits all generate a 64-bit read cycle so should be immaterial performance wise. -Chris
Ben Widawsky <ben@bwidawsk.net> writes: > 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. Thanks. I was just writing this patch yesterday since it still hadn't landed. What I was doing was very similar, I was just not including a size, since we're going to whitelist regs and the correct size is implied by the register offset. > +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; Should this be conditional on the gen having the timestamp register? > +struct drm_i915_reg_read { > + __u64 offset; > + __u32 size; > + __u64 val; /* Return value */ > + __u32 pad; > +}; Bad padding here. On i386 you'll get a struct like: { uint64_t offset uint32_t size uint32_t implicit_pad uint64_t val uint32_t pad }
On Thu, 12 Jul 2012 12:42:30 -0700 Eric Anholt <eric@anholt.net> wrote: > Ben Widawsky <ben@bwidawsk.net> writes: > > > 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. > > Thanks. I was just writing this patch yesterday since it still hadn't > landed. What I was doing was very similar, I was just not including a > size, since we're going to whitelist regs and the correct size is > implied by the register offset. Please check out: 1342116066-12164-1-git-send-email-ben@bwidawsk.net We went through this all on IRC already ;-) > > > +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; > > Should this be conditional on the gen having the timestamp register? > > > +struct drm_i915_reg_read { > > + __u64 offset; > > + __u32 size; > > + __u64 val; /* Return value */ > > + __u32 pad; > > +}; > > Bad padding here. On i386 you'll get a struct like: > > { > uint64_t offset > uint32_t size > uint32_t implicit_pad > uint64_t val > uint32_t pad > }
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; + + 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; +}; #endif /* _I915_DRM_H_ */
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> --- 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(+)