drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

From: Tvrtko Ursulin <[email protected]>

Currently our driver assumes BSD2 means hardware engine instance number two. This does not work for Icelake parts with two VCS engines, but which are hardware instances 0 and 2, and not 0 and 1 as with previous parts.

This makes the second engine not discoverable via HAS_BSD2 get param, nor it can be targetted by execbuf.

While we are working on the next generation execbuf put in a hack which allows discovery and access to this second VCS engine using legacy ABI.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Tony Ye <[email protected]>

---
Compile tested only.

Also, one could argue if this is just a temporary hack or could actually make sense to have this permanently in. It feels like the ABI semantics of BSD2 are blurry, or at least could be re-blurred for Gen11.

---
 drivers/gpu/drm/i915/i915_drv.c            |  8 +++++++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +0 -0

index b7dbeba72dec..a185366d9beb 100644
@@ -331,7 +331,13 @@ static int i915_getparam_ioctl(struct drm_device *dev, void *data,
                value = !!dev_priv->engine[VECS];

drivers/gpu/drm/i915/i915_drv.c +0 -0

                break;
        case I915_PARAM_HAS_LLC:
                value = HAS_LLC(dev_priv);

drivers/gpu/drm/i915/i915_gem_execbuffer.c +9 -1

index c74f5df3fb5a..80b32460567d 100644
@@ -2052,7 +2052,9 @@ eb_select_engine(struct drm_i915_private *dev_priv,
                return NULL;
        }
 
-       if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {
+       if (user_ring_id == I915_EXEC_BSD &&
+           (INTEL_INFO(dev_priv)->ring_mask &
+            (ENGINE_MASK(_VCS(1)) | ENGINE_MASK(_VCS(2))))) {
                unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
 
                if (bsd_idx == I915_EXEC_BSD_DEFAULT) {

@@ -2068,6 +2070,12 @@ eb_select_engine(struct drm_i915_private *dev_priv,
                }
 
                engine = dev_priv->engine[_VCS(bsd_idx)];
+
+               /*
+                * FIXME Temporarily map VCS2 to VCS1 on Icelake.
+                */
+               if (!engine && bsd_idx)
+                       engine = dev_priv->engine[_VCS(2)];
        } else {
                engine = dev_priv->engine[user_ring_map[user_ring_id]];
        }

Signature · 6 lines

2.14.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Intel-gfx <[email protected]> On Behalf Of Joonas Lahtinen
Sent: Wednesday, April 18, 2018 3:43 AM
To: [email protected]; Tvrtko Ursulin <[email protected]>
Subject: Re: [Intel-gfx] [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

Quoting Tvrtko Ursulin (2018-04-18 12:33:42)

Quoted 3 lines

From: Tvrtko Ursulin <[email protected]>

Currently our driver assumes BSD2 means hardware engine instance

number

Quoted 4 lines

two. This does not work for Icelake parts with two VCS engines, but which are hardware instances 0 and 2, and not 0 and 1 as with previous parts.

This makes the second engine not discoverable via HAS_BSD2 get param,

nor

Quoted 15 lines

it can be targetted by execbuf.

While we are working on the next generation execbuf put in a hack which allows discovery and access to this second VCS engine using legacy ABI.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Tony Ye <[email protected]>

---
Compile tested only.

Also, one could argue if this is just a temporary hack or could actually make sense to have this permanently in. It feels like the ABI semantics of BSD2 are blurry, or at least could be re-blurred for Gen11.

Review

Quoted 6 lines

---
 drivers/gpu/drm/i915/i915_drv.c            |  8 +++++++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_drv.c

drivers/gpu/drm/i915/i915_drv.c static int i915_getparam_ioctl(struct drm_device

Quoted 4 lines

index b7dbeba72dec..a185366d9beb 100644

drivers/gpu/drm/i915/i915_drv.c +0 -0

@@ -331,7 +331,13 @@ static int i915_getparam_ioctl(struct drm_device

*dev, void *data,

drivers/gpu/drm/i915/i915_drv.c static int i915_getparam_ioctl(struct drm_device *dev, void *data,

Quoted 8 lines

drivers/gpu/drm/i915/i915_drv.c +4 -1

                value = !!dev_priv->engine[VECS];
                break;
        case I915_PARAM_HAS_BSD2:
-               value = !!dev_priv->engine[VCS2];
+               /*
+                * FIXME: Temporary hack for Icelake.
+                *
+                * Make semantics of HAS_BSD2 "has second", or "has two"

VDBOXes

drivers/gpu/drm/i915/i915_drv.c static int i915_getparam_ioctl(struct drm_device *dev, void *data, · line 340

Quoted 3 lines

drivers/gpu/drm/i915/i915_drv.c +3 -0

+                * instead of "has VDBOX 2nd hardware instance".
+                */
+               value = dev_priv->engine[VCS2] || dev_priv->engine[VCS3];

There can be no temporary hacks for the uAPI... You either sign yourself up to keep it working indefinitely or don't :)

Regards, Joonas

-----Original Message-----
From: Tvrtko Ursulin <[email protected]>
Sent: Wednesday, April 18, 2018 2:34 AM
To: [email protected]
Cc: [email protected]; Ursulin, Tvrtko <[email protected]>; Chris Wilson <[email protected]>; Bloomfield, Jon <[email protected]>; Ye, Tony <[email protected]>
Subject: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

From: Tvrtko Ursulin <[email protected]>

Currently our driver assumes BSD2 means hardware engine instance number two. This does not work for Icelake parts with two VCS engines, but which are hardware instances 0 and 2, and not 0 and 1 as with previous parts.

This makes the second engine not discoverable via HAS_BSD2 get param, nor it can be targetted by execbuf.

While we are working on the next generation execbuf put in a hack which allows discovery and access to this second VCS engine using legacy ABI.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Tony Ye <[email protected]>

Quoted 22 lines

-----Original Message-----
From: Tvrtko Ursulin <[email protected]>
Sent: Wednesday, April 18, 2018 2:34 AM
To: [email protected]
Cc: [email protected]; Ursulin, Tvrtko <[email protected]>; Chris Wilson <[email protected]>; Bloomfield, Jon <[email protected]>; Ye, Tony <[email protected]>
Subject: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

From: Tvrtko Ursulin <[email protected]>

Currently our driver assumes BSD2 means hardware engine instance number two. This does not work for Icelake parts with two VCS engines, but which are hardware instances 0 and 2, and not 0 and 1 as with previous parts.

This makes the second engine not discoverable via HAS_BSD2 get param, nor it can be targetted by execbuf.

While we are working on the next generation execbuf put in a hack which allows discovery and access to this second VCS engine using legacy ABI.

Quoted 4 lines

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Tony Ye <[email protected]>

I would advocate this patch being merged while the new execbuf API is being developed. Currently there is no way to submit to 2 engine skus with non-sequential engine id's. This doesn't introduce a new ABI, and there is no reason that I can see that the new execbuf solution couldn't be made backward compatible with this.

-----Original Message-----
From: Tvrtko Ursulin <[email protected]>
Sent: Friday, April 20, 2018 9:56 AM
To: Bloomfield, Jon <[email protected]>; Tvrtko Ursulin <[email protected]>; [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

Quoted 10 lines · Jon

Quoted 10 lines

-----Original Message-----
From: Tvrtko Ursulin <[email protected]>
Sent: Wednesday, April 18, 2018 2:34 AM
To: [email protected]
Cc: [email protected]; Ursulin, Tvrtko <[email protected]>; Chris Wilson <[email protected]>; Bloomfield, Jon <[email protected]>; Ye, Tony <[email protected]>
Subject: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

Quoted 3 lines

Quoted 3 lines

From: Tvrtko Ursulin <[email protected]>

Currently our driver assumes BSD2 means hardware engine instance

number

Quoted 4 lines

Quoted 4 lines

two. This does not work for Icelake parts with two VCS engines, but which are hardware instances 0 and 2, and not 0 and 1 as with previous parts.

This makes the second engine not discoverable via HAS_BSD2 get param,

nor

Quoted 10 lines

Quoted 9 lines

it can be targetted by execbuf.

While we are working on the next generation execbuf put in a hack which allows discovery and access to this second VCS engine using legacy ABI.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Tony Ye <[email protected]>

I would advocate this patch being merged while the new execbuf API is

being

Quoted 1 line

developed. Currently there is no way to submit to 2 engine skus with non-

sequential

Quoted 1 line

engine id's. This doesn't introduce a new ABI, and there is no reason that I

can see

Quoted 1 line

that the new execbuf solution couldn't be made backward compatible with

this.

It is a bit of a awkward period to commit to this permanently because it only solves a subset of problem space and that makes it a hard sell in that context.

If there was legacy userspace which ran on 2 VCS Gen11 then maybe, but otherwise I think best is just wait for the new execbuf API. Or in fact would there be _any_ upstream userspace using this before the new execbuf API happens?