| Message ID | 20190524162139.4446-6-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On Fri, May 24, 2019 at 06:21:38PM +0200, Jacopo Mondi wrote: > Fix compilation error: > include/linux/intel-ipu3.h:2475:35: error: ‘ipu3_uapi_acc_param::awb_fr’ > offset 36756 in ‘ipu3_uapi_acc_param’ isn’t aligned to 32 > [-Werror=packed-not-aligned] > > by forcing alignment to 32 bytes for struct ipu3_uapi_awb_fr_config_s. > > As the header is exported from Linux v5.1 this is a workaround and > should probably be fixed in the kernel headers themselves. Does it change the layout of the structure in memory ? If so we can't do this, as the structure is passed to the hardware (or rather firmware) so its layout is fixed. > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/linux/intel-ipu3.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h > index f758c9ba230b..fe6e8ed15eb1 100644 > --- a/include/linux/intel-ipu3.h > +++ b/include/linux/intel-ipu3.h > @@ -2472,7 +2472,7 @@ struct ipu3_uapi_acc_param { > struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32))); > struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32))); > struct ipu3_uapi_anr_config anr; > - struct ipu3_uapi_awb_fr_config_s awb_fr; > + struct ipu3_uapi_awb_fr_config_s awb_fr __attribute__((aligned(32))); > struct ipu3_uapi_ae_config ae; > struct ipu3_uapi_af_config_s af; > struct ipu3_uapi_awb_config awb;
Hi Laurent, On Fri, May 24, 2019 at 10:37:22PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, May 24, 2019 at 06:21:38PM +0200, Jacopo Mondi wrote: > > Fix compilation error: > > include/linux/intel-ipu3.h:2475:35: error: ‘ipu3_uapi_acc_param::awb_fr’ > > offset 36756 in ‘ipu3_uapi_acc_param’ isn’t aligned to 32 > > [-Werror=packed-not-aligned] > > > > by forcing alignment to 32 bytes for struct ipu3_uapi_awb_fr_config_s. > > > > As the header is exported from Linux v5.1 this is a workaround and > > should probably be fixed in the kernel headers themselves. > > Does it change the layout of the structure in memory ? If so we can't do > this, as the structure is passed to the hardware (or rather firmware) so > its layout is fixed. Good question. The 'struct ipu3_uapi_awb_fr_config_s' is itself defined as struct ipu3_uapi_awb_fr_config_s { struct ipu3_uapi_grid_config grid_cfg; __u8 bayer_coeff[6]; __u16 reserved1; __u32 bayer_sign; __u8 bayer_nf; __u8 reserved2[3]; } __attribute__((aligned(32))) __attribute__((packed)); So it seems it is already 32 bytes aligned. The error I tried to fix here is reported by gcc8.3.0 while gcc5.4.0 (which I use to build on my Soraka target) does not complain about it. I also tried with clang8 and it actually complains about a different issue, but not this one: ------------------------------------------------------------------------------- o 'src/libcamera/4ab8042@@camera@sha/pipeline_ipu3_ipu3.cpp.o' -c ../src/libcamera/pipeline/ipu3/ipu3.cpp ../src/libcamera/pipeline/ipu3/ipu3.cpp:994:46: error: taking address of packed member 'awb_raw_buffer' of class or structure 'ipu3_uapi_stats_3a' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member] struct ipu3_uapi_awb_raw_buffer *raw_awb = &stats_3a->awb_raw_buffer; ^~~~~~~~~~~~~~~~~~~~~~~~ ------------------------------------------------------------------------------- This one solved as well by adding an __attribute__((aligned(32))) to the 'awb_raw_buffer' field definition in 'struct ipu3_uapi_stats_3a' To add fun, clang3.8 complains about yet-a-different-error but not this one, see below [1]). This inconsistencies between different compiler and different versions of the same compiler makes me think this is a newly introduced warning/error flag and compilers are still stabilizing on (or their behavior is actually diverging). To sum up: I think it is safe to add that __aligned__ directive, as printing out the size of the structure gives back a number which is a 32 bytes multiple. The error is only reported by gcc8 not gcc5.4 and not by any clang version, not clang8 nor clang3.8 (which in turn, complain about other two different issues...) I would lean on keeping this patch in our tree, just to silence gcc8 while verifing if the other version still complains about it or not, possibly with the help of buildroot builds, which might tests far more compiler versions than what we could do by ourselves alone. What do others (specifically who worked on integration with multiple toolchains as Kieran did) think on this? Thanks j [1] Compiling with clang3.8.0 (which I use on my Soraka taget) leads to yet-another-error, which I report for completness here. This might actually deserve a fix and I'll consider sending a patch. ------------------------------------------------------------------------------- [2/29] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/pipeline_rkisp1_rkisp1.cpp.o'. ninja: build stopped: subcommand failed. root@libcamera:/home/cam/libcamera.git/build-clang# ninja [29/29] Linking target test/timer. root@libcamera:/home/cam/libcamera.git/build-clang# ninja [5/30] Generating symbol file 'src/libcamera/4ab8042@@camera@sha/libcamera.so.symbols'. root@libcamera:/home/cam/libcamera.git/build-clang# ninja [3/30] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/pipeline_vimc.cpp.o'. FAILED: clang++ -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../src/libcamera -Iinclude -I../include -I../src/libcamera/include -Xclang -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++11 -g -Wno-unused-parameter -include config.h -fPIC -MD -MQ 'src/libcamera/4ab8042@@camera@sha/pipeline_vimc.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/pipeline_vimc.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/pipeline_vimc.cpp.o' -c ../src/libcamera/pipeline/vimc.cpp ../src/libcamera/pipeline/vimc.cpp:90:3: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] V4L2_PIX_FMT_BGR24, ^~~~~~~~~~~~~~~~~~~ ../include/linux/videodev2.h:520:30: note: expanded from macro 'V4L2_PIX_FMT_BGR24' #define V4L2_PIX_FMT_BGR24 v4l2_fourcc('B', 'G', 'R', '3') /* 24 BGR-8-8-8 */ ^ ../include/linux/videodev2.h:80:2: note: expanded from macro 'v4l2_fourcc' ((__u32)(a) | ((__u32)(b) << 8) | ((__u32)(c) << 16) | ((__u32)(d) << 24)) ^ 1 error generated. [3/30] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/pipeline_rkisp1_rkisp1.cpp.o'. FAILED: clang++ -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../src/libcamera -Iinclude -I../include -I../src/libcamera/include -Xclang -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++11 -g -Wno-unused-parameter -include config.h -fPIC -MD -MQ 'src/libcamera/4ab8042@@camera@sha/pipeline_rkisp1_rkisp1.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/pipeline_rkisp1_rkisp1.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/pipeline_rkisp1_rkisp1.cpp.o' -c ../src/libcamera/pipeline/rkisp1/rkisp1.cpp ../src/libcamera/pipeline/rkisp1/rkisp1.cpp:125:3: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] V4L2_PIX_FMT_YUYV, ^~~~~~~~~~~~~~~~~~ ../include/linux/videodev2.h:549:30: note: expanded from macro 'V4L2_PIX_FMT_YUYV' #define V4L2_PIX_FMT_YUYV v4l2_fourcc('Y', 'U', 'Y', 'V') /* 16 YUV 4:2:2 */ ^ ../include/linux/videodev2.h:80:2: note: expanded from macro 'v4l2_fourcc' ((__u32)(a) | ((__u32)(b) << 8) | ((__u32)(c) << 16) | ((__u32)(d) << 24)) ^ 1 error generated. [3/30] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/pipeline_ipu3_ipu3.cpp.o'. ninja: build stopped: subcommand failed. ------------------------------------------------------------------------------- > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/linux/intel-ipu3.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h > > index f758c9ba230b..fe6e8ed15eb1 100644 > > --- a/include/linux/intel-ipu3.h > > +++ b/include/linux/intel-ipu3.h > > @@ -2472,7 +2472,7 @@ struct ipu3_uapi_acc_param { > > struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32))); > > struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32))); > > struct ipu3_uapi_anr_config anr; > > - struct ipu3_uapi_awb_fr_config_s awb_fr; > > + struct ipu3_uapi_awb_fr_config_s awb_fr __attribute__((aligned(32))); > > struct ipu3_uapi_ae_config ae; > > struct ipu3_uapi_af_config_s af; > > struct ipu3_uapi_awb_config awb; > > -- > Regards, > > Laurent Pinchart
Hi, The proposed upstream fix is [1], I propose we take this patch as is but mention the upstream patch in the commit message. Then when the fix hits upstream refresh the header in libcamera. 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg145250.html With this mentioned in the commit message, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> On 2019-05-27 10:42:26 +0200, Jacopo Mondi wrote: > Hi Laurent, > > On Fri, May 24, 2019 at 10:37:22PM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > On Fri, May 24, 2019 at 06:21:38PM +0200, Jacopo Mondi wrote: > > > Fix compilation error: > > > include/linux/intel-ipu3.h:2475:35: error: ‘ipu3_uapi_acc_param::awb_fr’ > > > offset 36756 in ‘ipu3_uapi_acc_param’ isn’t aligned to 32 > > > [-Werror=packed-not-aligned] > > > > > > by forcing alignment to 32 bytes for struct ipu3_uapi_awb_fr_config_s. > > > > > > As the header is exported from Linux v5.1 this is a workaround and > > > should probably be fixed in the kernel headers themselves. > > > > Does it change the layout of the structure in memory ? If so we can't do > > this, as the structure is passed to the hardware (or rather firmware) so > > its layout is fixed. > > Good question. > > The 'struct ipu3_uapi_awb_fr_config_s' is itself defined as > > struct ipu3_uapi_awb_fr_config_s { > struct ipu3_uapi_grid_config grid_cfg; > __u8 bayer_coeff[6]; > __u16 reserved1; > __u32 bayer_sign; > __u8 bayer_nf; > __u8 reserved2[3]; > } __attribute__((aligned(32))) __attribute__((packed)); > > So it seems it is already 32 bytes aligned. > > The error I tried to fix here is reported by gcc8.3.0 while gcc5.4.0 > (which I use to build on my Soraka target) does not complain about it. > > I also tried with clang8 and it actually complains about a different issue, > but not this one: > > ------------------------------------------------------------------------------- > o 'src/libcamera/4ab8042@@camera@sha/pipeline_ipu3_ipu3.cpp.o' -c ../src/libcamera/pipeline/ipu3/ipu3.cpp > ../src/libcamera/pipeline/ipu3/ipu3.cpp:994:46: error: taking address of packed member 'awb_raw_buffer' of class or structure 'ipu3_uapi_stats_3a' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member] > struct ipu3_uapi_awb_raw_buffer *raw_awb = &stats_3a->awb_raw_buffer; > ^~~~~~~~~~~~~~~~~~~~~~~~ > ------------------------------------------------------------------------------- > > This one solved as well by adding an __attribute__((aligned(32))) to the > 'awb_raw_buffer' field definition in 'struct ipu3_uapi_stats_3a' > > To add fun, clang3.8 complains about yet-a-different-error but not > this one, see below [1]). > > This inconsistencies between different compiler and different versions > of the same compiler makes me think this is a newly introduced > warning/error flag and compilers are still stabilizing on (or their > behavior is actually diverging). > > To sum up: I think it is safe to add that __aligned__ directive, as > printing out the size of the structure gives back a number which is a > 32 bytes multiple. The error is only reported by gcc8 not gcc5.4 and > not by any clang version, not clang8 nor clang3.8 (which in turn, > complain about other two different issues...) > > I would lean on keeping this patch in our tree, just to silence gcc8 > while verifing if the other version still complains about it or not, > possibly with the help of buildroot builds, which might tests far more > compiler versions than what we could do by ourselves alone. > > What do others (specifically who worked on integration with multiple > toolchains as Kieran did) think on this? > > Thanks > j > > [1] Compiling with clang3.8.0 (which I use on my Soraka taget) leads to > yet-another-error, which I report for completness here. This might > actually deserve a fix and I'll consider sending a patch. > > ------------------------------------------------------------------------------- > [2/29] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/pipeline_rkisp1_rkisp1.cpp.o'. > ninja: build stopped: subcommand failed. > root@libcamera:/home/cam/libcamera.git/build-clang# ninja > [29/29] Linking target test/timer. > root@libcamera:/home/cam/libcamera.git/build-clang# ninja > [5/30] Generating symbol file 'src/libcamera/4ab8042@@camera@sha/libcamera.so.symbols'. > root@libcamera:/home/cam/libcamera.git/build-clang# ninja > [3/30] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/pipeline_vimc.cpp.o'. > FAILED: clang++ -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../src/libcamera -Iinclude -I../include -I../src/libcamera/include -Xclang -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++11 -g -Wno-unused-parameter -include config.h -fPIC -MD -MQ 'src/libcamera/4ab8042@@camera@sha/pipeline_vimc.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/pipeline_vimc.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/pipeline_vimc.cpp.o' -c ../src/libcamera/pipeline/vimc.cpp > ../src/libcamera/pipeline/vimc.cpp:90:3: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] > V4L2_PIX_FMT_BGR24, > ^~~~~~~~~~~~~~~~~~~ > ../include/linux/videodev2.h:520:30: note: expanded from macro 'V4L2_PIX_FMT_BGR24' > #define V4L2_PIX_FMT_BGR24 v4l2_fourcc('B', 'G', 'R', '3') /* 24 BGR-8-8-8 */ > ^ > ../include/linux/videodev2.h:80:2: note: expanded from macro 'v4l2_fourcc' > ((__u32)(a) | ((__u32)(b) << 8) | ((__u32)(c) << 16) | ((__u32)(d) << 24)) > ^ > 1 error generated. > [3/30] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/pipeline_rkisp1_rkisp1.cpp.o'. > FAILED: clang++ -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../src/libcamera -Iinclude -I../include -I../src/libcamera/include -Xclang -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++11 -g -Wno-unused-parameter -include config.h -fPIC -MD -MQ 'src/libcamera/4ab8042@@camera@sha/pipeline_rkisp1_rkisp1.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/pipeline_rkisp1_rkisp1.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/pipeline_rkisp1_rkisp1.cpp.o' -c ../src/libcamera/pipeline/rkisp1/rkisp1.cpp > ../src/libcamera/pipeline/rkisp1/rkisp1.cpp:125:3: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] > V4L2_PIX_FMT_YUYV, > ^~~~~~~~~~~~~~~~~~ > ../include/linux/videodev2.h:549:30: note: expanded from macro 'V4L2_PIX_FMT_YUYV' > #define V4L2_PIX_FMT_YUYV v4l2_fourcc('Y', 'U', 'Y', 'V') /* 16 YUV 4:2:2 */ > ^ > ../include/linux/videodev2.h:80:2: note: expanded from macro 'v4l2_fourcc' > ((__u32)(a) | ((__u32)(b) << 8) | ((__u32)(c) << 16) | ((__u32)(d) << 24)) > ^ > 1 error generated. > [3/30] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/pipeline_ipu3_ipu3.cpp.o'. > ninja: build stopped: subcommand failed. > ------------------------------------------------------------------------------- > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/linux/intel-ipu3.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h > > > index f758c9ba230b..fe6e8ed15eb1 100644 > > > --- a/include/linux/intel-ipu3.h > > > +++ b/include/linux/intel-ipu3.h > > > @@ -2472,7 +2472,7 @@ struct ipu3_uapi_acc_param { > > > struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32))); > > > struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32))); > > > struct ipu3_uapi_anr_config anr; > > > - struct ipu3_uapi_awb_fr_config_s awb_fr; > > > + struct ipu3_uapi_awb_fr_config_s awb_fr __attribute__((aligned(32))); > > > struct ipu3_uapi_ae_config ae; > > > struct ipu3_uapi_af_config_s af; > > > struct ipu3_uapi_awb_config awb; > > > > -- > > Regards, > > > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h index f758c9ba230b..fe6e8ed15eb1 100644 --- a/include/linux/intel-ipu3.h +++ b/include/linux/intel-ipu3.h @@ -2472,7 +2472,7 @@ struct ipu3_uapi_acc_param { struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32))); struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32))); struct ipu3_uapi_anr_config anr; - struct ipu3_uapi_awb_fr_config_s awb_fr; + struct ipu3_uapi_awb_fr_config_s awb_fr __attribute__((aligned(32))); struct ipu3_uapi_ae_config ae; struct ipu3_uapi_af_config_s af; struct ipu3_uapi_awb_config awb;
Fix compilation error: include/linux/intel-ipu3.h:2475:35: error: ‘ipu3_uapi_acc_param::awb_fr’ offset 36756 in ‘ipu3_uapi_acc_param’ isn’t aligned to 32 [-Werror=packed-not-aligned] by forcing alignment to 32 bytes for struct ipu3_uapi_awb_fr_config_s. As the header is exported from Linux v5.1 this is a workaround and should probably be fixed in the kernel headers themselves. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/linux/intel-ipu3.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)