| Message ID | 20170303154644.6709-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 03, 2017 at 03:46:44PM +0000, Chris Wilson wrote: > To prevent having to preserve the drm_crtc_state as we clear the > intel_crtc_state, only memset our extended state. > > Fixes: > drivers/gpu/drm/i915/intel_display.c: In function ‘clear_intel_crtc_state’: > drivers/gpu/drm/i915/intel_display.c:11301:1: error: the frame size of 1056 bytes is larger than 1024 bytes [-Werror=frame-larger-than=] > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_display.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e6881a69a88f..cfab4d135af3 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11269,7 +11269,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = > to_i915(crtc_state->base.crtc->dev); > - struct drm_crtc_state tmp_state; > struct intel_crtc_scaler_state scaler_state; > struct intel_dpll_hw_state dpll_hw_state; > struct intel_shared_dpll *shared_dpll; > @@ -11281,7 +11280,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > * fixed, so that the crtc_state can be safely duplicated. For now, > * only fields that are know to not cause problems are preserved. */ > > - tmp_state = crtc_state->base; > scaler_state = crtc_state->scaler_state; > shared_dpll = crtc_state->shared_dpll; > dpll_hw_state = crtc_state->dpll_hw_state; > @@ -11289,9 +11287,9 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > wm_state = crtc_state->wm; > > - memset(crtc_state, 0, sizeof *crtc_state); > + memset(&crtc_state->base + 1, 0, > + sizeof(*crtc_state) - sizeof(crtc_state->base)); Maybe add a comment like /* Only clear our part of the overall struct. */ or similar, since this is not entirely obvious what's going on. Also COMPILE_BUG_ON(offsetof(struct intel_crtc_state, base) != 0); maybe? Anyway, I'll let you decide on both, either way: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > - crtc_state->base = tmp_state; > crtc_state->scaler_state = scaler_state; > crtc_state->shared_dpll = shared_dpll; > crtc_state->dpll_hw_state = dpll_hw_state; > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Mar 06, 2017 at 06:46:16PM +0100, Daniel Vetter wrote: > On Fri, Mar 03, 2017 at 03:46:44PM +0000, Chris Wilson wrote: > > @@ -11289,9 +11287,9 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > wm_state = crtc_state->wm; > > > > - memset(crtc_state, 0, sizeof *crtc_state); > > + memset(&crtc_state->base + 1, 0, > > + sizeof(*crtc_state) - sizeof(crtc_state->base)); > > Maybe add a comment like /* Only clear our part of the overall struct. */ > or similar, since this is not entirely obvious what's going on. Also > > COMPILE_BUG_ON(offsetof(struct intel_crtc_state, base) != 0); > > maybe? Anyway, I'll let you decide on both, either way: I thought we had the BUILD_BUG_ON already -- the NULL correspondance is widely used. I did one version with it, then thought that it would be better in to_intel_crtc_state() and so left it out. -Chris
On Mon, Mar 06, 2017 at 06:46:16PM +0100, Daniel Vetter wrote: > On Fri, Mar 03, 2017 at 03:46:44PM +0000, Chris Wilson wrote: > > To prevent having to preserve the drm_crtc_state as we clear the > > intel_crtc_state, only memset our extended state. > > > > Fixes: > > drivers/gpu/drm/i915/intel_display.c: In function ‘clear_intel_crtc_state’: > > drivers/gpu/drm/i915/intel_display.c:11301:1: error: the frame size of 1056 bytes is larger than 1024 bytes [-Werror=frame-larger-than=] > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/intel_display.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index e6881a69a88f..cfab4d135af3 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -11269,7 +11269,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > > { > > struct drm_i915_private *dev_priv = > > to_i915(crtc_state->base.crtc->dev); > > - struct drm_crtc_state tmp_state; > > struct intel_crtc_scaler_state scaler_state; > > struct intel_dpll_hw_state dpll_hw_state; > > struct intel_shared_dpll *shared_dpll; > > @@ -11281,7 +11280,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > > * fixed, so that the crtc_state can be safely duplicated. For now, > > * only fields that are know to not cause problems are preserved. */ > > > > - tmp_state = crtc_state->base; > > scaler_state = crtc_state->scaler_state; > > shared_dpll = crtc_state->shared_dpll; > > dpll_hw_state = crtc_state->dpll_hw_state; > > @@ -11289,9 +11287,9 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > wm_state = crtc_state->wm; > > > > - memset(crtc_state, 0, sizeof *crtc_state); > > + memset(&crtc_state->base + 1, 0, > > + sizeof(*crtc_state) - sizeof(crtc_state->base)); > > Maybe add a comment like /* Only clear our part of the overall struct. */ > or similar, since this is not entirely obvious what's going on. Also > > COMPILE_BUG_ON(offsetof(struct intel_crtc_state, base) != 0); > > maybe? Anyway, I'll let you decide on both, either way: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Did both and pushed. Thanks, a few of my machines were tripping over this which is a bit puzzling as to how they have different values for the stack warning. -Chris
On Mon, Mar 06, 2017 at 09:05:47PM +0000, Chris Wilson wrote: > On Mon, Mar 06, 2017 at 06:46:16PM +0100, Daniel Vetter wrote: > > On Fri, Mar 03, 2017 at 03:46:44PM +0000, Chris Wilson wrote: > > > @@ -11289,9 +11287,9 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > wm_state = crtc_state->wm; > > > > > > - memset(crtc_state, 0, sizeof *crtc_state); > > > + memset(&crtc_state->base + 1, 0, > > > + sizeof(*crtc_state) - sizeof(crtc_state->base)); > > > > Maybe add a comment like /* Only clear our part of the overall struct. */ > > or similar, since this is not entirely obvious what's going on. Also > > > > COMPILE_BUG_ON(offsetof(struct intel_crtc_state, base) != 0); > > > > maybe? Anyway, I'll let you decide on both, either way: > > I thought we had the BUILD_BUG_ON already -- the NULL correspondance is > widely used. I did one version with it, then thought that it would be > better in to_intel_crtc_state() and so left it out. I posted a patch for that once but I never finished it. In the end I was even thinking of having both const and non-const to_foo() macros. If someone wants to continue my work I pushed it here: git://github.com/vsyrjala/linux.git kms_obj_base_0_2
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e6881a69a88f..cfab4d135af3 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11269,7 +11269,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); - struct drm_crtc_state tmp_state; struct intel_crtc_scaler_state scaler_state; struct intel_dpll_hw_state dpll_hw_state; struct intel_shared_dpll *shared_dpll; @@ -11281,7 +11280,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) * fixed, so that the crtc_state can be safely duplicated. For now, * only fields that are know to not cause problems are preserved. */ - tmp_state = crtc_state->base; scaler_state = crtc_state->scaler_state; shared_dpll = crtc_state->shared_dpll; dpll_hw_state = crtc_state->dpll_hw_state; @@ -11289,9 +11287,9 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) wm_state = crtc_state->wm; - memset(crtc_state, 0, sizeof *crtc_state); + memset(&crtc_state->base + 1, 0, + sizeof(*crtc_state) - sizeof(crtc_state->base)); - crtc_state->base = tmp_state; crtc_state->scaler_state = scaler_state; crtc_state->shared_dpll = shared_dpll; crtc_state->dpll_hw_state = dpll_hw_state;
To prevent having to preserve the drm_crtc_state as we clear the intel_crtc_state, only memset our extended state. Fixes: drivers/gpu/drm/i915/intel_display.c: In function ‘clear_intel_crtc_state’: drivers/gpu/drm/i915/intel_display.c:11301:1: error: the frame size of 1056 bytes is larger than 1024 bytes [-Werror=frame-larger-than=] Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_display.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)