Re: [PATCH v9 7/7] drm/i915: add a sysfs entry to let users set sseu configs

["Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>]

On Thu, 2018-08-02 at 11:00 +0100, Tvrtko Ursulin wrote:
> [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.

Thank you for volunteering:).
I talked with Lionel about that and he thinks he may be able to do a
respin next week. So, please, check with him also to avoid double work.

> 
> 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.htm
> l) 
> 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@lists.freedesktop.org> On
> > > Behalf Of
> > > Bloomfield, Jon
> > > Sent: Wednesday, July 18, 2018 9:44 AM
> > > To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Joonas
> > > Lahtinen
> > > <joonas.lahtinen@linux.intel.com>; Chris Wilson <chris@chris-wils
> > > on.co.uk>;
> > > Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>; intel-
> > > gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs
> > > entry to let users
> > > set sseu configs
> > > 
> > > > -----Original Message-----
> > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On
> > > > Behalf Of
> > > > Tvrtko Ursulin
> > > > Sent: Thursday, June 14, 2018 1:29 AM
> > > > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Chris
> > > > Wilson
> > > > <chris@chris-wilson.co.uk>; Landwerlin, Lionel G
> > > > <lionel.g.landwerlin@intel.com>; intel-gfx@lists.freedesktop.or
> > > > g
> > > > Subject: Re: [Intel-gfx] [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx