[libcamera-devel] [PATCH 5/6] include: linux: intel-ipu3: Force alignement to 32 bytes
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon May 27 15:22:39 CEST 2019
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 at 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 at 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 at sha/pipeline_rkisp1_rkisp1.cpp.o'.
> ninja: build stopped: subcommand failed.
> root at libcamera:/home/cam/libcamera.git/build-clang# ninja
> [29/29] Linking target test/timer.
> root at libcamera:/home/cam/libcamera.git/build-clang# ninja
> [5/30] Generating symbol file 'src/libcamera/4ab8042@@camera at sha/libcamera.so.symbols'.
> root at libcamera:/home/cam/libcamera.git/build-clang# ninja
> [3/30] Compiling C++ object 'src/libcamera/4ab8042@@camera at sha/pipeline_vimc.cpp.o'.
> FAILED: clang++ -Isrc/libcamera/4ab8042@@camera at 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 at sha/pipeline_vimc.cpp.o' -MF 'src/libcamera/4ab8042@@camera at sha/pipeline_vimc.cpp.o.d' -o 'src/libcamera/4ab8042@@camera at 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 at sha/pipeline_rkisp1_rkisp1.cpp.o'.
> FAILED: clang++ -Isrc/libcamera/4ab8042@@camera at 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 at sha/pipeline_rkisp1_rkisp1.cpp.o' -MF 'src/libcamera/4ab8042@@camera at sha/pipeline_rkisp1_rkisp1.cpp.o.d' -o 'src/libcamera/4ab8042@@camera at 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 at sha/pipeline_ipu3_ipu3.cpp.o'.
> ninja: build stopped: subcommand failed.
> -------------------------------------------------------------------------------
>
> >
> > > Signed-off-by: Jacopo Mondi <jacopo at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list