| Message ID | 20220118214357.33740-2-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Flush G2H handler during a GT reset | expand |
On 1/18/2022 13:43, Matthew Brost wrote: > Allocate intel_engine_coredump_alloc with ALLOW_FAIL rather than > GFP_KERNEL do fully decouple the error capture from fence signalling. s/do/to/ > > Fixes: 8b91cdd4f8649 ("drm/i915: Use __GFP_KSWAPD_RECLAIM in the capture code") Does this really count as a bug fix over that patch? Isn't it more of a changing in policy now that we do DRM fence signalling and that other changes related to error capture behaviour have been implemented. > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > --- > drivers/gpu/drm/i915/i915_gpu_error.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index 67f3515f07e7a..aee42eae4729f 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -1516,7 +1516,7 @@ capture_engine(struct intel_engine_cs *engine, > struct i915_request *rq = NULL; > unsigned long flags; > > - ee = intel_engine_coredump_alloc(engine, GFP_KERNEL); > + ee = intel_engine_coredump_alloc(engine, ALLOW_FAIL); This still makes me nervous that we will fail to allocate engine captures in stress test scenarios, which are exactly the kind of situations where we need valid error captures. There is also still a GFP_KERNEL in __i915_error_grow(). Doesn't that need updating as well? John. > if (!ee) > return NULL; >
On Tue, Jan 18, 2022 at 05:29:54PM -0800, John Harrison wrote: > On 1/18/2022 13:43, Matthew Brost wrote: > > Allocate intel_engine_coredump_alloc with ALLOW_FAIL rather than > > GFP_KERNEL do fully decouple the error capture from fence signalling. > s/do/to/ > Yep. > > > > Fixes: 8b91cdd4f8649 ("drm/i915: Use __GFP_KSWAPD_RECLAIM in the capture code") > Does this really count as a bug fix over that patch? Isn't it more of a > changing in policy now that we do DRM fence signalling and that other > changes related to error capture behaviour have been implemented. > That patch was supposed to allow signalling annotations to be added, without this change I think these annotations would be broken. So I think the Fixes is correct. > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gpu_error.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > > index 67f3515f07e7a..aee42eae4729f 100644 > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > @@ -1516,7 +1516,7 @@ capture_engine(struct intel_engine_cs *engine, > > struct i915_request *rq = NULL; > > unsigned long flags; > > - ee = intel_engine_coredump_alloc(engine, GFP_KERNEL); > > + ee = intel_engine_coredump_alloc(engine, ALLOW_FAIL); > This still makes me nervous that we will fail to allocate engine captures in > stress test scenarios, which are exactly the kind of situations where we > need valid error captures. > Me too, but this whole file has been changed to the ALLOW_FAIL. Thomas and Daniel seem to think this is correct. For what it's worth this allocation is less than a page, so it should be pretty safe to do with ALLOW_FAIL. > There is also still a GFP_KERNEL in __i915_error_grow(). Doesn't that need > updating as well? > Probably just should be deleted. If look it tries with ALLOW_FAIL first, then falls back to GFP_KERNEL. I didn't want to make that update in this series yet but that is something to keep an eye on. Matt > John. > > > if (!ee) > > return NULL; >
On 1/19/2022 12:47, Matthew Brost wrote: > On Tue, Jan 18, 2022 at 05:29:54PM -0800, John Harrison wrote: >> On 1/18/2022 13:43, Matthew Brost wrote: >>> Allocate intel_engine_coredump_alloc with ALLOW_FAIL rather than >>> GFP_KERNEL do fully decouple the error capture from fence signalling. >> s/do/to/ >> > Yep. > >>> Fixes: 8b91cdd4f8649 ("drm/i915: Use __GFP_KSWAPD_RECLAIM in the capture code") >> Does this really count as a bug fix over that patch? Isn't it more of a >> changing in policy now that we do DRM fence signalling and that other >> changes related to error capture behaviour have been implemented. >> > That patch was supposed to allow signalling annotations to be added, > without this change I think these annotations would be broken. So I > think the Fixes is correct. > >>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_gpu_error.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c >>> index 67f3515f07e7a..aee42eae4729f 100644 >>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c >>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c >>> @@ -1516,7 +1516,7 @@ capture_engine(struct intel_engine_cs *engine, >>> struct i915_request *rq = NULL; >>> unsigned long flags; >>> - ee = intel_engine_coredump_alloc(engine, GFP_KERNEL); >>> + ee = intel_engine_coredump_alloc(engine, ALLOW_FAIL); >> This still makes me nervous that we will fail to allocate engine captures in >> stress test scenarios, which are exactly the kind of situations where we >> need valid error captures. >> > Me too, but this whole file has been changed to the ALLOW_FAIL. Thomas > and Daniel seem to think this is correct. For what it's worth this > allocation is less than a page, so it should be pretty safe to do with > ALLOW_FAIL. > >> There is also still a GFP_KERNEL in __i915_error_grow(). Doesn't that need >> updating as well? >> > Probably just should be deleted. If look it tries with ALLOW_FAIL first, > then falls back to GFP_KERNEL. I didn't want to make that update in this > series yet but that is something to keep an eye on. > > Matt > Okay. Makes sense. With the description typo fixed: Reviewed-by: John Harrison <John.C.Harrison@Intel.com> >> John. >> >>> if (!ee) >>> return NULL;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 67f3515f07e7a..aee42eae4729f 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -1516,7 +1516,7 @@ capture_engine(struct intel_engine_cs *engine, struct i915_request *rq = NULL; unsigned long flags; - ee = intel_engine_coredump_alloc(engine, GFP_KERNEL); + ee = intel_engine_coredump_alloc(engine, ALLOW_FAIL); if (!ee) return NULL;
Allocate intel_engine_coredump_alloc with ALLOW_FAIL rather than GFP_KERNEL do fully decouple the error capture from fence signalling. Fixes: 8b91cdd4f8649 ("drm/i915: Use __GFP_KSWAPD_RECLAIM in the capture code") Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- drivers/gpu/drm/i915/i915_gpu_error.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)