Hi Sima On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote: > On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi > <jacopo.mondi@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> > > --- > > > > 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@xxxxxxxxxxxxxxxx/ > > > > 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 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@xxxxxxxxxxxxxxxx/ 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 ?) > > Cheers, Sima > > > > > --- > > 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 > > -- > > 2.43.0 > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
