Hi Tomasz,
On Sat, Apr 11, 2020 at 06:50:55PM +0200, Tomasz Figa wrote:
> 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.
Thanks for reporting this.
Unlike recent patches touching the struct, I didn't test (obviously) this
one with pahole.
>
> 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.
The current use of __aligned(32) with __packed especially makes this
cumbersome and risky. __packed alone is necessary.
I'll send patches to address the alignment issue; converting to use
reserved fields would be quite a bit more work.
Similar changes should be done to ipu3-abi.h as well, but it appears to
have been less problematic so far.
>
> 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 */
> };
>
--
Regards,
Sakari Ailus
