Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On Wed, Apr 21, 2021 at 2:23 PM Daniel Vetter wrote:
>
> On Wed, Apr 21, 2021 at 8:28 PM Tvrtko Ursulin
> wrote:
> > On 21/04/2021 18:17, Jason Ekstrand wrote:
> > > On Wed, Apr 21, 2021 at 9:25 AM Tvrtko Ursulin
> > > wrote:
> > >> On 21/04/2021 14:54, Jason Ekstrand wrote:
> > >>> On Wed, Apr 21, 2021 at 3:22 AM Tvrtko Ursulin
> > >>> wrote:
> > On 20/04/2021 18:00, Jason Ekstrand wrote:
> > I am not claiming to know memory region query will end up the same, and
> > I definitely agree we cannot guess the future. I am just saying rsvd
> > fields are inconsequential really in terms of maintenance burden and
> > have been proven useful in the past. So I disagree with the drive to
> > kick them all out.
> > >>>
> > >>> Sure, it doesn't cost anything to have extra zeros in the struct.
> > >>> However, if/when the API grows using rsvd fields, we end up with "if
> > >>> CAP_FOO is set, rsvd[5] means blah" which makes for a horribly
> > >>> confusing API. As a userspace person who has to remember how to use
> > >>> this stuff, I'd rather make another call or chain in a struct than try
> > >>> to remember and/or figure out what all 8 rsvd fields mean.
> > >>
> > >> Well it's not called rsvd in the uapi which is aware of the new field
> > >> but has a new name.
> > >
> > > Are we allowed to do that? This is a genuine question. When I've
> > > tried in the past (cliprects), I was told we couldn't rename it even
> > > though literally no one had used it in code for years.
> >
> > Well we did the union for pad_to_size so I thought we are allowed that
> > trick at least. From my experience backward source level compatibility
> > is not always there even with things like glibc. Despite that, are we
> > generally required to stay backward source compatible I will not claim
> > either way.
I'm starting to lose the will to care about this particular bike shed.
I think I'm fine with keeping some RSVD fields as long as:
1. We're very clear in the docs with flags and caps about what things
are inputs and what things are outputs and how they interact.
Preferably, everything is an output.
2. If we already know that caps is useless without supported_caps,
let's either add supported_caps in now or throw out both and use some
rsvd for them when they begin to be needed.
3. We have a plan for how we're going to use them in a
backwards-compatible way.
On 3, it sounds like we have a rough sketch of a plan but I'm still
unclear on some details. In particular, we have an rsvd[8] at the end
but, if we're going to replace individual bits of it, we can't ever
shorten or re-name that array. We could, potentially, do
union {
__u32 rsvd[8];
struct {
__u32 new_field;
};
};
and trust C to put all the fields of our anonymous struct at the top.
Otherwise, we've got to fill out our struct with more rsvd and that
gets to be a mess.
Another option would be to have
__u32 rsvd1;
__u32 rsvd2;
__u32 rsvd3;
/* etc... */
and we can replace them one at a time.
Again, my big point is that I want us to have a PLAN and not end up in
a scenario where we end up with rsvd[5] having magical meaning. What
I see in DII does not give me confidence. However, I do believe that
such a plan can exist.
--Jason
> I think the anonymous union with exactly same sized field is ok. We
> also try hard to be source compatible, but we have screwed up in the
> past and shrugged it off. The one example that comes to mind is
> extended structures at the bottom with new field, which the kernel
> automatically zero-extends for old userspace. But when you recompile,
> your new-old userspace might no longer clear the new fields because
> the ioctl code didn't start out by memset()ing the entire struct.
Also, we need to ensure that we memset everything to 0. :-)
--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On Wed, Apr 21, 2021 at 8:28 PM Tvrtko Ursulin wrote: > On 21/04/2021 18:17, Jason Ekstrand wrote: > > On Wed, Apr 21, 2021 at 9:25 AM Tvrtko Ursulin > > wrote: > >> On 21/04/2021 14:54, Jason Ekstrand wrote: > >>> On Wed, Apr 21, 2021 at 3:22 AM Tvrtko Ursulin > >>> wrote: > On 20/04/2021 18:00, Jason Ekstrand wrote: > I am not claiming to know memory region query will end up the same, and > I definitely agree we cannot guess the future. I am just saying rsvd > fields are inconsequential really in terms of maintenance burden and > have been proven useful in the past. So I disagree with the drive to > kick them all out. > >>> > >>> Sure, it doesn't cost anything to have extra zeros in the struct. > >>> However, if/when the API grows using rsvd fields, we end up with "if > >>> CAP_FOO is set, rsvd[5] means blah" which makes for a horribly > >>> confusing API. As a userspace person who has to remember how to use > >>> this stuff, I'd rather make another call or chain in a struct than try > >>> to remember and/or figure out what all 8 rsvd fields mean. > >> > >> Well it's not called rsvd in the uapi which is aware of the new field > >> but has a new name. > > > > Are we allowed to do that? This is a genuine question. When I've > > tried in the past (cliprects), I was told we couldn't rename it even > > though literally no one had used it in code for years. > > Well we did the union for pad_to_size so I thought we are allowed that > trick at least. From my experience backward source level compatibility > is not always there even with things like glibc. Despite that, are we > generally required to stay backward source compatible I will not claim > either way. I think the anonymous union with exactly same sized field is ok. We also try hard to be source compatible, but we have screwed up in the past and shrugged it off. The one example that comes to mind is extended structures at the bottom with new field, which the kernel automatically zero-extends for old userspace. But when you recompile, your new-old userspace might no longer clear the new fields because the ioctl code didn't start out by memset()ing the entire struct. But by we managed to not botch things up on source compat, but it's definitely a lot tricker than ABI compat. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On 21/04/2021 18:17, Jason Ekstrand wrote:
On Wed, Apr 21, 2021 at 9:25 AM Tvrtko Ursulin
wrote:
On 21/04/2021 14:54, Jason Ekstrand wrote:
On Wed, Apr 21, 2021 at 3:22 AM Tvrtko Ursulin
wrote:
On 20/04/2021 18:00, Jason Ekstrand wrote:
On Tue, Apr 20, 2021 at 11:34 AM Tvrtko Ursulin
wrote:
On 19/04/2021 16:19, Jason Ekstrand wrote:
On Mon, Apr 19, 2021 at 7:02 AM Matthew Auld wrote:
On 16/04/2021 17:38, Jason Ekstrand wrote:
On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld wrote:
Add an entry for the new uAPI needed for DG1.
v2(Daniel):
- include the overall upstreaming plan
- add a note for mmap, there are differences here for TTM vs i915
- bunch of other suggestions from Daniel
v3:
(Daniel)
- add a note for set/get caching stuff
- add some more docs for existing query and extensions stuff
- add an actual code example for regions query
- bunch of other stuff
(Jason)
- uAPI change(!):
- try a simpler design with the placements extension
- rather than have a generic setparam which can cover multiple
use cases, have each extension be responsible for one thing
only
Signed-off-by: Matthew Auld
Cc: Joonas Lahtinen
Cc: Jordan Justen
Cc: Daniel Vetter
Cc: Kenneth Graunke
Cc: Jason Ekstrand
Cc: Dave Airlie
Cc: dri-de...@lists.freedesktop.org
Cc: mesa-dev@lists.freedesktop.org
---
Documentation/gpu/rfc/i915_gem_lmem.h | 255
Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
Documentation/gpu/rfc/index.rst | 4 +
3 files changed, 398 insertions(+)
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
b/Documentation/gpu/rfc/i915_gem_lmem.h
new file mode 100644
index ..2a82a452e9f2
--- /dev/null
+++ b/Documentation/gpu/rfc/i915_gem_lmem.h
@@ -0,0 +1,255 @@
+/*
+ * Note that drm_i915_query_item and drm_i915_query are existing bits of uAPI.
+ * For the regions query we are just adding a new query id, so no actual new
+ * ioctl or anything, but including it here for reference.
+ */
+struct drm_i915_query_item {
+#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
+
+__u64 query_id;
+
+/*
+ * When set to zero by userspace, this is filled with the size of the
+ * data to be written at the data_ptr pointer. The kernel sets this
+ * value to a negative value to signal an error on a particular query
+ * item.
+ */
+__s32 length;
+
+__u32 flags;
+/*
+ * Data will be written at the location pointed by data_ptr when the
+ * value of length matches the length of the data to be written by the
+ * kernel.
+ */
+__u64 data_ptr;
+};
+
+struct drm_i915_query {
+__u32 num_items;
+/*
+ * Unused for now. Must be cleared to zero.
+ */
+__u32 flags;
+/*
+ * This points to an array of num_items drm_i915_query_item structures.
+ */
+__u64 items_ptr;
+};
+
+#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY,
struct drm_i915_query)
+
+/**
+ * enum drm_i915_gem_memory_class
+ */
+enum drm_i915_gem_memory_class {
+ /** @I915_MEMORY_CLASS_SYSTEM: system memory */
+ I915_MEMORY_CLASS_SYSTEM = 0,
+ /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
+ I915_MEMORY_CLASS_DEVICE,
+};
+
+/**
+ * struct drm_i915_gem_memory_class_instance
+ */
+struct drm_i915_gem_memory_class_instance {
+ /** @memory_class: see enum drm_i915_gem_memory_class */
+ __u16 memory_class;
+
+ /** @memory_instance: which instance */
+ __u16 memory_instance;
+};
+
+/**
+ * struct drm_i915_memory_region_info
+ *
+ * Describes one region as known to the driver.
+ *
+ * Note that we reserve quite a lot of stuff here for potential future work. As
+ * an example we might want expose the capabilities(see caps) for a given
+ * region, which could include things like if the region is CPU
+ * mappable/accessible etc.
I get caps but I'm seriously at a loss as to what the rest of this
would be used for. Why are caps and flags both there and separate?
Flags are typically something you set, not query. Also, what's with
rsvd1 at the end? This smells of substantial over-building to me.
I thought to myself, "maybe I'm missing a future use-case" so I looked
at the internal tree and none of this is being used there either.
This indicates to me that either I'm missing something and there's
code somewhere I don't know about or, with three years of building on
internal branches, we still haven't proven that any of this is needed.
If it's the latter, which I strongly suspect, maybe we should drop the
unnecessary bits and only add them back in if and when we have
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On Wed, Apr 21, 2021 at 9:25 AM Tvrtko Ursulin
wrote:
>
>
> On 21/04/2021 14:54, Jason Ekstrand wrote:
> > On Wed, Apr 21, 2021 at 3:22 AM Tvrtko Ursulin
> > wrote:
> >>
> >> On 20/04/2021 18:00, Jason Ekstrand wrote:
> >>> On Tue, Apr 20, 2021 at 11:34 AM Tvrtko Ursulin
> >>> wrote:
>
>
> On 19/04/2021 16:19, Jason Ekstrand wrote:
> > On Mon, Apr 19, 2021 at 7:02 AM Matthew Auld
> > wrote:
> >>
> >> On 16/04/2021 17:38, Jason Ekstrand wrote:
> >>> On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld
> >>> wrote:
>
> Add an entry for the new uAPI needed for DG1.
>
> v2(Daniel):
> - include the overall upstreaming plan
> - add a note for mmap, there are differences here for TTM vs
> i915
> - bunch of other suggestions from Daniel
> v3:
> (Daniel)
> - add a note for set/get caching stuff
> - add some more docs for existing query and extensions stuff
> - add an actual code example for regions query
> - bunch of other stuff
> (Jason)
> - uAPI change(!):
> - try a simpler design with the placements extension
> - rather than have a generic setparam which can cover
> multiple
> use cases, have each extension be responsible for one
> thing
> only
>
> Signed-off-by: Matthew Auld
> Cc: Joonas Lahtinen
> Cc: Jordan Justen
> Cc: Daniel Vetter
> Cc: Kenneth Graunke
> Cc: Jason Ekstrand
> Cc: Dave Airlie
> Cc: dri-de...@lists.freedesktop.org
> Cc: mesa-dev@lists.freedesktop.org
> ---
> Documentation/gpu/rfc/i915_gem_lmem.h | 255
>
> Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
> Documentation/gpu/rfc/index.rst | 4 +
> 3 files changed, 398 insertions(+)
> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
>
> diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
> b/Documentation/gpu/rfc/i915_gem_lmem.h
> new file mode 100644
> index ..2a82a452e9f2
> --- /dev/null
> +++ b/Documentation/gpu/rfc/i915_gem_lmem.h
> @@ -0,0 +1,255 @@
> +/*
> + * Note that drm_i915_query_item and drm_i915_query are existing
> bits of uAPI.
> + * For the regions query we are just adding a new query id, so no
> actual new
> + * ioctl or anything, but including it here for reference.
> + */
> +struct drm_i915_query_item {
> +#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
> +
> +__u64 query_id;
> +
> +/*
> + * When set to zero by userspace, this is filled with the
> size of the
> + * data to be written at the data_ptr pointer. The kernel
> sets this
> + * value to a negative value to signal an error on a
> particular query
> + * item.
> + */
> +__s32 length;
> +
> +__u32 flags;
> +/*
> + * Data will be written at the location pointed by data_ptr
> when the
> + * value of length matches the length of the data to be
> written by the
> + * kernel.
> + */
> +__u64 data_ptr;
> +};
> +
> +struct drm_i915_query {
> +__u32 num_items;
> +/*
> + * Unused for now. Must be cleared to zero.
> + */
> +__u32 flags;
> +/*
> + * This points to an array of num_items drm_i915_query_item
> structures.
> + */
> +__u64 items_ptr;
> +};
> +
> +#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_QUERY, struct drm_i915_query)
> +
> +/**
> + * enum drm_i915_gem_memory_class
> + */
> +enum drm_i915_gem_memory_class {
> + /** @I915_MEMORY_CLASS_SYSTEM: system memory */
> + I915_MEMORY_CLASS_SYSTEM = 0,
> + /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
> + I915_MEMORY_CLASS_DEVICE,
> +};
> +
> +/**
> + * struct drm_i915_gem_memory_class_instance
> + */
> +struct drm_i915_gem_memory_class_instance {
> +
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On 21/04/2021 14:54, Jason Ekstrand wrote:
On Wed, Apr 21, 2021 at 3:22 AM Tvrtko Ursulin
wrote:
On 20/04/2021 18:00, Jason Ekstrand wrote:
On Tue, Apr 20, 2021 at 11:34 AM Tvrtko Ursulin
wrote:
On 19/04/2021 16:19, Jason Ekstrand wrote:
On Mon, Apr 19, 2021 at 7:02 AM Matthew Auld wrote:
On 16/04/2021 17:38, Jason Ekstrand wrote:
On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld wrote:
Add an entry for the new uAPI needed for DG1.
v2(Daniel):
- include the overall upstreaming plan
- add a note for mmap, there are differences here for TTM vs i915
- bunch of other suggestions from Daniel
v3:
(Daniel)
- add a note for set/get caching stuff
- add some more docs for existing query and extensions stuff
- add an actual code example for regions query
- bunch of other stuff
(Jason)
- uAPI change(!):
- try a simpler design with the placements extension
- rather than have a generic setparam which can cover multiple
use cases, have each extension be responsible for one thing
only
Signed-off-by: Matthew Auld
Cc: Joonas Lahtinen
Cc: Jordan Justen
Cc: Daniel Vetter
Cc: Kenneth Graunke
Cc: Jason Ekstrand
Cc: Dave Airlie
Cc: dri-de...@lists.freedesktop.org
Cc: mesa-dev@lists.freedesktop.org
---
Documentation/gpu/rfc/i915_gem_lmem.h | 255
Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
Documentation/gpu/rfc/index.rst | 4 +
3 files changed, 398 insertions(+)
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
b/Documentation/gpu/rfc/i915_gem_lmem.h
new file mode 100644
index ..2a82a452e9f2
--- /dev/null
+++ b/Documentation/gpu/rfc/i915_gem_lmem.h
@@ -0,0 +1,255 @@
+/*
+ * Note that drm_i915_query_item and drm_i915_query are existing bits of uAPI.
+ * For the regions query we are just adding a new query id, so no actual new
+ * ioctl or anything, but including it here for reference.
+ */
+struct drm_i915_query_item {
+#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
+
+__u64 query_id;
+
+/*
+ * When set to zero by userspace, this is filled with the size of the
+ * data to be written at the data_ptr pointer. The kernel sets this
+ * value to a negative value to signal an error on a particular query
+ * item.
+ */
+__s32 length;
+
+__u32 flags;
+/*
+ * Data will be written at the location pointed by data_ptr when the
+ * value of length matches the length of the data to be written by the
+ * kernel.
+ */
+__u64 data_ptr;
+};
+
+struct drm_i915_query {
+__u32 num_items;
+/*
+ * Unused for now. Must be cleared to zero.
+ */
+__u32 flags;
+/*
+ * This points to an array of num_items drm_i915_query_item structures.
+ */
+__u64 items_ptr;
+};
+
+#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY,
struct drm_i915_query)
+
+/**
+ * enum drm_i915_gem_memory_class
+ */
+enum drm_i915_gem_memory_class {
+ /** @I915_MEMORY_CLASS_SYSTEM: system memory */
+ I915_MEMORY_CLASS_SYSTEM = 0,
+ /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
+ I915_MEMORY_CLASS_DEVICE,
+};
+
+/**
+ * struct drm_i915_gem_memory_class_instance
+ */
+struct drm_i915_gem_memory_class_instance {
+ /** @memory_class: see enum drm_i915_gem_memory_class */
+ __u16 memory_class;
+
+ /** @memory_instance: which instance */
+ __u16 memory_instance;
+};
+
+/**
+ * struct drm_i915_memory_region_info
+ *
+ * Describes one region as known to the driver.
+ *
+ * Note that we reserve quite a lot of stuff here for potential future work. As
+ * an example we might want expose the capabilities(see caps) for a given
+ * region, which could include things like if the region is CPU
+ * mappable/accessible etc.
I get caps but I'm seriously at a loss as to what the rest of this
would be used for. Why are caps and flags both there and separate?
Flags are typically something you set, not query. Also, what's with
rsvd1 at the end? This smells of substantial over-building to me.
I thought to myself, "maybe I'm missing a future use-case" so I looked
at the internal tree and none of this is being used there either.
This indicates to me that either I'm missing something and there's
code somewhere I don't know about or, with three years of building on
internal branches, we still haven't proven that any of this is needed.
If it's the latter, which I strongly suspect, maybe we should drop the
unnecessary bits and only add them back in if and when we have proof
that they're useful.
Do you mean just drop caps/flags here, but keep/inflate rsvd0/rsvd1,
which is less opinionated
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On Wed, Apr 21, 2021 at 3:22 AM Tvrtko Ursulin
wrote:
>
> On 20/04/2021 18:00, Jason Ekstrand wrote:
> > On Tue, Apr 20, 2021 at 11:34 AM Tvrtko Ursulin
> > wrote:
> >>
> >>
> >> On 19/04/2021 16:19, Jason Ekstrand wrote:
> >>> On Mon, Apr 19, 2021 at 7:02 AM Matthew Auld
> >>> wrote:
>
> On 16/04/2021 17:38, Jason Ekstrand wrote:
> > On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld
> > wrote:
> >>
> >> Add an entry for the new uAPI needed for DG1.
> >>
> >> v2(Daniel):
> >> - include the overall upstreaming plan
> >> - add a note for mmap, there are differences here for TTM vs i915
> >> - bunch of other suggestions from Daniel
> >> v3:
> >> (Daniel)
> >> - add a note for set/get caching stuff
> >> - add some more docs for existing query and extensions stuff
> >> - add an actual code example for regions query
> >> - bunch of other stuff
> >> (Jason)
> >> - uAPI change(!):
> >>- try a simpler design with the placements extension
> >>- rather than have a generic setparam which can cover
> >> multiple
> >> use cases, have each extension be responsible for one
> >> thing
> >> only
> >>
> >> Signed-off-by: Matthew Auld
> >> Cc: Joonas Lahtinen
> >> Cc: Jordan Justen
> >> Cc: Daniel Vetter
> >> Cc: Kenneth Graunke
> >> Cc: Jason Ekstrand
> >> Cc: Dave Airlie
> >> Cc: dri-de...@lists.freedesktop.org
> >> Cc: mesa-dev@lists.freedesktop.org
> >> ---
> >> Documentation/gpu/rfc/i915_gem_lmem.h | 255
> >>
> >> Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
> >> Documentation/gpu/rfc/index.rst | 4 +
> >> 3 files changed, 398 insertions(+)
> >> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
> >> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
> >>
> >> diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
> >> b/Documentation/gpu/rfc/i915_gem_lmem.h
> >> new file mode 100644
> >> index ..2a82a452e9f2
> >> --- /dev/null
> >> +++ b/Documentation/gpu/rfc/i915_gem_lmem.h
> >> @@ -0,0 +1,255 @@
> >> +/*
> >> + * Note that drm_i915_query_item and drm_i915_query are existing bits
> >> of uAPI.
> >> + * For the regions query we are just adding a new query id, so no
> >> actual new
> >> + * ioctl or anything, but including it here for reference.
> >> + */
> >> +struct drm_i915_query_item {
> >> +#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
> >> +
> >> +__u64 query_id;
> >> +
> >> +/*
> >> + * When set to zero by userspace, this is filled with the
> >> size of the
> >> + * data to be written at the data_ptr pointer. The kernel
> >> sets this
> >> + * value to a negative value to signal an error on a
> >> particular query
> >> + * item.
> >> + */
> >> +__s32 length;
> >> +
> >> +__u32 flags;
> >> +/*
> >> + * Data will be written at the location pointed by data_ptr
> >> when the
> >> + * value of length matches the length of the data to be
> >> written by the
> >> + * kernel.
> >> + */
> >> +__u64 data_ptr;
> >> +};
> >> +
> >> +struct drm_i915_query {
> >> +__u32 num_items;
> >> +/*
> >> + * Unused for now. Must be cleared to zero.
> >> + */
> >> +__u32 flags;
> >> +/*
> >> + * This points to an array of num_items drm_i915_query_item
> >> structures.
> >> + */
> >> +__u64 items_ptr;
> >> +};
> >> +
> >> +#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE +
> >> DRM_I915_QUERY, struct drm_i915_query)
> >> +
> >> +/**
> >> + * enum drm_i915_gem_memory_class
> >> + */
> >> +enum drm_i915_gem_memory_class {
> >> + /** @I915_MEMORY_CLASS_SYSTEM: system memory */
> >> + I915_MEMORY_CLASS_SYSTEM = 0,
> >> + /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
> >> + I915_MEMORY_CLASS_DEVICE,
> >> +};
> >> +
> >> +/**
> >> + * struct drm_i915_gem_memory_class_instance
> >> + */
> >> +struct drm_i915_gem_memory_class_instance {
> >> + /** @memory_class: see enum drm_i915_gem_memory_class */
> >> + __u16 memory_class;
> >> +
> >> + /** @memory_instance: which instance */
> >> + __u16 memory_instance;
> >> +};
> >> +
> >> +/**
> >> + * struct drm_i915_memory_region_info
> >> + *
> >> + * Describes one region as known to the driver.
> >> + *
> >> + * Note
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On 20/04/2021 18:00, Jason Ekstrand wrote:
On Tue, Apr 20, 2021 at 11:34 AM Tvrtko Ursulin
wrote:
On 19/04/2021 16:19, Jason Ekstrand wrote:
On Mon, Apr 19, 2021 at 7:02 AM Matthew Auld wrote:
On 16/04/2021 17:38, Jason Ekstrand wrote:
On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld wrote:
Add an entry for the new uAPI needed for DG1.
v2(Daniel):
- include the overall upstreaming plan
- add a note for mmap, there are differences here for TTM vs i915
- bunch of other suggestions from Daniel
v3:
(Daniel)
- add a note for set/get caching stuff
- add some more docs for existing query and extensions stuff
- add an actual code example for regions query
- bunch of other stuff
(Jason)
- uAPI change(!):
- try a simpler design with the placements extension
- rather than have a generic setparam which can cover multiple
use cases, have each extension be responsible for one thing
only
Signed-off-by: Matthew Auld
Cc: Joonas Lahtinen
Cc: Jordan Justen
Cc: Daniel Vetter
Cc: Kenneth Graunke
Cc: Jason Ekstrand
Cc: Dave Airlie
Cc: dri-de...@lists.freedesktop.org
Cc: mesa-dev@lists.freedesktop.org
---
Documentation/gpu/rfc/i915_gem_lmem.h | 255
Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
Documentation/gpu/rfc/index.rst | 4 +
3 files changed, 398 insertions(+)
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
b/Documentation/gpu/rfc/i915_gem_lmem.h
new file mode 100644
index ..2a82a452e9f2
--- /dev/null
+++ b/Documentation/gpu/rfc/i915_gem_lmem.h
@@ -0,0 +1,255 @@
+/*
+ * Note that drm_i915_query_item and drm_i915_query are existing bits of uAPI.
+ * For the regions query we are just adding a new query id, so no actual new
+ * ioctl or anything, but including it here for reference.
+ */
+struct drm_i915_query_item {
+#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
+
+__u64 query_id;
+
+/*
+ * When set to zero by userspace, this is filled with the size of the
+ * data to be written at the data_ptr pointer. The kernel sets this
+ * value to a negative value to signal an error on a particular query
+ * item.
+ */
+__s32 length;
+
+__u32 flags;
+/*
+ * Data will be written at the location pointed by data_ptr when the
+ * value of length matches the length of the data to be written by the
+ * kernel.
+ */
+__u64 data_ptr;
+};
+
+struct drm_i915_query {
+__u32 num_items;
+/*
+ * Unused for now. Must be cleared to zero.
+ */
+__u32 flags;
+/*
+ * This points to an array of num_items drm_i915_query_item structures.
+ */
+__u64 items_ptr;
+};
+
+#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY,
struct drm_i915_query)
+
+/**
+ * enum drm_i915_gem_memory_class
+ */
+enum drm_i915_gem_memory_class {
+ /** @I915_MEMORY_CLASS_SYSTEM: system memory */
+ I915_MEMORY_CLASS_SYSTEM = 0,
+ /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
+ I915_MEMORY_CLASS_DEVICE,
+};
+
+/**
+ * struct drm_i915_gem_memory_class_instance
+ */
+struct drm_i915_gem_memory_class_instance {
+ /** @memory_class: see enum drm_i915_gem_memory_class */
+ __u16 memory_class;
+
+ /** @memory_instance: which instance */
+ __u16 memory_instance;
+};
+
+/**
+ * struct drm_i915_memory_region_info
+ *
+ * Describes one region as known to the driver.
+ *
+ * Note that we reserve quite a lot of stuff here for potential future work. As
+ * an example we might want expose the capabilities(see caps) for a given
+ * region, which could include things like if the region is CPU
+ * mappable/accessible etc.
I get caps but I'm seriously at a loss as to what the rest of this
would be used for. Why are caps and flags both there and separate?
Flags are typically something you set, not query. Also, what's with
rsvd1 at the end? This smells of substantial over-building to me.
I thought to myself, "maybe I'm missing a future use-case" so I looked
at the internal tree and none of this is being used there either.
This indicates to me that either I'm missing something and there's
code somewhere I don't know about or, with three years of building on
internal branches, we still haven't proven that any of this is needed.
If it's the latter, which I strongly suspect, maybe we should drop the
unnecessary bits and only add them back in if and when we have proof
that they're useful.
Do you mean just drop caps/flags here, but keep/inflate rsvd0/rsvd1,
which is less opinionated about future unknowns? If so, makes sense to me.
I meant drop flags and rsvd1. We need rsvd0 for padding and I can
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On Tue, Apr 20, 2021 at 11:34 AM Tvrtko Ursulin
wrote:
>
>
> On 19/04/2021 16:19, Jason Ekstrand wrote:
> > On Mon, Apr 19, 2021 at 7:02 AM Matthew Auld wrote:
> >>
> >> On 16/04/2021 17:38, Jason Ekstrand wrote:
> >>> On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld
> >>> wrote:
>
> Add an entry for the new uAPI needed for DG1.
>
> v2(Daniel):
> - include the overall upstreaming plan
> - add a note for mmap, there are differences here for TTM vs i915
> - bunch of other suggestions from Daniel
> v3:
> (Daniel)
> - add a note for set/get caching stuff
> - add some more docs for existing query and extensions stuff
> - add an actual code example for regions query
> - bunch of other stuff
> (Jason)
> - uAPI change(!):
> - try a simpler design with the placements extension
> - rather than have a generic setparam which can cover multiple
> use cases, have each extension be responsible for one thing
> only
>
> Signed-off-by: Matthew Auld
> Cc: Joonas Lahtinen
> Cc: Jordan Justen
> Cc: Daniel Vetter
> Cc: Kenneth Graunke
> Cc: Jason Ekstrand
> Cc: Dave Airlie
> Cc: dri-de...@lists.freedesktop.org
> Cc: mesa-dev@lists.freedesktop.org
> ---
> Documentation/gpu/rfc/i915_gem_lmem.h | 255
> Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
> Documentation/gpu/rfc/index.rst | 4 +
> 3 files changed, 398 insertions(+)
> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
>
> diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
> b/Documentation/gpu/rfc/i915_gem_lmem.h
> new file mode 100644
> index ..2a82a452e9f2
> --- /dev/null
> +++ b/Documentation/gpu/rfc/i915_gem_lmem.h
> @@ -0,0 +1,255 @@
> +/*
> + * Note that drm_i915_query_item and drm_i915_query are existing bits
> of uAPI.
> + * For the regions query we are just adding a new query id, so no
> actual new
> + * ioctl or anything, but including it here for reference.
> + */
> +struct drm_i915_query_item {
> +#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
> +
> +__u64 query_id;
> +
> +/*
> + * When set to zero by userspace, this is filled with the size
> of the
> + * data to be written at the data_ptr pointer. The kernel sets
> this
> + * value to a negative value to signal an error on a particular
> query
> + * item.
> + */
> +__s32 length;
> +
> +__u32 flags;
> +/*
> + * Data will be written at the location pointed by data_ptr
> when the
> + * value of length matches the length of the data to be written
> by the
> + * kernel.
> + */
> +__u64 data_ptr;
> +};
> +
> +struct drm_i915_query {
> +__u32 num_items;
> +/*
> + * Unused for now. Must be cleared to zero.
> + */
> +__u32 flags;
> +/*
> + * This points to an array of num_items drm_i915_query_item
> structures.
> + */
> +__u64 items_ptr;
> +};
> +
> +#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_QUERY, struct drm_i915_query)
> +
> +/**
> + * enum drm_i915_gem_memory_class
> + */
> +enum drm_i915_gem_memory_class {
> + /** @I915_MEMORY_CLASS_SYSTEM: system memory */
> + I915_MEMORY_CLASS_SYSTEM = 0,
> + /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
> + I915_MEMORY_CLASS_DEVICE,
> +};
> +
> +/**
> + * struct drm_i915_gem_memory_class_instance
> + */
> +struct drm_i915_gem_memory_class_instance {
> + /** @memory_class: see enum drm_i915_gem_memory_class */
> + __u16 memory_class;
> +
> + /** @memory_instance: which instance */
> + __u16 memory_instance;
> +};
> +
> +/**
> + * struct drm_i915_memory_region_info
> + *
> + * Describes one region as known to the driver.
> + *
> + * Note that we reserve quite a lot of stuff here for potential future
> work. As
> + * an example we might want expose the capabilities(see caps) for a
> given
> + * region, which could include things like if the region is CPU
> + * mappable/accessible etc.
> >>>
> >>> I get caps but I'm seriously at a loss as to what the rest of this
> >>> would be used for. Why are caps and flags both there and
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On 19/04/2021 16:19, Jason Ekstrand wrote:
On Mon, Apr 19, 2021 at 7:02 AM Matthew Auld wrote:
On 16/04/2021 17:38, Jason Ekstrand wrote:
On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld wrote:
Add an entry for the new uAPI needed for DG1.
v2(Daniel):
- include the overall upstreaming plan
- add a note for mmap, there are differences here for TTM vs i915
- bunch of other suggestions from Daniel
v3:
(Daniel)
- add a note for set/get caching stuff
- add some more docs for existing query and extensions stuff
- add an actual code example for regions query
- bunch of other stuff
(Jason)
- uAPI change(!):
- try a simpler design with the placements extension
- rather than have a generic setparam which can cover multiple
use cases, have each extension be responsible for one thing
only
Signed-off-by: Matthew Auld
Cc: Joonas Lahtinen
Cc: Jordan Justen
Cc: Daniel Vetter
Cc: Kenneth Graunke
Cc: Jason Ekstrand
Cc: Dave Airlie
Cc: dri-de...@lists.freedesktop.org
Cc: mesa-dev@lists.freedesktop.org
---
Documentation/gpu/rfc/i915_gem_lmem.h | 255
Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
Documentation/gpu/rfc/index.rst | 4 +
3 files changed, 398 insertions(+)
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
b/Documentation/gpu/rfc/i915_gem_lmem.h
new file mode 100644
index ..2a82a452e9f2
--- /dev/null
+++ b/Documentation/gpu/rfc/i915_gem_lmem.h
@@ -0,0 +1,255 @@
+/*
+ * Note that drm_i915_query_item and drm_i915_query are existing bits of uAPI.
+ * For the regions query we are just adding a new query id, so no actual new
+ * ioctl or anything, but including it here for reference.
+ */
+struct drm_i915_query_item {
+#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
+
+__u64 query_id;
+
+/*
+ * When set to zero by userspace, this is filled with the size of the
+ * data to be written at the data_ptr pointer. The kernel sets this
+ * value to a negative value to signal an error on a particular query
+ * item.
+ */
+__s32 length;
+
+__u32 flags;
+/*
+ * Data will be written at the location pointed by data_ptr when the
+ * value of length matches the length of the data to be written by the
+ * kernel.
+ */
+__u64 data_ptr;
+};
+
+struct drm_i915_query {
+__u32 num_items;
+/*
+ * Unused for now. Must be cleared to zero.
+ */
+__u32 flags;
+/*
+ * This points to an array of num_items drm_i915_query_item structures.
+ */
+__u64 items_ptr;
+};
+
+#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY,
struct drm_i915_query)
+
+/**
+ * enum drm_i915_gem_memory_class
+ */
+enum drm_i915_gem_memory_class {
+ /** @I915_MEMORY_CLASS_SYSTEM: system memory */
+ I915_MEMORY_CLASS_SYSTEM = 0,
+ /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
+ I915_MEMORY_CLASS_DEVICE,
+};
+
+/**
+ * struct drm_i915_gem_memory_class_instance
+ */
+struct drm_i915_gem_memory_class_instance {
+ /** @memory_class: see enum drm_i915_gem_memory_class */
+ __u16 memory_class;
+
+ /** @memory_instance: which instance */
+ __u16 memory_instance;
+};
+
+/**
+ * struct drm_i915_memory_region_info
+ *
+ * Describes one region as known to the driver.
+ *
+ * Note that we reserve quite a lot of stuff here for potential future work. As
+ * an example we might want expose the capabilities(see caps) for a given
+ * region, which could include things like if the region is CPU
+ * mappable/accessible etc.
I get caps but I'm seriously at a loss as to what the rest of this
would be used for. Why are caps and flags both there and separate?
Flags are typically something you set, not query. Also, what's with
rsvd1 at the end? This smells of substantial over-building to me.
I thought to myself, "maybe I'm missing a future use-case" so I looked
at the internal tree and none of this is being used there either.
This indicates to me that either I'm missing something and there's
code somewhere I don't know about or, with three years of building on
internal branches, we still haven't proven that any of this is needed.
If it's the latter, which I strongly suspect, maybe we should drop the
unnecessary bits and only add them back in if and when we have proof
that they're useful.
Do you mean just drop caps/flags here, but keep/inflate rsvd0/rsvd1,
which is less opinionated about future unknowns? If so, makes sense to me.
I meant drop flags and rsvd1. We need rsvd0 for padding and I can
see some value to caps. We may want to advertise, for instance, what
mapping coherency types are available per-heap. But I
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On Mon, Apr 19, 2021 at 7:02 AM Matthew Auld wrote:
>
> On 16/04/2021 17:38, Jason Ekstrand wrote:
> > On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld
> > wrote:
> >>
> >> Add an entry for the new uAPI needed for DG1.
> >>
> >> v2(Daniel):
> >>- include the overall upstreaming plan
> >>- add a note for mmap, there are differences here for TTM vs i915
> >>- bunch of other suggestions from Daniel
> >> v3:
> >> (Daniel)
> >>- add a note for set/get caching stuff
> >>- add some more docs for existing query and extensions stuff
> >>- add an actual code example for regions query
> >>- bunch of other stuff
> >> (Jason)
> >>- uAPI change(!):
> >> - try a simpler design with the placements extension
> >> - rather than have a generic setparam which can cover multiple
> >>use cases, have each extension be responsible for one thing
> >>only
> >>
> >> Signed-off-by: Matthew Auld
> >> Cc: Joonas Lahtinen
> >> Cc: Jordan Justen
> >> Cc: Daniel Vetter
> >> Cc: Kenneth Graunke
> >> Cc: Jason Ekstrand
> >> Cc: Dave Airlie
> >> Cc: dri-de...@lists.freedesktop.org
> >> Cc: mesa-dev@lists.freedesktop.org
> >> ---
> >> Documentation/gpu/rfc/i915_gem_lmem.h | 255
> >> Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
> >> Documentation/gpu/rfc/index.rst | 4 +
> >> 3 files changed, 398 insertions(+)
> >> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
> >> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
> >>
> >> diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
> >> b/Documentation/gpu/rfc/i915_gem_lmem.h
> >> new file mode 100644
> >> index ..2a82a452e9f2
> >> --- /dev/null
> >> +++ b/Documentation/gpu/rfc/i915_gem_lmem.h
> >> @@ -0,0 +1,255 @@
> >> +/*
> >> + * Note that drm_i915_query_item and drm_i915_query are existing bits of
> >> uAPI.
> >> + * For the regions query we are just adding a new query id, so no actual
> >> new
> >> + * ioctl or anything, but including it here for reference.
> >> + */
> >> +struct drm_i915_query_item {
> >> +#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
> >> +
> >> +__u64 query_id;
> >> +
> >> +/*
> >> + * When set to zero by userspace, this is filled with the size of
> >> the
> >> + * data to be written at the data_ptr pointer. The kernel sets
> >> this
> >> + * value to a negative value to signal an error on a particular
> >> query
> >> + * item.
> >> + */
> >> +__s32 length;
> >> +
> >> +__u32 flags;
> >> +/*
> >> + * Data will be written at the location pointed by data_ptr when
> >> the
> >> + * value of length matches the length of the data to be written
> >> by the
> >> + * kernel.
> >> + */
> >> +__u64 data_ptr;
> >> +};
> >> +
> >> +struct drm_i915_query {
> >> +__u32 num_items;
> >> +/*
> >> + * Unused for now. Must be cleared to zero.
> >> + */
> >> +__u32 flags;
> >> +/*
> >> + * This points to an array of num_items drm_i915_query_item
> >> structures.
> >> + */
> >> +__u64 items_ptr;
> >> +};
> >> +
> >> +#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE +
> >> DRM_I915_QUERY, struct drm_i915_query)
> >> +
> >> +/**
> >> + * enum drm_i915_gem_memory_class
> >> + */
> >> +enum drm_i915_gem_memory_class {
> >> + /** @I915_MEMORY_CLASS_SYSTEM: system memory */
> >> + I915_MEMORY_CLASS_SYSTEM = 0,
> >> + /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
> >> + I915_MEMORY_CLASS_DEVICE,
> >> +};
> >> +
> >> +/**
> >> + * struct drm_i915_gem_memory_class_instance
> >> + */
> >> +struct drm_i915_gem_memory_class_instance {
> >> + /** @memory_class: see enum drm_i915_gem_memory_class */
> >> + __u16 memory_class;
> >> +
> >> + /** @memory_instance: which instance */
> >> + __u16 memory_instance;
> >> +};
> >> +
> >> +/**
> >> + * struct drm_i915_memory_region_info
> >> + *
> >> + * Describes one region as known to the driver.
> >> + *
> >> + * Note that we reserve quite a lot of stuff here for potential future
> >> work. As
> >> + * an example we might want expose the capabilities(see caps) for a given
> >> + * region, which could include things like if the region is CPU
> >> + * mappable/accessible etc.
> >
> > I get caps but I'm seriously at a loss as to what the rest of this
> > would be used for. Why are caps and flags both there and separate?
> > Flags are typically something you set, not query. Also, what's with
> > rsvd1 at the end? This smells of substantial over-building to me.
> >
> > I thought to myself, "maybe I'm missing a future use-case" so I looked
> > at the internal tree and none of this is being used there either.
> > This indicates to me that either I'm missing something and there's
> > code somewhere I don't know about
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On 16/04/2021 17:38, Jason Ekstrand wrote:
On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld wrote:
Add an entry for the new uAPI needed for DG1.
v2(Daniel):
- include the overall upstreaming plan
- add a note for mmap, there are differences here for TTM vs i915
- bunch of other suggestions from Daniel
v3:
(Daniel)
- add a note for set/get caching stuff
- add some more docs for existing query and extensions stuff
- add an actual code example for regions query
- bunch of other stuff
(Jason)
- uAPI change(!):
- try a simpler design with the placements extension
- rather than have a generic setparam which can cover multiple
use cases, have each extension be responsible for one thing
only
Signed-off-by: Matthew Auld
Cc: Joonas Lahtinen
Cc: Jordan Justen
Cc: Daniel Vetter
Cc: Kenneth Graunke
Cc: Jason Ekstrand
Cc: Dave Airlie
Cc: dri-de...@lists.freedesktop.org
Cc: mesa-dev@lists.freedesktop.org
---
Documentation/gpu/rfc/i915_gem_lmem.h | 255
Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
Documentation/gpu/rfc/index.rst | 4 +
3 files changed, 398 insertions(+)
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
b/Documentation/gpu/rfc/i915_gem_lmem.h
new file mode 100644
index ..2a82a452e9f2
--- /dev/null
+++ b/Documentation/gpu/rfc/i915_gem_lmem.h
@@ -0,0 +1,255 @@
+/*
+ * Note that drm_i915_query_item and drm_i915_query are existing bits of uAPI.
+ * For the regions query we are just adding a new query id, so no actual new
+ * ioctl or anything, but including it here for reference.
+ */
+struct drm_i915_query_item {
+#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
+
+__u64 query_id;
+
+/*
+ * When set to zero by userspace, this is filled with the size of the
+ * data to be written at the data_ptr pointer. The kernel sets this
+ * value to a negative value to signal an error on a particular query
+ * item.
+ */
+__s32 length;
+
+__u32 flags;
+/*
+ * Data will be written at the location pointed by data_ptr when the
+ * value of length matches the length of the data to be written by the
+ * kernel.
+ */
+__u64 data_ptr;
+};
+
+struct drm_i915_query {
+__u32 num_items;
+/*
+ * Unused for now. Must be cleared to zero.
+ */
+__u32 flags;
+/*
+ * This points to an array of num_items drm_i915_query_item structures.
+ */
+__u64 items_ptr;
+};
+
+#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY,
struct drm_i915_query)
+
+/**
+ * enum drm_i915_gem_memory_class
+ */
+enum drm_i915_gem_memory_class {
+ /** @I915_MEMORY_CLASS_SYSTEM: system memory */
+ I915_MEMORY_CLASS_SYSTEM = 0,
+ /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
+ I915_MEMORY_CLASS_DEVICE,
+};
+
+/**
+ * struct drm_i915_gem_memory_class_instance
+ */
+struct drm_i915_gem_memory_class_instance {
+ /** @memory_class: see enum drm_i915_gem_memory_class */
+ __u16 memory_class;
+
+ /** @memory_instance: which instance */
+ __u16 memory_instance;
+};
+
+/**
+ * struct drm_i915_memory_region_info
+ *
+ * Describes one region as known to the driver.
+ *
+ * Note that we reserve quite a lot of stuff here for potential future work. As
+ * an example we might want expose the capabilities(see caps) for a given
+ * region, which could include things like if the region is CPU
+ * mappable/accessible etc.
I get caps but I'm seriously at a loss as to what the rest of this
would be used for. Why are caps and flags both there and separate?
Flags are typically something you set, not query. Also, what's with
rsvd1 at the end? This smells of substantial over-building to me.
I thought to myself, "maybe I'm missing a future use-case" so I looked
at the internal tree and none of this is being used there either.
This indicates to me that either I'm missing something and there's
code somewhere I don't know about or, with three years of building on
internal branches, we still haven't proven that any of this is needed.
If it's the latter, which I strongly suspect, maybe we should drop the
unnecessary bits and only add them back in if and when we have proof
that they're useful.
Do you mean just drop caps/flags here, but keep/inflate rsvd0/rsvd1,
which is less opinionated about future unknowns? If so, makes sense to me.
To be clear, I don't mind the query API as such and the class/instance
stuff seems fine and I really like being able to get the sizes
directly. What concerns me is all this extra future-proofing that we
have zero proof is actually useful. In my experience, when you build
out like this without so much as a
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On 4/16/2021 10:02 AM, Daniel Vetter wrote:
On Fri, Apr 16, 2021 at 6:38 PM Jason Ekstrand wrote:
On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld wrote:
Add an entry for the new uAPI needed for DG1.
v2(Daniel):
- include the overall upstreaming plan
- add a note for mmap, there are differences here for TTM vs i915
- bunch of other suggestions from Daniel
v3:
(Daniel)
- add a note for set/get caching stuff
- add some more docs for existing query and extensions stuff
- add an actual code example for regions query
- bunch of other stuff
(Jason)
- uAPI change(!):
- try a simpler design with the placements extension
- rather than have a generic setparam which can cover multiple
use cases, have each extension be responsible for one thing
only
Signed-off-by: Matthew Auld
Cc: Joonas Lahtinen
Cc: Jordan Justen
Cc: Daniel Vetter
Cc: Kenneth Graunke
Cc: Jason Ekstrand
Cc: Dave Airlie
Cc: dri-de...@lists.freedesktop.org
Cc: mesa-dev@lists.freedesktop.org
---
Documentation/gpu/rfc/i915_gem_lmem.h | 255
Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
Documentation/gpu/rfc/index.rst | 4 +
3 files changed, 398 insertions(+)
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
b/Documentation/gpu/rfc/i915_gem_lmem.h
new file mode 100644
index ..2a82a452e9f2
--- /dev/null
+++ b/Documentation/gpu/rfc/i915_gem_lmem.h
@@ -0,0 +1,255 @@
+/*
+ * Note that drm_i915_query_item and drm_i915_query are existing bits of uAPI.
+ * For the regions query we are just adding a new query id, so no actual new
+ * ioctl or anything, but including it here for reference.
+ */
+struct drm_i915_query_item {
+#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
+
+__u64 query_id;
+
+/*
+ * When set to zero by userspace, this is filled with the size of the
+ * data to be written at the data_ptr pointer. The kernel sets this
+ * value to a negative value to signal an error on a particular query
+ * item.
+ */
+__s32 length;
+
+__u32 flags;
+/*
+ * Data will be written at the location pointed by data_ptr when the
+ * value of length matches the length of the data to be written by the
+ * kernel.
+ */
+__u64 data_ptr;
+};
+
+struct drm_i915_query {
+__u32 num_items;
+/*
+ * Unused for now. Must be cleared to zero.
+ */
+__u32 flags;
+/*
+ * This points to an array of num_items drm_i915_query_item structures.
+ */
+__u64 items_ptr;
+};
+
+#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY,
struct drm_i915_query)
+
+/**
+ * enum drm_i915_gem_memory_class
+ */
+enum drm_i915_gem_memory_class {
+ /** @I915_MEMORY_CLASS_SYSTEM: system memory */
+ I915_MEMORY_CLASS_SYSTEM = 0,
+ /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
+ I915_MEMORY_CLASS_DEVICE,
+};
+
+/**
+ * struct drm_i915_gem_memory_class_instance
+ */
+struct drm_i915_gem_memory_class_instance {
+ /** @memory_class: see enum drm_i915_gem_memory_class */
+ __u16 memory_class;
+
+ /** @memory_instance: which instance */
+ __u16 memory_instance;
+};
+
+/**
+ * struct drm_i915_memory_region_info
+ *
+ * Describes one region as known to the driver.
+ *
+ * Note that we reserve quite a lot of stuff here for potential future work. As
+ * an example we might want expose the capabilities(see caps) for a given
+ * region, which could include things like if the region is CPU
+ * mappable/accessible etc.
I get caps but I'm seriously at a loss as to what the rest of this
would be used for. Why are caps and flags both there and separate?
Flags are typically something you set, not query. Also, what's with
rsvd1 at the end? This smells of substantial over-building to me.
I thought to myself, "maybe I'm missing a future use-case" so I looked
at the internal tree and none of this is being used there either.
This indicates to me that either I'm missing something and there's
code somewhere I don't know about or, with three years of building on
internal branches, we still haven't proven that any of this is needed.
If it's the latter, which I strongly suspect, maybe we should drop the
unnecessary bits and only add them back in if and when we have proof
that they're useful.
To be clear, I don't mind the query API as such and the class/instance
stuff seems fine and I really like being able to get the sizes
directly. What concerns me is all this extra future-proofing that we
have zero proof is actually useful. In my experience, when you build
out like this without so much as a use-case, you always end up
building the wrong thing.
+ */
+struct
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On Fri, Apr 16, 2021 at 6:38 PM Jason Ekstrand wrote:
>
> On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld wrote:
> >
> > Add an entry for the new uAPI needed for DG1.
> >
> > v2(Daniel):
> > - include the overall upstreaming plan
> > - add a note for mmap, there are differences here for TTM vs i915
> > - bunch of other suggestions from Daniel
> > v3:
> > (Daniel)
> > - add a note for set/get caching stuff
> > - add some more docs for existing query and extensions stuff
> > - add an actual code example for regions query
> > - bunch of other stuff
> > (Jason)
> > - uAPI change(!):
> > - try a simpler design with the placements extension
> > - rather than have a generic setparam which can cover multiple
> > use cases, have each extension be responsible for one thing
> > only
> >
> > Signed-off-by: Matthew Auld
> > Cc: Joonas Lahtinen
> > Cc: Jordan Justen
> > Cc: Daniel Vetter
> > Cc: Kenneth Graunke
> > Cc: Jason Ekstrand
> > Cc: Dave Airlie
> > Cc: dri-de...@lists.freedesktop.org
> > Cc: mesa-dev@lists.freedesktop.org
> > ---
> > Documentation/gpu/rfc/i915_gem_lmem.h | 255
> > Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
> > Documentation/gpu/rfc/index.rst | 4 +
> > 3 files changed, 398 insertions(+)
> > create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
> > create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
> >
> > diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
> > b/Documentation/gpu/rfc/i915_gem_lmem.h
> > new file mode 100644
> > index ..2a82a452e9f2
> > --- /dev/null
> > +++ b/Documentation/gpu/rfc/i915_gem_lmem.h
> > @@ -0,0 +1,255 @@
> > +/*
> > + * Note that drm_i915_query_item and drm_i915_query are existing bits of
> > uAPI.
> > + * For the regions query we are just adding a new query id, so no actual
> > new
> > + * ioctl or anything, but including it here for reference.
> > + */
> > +struct drm_i915_query_item {
> > +#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
> > +
> > +__u64 query_id;
> > +
> > +/*
> > + * When set to zero by userspace, this is filled with the size of
> > the
> > + * data to be written at the data_ptr pointer. The kernel sets this
> > + * value to a negative value to signal an error on a particular
> > query
> > + * item.
> > + */
> > +__s32 length;
> > +
> > +__u32 flags;
> > +/*
> > + * Data will be written at the location pointed by data_ptr when
> > the
> > + * value of length matches the length of the data to be written by
> > the
> > + * kernel.
> > + */
> > +__u64 data_ptr;
> > +};
> > +
> > +struct drm_i915_query {
> > +__u32 num_items;
> > +/*
> > + * Unused for now. Must be cleared to zero.
> > + */
> > +__u32 flags;
> > +/*
> > + * This points to an array of num_items drm_i915_query_item
> > structures.
> > + */
> > +__u64 items_ptr;
> > +};
> > +
> > +#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY,
> > struct drm_i915_query)
> > +
> > +/**
> > + * enum drm_i915_gem_memory_class
> > + */
> > +enum drm_i915_gem_memory_class {
> > + /** @I915_MEMORY_CLASS_SYSTEM: system memory */
> > + I915_MEMORY_CLASS_SYSTEM = 0,
> > + /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
> > + I915_MEMORY_CLASS_DEVICE,
> > +};
> > +
> > +/**
> > + * struct drm_i915_gem_memory_class_instance
> > + */
> > +struct drm_i915_gem_memory_class_instance {
> > + /** @memory_class: see enum drm_i915_gem_memory_class */
> > + __u16 memory_class;
> > +
> > + /** @memory_instance: which instance */
> > + __u16 memory_instance;
> > +};
> > +
> > +/**
> > + * struct drm_i915_memory_region_info
> > + *
> > + * Describes one region as known to the driver.
> > + *
> > + * Note that we reserve quite a lot of stuff here for potential future
> > work. As
> > + * an example we might want expose the capabilities(see caps) for a given
> > + * region, which could include things like if the region is CPU
> > + * mappable/accessible etc.
>
> I get caps but I'm seriously at a loss as to what the rest of this
> would be used for. Why are caps and flags both there and separate?
> Flags are typically something you set, not query. Also, what's with
> rsvd1 at the end? This smells of substantial over-building to me.
>
> I thought to myself, "maybe I'm missing a future use-case" so I looked
> at the internal tree and none of this is being used there either.
> This indicates to me that either I'm missing something and there's
> code somewhere I don't know about or, with three years of building on
> internal branches, we still haven't proven that any of this is needed.
> If it's the latter, which I strongly suspect, maybe we should drop the
> unnecessary bits and only add them
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On Thu, Apr 15, 2021 at 11:04 AM Matthew Auld wrote:
>
> Add an entry for the new uAPI needed for DG1.
>
> v2(Daniel):
> - include the overall upstreaming plan
> - add a note for mmap, there are differences here for TTM vs i915
> - bunch of other suggestions from Daniel
> v3:
> (Daniel)
> - add a note for set/get caching stuff
> - add some more docs for existing query and extensions stuff
> - add an actual code example for regions query
> - bunch of other stuff
> (Jason)
> - uAPI change(!):
> - try a simpler design with the placements extension
> - rather than have a generic setparam which can cover multiple
> use cases, have each extension be responsible for one thing
> only
>
> Signed-off-by: Matthew Auld
> Cc: Joonas Lahtinen
> Cc: Jordan Justen
> Cc: Daniel Vetter
> Cc: Kenneth Graunke
> Cc: Jason Ekstrand
> Cc: Dave Airlie
> Cc: dri-de...@lists.freedesktop.org
> Cc: mesa-dev@lists.freedesktop.org
> ---
> Documentation/gpu/rfc/i915_gem_lmem.h | 255
> Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
> Documentation/gpu/rfc/index.rst | 4 +
> 3 files changed, 398 insertions(+)
> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
>
> diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
> b/Documentation/gpu/rfc/i915_gem_lmem.h
> new file mode 100644
> index ..2a82a452e9f2
> --- /dev/null
> +++ b/Documentation/gpu/rfc/i915_gem_lmem.h
> @@ -0,0 +1,255 @@
> +/*
> + * Note that drm_i915_query_item and drm_i915_query are existing bits of
> uAPI.
> + * For the regions query we are just adding a new query id, so no actual new
> + * ioctl or anything, but including it here for reference.
> + */
> +struct drm_i915_query_item {
> +#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
> +
> +__u64 query_id;
> +
> +/*
> + * When set to zero by userspace, this is filled with the size of the
> + * data to be written at the data_ptr pointer. The kernel sets this
> + * value to a negative value to signal an error on a particular query
> + * item.
> + */
> +__s32 length;
> +
> +__u32 flags;
> +/*
> + * Data will be written at the location pointed by data_ptr when the
> + * value of length matches the length of the data to be written by
> the
> + * kernel.
> + */
> +__u64 data_ptr;
> +};
> +
> +struct drm_i915_query {
> +__u32 num_items;
> +/*
> + * Unused for now. Must be cleared to zero.
> + */
> +__u32 flags;
> +/*
> + * This points to an array of num_items drm_i915_query_item
> structures.
> + */
> +__u64 items_ptr;
> +};
> +
> +#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY,
> struct drm_i915_query)
> +
> +/**
> + * enum drm_i915_gem_memory_class
> + */
> +enum drm_i915_gem_memory_class {
> + /** @I915_MEMORY_CLASS_SYSTEM: system memory */
> + I915_MEMORY_CLASS_SYSTEM = 0,
> + /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
> + I915_MEMORY_CLASS_DEVICE,
> +};
> +
> +/**
> + * struct drm_i915_gem_memory_class_instance
> + */
> +struct drm_i915_gem_memory_class_instance {
> + /** @memory_class: see enum drm_i915_gem_memory_class */
> + __u16 memory_class;
> +
> + /** @memory_instance: which instance */
> + __u16 memory_instance;
> +};
> +
> +/**
> + * struct drm_i915_memory_region_info
> + *
> + * Describes one region as known to the driver.
> + *
> + * Note that we reserve quite a lot of stuff here for potential future work.
> As
> + * an example we might want expose the capabilities(see caps) for a given
> + * region, which could include things like if the region is CPU
> + * mappable/accessible etc.
I get caps but I'm seriously at a loss as to what the rest of this
would be used for. Why are caps and flags both there and separate?
Flags are typically something you set, not query. Also, what's with
rsvd1 at the end? This smells of substantial over-building to me.
I thought to myself, "maybe I'm missing a future use-case" so I looked
at the internal tree and none of this is being used there either.
This indicates to me that either I'm missing something and there's
code somewhere I don't know about or, with three years of building on
internal branches, we still haven't proven that any of this is needed.
If it's the latter, which I strongly suspect, maybe we should drop the
unnecessary bits and only add them back in if and when we have proof
that they're useful.
To be clear, I don't mind the query API as such and the class/instance
stuff seems fine and I really like being able to get the sizes
directly. What concerns me is all this extra future-proofing that we
have zero proof is actually useful. In my experience, when you build
out like this
Re: [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI
On Thu, Apr 15, 2021 at 04:59:58PM +0100, Matthew Auld wrote:
> Add an entry for the new uAPI needed for DG1.
>
> v2(Daniel):
> - include the overall upstreaming plan
> - add a note for mmap, there are differences here for TTM vs i915
> - bunch of other suggestions from Daniel
> v3:
> (Daniel)
> - add a note for set/get caching stuff
> - add some more docs for existing query and extensions stuff
> - add an actual code example for regions query
> - bunch of other stuff
> (Jason)
> - uAPI change(!):
> - try a simpler design with the placements extension
> - rather than have a generic setparam which can cover multiple
> use cases, have each extension be responsible for one thing
> only
>
> Signed-off-by: Matthew Auld
> Cc: Joonas Lahtinen
> Cc: Jordan Justen
> Cc: Daniel Vetter
> Cc: Kenneth Graunke
> Cc: Jason Ekstrand
> Cc: Dave Airlie
> Cc: dri-de...@lists.freedesktop.org
> Cc: mesa-dev@lists.freedesktop.org
> ---
> Documentation/gpu/rfc/i915_gem_lmem.h | 255
> Documentation/gpu/rfc/i915_gem_lmem.rst | 139 +
> Documentation/gpu/rfc/index.rst | 4 +
> 3 files changed, 398 insertions(+)
> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.h
> create mode 100644 Documentation/gpu/rfc/i915_gem_lmem.rst
>
> diff --git a/Documentation/gpu/rfc/i915_gem_lmem.h
> b/Documentation/gpu/rfc/i915_gem_lmem.h
> new file mode 100644
> index ..2a82a452e9f2
> --- /dev/null
> +++ b/Documentation/gpu/rfc/i915_gem_lmem.h
> @@ -0,0 +1,255 @@
> +/*
> + * Note that drm_i915_query_item and drm_i915_query are existing bits of
> uAPI.
> + * For the regions query we are just adding a new query id, so no actual new
> + * ioctl or anything, but including it here for reference.
> + */
Oops I didn't realize this. I think the better/prettier way is to just
mention how it's built on top of the query ioctl and structs, and use
kerneldoc hyperlinks to point there. That way it's still easy to find, and
also serves as better documentation for the uapi when it's all merged.
See
https://dri.freedesktop.org/docs/drm/doc-guide/kernel-doc.html#highlights-and-cross-references
That's also why it matters that we pull the kerneldoc into our overall
documentation, otherwise the hyperlinks aren't working.
> +struct drm_i915_query_item {
> +#define DRM_I915_QUERY_MEMORY_REGIONS 0xdeadbeaf
> +
> +__u64 query_id;
> +
> +/*
> + * When set to zero by userspace, this is filled with the size of the
> + * data to be written at the data_ptr pointer. The kernel sets this
> + * value to a negative value to signal an error on a particular query
> + * item.
> + */
> +__s32 length;
> +
> +__u32 flags;
> +/*
> + * Data will be written at the location pointed by data_ptr when the
> + * value of length matches the length of the data to be written by
> the
> + * kernel.
> + */
> +__u64 data_ptr;
> +};
> +
> +struct drm_i915_query {
> +__u32 num_items;
> +/*
> + * Unused for now. Must be cleared to zero.
> + */
> +__u32 flags;
> +/*
> + * This points to an array of num_items drm_i915_query_item
> structures.
> + */
> +__u64 items_ptr;
> +};
> +
> +#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY,
> struct drm_i915_query)
> +
> +/**
> + * enum drm_i915_gem_memory_class
> + */
> +enum drm_i915_gem_memory_class {
> + /** @I915_MEMORY_CLASS_SYSTEM: system memory */
> + I915_MEMORY_CLASS_SYSTEM = 0,
> + /** @I915_MEMORY_CLASS_DEVICE: device local-memory */
> + I915_MEMORY_CLASS_DEVICE,
> +};
> +
> +/**
> + * struct drm_i915_gem_memory_class_instance
> + */
> +struct drm_i915_gem_memory_class_instance {
> + /** @memory_class: see enum drm_i915_gem_memory_class */
> + __u16 memory_class;
> +
> + /** @memory_instance: which instance */
> + __u16 memory_instance;
> +};
> +
> +/**
> + * struct drm_i915_memory_region_info
> + *
> + * Describes one region as known to the driver.
> + *
> + * Note that we reserve quite a lot of stuff here for potential future work.
> As
> + * an example we might want expose the capabilities(see caps) for a given
> + * region, which could include things like if the region is CPU
> + * mappable/accessible etc.
> + */
> +struct drm_i915_memory_region_info {
> + /** @region: class:instance pair encoding */
> + struct drm_i915_gem_memory_class_instance region;
> +
> + /** @rsvd0: MBZ */
> + __u32 rsvd0;
> +
> + /** @caps: MBZ */
> + __u64 caps;
> +
> + /** @flags: MBZ */
> + __u64 flags;
> +
> + /** @probed_size: Memory probed by the driver (-1 = unknown) */
> + __u64 probed_size;
> +
> + /** @unallocated_size: Estimate of memory remaining (-1 = unknown) */
> + __u64 unallocated_size;
> +
> + /** @rsvd1: MBZ */
> +
