[RFC] drm/fourcc: Add RPI modifiers

[Laurent Pinchart]

On Mon, Feb 26, 2024 at 05:24:41PM +0100, Jacopo Mondi wrote:
> On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote:
> > On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi wrote:
> > >
> > > Add modifiers for the Raspberry Pi PiSP compressed formats.
> > >
> > > The compressed formats are documented at:
> > > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst
> > >
> > > and in the PiSP datasheet:
> > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > ---
> > >
> > > Background:
> > > -----------
> > >
> > > The Raspberry Pi PiSP camera subsystem is on its way to upstream through the
> > > Video4Linux2 subsystem:
> > > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310
> > >
> > > The PiSP camera system is composed by a "Front End" and a "Back End".
> > > The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory and
> > > produce statistics, and the BackEnd is a memory-to-memory ISP that converts
> > > images in a format usable by application.
> > >
> > > The "FrontEnd" is capable of encoding RAW Bayer images as received by the
> > > image sensor in a 'compressed' format defined by Raspberry Pi and fully
> > > documented in the PiSP manual:
> > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > >
> > > The compression scheme is documented in the in-review patch series for the BE
> > > support at:
> > > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/
> > >
> > > The "BackEnd" is capable of consuming images in the compressed format and
> > > optionally user application might want to inspect those images for debugging
> > > purposes.
> > >
> > > Why a DRM modifier
> > > ------------------
> > >
> > > The PiSP support is entirely implemented in libcamera, with the support of an
> > > hw-specific library called 'libpisp'.
> > >
> > > libcamera uses the fourcc codes defined by DRM to define its formats:
> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml
> > >
> > > And to define a new libcamera format for the Raspberry Pi compressed ones we
> > > need to associate the above proposed modifiers with a RAW Bayer format
> > > identifier.
> > >
> > > In example:
> > >
> > >   - RGGB16_PISP_COMP1:
> > >       fourcc: DRM_FORMAT_SRGGB16
> > >       mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > >   - GRBG16_PISP_COMP1:
> > >       fourcc: DRM_FORMAT_SGRBG16
> > >       mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > >   - GBRG16_PISP_COMP1:
> > >       fourcc: DRM_FORMAT_SGBRG16
> > >       mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > >   - BGGR16_PISP_COMP1:
> > >       fourcc: DRM_FORMAT_SBGGR16
> > >       mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > >   - MONO_PISP_COMP1:
> > >       fourcc: DRM_FORMAT_R16
> > >       mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > >
> > > See
> > > https://patchwork.libcamera.org/patch/19503/
> > >
> > > Would if be acceptable for DRM to include the above proposed modifiers for the
> > > purpose of defining the above presented libcamera formats ? There will be no
> > > graphic format associated with these modifiers as their purpose it not
> > > displaying images but rather exchange them between the components of the
> > > camera subsystem (and possibly be inspected by specialized test applications).
> >
> > Yeah I think libcamera using drm-fourcc formats and modifiers is
> > absolutely ok, and has my ack in principle. And for these users we're
> > ok with merging modifiers that the kernel doesn't use.
> 
> Great!
> 
> > I think it would be really good to formalize this by adding libcamera
> > to the officially listed users in the "Open Source User Waiver"
> > section in the drm_fourcc.h docs:
> >
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#open-source-user-waiver
> >
> > You might want to convert that into a list, it could get a bit
> > confusing. Then we can get that patch properly acked (by kernel and
> > libcamera folks) to record the community consensus.
> 
> I see your point, but I wonder if libcamera actually need to be part
> of this list; we want in-kernel upstream driver and open-source

For the kernel side we'll use V4L2, so the DRM modifier won't have an
in-kernel user.

On the userspace side, yes, there will be an open-source user with
libcamera.

> userspace components. We allow binary 3A modules to be loaded by the
> library, but I'm not sure they will ever need to modify the DRM 4cc list.
> 
> Anyway, with other libcamera people ack, I can certainly do so!
> 
> > For the rpi modifiers themselves: They need to be properly documented,
> > least to exclude a screw-up like with the rpi modifiers we already
> > have, which unfortunately encode the buffer height (instead of just
> > the rounding algorithim to align the height to the right tile size) in
> > the modifiers, which breaks assumptions everywhere. For details see
> > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057
> 
> I see. The formats are fully documented in the above linked datasheet,
> and I've provided (with the help of RPi engineers, as I don't
> understand the compression algorithm part :) a shorter description as
> part of the V4L2 patch series that upstreams the BE driver
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/
> 
> The only indication about the buffer's layout I see is
> 
>   Each scanline is padded to a multiple of 8 pixels wide, and each block of 8
>   horizontally-contiguous pixels is coded using 8 bytes.
> 
> While the rest of the text describes the compression algorithm which
> (afaiu) doesn't affect the buffer geometry.
> 
> Would you be ok with me replicating the above description (or maybe
> just reference the V4L2 documentation ?)
> 
> > > ---
> > >  include/uapi/drm/drm_fourcc.h | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > index 00db00083175..09b182a959ad 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -425,6 +425,7 @@ extern "C" {
> > >  #define DRM_FORMAT_MOD_VENDOR_ARM     0x08
> > >  #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
> > >  #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
> > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b
> > >
> > >  /* add more to the end as needed */
> > >
> > > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
> > >  #define AMD_FMT_MOD_CLEAR(field) \
> > >         (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))
> > >
> > > +/* RPI (Raspberry Pi) modifiers */
> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)
> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)
> > > +
> > >  #if defined(__cplusplus)
> > >  }
> > >  #endif

-- 
Regards,

Laurent Pinchart