Message ID | 1448357676-27837-10-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote: > This allows iteration over encoders without requiring connection_mutex. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > drivers/gpu/drm/i915/intel_display.c | 3 +++ > include/drm/drm_crtc.h | 2 ++ > 3 files changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index fb79c54b2aed..f3fd8f131f92 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state) > continue; > > drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode); > + > + crtc_state->encoder_mask = 0; Hm, I think a small function to set the best_encoder (like we do to set the crtc for connector or planes) would be good. Otherwise we'll frob around the code and forget this, and much confusion will ensue. That helper should probably be in core drm_atomic.c, like the other set_foo_for_bar helpers. -Daniel > } > > for_each_connector_in_state(state, connector, conn_state, i) { > @@ -288,6 +290,8 @@ mode_fixup(struct drm_atomic_state *state) > * it away), so we won't call ->mode_fixup twice. > */ > encoder = conn_state->best_encoder; > + crtc_state->encoder_mask |= 1 << drm_encoder_index(encoder); > + > funcs = encoder->helper_private; > if (!funcs) > continue; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 21b1984e828b..3317db3806a8 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15310,6 +15310,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > crtc->base.state->connector_mask = 0; > + crtc->base.state->encoder_mask = 0; > > /* Because we only establish the connector -> encoder -> > * crtc links if something is active, this means the > @@ -15363,6 +15364,8 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) > > crtc->state->connector_mask |= > 1 << drm_connector_index(&connector->base); > + crtc->state->encoder_mask |= > + 1 << drm_encoder_index(&encoder->base); > } > > if (active && !has_active_crtc) { > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 4999fe530f37..7ea83b6a2864 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -265,6 +265,7 @@ struct drm_atomic_state; > * @connectors_changed: connectors to this crtc have been updated > * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes > * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors > + * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders > * @last_vblank_count: for helpers and drivers to capture the vblank of the > * update to ensure framebuffer cleanup isn't done too early > * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings > @@ -299,6 +300,7 @@ struct drm_crtc_state { > u32 plane_mask; > > u32 connector_mask; > + u32 encoder_mask; > > /* last_vblank_count: for vblank waits before cleanup */ > u32 last_vblank_count; > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 03-12-15 om 10:53 schreef Daniel Vetter: > On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote: >> This allows iteration over encoders without requiring connection_mutex. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ >> drivers/gpu/drm/i915/intel_display.c | 3 +++ >> include/drm/drm_crtc.h | 2 ++ >> 3 files changed, 9 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index fb79c54b2aed..f3fd8f131f92 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state) >> continue; >> >> drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode); >> + >> + crtc_state->encoder_mask = 0; > Hm, I think a small function to set the best_encoder (like we do to set > the crtc for connector or planes) would be good. Otherwise we'll frob > around the code and forget this, and much confusion will ensue. > > That helper should probably be in core drm_atomic.c, like the other > set_foo_for_bar helpers. As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :( At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much.. ~Maarten
On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote: > Op 03-12-15 om 10:53 schreef Daniel Vetter: > > On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote: > >> This allows iteration over encoders without requiring connection_mutex. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > >> drivers/gpu/drm/i915/intel_display.c | 3 +++ > >> include/drm/drm_crtc.h | 2 ++ > >> 3 files changed, 9 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >> index fb79c54b2aed..f3fd8f131f92 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state) > >> continue; > >> > >> drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode); > >> + > >> + crtc_state->encoder_mask = 0; > > Hm, I think a small function to set the best_encoder (like we do to set > > the crtc for connector or planes) would be good. Otherwise we'll frob > > around the code and forget this, and much confusion will ensue. > > > > That helper should probably be in core drm_atomic.c, like the other > > set_foo_for_bar helpers. > As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :( > At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much.. I've meant just for the atomic helpers. i915 is a mess, yes, but that's not really an excuse to not make shared code pretty ;-) -Daniel
Op 04-12-15 om 09:12 schreef Daniel Vetter: > On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote: >> Op 03-12-15 om 10:53 schreef Daniel Vetter: >>> On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote: >>>> This allows iteration over encoders without requiring connection_mutex. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ >>>> drivers/gpu/drm/i915/intel_display.c | 3 +++ >>>> include/drm/drm_crtc.h | 2 ++ >>>> 3 files changed, 9 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>>> index fb79c54b2aed..f3fd8f131f92 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state) >>>> continue; >>>> >>>> drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode); >>>> + >>>> + crtc_state->encoder_mask = 0; >>> Hm, I think a small function to set the best_encoder (like we do to set >>> the crtc for connector or planes) would be good. Otherwise we'll frob >>> around the code and forget this, and much confusion will ensue. >>> >>> That helper should probably be in core drm_atomic.c, like the other >>> set_foo_for_bar helpers. >> As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :( >> At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much.. > I've meant just for the atomic helpers. i915 is a mess, yes, but that's > not really an excuse to not make shared code pretty ;-) > It's not really possible to do it in a helper. The encoder might be moved with the connector, or have a fixed mapping depending on crtc. (i915 MST) So unfortunately there can be no generic helper, but it has to be dealt with in this function, when assigning best_encoder per crtc. ~Maarten
On Mon, Dec 14, 2015 at 01:06:12PM +0100, Maarten Lankhorst wrote: > Op 04-12-15 om 09:12 schreef Daniel Vetter: > > On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote: > >> Op 03-12-15 om 10:53 schreef Daniel Vetter: > >>> On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote: > >>>> This allows iteration over encoders without requiring connection_mutex. > >>>> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> --- > >>>> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > >>>> drivers/gpu/drm/i915/intel_display.c | 3 +++ > >>>> include/drm/drm_crtc.h | 2 ++ > >>>> 3 files changed, 9 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >>>> index fb79c54b2aed..f3fd8f131f92 100644 > >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c > >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >>>> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state) > >>>> continue; > >>>> > >>>> drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode); > >>>> + > >>>> + crtc_state->encoder_mask = 0; > >>> Hm, I think a small function to set the best_encoder (like we do to set > >>> the crtc for connector or planes) would be good. Otherwise we'll frob > >>> around the code and forget this, and much confusion will ensue. > >>> > >>> That helper should probably be in core drm_atomic.c, like the other > >>> set_foo_for_bar helpers. > >> As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :( > >> At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much.. > > I've meant just for the atomic helpers. i915 is a mess, yes, but that's > > not really an excuse to not make shared code pretty ;-) > > > It's not really possible to do it in a helper. The encoder might be > moved with the connector, or have a fixed mapping depending on crtc. > (i915 MST) > > So unfortunately there can be no generic helper, but it has to be dealt > with in this function, when assigning best_encoder per crtc. I meant a drm_atomic_set_best_encoder function, which sets both best_encoder and updates the encoder_mask for the crtc. Why would that not work? Of course actually figuring out what the best_encoder is would not be done by that function, it would only update the book-keeping. And then we could reuse it in our state reconstruction code too. -Daniel
Op 15-12-15 om 10:17 schreef Daniel Vetter: > On Mon, Dec 14, 2015 at 01:06:12PM +0100, Maarten Lankhorst wrote: >> Op 04-12-15 om 09:12 schreef Daniel Vetter: >>> On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote: >>>> Op 03-12-15 om 10:53 schreef Daniel Vetter: >>>>> On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote: >>>>>> This allows iteration over encoders without requiring connection_mutex. >>>>>> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ >>>>>> drivers/gpu/drm/i915/intel_display.c | 3 +++ >>>>>> include/drm/drm_crtc.h | 2 ++ >>>>>> 3 files changed, 9 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>>>>> index fb79c54b2aed..f3fd8f131f92 100644 >>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>>>> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state) >>>>>> continue; >>>>>> >>>>>> drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode); >>>>>> + >>>>>> + crtc_state->encoder_mask = 0; >>>>> Hm, I think a small function to set the best_encoder (like we do to set >>>>> the crtc for connector or planes) would be good. Otherwise we'll frob >>>>> around the code and forget this, and much confusion will ensue. >>>>> >>>>> That helper should probably be in core drm_atomic.c, like the other >>>>> set_foo_for_bar helpers. >>>> As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :( >>>> At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much.. >>> I've meant just for the atomic helpers. i915 is a mess, yes, but that's >>> not really an excuse to not make shared code pretty ;-) >>> >> It's not really possible to do it in a helper. The encoder might be >> moved with the connector, or have a fixed mapping depending on crtc. >> (i915 MST) >> >> So unfortunately there can be no generic helper, but it has to be dealt >> with in this function, when assigning best_encoder per crtc. > I meant a drm_atomic_set_best_encoder function, which sets both > best_encoder and updates the encoder_mask for the crtc. Why would that not > work? Of course actually figuring out what the best_encoder is would not > be done by that function, it would only update the book-keeping. And then > we could reuse it in our state reconstruction code too. How do you want to do this race free in case of a fixed mapping of encoder to crtc? MST display, con 1 & 2 with enc 1 & 2 and crtc 1 & 2. crtc1 has con1 and enc1 crtc2 has con2 and enc2 Modeset with con1 moved to crtc2, and con2 to crtc1. No matter what order you use, if you clear anything in drm_atomic_set_best_encoder you would have a mismatch here. con1 first mapped to enc2: crtc1->encoder_mask = 0 crtc2->encoder_mask = enc2 | enc2 = enc2 con2 mapped to enc1, it was previously mapped to enc2, so lets clear it..: crtc1->encoder_mask = enc1. crtc2->encoder_mask = 0 <-- Oops! Same story if you do con2 first. This is why I chose to clear encoder_mask in mode_fixup instead. ~Maarten
On Thu, Dec 17, 2015 at 10:06:53AM +0100, Maarten Lankhorst wrote: > Op 15-12-15 om 10:17 schreef Daniel Vetter: > > On Mon, Dec 14, 2015 at 01:06:12PM +0100, Maarten Lankhorst wrote: > >> Op 04-12-15 om 09:12 schreef Daniel Vetter: > >>> On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote: > >>>> Op 03-12-15 om 10:53 schreef Daniel Vetter: > >>>>> On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote: > >>>>>> This allows iteration over encoders without requiring connection_mutex. > >>>>>> > >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>>> --- > >>>>>> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > >>>>>> drivers/gpu/drm/i915/intel_display.c | 3 +++ > >>>>>> include/drm/drm_crtc.h | 2 ++ > >>>>>> 3 files changed, 9 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >>>>>> index fb79c54b2aed..f3fd8f131f92 100644 > >>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c > >>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >>>>>> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state) > >>>>>> continue; > >>>>>> > >>>>>> drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode); > >>>>>> + > >>>>>> + crtc_state->encoder_mask = 0; > >>>>> Hm, I think a small function to set the best_encoder (like we do to set > >>>>> the crtc for connector or planes) would be good. Otherwise we'll frob > >>>>> around the code and forget this, and much confusion will ensue. > >>>>> > >>>>> That helper should probably be in core drm_atomic.c, like the other > >>>>> set_foo_for_bar helpers. > >>>> As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :( > >>>> At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much.. > >>> I've meant just for the atomic helpers. i915 is a mess, yes, but that's > >>> not really an excuse to not make shared code pretty ;-) > >>> > >> It's not really possible to do it in a helper. The encoder might be > >> moved with the connector, or have a fixed mapping depending on crtc. > >> (i915 MST) > >> > >> So unfortunately there can be no generic helper, but it has to be dealt > >> with in this function, when assigning best_encoder per crtc. > > I meant a drm_atomic_set_best_encoder function, which sets both > > best_encoder and updates the encoder_mask for the crtc. Why would that not > > work? Of course actually figuring out what the best_encoder is would not > > be done by that function, it would only update the book-keeping. And then > > we could reuse it in our state reconstruction code too. > How do you want to do this race free in case of a fixed mapping of encoder to crtc? > > MST display, con 1 & 2 with enc 1 & 2 and crtc 1 & 2. > > crtc1 has con1 and enc1 > crtc2 has con2 and enc2 > > Modeset with con1 moved to crtc2, and con2 to crtc1. > > No matter what order you use, if you clear anything in drm_atomic_set_best_encoder you would have a mismatch here. > > con1 first mapped to enc2: > > crtc1->encoder_mask = 0 > crtc2->encoder_mask = enc2 | enc2 = enc2 > > con2 mapped to enc1, it was previously mapped to enc2, so lets clear it..: > crtc1->encoder_mask = enc1. > crtc2->encoder_mask = 0 <-- Oops! > > Same story if you do con2 first. > > This is why I chose to clear encoder_mask in mode_fixup instead. With the recent fix to reassing encoders only once this should work, since it's really 4 steps: 1. steal enc2 from crtc2. We already figure out which connector enc2 has used, and by looking at con2->state->crtc we can get at the old crtc. So should be easy to clear crtc2_state->encoder_mask to no longer contain enc2. I.e. we have enc2, conn2 as known, so just do: get_crtc_state(conn2->state->crtc)->encoder_mask &= ~encoder_id(enc2); 2. We notice that conn1 already has a encoder assigned (enc1), and we need to clear that first. This step is new (well we could hide it in the set_best_encoder function), but works exactly like step 1. 3. add enc2 to crtc1: That's the easy part, just or in the new bit. Easy: crtc1_sate->encoder_mask |= encoder_id(enc2); With this enc2 is updated, and enc1 is cleared everywhere, including encoder_masks. 4. add enc1 for con1 like step 3. The important bit is simply to not overwrite a pointer before clearing the mask for the old value completely. If you look at set_crtc_* funcs that's exactly what they're doing. And since we can always get at the old state it should always be possible to clear out the old mask. Cheers, Daniel
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index fb79c54b2aed..f3fd8f131f92 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state) continue; drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode); + + crtc_state->encoder_mask = 0; } for_each_connector_in_state(state, connector, conn_state, i) { @@ -288,6 +290,8 @@ mode_fixup(struct drm_atomic_state *state) * it away), so we won't call ->mode_fixup twice. */ encoder = conn_state->best_encoder; + crtc_state->encoder_mask |= 1 << drm_encoder_index(encoder); + funcs = encoder->helper_private; if (!funcs) continue; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 21b1984e828b..3317db3806a8 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15310,6 +15310,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; crtc->base.state->connector_mask = 0; + crtc->base.state->encoder_mask = 0; /* Because we only establish the connector -> encoder -> * crtc links if something is active, this means the @@ -15363,6 +15364,8 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) crtc->state->connector_mask |= 1 << drm_connector_index(&connector->base); + crtc->state->encoder_mask |= + 1 << drm_encoder_index(&encoder->base); } if (active && !has_active_crtc) { diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 4999fe530f37..7ea83b6a2864 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -265,6 +265,7 @@ struct drm_atomic_state; * @connectors_changed: connectors to this crtc have been updated * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors + * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders * @last_vblank_count: for helpers and drivers to capture the vblank of the * update to ensure framebuffer cleanup isn't done too early * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings @@ -299,6 +300,7 @@ struct drm_crtc_state { u32 plane_mask; u32 connector_mask; + u32 encoder_mask; /* last_vblank_count: for vblank waits before cleanup */ u32 last_vblank_count;
This allows iteration over encoders without requiring connection_mutex. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ drivers/gpu/drm/i915/intel_display.c | 3 +++ include/drm/drm_crtc.h | 2 ++ 3 files changed, 9 insertions(+)