Re: [Intel-gfx] [PATCH v2 2/7] drm/i915: Mark the ggtt_view structs as packed
On 13/01/2017 08:47, Chris Wilson wrote: On Fri, Jan 13, 2017 at 08:44:34AM +, Tvrtko Ursulin wrote: On 12/01/2017 21:35, Chris Wilson wrote: In the next few patches, we will depend upon there being no uninitialised bits inside the ggtt_view. To ensure this we add the __packed attribute and double check with a build on that gcc hasn't expanded the struct to include some padding bytes. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_gem_gtt.h | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 5dd3755a5a45..57cbd532dae1 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -156,12 +156,22 @@ struct intel_rotation_info { /* tiles */ unsigned int width, height, stride, offset; } plane[2]; Isn't packed theoretically needed on the intel_rotation_plane_info name in the previous patch as well? Otherwise there could be a hole between the array elements if the structure got changed in the future. Possibly, not too sure on the inner details of __packed. The assert below will catch accidental holes in future (and if they change the struct without changing the assert, a nuisance). +static inline void assert_intel_rotation_info_is_packed(void) +{ + BUILD_BUG_ON(sizeof(struct intel_rotation_info) != 8*sizeof(unsigned int)); +} Somehow I only saw the assert for partial info. Reviewed-by: Tvrtko Ursulin Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/7] drm/i915: Mark the ggtt_view structs as packed
On Fri, Jan 13, 2017 at 08:44:34AM +, Tvrtko Ursulin wrote: > > On 12/01/2017 21:35, Chris Wilson wrote: > >In the next few patches, we will depend upon there being no > >uninitialised bits inside the ggtt_view. To ensure this we add the > >__packed attribute and double check with a build on that gcc hasn't > >expanded the struct to include some padding bytes. > > > >Signed-off-by: Chris Wilson> >--- > > drivers/gpu/drm/i915/i915_gem_gtt.h | 14 -- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > >b/drivers/gpu/drm/i915/i915_gem_gtt.h > >index 5dd3755a5a45..57cbd532dae1 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > >@@ -156,12 +156,22 @@ struct intel_rotation_info { > > /* tiles */ > > unsigned int width, height, stride, offset; > > } plane[2]; > > Isn't packed theoretically needed on the intel_rotation_plane_info > name in the previous patch as well? Otherwise there could be a hole > between the array elements if the structure got changed in the > future. Possibly, not too sure on the inner details of __packed. The assert below will catch accidental holes in future (and if they change the struct without changing the assert, a nuisance). > >+static inline void assert_intel_rotation_info_is_packed(void) > >+{ > >+BUILD_BUG_ON(sizeof(struct intel_rotation_info) != 8*sizeof(unsigned > >int)); > >+} -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/7] drm/i915: Mark the ggtt_view structs as packed
On 12/01/2017 21:35, Chris Wilson wrote: In the next few patches, we will depend upon there being no uninitialised bits inside the ggtt_view. To ensure this we add the __packed attribute and double check with a build on that gcc hasn't expanded the struct to include some padding bytes. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_gem_gtt.h | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 5dd3755a5a45..57cbd532dae1 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -156,12 +156,22 @@ struct intel_rotation_info { /* tiles */ unsigned int width, height, stride, offset; } plane[2]; Isn't packed theoretically needed on the intel_rotation_plane_info name in the previous patch as well? Otherwise there could be a hole between the array elements if the structure got changed in the future. Regards, Tvrtko -}; +} __packed; + +static inline void assert_intel_rotation_info_is_packed(void) +{ + BUILD_BUG_ON(sizeof(struct intel_rotation_info) != 8*sizeof(unsigned int)); +} struct intel_partial_info { u64 offset; unsigned int size; -}; +} __packed; + +static inline void assert_intel_partial_info_is_packed(void) +{ + BUILD_BUG_ON(sizeof(struct intel_partial_info) != sizeof(u64) + sizeof(unsigned int)); +} struct i915_ggtt_view { enum i915_ggtt_view_type type; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx