Hi again On Fri, Jun 28, 2024 at 09:09:10AM GMT, Jacopo Mondi wrote: > Hi Sakari > > On Thu, Jun 27, 2024 at 04:59:35PM GMT, Sakari Ailus wrote: > > Hi Jacopo, > > > > On Thu, Jun 27, 2024 at 04:30:57PM +0200, Jacopo Mondi wrote: > > > Add definition and test for 32-bits image formats to the pisp_common.h > > > uAPI header. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> > > > > > > --- > > > RPi: I found not traces of this in your BSP pisp_types.h header but > > > these identifiers are used by libpisp and are part of the pisp_types.h > > > header shipped with the library. > > > > > > https://github.com/raspberrypi/libpisp/blob/main/src/libpisp/backend/backend_prepare.cpp#L374 > > > https://github.com/raspberrypi/libpisp/blob/main/src/libpisp/common/pisp_types.h#L137 > > > > > > in mainline, pisp_types.h has been absorbed by pisp_common.h so I'm > > > adding it here (only PISP_IMAGE_FORMAT_bpp_32, as it's the only one used > > > by libpisp). > > > > > > Could you ack/nack this patch please ? > > > --- > > > --- > > > include/uapi/linux/media/raspberrypi/pisp_common.h | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/include/uapi/linux/media/raspberrypi/pisp_common.h b/include/uapi/linux/media/raspberrypi/pisp_common.h > > > index b2522e29c976..031fdaa4da69 100644 > > > --- a/include/uapi/linux/media/raspberrypi/pisp_common.h > > > +++ b/include/uapi/linux/media/raspberrypi/pisp_common.h > > > @@ -72,6 +72,8 @@ enum pisp_image_format { > > > PISP_IMAGE_FORMAT_SHIFT_8 = 0x00080000, > > > PISP_IMAGE_FORMAT_SHIFT_MASK = 0x000f0000, > > > > > > + PISP_IMAGE_FORMAT_BPP_32 = 0x00100000, > > > + > > > PISP_IMAGE_FORMAT_UNCOMPRESSED = 0x00000000, > > > PISP_IMAGE_FORMAT_COMPRESSION_MODE_1 = 0x01000000, > > > PISP_IMAGE_FORMAT_COMPRESSION_MODE_2 = 0x02000000, > > > @@ -134,6 +136,7 @@ enum pisp_image_format { > > > PISP_IMAGE_FORMAT_PLANARITY_PLANAR) > > > #define PISP_IMAGE_FORMAT_wallpaper(fmt) \ > > > ((fmt) & PISP_IMAGE_FORMAT_WALLPAPER_ROLL) > > > +#define PISP_IMAGE_FORMAT_bpp_32(fmt) ((fmt) & PISP_IMAGE_FORMAT_BPP_32) > > > > Why lower case "bpp"? > > No idea, I don't like it but the existing > > PISP_IMAGE_FORMAT_bps_8 > PISP_IMAGE_FORMAT_bps_10 > PISP_IMAGE_FORMAT_bps_12 > PISP_IMAGE_FORMAT_bps_16 > PISP_IMAGE_FORMAT_three_channel > PISP_IMAGE_FORMAT_single_channel > > etc etc > > are all lowecase. > > Also it is not clear to me why this is _bpp_ the other _bps_. Ah, https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf at page 10 provides a description. The _bps_ identify the bits-per-sample while the _bpp_32 describe RGB formats where a single pixel is packed in 32 bits. As the datasheet uses _bps_ and _bpp_ I would keep using them here. Also, I can use uppercase for all macros here if preferred (I'll send a patch for libpisp in case) > > If I had to upstream this code from scratch I would use uppercase. > > > > > As this isn't a definition of a value as such, I'd call it differently, > > e.g. PISP_IMAGE_FORMAT_BPP_IS_32. > > > > > #define PISP_IMAGE_FORMAT_HOG(fmt) \ > > > ((fmt) & \ > > > (PISP_IMAGE_FORMAT_HOG_SIGNED | PISP_IMAGE_FORMAT_HOG_UNSIGNED)) > > > > -- > > Regards, > > > > Sakari Ailus
