| Message ID | Zc3iIVsiAwo+bu10@tursulin-desk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL] drm-intel-gt-next | expand |
On Thu, 15 Feb 2024 at 20:06, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > Hi Dave, Daniel, > > First pull request for 6.9 with probably one more coming in one to two > weeks. > > Nothing to interesting in this one, mostly a sprinkle of small fixes in > GuC, HuC, Perf/OA, a tiny bit of prep work for future platforms and some > code cleanups. > > One new uapi in the form of a GuC submission version query which Mesa > wants for implementing Vulkan async compute queues. > > Regards, > > Tvrtko > > drm-intel-gt-next-2024-02-15: > UAPI Changes: > > - Add GuC submission interface version query (Tvrtko Ursulin) > > Driver Changes: > > Fixes/improvements/new stuff: > > - Atomically invalidate userptr on mmu-notifier (Jonathan Cavitt) I've pulled this, but the above patch is triggering my this seems wrong spider sense. This and probably the preceeding patch that this references seem to move i915 to a long term pinning of userptr in memory with what I can see no accounting, and away from what the desired behaviour for drivers should be. It also feels like the authorship on this might be lies which also worries me. Dave.
Hi, Dave On Fri, 2024-02-16 at 12:58 +1000, Dave Airlie wrote: > On Thu, 15 Feb 2024 at 20:06, Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: > > > > Hi Dave, Daniel, > > > > First pull request for 6.9 with probably one more coming in one to > > two > > weeks. > > > > Nothing to interesting in this one, mostly a sprinkle of small > > fixes in > > GuC, HuC, Perf/OA, a tiny bit of prep work for future platforms and > > some > > code cleanups. > > > > One new uapi in the form of a GuC submission version query which > > Mesa > > wants for implementing Vulkan async compute queues. > > > > Regards, > > > > Tvrtko > > > > drm-intel-gt-next-2024-02-15: > > UAPI Changes: > > > > - Add GuC submission interface version query (Tvrtko Ursulin) > > > > Driver Changes: > > > > Fixes/improvements/new stuff: > > > > - Atomically invalidate userptr on mmu-notifier (Jonathan Cavitt) > > I've pulled this, but the above patch is triggering my this seems > wrong spider sense. > > This and probably the preceeding patch that this references seem to > move i915 to a long term pinning of userptr in memory with what I can > see no accounting, and away from what the desired behaviour for > drivers should be. I can only answer for the first patch there, It was some time ago it was written, but at that point the pinning was held both by get_pages() and by submission. I removed the submission pinning and instead moved get_pages() to start of submission. So no significant change in pinning time there. For some reason I can't clearly remember the submission pinning got in the way of the vm_bind implementation. That said, the pinning AFAIR is released in the gem shrinker. And it's different from what other drivers are doing. i915 never got to the point where it completely dropped the pinning after the binding. /Thomas > > It also feels like the authorship on this might be lies which also > worries me. > > Dave.
On Fri, 2024-02-16 at 10:31 +0100, Thomas Hellström wrote: > Hi, Dave > > On Fri, 2024-02-16 at 12:58 +1000, Dave Airlie wrote: > > On Thu, 15 Feb 2024 at 20:06, Tvrtko Ursulin > > <tvrtko.ursulin@linux.intel.com> wrote: > > > > > > Hi Dave, Daniel, > > > > > > First pull request for 6.9 with probably one more coming in one > > > to > > > two > > > weeks. > > > > > > Nothing to interesting in this one, mostly a sprinkle of small > > > fixes in > > > GuC, HuC, Perf/OA, a tiny bit of prep work for future platforms > > > and > > > some > > > code cleanups. > > > > > > One new uapi in the form of a GuC submission version query which > > > Mesa > > > wants for implementing Vulkan async compute queues. > > > > > > Regards, > > > > > > Tvrtko > > > > > > drm-intel-gt-next-2024-02-15: > > > UAPI Changes: > > > > > > - Add GuC submission interface version query (Tvrtko Ursulin) > > > > > > Driver Changes: > > > > > > Fixes/improvements/new stuff: > > > > > > - Atomically invalidate userptr on mmu-notifier (Jonathan Cavitt) > > > > I've pulled this, but the above patch is triggering my this seems > > wrong spider sense. > > > > This and probably the preceeding patch that this references seem to > > move i915 to a long term pinning of userptr in memory with what I > > can > > see no accounting, and away from what the desired behaviour for > > drivers should be. > > I can only answer for the first patch there, It was some time ago it > was written, but at that point the pinning was held both by > get_pages() > and by submission. I removed the submission pinning and instead moved > get_pages() to start of submission. So no significant change in > pinning > time there. For some reason I can't clearly remember the submission > pinning got in the way of the vm_bind implementation. That said, the > pinning AFAIR is released in the gem shrinker. And it's different > from > what other drivers are doing. i915 never got to the point where it > completely dropped the pinning after the binding. (And with the first patch I mean "Simplify userptr locking") /Thomas > > /Thomas > > > > > > It also feels like the authorship on this might be lies which also > > worries me. > > > > Dave. >
(+ Jonathan) Quoting Dave Airlie (2024-02-16 04:58:03) > On Thu, 15 Feb 2024 at 20:06, Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: > > > > Hi Dave, Daniel, > > > > First pull request for 6.9 with probably one more coming in one to two > > weeks. > > > > Nothing to interesting in this one, mostly a sprinkle of small fixes in > > GuC, HuC, Perf/OA, a tiny bit of prep work for future platforms and some > > code cleanups. > > > > One new uapi in the form of a GuC submission version query which Mesa > > wants for implementing Vulkan async compute queues. > > > > Regards, > > > > Tvrtko > > > > drm-intel-gt-next-2024-02-15: > > UAPI Changes: > > > > - Add GuC submission interface version query (Tvrtko Ursulin) > > > > Driver Changes: > > > > Fixes/improvements/new stuff: > > > > - Atomically invalidate userptr on mmu-notifier (Jonathan Cavitt) > > I've pulled this, but the above patch is triggering my this seems > wrong spider sense. > > This and probably the preceeding patch that this references seem to > move i915 to a long term pinning of userptr in memory with what I can > see no accounting, and away from what the desired behaviour for > drivers should be. I asked Thomas to take a more detailed look. Jonathan, Thomas really should have been Cc'd in the original patch as the patch was explicitly referred in the text even. > It also feels like the authorship on this might be lies which also worries me. Fear not. This can probably be blamed on the i915 maintainers. When we have an internal patch which has many revisions and is then essentially rewritten for upstreaming, we specifically asked NOT to keep the "From:" line intact, but instead swap in person who rewrote the patch[1]. To document credits/involvement of the original author we've recommended to keep the Signed-off-by line however. "Co-developed-by" does not really express the situation correctly. "Based on patch by" style pure textual credit reference was also discussed but is hard to grep. Discussed with Sima who suggested if we should consider something like "Original-patch-by:" tag to better express this situation? Regards, Joonas [1] If the "From: " line is not updated, it sometimes leads to situation where you can see a patch with "From:" pointing to you, that doesn't contain a single unmodified line anymore. > > Dave.
Quoting Joonas Lahtinen (2024-02-16 11:41:44) > (+ Jonathan) > > Quoting Dave Airlie (2024-02-16 04:58:03) > > On Thu, 15 Feb 2024 at 20:06, Tvrtko Ursulin > > <tvrtko.ursulin@linux.intel.com> wrote: > > > > > > Hi Dave, Daniel, > > > > > > First pull request for 6.9 with probably one more coming in one to two > > > weeks. > > > > > > Nothing to interesting in this one, mostly a sprinkle of small fixes in > > > GuC, HuC, Perf/OA, a tiny bit of prep work for future platforms and some > > > code cleanups. > > > > > > One new uapi in the form of a GuC submission version query which Mesa > > > wants for implementing Vulkan async compute queues. > > > > > > Regards, > > > > > > Tvrtko > > > > > > drm-intel-gt-next-2024-02-15: > > > UAPI Changes: > > > > > > - Add GuC submission interface version query (Tvrtko Ursulin) > > > > > > Driver Changes: > > > > > > Fixes/improvements/new stuff: > > > > > > - Atomically invalidate userptr on mmu-notifier (Jonathan Cavitt) > > > > I've pulled this, but the above patch is triggering my this seems > > wrong spider sense. > > > > This and probably the preceeding patch that this references seem to > > move i915 to a long term pinning of userptr in memory with what I can > > see no accounting, and away from what the desired behaviour for > > drivers should be. > > I asked Thomas to take a more detailed look. Jonathan, Thomas really should > have been Cc'd in the original patch as the patch was explicitly referred > in the text even. > > > It also feels like the authorship on this might be lies which also worries me. > > Fear not. This can probably be blamed on the i915 maintainers. > > When we have an internal patch which has many revisions and is then > essentially rewritten for upstreaming, we specifically asked NOT to keep > the "From:" line intact, but instead swap in person who rewrote the patch[1]. Just to state the obvious for the public record: This should never be done lightly or without reaching out to the original author. This should only be for the exceptional cases where the patch has significantly changed. This was just the explanation why it's not an immediate red flag to see such a patch. Based on the discussion around the topic we should be more explicit if such a case has happened or if there simply has been an error in the patch handling. So we'll work on clarifying the instructions here. Regards, Joonas > To document credits/involvement of the original author we've recommended > to keep the Signed-off-by line however. "Co-developed-by" does not really > express the situation correctly. "Based on patch by" style pure textual > credit reference was also discussed but is hard to grep. > > Discussed with Sima who suggested if we should consider something like > "Original-patch-by:" tag to better express this situation? > > Regards, Joonas > > [1] If the "From: " line is not updated, it sometimes leads to > situation where you can see a patch with "From:" pointing to you, that > doesn't contain a single unmodified line anymore. > > > > > Dave.
Hi Dave, Daniel, First pull request for 6.9 with probably one more coming in one to two weeks. Nothing to interesting in this one, mostly a sprinkle of small fixes in GuC, HuC, Perf/OA, a tiny bit of prep work for future platforms and some code cleanups. One new uapi in the form of a GuC submission version query which Mesa wants for implementing Vulkan async compute queues. Regards, Tvrtko drm-intel-gt-next-2024-02-15: UAPI Changes: - Add GuC submission interface version query (Tvrtko Ursulin) Driver Changes: Fixes/improvements/new stuff: - Atomically invalidate userptr on mmu-notifier (Jonathan Cavitt) - Update handling of MMIO triggered reports (Umesh Nerlige Ramappa) - Don't make assumptions about intel_wakeref_t type (Jani Nikula) - Add workaround 14019877138 [xelpg] (Tejas Upadhyay) - Allow for very slow HuC loading [huc] (John Harrison) - Flush context destruction worker at suspend [guc] (Alan Previn) - Close deregister-context race against CT-loss [guc] (Alan Previn) - Avoid circular locking issue on busyness flush [guc] (John Harrison) - Use rc6.supported flag from intel_gt for rc6_enable sysfs (Juan Escamilla) - Reflect the true and current status of rc6_enable (Juan Escamilla) - Wake GT before sending H2G message [mtl] (Vinay Belgaumkar) - Restart the heartbeat timer when forcing a pulse (John Harrison) Future platform enablement: - Extend driver code of Xe_LPG to Xe_LPG+ [xelpg] (Harish Chegondi) - Extend some workarounds/tuning to gfx version 12.74 [xelpg] (Matt Roper) Miscellaneous: - Reconcile Excess struct member kernel-doc warnings (Randy Dunlap) - Change wa and EU_PERF_CNTL registers to MCR type [guc] (Shuicheng Lin) - Add flex arrays to struct i915_syncmap (Erick Archer) - Increasing the sleep time for live_rc6_manual [selftests] (Anirban Sk) The following changes since commit 31accc37eaee98a90b25809ed58c6ee4956ab642: drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c (2023-12-15 09:34:31 +0000) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-gt-next-2024-02-15 for you to fetch changes up to eb927f01dfb6309c8a184593c2c0618c4000c481: drm/i915/gt: Restart the heartbeat timer when forcing a pulse (2024-02-14 17:17:35 -0800) ---------------------------------------------------------------- UAPI Changes: - Add GuC submission interface version query (Tvrtko Ursulin) Driver Changes: Fixes/improvements/new stuff: - Atomically invalidate userptr on mmu-notifier (Jonathan Cavitt) - Update handling of MMIO triggered reports (Umesh Nerlige Ramappa) - Don't make assumptions about intel_wakeref_t type (Jani Nikula) - Add workaround 14019877138 [xelpg] (Tejas Upadhyay) - Allow for very slow HuC loading [huc] (John Harrison) - Flush context destruction worker at suspend [guc] (Alan Previn) - Close deregister-context race against CT-loss [guc] (Alan Previn) - Avoid circular locking issue on busyness flush [guc] (John Harrison) - Use rc6.supported flag from intel_gt for rc6_enable sysfs (Juan Escamilla) - Reflect the true and current status of rc6_enable (Juan Escamilla) - Wake GT before sending H2G message [mtl] (Vinay Belgaumkar) - Restart the heartbeat timer when forcing a pulse (John Harrison) Future platform enablement: - Extend driver code of Xe_LPG to Xe_LPG+ [xelpg] (Harish Chegondi) - Extend some workarounds/tuning to gfx version 12.74 [xelpg] (Matt Roper) Miscellaneous: - Reconcile Excess struct member kernel-doc warnings (Randy Dunlap) - Change wa and EU_PERF_CNTL registers to MCR type [guc] (Shuicheng Lin) - Add flex arrays to struct i915_syncmap (Erick Archer) - Increasing the sleep time for live_rc6_manual [selftests] (Anirban Sk) ---------------------------------------------------------------- Alan Previn (2): drm/i915/guc: Flush context destruction worker at suspend drm/i915/guc: Close deregister-context race against CT-loss Anirban Sk (1): drm/i915/selftests: Increasing the sleep time for live_rc6_manual Erick Archer (1): drm/i915: Add flex arrays to struct i915_syncmap Harish Chegondi (1): drm/i915/xelpg: Extend driver code of Xe_LPG to Xe_LPG+ Jani Nikula (1): drm/i915: don't make assumptions about intel_wakeref_t type John Harrison (3): drm/i915/huc: Allow for very slow HuC loading drm/i915/guc: Avoid circular locking issue on busyness flush drm/i915/gt: Restart the heartbeat timer when forcing a pulse Jonathan Cavitt (1): drm/i915/gem: Atomically invalidate userptr on mmu-notifier Juan Escamilla (2): drm/i915/gt: Use rc6.supported flag from intel_gt for rc6_enable sysfs drm/i915/gt: Reflect the true and current status of rc6_enable Matt Roper (1): drm/i915/xelpg: Extend some workarounds/tuning to gfx version 12.74 Randy Dunlap (4): drm/i915/gem: reconcile Excess struct member kernel-doc warnings drm/i915/gt: reconcile Excess struct member kernel-doc warnings drm/i915/guc: reconcile Excess struct member kernel-doc warnings drm/i915/perf: reconcile Excess struct member kernel-doc warnings Shuicheng Lin (1): drm/i915/guc: Change wa and EU_PERF_CNTL registers to MCR type Tejas Upadhyay (1): drm/i915/xelpg: Add workaround 14019877138 Tvrtko Ursulin (1): drm/i915: Add GuC submission interface version query Umesh Nerlige Ramappa (1): drm/i915/perf: Update handling of MMIO triggered reports Vinay Belgaumkar (1): drm/i915/mtl: Wake GT before sending H2G message drivers/gpu/drm/i915/display/intel_display_power.c | 4 +- drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 4 +- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 -- drivers/gpu/drm/i915/gem/i915_gem_pm.c | 10 ++ drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 42 ------- drivers/gpu/drm/i915/gem/i915_gem_userptr.h | 14 --- drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 4 +- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 3 +- drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 3 + drivers/gpu/drm/i915/gt/intel_gsc.h | 7 +- drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 18 +-- drivers/gpu/drm/i915/gt/intel_mocs.c | 2 +- drivers/gpu/drm/i915/gt/intel_rc6.c | 2 +- drivers/gpu/drm/i915/gt/intel_workarounds.c | 27 +++-- drivers/gpu/drm/i915/gt/selftest_rc6.c | 4 +- drivers/gpu/drm/i915/gt/uc/intel_guc.h | 75 ++++++------ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 21 ++-- drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 10 +- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 126 ++++++++++++++++++++- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 2 + drivers/gpu/drm/i915/gt/uc/intel_huc.c | 64 ++++++++++- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 4 +- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 8 -- drivers/gpu/drm/i915/i915_gem.c | 5 - drivers/gpu/drm/i915/i915_perf.c | 41 ++++++- drivers/gpu/drm/i915/i915_perf_types.h | 9 +- drivers/gpu/drm/i915/i915_query.c | 33 ++++++ drivers/gpu/drm/i915/i915_syncmap.c | 19 ++-- drivers/gpu/drm/i915/intel_uncore.c | 5 +- include/uapi/drm/i915_drm.h | 12 ++ 31 files changed, 392 insertions(+), 196 deletions(-) delete mode 100644 drivers/gpu/drm/i915/gem/i915_gem_userptr.h