| Message ID | 20210208105236.28498-30-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/31] drm/i915/gt: Ratelimit heartbeat completion probing | expand |
On Mon, 8 Feb 2021 at 20:53, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Re-enable secure dispatch for gen6/gen7, primarily to workaround the > command parser and overly zealous command validation on Haswell. For > example this prevents making accurate measurements using a journal for > store results from the GPU without CPU intervention. There's 31 patches in this series, and I can't find any 00/31 or justification for any of this work. I see patches like this which seem to undo work done for security reasons under CVE patches with no oversight. Again, the GT team is not doing the right thing here, stop focusing on individual pieces of Chris's work, push back for high level architectural reviews and I want them on the list in public. All I want from the GT team in the next pull request is dma_resv locking work and restoring the hangcheck timers that seems like a regression that Chris found acceptable and nobody has pushed back on. For like the 500th time, if you want DG1 and stuff in the tree, stop this shit already, real reviewers, high-level architectural reviews, NAK the bullshit in public on the list. Dave.
Quoting Dave Airlie (2021-02-08 20:55:19) > On Mon, 8 Feb 2021 at 20:53, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Re-enable secure dispatch for gen6/gen7, primarily to workaround the > > command parser and overly zealous command validation on Haswell. For > > example this prevents making accurate measurements using a journal for > > store results from the GPU without CPU intervention. > > There's 31 patches in this series, and I can't find any 00/31 or > justification for any of this work. You don't agree with the overview in 11? Or the test design to reproduce the reported problems with multiple clients? There's some code motion to align with upstreaming guc patches later on; a bug fix for an iterative depth-first-search not being a depth-first-search; the change in sort key for scheduling policy; switching the late greedy virtual engine to work on the same interface as execlists/guc; the CS emitters to switch off absolute addressing for breadcrumbs; and finally request reordering for the ringbuffer. > I see patches like this which seem to undo work done for security > reasons under CVE patches with no oversight. Seems to remove clear_residuals? The same clear_residuals between contexts on gen7 is there. > Again, the GT team is not doing the right thing here, stop focusing on > individual pieces of Chris's work, push back for high level > architectural reviews and I want them on the list in public. The architectural bit here is the code motion; getting the backend agnostic list management all into a common layer. Trying to align that with what drm_sched offers, with the optimistic view that one day drm_sched may offer enough to start replacing it. > All I want from the GT team in the next pull request is dma_resv > locking work and restoring the hangcheck timers that seems like a > regression that Chris found acceptable and nobody has pushed back on. The choice here in sort key is still entirely orthogonal to dma-resv. The hangcheck is still driven off a timer. The behaviour of the current code is still the same as the much older global seqno hangcheck around preemption (hangcheck being postponed whenever the seqno changed and/or RING_START changed). The direction to use periodic pulses for both issuing resets (which is actually much faster in detecting hangs than the older seqno hangchecking allowed for), power management and tracking of GPU resource was not mine alone, but yes I did find it acceptable. > For like the 500th time, if you want DG1 and stuff in the tree, stop > this shit already, real reviewers, high-level architectural reviews, > NAK the bullshit in public on the list. I do not understand the hostility to fixing user issues, and improving both existing and future products when it does not interfere with anything else. -Chris
On 08/02/2021 20:55, Dave Airlie wrote: > On Mon, 8 Feb 2021 at 20:53, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> >> Re-enable secure dispatch for gen6/gen7, primarily to workaround the >> command parser and overly zealous command validation on Haswell. For >> example this prevents making accurate measurements using a journal for >> store results from the GPU without CPU intervention. > > There's 31 patches in this series, and I can't find any 00/31 or > justification for any of this work. > > I see patches like this which seem to undo work done for security > reasons under CVE patches with no oversight. > > Again, the GT team is not doing the right thing here, stop focusing on > individual pieces of Chris's work, push back for high level > architectural reviews and I want them on the list in public. > > All I want from the GT team in the next pull request is dma_resv > locking work and restoring the hangcheck timers that seems like a > regression that Chris found acceptable and nobody has pushed back on. > > For like the 500th time, if you want DG1 and stuff in the tree, stop > this shit already, real reviewers, high-level architectural reviews, > NAK the bullshit in public on the list. Since it's mostly been me reviewing the scheduler improvements in this series, I gather we have met and talked, or that you have at least have been following me closely enough to conclude I am not a "real" reviewer. Fair? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index cee180ca7f5a..bb2660e4236a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1630,7 +1630,7 @@ tgl_stepping_get(struct drm_i915_private *dev_priv) #define HAS_LLC(dev_priv) (INTEL_INFO(dev_priv)->has_llc) #define HAS_SNOOP(dev_priv) (INTEL_INFO(dev_priv)->has_snoop) #define HAS_EDRAM(dev_priv) ((dev_priv)->edram_size_mb) -#define HAS_SECURE_BATCHES(dev_priv) (INTEL_GEN(dev_priv) < 6) +#define HAS_SECURE_BATCHES(dev_priv) (INTEL_GEN(dev_priv) < 8) #define HAS_WT(dev_priv) HAS_EDRAM(dev_priv) #define HWS_NEEDS_PHYSICAL(dev_priv) (INTEL_INFO(dev_priv)->hws_needs_physical)
Re-enable secure dispatch for gen6/gen7, primarily to workaround the command parser and overly zealous command validation on Haswell. For example this prevents making accurate measurements using a journal for store results from the GPU without CPU intervention. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)