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@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@chris-wils > > > on.co.uk>; > > > 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@xxxxxxxxxxxxxxxxxxxx > > > > g > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
