On Mon, Jul 12, 2021 at 9:38 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> On Mon, Jul 12, 2021 at 09:31:13AM -0500, Jason Ekstrand wrote:
> > On Mon, Jul 12, 2021 at 9:12 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
> > >
> > > On Mon, Jul 12, 2021 at 08:47:24AM -0500, Jason Ekstrand wrote:
> > > > On Fri, Jul 9, 2021 at 6:41 AM Maarten Lankhorst
> > > > <maarten.lankhorst@xxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > This is only used for ttm, and tells userspace that the mapping type is
> > > > > ignored. This disables the other type of mmap offsets when discrete
> > > > > memory is used, so fix the selftests as well.
> > > > >
> > > > > Document the struct as well, so it shows up in docbook correctly.
> > > > >
> > > > > Changes since v1:
> > > > > - Add docbook entries.
> > > > >
> > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 17 ++++++-
> > > > > .../gpu/drm/i915/gem/i915_gem_object_types.h | 1 +
> > > > > .../drm/i915/gem/selftests/i915_gem_mman.c | 27 +++++++++-
> > > > > include/uapi/drm/i915_drm.h | 51 +++++++++++++++----
> > > > > 4 files changed, 82 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > > index a90f796e85c0..b34be9e5d094 100644
> > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > > @@ -679,10 +679,16 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj,
> > > > > return -ENODEV;
> > > > >
> > > > > if (obj->ops->mmap_offset) {
> > > > > + if (mmap_type != I915_MMAP_TYPE_TTM)
> > > > > + return -ENODEV;
> > > > > +
> > > > > *offset = obj->ops->mmap_offset(obj);
> > > > > return 0;
> > > > > }
> > > > >
> > > > > + if (mmap_type == I915_MMAP_TYPE_TTM)
> > > > > + return -ENODEV;
> > > > > +
> > > > > if (mmap_type != I915_MMAP_TYPE_GTT &&
> > > > > !i915_gem_object_has_struct_page(obj) &&
> > > > > !i915_gem_object_has_iomem(obj))
> > > > > @@ -727,7 +733,9 @@ i915_gem_dumb_mmap_offset(struct drm_file *file,
> > > > > {
> > > > > enum i915_mmap_type mmap_type;
> > > > >
> > > > > - if (boot_cpu_has(X86_FEATURE_PAT))
> > > > > + if (HAS_LMEM(to_i915(dev)))
> > > > > + mmap_type = I915_MMAP_TYPE_TTM;
> > > > > + else if (boot_cpu_has(X86_FEATURE_PAT))
> > > > > mmap_type = I915_MMAP_TYPE_WC;
> > > > > else if (!i915_ggtt_has_aperture(&to_i915(dev)->ggtt))
> > > > > return -ENODEV;
> > > > > @@ -798,6 +806,10 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
> > > > > type = I915_MMAP_TYPE_UC;
> > > > > break;
> > > > >
> > > > > + case I915_MMAP_OFFSET_TTM:
> > > > > + type = I915_MMAP_TYPE_TTM;
> > > > > + break;
> > > > > +
> > > > > default:
> > > > > return -EINVAL;
> > > > > }
> > > > > @@ -968,6 +980,9 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> > > > > vma->vm_ops = &vm_ops_cpu;
> > > > > break;
> > > > >
> > > > > + case I915_MMAP_TYPE_TTM:
> > > > > + GEM_WARN_ON(mmo->mmap_type == I915_MMAP_TYPE_TTM);
> > > > > + /* fall-through */
> > > > > case I915_MMAP_TYPE_WB:
> > > > > vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> > > > > vma->vm_ops = &vm_ops_cpu;
> > > > > 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 ef3de2ae9723..d4c42bcdfeb6 100644
> > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > > > > @@ -105,6 +105,7 @@ enum i915_mmap_type {
> > > > > I915_MMAP_TYPE_WC,
> > > > > I915_MMAP_TYPE_WB,
> > > > > I915_MMAP_TYPE_UC,
> > > > > + I915_MMAP_TYPE_TTM,
> > > > > };
> > > > >
> > > > > struct i915_mmap_offset {
> > > > > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> > > > > index 1da8bd675e54..27a35d88e5f5 100644
> > > > > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> > > > > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> > > > > @@ -573,6 +573,14 @@ static int make_obj_busy(struct drm_i915_gem_object *obj)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static enum i915_mmap_type default_mapping(struct drm_i915_private *i915)
> > > > > +{
> > > > > + if (HAS_LMEM(i915))
> > > > > + return I915_MMAP_TYPE_TTM;
> > > > > +
> > > > > + return I915_MMAP_TYPE_GTT;
> > > > > +}
> > > > > +
> > > > > static bool assert_mmap_offset(struct drm_i915_private *i915,
> > > > > unsigned long size,
> > > > > int expected)
> > > > > @@ -585,7 +593,7 @@ static bool assert_mmap_offset(struct drm_i915_private *i915,
> > > > > if (IS_ERR(obj))
> > > > > return expected && expected == PTR_ERR(obj);
> > > > >
> > > > > - ret = __assign_mmap_offset(obj, I915_MMAP_TYPE_GTT, &offset, NULL);
> > > > > + ret = __assign_mmap_offset(obj, default_mapping(i915), &offset, NULL);
> > > > > i915_gem_object_put(obj);
> > > > >
> > > > > return ret == expected;
> > > > > @@ -689,7 +697,7 @@ static int igt_mmap_offset_exhaustion(void *arg)
> > > > > goto out;
> > > > > }
> > > > >
> > > > > - err = __assign_mmap_offset(obj, I915_MMAP_TYPE_GTT, &offset, NULL);
> > > > > + err = __assign_mmap_offset(obj, default_mapping(i915), &offset, NULL);
> > > > > if (err) {
> > > > > pr_err("Unable to insert object into reclaimed hole\n");
> > > > > goto err_obj;
> > > > > @@ -831,8 +839,14 @@ static int wc_check(struct drm_i915_gem_object *obj)
> > > > >
> > > > > static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type)
> > > > > {
> > > > > + struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > > > > bool no_map;
> > > > >
> > > > > + if (HAS_LMEM(i915))
> > > > > + return type == I915_MMAP_TYPE_TTM;
> > > > > + else if (type == I915_MMAP_TYPE_TTM)
> > > > > + return false;
> > > > > +
> > > > > if (type == I915_MMAP_TYPE_GTT &&
> > > > > !i915_ggtt_has_aperture(&to_i915(obj->base.dev)->ggtt))
> > > > > return false;
> > > > > @@ -970,6 +984,8 @@ static int igt_mmap(void *arg)
> > > > > err = __igt_mmap(i915, obj, I915_MMAP_TYPE_GTT);
> > > > > if (err == 0)
> > > > > err = __igt_mmap(i915, obj, I915_MMAP_TYPE_WC);
> > > > > + if (err == 0)
> > > > > + err = __igt_mmap(i915, obj, I915_MMAP_TYPE_TTM);
> > > > >
> > > > > i915_gem_object_put(obj);
> > > > > if (err)
> > > > > @@ -987,6 +1003,7 @@ static const char *repr_mmap_type(enum i915_mmap_type type)
> > > > > case I915_MMAP_TYPE_WB: return "wb";
> > > > > case I915_MMAP_TYPE_WC: return "wc";
> > > > > case I915_MMAP_TYPE_UC: return "uc";
> > > > > + case I915_MMAP_TYPE_TTM: return "ttm";
> > > > > default: return "unknown";
> > > > > }
> > > > > }
> > > > > @@ -1100,6 +1117,8 @@ static int igt_mmap_access(void *arg)
> > > > > err = __igt_mmap_access(i915, obj, I915_MMAP_TYPE_WC);
> > > > > if (err == 0)
> > > > > err = __igt_mmap_access(i915, obj, I915_MMAP_TYPE_UC);
> > > > > + if (err == 0)
> > > > > + err = __igt_mmap_access(i915, obj, I915_MMAP_TYPE_TTM);
> > > > >
> > > > > i915_gem_object_put(obj);
> > > > > if (err)
> > > > > @@ -1241,6 +1260,8 @@ static int igt_mmap_gpu(void *arg)
> > > > > err = __igt_mmap_gpu(i915, obj, I915_MMAP_TYPE_GTT);
> > > > > if (err == 0)
> > > > > err = __igt_mmap_gpu(i915, obj, I915_MMAP_TYPE_WC);
> > > > > + if (err == 0)
> > > > > + err = __igt_mmap_gpu(i915, obj, I915_MMAP_TYPE_TTM);
> > > > >
> > > > > i915_gem_object_put(obj);
> > > > > if (err)
> > > > > @@ -1396,6 +1417,8 @@ static int igt_mmap_revoke(void *arg)
> > > > > err = __igt_mmap_revoke(i915, obj, I915_MMAP_TYPE_GTT);
> > > > > if (err == 0)
> > > > > err = __igt_mmap_revoke(i915, obj, I915_MMAP_TYPE_WC);
> > > > > + if (err == 0)
> > > > > + err = __igt_mmap_revoke(i915, obj, I915_MMAP_TYPE_TTM);
> > > > >
> > > > > i915_gem_object_put(obj);
> > > > > if (err)
> > > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > > > > index e334a8b14ef2..1610ed40b4b5 100644
> > > > > --- a/include/uapi/drm/i915_drm.h
> > > > > +++ b/include/uapi/drm/i915_drm.h
> > > > > @@ -849,31 +849,60 @@ struct drm_i915_gem_mmap_gtt {
> > > > > __u64 offset;
> > > > > };
> > > > >
> > > > > +/**
> > > > > + * struct drm_i915_gem_mmap_offset - Retrieve an offset so we can mmap this buffer object.
> > > > > + *
> > > > > + * This struct is passed as argument to the `DRM_IOCTL_I915_GEM_MMAP_OFFSET` ioctl,
> > > > > + * and is used to retrieve the fake offset to mmap an object specified by &handle.
> > > > > + *
> > > > > + * The legacy way of using `DRM_IOCTL_I915_GEM_MMAP` is removed on gen12+.
> > > > > + * `DRM_IOCTL_I915_GEM_MMAP_GTT` is an older supported alias to this struct, but will behave
> > > > > + * as setting the &extensions to 0, and &flags to `I915_MMAP_OFFSET_GTT`.
> > > > > + */
> > > > > struct drm_i915_gem_mmap_offset {
> > > > > - /** Handle for the object being mapped. */
> > > > > + /** @handle: Handle for the object being mapped. */
> > > > > __u32 handle;
> > > > > + /** @pad: Must be zero */
> > > > > __u32 pad;
> > > > > /**
> > > > > - * Fake offset to use for subsequent mmap call
> > > > > + * @offset: The fake offset to use for subsequent mmap call
> > > > > *
> > > > > * This is a fixed-size type for 32/64 compatibility.
> > > > > */
> > > > > __u64 offset;
> > > > >
> > > > > /**
> > > > > - * Flags for extended behaviour.
> > > > > + * @flags: Flags for extended behaviour.
> > > > > + *
> > > > > + * It is mandatory that one of the `MMAP_OFFSET` types
> > > > > + * should be included:
> > > > > + * - `I915_MMAP_OFFSET_GTT`: Use mmap with the object bound to GTT.
> > > > > + * - `I915_MMAP_OFFSET_WC`: Use Write-Combined caching.
> > > > > + * - `I915_MMAP_OFFSET_WB`: Use Write-Back caching.
> > > > > + * - `I915_MMAP_OFFSET_TTM`: Use TTM to determine caching based on object placement.
> > > > > + *
> > > > > + * Only on devices with local memory is `I915_MMAP_OFFSET_TTM` valid. On
> > > > > + * devices without local memory, this caching mode is invalid.
> > > > > *
> > > > > - * It is mandatory that one of the MMAP_OFFSET types
> > > > > - * (GTT, WC, WB, UC, etc) should be included.
> > > > > + * As caching mode when specifying `I915_MMAP_OFFSET_TTM`, WC or WB will
> > > > > + * be used, depending on the object placement. WC will be used
> > > > > + * when the object resides in local memory, WB otherwise.
> > > > > */
> > > > > __u64 flags;
> > > > > -#define I915_MMAP_OFFSET_GTT 0
> > > > > -#define I915_MMAP_OFFSET_WC 1
> > > > > -#define I915_MMAP_OFFSET_WB 2
> > > > > -#define I915_MMAP_OFFSET_UC 3
> > > > >
> > > > > - /*
> > > > > - * Zero-terminated chain of extensions.
> > > > > +/** Use an mmap for the object by binding to GTT. */
> > > > > +#define I915_MMAP_OFFSET_GTT 0
> > > > > +/** Use Write-Combined caching. */
> > > > > +#define I915_MMAP_OFFSET_WC 1
> > > > > +/** Use Write-Back caching. */
> > > > > +#define I915_MMAP_OFFSET_WB 2
> > > > > +/** Do not use caching when binding this mmap. */
> > > > > +#define I915_MMAP_OFFSET_UC 3
> > > > > +/** Use the TTM binding, which determines the appropriate caching mode. */
> > > > > +#define I915_MMAP_OFFSET_TTM 4
> > > >
> > > > I'm not sure I like the name here. TTM is an implementation detail,
> > > > not a kind of map. I think we want something more like
> > > > I915_MMAP_OFFSET_IMPLICIT or something like that. The semantics here,
> > > > as far as I can tell, are "the mmap type is based on the object; you
> > > > can't change it." On DG1, the mmap type is fixed for all objects. On
> > > > integrated, we could have a BO create option for the mmap type but we
> > > > don't yet.
> > >
> > > Yeah it's not a great name, but also we didn't come up with anything
> > > substantially better on irc. More ideas:
> > > - PREDEFINED
> > > - PRESELECTED
> > >
> > > Note that we want to pick a value here which also makes sense for when we
> > > extend GEM_CREATE_EXT to allow you to select the single mmap mode you get
> > > at creation time (for igfx cleanup of the uapi). So IMPLICIT isn't the
> > > greatest name either, when we'll allow userspace to explicit chose it -
> > > just not here anymore.
> >
> > Yeah, none of them are great. Maybe FIXED? Or PER_BO? Or BO_CREATE?
> > Or just BO? I think the semantics we want are "whatever was on the
> > BO at create time".
>
> _FIXED sounds like the least horrible one to me.
Fine with me.
> -Daniel
>
> >
> > --Jason
> >
> > >
> > > Anyway, pick a name and we'll paint this bikeshed, I don't care much
> > > really. As long as there's kerneldoc :-)
> > > -Daniel
> > >
> > > >
> > > > --Jason
> > > >
> > > > > +
> > > > > + /**
> > > > > + * @extensions: Zero-terminated chain of extensions.
> > > > > *
> > > > > * No current extensions defined; mbz.
> > > > > */
> > > > > --
> > > > > 2.31.0
> > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
