Hi Sakari,
On Fri, Mar 1, 2019 at 12:24 PM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Address a compiler warnings on alignment of struct ipu3_uapi_awb_fr_config_s
> by adding __attribute__((aligned(32))) to a struct member of that type as
> well.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Tested-by: Rajmohan Mani <rajmohan.mani@xxxxxxxxx>
> ---
> drivers/staging/media/ipu3/include/intel-ipu3.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> index eb6f52aca9929..4a0e97b0cfd2b 100644
> --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/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;
> --
> 2.11.0
>
I don't know how this patch was tested, but for me it breaks the
layout of the ipu3_uapi_acc_param struct. The result is that awb_fr is
moved from offset 37656 to 36768 and so everything else is shifted
too. The end result is "set parameters failed" when the userspace
tries to queue a parameter buffer.
To be honest, I don't like how alignments are used to define the ABI
layout. Since these are definitions specified by the IMGU firmware,
I'd suggest removing any compiler alignments, making all the structs
packed and adding explicit padding in terms of u8 arrays, which is the
proper way to enforce a specific binary layout.
Following is the relevant snippet from a diff of pahole output before
and after this patch:
--- pahole.good 2020-04-11 16:40:18.706133867 +0000
+++ pahole.bad 2020-04-11 16:29:48.513110117 +0000
@@ -7353,29 +7349,27 @@ // struct ipu3_uapi_acc_param {
struct ipu3_uapi_anr_config anr; /* 36020 736 */
/* XXX last struct has 24 bytes of padding */
+ /* XXX 12 bytes hole, try to pack */
- /* --- cacheline 574 boundary (36736 bytes) was 20 bytes ago --- */
- struct ipu3_uapi_awb_fr_config_s awb_fr
__attribute__((__aligned__(32))); /* 36756 32 */
- struct ipu3_uapi_ae_config ae; /* 36788 480 */
+ /* --- cacheline 574 boundary (36736 bytes) was 32 bytes ago --- */
+ struct ipu3_uapi_awb_fr_config_s awb_fr
__attribute__((__aligned__(32))); /* 36768 32 */
+ /* --- cacheline 575 boundary (36800 bytes) --- */
+ struct ipu3_uapi_ae_config ae; /* 36800 480 */
/* XXX last struct has 24 bytes of padding */
- /* --- cacheline 582 boundary (37248 bytes) was 20 bytes ago --- */
- struct ipu3_uapi_af_config_s af; /* 37268 96 */
+ /* --- cacheline 582 boundary (37248 bytes) was 32 bytes ago --- */
+ struct ipu3_uapi_af_config_s af; /* 37280 96 */
/* XXX last struct has 20 bytes of padding */
- /* --- cacheline 583 boundary (37312 bytes) was 52 bytes ago --- */
- struct ipu3_uapi_awb_config awb; /* 37364 32 */
-
- /* Force padding: */
- struct ipu3_uapi_awb_config :256;
+ /* --- cacheline 584 boundary (37376 bytes) --- */
+ struct ipu3_uapi_awb_config awb; /* 37376 32 */
/* size: 37408, cachelines: 585, members: 21 */
- /* sum members: 37152, holes: 13, sum holes: 244 */
- /* padding: 12 */
+ /* sum members: 37152, holes: 14, sum holes: 256 */
/* paddings: 3, sum paddings: 68 */
- /* forced alignments: 16, forced holes: 13, sum forced holes: 244 */
+ /* forced alignments: 16, forced holes: 14, sum forced holes: 256 */
/* last cacheline: 32 bytes */
};
Best regards,
Tomasz
