Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET
Quoting Chris Wilson (2019-10-07 21:16:29) > Quoting Bloomfield, Jon (2019-10-07 17:55:16) > > > -Original Message- > > > From: Intel-gfx On Behalf Of > > > Abdiel > > > Janulgue > > > Sent: Monday, October 7, 2019 2:19 AM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Auld, Matthew > > > 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 > > > Signed-off-by: Matthew Auld > > > Cc: Joonas Lahtinen > > > --- > > > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET
Quoting Bloomfield, Jon (2019-10-07 17:55:16) > > -Original Message- > > From: Intel-gfx On Behalf Of > > Abdiel > > Janulgue > > Sent: Monday, October 7, 2019 2:19 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Auld, Matthew > > 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 > > Signed-off-by: Matthew Auld > > Cc: Joonas Lahtinen > > --- > > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET
> -Original Message- > From: Intel-gfx On Behalf Of Abdiel > Janulgue > Sent: Monday, October 7, 2019 2:19 AM > To: intel-gfx@lists.freedesktop.org > Cc: Auld, Matthew > 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 > Signed-off-by: Matthew Auld > Cc: Joonas Lahtinen > --- > 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. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET
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_DESTROY0x3b
> +#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_INITDRM_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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
