Quoting Ville Syrjälä (2019-02-05 19:10:39)
> On Tue, Feb 05, 2019 at 09:27:59AM +0000, Chris Wilson wrote:
> > clear_intel_crtc_state() uses the stack for saving a temporary copy of
> > certain bits of the inherited crtc_state before clearing the unwanted
> > bits. This pushes it over the stack limit for my little 32b Pineview,
> > so move the temporary allocation to the heap instead. As we now use a
> > zeroed struct, we can copy the whole extended state back to both
> > preserve what bits need to be preserved and zero the rest.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++---------------
> > 1 file changed, 18 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 636b703e02cb..6ee0cd27e875 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11468,44 +11468,38 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
> > return ret;
> > }
> >
> > -static void
> > +static int
> > clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > {
> > struct drm_i915_private *dev_priv =
> > to_i915(crtc_state->base.crtc->dev);
> > - struct intel_crtc_scaler_state scaler_state;
> > - struct intel_dpll_hw_state dpll_hw_state;
> > - struct intel_shared_dpll *shared_dpll;
> > - struct intel_crtc_wm_state wm_state;
> > - bool force_thru, ips_force_disable;
> > + struct intel_crtc_state *saved_state;
> > +
> > + saved_state = kzalloc(sizeof(*saved_state), GFP_KERNEL);
> > + if (!saved_state)
> > + return -ENOMEM;
> >
> > /* FIXME: before the switch to atomic started, a new pipe_config was
> > * kzalloc'd. Code that depends on any field being zero should be
> > * fixed, so that the crtc_state can be safely duplicated. For now,
> > * only fields that are know to not cause problems are preserved. */
> >
> > - scaler_state = crtc_state->scaler_state;
> > - shared_dpll = crtc_state->shared_dpll;
> > - dpll_hw_state = crtc_state->dpll_hw_state;
> > - force_thru = crtc_state->pch_pfit.force_thru;
> > - ips_force_disable = crtc_state->ips_force_disable;
> > + saved_state->scaler_state = crtc_state->scaler_state;
> > + saved_state->shared_dpll = crtc_state->shared_dpll;
> > + saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > + saved_state->pch_pfit.force_thru = crtc_state->pch_pfit.force_thru;
> > + saved_state->ips_force_disable = crtc_state->ips_force_disable;
> > if (IS_G4X(dev_priv) ||
> > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > - wm_state = crtc_state->wm;
> > + saved_state->wm = crtc_state->wm;
> >
> > /* Keep base drm_crtc_state intact, only clear our extended struct */
> > BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > - memset(&crtc_state->base + 1, 0,
> > + memcpy(&crtc_state->base + 1, &saved_state->base + 1,
> > sizeof(*crtc_state) - sizeof(crtc_state->base));
> >
> > - crtc_state->scaler_state = scaler_state;
> > - crtc_state->shared_dpll = shared_dpll;
> > - crtc_state->dpll_hw_state = dpll_hw_state;
> > - crtc_state->pch_pfit.force_thru = force_thru;
> > - crtc_state->ips_force_disable = ips_force_disable;
> > - if (IS_G4X(dev_priv) ||
> > - IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > - crtc_state->wm = wm_state;
>
> Nice. Less risk of someone messing up the save vs. restore.
Yup, using the heap is a small price to pay for less typing ;)
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Ta, pushed.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
