On 6/25/24 13:15, Jacopo Mondi wrote: > Hello again, > > On Mon, Jun 24, 2024 at 03:52:41PM GMT, Hans Verkuil wrote: >> Hi Jacopo, >> >> On 31/05/2024 10:07, Jacopo Mondi wrote: >>> Add the Raspberry Pi PiSP Back End uAPI header. >>> >>> The header defines the data type used to configure the PiSP Back End >>> ISP. >>> >>> The detailed description of the types and of the ISP configuration >>> procedure is available at >>> https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf >>> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> >>> --- >>> MAINTAINERS | 7 + >>> .../linux/media/raspberrypi/pisp_be_config.h | 927 ++++++++++++++++++ >>> .../linux/media/raspberrypi/pisp_common.h | 199 ++++ >>> 3 files changed, 1133 insertions(+) >>> create mode 100644 include/uapi/linux/media/raspberrypi/pisp_be_config.h >>> create mode 100644 include/uapi/linux/media/raspberrypi/pisp_common.h >>> >> >> <snip> >> >>> diff --git a/include/uapi/linux/media/raspberrypi/pisp_be_config.h b/include/uapi/linux/media/raspberrypi/pisp_be_config.h >>> new file mode 100644 >>> index 000000000000..3eb4be03c5f4 >>> --- /dev/null >>> +++ b/include/uapi/linux/media/raspberrypi/pisp_be_config.h >>> @@ -0,0 +1,927 @@ >> >> <snip> >> >>> +/** >>> + * struct pisp_be_tiles_config - Raspberry Pi PiSP Back End configuration >>> + * @tiles: Tile descriptors >>> + * @num_tiles: Number of tiles >>> + * @config: PiSP Back End configuration >>> + */ >>> +struct pisp_be_tiles_config { >>> + struct pisp_tile tiles[PISP_BACK_END_NUM_TILES]; >>> + int num_tiles; >> >> Everything else is nicely __u8/16/32, and then there is suddenly an 'int' >> where I would expect to see a __u32. >> >> I think a v10 is needed anyway (see next review), so it would be nice to >> pick up this change for v10. > > Sure I can change it. > > While at it I passed ' struct pisp_be_tiles_config' through pahole. > > struct pisp_be_tiles_config { > struct pisp_tile tiles[64]; /* 0 10240 */ > /* --- cacheline 160 boundary (10240 bytes) --- */ > __u32 num_tiles; /* 10240 4 */ > struct pisp_be_config config; /* 10244 6276 */ > > /* size: 16520, cachelines: 259, members: 3 */ > /* last cacheline: 8 bytes */ > }; > > if 'config' gets accessed by pointer on aarch64 it will result in an > unaligned access ? Where do you see that? AFAICT it is perfectly fine to have a struct pisp_be_config pointer set to &foo.config. Regards, Hans > > Do we need to insert a 32 bits padding between 'num_tiles' and > 'config' ? > >> >> Regards, >> >> Hans >> >>> + struct pisp_be_config config; >>> +} __attribute__((packed)); >>> + >>> +#endif /* _UAPI_PISP_BE_CONFIG_H_ */ >>
