[2/2] media: uapi: pisp_common: Add 32 bpp format test

Message ID 20240627143059.300796-3-jacopo.mondi@ideasonboard.com (mailing list archive)
State Superseded
Headers
Series media: uapi: pisp_be: Two late fixes |

Commit Message

Jacopo Mondi June 27, 2024, 2:30 p.m. UTC
  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
  

Comments

David Plowman June 27, 2024, 2:43 p.m. UTC | #1
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
>
  
Sakari Ailus June 27, 2024, 4:59 p.m. UTC | #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))
  
Jacopo Mondi June 28, 2024, 7:09 a.m. UTC | #3
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
  
Jacopo Mondi June 28, 2024, 7:28 a.m. UTC | #4
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
  
Sakari Ailus June 28, 2024, 11:47 a.m. UTC | #5
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.
  

Patch

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))