| Message ID | 20190920114235.22411-7-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/23] drm/i915/dp: Fix dsc bpp calculations, v2. | expand |
On Fri, Sep 20, 2019 at 01:42:19PM +0200, Maarten Lankhorst wrote: > This can all be done from the intel_update_crtc function. Split out the > pipe update into a separate function, just like is done for the planes. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> The code here all looks logically correct, but I think our various pipe functions are still somewhat confusing. E.g., intel_update_pipe_config() and commit_pipe_config() have names that make it sound like they're doing the same thing. At the moment intel_update_pipe_config() is basically the fastset-specific stuff, but is there any reason most of those operations can't just be done on all commits? Re-writing the pfit/scaler registers or pipe chicken seems like a pretty small number of registers to try to avoid. If we want to keep them separate, I'd suggest renaming intel_update_pipe_config() (and probably the 'update_pipe' flag too) to make it more clear that it's meant for the fastset special case. Matt > --- > drivers/gpu/drm/i915/display/intel_display.c | 124 ++++++++----------- > 1 file changed, 52 insertions(+), 72 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 520c66071e67..fd8b398733b8 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -136,8 +136,6 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, > const struct intel_crtc_state *pipe_config); > static void chv_prepare_pll(struct intel_crtc *crtc, > const struct intel_crtc_state *pipe_config); > -static void intel_begin_crtc_commit(struct intel_atomic_state *, struct intel_crtc *); > -static void intel_finish_crtc_commit(struct intel_atomic_state *, struct intel_crtc *); > static void intel_crtc_init_scalers(struct intel_crtc *crtc, > struct intel_crtc_state *crtc_state); > static void skylake_pfit_enable(const struct intel_crtc_state *crtc_state); > @@ -13673,13 +13671,54 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc) > return crtc->base.funcs->get_vblank_counter(&crtc->base); > } > > +void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + > + if (!IS_GEN(dev_priv, 2)) > + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); > + > + if (crtc_state->has_pch_encoder) { > + enum pipe pch_transcoder = > + intel_crtc_pch_transcoder(crtc); > + > + intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true); > + } > +} > + > +static void commit_pipe_config(struct intel_atomic_state *state, > + struct intel_crtc_state *old_crtc_state, > + struct intel_crtc_state *new_crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + bool modeset = needs_modeset(new_crtc_state); > + > + if (!modeset) { > + if (new_crtc_state->uapi.color_mgmt_changed || > + new_crtc_state->update_pipe) > + intel_color_commit(new_crtc_state); > + > + if (new_crtc_state->update_pipe) > + intel_update_pipe_config(old_crtc_state, new_crtc_state); > + else if (INTEL_GEN(dev_priv) >= 9) > + skl_detach_scalers(new_crtc_state); > + > + if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > + bdw_set_pipemisc(new_crtc_state); > + } > + > + if (dev_priv->display.atomic_update_watermarks) > + dev_priv->display.atomic_update_watermarks(state, > + new_crtc_state); > +} > + > static void intel_update_crtc(struct intel_crtc *crtc, > struct intel_atomic_state *state, > struct intel_crtc_state *old_crtc_state, > struct intel_crtc_state *new_crtc_state) > { > - struct drm_device *dev = state->base.dev; > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > bool modeset = needs_modeset(new_crtc_state); > struct intel_plane_state *new_plane_state = > intel_atomic_get_new_plane_state(state, > @@ -13703,14 +13742,21 @@ static void intel_update_crtc(struct intel_crtc *crtc, > else if (new_plane_state) > intel_fbc_enable(crtc, new_crtc_state, new_plane_state); > > - intel_begin_crtc_commit(state, crtc); > + /* Perform vblank evasion around commit operation */ > + intel_pipe_update_start(new_crtc_state); > + > + commit_pipe_config(state, old_crtc_state, new_crtc_state); > > if (INTEL_GEN(dev_priv) >= 9) > skl_update_planes_on_crtc(state, crtc); > else > i9xx_update_planes_on_crtc(state, crtc); > > - intel_finish_crtc_commit(state, crtc); > + intel_pipe_update_end(new_crtc_state); > + > + if (new_crtc_state->update_pipe && !modeset && > + old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED) > + intel_crtc_arm_fifo_underrun(crtc, new_crtc_state); > } > > static void intel_old_crtc_state_disables(struct intel_atomic_state *state, > @@ -14507,72 +14553,6 @@ skl_max_scale(const struct intel_crtc_state *crtc_state, > return max_scale; > } > > -static void intel_begin_crtc_commit(struct intel_atomic_state *state, > - struct intel_crtc *crtc) > -{ > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - struct intel_crtc_state *old_crtc_state = > - intel_atomic_get_old_crtc_state(state, crtc); > - struct intel_crtc_state *new_crtc_state = > - intel_atomic_get_new_crtc_state(state, crtc); > - bool modeset = needs_modeset(new_crtc_state); > - > - /* Perform vblank evasion around commit operation */ > - intel_pipe_update_start(new_crtc_state); > - > - if (modeset) > - goto out; > - > - if (new_crtc_state->uapi.color_mgmt_changed || > - new_crtc_state->update_pipe) > - intel_color_commit(new_crtc_state); > - > - if (new_crtc_state->update_pipe) > - intel_update_pipe_config(old_crtc_state, new_crtc_state); > - else if (INTEL_GEN(dev_priv) >= 9) > - skl_detach_scalers(new_crtc_state); > - > - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > - bdw_set_pipemisc(new_crtc_state); > - > -out: > - if (dev_priv->display.atomic_update_watermarks) > - dev_priv->display.atomic_update_watermarks(state, > - new_crtc_state); > -} > - > -void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state) > -{ > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - > - if (!IS_GEN(dev_priv, 2)) > - intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); > - > - if (crtc_state->has_pch_encoder) { > - enum pipe pch_transcoder = > - intel_crtc_pch_transcoder(crtc); > - > - intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true); > - } > -} > - > -static void intel_finish_crtc_commit(struct intel_atomic_state *state, > - struct intel_crtc *crtc) > -{ > - struct intel_crtc_state *old_crtc_state = > - intel_atomic_get_old_crtc_state(state, crtc); > - struct intel_crtc_state *new_crtc_state = > - intel_atomic_get_new_crtc_state(state, crtc); > - > - intel_pipe_update_end(new_crtc_state); > - > - if (new_crtc_state->update_pipe && > - !needs_modeset(new_crtc_state) && > - old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED) > - intel_crtc_arm_fifo_underrun(crtc, new_crtc_state); > -} > - > /** > * intel_plane_destroy - destroy a plane > * @plane: plane to destroy > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 25-09-2019 om 06:17 schreef Matt Roper: > On Fri, Sep 20, 2019 at 01:42:19PM +0200, Maarten Lankhorst wrote: >> This can all be done from the intel_update_crtc function. Split out the >> pipe update into a separate function, just like is done for the planes. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > The code here all looks logically correct, but I think our various > pipe functions are still somewhat confusing. E.g., > intel_update_pipe_config() and commit_pipe_config() have names that > make it sound like they're doing the same thing. At the moment > intel_update_pipe_config() is basically the fastset-specific stuff, but > is there any reason most of those operations can't just be done on all > commits? Re-writing the pfit/scaler registers or pipe chicken seems > like a pretty small number of registers to try to avoid. > > If we want to keep them separate, I'd suggest renaming > intel_update_pipe_config() (and probably the 'update_pipe' flag too) to > make it more clear that it's meant for the fastset special case. > I'm sending a new version with intel_update_pipe_config inlined. It makes sense to avoid, especially the chicken bits because that function performs a read as well.
Hi Maarten, Could you hold off on merging this patch and big joiner enabling patch else my 2p2p series which is almost ready to merge except for the final r-b from you on the HW state patch will need to rebased and that will delay the landing of the series. Manasi On Fri, Sep 20, 2019 at 01:42:19PM +0200, Maarten Lankhorst wrote: > This can all be done from the intel_update_crtc function. Split out the > pipe update into a separate function, just like is done for the planes. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 124 ++++++++----------- > 1 file changed, 52 insertions(+), 72 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 520c66071e67..fd8b398733b8 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -136,8 +136,6 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, > const struct intel_crtc_state *pipe_config); > static void chv_prepare_pll(struct intel_crtc *crtc, > const struct intel_crtc_state *pipe_config); > -static void intel_begin_crtc_commit(struct intel_atomic_state *, struct intel_crtc *); > -static void intel_finish_crtc_commit(struct intel_atomic_state *, struct intel_crtc *); > static void intel_crtc_init_scalers(struct intel_crtc *crtc, > struct intel_crtc_state *crtc_state); > static void skylake_pfit_enable(const struct intel_crtc_state *crtc_state); > @@ -13673,13 +13671,54 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc) > return crtc->base.funcs->get_vblank_counter(&crtc->base); > } > > +void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + > + if (!IS_GEN(dev_priv, 2)) > + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); > + > + if (crtc_state->has_pch_encoder) { > + enum pipe pch_transcoder = > + intel_crtc_pch_transcoder(crtc); > + > + intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true); > + } > +} > + > +static void commit_pipe_config(struct intel_atomic_state *state, > + struct intel_crtc_state *old_crtc_state, > + struct intel_crtc_state *new_crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + bool modeset = needs_modeset(new_crtc_state); > + > + if (!modeset) { > + if (new_crtc_state->uapi.color_mgmt_changed || > + new_crtc_state->update_pipe) > + intel_color_commit(new_crtc_state); > + > + if (new_crtc_state->update_pipe) > + intel_update_pipe_config(old_crtc_state, new_crtc_state); > + else if (INTEL_GEN(dev_priv) >= 9) > + skl_detach_scalers(new_crtc_state); > + > + if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > + bdw_set_pipemisc(new_crtc_state); > + } > + > + if (dev_priv->display.atomic_update_watermarks) > + dev_priv->display.atomic_update_watermarks(state, > + new_crtc_state); > +} > + > static void intel_update_crtc(struct intel_crtc *crtc, > struct intel_atomic_state *state, > struct intel_crtc_state *old_crtc_state, > struct intel_crtc_state *new_crtc_state) > { > - struct drm_device *dev = state->base.dev; > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > bool modeset = needs_modeset(new_crtc_state); > struct intel_plane_state *new_plane_state = > intel_atomic_get_new_plane_state(state, > @@ -13703,14 +13742,21 @@ static void intel_update_crtc(struct intel_crtc *crtc, > else if (new_plane_state) > intel_fbc_enable(crtc, new_crtc_state, new_plane_state); > > - intel_begin_crtc_commit(state, crtc); > + /* Perform vblank evasion around commit operation */ > + intel_pipe_update_start(new_crtc_state); > + > + commit_pipe_config(state, old_crtc_state, new_crtc_state); > > if (INTEL_GEN(dev_priv) >= 9) > skl_update_planes_on_crtc(state, crtc); > else > i9xx_update_planes_on_crtc(state, crtc); > > - intel_finish_crtc_commit(state, crtc); > + intel_pipe_update_end(new_crtc_state); > + > + if (new_crtc_state->update_pipe && !modeset && > + old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED) > + intel_crtc_arm_fifo_underrun(crtc, new_crtc_state); > } > > static void intel_old_crtc_state_disables(struct intel_atomic_state *state, > @@ -14507,72 +14553,6 @@ skl_max_scale(const struct intel_crtc_state *crtc_state, > return max_scale; > } > > -static void intel_begin_crtc_commit(struct intel_atomic_state *state, > - struct intel_crtc *crtc) > -{ > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - struct intel_crtc_state *old_crtc_state = > - intel_atomic_get_old_crtc_state(state, crtc); > - struct intel_crtc_state *new_crtc_state = > - intel_atomic_get_new_crtc_state(state, crtc); > - bool modeset = needs_modeset(new_crtc_state); > - > - /* Perform vblank evasion around commit operation */ > - intel_pipe_update_start(new_crtc_state); > - > - if (modeset) > - goto out; > - > - if (new_crtc_state->uapi.color_mgmt_changed || > - new_crtc_state->update_pipe) > - intel_color_commit(new_crtc_state); > - > - if (new_crtc_state->update_pipe) > - intel_update_pipe_config(old_crtc_state, new_crtc_state); > - else if (INTEL_GEN(dev_priv) >= 9) > - skl_detach_scalers(new_crtc_state); > - > - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > - bdw_set_pipemisc(new_crtc_state); > - > -out: > - if (dev_priv->display.atomic_update_watermarks) > - dev_priv->display.atomic_update_watermarks(state, > - new_crtc_state); > -} > - > -void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state) > -{ > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - > - if (!IS_GEN(dev_priv, 2)) > - intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); > - > - if (crtc_state->has_pch_encoder) { > - enum pipe pch_transcoder = > - intel_crtc_pch_transcoder(crtc); > - > - intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true); > - } > -} > - > -static void intel_finish_crtc_commit(struct intel_atomic_state *state, > - struct intel_crtc *crtc) > -{ > - struct intel_crtc_state *old_crtc_state = > - intel_atomic_get_old_crtc_state(state, crtc); > - struct intel_crtc_state *new_crtc_state = > - intel_atomic_get_new_crtc_state(state, crtc); > - > - intel_pipe_update_end(new_crtc_state); > - > - if (new_crtc_state->update_pipe && > - !needs_modeset(new_crtc_state) && > - old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED) > - intel_crtc_arm_fifo_underrun(crtc, new_crtc_state); > -} > - > /** > * intel_plane_destroy - destroy a plane > * @plane: plane to destroy > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 520c66071e67..fd8b398733b8 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -136,8 +136,6 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, const struct intel_crtc_state *pipe_config); static void chv_prepare_pll(struct intel_crtc *crtc, const struct intel_crtc_state *pipe_config); -static void intel_begin_crtc_commit(struct intel_atomic_state *, struct intel_crtc *); -static void intel_finish_crtc_commit(struct intel_atomic_state *, struct intel_crtc *); static void intel_crtc_init_scalers(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); static void skylake_pfit_enable(const struct intel_crtc_state *crtc_state); @@ -13673,13 +13671,54 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc) return crtc->base.funcs->get_vblank_counter(&crtc->base); } +void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, + struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + + if (!IS_GEN(dev_priv, 2)) + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); + + if (crtc_state->has_pch_encoder) { + enum pipe pch_transcoder = + intel_crtc_pch_transcoder(crtc); + + intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true); + } +} + +static void commit_pipe_config(struct intel_atomic_state *state, + struct intel_crtc_state *old_crtc_state, + struct intel_crtc_state *new_crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + bool modeset = needs_modeset(new_crtc_state); + + if (!modeset) { + if (new_crtc_state->uapi.color_mgmt_changed || + new_crtc_state->update_pipe) + intel_color_commit(new_crtc_state); + + if (new_crtc_state->update_pipe) + intel_update_pipe_config(old_crtc_state, new_crtc_state); + else if (INTEL_GEN(dev_priv) >= 9) + skl_detach_scalers(new_crtc_state); + + if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) + bdw_set_pipemisc(new_crtc_state); + } + + if (dev_priv->display.atomic_update_watermarks) + dev_priv->display.atomic_update_watermarks(state, + new_crtc_state); +} + static void intel_update_crtc(struct intel_crtc *crtc, struct intel_atomic_state *state, struct intel_crtc_state *old_crtc_state, struct intel_crtc_state *new_crtc_state) { - struct drm_device *dev = state->base.dev; - struct drm_i915_private *dev_priv = to_i915(dev); + struct drm_i915_private *dev_priv = to_i915(state->base.dev); bool modeset = needs_modeset(new_crtc_state); struct intel_plane_state *new_plane_state = intel_atomic_get_new_plane_state(state, @@ -13703,14 +13742,21 @@ static void intel_update_crtc(struct intel_crtc *crtc, else if (new_plane_state) intel_fbc_enable(crtc, new_crtc_state, new_plane_state); - intel_begin_crtc_commit(state, crtc); + /* Perform vblank evasion around commit operation */ + intel_pipe_update_start(new_crtc_state); + + commit_pipe_config(state, old_crtc_state, new_crtc_state); if (INTEL_GEN(dev_priv) >= 9) skl_update_planes_on_crtc(state, crtc); else i9xx_update_planes_on_crtc(state, crtc); - intel_finish_crtc_commit(state, crtc); + intel_pipe_update_end(new_crtc_state); + + if (new_crtc_state->update_pipe && !modeset && + old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED) + intel_crtc_arm_fifo_underrun(crtc, new_crtc_state); } static void intel_old_crtc_state_disables(struct intel_atomic_state *state, @@ -14507,72 +14553,6 @@ skl_max_scale(const struct intel_crtc_state *crtc_state, return max_scale; } -static void intel_begin_crtc_commit(struct intel_atomic_state *state, - struct intel_crtc *crtc) -{ - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - struct intel_crtc_state *old_crtc_state = - intel_atomic_get_old_crtc_state(state, crtc); - struct intel_crtc_state *new_crtc_state = - intel_atomic_get_new_crtc_state(state, crtc); - bool modeset = needs_modeset(new_crtc_state); - - /* Perform vblank evasion around commit operation */ - intel_pipe_update_start(new_crtc_state); - - if (modeset) - goto out; - - if (new_crtc_state->uapi.color_mgmt_changed || - new_crtc_state->update_pipe) - intel_color_commit(new_crtc_state); - - if (new_crtc_state->update_pipe) - intel_update_pipe_config(old_crtc_state, new_crtc_state); - else if (INTEL_GEN(dev_priv) >= 9) - skl_detach_scalers(new_crtc_state); - - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) - bdw_set_pipemisc(new_crtc_state); - -out: - if (dev_priv->display.atomic_update_watermarks) - dev_priv->display.atomic_update_watermarks(state, - new_crtc_state); -} - -void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state) -{ - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - - if (!IS_GEN(dev_priv, 2)) - intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); - - if (crtc_state->has_pch_encoder) { - enum pipe pch_transcoder = - intel_crtc_pch_transcoder(crtc); - - intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true); - } -} - -static void intel_finish_crtc_commit(struct intel_atomic_state *state, - struct intel_crtc *crtc) -{ - struct intel_crtc_state *old_crtc_state = - intel_atomic_get_old_crtc_state(state, crtc); - struct intel_crtc_state *new_crtc_state = - intel_atomic_get_new_crtc_state(state, crtc); - - intel_pipe_update_end(new_crtc_state); - - if (new_crtc_state->update_pipe && - !needs_modeset(new_crtc_state) && - old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED) - intel_crtc_arm_fifo_underrun(crtc, new_crtc_state); -} - /** * intel_plane_destroy - destroy a plane * @plane: plane to destroy
This can all be done from the intel_update_crtc function. Split out the pipe update into a separate function, just like is done for the planes. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 124 ++++++++----------- 1 file changed, 52 insertions(+), 72 deletions(-)