| Message ID | 20241021105100.2861830-2-mitulkumar.ajitkumar.golani@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AS_SDP to fastset | expand |
Hello Ville,
As suggested on Revision 1,
Added,
1. State checker and dump for vrr.vsync_{start,end},
2. Removed dependency from vrr.enable
Any further inputs on this ?
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Mitul Golani
> Sent: 21 October 2024 16:21
> To: intel-gfx@lists.freedesktop.org
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> ville.syrjala@linux.intel.com
> Subject: [PATCH v3 1/3] drm/i915/vrr: Update vrr.vsync_{start, end}
> computation
>
> vrr.vsync_{start,end} computation should not depend on crtc_state-
> >vrr.enable. Also add them to state dump.
>
> --v1:
> - Explain commit message more clearly [Jani]
> - Instead of tweaking to fastset use vrr.flipline while computing AS_SDP.
> --v2:
> - Correct computation of vrr.vsync_start/end should not depend on
> vrr.enable.[ville]
> - vrr enable disable requirement should not obstruct by SDP enable
> disable requirements. [Ville]
>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
> .../drm/i915/display/intel_crtc_state_dump.c | 4 +++-
> drivers/gpu/drm/i915/display/intel_vrr.c | 24 +++++++------------
> 2 files changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> index 705ec5ad385c..296a6c9de1f7 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> @@ -303,7 +303,9 @@ void intel_crtc_state_dump(const struct
> intel_crtc_state *pipe_config,
> pipe_config->vrr.flipline,
> intel_vrr_vmin_vblank_start(pipe_config),
> intel_vrr_vmax_vblank_start(pipe_config));
> -
> + drm_printf(&p, "vrr_vsync_start: %d, vrr_vsync_end: %d\n",
> + pipe_config->vrr.vsync_start,
> + pipe_config->vrr.vsync_end);
> drm_printf(&p, "requested mode: " DRM_MODE_FMT "\n",
> DRM_MODE_ARG(&pipe_config->hw.mode));
> drm_printf(&p, "adjusted mode: " DRM_MODE_FMT "\n", diff --git
> a/drivers/gpu/drm/i915/display/intel_vrr.c
> b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 19a5d0076bb8..89696243f320 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -236,7 +236,7 @@ intel_vrr_compute_config(struct intel_crtc_state
> *crtc_state,
> crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> }
>
> - if (intel_dp->as_sdp_supported && crtc_state->vrr.enable) {
> + if (HAS_AS_SDP(display)) {
> crtc_state->vrr.vsync_start =
> (crtc_state->hw.adjusted_mode.crtc_vtotal -
> crtc_state->hw.adjusted_mode.vsync_start);
> @@ -316,6 +316,11 @@ void intel_vrr_set_transcoder_timings(const struct
> intel_crtc_state *crtc_state)
> trans_vrr_ctl(crtc_state));
> intel_de_write(display, TRANS_VRR_FLIPLINE(display,
> cpu_transcoder),
> crtc_state->vrr.flipline - 1);
> + if (HAS_AS_SDP(display))
> + intel_de_write(display,
> + TRANS_VRR_VSYNC(display, cpu_transcoder),
> + VRR_VSYNC_END(crtc_state->vrr.vsync_end) |
> + VRR_VSYNC_START(crtc_state->vrr.vsync_start));
> }
>
> void intel_vrr_send_push(const struct intel_crtc_state *crtc_state) @@ -
> 352,12 +357,6 @@ void intel_vrr_enable(const struct intel_crtc_state
> *crtc_state)
> intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
> TRANS_PUSH_EN);
>
> - if (HAS_AS_SDP(display))
> - intel_de_write(display,
> - TRANS_VRR_VSYNC(display, cpu_transcoder),
> - VRR_VSYNC_END(crtc_state->vrr.vsync_end) |
> - VRR_VSYNC_START(crtc_state->vrr.vsync_start));
> -
> if (crtc_state->cmrr.enable) {
> intel_de_write(display, TRANS_VRR_CTL(display,
> cpu_transcoder),
> VRR_CTL_VRR_ENABLE |
> VRR_CTL_CMRR_ENABLE | @@ -382,10 +381,6 @@ void
> intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
> TRANS_VRR_STATUS(display,
> cpu_transcoder),
> VRR_STATUS_VRR_EN_LIVE, 1000);
> intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), 0);
> -
> - if (HAS_AS_SDP(display))
> - intel_de_write(display,
> - TRANS_VRR_VSYNC(display, cpu_transcoder), 0);
> }
>
> void intel_vrr_get_config(struct intel_crtc_state *crtc_state) @@ -425,10
> +420,6 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>
> TRANS_VRR_VMAX(display, cpu_transcoder)) + 1;
> crtc_state->vrr.vmin = intel_de_read(display,
>
> TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
> - }
> -
> - if (crtc_state->vrr.enable) {
> - crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
>
> if (HAS_AS_SDP(display)) {
> trans_vrr_vsync =
> @@ -440,4 +431,7 @@ void intel_vrr_get_config(struct intel_crtc_state
> *crtc_state)
> REG_FIELD_GET(VRR_VSYNC_END_MASK,
> trans_vrr_vsync);
> }
> }
> +
> + if (crtc_state->vrr.enable)
> + crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> }
> --
> 2.46.0
On 10/21/2024 4:20 PM, Mitul Golani wrote: > vrr.vsync_{start,end} computation should not depend on > crtc_state->vrr.enable. Also add them to state dump. > > --v1: > - Explain commit message more clearly [Jani] > - Instead of tweaking to fastset use vrr.flipline while computing AS_SDP. > --v2: > - Correct computation of vrr.vsync_start/end should not depend on > vrr.enable.[ville] > - vrr enable disable requirement should not obstruct by SDP enable > disable requirements. [Ville] > > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> > --- > .../drm/i915/display/intel_crtc_state_dump.c | 4 +++- > drivers/gpu/drm/i915/display/intel_vrr.c | 24 +++++++------------ > 2 files changed, 12 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c > index 705ec5ad385c..296a6c9de1f7 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c > +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c > @@ -303,7 +303,9 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config, > pipe_config->vrr.flipline, > intel_vrr_vmin_vblank_start(pipe_config), > intel_vrr_vmax_vblank_start(pipe_config)); > - > + drm_printf(&p, "vrr_vsync_start: %d, vrr_vsync_end: %d\n", > + pipe_config->vrr.vsync_start, > + pipe_config->vrr.vsync_end); I think it would be good to have this in same line as other vrr parameters. Also lets have this in a separate patch. > drm_printf(&p, "requested mode: " DRM_MODE_FMT "\n", > DRM_MODE_ARG(&pipe_config->hw.mode)); > drm_printf(&p, "adjusted mode: " DRM_MODE_FMT "\n", > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c > index 19a5d0076bb8..89696243f320 100644 > --- a/drivers/gpu/drm/i915/display/intel_vrr.c > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > @@ -236,7 +236,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, > crtc_state->mode_flags |= I915_MODE_FLAG_VRR; > } > > - if (intel_dp->as_sdp_supported && crtc_state->vrr.enable) { > + if (HAS_AS_SDP(display)) { > crtc_state->vrr.vsync_start = > (crtc_state->hw.adjusted_mode.crtc_vtotal - > crtc_state->hw.adjusted_mode.vsync_start); This again should be separate patch where we are just using HAS_AS_SDP() in compute_config and get_config. > @@ -316,6 +316,11 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state) > trans_vrr_ctl(crtc_state)); > intel_de_write(display, TRANS_VRR_FLIPLINE(display, cpu_transcoder), > crtc_state->vrr.flipline - 1); > + if (HAS_AS_SDP(display)) > + intel_de_write(display, > + TRANS_VRR_VSYNC(display, cpu_transcoder), > + VRR_VSYNC_END(crtc_state->vrr.vsync_end) | > + VRR_VSYNC_START(crtc_state->vrr.vsync_start)); This should be yet another patch where we are setting VRR_VSYNC along with other vrr transcoder timings. Along with removal from existing places in vrr_enable/disable. Apart from splitting into separate patches, changes look good to me. Regards, Ankit > } > > void intel_vrr_send_push(const struct intel_crtc_state *crtc_state) > @@ -352,12 +357,6 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state) > intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), > TRANS_PUSH_EN); > > - if (HAS_AS_SDP(display)) > - intel_de_write(display, > - TRANS_VRR_VSYNC(display, cpu_transcoder), > - VRR_VSYNC_END(crtc_state->vrr.vsync_end) | > - VRR_VSYNC_START(crtc_state->vrr.vsync_start)); > - > if (crtc_state->cmrr.enable) { > intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), > VRR_CTL_VRR_ENABLE | VRR_CTL_CMRR_ENABLE | > @@ -382,10 +381,6 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state) > TRANS_VRR_STATUS(display, cpu_transcoder), > VRR_STATUS_VRR_EN_LIVE, 1000); > intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), 0); > - > - if (HAS_AS_SDP(display)) > - intel_de_write(display, > - TRANS_VRR_VSYNC(display, cpu_transcoder), 0); > } > > void intel_vrr_get_config(struct intel_crtc_state *crtc_state) > @@ -425,10 +420,6 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) > TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; > crtc_state->vrr.vmin = intel_de_read(display, > TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; > - } > - > - if (crtc_state->vrr.enable) { > - crtc_state->mode_flags |= I915_MODE_FLAG_VRR; > > if (HAS_AS_SDP(display)) { > trans_vrr_vsync = > @@ -440,4 +431,7 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) > REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); > } > } > + > + if (crtc_state->vrr.enable) > + crtc_state->mode_flags |= I915_MODE_FLAG_VRR; > }
Thanks Ankit for the review, > -----Original Message----- > From: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com> > Sent: 18 November 2024 13:26 > To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>; > intel-gfx@lists.freedesktop.org > Cc: ville.syrjala@linux.intel.com > Subject: Re: [PATCH v3 1/3] drm/i915/vrr: Update vrr.vsync_{start,end} > computation > > > On 10/21/2024 4:20 PM, Mitul Golani wrote: > > vrr.vsync_{start,end} computation should not depend on > > crtc_state->vrr.enable. Also add them to state dump. > > > > --v1: > > - Explain commit message more clearly [Jani] > > - Instead of tweaking to fastset use vrr.flipline while computing AS_SDP. > > --v2: > > - Correct computation of vrr.vsync_start/end should not depend on > > vrr.enable.[ville] > > - vrr enable disable requirement should not obstruct by SDP enable > > disable requirements. [Ville] > > > > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> > > --- > > .../drm/i915/display/intel_crtc_state_dump.c | 4 +++- > > drivers/gpu/drm/i915/display/intel_vrr.c | 24 +++++++------------ > > 2 files changed, 12 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c > > b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c > > index 705ec5ad385c..296a6c9de1f7 100644 > > --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c > > +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c > > @@ -303,7 +303,9 @@ void intel_crtc_state_dump(const struct > intel_crtc_state *pipe_config, > > pipe_config->vrr.flipline, > > intel_vrr_vmin_vblank_start(pipe_config), > > intel_vrr_vmax_vblank_start(pipe_config)); > > - > > + drm_printf(&p, "vrr_vsync_start: %d, vrr_vsync_end: %d\n", > > + pipe_config->vrr.vsync_start, > > + pipe_config->vrr.vsync_end); > > I think it would be good to have this in same line as other vrr parameters. > > Also lets have this in a separate patch. Agreed, I will add to separate patch in single line. > > > drm_printf(&p, "requested mode: " DRM_MODE_FMT "\n", > > DRM_MODE_ARG(&pipe_config->hw.mode)); > > drm_printf(&p, "adjusted mode: " DRM_MODE_FMT "\n", diff --git > > a/drivers/gpu/drm/i915/display/intel_vrr.c > > b/drivers/gpu/drm/i915/display/intel_vrr.c > > index 19a5d0076bb8..89696243f320 100644 > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > > @@ -236,7 +236,7 @@ intel_vrr_compute_config(struct intel_crtc_state > *crtc_state, > > crtc_state->mode_flags |= I915_MODE_FLAG_VRR; > > } > > > > - if (intel_dp->as_sdp_supported && crtc_state->vrr.enable) { > > + if (HAS_AS_SDP(display)) { > > crtc_state->vrr.vsync_start = > > (crtc_state->hw.adjusted_mode.crtc_vtotal - > > crtc_state->hw.adjusted_mode.vsync_start); > > This again should be separate patch where we are just using HAS_AS_SDP() > in compute_config and get_config. > > > > @@ -316,6 +316,11 @@ void intel_vrr_set_transcoder_timings(const struct > intel_crtc_state *crtc_state) > > trans_vrr_ctl(crtc_state)); > > intel_de_write(display, TRANS_VRR_FLIPLINE(display, > cpu_transcoder), > > crtc_state->vrr.flipline - 1); > > + if (HAS_AS_SDP(display)) > > + intel_de_write(display, > > + TRANS_VRR_VSYNC(display, cpu_transcoder), > > + VRR_VSYNC_END(crtc_state->vrr.vsync_end) | > > + VRR_VSYNC_START(crtc_state->vrr.vsync_start)); > > This should be yet another patch where we are setting VRR_VSYNC along > with other vrr transcoder timings. > > Along with removal from existing places in vrr_enable/disable. > > Apart from splitting into separate patches, changes look good to me. > Splitting this from transcoder timing update, in either way it can create state mismatch when source/sink supports AS SDP but vrr is not enabled from uapi. Is it ok to keep this as a single patch ? > > Regards, > > Ankit > > > > } > > > > void intel_vrr_send_push(const struct intel_crtc_state *crtc_state) > > @@ -352,12 +357,6 @@ void intel_vrr_enable(const struct intel_crtc_state > *crtc_state) > > intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), > > TRANS_PUSH_EN); > > > > - if (HAS_AS_SDP(display)) > > - intel_de_write(display, > > - TRANS_VRR_VSYNC(display, cpu_transcoder), > > - VRR_VSYNC_END(crtc_state->vrr.vsync_end) | > > - VRR_VSYNC_START(crtc_state->vrr.vsync_start)); > > - > > if (crtc_state->cmrr.enable) { > > intel_de_write(display, TRANS_VRR_CTL(display, > cpu_transcoder), > > VRR_CTL_VRR_ENABLE | > VRR_CTL_CMRR_ENABLE | > > @@ -382,10 +381,6 @@ void intel_vrr_disable(const struct intel_crtc_state > *old_crtc_state) > > TRANS_VRR_STATUS(display, > cpu_transcoder), > > VRR_STATUS_VRR_EN_LIVE, 1000); > > intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), 0); > > - > > - if (HAS_AS_SDP(display)) > > - intel_de_write(display, > > - TRANS_VRR_VSYNC(display, cpu_transcoder), 0); > > } > > > > void intel_vrr_get_config(struct intel_crtc_state *crtc_state) > > @@ -425,10 +420,6 @@ void intel_vrr_get_config(struct intel_crtc_state > *crtc_state) > > > TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; > > crtc_state->vrr.vmin = intel_de_read(display, > > TRANS_VRR_VMIN(display, > cpu_transcoder)) + 1; > > - } > > - > > - if (crtc_state->vrr.enable) { > > - crtc_state->mode_flags |= I915_MODE_FLAG_VRR; > > > > if (HAS_AS_SDP(display)) { > > trans_vrr_vsync = > > @@ -440,4 +431,7 @@ void intel_vrr_get_config(struct intel_crtc_state > *crtc_state) > > REG_FIELD_GET(VRR_VSYNC_END_MASK, > trans_vrr_vsync); > > } > > } > > + > > + if (crtc_state->vrr.enable) > > + crtc_state->mode_flags |= I915_MODE_FLAG_VRR; > > }
diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c index 705ec5ad385c..296a6c9de1f7 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c @@ -303,7 +303,9 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config, pipe_config->vrr.flipline, intel_vrr_vmin_vblank_start(pipe_config), intel_vrr_vmax_vblank_start(pipe_config)); - + drm_printf(&p, "vrr_vsync_start: %d, vrr_vsync_end: %d\n", + pipe_config->vrr.vsync_start, + pipe_config->vrr.vsync_end); drm_printf(&p, "requested mode: " DRM_MODE_FMT "\n", DRM_MODE_ARG(&pipe_config->hw.mode)); drm_printf(&p, "adjusted mode: " DRM_MODE_FMT "\n", diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c index 19a5d0076bb8..89696243f320 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.c +++ b/drivers/gpu/drm/i915/display/intel_vrr.c @@ -236,7 +236,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, crtc_state->mode_flags |= I915_MODE_FLAG_VRR; } - if (intel_dp->as_sdp_supported && crtc_state->vrr.enable) { + if (HAS_AS_SDP(display)) { crtc_state->vrr.vsync_start = (crtc_state->hw.adjusted_mode.crtc_vtotal - crtc_state->hw.adjusted_mode.vsync_start); @@ -316,6 +316,11 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state) trans_vrr_ctl(crtc_state)); intel_de_write(display, TRANS_VRR_FLIPLINE(display, cpu_transcoder), crtc_state->vrr.flipline - 1); + if (HAS_AS_SDP(display)) + intel_de_write(display, + TRANS_VRR_VSYNC(display, cpu_transcoder), + VRR_VSYNC_END(crtc_state->vrr.vsync_end) | + VRR_VSYNC_START(crtc_state->vrr.vsync_start)); } void intel_vrr_send_push(const struct intel_crtc_state *crtc_state) @@ -352,12 +357,6 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state) intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), TRANS_PUSH_EN); - if (HAS_AS_SDP(display)) - intel_de_write(display, - TRANS_VRR_VSYNC(display, cpu_transcoder), - VRR_VSYNC_END(crtc_state->vrr.vsync_end) | - VRR_VSYNC_START(crtc_state->vrr.vsync_start)); - if (crtc_state->cmrr.enable) { intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), VRR_CTL_VRR_ENABLE | VRR_CTL_CMRR_ENABLE | @@ -382,10 +381,6 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state) TRANS_VRR_STATUS(display, cpu_transcoder), VRR_STATUS_VRR_EN_LIVE, 1000); intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), 0); - - if (HAS_AS_SDP(display)) - intel_de_write(display, - TRANS_VRR_VSYNC(display, cpu_transcoder), 0); } void intel_vrr_get_config(struct intel_crtc_state *crtc_state) @@ -425,10 +420,6 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; crtc_state->vrr.vmin = intel_de_read(display, TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; - } - - if (crtc_state->vrr.enable) { - crtc_state->mode_flags |= I915_MODE_FLAG_VRR; if (HAS_AS_SDP(display)) { trans_vrr_vsync = @@ -440,4 +431,7 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); } } + + if (crtc_state->vrr.enable) + crtc_state->mode_flags |= I915_MODE_FLAG_VRR; }
vrr.vsync_{start,end} computation should not depend on crtc_state->vrr.enable. Also add them to state dump. --v1: - Explain commit message more clearly [Jani] - Instead of tweaking to fastset use vrr.flipline while computing AS_SDP. --v2: - Correct computation of vrr.vsync_start/end should not depend on vrr.enable.[ville] - vrr enable disable requirement should not obstruct by SDP enable disable requirements. [Ville] Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> --- .../drm/i915/display/intel_crtc_state_dump.c | 4 +++- drivers/gpu/drm/i915/display/intel_vrr.c | 24 +++++++------------ 2 files changed, 12 insertions(+), 16 deletions(-)