On 6/25/24 14:11, Jacopo Mondi wrote: > 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: As far I know the 8 byte alignment is only relevant if you need to read 8-byte values (u64 or pointers): those should be 8 byte aligned. Which is why the compiler will add padding for a struct like this on a 64 bit architecture: struct foo { u32 f1; u64 f2; }; After f1 there is a 4 byte hole. But struct pisp_be_config doesn't contain any u64 or pointers, so there is no need for aligning to 8 bytes. Regards, Hans > > 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_ */ >>>> >> >
