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_. 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
