On Thu, Mar 11, 2021 at 12:57:25PM -0600, Jason Ekstrand wrote: > On Thu, Mar 11, 2021 at 12:20 PM Zbigniew Kempczyński > <zbigniew.kempczynski@xxxxxxxxx> wrote: > > > > On Thu, Mar 11, 2021 at 11:18:11AM -0600, Jason Ekstrand wrote: > > > On Thu, Mar 11, 2021 at 10:51 AM Zbigniew Kempczyński > > > <zbigniew.kempczynski@xxxxxxxxx> wrote: > > > > > > > > On Thu, Mar 11, 2021 at 10:24:38AM -0600, Jason Ekstrand wrote: > > > > > On Thu, Mar 11, 2021 at 9:57 AM Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > > > > > > > > > On Thu, Mar 11, 2021 at 4:50 PM Jason Ekstrand <jason@xxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > > > On Thu, Mar 11, 2021 at 5:44 AM Zbigniew Kempczyński > > > > > > > <zbigniew.kempczynski@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote: > > > > > > > > > The Vulkan driver in Mesa for Intel hardware never uses relocations if > > > > > > > > > it's running on a version of i915 that supports at least softpin which > > > > > > > > > all versions of i915 supporting Gen12 do. On the OpenGL side, Gen12+ is > > > > > > > > > only supported by iris which never uses relocations. The older i965 > > > > > > > > > driver in Mesa does use relocations but it only supports Intel hardware > > > > > > > > > through Gen11 and has been deprecated for all hardware Gen9+. The > > > > > > > > > compute driver also never uses relocations. This only leaves the media > > > > > > > > > driver which is supposed to be switching to softpin going forward. > > > > > > > > > Making softpin a requirement for all future hardware seems reasonable. > > > > > > > > > > > > > > > > > > Rejecting relocations starting with Gen12 has the benefit that we don't > > > > > > > > > have to bother supporting it on platforms with local memory. Given how > > > > > > > > > much CPU touching of memory is required for relocations, not having to > > > > > > > > > do so on platforms where not all memory is directly CPU-accessible > > > > > > > > > carries significant advantages. > > > > > > > > > > > > > > > > > > v2 (Jason Ekstrand): > > > > > > > > > - Allow TGL-LP platforms as they've already shipped > > > > > > > > > > > > > > > > > > v3 (Jason Ekstrand): > > > > > > > > > - WARN_ON platforms with LMEM support in case the check is wrong > > > > > > > > > > > > > > > > I was asked to review of this patch. It works along with expected > > > > > > > > IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25 > > > > > > > > > > > > > > > > Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place > > > > > > > > to do for loop just after copy_from_user() and check relocation_count? > > > > > > > > We have an access to exec2_list there, we know the gen so we're able to say > > > > > > > > relocations are not supported immediate, without entering i915_gem_do_execbuffer(). > > > > > > > > > > > > > > I considered that but it adds an extra object list walk for a case > > > > > > > which we expect to not happen. I'm not sure how expensive the list > > > > > > > walk would be if all we do is check the number of relocations on each > > > > > > > object. I guess, if it comes right after a copy_from_user, it's all > > > > > > > hot in the cache so it shouldn't matter. Ok. I've convinced myself. > > > > > > > I'll move it. > > > > > > > > > > > > I really wouldn't move it if it's another list walk. Execbuf has a lot > > > > > > of fast-paths going on, and we have extensive tests to make sure it > > > > > > unwinds correctly in all cases. It's not very intuitive, but execbuf > > > > > > code isn't scoring very high on that. > > > > > > > > > > And here I'd just finished doing the typing to move it. Good thing I > > > > > hadn't closed vim yet and it was still in my undo buffer. :-) > > > > > > > > Before entering "slower" path from my perspective I would just check > > > > batch object at that place. We still would have single list walkthrough > > > > and quick check on the very beginning. > > > > > > Can you be more specific about what exactly you think we can check at > > > the beginning? Either we add a flag that we can O(1) check at the > > > beginning or we have to check everything in exec2_list for > > > exec2_list[n].relocation_count == 0. That's a list walk. I'm not > > > seeing what up-front check you're thinking we can do without the list > > > walk. > > > > I expect that last (default) or first (I915_EXEC_BATCH_FIRST) execobj > > (batch) will likely has relocations. So we can check that single > > object without entering i915_gem_do_execbuffer(). If that check > > is missed (relocation_count = 0) you'll catch relocations in other > > objects in check_relocations() as you already did. This is simple > > optimization but we can avoid two iterations over buffer list > > (first is in eb_lookup_vmas()). > > Sure, we can do an early-exit check of the first and last objects. > I'm just not seeing what that saves us given that we still have to do > the full list-walk check anyway. Especially since this is an error > path which shouldn't be hit by real userspace drivers. Yeah optimizing error checking sounds like the wrong thing to optimize. Userspace is wrong, it might as well have to wait a bit until it gets that rejection :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
