| Message ID | 20191007091920.2176-3-abdiel.janulgue@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/5] drm/i915: Allow i915 to manage the vma offset nodes instead of drm core | expand |
Quoting Abdiel Janulgue (2019-10-07 10:19:18) > +static int gem_mmap_offset(struct drm_device *dev, void *data, > + struct drm_file *file) > +{ > + struct drm_i915_gem_mmap_offset *args = data; > + enum i915_mmap_type type; > + > + if ((args->flags & (I915_MMAP_OFFSET_WC | I915_MMAP_OFFSET_WB)) && _WB??? What's the default behaviour for every phys page in the system? -Chris
Quoting Abdiel Janulgue (2019-10-07 10:19:18) > enum i915_mmap_type { > I915_MMAP_TYPE_GTT = 0, > + I915_MMAP_TYPE_OFFSET_WC, > + I915_MMAP_TYPE_OFFSET_WB, > + I915_MMAP_TYPE_OFFSET_UC, _OFFSET_ is worse than redundant. -Chris
Quoting Abdiel Janulgue (2019-10-07 10:19:18) > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index cc70aba6ac26..9182da57182b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -2696,7 +2696,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = { > DRM_IOCTL_DEF_DRV(I915_GEM_PREAD, i915_gem_pread_ioctl, DRM_RENDER_ALLOW), > DRM_IOCTL_DEF_DRV(I915_GEM_PWRITE, i915_gem_pwrite_ioctl, DRM_RENDER_ALLOW), > DRM_IOCTL_DEF_DRV(I915_GEM_MMAP, i915_gem_mmap_ioctl, DRM_RENDER_ALLOW), > - DRM_IOCTL_DEF_DRV(I915_GEM_MMAP_GTT, i915_gem_mmap_gtt_ioctl, DRM_RENDER_ALLOW), > + DRM_IOCTL_DEF_DRV(I915_GEM_MMAP_OFFSET, i915_gem_mmap_gtt_ioctl, DRM_RENDER_ALLOW), Ahem. -Chris
Quoting Abdiel Janulgue (2019-10-07 10:19:18) > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 30c542144016..bc85656ab7fd 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -359,6 +359,7 @@ typedef struct _drm_i915_sarea { > #define DRM_I915_QUERY 0x39 > #define DRM_I915_GEM_VM_CREATE 0x3a > #define DRM_I915_GEM_VM_DESTROY 0x3b > +#define DRM_I915_GEM_MMAP_OFFSET DRM_I915_GEM_MMAP_GTT Put the alias next to its value. Or just not bother with an alias since that information is encoded into the ioctl. > /* Must be kept compact -- no holes */ > > #define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t) > @@ -421,6 +422,7 @@ typedef struct _drm_i915_sarea { > #define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY, struct drm_i915_query) > #define DRM_IOCTL_I915_GEM_VM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_VM_CREATE, struct drm_i915_gem_vm_control) > #define DRM_IOCTL_I915_GEM_VM_DESTROY DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_VM_DESTROY, struct drm_i915_gem_vm_control) > +#define DRM_IOCTL_I915_GEM_MMAP_OFFSET DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MMAP_OFFSET, struct drm_i915_gem_mmap_offset) > > /* Allow drivers to submit batchbuffers directly to hardware, relying > * on the security mechanisms provided by hardware. > @@ -611,6 +613,7 @@ typedef struct drm_i915_irq_wait { > * See I915_EXEC_FENCE_OUT and I915_EXEC_FENCE_SUBMIT. > */ > #define I915_PARAM_HAS_EXEC_SUBMIT_FENCE 53 > + Gratuitous. > /* Must be kept compact -- no holes and well documented */ > > typedef struct drm_i915_getparam { > @@ -786,6 +789,31 @@ struct drm_i915_gem_mmap_gtt { > __u64 offset; > }; > > +struct drm_i915_gem_mmap_offset { > + /** Handle for the object being mapped. */ > + __u32 handle; > + __u32 pad; > + /** > + * Fake offset to use for subsequent mmap call > + * > + * This is a fixed-size type for 32/64 compatibility. > + */ > + __u64 offset; > + > + /** > + * Flags for extended behaviour. > + * > + * It is mandatory that either one of the _WC/_WB flags > + * should be passed here. > + */ > + __u64 flags; > +#define I915_MMAP_OFFSET_WC (1 << 0) > +#define I915_MMAP_OFFSET_WB (1 << 1) > +#define I915_MMAP_OFFSET_UC (1 << 2) > +#define I915_MMAP_OFFSET_FLAGS \ > + (I915_MMAP_OFFSET_WC | I915_MMAP_OFFSET_WB | I915_MMAP_OFFSET_UC) You've forgotten the i915_user_extension. -Chris
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Abdiel > Janulgue > Sent: Monday, October 7, 2019 2:19 AM > To: intel-gfx@lists.freedesktop.org > Cc: Auld, Matthew <matthew.auld@intel.com> > Subject: [Intel-gfx] [PATCH v2 3/5] drm/i915: Introduce > DRM_I915_GEM_MMAP_OFFSET > > This is really just an alias of mmap_gtt. Add a new CPU mmap > implementation that allows multiple fault handlers that depends on > the object's backing pages. > > Note that we multiplex mmap_gtt and mmap_offset through the same ioctl, > and use the zero extending behaviour of drm to differentiate between > them, when we inspect the flags. > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 36 +++++++++++++++++-- > .../gpu/drm/i915/gem/i915_gem_object_types.h | 3 ++ > drivers/gpu/drm/i915/i915_drv.c | 2 +- > include/uapi/drm/i915_drm.h | 28 +++++++++++++++ > 4 files changed, 66 insertions(+), 3 deletions(-) How does the label 'offset' fit into this API if it's really about multiple fault handlers? Could do with a much better description here I think. Who would use this, and why, would help a lot.
Quoting Bloomfield, Jon (2019-10-07 17:55:16) > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Abdiel > > Janulgue > > Sent: Monday, October 7, 2019 2:19 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Auld, Matthew <matthew.auld@intel.com> > > Subject: [Intel-gfx] [PATCH v2 3/5] drm/i915: Introduce > > DRM_I915_GEM_MMAP_OFFSET > > > > This is really just an alias of mmap_gtt. Add a new CPU mmap > > implementation that allows multiple fault handlers that depends on > > the object's backing pages. > > > > Note that we multiplex mmap_gtt and mmap_offset through the same ioctl, > > and use the zero extending behaviour of drm to differentiate between > > them, when we inspect the flags. > > > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 36 +++++++++++++++++-- > > .../gpu/drm/i915/gem/i915_gem_object_types.h | 3 ++ > > drivers/gpu/drm/i915/i915_drv.c | 2 +- > > include/uapi/drm/i915_drm.h | 28 +++++++++++++++ > > 4 files changed, 66 insertions(+), 3 deletions(-) > > How does the label 'offset' fit into this API if it's really about multiple fault handlers? > Could do with a much better description here I think. Who would use this, and why, would help a lot. The ioctl returns the offset into the device fd userpace uses with mmap(2). Hence DRM_IOCTL_I915_GEM_MMAP_OFFSET. -Chris
Quoting Chris Wilson (2019-10-07 21:16:29) > Quoting Bloomfield, Jon (2019-10-07 17:55:16) > > > -----Original Message----- > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Abdiel > > > Janulgue > > > Sent: Monday, October 7, 2019 2:19 AM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Auld, Matthew <matthew.auld@intel.com> > > > Subject: [Intel-gfx] [PATCH v2 3/5] drm/i915: Introduce > > > DRM_I915_GEM_MMAP_OFFSET > > > > > > This is really just an alias of mmap_gtt. Add a new CPU mmap > > > implementation that allows multiple fault handlers that depends on > > > the object's backing pages. > > > > > > Note that we multiplex mmap_gtt and mmap_offset through the same ioctl, > > > and use the zero extending behaviour of drm to differentiate between > > > them, when we inspect the flags. > > > > > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 36 +++++++++++++++++-- > > > .../gpu/drm/i915/gem/i915_gem_object_types.h | 3 ++ > > > drivers/gpu/drm/i915/i915_drv.c | 2 +- > > > include/uapi/drm/i915_drm.h | 28 +++++++++++++++ > > > 4 files changed, 66 insertions(+), 3 deletions(-) > > > > How does the label 'offset' fit into this API if it's really about multiple fault handlers? > > Could do with a much better description here I think. Who would use this, and why, would help a lot. > > The ioctl returns the offset into the device fd userpace uses with > mmap(2). Hence DRM_IOCTL_I915_GEM_MMAP_OFFSET. Yeah, that should have been explained in the changelog why the name was chosen to reflect expected usage. -Chris
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index af4377a80502..ae1e687408da 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -144,6 +144,9 @@ static unsigned int tile_row_pages(const struct drm_i915_gem_object *obj) * 3 - Remove implicit set-domain(GTT) and synchronisation on initial * pagefault; swapin remains transparent. * + * 4 - Support multiple fault handlers per object depending on object's + * backing storage (a.k.a. MMAP_OFFSET). + * * Restrictions: * * * snoopable objects cannot be accessed via the GTT. It can cause machine @@ -171,7 +174,7 @@ static unsigned int tile_row_pages(const struct drm_i915_gem_object *obj) */ int i915_gem_mmap_gtt_version(void) { - return 3; + return 4; } static inline struct i915_ggtt_view @@ -545,6 +548,27 @@ __assign_gem_object_mmap_data(struct drm_file *file, return ret; } +static int gem_mmap_offset(struct drm_device *dev, void *data, + struct drm_file *file) +{ + struct drm_i915_gem_mmap_offset *args = data; + enum i915_mmap_type type; + + if ((args->flags & (I915_MMAP_OFFSET_WC | I915_MMAP_OFFSET_WB)) && + !boot_cpu_has(X86_FEATURE_PAT)) + return -ENODEV; + + if (args->flags & I915_MMAP_OFFSET_WC) + type = I915_MMAP_TYPE_OFFSET_WC; + else if (args->flags & I915_MMAP_OFFSET_WB) + type = I915_MMAP_TYPE_OFFSET_WB; + else if (args->flags & I915_MMAP_OFFSET_UC) + type = I915_MMAP_TYPE_OFFSET_UC; + + return __assign_gem_object_mmap_data(file, args->handle, type, + &args->offset); +} + /** * i915_gem_mmap_gtt_ioctl - prepare an object for GTT mmap'ing * @dev: DRM device @@ -564,7 +588,15 @@ int i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data, struct drm_file *file) { - struct drm_i915_gem_mmap_gtt *args = data; + struct drm_i915_gem_mmap_offset *args = data; + struct drm_i915_private *i915 = to_i915(dev); + + if (args->flags & I915_MMAP_OFFSET_FLAGS) + return gem_mmap_offset(dev, data, file); + + if (!HAS_MAPPABLE_APERTURE(i915)) + /* No aperture, cannot mmap via legacy GTT */ + return -ENODEV; return __assign_gem_object_mmap_data(file, args->handle, I915_MMAP_TYPE_GTT, diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index 74544eca5e2c..18e891f4e9d3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -64,6 +64,9 @@ struct drm_i915_gem_object_ops { enum i915_mmap_type { I915_MMAP_TYPE_GTT = 0, + I915_MMAP_TYPE_OFFSET_WC, + I915_MMAP_TYPE_OFFSET_WB, + I915_MMAP_TYPE_OFFSET_UC, }; struct i915_mmap_offset { diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index cc70aba6ac26..9182da57182b 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2696,7 +2696,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = { DRM_IOCTL_DEF_DRV(I915_GEM_PREAD, i915_gem_pread_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_GEM_PWRITE, i915_gem_pwrite_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_GEM_MMAP, i915_gem_mmap_ioctl, DRM_RENDER_ALLOW), - DRM_IOCTL_DEF_DRV(I915_GEM_MMAP_GTT, i915_gem_mmap_gtt_ioctl, DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_MMAP_OFFSET, i915_gem_mmap_gtt_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_GEM_SET_DOMAIN, i915_gem_set_domain_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_GEM_SW_FINISH, i915_gem_sw_finish_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_GEM_SET_TILING, i915_gem_set_tiling_ioctl, DRM_RENDER_ALLOW), diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 30c542144016..bc85656ab7fd 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -359,6 +359,7 @@ typedef struct _drm_i915_sarea { #define DRM_I915_QUERY 0x39 #define DRM_I915_GEM_VM_CREATE 0x3a #define DRM_I915_GEM_VM_DESTROY 0x3b +#define DRM_I915_GEM_MMAP_OFFSET DRM_I915_GEM_MMAP_GTT /* Must be kept compact -- no holes */ #define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t) @@ -421,6 +422,7 @@ typedef struct _drm_i915_sarea { #define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY, struct drm_i915_query) #define DRM_IOCTL_I915_GEM_VM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_VM_CREATE, struct drm_i915_gem_vm_control) #define DRM_IOCTL_I915_GEM_VM_DESTROY DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_VM_DESTROY, struct drm_i915_gem_vm_control) +#define DRM_IOCTL_I915_GEM_MMAP_OFFSET DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MMAP_OFFSET, struct drm_i915_gem_mmap_offset) /* Allow drivers to submit batchbuffers directly to hardware, relying * on the security mechanisms provided by hardware. @@ -611,6 +613,7 @@ typedef struct drm_i915_irq_wait { * See I915_EXEC_FENCE_OUT and I915_EXEC_FENCE_SUBMIT. */ #define I915_PARAM_HAS_EXEC_SUBMIT_FENCE 53 + /* Must be kept compact -- no holes and well documented */ typedef struct drm_i915_getparam { @@ -786,6 +789,31 @@ struct drm_i915_gem_mmap_gtt { __u64 offset; }; +struct drm_i915_gem_mmap_offset { + /** Handle for the object being mapped. */ + __u32 handle; + __u32 pad; + /** + * Fake offset to use for subsequent mmap call + * + * This is a fixed-size type for 32/64 compatibility. + */ + __u64 offset; + + /** + * Flags for extended behaviour. + * + * It is mandatory that either one of the _WC/_WB flags + * should be passed here. + */ + __u64 flags; +#define I915_MMAP_OFFSET_WC (1 << 0) +#define I915_MMAP_OFFSET_WB (1 << 1) +#define I915_MMAP_OFFSET_UC (1 << 2) +#define I915_MMAP_OFFSET_FLAGS \ + (I915_MMAP_OFFSET_WC | I915_MMAP_OFFSET_WB | I915_MMAP_OFFSET_UC) +}; + struct drm_i915_gem_set_domain { /** Handle for the object */ __u32 handle;