| Message ID | 20211129134735.628712-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/i915: Remove short term pins from execbuf. | expand |
Hi, On 29/11/2021 13:47, Maarten Lankhorst wrote: > New version of the series, with feedback from previous series added. If there was a cover letter sent for this work in the past could you please keep attaching it? Or if there wasn't, could you please write one? I am worried about two things. First is that we need to have a high level overview of the rules/design changes documented so third party people have any hope of getting code right after this lands. (Where we are, where we are going, how we will get there, how far did we get and when we will get to the end.) Second is that when parts of the series land piecemeal (Which they have in this right, right?), it gets very hard to write up a maintainer level changelog. But in any case, even on the mundane process level, we need to have cover letters for any non trivial work was the conclusion since some time ago. Regards, Tvrtko > First 11 patches are clean, some small fixes might required still for all to pass. > > Maarten Lankhorst (16): > drm/i915: Remove unused bits of i915_vma/active api > drm/i915: Change shrink ordering to use locking around unbinding. > drm/i915: Remove pages_mutex and intel_gtt->vma_ops.set/clear_pages > members, v2. > drm/i915: Take object lock in i915_ggtt_pin if ww is not set > drm/i915: Force ww lock for i915_gem_object_ggtt_pin_ww > drm/i915: Ensure gem_contexts selftests work with unbind changes. > drm/i915: Take trylock during eviction, v2. > drm/i915: Pass trylock context to callers > drm/i915: Ensure i915_vma tests do not get -ENOSPC with the locking > changes. > drm/i915: Make i915_gem_evict_vm work correctly for already locked > objects > drm/i915: Call i915_gem_evict_vm in vm_fault_gtt to prevent new ENOSPC > errors > drm/i915: Add i915_vma_unbind_unlocked, and take obj lock for > i915_vma_unbind > drm/i915: Require object lock when freeing pages during destruction > drm/i915: Remove assert_object_held_shared > drm/i915: Remove support for unlocked i915_vma unbind > drm/i915: Remove short-term pins from execbuf, v5. > > drivers/gpu/drm/i915/display/intel_dpt.c | 2 - > drivers/gpu/drm/i915/display/intel_fb_pin.c | 2 +- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 250 ++++---- > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 18 +- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 9 +- > drivers/gpu/drm/i915/gem/i915_gem_object.h | 22 +- > drivers/gpu/drm/i915/gem/i915_gem_pages.c | 12 +- > drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 44 +- > drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 2 +- > .../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +- > .../i915/gem/selftests/i915_gem_client_blt.c | 2 +- > .../drm/i915/gem/selftests/i915_gem_context.c | 54 +- > .../drm/i915/gem/selftests/i915_gem_mman.c | 6 + > drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 15 - > drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +- > drivers/gpu/drm/i915/gt/intel_ggtt.c | 450 ++------------ > drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 1 - > drivers/gpu/drm/i915/gt/intel_gtt.c | 13 - > drivers/gpu/drm/i915/gt/intel_gtt.h | 7 - > drivers/gpu/drm/i915/gt/intel_ppgtt.c | 12 - > drivers/gpu/drm/i915/gt/mock_engine.c | 2 +- > drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 2 +- > drivers/gpu/drm/i915/gt/selftest_migrate.c | 2 +- > drivers/gpu/drm/i915/gvt/aperture_gm.c | 2 +- > drivers/gpu/drm/i915/i915_active.c | 28 +- > drivers/gpu/drm/i915/i915_active.h | 17 +- > drivers/gpu/drm/i915/i915_drv.h | 12 +- > drivers/gpu/drm/i915/i915_gem.c | 28 +- > drivers/gpu/drm/i915/i915_gem_evict.c | 64 +- > drivers/gpu/drm/i915/i915_gem_gtt.c | 8 +- > drivers/gpu/drm/i915/i915_gem_gtt.h | 4 + > drivers/gpu/drm/i915/i915_vgpu.c | 2 +- > drivers/gpu/drm/i915/i915_vma.c | 580 +++++++++++++++--- > drivers/gpu/drm/i915/i915_vma.h | 6 +- > drivers/gpu/drm/i915/i915_vma_types.h | 1 - > .../gpu/drm/i915/selftests/i915_gem_evict.c | 27 +- > drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 48 +- > drivers/gpu/drm/i915/selftests/i915_vma.c | 19 +- > drivers/gpu/drm/i915/selftests/mock_gtt.c | 4 - > 40 files changed, 942 insertions(+), 841 deletions(-) >
On 30-11-2021 09:54, Tvrtko Ursulin wrote: > > Hi, > > On 29/11/2021 13:47, Maarten Lankhorst wrote: >> New version of the series, with feedback from previous series added. > > If there was a cover letter sent for this work in the past could you please keep attaching it? Or if there wasn't, could you please write one? > > I am worried about two things. First is that we need to have a high level overview of the rules/design changes documented so third party people have any hope of getting code right after this lands. (Where we are, where we are going, how we will get there, how far did we get and when we will get to the end.) > > Second is that when parts of the series land piecemeal (Which they have in this right, right?), it gets very hard to write up a maintainer level changelog. The preparation part is to ensure we always hold vma->obj->resv when unbinding. The first preparation series ensured vma->obj always existed. This was not the case for mock gtt and gen6 aliasing gtt. This allowed us to remove all the special handling for those uncommon cases, and actually enforce we can always take that lock. This part is merged. Patch 2-11 in this series adds the vma->obj->resv to eviction and shrinker. Those are the only parts where we don't take the lock yet. After that, we always hold the lock when required, and we can start requiring the obj-> resv lock when unbinding. This is completed in patch 15. With that fixed, removing short term pins can be done, because for unbind we now always take obj->resv, so holding obj->resv during execbuf submission is sufficient, and all short term pinning can be removed. We only pin temporarily when calling i915_gem_evict_vm in execbuf, which could also be handled in theory by just marking all objects as unpinned. As a bonus, using TTM for delayed eviction on all objects becomes easy, just need to get rid of i915_active in i915_vma, as it keeps the object refcount alive. Remainder is removing refcount to i915_vma, to make it a real > But in any case, even on the mundane process level, we need to have cover letters for any non trivial work was the conclusion since some time ago. Here you go! I hope it explains the reasoning. ~Maarten
On 30/11/2021 11:17, Maarten Lankhorst wrote: > On 30-11-2021 09:54, Tvrtko Ursulin wrote: >> >> Hi, >> >> On 29/11/2021 13:47, Maarten Lankhorst wrote: >>> New version of the series, with feedback from previous series added. >> >> If there was a cover letter sent for this work in the past could you please keep attaching it? Or if there wasn't, could you please write one? >> >> I am worried about two things. First is that we need to have a high level overview of the rules/design changes documented so third party people have any hope of getting code right after this lands. (Where we are, where we are going, how we will get there, how far did we get and when we will get to the end.) >> >> Second is that when parts of the series land piecemeal (Which they have in this right, right?), it gets very hard to write up a maintainer level changelog. > > The preparation part is to ensure we always hold vma->obj->resv when unbinding. > > The first preparation series ensured vma->obj always existed. This was not the case for mock gtt and gen6 aliasing gtt. This allowed us to remove all the special handling for those uncommon cases, and actually enforce we can always take that lock. This part is merged. Sounds good. But also mention the high level motivation for why we always want to hold vma->obj->resv when unbinding in the introduction as well. > > Patch 2-11 in this series adds the vma->obj->resv to eviction and shrinker. Those are the only parts where we don't take the lock yet. > > After that, we always hold the lock when required, and we can start requiring the obj-> resv lock when unbinding. This is completed in patch 15. > > With that fixed, removing short term pins can be done, because for unbind we now always take obj->resv, so holding obj->resv during execbuf submission is sufficient, and all short term pinning can be removed. I'd also like the cover letter to contain a high level description on _why_ is removing short term pins needed or beneficial. What was the flow and object lifetimes so far, and what it will be going forward etc. > > We only pin temporarily when calling i915_gem_evict_vm in execbuf, which could also be handled in theory by just marking all objects as unpinned. > > As a bonus, using TTM for delayed eviction on all objects becomes easy, just need to get rid of i915_active in i915_vma, as it keeps the object refcount alive. > > Remainder is removing refcount to i915_vma, to make it a real Sounds on the right track with maybe a bit more text so the readers can easily understand it on the higher level. > >> But in any case, even on the mundane process level, we need to have cover letters for any non trivial work was the conclusion since some time ago. > > Here you go! I hope it explains the reasoning. It is on the right track. I think just needs to be expanded a bit with high level direction and plan, pointing out where in the grand scheme this series is. And then don't forget to add the improved text as cover letter when sending next time please. Regards, Tvrtko
On 30-11-2021 19:38, Tvrtko Ursulin wrote: > > On 30/11/2021 11:17, Maarten Lankhorst wrote: >> On 30-11-2021 09:54, Tvrtko Ursulin wrote: >>> >>> Hi, >>> >>> On 29/11/2021 13:47, Maarten Lankhorst wrote: >>>> New version of the series, with feedback from previous series added. >>> >>> If there was a cover letter sent for this work in the past could you please keep attaching it? Or if there wasn't, could you please write one? >>> >>> I am worried about two things. First is that we need to have a high level overview of the rules/design changes documented so third party people have any hope of getting code right after this lands. (Where we are, where we are going, how we will get there, how far did we get and when we will get to the end.) >>> >>> Second is that when parts of the series land piecemeal (Which they have in this right, right?), it gets very hard to write up a maintainer level changelog. >> >> The preparation part is to ensure we always hold vma->obj->resv when unbinding. >> >> The first preparation series ensured vma->obj always existed. This was not the case for mock gtt and gen6 aliasing gtt. This allowed us to remove all the special handling for those uncommon cases, and actually enforce we can always take that lock. This part is merged. > > Sounds good. But also mention the high level motivation for why we always want to hold vma->obj->resv when unbinding in the introduction as well. > >> >> Patch 2-11 in this series adds the vma->obj->resv to eviction and shrinker. Those are the only parts where we don't take the lock yet. >> >> After that, we always hold the lock when required, and we can start requiring the obj-> resv lock when unbinding. This is completed in patch 15. >> >> With that fixed, removing short term pins can be done, because for unbind we now always take obj->resv, so holding obj->resv during execbuf submission is sufficient, and all short term pinning can be removed. > > I'd also like the cover letter to contain a high level description on _why_ is removing short term pins needed or beneficial. > > What was the flow and object lifetimes so far, and what it will be going forward etc. Previously, short term pinning in execbuf was required because i915_vma was effectively independent from objects, and has its own refcount, locking, and lifetime rules and pinning. This series removes the separate locking, by requiring vma->obj->resv to be held when pinning and unbinding. This will also be required for VM_BIND work. With pinning required for pinning and unbinding, the lock is enough to prevent unbinding when trying to pin with the lock held, like in execbuf. This makes binding/unbinding similar to ttm_bo_validate()'s use, which just cares that an object is in a certain place, without pinning it in place. Having it part of gem bo removes a lot of the vma refcounting, and makes i915_vma more a part of the bo, instead of its own floating object that just happens to be part of a bo. This is also required to make it more compatible with TTM, and migration in general. For future work, it makes things a lot simpler and clear. We want to end up with i915_vma just being a specific mapping of the BO, just like is the case in other drivers. i915_vma->active removal is the next step there, and makes it when object is destroyed, the bindings are destroyed (after idle), instead of object being destroyed when bindings are idle. >> >> We only pin temporarily when calling i915_gem_evict_vm in execbuf, which could also be handled in theory by just marking all objects as unpinned. >> >> As a bonus, using TTM for delayed eviction on all objects becomes easy, just need to get rid of i915_active in i915_vma, as it keeps the object refcount alive. >> >> Remainder is removing refcount to i915_vma, to make it a real > > Sounds on the right track with maybe a bit more text so the readers can easily understand it on the higher level. With the obj->resv being the master lock, pinning for execbuf becomes obsolete and can be removed. > >> >>> But in any case, even on the mundane process level, we need to have cover letters for any non trivial work was the conclusion since some time ago. >> >> Here you go! I hope it explains the reasoning. > > It is on the right track. I think just needs to be expanded a bit with high level direction and plan, pointing out where in the grand scheme this series is. And then don't forget to add the improved text as cover letter when sending next time please. > > Regards, > > Tvrtko
On 01/12/2021 11:15, Maarten Lankhorst wrote: > On 30-11-2021 19:38, Tvrtko Ursulin wrote: >> >> On 30/11/2021 11:17, Maarten Lankhorst wrote: >>> On 30-11-2021 09:54, Tvrtko Ursulin wrote: >>>> >>>> Hi, >>>> >>>> On 29/11/2021 13:47, Maarten Lankhorst wrote: >>>>> New version of the series, with feedback from previous series added. >>>> >>>> If there was a cover letter sent for this work in the past could you please keep attaching it? Or if there wasn't, could you please write one? >>>> >>>> I am worried about two things. First is that we need to have a high level overview of the rules/design changes documented so third party people have any hope of getting code right after this lands. (Where we are, where we are going, how we will get there, how far did we get and when we will get to the end.) >>>> >>>> Second is that when parts of the series land piecemeal (Which they have in this right, right?), it gets very hard to write up a maintainer level changelog. >>> >>> The preparation part is to ensure we always hold vma->obj->resv when unbinding. >>> >>> The first preparation series ensured vma->obj always existed. This was not the case for mock gtt and gen6 aliasing gtt. This allowed us to remove all the special handling for those uncommon cases, and actually enforce we can always take that lock. This part is merged. >> >> Sounds good. But also mention the high level motivation for why we always want to hold vma->obj->resv when unbinding in the introduction as well. >> >>> >>> Patch 2-11 in this series adds the vma->obj->resv to eviction and shrinker. Those are the only parts where we don't take the lock yet. >>> >>> After that, we always hold the lock when required, and we can start requiring the obj-> resv lock when unbinding. This is completed in patch 15. >>> >>> With that fixed, removing short term pins can be done, because for unbind we now always take obj->resv, so holding obj->resv during execbuf submission is sufficient, and all short term pinning can be removed. >> >> I'd also like the cover letter to contain a high level description on _why_ is removing short term pins needed or beneficial. >> >> What was the flow and object lifetimes so far, and what it will be going forward etc. > > Previously, short term pinning in execbuf was required because i915_vma was effectively independent from objects, and has its own refcount, locking, and lifetime rules and pinning. > This series removes the separate locking, by requiring vma->obj->resv to be held when pinning and unbinding. This will also be required for VM_BIND work. > With pinning required for pinning and unbinding, the lock is enough to prevent unbinding when trying to pin with the lock held, like in execbuf. > This makes binding/unbinding similar to ttm_bo_validate()'s use, which just cares that an object is in a certain place, without pinning it in place. > > Having it part of gem bo removes a lot of the vma refcounting, and makes i915_vma more a part of the bo, instead of its own floating object that just > happens to be part of a bo. This is also required to make it more compatible with TTM, and migration in general. > > For future work, it makes things a lot simpler and clear. We want to end up with i915_vma just being a specific mapping of the BO, just like is the > case in other drivers. i915_vma->active removal is the next step there, and makes it when object is destroyed, the bindings are destroyed (after idle), > instead of object being destroyed when bindings are idle. Excellent, that's exactly the level needed for the cover letter. So that as intro, plus some text about the series details like which part of the overall plan it implements and it will be good. >>> >>> We only pin temporarily when calling i915_gem_evict_vm in execbuf, which could also be handled in theory by just marking all objects as unpinned. >>> >>> As a bonus, using TTM for delayed eviction on all objects becomes easy, just need to get rid of i915_active in i915_vma, as it keeps the object refcount alive. >>> >>> Remainder is removing refcount to i915_vma, to make it a real >> >> Sounds on the right track with maybe a bit more text so the readers can easily understand it on the higher level. > > With the obj->resv being the master lock, pinning for execbuf becomes obsolete and can be removed. Out of personal curiosity about the long term plans - what will be the flow on request retirement in terms of unbinding? Regards, Tvrtko
New version of the series, with feedback from previous series added. First 11 patches are clean, some small fixes might required still for all to pass. Maarten Lankhorst (16): drm/i915: Remove unused bits of i915_vma/active api drm/i915: Change shrink ordering to use locking around unbinding. drm/i915: Remove pages_mutex and intel_gtt->vma_ops.set/clear_pages members, v2. drm/i915: Take object lock in i915_ggtt_pin if ww is not set drm/i915: Force ww lock for i915_gem_object_ggtt_pin_ww drm/i915: Ensure gem_contexts selftests work with unbind changes. drm/i915: Take trylock during eviction, v2. drm/i915: Pass trylock context to callers drm/i915: Ensure i915_vma tests do not get -ENOSPC with the locking changes. drm/i915: Make i915_gem_evict_vm work correctly for already locked objects drm/i915: Call i915_gem_evict_vm in vm_fault_gtt to prevent new ENOSPC errors drm/i915: Add i915_vma_unbind_unlocked, and take obj lock for i915_vma_unbind drm/i915: Require object lock when freeing pages during destruction drm/i915: Remove assert_object_held_shared drm/i915: Remove support for unlocked i915_vma unbind drm/i915: Remove short-term pins from execbuf, v5. drivers/gpu/drm/i915/display/intel_dpt.c | 2 - drivers/gpu/drm/i915/display/intel_fb_pin.c | 2 +- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 250 ++++---- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 18 +- drivers/gpu/drm/i915/gem/i915_gem_object.c | 9 +- drivers/gpu/drm/i915/gem/i915_gem_object.h | 22 +- drivers/gpu/drm/i915/gem/i915_gem_pages.c | 12 +- drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 44 +- drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 2 +- .../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +- .../i915/gem/selftests/i915_gem_client_blt.c | 2 +- .../drm/i915/gem/selftests/i915_gem_context.c | 54 +- .../drm/i915/gem/selftests/i915_gem_mman.c | 6 + drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 15 - drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +- drivers/gpu/drm/i915/gt/intel_ggtt.c | 450 ++------------ drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 1 - drivers/gpu/drm/i915/gt/intel_gtt.c | 13 - drivers/gpu/drm/i915/gt/intel_gtt.h | 7 - drivers/gpu/drm/i915/gt/intel_ppgtt.c | 12 - drivers/gpu/drm/i915/gt/mock_engine.c | 2 +- drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 2 +- drivers/gpu/drm/i915/gt/selftest_migrate.c | 2 +- drivers/gpu/drm/i915/gvt/aperture_gm.c | 2 +- drivers/gpu/drm/i915/i915_active.c | 28 +- drivers/gpu/drm/i915/i915_active.h | 17 +- drivers/gpu/drm/i915/i915_drv.h | 12 +- drivers/gpu/drm/i915/i915_gem.c | 28 +- drivers/gpu/drm/i915/i915_gem_evict.c | 64 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 8 +- drivers/gpu/drm/i915/i915_gem_gtt.h | 4 + drivers/gpu/drm/i915/i915_vgpu.c | 2 +- drivers/gpu/drm/i915/i915_vma.c | 580 +++++++++++++++--- drivers/gpu/drm/i915/i915_vma.h | 6 +- drivers/gpu/drm/i915/i915_vma_types.h | 1 - .../gpu/drm/i915/selftests/i915_gem_evict.c | 27 +- drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 48 +- drivers/gpu/drm/i915/selftests/i915_vma.c | 19 +- drivers/gpu/drm/i915/selftests/mock_gtt.c | 4 - 40 files changed, 942 insertions(+), 841 deletions(-)