| Message ID | 20180919195544.1511-11-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/40] drm: Use default dma_fence hooks where possible for null syncobj | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > Fix up the error unwind for logical_ring_init() failing by moving the > cleanup into the callers who own the various bits of state during > initialisation. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
On 19/09/2018 20:55, Chris Wilson wrote: > Fix up the error unwind for logical_ring_init() failing by moving the Could you say in the commit what was broken? Regards, Tvrtko > cleanup into the callers who own the various bits of state during > initialisation. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_lrc.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index b4448b05d78a..3edb417caa7b 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -2415,7 +2415,7 @@ static int logical_ring_init(struct intel_engine_cs *engine) > > ret = intel_engine_init_common(engine); > if (ret) > - goto error; > + return ret; > > if (HAS_LOGICAL_RING_ELSQ(i915)) { > execlists->submit_reg = i915->regs + > @@ -2457,10 +2457,6 @@ static int logical_ring_init(struct intel_engine_cs *engine) > reset_csb_pointers(execlists); > > return 0; > - > -error: > - intel_logical_ring_cleanup(engine); > - return ret; > } > > int logical_render_ring_init(struct intel_engine_cs *engine) > @@ -2483,10 +2479,14 @@ int logical_render_ring_init(struct intel_engine_cs *engine) > engine->emit_breadcrumb = gen8_emit_breadcrumb_rcs; > engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_rcs_sz; > > - ret = intel_engine_create_scratch(engine, PAGE_SIZE); > + ret = logical_ring_init(engine); > if (ret) > return ret; > > + ret = intel_engine_create_scratch(engine, PAGE_SIZE); > + if (ret) > + goto err_cleanup_common; > + > ret = intel_init_workaround_bb(engine); > if (ret) { > /* > @@ -2498,7 +2498,11 @@ int logical_render_ring_init(struct intel_engine_cs *engine) > ret); > } > > - return logical_ring_init(engine); > + return 0; > + > +err_cleanup_common: > + intel_engine_cleanup_common(engine); > + return ret; > } > > int logical_xcs_ring_init(struct intel_engine_cs *engine) >
Quoting Tvrtko Ursulin (2018-09-20 15:21:47) > > On 19/09/2018 20:55, Chris Wilson wrote: > > Fix up the error unwind for logical_ring_init() failing by moving the > > Could you say in the commit what was broken? We didn't cleanup all the state we allocated in the caller. -Chris
On 20/09/2018 20:59, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-09-20 15:21:47) >> >> On 19/09/2018 20:55, Chris Wilson wrote: >>> Fix up the error unwind for logical_ring_init() failing by moving the >> >> Could you say in the commit what was broken? > > We didn't cleanup all the state we allocated in the caller. That was kind of obvious, but it would have been helpful to the reviewer at least, if not to the commit message quality itself, to say what exactly. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-09-21 11:00:06) > > On 20/09/2018 20:59, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-09-20 15:21:47) > >> > >> On 19/09/2018 20:55, Chris Wilson wrote: > >>> Fix up the error unwind for logical_ring_init() failing by moving the > >> > >> Could you say in the commit what was broken? > > > > We didn't cleanup all the state we allocated in the caller. > > That was kind of obvious, but it would have been helpful to the reviewer > at least, if not to the commit message quality itself, to say what exactly. I hope the addendum in the commitmsg was helpful. -Chris
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index b4448b05d78a..3edb417caa7b 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2415,7 +2415,7 @@ static int logical_ring_init(struct intel_engine_cs *engine) ret = intel_engine_init_common(engine); if (ret) - goto error; + return ret; if (HAS_LOGICAL_RING_ELSQ(i915)) { execlists->submit_reg = i915->regs + @@ -2457,10 +2457,6 @@ static int logical_ring_init(struct intel_engine_cs *engine) reset_csb_pointers(execlists); return 0; - -error: - intel_logical_ring_cleanup(engine); - return ret; } int logical_render_ring_init(struct intel_engine_cs *engine) @@ -2483,10 +2479,14 @@ int logical_render_ring_init(struct intel_engine_cs *engine) engine->emit_breadcrumb = gen8_emit_breadcrumb_rcs; engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_rcs_sz; - ret = intel_engine_create_scratch(engine, PAGE_SIZE); + ret = logical_ring_init(engine); if (ret) return ret; + ret = intel_engine_create_scratch(engine, PAGE_SIZE); + if (ret) + goto err_cleanup_common; + ret = intel_init_workaround_bb(engine); if (ret) { /* @@ -2498,7 +2498,11 @@ int logical_render_ring_init(struct intel_engine_cs *engine) ret); } - return logical_ring_init(engine); + return 0; + +err_cleanup_common: + intel_engine_cleanup_common(engine); + return ret; } int logical_xcs_ring_init(struct intel_engine_cs *engine)
Fix up the error unwind for logical_ring_init() failing by moving the cleanup into the callers who own the various bits of state during initialisation. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_lrc.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)