Hi Kieran, Thank you for the patch. On Friday, 3 August 2018 14:37:21 EEST Kieran Bingham wrote: > From: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx> > > The kernel provides a __packed definition to abstract away from the > compiler specific attributes tag. > > Convert all packed structures in VSP1 to use it. > > The GCC documentation [0] describes this attribute as "the structure or > union is placed to minimize the memory required". > > The Keil compiler documentation at [1] warns that the use of this > attribute can cause a performance penalty in the event that the compiler > can not deduce the allignment of each field. > > Careful examination of the object code generated both with and without > this attribute shows that these structures are accessed identically and > are not affected by any performance penalty. The structures are > correctly aligned and padded to match the needs of the hardware already. > > This patch does not serve to make a decision as to the use of the > attribute, but purely to clean up the code to use the kernel defined > abstraction as per [2]. > > [0] > https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-packed > -type-attribute [1] > http://www.keil.com/support/man/docs/armcc/armcc_chr1359124230195.htm [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/inc > lude/linux/compiler-gcc.h?h=v4.16-rc5#n92 > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > --- > > This patch had some lengthy discussion about the use of the packed > attribute. In the end, after discussing further with Laurent, we decided > that as no performance impact occurs on the VSP1 driver (object code > generated is identical), and that as this attribute clearly marks > structures which are read by the VSP1, we can just keep it. > > This patch neither adds, nor removes the attribute - but purely adapts > to using the linux macro as defined at [2] to ensure that compiler > specifics are abstracted out. And in particular I feel that __packed is > cleaner than __attribute__((__packed__)) > > v2: > - Remove attributes entirely > > v6: > - Re-added the attributes (back to v1 of this patch) > > drivers/media/platform/vsp1/vsp1_dl.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_dl.c > b/drivers/media/platform/vsp1/vsp1_dl.c index 10a24bde2299..e4aae334f047 > 100644 > --- a/drivers/media/platform/vsp1/vsp1_dl.c > +++ b/drivers/media/platform/vsp1/vsp1_dl.c > @@ -25,19 +25,19 @@ > struct vsp1_dl_header_list { > u32 num_bytes; > u32 addr; > -} __attribute__((__packed__)); > +} __packed; > > struct vsp1_dl_header { > u32 num_lists; > struct vsp1_dl_header_list lists[8]; > u32 next_header; > u32 flags; > -} __attribute__((__packed__)); > +} __packed; > > struct vsp1_dl_entry { > u32 addr; > u32 data; > -} __attribute__((__packed__)); > +} __packed; > > /** > * struct vsp1_dl_body - Display list body -- Regards, Laurent Pinchart
