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

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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 <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Tony Ye <tony.ye@intel.com>
---
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(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b7dbeba72dec..a185366d9beb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -331,7 +331,13 @@ static int i915_getparam_ioctl(struct drm_device *dev, void *data,
 		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
+		 * instead of "has VDBOX 2nd hardware instance".
+		 */
+		value = dev_priv->engine[VCS2] || dev_priv->engine[VCS3];
 		break;
 	case I915_PARAM_HAS_LLC:
 		value = HAS_LLC(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index c74f5df3fb5a..80b32460567d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -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]];
 	}
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

[Joonas Lahtinen]

Quoting Tvrtko Ursulin (2018-04-18 12:33:42)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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 <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Tony Ye <tony.ye@intel.com>
> ---
> 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(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b7dbeba72dec..a185366d9beb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -331,7 +331,13 @@ static int i915_getparam_ioctl(struct drm_device *dev, void *data,
>                 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
> +                * 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

>                 break;
>         case I915_PARAM_HAS_LLC:
>                 value = HAS_LLC(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index c74f5df3fb5a..80b32460567d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -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]];
>         }
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

[Patchwork]

== Series Details ==

Series: drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance
URL   : https://patchwork.freedesktop.org/series/41883/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
7f451ce363f7 drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance
-:12: WARNING:TYPO_SPELLING: 'targetted' may be misspelled - perhaps 'targeted'?
#12: 
it can be targetted by execbuf.

total: 0 errors, 1 warnings, 0 checks, 36 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

[Patchwork]

== Series Details ==

Series: drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance
URL   : https://patchwork.freedesktop.org/series/41883/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4066 -> Patchwork_8733 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/41883/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_8733 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS

    
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (33 -> 31) ==

  Additional (2): fi-glk-j4005 fi-bxt-dsi 
  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-glk-1 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4066 -> Patchwork_8733

  CI_DRM_4066: e1fbca4821d0700551df233285a5c28db09fd0f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4441: 83ba5b7d3bde48b383df41792fc9c955a5a23bdb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8733: 7f451ce363f70a1962ea45efa8fc972afcbc000d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4441: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

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

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8733/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

[Patchwork]

== Series Details ==

Series: drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance
URL   : https://patchwork.freedesktop.org/series/41883/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4066_full -> Patchwork_8733_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8733_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8733_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/41883/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8733_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-dirty-render:
      shard-kbl:          SKIP -> PASS +2

    igt@gem_mocs_settings@mocs-rc6-vebox:
      shard-kbl:          PASS -> SKIP +1

    
== Known issues ==

  Here are the changes found in Patchwork_8733_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_store@cachelines-bsd:
      shard-hsw:          PASS -> FAIL (fdo#100007)

    igt@kms_flip@2x-flip-vs-wf_vblank:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_flip@flip-vs-wf_vblank-interruptible:
      shard-apl:          PASS -> FAIL (fdo#100368)

    igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-blt:
      shard-apl:          PASS -> FAIL (fdo#103167)

    igt@kms_setmode@basic:
      shard-hsw:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
      shard-hsw:          FAIL (fdo#105767) -> PASS

    igt@kms_flip@2x-dpms-vs-vblank-race:
      shard-hsw:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@flip-vs-blocking-wf-vblank:
      shard-hsw:          FAIL (fdo#100368) -> PASS

    igt@kms_frontbuffer_tracking@fbc-farfromfence:
      shard-kbl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +2

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (9 -> 4) ==

  Missing    (5): shard-glk8 shard-glk6 shard-glk7 shard-glk shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4066 -> Patchwork_8733

  CI_DRM_4066: e1fbca4821d0700551df233285a5c28db09fd0f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4441: 83ba5b7d3bde48b383df41792fc9c955a5a23bdb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8733: 7f451ce363f70a1962ea45efa8fc972afcbc000d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4441: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8733/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

[Bloomfield, Jon]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Joonas Lahtinen
> Sent: Wednesday, April 18, 2018 3:43 AM
> To: Intel-gfx@lists.freedesktop.org; Tvrtko Ursulin <tursulin@ursulin.net>
> 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)
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > 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 <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> > Cc: Tony Ye <tony.ye@intel.com>
> > ---
> > 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(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> > index b7dbeba72dec..a185366d9beb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -331,7 +331,13 @@ static int i915_getparam_ioctl(struct drm_device
> *dev, void *data,
> >                 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
> > +                * 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
This doesn't really change the API does it? In fact I'd argue this is simply fixing
a breakage in the API wrt to previous devices. It makes no sense to expose holes
in the engine map to userspace. If a device has two useable VCS engines, HAS_BSD2
should reflect that, and the second engine (wherever it sits physically), should be
addressable through the legacy BSD2 execbuf interface.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

[Bloomfield, Jon]

> -----Original Message-----
> From: Tvrtko Ursulin <tursulin@ursulin.net>
> Sent: Wednesday, April 18, 2018 2:34 AM
> To: Intel-gfx@lists.freedesktop.org
> Cc: tursulin@ursulin.net; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; Chris
> Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon
> <jon.bloomfield@intel.com>; Ye, Tony <tony.ye@intel.com>
> Subject: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second
> VCS instance
> 
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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 <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Tony Ye <tony.ye@intel.com>
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.

Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

[Tvrtko Ursulin]

On 20/04/2018 15:19, Bloomfield, Jon wrote:
>> -----Original Message-----
>> From: Tvrtko Ursulin <tursulin@ursulin.net>
>> Sent: Wednesday, April 18, 2018 2:34 AM
>> To: Intel-gfx@lists.freedesktop.org
>> Cc: tursulin@ursulin.net; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; Chris
>> Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon
>> <jon.bloomfield@intel.com>; Ye, Tony <tony.ye@intel.com>
>> Subject: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second
>> VCS instance
>>
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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 <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>> Cc: Tony Ye <tony.ye@intel.com>
> 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.

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?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

[Bloomfield, Jon]

> -----Original Message-----
> From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Sent: Friday, April 20, 2018 9:56 AM
> To: Bloomfield, Jon <jon.bloomfield@intel.com>; Tvrtko Ursulin
> <tursulin@ursulin.net>; Intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean
> any second VCS instance
> 
> 
> On 20/04/2018 15:19, Bloomfield, Jon wrote:
> >> -----Original Message-----
> >> From: Tvrtko Ursulin <tursulin@ursulin.net>
> >> Sent: Wednesday, April 18, 2018 2:34 AM
> >> To: Intel-gfx@lists.freedesktop.org
> >> Cc: tursulin@ursulin.net; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; Chris
> >> Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon
> >> <jon.bloomfield@intel.com>; Ye, Tony <tony.ye@intel.com>
> >> Subject: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second
> >> VCS instance
> >>
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> 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 <tvrtko.ursulin@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> >> Cc: Tony Ye <tony.ye@intel.com>
> > 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.
> 
> 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?
> 
Fair point. Will you be physically inhibiting this legacy ABI for gen11? If you
intend to allow it it's worth merging, because right now it simply doesn't
work.
If you will never allow the legacy ABI, and will forcibly prevent it (hardcode
HAS_BSD2==0, for gen11+), then I agree we may as well carry the patch as
a delta until the new execbuf lands.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx