Hi Hans On Tue, Jun 25, 2024 at 01:56:46PM GMT, Hans Verkuil wrote: > 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. 'config' is at 10244 bytes from the struct beginning. If accessed as u64 this is not 8-bytes aligned (which afaik is legit but more expensive on aarch64). But as the driver accesses it as 32-bits integers: unsigned int begin, end; begin = offsetof(struct pisp_be_config, global.bayer_order) / sizeof(u32); end = sizeof(struct pisp_be_config) / sizeof(u32); for (unsigned int u = begin; u < end; u++) pispbe_wr(pispbe, PISP_BE_CONFIG_BASE_REG + sizeof(u32) * u, ((u32 *)job->config)[u]); this should be fine yes > > 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_ */ > >> >
