| Message ID | 20240627143059.300796-3-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uapi: pisp_be: Two late fixes | expand |
Hi Jacopo Yes, that looks right. The 32bpp format was a slightly later addition, and so might have slipped through the net previously. On Thu, 27 Jun 2024 at 15:31, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Add definition and test for 32-bits image formats to the pisp_common.h > uAPI header. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Acked-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > > --- > 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) > #define PISP_IMAGE_FORMAT_HOG(fmt) \ > ((fmt) & \ > (PISP_IMAGE_FORMAT_HOG_SIGNED | PISP_IMAGE_FORMAT_HOG_UNSIGNED)) > -- > 2.45.2 >
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@ideasonboard.com> > > --- > 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"? 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))
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@ideasonboard.com> > > > > --- > > 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
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@ideasonboard.com> > > > > > > --- > > > 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
On Fri, Jun 28, 2024 at 09:09:10AM +0200, 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@ideasonboard.com> > > > > > > --- > > > 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. Ack, fine for me then. It's a driver specific header anyway and the all-important somehow reasonable prefix is there so ok.
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) #define PISP_IMAGE_FORMAT_HOG(fmt) \ ((fmt) & \ (PISP_IMAGE_FORMAT_HOG_SIGNED | PISP_IMAGE_FORMAT_HOG_UNSIGNED))
Add definition and test for 32-bits image formats to the pisp_common.h uAPI header. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- 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(+) -- 2.45.2