All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v9 7/7] drm/i915: add a sysfs entry to let users set sseu configs
Date: Tue, 12 Jun 2018 11:52:10 +0100	[thread overview]
Message-ID: <aca72a11-6625-be58-1855-b5a581203497@intel.com> (raw)
In-Reply-To: <152879985216.11830.11337903661997695459@mail.alporthouse.com>

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.

-
Lionel

>> There are a number of processes that can hold onto a gem context and
>> therefore prevent agreement.
>> On my system plymouthd & systemd-login have a number of contexts opened.
> But they should be dontcare?
>
> There should only be a few processes that insist on a particular
> configuration, afui.
> -Chris
>

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

  reply	other threads:[~2018-06-12 10:52 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-30 14:33 [PATCH v9 0/7] drm/i915: per context slice/subslice powergating Lionel Landwerlin
2018-05-30 14:33 ` [PATCH v9 1/7] drm/i915: Program RPCS for Broadwell Lionel Landwerlin
2018-05-30 14:33 ` [PATCH v9 2/7] drm/i915: Record the sseu configuration per-context & engine Lionel Landwerlin
2018-05-30 14:33 ` [PATCH v9 3/7] drm/i915/perf: simplify configure all context function Lionel Landwerlin
2018-05-30 14:33 ` [PATCH v9 4/7] drm/i915/perf: reuse intel_lrc ctx regs macro Lionel Landwerlin
2018-05-30 14:33 ` [PATCH v9 5/7] drm/i915/perf: lock powergating configuration to default when active Lionel Landwerlin
2018-05-30 14:33 ` [PATCH v9 6/7] drm/i915: Expose RPCS (SSEU) configuration to userspace Lionel Landwerlin
2018-05-30 14:33 ` [PATCH v9 7/7] drm/i915: add a sysfs entry to let users set sseu configs Lionel Landwerlin
2018-06-11 12:10   ` Tvrtko Ursulin
2018-06-11 13:46     ` Lionel Landwerlin
2018-06-11 15:02       ` Chris Wilson
2018-06-12  9:20         ` Joonas Lahtinen
2018-06-12 10:33           ` Lionel Landwerlin
2018-06-12 10:37             ` Chris Wilson
2018-06-12 10:52               ` Lionel Landwerlin [this message]
2018-06-12 12:02                 ` Tvrtko Ursulin
2018-06-13 12:49                   ` Joonas Lahtinen
2018-06-14  8:28                     ` Tvrtko Ursulin
2018-07-18 16:44                       ` Bloomfield, Jon
2018-07-19  7:04                         ` Joonas Lahtinen
2018-07-24 21:50                         ` Bloomfield, Jon
2018-07-30 20:40                           ` Rogozhkin, Dmitry V
2018-08-02 10:00                           ` Tvrtko Ursulin
2018-08-02 19:24                             ` Rogozhkin, Dmitry V
2018-06-12 12:02                 ` Chris Wilson
2018-05-30 16:24 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: per context slice/subslice powergating (rev8) Patchwork
2018-05-30 16:26 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-05-30 16:40 ` ✓ Fi.CI.BAT: success " Patchwork
2018-05-30 17:30 ` ✗ Fi.CI.IGT: failure " Patchwork

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=aca72a11-6625-be58-1855-b5a581203497@intel.com \
    --to=lionel.g.landwerlin@intel.com \
    --cc=chris@chris-wilson.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=joonas.lahtinen@linux.intel.com \
    --cc=tvrtko.ursulin@linux.intel.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.