| Message ID | 20180323101824.14645-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris Wilson <chris@chris-wilson.co.uk> writes: > Commit 0f36a85c3bd5 ("drm/i915: Flush pending interrupt following a GPU > reset") got confused and only applied the flush to the set-wedge path > (which itself is proving troublesome), but we also need the > serialisation on the regular reset path. Oops. > > Move the interrupt into reset_irq() and make it common to the reset and > final set-wedge. > > v2: reset_irq() after port cancellation, as we assert that > execlists->active is sane for cancellation (and is being reset by > reset_irq). > > References: 0f36a85c3bd5 ("drm/i915: Flush pending interrupt following a GPU reset") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Jeff McGee <jeff.mcgee@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 107 +++++++++++++++++++-------------------- > 1 file changed, 53 insertions(+), 54 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index ce09c5ad334f..b4ab06b05e58 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -740,6 +740,57 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists) > } > } > > +static void clear_gtiir(struct intel_engine_cs *engine) > +{ > + static const u8 gtiir[] = { > + [RCS] = 0, > + [BCS] = 0, > + [VCS] = 1, > + [VCS2] = 1, > + [VECS] = 3, > + }; > + struct drm_i915_private *dev_priv = engine->i915; > + int i; > + > + /* TODO: correctly reset irqs for gen11 */ > + if (WARN_ON_ONCE(INTEL_GEN(engine->i915) >= 11)) > + return; > + > + GEM_BUG_ON(engine->id >= ARRAY_SIZE(gtiir)); > + > + /* > + * Clear any pending interrupt state. > + * > + * We do it twice out of paranoia that some of the IIR are > + * double buffered, and so if we only reset it once there may > + * still be an interrupt pending. > + */ > + for (i = 0; i < 2; i++) { > + I915_WRITE(GEN8_GT_IIR(gtiir[engine->id]), > + engine->irq_keep_mask); > + POSTING_READ(GEN8_GT_IIR(gtiir[engine->id])); > + } > + GEM_BUG_ON(I915_READ(GEN8_GT_IIR(gtiir[engine->id])) & > + engine->irq_keep_mask); > +} > + > +static void reset_irq(struct intel_engine_cs *engine) > +{ > + /* Mark all CS interrupts as complete */ > + smp_store_mb(engine->execlists.active, 0); > + synchronize_hardirq(engine->i915->drm.irq); > + > + clear_gtiir(engine); Should clearing the iir be before we synchronize? Our master irq is off, so no bit can light up, clear iir, prevent tasklet with active and synchronize. For documentation it would be nice that we have assert that the tasklet is indeed disabled. -Mika > + > + /* > + * The port is checked prior to scheduling a tasklet, but > + * just in case we have suspended the tasklet to do the > + * wedging make sure that when it wakes, it decides there > + * is no work to do by clearing the irq_posted bit. > + */ > + clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > +} > + > static void execlists_cancel_requests(struct intel_engine_cs *engine) > { > struct intel_engine_execlists * const execlists = &engine->execlists; > @@ -767,6 +818,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine) > > /* Cancel the requests on the HW and clear the ELSP tracker. */ > execlists_cancel_port_requests(execlists); > + reset_irq(engine); > > spin_lock(&engine->timeline->lock); > > @@ -805,18 +857,6 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine) > > spin_unlock(&engine->timeline->lock); > > - /* Mark all CS interrupts as complete */ > - smp_store_mb(execlists->active, 0); > - synchronize_hardirq(engine->i915->drm.irq); > - > - /* > - * The port is checked prior to scheduling a tasklet, but > - * just in case we have suspended the tasklet to do the > - * wedging make sure that when it wakes, it decides there > - * is no work to do by clearing the irq_posted bit. > - */ > - clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > - > local_irq_restore(flags); > } > > @@ -1566,14 +1606,6 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) > return ret; > } > > -static u8 gtiir[] = { > - [RCS] = 0, > - [BCS] = 0, > - [VCS] = 1, > - [VCS2] = 1, > - [VECS] = 3, > -}; > - > static void enable_execlists(struct intel_engine_cs *engine) > { > struct drm_i915_private *dev_priv = engine->i915; > @@ -1657,35 +1689,6 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine) > return init_workarounds_ring(engine); > } > > -static void reset_irq(struct intel_engine_cs *engine) > -{ > - struct drm_i915_private *dev_priv = engine->i915; > - int i; > - > - /* TODO: correctly reset irqs for gen11 */ > - if (WARN_ON_ONCE(INTEL_GEN(engine->i915) >= 11)) > - return; > - > - GEM_BUG_ON(engine->id >= ARRAY_SIZE(gtiir)); > - > - /* > - * Clear any pending interrupt state. > - * > - * We do it twice out of paranoia that some of the IIR are double > - * buffered, and if we only reset it once there may still be > - * an interrupt pending. > - */ > - for (i = 0; i < 2; i++) { > - I915_WRITE(GEN8_GT_IIR(gtiir[engine->id]), > - engine->irq_keep_mask); > - POSTING_READ(GEN8_GT_IIR(gtiir[engine->id])); > - } > - GEM_BUG_ON(I915_READ(GEN8_GT_IIR(gtiir[engine->id])) & > - engine->irq_keep_mask); > - > - clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > -} > - > static void reset_common_ring(struct intel_engine_cs *engine, > struct i915_request *request) > { > @@ -1699,8 +1702,6 @@ static void reset_common_ring(struct intel_engine_cs *engine, > /* See execlists_cancel_requests() for the irq/spinlock split. */ > local_irq_save(flags); > > - reset_irq(engine); > - > /* > * Catch up with any missed context-switch interrupts. > * > @@ -1711,15 +1712,13 @@ static void reset_common_ring(struct intel_engine_cs *engine, > * requests were completed. > */ > execlists_cancel_port_requests(execlists); > + reset_irq(engine); > > /* Push back any incomplete requests for replay after the reset. */ > spin_lock(&engine->timeline->lock); > __unwind_incomplete_requests(engine); > spin_unlock(&engine->timeline->lock); > > - /* Mark all CS interrupts as complete */ > - execlists->active = 0; > - > local_irq_restore(flags); > > /* > -- > 2.16.2
Quoting Mika Kuoppala (2018-03-23 10:53:00) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Commit 0f36a85c3bd5 ("drm/i915: Flush pending interrupt following a GPU > > reset") got confused and only applied the flush to the set-wedge path > > (which itself is proving troublesome), but we also need the > > serialisation on the regular reset path. Oops. > > > > Move the interrupt into reset_irq() and make it common to the reset and > > final set-wedge. > > > > v2: reset_irq() after port cancellation, as we assert that > > execlists->active is sane for cancellation (and is being reset by > > reset_irq). > > > > References: 0f36a85c3bd5 ("drm/i915: Flush pending interrupt following a GPU reset") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Cc: Michel Thierry <michel.thierry@intel.com> > > Cc: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Jeff McGee <jeff.mcgee@intel.com> > > --- > > drivers/gpu/drm/i915/intel_lrc.c | 107 +++++++++++++++++++-------------------- > > 1 file changed, 53 insertions(+), 54 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index ce09c5ad334f..b4ab06b05e58 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -740,6 +740,57 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists) > > } > > } > > > > +static void clear_gtiir(struct intel_engine_cs *engine) > > +{ > > + static const u8 gtiir[] = { > > + [RCS] = 0, > > + [BCS] = 0, > > + [VCS] = 1, > > + [VCS2] = 1, > > + [VECS] = 3, > > + }; > > + struct drm_i915_private *dev_priv = engine->i915; > > + int i; > > + > > + /* TODO: correctly reset irqs for gen11 */ > > + if (WARN_ON_ONCE(INTEL_GEN(engine->i915) >= 11)) > > + return; > > + > > + GEM_BUG_ON(engine->id >= ARRAY_SIZE(gtiir)); > > + > > + /* > > + * Clear any pending interrupt state. > > + * > > + * We do it twice out of paranoia that some of the IIR are > > + * double buffered, and so if we only reset it once there may > > + * still be an interrupt pending. > > + */ > > + for (i = 0; i < 2; i++) { > > + I915_WRITE(GEN8_GT_IIR(gtiir[engine->id]), > > + engine->irq_keep_mask); > > + POSTING_READ(GEN8_GT_IIR(gtiir[engine->id])); > > + } > > + GEM_BUG_ON(I915_READ(GEN8_GT_IIR(gtiir[engine->id])) & > > + engine->irq_keep_mask); > > +} > > + > > +static void reset_irq(struct intel_engine_cs *engine) > > +{ > > + /* Mark all CS interrupts as complete */ > > + smp_store_mb(engine->execlists.active, 0); > > + synchronize_hardirq(engine->i915->drm.irq); > > + > > + clear_gtiir(engine); > > Should clearing the iir be before we synchronize? > Our master irq is off, so no bit can light up, > clear iir, prevent tasklet with active and synchronize. After I think. Before we risk running at the same time as an interrupt handler on another thread. Hence the mb on disabling the execlists (corresponding with the READ_ONE(active) in gen8_cs_irq_handler). Not that is makes much difference (since we don't trust the consistentcy of IIR inside the irq handler, because we do randomly reset it), but I think it makes sense; clear off the interrupt handler, cancel IIR, let the system resume processing interrupts. > For documentation it would be nice that we have > assert that the tasklet is indeed disabled. Not without poking inside tasklet :) In a couple of patches time I plan to have tasklet_disable/tasklet_enable from inside intel_lrc. I'll shovel something in then. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Commit 0f36a85c3bd5 ("drm/i915: Flush pending interrupt following a GPU > reset") got confused and only applied the flush to the set-wedge path > (which itself is proving troublesome), but we also need the > serialisation on the regular reset path. Oops. > > Move the interrupt into reset_irq() and make it common to the reset and > final set-wedge. > > v2: reset_irq() after port cancellation, as we assert that > execlists->active is sane for cancellation (and is being reset by > reset_irq). > > References: 0f36a85c3bd5 ("drm/i915: Flush pending interrupt following a GPU reset") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Jeff McGee <jeff.mcgee@intel.com> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 107 +++++++++++++++++++-------------------- > 1 file changed, 53 insertions(+), 54 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index ce09c5ad334f..b4ab06b05e58 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -740,6 +740,57 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists) > } > } > > +static void clear_gtiir(struct intel_engine_cs *engine) > +{ > + static const u8 gtiir[] = { > + [RCS] = 0, > + [BCS] = 0, > + [VCS] = 1, > + [VCS2] = 1, > + [VECS] = 3, > + }; > + struct drm_i915_private *dev_priv = engine->i915; > + int i; > + > + /* TODO: correctly reset irqs for gen11 */ > + if (WARN_ON_ONCE(INTEL_GEN(engine->i915) >= 11)) > + return; > + > + GEM_BUG_ON(engine->id >= ARRAY_SIZE(gtiir)); > + > + /* > + * Clear any pending interrupt state. > + * > + * We do it twice out of paranoia that some of the IIR are > + * double buffered, and so if we only reset it once there may > + * still be an interrupt pending. > + */ > + for (i = 0; i < 2; i++) { > + I915_WRITE(GEN8_GT_IIR(gtiir[engine->id]), > + engine->irq_keep_mask); > + POSTING_READ(GEN8_GT_IIR(gtiir[engine->id])); > + } > + GEM_BUG_ON(I915_READ(GEN8_GT_IIR(gtiir[engine->id])) & > + engine->irq_keep_mask); > +} > + > +static void reset_irq(struct intel_engine_cs *engine) > +{ > + /* Mark all CS interrupts as complete */ > + smp_store_mb(engine->execlists.active, 0); > + synchronize_hardirq(engine->i915->drm.irq); > + > + clear_gtiir(engine); > + > + /* > + * The port is checked prior to scheduling a tasklet, but > + * just in case we have suspended the tasklet to do the > + * wedging make sure that when it wakes, it decides there > + * is no work to do by clearing the irq_posted bit. > + */ > + clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > +} > + > static void execlists_cancel_requests(struct intel_engine_cs *engine) > { > struct intel_engine_execlists * const execlists = &engine->execlists; > @@ -767,6 +818,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine) > > /* Cancel the requests on the HW and clear the ELSP tracker. */ > execlists_cancel_port_requests(execlists); > + reset_irq(engine); > > spin_lock(&engine->timeline->lock); > > @@ -805,18 +857,6 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine) > > spin_unlock(&engine->timeline->lock); > > - /* Mark all CS interrupts as complete */ > - smp_store_mb(execlists->active, 0); > - synchronize_hardirq(engine->i915->drm.irq); > - > - /* > - * The port is checked prior to scheduling a tasklet, but > - * just in case we have suspended the tasklet to do the > - * wedging make sure that when it wakes, it decides there > - * is no work to do by clearing the irq_posted bit. > - */ > - clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > - > local_irq_restore(flags); > } > > @@ -1566,14 +1606,6 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) > return ret; > } > > -static u8 gtiir[] = { > - [RCS] = 0, > - [BCS] = 0, > - [VCS] = 1, > - [VCS2] = 1, > - [VECS] = 3, > -}; > - > static void enable_execlists(struct intel_engine_cs *engine) > { > struct drm_i915_private *dev_priv = engine->i915; > @@ -1657,35 +1689,6 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine) > return init_workarounds_ring(engine); > } > > -static void reset_irq(struct intel_engine_cs *engine) > -{ > - struct drm_i915_private *dev_priv = engine->i915; > - int i; > - > - /* TODO: correctly reset irqs for gen11 */ > - if (WARN_ON_ONCE(INTEL_GEN(engine->i915) >= 11)) > - return; > - > - GEM_BUG_ON(engine->id >= ARRAY_SIZE(gtiir)); > - > - /* > - * Clear any pending interrupt state. > - * > - * We do it twice out of paranoia that some of the IIR are double > - * buffered, and if we only reset it once there may still be > - * an interrupt pending. > - */ > - for (i = 0; i < 2; i++) { > - I915_WRITE(GEN8_GT_IIR(gtiir[engine->id]), > - engine->irq_keep_mask); > - POSTING_READ(GEN8_GT_IIR(gtiir[engine->id])); > - } > - GEM_BUG_ON(I915_READ(GEN8_GT_IIR(gtiir[engine->id])) & > - engine->irq_keep_mask); > - > - clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > -} > - > static void reset_common_ring(struct intel_engine_cs *engine, > struct i915_request *request) > { > @@ -1699,8 +1702,6 @@ static void reset_common_ring(struct intel_engine_cs *engine, > /* See execlists_cancel_requests() for the irq/spinlock split. */ > local_irq_save(flags); > > - reset_irq(engine); > - > /* > * Catch up with any missed context-switch interrupts. > * > @@ -1711,15 +1712,13 @@ static void reset_common_ring(struct intel_engine_cs *engine, > * requests were completed. > */ > execlists_cancel_port_requests(execlists); > + reset_irq(engine); > > /* Push back any incomplete requests for replay after the reset. */ > spin_lock(&engine->timeline->lock); > __unwind_incomplete_requests(engine); > spin_unlock(&engine->timeline->lock); > > - /* Mark all CS interrupts as complete */ > - execlists->active = 0; > - > local_irq_restore(flags); > > /* > -- > 2.16.2
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index ce09c5ad334f..b4ab06b05e58 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -740,6 +740,57 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists) } } +static void clear_gtiir(struct intel_engine_cs *engine) +{ + static const u8 gtiir[] = { + [RCS] = 0, + [BCS] = 0, + [VCS] = 1, + [VCS2] = 1, + [VECS] = 3, + }; + struct drm_i915_private *dev_priv = engine->i915; + int i; + + /* TODO: correctly reset irqs for gen11 */ + if (WARN_ON_ONCE(INTEL_GEN(engine->i915) >= 11)) + return; + + GEM_BUG_ON(engine->id >= ARRAY_SIZE(gtiir)); + + /* + * Clear any pending interrupt state. + * + * We do it twice out of paranoia that some of the IIR are + * double buffered, and so if we only reset it once there may + * still be an interrupt pending. + */ + for (i = 0; i < 2; i++) { + I915_WRITE(GEN8_GT_IIR(gtiir[engine->id]), + engine->irq_keep_mask); + POSTING_READ(GEN8_GT_IIR(gtiir[engine->id])); + } + GEM_BUG_ON(I915_READ(GEN8_GT_IIR(gtiir[engine->id])) & + engine->irq_keep_mask); +} + +static void reset_irq(struct intel_engine_cs *engine) +{ + /* Mark all CS interrupts as complete */ + smp_store_mb(engine->execlists.active, 0); + synchronize_hardirq(engine->i915->drm.irq); + + clear_gtiir(engine); + + /* + * The port is checked prior to scheduling a tasklet, but + * just in case we have suspended the tasklet to do the + * wedging make sure that when it wakes, it decides there + * is no work to do by clearing the irq_posted bit. + */ + clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); +} + static void execlists_cancel_requests(struct intel_engine_cs *engine) { struct intel_engine_execlists * const execlists = &engine->execlists; @@ -767,6 +818,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine) /* Cancel the requests on the HW and clear the ELSP tracker. */ execlists_cancel_port_requests(execlists); + reset_irq(engine); spin_lock(&engine->timeline->lock); @@ -805,18 +857,6 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine) spin_unlock(&engine->timeline->lock); - /* Mark all CS interrupts as complete */ - smp_store_mb(execlists->active, 0); - synchronize_hardirq(engine->i915->drm.irq); - - /* - * The port is checked prior to scheduling a tasklet, but - * just in case we have suspended the tasklet to do the - * wedging make sure that when it wakes, it decides there - * is no work to do by clearing the irq_posted bit. - */ - clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); - local_irq_restore(flags); } @@ -1566,14 +1606,6 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) return ret; } -static u8 gtiir[] = { - [RCS] = 0, - [BCS] = 0, - [VCS] = 1, - [VCS2] = 1, - [VECS] = 3, -}; - static void enable_execlists(struct intel_engine_cs *engine) { struct drm_i915_private *dev_priv = engine->i915; @@ -1657,35 +1689,6 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine) return init_workarounds_ring(engine); } -static void reset_irq(struct intel_engine_cs *engine) -{ - struct drm_i915_private *dev_priv = engine->i915; - int i; - - /* TODO: correctly reset irqs for gen11 */ - if (WARN_ON_ONCE(INTEL_GEN(engine->i915) >= 11)) - return; - - GEM_BUG_ON(engine->id >= ARRAY_SIZE(gtiir)); - - /* - * Clear any pending interrupt state. - * - * We do it twice out of paranoia that some of the IIR are double - * buffered, and if we only reset it once there may still be - * an interrupt pending. - */ - for (i = 0; i < 2; i++) { - I915_WRITE(GEN8_GT_IIR(gtiir[engine->id]), - engine->irq_keep_mask); - POSTING_READ(GEN8_GT_IIR(gtiir[engine->id])); - } - GEM_BUG_ON(I915_READ(GEN8_GT_IIR(gtiir[engine->id])) & - engine->irq_keep_mask); - - clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); -} - static void reset_common_ring(struct intel_engine_cs *engine, struct i915_request *request) { @@ -1699,8 +1702,6 @@ static void reset_common_ring(struct intel_engine_cs *engine, /* See execlists_cancel_requests() for the irq/spinlock split. */ local_irq_save(flags); - reset_irq(engine); - /* * Catch up with any missed context-switch interrupts. * @@ -1711,15 +1712,13 @@ static void reset_common_ring(struct intel_engine_cs *engine, * requests were completed. */ execlists_cancel_port_requests(execlists); + reset_irq(engine); /* Push back any incomplete requests for replay after the reset. */ spin_lock(&engine->timeline->lock); __unwind_incomplete_requests(engine); spin_unlock(&engine->timeline->lock); - /* Mark all CS interrupts as complete */ - execlists->active = 0; - local_irq_restore(flags); /*
Commit 0f36a85c3bd5 ("drm/i915: Flush pending interrupt following a GPU reset") got confused and only applied the flush to the set-wedge path (which itself is proving troublesome), but we also need the serialisation on the regular reset path. Oops. Move the interrupt into reset_irq() and make it common to the reset and final set-wedge. v2: reset_irq() after port cancellation, as we assert that execlists->active is sane for cancellation (and is being reset by reset_irq). References: 0f36a85c3bd5 ("drm/i915: Flush pending interrupt following a GPU reset") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Michel Thierry <michel.thierry@intel.com> Cc: Michał Winiarski <michal.winiarski@intel.com> Cc: Jeff McGee <jeff.mcgee@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 107 +++++++++++++++++++-------------------- 1 file changed, 53 insertions(+), 54 deletions(-)