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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux