| Message ID | 20210803222943.27686-15-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Parallel submission aka multi-bb execbuf | expand |
On Tue, Aug 03, 2021 at 03:29:11PM -0700, Matthew Brost wrote: > Expose logical engine instance to user via query engine info IOCTL. This > is required for split-frame workloads as these needs to be placed on > engines in a logically contiguous order. The logical mapping can change > based on fusing. Rather than having user have knowledge of the fusing we > simply just expose the logical mapping with the existing query engine > info IOCTL. > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Signed-off-by: Matthew Brost <matthew.brost@intel.com> Uapi must have a link to the userspace MR/patch set using this, and to the igt patch set validating it. Ideally in each patch, since it's way too hard to unfortunately find the cover letter late on. Jason even went as far as making this a hard requirement because he wasted a bit too much time trying to find the userspace for new uapi: https://lore.kernel.org/dri-devel/20210804185704.624883-1-jason@jlekstrand.net/ Cheers, Daniel >--- > drivers/gpu/drm/i915/i915_query.c | 2 ++ > include/uapi/drm/i915_drm.h | 8 +++++++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > index e49da36c62fb..8a72923fbdba 100644 > --- a/drivers/gpu/drm/i915/i915_query.c > +++ b/drivers/gpu/drm/i915/i915_query.c > @@ -124,7 +124,9 @@ query_engine_info(struct drm_i915_private *i915, > for_each_uabi_engine(engine, i915) { > info.engine.engine_class = engine->uabi_class; > info.engine.engine_instance = engine->uabi_instance; > + info.flags = I915_ENGINE_INFO_HAS_LOGICAL_INSTANCE; > info.capabilities = engine->uabi_capabilities; > + info.logical_instance = ilog2(engine->logical_mask); > > if (copy_to_user(info_ptr, &info, sizeof(info))) > return -EFAULT; > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 7f13d241417f..ef72e07fe08c 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -2706,14 +2706,20 @@ struct drm_i915_engine_info { > > /** @flags: Engine flags. */ > __u64 flags; > +#define I915_ENGINE_INFO_HAS_LOGICAL_INSTANCE (1 << 0) > > /** @capabilities: Capabilities of this engine. */ > __u64 capabilities; > #define I915_VIDEO_CLASS_CAPABILITY_HEVC (1 << 0) > #define I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC (1 << 1) > > + /** @logical_instance: Logical instance of engine */ > + __u16 logical_instance; > + > /** @rsvd1: Reserved fields. */ > - __u64 rsvd1[4]; > + __u16 rsvd1[3]; > + /** @rsvd2: Reserved fields. */ > + __u64 rsvd2[3]; > }; > > /** > -- > 2.28.0 >
On Mon, Aug 09, 2021 at 04:30:06PM +0200, Daniel Vetter wrote: > On Tue, Aug 03, 2021 at 03:29:11PM -0700, Matthew Brost wrote: > > Expose logical engine instance to user via query engine info IOCTL. This > > is required for split-frame workloads as these needs to be placed on > > engines in a logically contiguous order. The logical mapping can change > > based on fusing. Rather than having user have knowledge of the fusing we > > simply just expose the logical mapping with the existing query engine > > info IOCTL. > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > Uapi must have a link to the userspace MR/patch set using this, and to the > igt patch set validating it. > Have an IGT: https://patchwork.freedesktop.org/patch/447008/?series=93071&rev=1 Not sure when the media UMD is going to be updated upstream to use this. Does that mean I can't merge this until the media UMD is ready? Seems like it but isn't that a circular dependency? How can the media team develop for a new uAPI that isn't in the kernel yet? For what it is worth the downstream release is already using this. Matt > Ideally in each patch, since it's way too hard to unfortunately find the > cover letter late on. > > Jason even went as far as making this a hard requirement because he wasted > a bit too much time trying to find the userspace for new uapi: > > https://lore.kernel.org/dri-devel/20210804185704.624883-1-jason@jlekstrand.net/ > > Cheers, Daniel > > >--- > > drivers/gpu/drm/i915/i915_query.c | 2 ++ > > include/uapi/drm/i915_drm.h | 8 +++++++- > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > > index e49da36c62fb..8a72923fbdba 100644 > > --- a/drivers/gpu/drm/i915/i915_query.c > > +++ b/drivers/gpu/drm/i915/i915_query.c > > @@ -124,7 +124,9 @@ query_engine_info(struct drm_i915_private *i915, > > for_each_uabi_engine(engine, i915) { > > info.engine.engine_class = engine->uabi_class; > > info.engine.engine_instance = engine->uabi_instance; > > + info.flags = I915_ENGINE_INFO_HAS_LOGICAL_INSTANCE; > > info.capabilities = engine->uabi_capabilities; > > + info.logical_instance = ilog2(engine->logical_mask); > > > > if (copy_to_user(info_ptr, &info, sizeof(info))) > > return -EFAULT; > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index 7f13d241417f..ef72e07fe08c 100644 > > --- a/include/uapi/drm/i915_drm.h > > +++ b/include/uapi/drm/i915_drm.h > > @@ -2706,14 +2706,20 @@ struct drm_i915_engine_info { > > > > /** @flags: Engine flags. */ > > __u64 flags; > > +#define I915_ENGINE_INFO_HAS_LOGICAL_INSTANCE (1 << 0) > > > > /** @capabilities: Capabilities of this engine. */ > > __u64 capabilities; > > #define I915_VIDEO_CLASS_CAPABILITY_HEVC (1 << 0) > > #define I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC (1 << 1) > > > > + /** @logical_instance: Logical instance of engine */ > > + __u16 logical_instance; > > + > > /** @rsvd1: Reserved fields. */ > > - __u64 rsvd1[4]; > > + __u16 rsvd1[3]; > > + /** @rsvd2: Reserved fields. */ > > + __u64 rsvd2[3]; > > }; > > > > /** > > -- > > 2.28.0 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Mon, Aug 09, 2021 at 06:37:01PM +0000, Matthew Brost wrote: > On Mon, Aug 09, 2021 at 04:30:06PM +0200, Daniel Vetter wrote: > > On Tue, Aug 03, 2021 at 03:29:11PM -0700, Matthew Brost wrote: > > > Expose logical engine instance to user via query engine info IOCTL. This > > > is required for split-frame workloads as these needs to be placed on > > > engines in a logically contiguous order. The logical mapping can change > > > based on fusing. Rather than having user have knowledge of the fusing we > > > simply just expose the logical mapping with the existing query engine > > > info IOCTL. > > > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > > > Uapi must have a link to the userspace MR/patch set using this, and to the > > igt patch set validating it. > > > > Have an IGT: > https://patchwork.freedesktop.org/patch/447008/?series=93071&rev=1 > > Not sure when the media UMD is going to be updated upstream to use this. > Does that mean I can't merge this until the media UMD is ready? Seems > like it but isn't that a circular dependency? How can the media team > develop for a new uAPI that isn't in the kernel yet? Yes and no. Full explainer here: https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements In the drm subsystem this is pretty much the only rule where if you break it the book will be thrown at you with extreme prejudice. Also wrt circular: If the umd aren't set up to test their branches against kernel branches they need to fix their stuff. I know that internally that's not been done, and its a disaster, but in upstream there's no room for excuses. Both kernel and userspace needs to be in branches until it's ready for merging. > For what it is worth the downstream release is already using this. Yeah which is another problem, shipping new uapi in downstream before it's in upstream is decidedly not great. -Daniel > > Matt > > > Ideally in each patch, since it's way too hard to unfortunately find the > > cover letter late on. > > > > Jason even went as far as making this a hard requirement because he wasted > > a bit too much time trying to find the userspace for new uapi: > > > > https://lore.kernel.org/dri-devel/20210804185704.624883-1-jason@jlekstrand.net/ > > > > Cheers, Daniel > > > > >--- > > > drivers/gpu/drm/i915/i915_query.c | 2 ++ > > > include/uapi/drm/i915_drm.h | 8 +++++++- > > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > > > index e49da36c62fb..8a72923fbdba 100644 > > > --- a/drivers/gpu/drm/i915/i915_query.c > > > +++ b/drivers/gpu/drm/i915/i915_query.c > > > @@ -124,7 +124,9 @@ query_engine_info(struct drm_i915_private *i915, > > > for_each_uabi_engine(engine, i915) { > > > info.engine.engine_class = engine->uabi_class; > > > info.engine.engine_instance = engine->uabi_instance; > > > + info.flags = I915_ENGINE_INFO_HAS_LOGICAL_INSTANCE; > > > info.capabilities = engine->uabi_capabilities; > > > + info.logical_instance = ilog2(engine->logical_mask); > > > > > > if (copy_to_user(info_ptr, &info, sizeof(info))) > > > return -EFAULT; > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > > index 7f13d241417f..ef72e07fe08c 100644 > > > --- a/include/uapi/drm/i915_drm.h > > > +++ b/include/uapi/drm/i915_drm.h > > > @@ -2706,14 +2706,20 @@ struct drm_i915_engine_info { > > > > > > /** @flags: Engine flags. */ > > > __u64 flags; > > > +#define I915_ENGINE_INFO_HAS_LOGICAL_INSTANCE (1 << 0) > > > > > > /** @capabilities: Capabilities of this engine. */ > > > __u64 capabilities; > > > #define I915_VIDEO_CLASS_CAPABILITY_HEVC (1 << 0) > > > #define I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC (1 << 1) > > > > > > + /** @logical_instance: Logical instance of engine */ > > > + __u16 logical_instance; > > > + > > > /** @rsvd1: Reserved fields. */ > > > - __u64 rsvd1[4]; > > > + __u16 rsvd1[3]; > > > + /** @rsvd2: Reserved fields. */ > > > + __u64 rsvd2[3]; > > > }; > > > > > > /** > > > -- > > > 2.28.0 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
On Tue, Aug 10, 2021 at 08:53:16AM +0200, Daniel Vetter wrote: > On Mon, Aug 09, 2021 at 06:37:01PM +0000, Matthew Brost wrote: > > On Mon, Aug 09, 2021 at 04:30:06PM +0200, Daniel Vetter wrote: > > > On Tue, Aug 03, 2021 at 03:29:11PM -0700, Matthew Brost wrote: > > > > Expose logical engine instance to user via query engine info IOCTL. This > > > > is required for split-frame workloads as these needs to be placed on > > > > engines in a logically contiguous order. The logical mapping can change > > > > based on fusing. Rather than having user have knowledge of the fusing we > > > > simply just expose the logical mapping with the existing query engine > > > > info IOCTL. > > > > > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > > > > > Uapi must have a link to the userspace MR/patch set using this, and to the > > > igt patch set validating it. > > > > > > > Have an IGT: > > https://patchwork.freedesktop.org/patch/447008/?series=93071&rev=1 > > > > Not sure when the media UMD is going to be updated upstream to use this. > > Does that mean I can't merge this until the media UMD is ready? Seems > > like it but isn't that a circular dependency? How can the media team > > develop for a new uAPI that isn't in the kernel yet? > > Yes and no. Full explainer here: > > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > > In the drm subsystem this is pretty much the only rule where if you break > it the book will be thrown at you with extreme prejudice. > Well I don't want a book thrown at, new here and trying to play by the rules. > Also wrt circular: If the umd aren't set up to test their branches against > kernel branches they need to fix their stuff. I know that internally > that's not been done, and its a disaster, but in upstream there's no room > for excuses. Both kernel and userspace needs to be in branches until it's > ready for merging. > Ok, looks like a have a few things to learn. I'll coordinate with the media team on this. Likely won't have links to the UMD in the next spin but I'll have a branch for them to prep their patches on. Matt > > For what it is worth the downstream release is already using this. > > Yeah which is another problem, shipping new uapi in downstream before it's > in upstream is decidedly not great. > -Daniel > > > > > Matt > > > > > Ideally in each patch, since it's way too hard to unfortunately find the > > > cover letter late on. > > > > > > Jason even went as far as making this a hard requirement because he wasted > > > a bit too much time trying to find the userspace for new uapi: > > > > > > https://lore.kernel.org/dri-devel/20210804185704.624883-1-jason@jlekstrand.net/ > > > > > > Cheers, Daniel > > > > > > >--- > > > > drivers/gpu/drm/i915/i915_query.c | 2 ++ > > > > include/uapi/drm/i915_drm.h | 8 +++++++- > > > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > > > > index e49da36c62fb..8a72923fbdba 100644 > > > > --- a/drivers/gpu/drm/i915/i915_query.c > > > > +++ b/drivers/gpu/drm/i915/i915_query.c > > > > @@ -124,7 +124,9 @@ query_engine_info(struct drm_i915_private *i915, > > > > for_each_uabi_engine(engine, i915) { > > > > info.engine.engine_class = engine->uabi_class; > > > > info.engine.engine_instance = engine->uabi_instance; > > > > + info.flags = I915_ENGINE_INFO_HAS_LOGICAL_INSTANCE; > > > > info.capabilities = engine->uabi_capabilities; > > > > + info.logical_instance = ilog2(engine->logical_mask); > > > > > > > > if (copy_to_user(info_ptr, &info, sizeof(info))) > > > > return -EFAULT; > > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > > > index 7f13d241417f..ef72e07fe08c 100644 > > > > --- a/include/uapi/drm/i915_drm.h > > > > +++ b/include/uapi/drm/i915_drm.h > > > > @@ -2706,14 +2706,20 @@ struct drm_i915_engine_info { > > > > > > > > /** @flags: Engine flags. */ > > > > __u64 flags; > > > > +#define I915_ENGINE_INFO_HAS_LOGICAL_INSTANCE (1 << 0) > > > > > > > > /** @capabilities: Capabilities of this engine. */ > > > > __u64 capabilities; > > > > #define I915_VIDEO_CLASS_CAPABILITY_HEVC (1 << 0) > > > > #define I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC (1 << 1) > > > > > > > > + /** @logical_instance: Logical instance of engine */ > > > > + __u16 logical_instance; > > > > + > > > > /** @rsvd1: Reserved fields. */ > > > > - __u64 rsvd1[4]; > > > > + __u16 rsvd1[3]; > > > > + /** @rsvd2: Reserved fields. */ > > > > + __u64 rsvd2[3]; > > > > }; > > > > > > > > /** > > > > -- > > > > 2.28.0 > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index e49da36c62fb..8a72923fbdba 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -124,7 +124,9 @@ query_engine_info(struct drm_i915_private *i915, for_each_uabi_engine(engine, i915) { info.engine.engine_class = engine->uabi_class; info.engine.engine_instance = engine->uabi_instance; + info.flags = I915_ENGINE_INFO_HAS_LOGICAL_INSTANCE; info.capabilities = engine->uabi_capabilities; + info.logical_instance = ilog2(engine->logical_mask); if (copy_to_user(info_ptr, &info, sizeof(info))) return -EFAULT; diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 7f13d241417f..ef72e07fe08c 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -2706,14 +2706,20 @@ struct drm_i915_engine_info { /** @flags: Engine flags. */ __u64 flags; +#define I915_ENGINE_INFO_HAS_LOGICAL_INSTANCE (1 << 0) /** @capabilities: Capabilities of this engine. */ __u64 capabilities; #define I915_VIDEO_CLASS_CAPABILITY_HEVC (1 << 0) #define I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC (1 << 1) + /** @logical_instance: Logical instance of engine */ + __u16 logical_instance; + /** @rsvd1: Reserved fields. */ - __u64 rsvd1[4]; + __u16 rsvd1[3]; + /** @rsvd2: Reserved fields. */ + __u64 rsvd2[3]; }; /**
Expose logical engine instance to user via query engine info IOCTL. This is required for split-frame workloads as these needs to be placed on engines in a logically contiguous order. The logical mapping can change based on fusing. Rather than having user have knowledge of the fusing we simply just expose the logical mapping with the existing query engine info IOCTL. Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- drivers/gpu/drm/i915/i915_query.c | 2 ++ include/uapi/drm/i915_drm.h | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-)