[Picking this point in the thread to reply on some points mentioned by
folks in the whole thread.]
I don't remember if any patches from Lionel's series actually had r-b's,
but a few people including myself have certainly been reviewing them. If
I had applied the final r-b I wouldn't have made much difference due
lack of userspace and disagreement on the DoS mitigation story. So to
say effectively put your money where your mouth is and review is not
entirely fair.
Suggestion to add a master sysfs switch was to alleviate the DoS
concerns because dynamic switching has a cost towards all GPU clients,
not that it just has potential to slow down media.
Suggestion was also that this switch becomes immutable and defaults to
"allow" on Gen11 onwards.
I preferred sysfs versus a modparam since it makes testing (Both for
developers and users (what config works better for my use case?) easier.)
I am fine with the suggestion to drive the Gen11 part first, which
removes the need for any of this.
Patch series is already (AFAIR) nicely split so only the last patch
needs to be dropped.
If at some point we decide to revisit the Gen8/9 story, we can
evaluate/measure whether any master switch is actually warranted. I
think Lionel did some micro-benchmarking which showed impact is not so
severe, so perhaps for real-world use cases it would be even less.
I can re-read the public patches and review them, or perhaps even adopt
them if they have been orphaned. Have to check with Francesco first
before I commit to the latter.
On the uAPI front interface looked fine to me.
One recent interesting development are Mesa patches posted to mesa-dev
(https://lists.freedesktop.org/archives/mesa-dev/2018-July/200607.html)
which add EGL_CONTEXT_load_type extension (low/medium/high). This would
need some sort of mapping between low/medium/high to more specific
settings but still seems okay to me.
This may bring another (open source?) user for the patches. Which Gen's
they are interested in is also a question.
Regards,
Tvrtko
On 24/07/2018 22:50, Bloomfield, Jon wrote:
Gratuitous top posting to re-kick the thread.
For Gen11 we can't have an on/off switch anyway (media simply won't run
with an oncompatible slice config), so let's agree on an api to allow userland
to select the slice configuration for its contexts, for Gen11 only. I'd prefer a
simple {MediaConfig/GeneralConfig} type context param but I really don't
care that much.
For gen9/10 it's arguable whether we need this at all. The effect on media
workloads varies, but I'm guessing normal users (outside a transcode
type environment) will never know they're missing anything. Either way,
we can continue discussing while we progress the gen11 solution.
Jon
-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
Bloomfield, Jon
Sent: Wednesday, July 18, 2018 9:44 AM
To: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>; Joonas Lahtinen
<joonas.lahtinen@xxxxxxxxxxxxxxx>; Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>;
Landwerlin, Lionel G <lionel.g.landwerlin@xxxxxxxxx>; intel-
gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH v9 7/7] drm/i915: add a sysfs entry to let users
set sseu configs
-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
Tvrtko Ursulin
Sent: Thursday, June 14, 2018 1:29 AM
To: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>; Chris Wilson
<chris@xxxxxxxxxxxxxxxxxx>; Landwerlin, Lionel G
<lionel.g.landwerlin@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH v9 7/7] drm/i915: add a sysfs entry to let
users
set sseu configs
On 13/06/2018 13:49, Joonas Lahtinen wrote:
Quoting Tvrtko Ursulin (2018-06-12 15:02:07)
On 12/06/2018 11:52, Lionel Landwerlin wrote:
On 12/06/18 11:37, Chris Wilson wrote:
Quoting Lionel Landwerlin (2018-06-12 11:33:34)
On 12/06/18 10:20, Joonas Lahtinen wrote:
Quoting Chris Wilson (2018-06-11 18:02:37)
Quoting Lionel Landwerlin (2018-06-11 14:46:07)
On 11/06/18 13:10, Tvrtko Ursulin wrote:
On 30/05/2018 15:33, Lionel Landwerlin wrote:
There are concerns about denial of service around the per
context sseu
configuration capability. In a previous commit introducing the
capability we allowed it only for capable users. This changes
adds a
new debugfs entry to let any user configure its own context
powergating setup.
As far as I understood it, Joonas' concerns here are:
1) That in the containers use case individual containers
wouldn't
be
able to turn on the sysfs toggle for them.
2) That also in the containers use case if box admin turns on
the
feature, some containers would potentially start negatively
affecting
the others (via the accumulated cost of slice re-configuration
on
context switching).
I am not familiar with typical container setups to be
authoritative
here, but intuitively I find it reasonable that a low-level
hardware
switch like this would be under the control of a master domain
administrator. ("If you are installing our product in the
container
environment, make sure your system administrator enables
this
hardware
feature.", "Note to system administrators: Enabling this
features
may
negatively affect the performance of other containers.")
Alternative proposal is for the i915 to apply an "or" filter on all
requested masks and in that way ensure dynamic re-
configuration
doesn't happen on context switches, but driven from
userspace
via
ioctls.
In other words, should _all_ userspace agree between
themselves that
they want to turn off a slice, they would then need to send out
a
concerted ioctl storm, where number of needed ioctls equals
the
number
of currently active contexts. (This may have its own
performance
consequences caused by the barriers needed to modify all
context
images.)
This was deemed acceptable the the media use case, but my
concern is
the approach is not elegant and will tie us with the "or" policy
in
the ABI. (Performance concerns I haven't evaluated yet, but
they
also
may be significant.)
If we go back thinking about the containers use case, then it
transpires that even though the "or" policy does prevent one
container
from affecting the other from one angle, it also prevents one
container from exercising the feature unless all containers
co-operate.
As such, we can view the original problem statement where
we
have an
issue if not everyone co-operates, as conceptually the same
just
from
an opposite angle. (Rather than one container incurring the
increased
cost of context switches to the rest, we would have one
container
preventing the optimized slice configuration to the other.)
From this follows that both proposals require complete
co-operation
from all running userspace to avoid complete control of the
feature.
Since the balance between the benefit of optimized slice
configuration
(or penalty of suboptimal one), versus the penalty of increased
context switch times, cannot be know by the driver (barring
venturing
into the heuristics territory), that is another reason why I find
the
"or" policy in the driver questionable.
We can also ask a question of - If we go with the "or" policy,
why
require N per-context ioctls to modify the global GPU
configuration
and not instead add a global driver ioctl to modify the state?
If a future hardware requires, or enables, the per-context
behaviour
in a more efficient way, we could then revisit the problem
space.
In the mean time I see the "or" policy solution as adding some
ABI
which doesn't do anything for many use cases without any way
for the
sysadmin to enable it. At the same time master sysfs knob at
least
enables the sysadmin to make a decision. Here I am thinking
about a
random client environment where not all userspace co-
operates,
but for
instance user is running the feature aware media stack, and
non-feature aware OpenCL/3d stack.
I guess the complete story boils down to - is the master sysfs
knob
really a problem in container use cases.
Regards,
Tvrtko
Hey Tvrtko,
Thanks for summarizing a bunch of discussions.
Essentially I agree with every you wrote above.
If we have a global setting (determined by the OR policy), what's
the
point of per context settings?
In Dmitry's scenario, all userspace applications will work
together to
reach the consensus so it sounds like we're reimplementing the
policy
that is already existing in userspace.
Anyway, I'm implementing Joonas' suggestion. Hopefully
somebody else
than me pick one or the other :)
I'll just mention the voting/consensus approach to see if anyone
else
likes it.
Each context has a CONTEXT_PARAM_HINT_SSEU { small,
dontcare, large }
(or some other abstract names).
Yeah, the param name should have the word _HINT_ in it when it's
not a
definitive set.
There's no global setter across containers, only a scenario when
everyone agrees or not. Tallying up the votes and going with a
majority
vote might be an option, too.
Regards, Joonas
Trying to test the "everyone agrees" approach here.
It's not everyone agrees, but the greater good.
I'm looking forward to the definition of the greater good :)
Tvrtko wanted to avoid the heuristic territory, it seems like we're just
stepping into it.
I am not sure that "small, dontcare, large" models brings much. No one
would probably set "dontcare" since we have to default new contexts
to
large to be compatible.
Don't know, I still prefer the master knob option. Honestly don't yet
see the containers use case as a problem. There is always a master
domain in any system where the knob can be enabled if the customers
on
the system are such to warrant it. On mixed systems enable it or not
someone always suffers. And with the knob we are free to add
heuristics
later, keep the uapi, and just default the knob to on.
Master knob effectively means dead code behind a switch, that's not
very
upstream friendly.
Hey at least I wasn't proposing a modparam! :)))
Yes it is not the best software practice, upstream or not, however I am
trying to be pragmatical here and choose the simplest, smallest, good
enough, and least headache inducing in the future solution.
One way of of looking at the master switch could be like tune your
system for XYZ - change CPU frequency governor, disable SATA link
saving, allow i915 media optimized mode. Some live code out, some dead
code in.
But perhaps discussion is moot since we don't have userspace anyway.
Regards,
Tvrtko
I'm surprised by the "no deadcode behind knobs comment". We do this all
the time - "display=0" anyone? Or "enable_cmdparser=false"?
Allowing user space to reduce EU performance for the system as a whole
is not a great idea imho. Only sysadmin should have the right to let that
happen, and an admin switch (I WOULD go with a modparam personally) is
a good way to ensure that we can get the optimal media configuration for
dedicated media systems, while for general systems we get the best overall
performance (not just favouring media).
Why would we want to over engineer this?
Jon
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
