From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>,
Maling list - DRI developers <dri-devel@lists.freedesktop.org>,
Jason Ekstrand <jason@jlekstrand.net>
Subject: Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
Date: Fri, 30 Apr 2021 13:44:21 +0100 [thread overview]
Message-ID: <e224426b-a76e-fe8f-7d51-1bb4c811711c@linux.intel.com> (raw)
In-Reply-To: <CAKMK7uGHaoMxcaye=ij0-jYE041+xUVneQv6a_J88dgFOh=hiQ@mail.gmail.com>
On 30/04/2021 13:30, Daniel Vetter wrote:
> On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>> On 30/04/2021 07:53, Daniel Vetter wrote:
>>> On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
>>>>
>>>> On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>>
>>>>> On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:
>>>>>> On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>>>> On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:
>>>>>>>> On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>>>>>>> + ret = set_proto_ctx_param(file_priv, pc, args);
>>>>>>>>>
>>>>>>>>> I think we should have a FIXME here of not allowing this on some future
>>>>>>>>> platforms because just use CTX_CREATE_EXT.
>>>>>>>>
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>>> + if (ret == -ENOTSUPP) {
>>>>>>>>>> + /* Some params, specifically SSEU, can only be set on fully
>>>>>>>>>
>>>>>>>>> I think this needs a FIXME: that this only holds during the conversion?
>>>>>>>>> Otherwise we kinda have a bit a problem me thinks ...
>>>>>>>>
>>>>>>>> I'm not sure what you mean by that.
>>>>>>>
>>>>>>> Well I'm at least assuming that we wont have this case anymore, i.e.
>>>>>>> there's only two kinds of parameters:
>>>>>>> - those which are valid only on proto context
>>>>>>> - those which are valid on both (like priority)
>>>>>>>
>>>>>>> This SSEU thing looks like a 3rd parameter, which is only valid on
>>>>>>> finalized context. That feels all kinds of wrong. Will it stay? If yes
>>>>>>> *ugh* and why?
>>>>>>
>>>>>> Because I was being lazy. The SSEU stuff is a fairly complex param to
>>>>>> parse and it's always set live. I can factor out the SSEU parsing
>>>>>> code if you want and it shouldn't be too bad in the end.
>>>>>
>>>>> Yeah I think the special case here is a bit too jarring.
>>>>
>>>> I rolled a v5 that allows you to set SSEU as a create param. I'm not
>>>> a huge fan of that much code duplication for the SSEU set but I guess
>>>> that's what we get for deciding to "unify" our context creation
>>>> parameter path with our on-the-fly parameter path....
>>>>
>>>> You can look at it here:
>>>>
>>>> https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588
>>>
>>> Hm yeah the duplication of the render engine check is a bit annoying.
>>> What's worse, if you tthrow another set_engines on top it's probably
>>> all wrong then. The old thing solved that by just throwing that
>>> intel_context away.
>>>
>>> You're also not keeping the engine id in the proto ctx for this, so
>>> there's probably some gaps there. We'd need to clear the SSEU if
>>> userspace puts another context there. But also no userspace does that.
>>>
>>> Plus cursory review of userspace show
>>> - mesa doesn't set this
>>> - compute sets its right before running the batch
>>> - media sets it as the last thing of context creation
>>
>> Noticed a long sub-thread so looked inside..
>>
>> SSEU is a really an interesting one.
>>
>> For current userspace limiting to context creation is fine, since it is
>> only allowed for Icelake/VME use case. But if you notice the comment inside:
>>
>> /* ABI restriction - VME use case only. */
>>
>> It is a hint there was, or could be, more to this uapi than that.
>>
>> And from memory I think limiting to creation time will nip the hopes
>> media had to use this dynamically on other platforms in the bud. So not
>> that good really. They had convincing numbers what gets significantly
>> better if we allowed dynamic control to this, just that as always, open
>> source userspace was not there so we never allowed it. However if you
>> come up with a new world order where it can only be done at context
>> creation, as said already, the possibility for that improvement (aka
>> further improving the competitive advantage) is most likely dashed.
>
> Hm are you sure that this is create-time only? media-driver uses it
> like that, but from my checking compute-runtime updates SSEU mode
> before every execbuf call. So it very much looked like we have to keep
> this dynamic.
Ah okay, I assumed it's more of the overall drive to eliminate
set_param. If sseu set_param stays then it's fine for what I had in mind.
> Or do you mean this is defacto dead code? this = compute setting it
> before every batch I mean here.
No idea, wasn't aware of the compute usage.
Before every execbuf is not very ideal though since we have to inject a
foreign context operation to update context image, which means stream of
work belonging to the context cannot be coalesced (assuming it could to
start with). There is also a hw cost to reconfigure the sseu which adds
latency on top.
Anyway, I was only aware of the current media usage, which is static as
you say, and future/wishlist media usage, which would be dynamic, but a
complicated story to get right (partly due downsides mentioned in the
previous paragraph mean balancing benefit vs cost of dynamic sseu is not
easy).
Regards,
Tvrtko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-04-30 12:44 UTC|newest]
Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-23 22:31 [PATCH 00/21] drm/i915/gem: ioctl clean-ups Jason Ekstrand
2021-04-23 22:31 ` [PATCH 01/21] drm/i915: Drop I915_CONTEXT_PARAM_RINGSIZE Jason Ekstrand
2021-04-27 9:32 ` Daniel Vetter
2021-04-28 3:33 ` Jason Ekstrand
2021-04-23 22:31 ` [PATCH 02/21] drm/i915: Drop I915_CONTEXT_PARAM_NO_ZEROMAP Jason Ekstrand
2021-04-27 9:38 ` [Intel-gfx] " Daniel Vetter
2021-04-23 22:31 ` [PATCH 03/21] drm/i915/gem: Set the watchdog timeout directly in intel_context_set_gem Jason Ekstrand
2021-04-27 9:42 ` Daniel Vetter
2021-04-28 15:55 ` [Intel-gfx] " Tvrtko Ursulin
2021-04-28 17:24 ` Jason Ekstrand
2021-04-29 8:04 ` Tvrtko Ursulin
2021-04-29 14:54 ` Jason Ekstrand
2021-04-29 17:12 ` Daniel Vetter
2021-04-29 17:13 ` Daniel Vetter
2021-04-29 18:41 ` Jason Ekstrand
2021-04-30 11:18 ` Tvrtko Ursulin
2021-04-30 15:35 ` Jason Ekstrand
2021-04-23 22:31 ` [PATCH 04/21] drm/i915/gem: Return void from context_apply_all Jason Ekstrand
2021-04-27 9:42 ` Daniel Vetter
2021-04-23 22:31 ` [PATCH 05/21] drm/i915: Drop the CONTEXT_CLONE API Jason Ekstrand
2021-04-27 9:49 ` [Intel-gfx] " Daniel Vetter
2021-04-28 17:38 ` Jason Ekstrand
2021-04-28 15:59 ` Tvrtko Ursulin
2021-04-23 22:31 ` [PATCH 06/21] drm/i915: Implement SINGLE_TIMELINE with a syncobj (v3) Jason Ekstrand
2021-04-27 9:55 ` [Intel-gfx] " Daniel Vetter
2021-04-28 15:49 ` Tvrtko Ursulin
2021-04-28 17:26 ` Jason Ekstrand
2021-04-29 8:06 ` Tvrtko Ursulin
2021-04-29 12:08 ` Daniel Vetter
2021-04-29 14:47 ` Jason Ekstrand
2021-04-23 22:31 ` [PATCH 07/21] drm/i915: Drop getparam support for I915_CONTEXT_PARAM_ENGINES Jason Ekstrand
2021-04-27 9:58 ` Daniel Vetter
2021-04-23 22:31 ` [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines Jason Ekstrand
2021-04-26 23:43 ` [PATCH 08/20] drm/i915/gem: Disallow bonding of virtual engines (v2) Jason Ekstrand
2021-04-27 13:58 ` [Intel-gfx] " Daniel Vetter
2021-04-27 13:51 ` [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines Jason Ekstrand
2021-04-28 10:13 ` Daniel Vetter
2021-04-28 17:18 ` Jason Ekstrand
2021-04-28 17:18 ` [Intel-gfx] " Matthew Brost
2021-04-28 17:46 ` Jason Ekstrand
2021-04-28 17:55 ` Matthew Brost
2021-04-28 18:17 ` Jason Ekstrand
2021-04-29 12:14 ` Daniel Vetter
2021-04-30 4:03 ` Matthew Brost
2021-04-30 10:11 ` Daniel Vetter
2021-05-01 17:17 ` Matthew Brost
2021-05-04 7:36 ` Daniel Vetter
2021-04-28 18:58 ` Jason Ekstrand
2021-04-29 12:16 ` Daniel Vetter
2021-04-29 16:02 ` Jason Ekstrand
2021-04-29 17:14 ` Daniel Vetter
2021-04-28 15:51 ` [Intel-gfx] " Tvrtko Ursulin
2021-04-29 12:24 ` Daniel Vetter
2021-04-29 12:54 ` Tvrtko Ursulin
2021-04-29 15:41 ` Jason Ekstrand
2021-04-23 22:31 ` [PATCH 09/21] drm/i915/gem: Disallow creating contexts with too many engines Jason Ekstrand
2021-04-28 10:16 ` [Intel-gfx] " Daniel Vetter
2021-04-28 10:42 ` Tvrtko Ursulin
2021-04-28 14:02 ` Daniel Vetter
2021-04-28 14:26 ` Tvrtko Ursulin
2021-04-28 17:09 ` Jason Ekstrand
2021-04-29 8:01 ` Tvrtko Ursulin
2021-04-29 19:16 ` Jason Ekstrand
2021-04-30 11:40 ` Tvrtko Ursulin
2021-04-30 15:54 ` Jason Ekstrand
2021-04-23 22:31 ` [PATCH 10/21] drm/i915/request: Remove the hook from await_execution Jason Ekstrand
2021-04-26 23:44 ` Jason Ekstrand
2021-04-23 22:31 ` [PATCH 11/21] drm/i915: Stop manually RCU banging in reset_stats_ioctl Jason Ekstrand
2021-04-28 10:27 ` Daniel Vetter
2021-04-28 18:22 ` Jason Ekstrand
2021-04-29 12:22 ` Daniel Vetter
2021-04-23 22:31 ` [PATCH 12/21] drm/i915/gem: Add a separate validate_priority helper Jason Ekstrand
2021-04-28 14:37 ` Daniel Vetter
2021-04-23 22:31 ` [PATCH 13/21] drm/i915/gem: Add an intermediate proto_context struct Jason Ekstrand
2021-04-29 13:02 ` [Intel-gfx] " Daniel Vetter
2021-04-29 16:44 ` Jason Ekstrand
2021-04-23 22:31 ` [PATCH 14/21] drm/i915/gem: Return an error ptr from context_lookup Jason Ekstrand
2021-04-29 13:27 ` [Intel-gfx] " Daniel Vetter
2021-04-29 15:29 ` Jason Ekstrand
2021-04-29 17:16 ` Daniel Vetter
2021-04-23 22:31 ` [PATCH 15/21] drm/i915/gt: Drop i915_address_space::file Jason Ekstrand
2021-04-29 12:37 ` Daniel Vetter
2021-04-29 15:26 ` Jason Ekstrand
2021-04-23 22:31 ` [PATCH 16/21] drm/i915/gem: Delay context creation Jason Ekstrand
2021-04-24 3:21 ` [Intel-gfx] " kernel test robot
2021-04-24 3:24 ` kernel test robot
2021-04-29 15:51 ` Daniel Vetter
2021-04-29 18:16 ` Jason Ekstrand
2021-04-29 18:56 ` Daniel Vetter
2021-04-29 19:01 ` Jason Ekstrand
2021-04-29 19:07 ` Daniel Vetter
2021-04-29 21:35 ` Jason Ekstrand
2021-04-30 6:53 ` Daniel Vetter
2021-04-30 11:58 ` Tvrtko Ursulin
2021-04-30 12:30 ` Daniel Vetter
2021-04-30 12:44 ` Tvrtko Ursulin [this message]
2021-04-30 13:07 ` Daniel Vetter
2021-04-30 13:15 ` Tvrtko Ursulin
2021-04-30 16:27 ` Jason Ekstrand
2021-04-30 16:33 ` Daniel Vetter
2021-04-30 16:57 ` Jason Ekstrand
2021-04-30 17:08 ` Daniel Vetter
2021-04-23 22:31 ` [PATCH 17/21] drm/i915/gem: Don't allow changing the VM on running contexts Jason Ekstrand
2021-04-23 22:31 ` [PATCH 18/21] drm/i915/gem: Don't allow changing the engine set " Jason Ekstrand
2021-04-29 17:21 ` Daniel Vetter
2021-04-23 22:31 ` [PATCH 19/21] drm/i915/selftests: Take a VM in kernel_context() Jason Ekstrand
2021-04-23 22:31 ` [PATCH 20/21] i915/gem/selftests: Assign the VM at context creation in igt_shared_ctx_exec Jason Ekstrand
2021-04-29 17:19 ` [Intel-gfx] " Daniel Vetter
2021-04-23 22:31 ` [PATCH 21/21] drm/i915/gem: Roll all of context creation together Jason Ekstrand
2021-04-29 17:25 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e224426b-a76e-fe8f-7d51-1bb4c811711c@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jason@jlekstrand.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).