[{"id":28740,"web_url":"https://patchwork.libcamera.org/comment/28740/","msgid":"","date":"2024-02-26T15:46:24","subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","submitter":{"id":187,"url":"https://patchwork.libcamera.org/api/people/187/","name":"Daniel Vetter","email":"daniel@ffwll.ch"},"content":"On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi\n wrote:\n>\n> Add modifiers for the Raspberry Pi PiSP compressed formats.\n>\n> The compressed formats are documented at:\n> Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst\n>\n> and in the PiSP datasheet:\n> https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n>\n> Background:\n> -----------\n>\n> The Raspberry Pi PiSP camera subsystem is on its way to upstream through the\n> Video4Linux2 subsystem:\n> https://patchwork.linuxtv.org/project/linux-media/list/?series=12310\n>\n> The PiSP camera system is composed by a \"Front End\" and a \"Back End\".\n> The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory and\n> produce statistics, and the BackEnd is a memory-to-memory ISP that converts\n> images in a format usable by application.\n>\n> The \"FrontEnd\" is capable of encoding RAW Bayer images as received by the\n> image sensor in a 'compressed' format defined by Raspberry Pi and fully\n> documented in the PiSP manual:\n> https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n>\n> The compression scheme is documented in the in-review patch series for the BE\n> support at:\n> https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n>\n> The \"BackEnd\" is capable of consuming images in the compressed format and\n> optionally user application might want to inspect those images for debugging\n> purposes.\n>\n> Why a DRM modifier\n> ------------------\n>\n> The PiSP support is entirely implemented in libcamera, with the support of an\n> hw-specific library called 'libpisp'.\n>\n> libcamera uses the fourcc codes defined by DRM to define its formats:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml\n>\n> And to define a new libcamera format for the Raspberry Pi compressed ones we\n> need to associate the above proposed modifiers with a RAW Bayer format\n> identifier.\n>\n> In example:\n>\n> - RGGB16_PISP_COMP1:\n> fourcc: DRM_FORMAT_SRGGB16\n> mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> - GRBG16_PISP_COMP1:\n> fourcc: DRM_FORMAT_SGRBG16\n> mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> - GBRG16_PISP_COMP1:\n> fourcc: DRM_FORMAT_SGBRG16\n> mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> - BGGR16_PISP_COMP1:\n> fourcc: DRM_FORMAT_SBGGR16\n> mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> - MONO_PISP_COMP1:\n> fourcc: DRM_FORMAT_R16\n> mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n>\n> See\n> https://patchwork.libcamera.org/patch/19503/\n>\n> Would if be acceptable for DRM to include the above proposed modifiers for the\n> purpose of defining the above presented libcamera formats ? There will be no\n> graphic format associated with these modifiers as their purpose it not\n> displaying images but rather exchange them between the components of the\n> camera subsystem (and possibly be inspected by specialized test applications).\n\nYeah I think libcamera using drm-fourcc formats and modifiers is\nabsolutely ok, and has my ack in principle. And for these users we're\nok with merging modifiers that the kernel doesn't use.\n\nI think it would be really good to formalize this by adding libcamera\nto the officially listed users in the \"Open Source User Waiver\"\nsection in the drm_fourcc.h docs:\n\nhttps://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#open-source-user-waiver\n\nYou might want to convert that into a list, it could get a bit\nconfusing. Then we can get that patch properly acked (by kernel and\nlibcamera folks) to record the community consensus.\n\nFor the rpi modifiers themselves: They need to be properly documented,\nleast to exclude a screw-up like with the rpi modifiers we already\nhave, which unfortunately encode the buffer height (instead of just\nthe rounding algorithim to align the height to the right tile size) in\nthe modifiers, which breaks assumptions everywhere. For details see\nhttps://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057\n\nCheers, Sima\n\n>\n> ---\n> include/uapi/drm/drm_fourcc.h | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h\n> index 00db00083175..09b182a959ad 100644\n> --- a/include/uapi/drm/drm_fourcc.h\n> +++ b/include/uapi/drm/drm_fourcc.h\n> @@ -425,6 +425,7 @@ extern \"C\" {\n> #define DRM_FORMAT_MOD_VENDOR_ARM 0x08\n> #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09\n> #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a\n> +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b\n>\n> /* add more to the end as needed */\n>\n> @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)\n> #define AMD_FMT_MOD_CLEAR(field) \\\n> (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))\n>\n> +/* RPI (Raspberry Pi) modifiers */\n> +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)\n> +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)\n> +\n> #if defined(__cplusplus)\n> }\n> #endif\n> --\n> 2.43.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1EF11BD160\n\tfor ;\n\tMon, 26 Feb 2024 15:50:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 309666285F;\n\tMon, 26 Feb 2024 16:50:00 +0100 (CET)","from mail-oa1-x2a.google.com (mail-oa1-x2a.google.com\n\t[IPv6:2001:4860:4864:20::2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CBEC161C98\n\tfor ;\n\tMon, 26 Feb 2024 16:46:36 +0100 (CET)","by mail-oa1-x2a.google.com with SMTP id\n\t586e51a60fabf-21f10aae252so367641fac.0\n\tfor ;\n\tMon, 26 Feb 2024 07:46:36 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ffwll.ch header.i=@ffwll.ch header.b=\"T3wcSZ4C\";\n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ffwll.ch; s=google; t=1708962395; x=1709567195;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=unAClle+u941FHaZg01gKmCQSRKv2wx+bkTeMlPTsH4=;\n\tb=T3wcSZ4CqZ9q8kclg3bFrN/6woTzwzmQxifM0xVgaOYkatJhO4uIlAS17xFQyvUKog\n\tFwk8p3LVQdwWvm6MVVTGso936VtrPqVzcxikop6tJInqvxzAhQFiXhvhajguyqq8n2DC\n\tTHxjZ9E1KacZQwS2LP6UM1iyDF+hh9vLw8B4g=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1708962395; x=1709567195;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=unAClle+u941FHaZg01gKmCQSRKv2wx+bkTeMlPTsH4=;\n\tb=VB9cgKszxqhEyLSgHC5p1nlSMvtPPnrEaWxgjIIiBf2FnatvvEo9W/d2rOQCAqTlOi\n\tMQkfxv/JTR2dp/OxmCzMYYMMoplnp6JYmEvNcKZNWxEnQ70G7l/yaWV+ekitZOvUDK0q\n\tnucp8VpMW1wdViAh7GsH02yrQDiENIvTmraxLjBpyxTb8ATDm+z05EJblhmzRfuzDErZ\n\tx35CS+An2llUR2KfU39fv0H7UXXi2hvKTVwO+eQ1rcxhLnhhk1B4sq3womJmX+Wk/+g3\n\tkbF1G8opyShTsRtLf689J8TRjs3j+B0sUx21nMYnTHagMV386KDZrnO0dk5XevYyiO+a\n\tJL9A==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCU+QbiDRMj3mBwOQeYOFJZDnnCa4rLResXcvxbXZKXGQ9xIldZoc+YDAI9Sl7zxJJOfrMvnPsP9yrkgnxXAsLm3B6GMBGRtWiDd6hZ6x6FKyj48UA==","X-Gm-Message-State":"AOJu0YwC5i/BxlBhbN5rRmdWbJEBDUj4QkcqrVWuw/pNaKXZOuhG3175\n\tT8Ck8J2ZF0nm08QZOulf4Ub4cprffu6jtYVaEPQIoZATld8PDow7EePdW2WUe5cE39EgeVLNGcT\n\twbmIFxqP9g/xsp1IJtbzH3QsdUW9/STQPcujkdw==","X-Google-Smtp-Source":"AGHT+IExO/jK+BYNdSXCJ92BRHeixfVc9MgUCXfmYlTkBRUqn0eGZFua/I3/VuTDS8dSpblfkJjSETcZ0f91dDwn7W4=","X-Received":"by 2002:a05:6870:6b9b:b0:21f:9c9c:b399 with SMTP id\n\tms27-20020a0568706b9b00b0021f9c9cb399mr8493265oab.1.1708962395519;\n\tMon, 26 Feb 2024 07:46:35 -0800 (PST)","MIME-Version":"1.0","References":"<20240226153854.99471-1-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20240226153854.99471-1-jacopo.mondi@ideasonboard.com>","From":"Daniel Vetter ","Date":"Mon, 26 Feb 2024 16:46:24 +0100","Message-ID":"","Subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailman-Approved-At":"Mon, 26 Feb 2024 16:49:58 +0100","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"dri-devel@lists.freedesktop.org,\n\tMaarten Lankhorst ,\n\tMaxime Ripard , libcamera-devel@lists.libcamera.org, \n\tThomas Zimmermann , David Airlie ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28741,"web_url":"https://patchwork.libcamera.org/comment/28741/","msgid":"<473ei7jyjevynhb7roinhdaj2hnmsog6owiognlyp5fpfc63pa@mdn73c5gu46r>","date":"2024-02-26T16:24:41","subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Sima\n\nOn Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote:\n> On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi\n> wrote:\n> >\n> > Add modifiers for the Raspberry Pi PiSP compressed formats.\n> >\n> > The compressed formats are documented at:\n> > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst\n> >\n> > and in the PiSP datasheet:\n> > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> >\n> > Background:\n> > -----------\n> >\n> > The Raspberry Pi PiSP camera subsystem is on its way to upstream through the\n> > Video4Linux2 subsystem:\n> > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310\n> >\n> > The PiSP camera system is composed by a \"Front End\" and a \"Back End\".\n> > The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory and\n> > produce statistics, and the BackEnd is a memory-to-memory ISP that converts\n> > images in a format usable by application.\n> >\n> > The \"FrontEnd\" is capable of encoding RAW Bayer images as received by the\n> > image sensor in a 'compressed' format defined by Raspberry Pi and fully\n> > documented in the PiSP manual:\n> > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> >\n> > The compression scheme is documented in the in-review patch series for the BE\n> > support at:\n> > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n> >\n> > The \"BackEnd\" is capable of consuming images in the compressed format and\n> > optionally user application might want to inspect those images for debugging\n> > purposes.\n> >\n> > Why a DRM modifier\n> > ------------------\n> >\n> > The PiSP support is entirely implemented in libcamera, with the support of an\n> > hw-specific library called 'libpisp'.\n> >\n> > libcamera uses the fourcc codes defined by DRM to define its formats:\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml\n> >\n> > And to define a new libcamera format for the Raspberry Pi compressed ones we\n> > need to associate the above proposed modifiers with a RAW Bayer format\n> > identifier.\n> >\n> > In example:\n> >\n> > - RGGB16_PISP_COMP1:\n> > fourcc: DRM_FORMAT_SRGGB16\n> > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > - GRBG16_PISP_COMP1:\n> > fourcc: DRM_FORMAT_SGRBG16\n> > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > - GBRG16_PISP_COMP1:\n> > fourcc: DRM_FORMAT_SGBRG16\n> > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > - BGGR16_PISP_COMP1:\n> > fourcc: DRM_FORMAT_SBGGR16\n> > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > - MONO_PISP_COMP1:\n> > fourcc: DRM_FORMAT_R16\n> > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> >\n> > See\n> > https://patchwork.libcamera.org/patch/19503/\n> >\n> > Would if be acceptable for DRM to include the above proposed modifiers for the\n> > purpose of defining the above presented libcamera formats ? There will be no\n> > graphic format associated with these modifiers as their purpose it not\n> > displaying images but rather exchange them between the components of the\n> > camera subsystem (and possibly be inspected by specialized test applications).\n>\n> Yeah I think libcamera using drm-fourcc formats and modifiers is\n> absolutely ok, and has my ack in principle. And for these users we're\n> ok with merging modifiers that the kernel doesn't use.\n>\n\nGreat!\n\n> I think it would be really good to formalize this by adding libcamera\n> to the officially listed users in the \"Open Source User Waiver\"\n> section in the drm_fourcc.h docs:\n>\n> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#open-source-user-waiver\n>\n> You might want to convert that into a list, it could get a bit\n> confusing. Then we can get that patch properly acked (by kernel and\n> libcamera folks) to record the community consensus.\n>\n\nI see your point, but I wonder if libcamera actually need to be part\nof this list; we want in-kernel upstream driver and open-source\nuserspace components. We allow binary 3A modules to be loaded by the\nlibrary, but I'm not sure they will ever need to modify the DRM 4cc list.\n\nAnyway, with other libcamera people ack, I can certainly do so!\n\n> For the rpi modifiers themselves: They need to be properly documented,\n> least to exclude a screw-up like with the rpi modifiers we already\n> have, which unfortunately encode the buffer height (instead of just\n> the rounding algorithim to align the height to the right tile size) in\n> the modifiers, which breaks assumptions everywhere. For details see\n> https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057\n\nI see. The formats are fully documented in the above linked datasheet,\nand I've provided (with the help of RPi engineers, as I don't\nunderstand the compression algorithm part :) a shorter description as\npart of the V4L2 patch series that upstreams the BE driver\n\nhttps://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n\nThe only indication about the buffer's layout I see is\n\n Each scanline is padded to a multiple of 8 pixels wide, and each block of 8\n horizontally-contiguous pixels is coded using 8 bytes.\n\nWhile the rest of the text describes the compression algorithm which\n(afaiu) doesn't affect the buffer geometry.\n\nWould you be ok with me replicating the above description (or maybe\njust reference the V4L2 documentation ?)\n\n>\n> Cheers, Sima\n>\n> >\n> > ---\n> > include/uapi/drm/drm_fourcc.h | 5 +++++\n> > 1 file changed, 5 insertions(+)\n> >\n> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h\n> > index 00db00083175..09b182a959ad 100644\n> > --- a/include/uapi/drm/drm_fourcc.h\n> > +++ b/include/uapi/drm/drm_fourcc.h\n> > @@ -425,6 +425,7 @@ extern \"C\" {\n> > #define DRM_FORMAT_MOD_VENDOR_ARM 0x08\n> > #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09\n> > #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a\n> > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b\n> >\n> > /* add more to the end as needed */\n> >\n> > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)\n> > #define AMD_FMT_MOD_CLEAR(field) \\\n> > (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))\n> >\n> > +/* RPI (Raspberry Pi) modifiers */\n> > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)\n> > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)\n> > +\n> > #if defined(__cplusplus)\n> > }\n> > #endif\n> > --\n> > 2.43.0\n> >\n>\n>\n> --\n> Daniel Vetter\n> Software Engineer, Intel Corporation\n> http://blog.ffwll.ch","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 69A91BD80A\n\tfor ;\n\tMon, 26 Feb 2024 16:24:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95DBB61C9A;\n\tMon, 26 Feb 2024 17:24:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 09D1461C98\n\tfor ;\n\tMon, 26 Feb 2024 17:24:45 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2EC21CD1;\n\tMon, 26 Feb 2024 17:24:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eTxF+fcV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1708964673;\n\tbh=ugSvBfochfUcf2Jzb9upyp/8f12Rq3WglkeSDD3A+Tg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eTxF+fcVNR/zBTlCqFeMNF0lP1NuO242040KBzpm2YsW6uy/N1boOB/MeWZtSh0vD\n\tpOO+fUf3Ty2RswWwjNw6QrqUO/cAA8q91OuwjfitaJ/hoJE3L9gG05RuDt0U1Hgm8H\n\t/d/2/sNDg5FRzE1MoFGWS64XHeLtb8ntYqnspZwE=","Date":"Mon, 26 Feb 2024 17:24:41 +0100","From":"Jacopo Mondi ","To":"Daniel Vetter ","Subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","Message-ID":"<473ei7jyjevynhb7roinhdaj2hnmsog6owiognlyp5fpfc63pa@mdn73c5gu46r>","References":"<20240226153854.99471-1-jacopo.mondi@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maarten Lankhorst ,\n\tMaxime Ripard , libcamera-devel@lists.libcamera.org, \n\tdri-devel@lists.freedesktop.org,\n\tThomas Zimmermann , \n\tJacopo Mondi ,\n\tDavid Airlie ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28753,"web_url":"https://patchwork.libcamera.org/comment/28753/","msgid":"<20240227130827.GA5863@pendragon.ideasonboard.com>","date":"2024-02-27T13:08:27","subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote:\n> On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi wrote:\n> >\n> > Add modifiers for the Raspberry Pi PiSP compressed formats.\n> >\n> > The compressed formats are documented at:\n> > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst\n> >\n> > and in the PiSP datasheet:\n> > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> >\n> > Background:\n> > -----------\n> >\n> > The Raspberry Pi PiSP camera subsystem is on its way to upstream through the\n> > Video4Linux2 subsystem:\n> > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310\n> >\n> > The PiSP camera system is composed by a \"Front End\" and a \"Back End\".\n> > The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory and\n> > produce statistics, and the BackEnd is a memory-to-memory ISP that converts\n> > images in a format usable by application.\n> >\n> > The \"FrontEnd\" is capable of encoding RAW Bayer images as received by the\n> > image sensor in a 'compressed' format defined by Raspberry Pi and fully\n> > documented in the PiSP manual:\n> > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> >\n> > The compression scheme is documented in the in-review patch series for the BE\n> > support at:\n> > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n> >\n> > The \"BackEnd\" is capable of consuming images in the compressed format and\n> > optionally user application might want to inspect those images for debugging\n> > purposes.\n> >\n> > Why a DRM modifier\n> > ------------------\n> >\n> > The PiSP support is entirely implemented in libcamera, with the support of an\n> > hw-specific library called 'libpisp'.\n> >\n> > libcamera uses the fourcc codes defined by DRM to define its formats:\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml\n> >\n> > And to define a new libcamera format for the Raspberry Pi compressed ones we\n> > need to associate the above proposed modifiers with a RAW Bayer format\n> > identifier.\n> >\n> > In example:\n> >\n> > - RGGB16_PISP_COMP1:\n> > fourcc: DRM_FORMAT_SRGGB16\n\nAn \"interesting\" issue here is that these formats currently live in\nlibcamera only, we haven't merged them in DRM \"yet\". This may be a\nprerequisite ?\n\n> > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > - GRBG16_PISP_COMP1:\n> > fourcc: DRM_FORMAT_SGRBG16\n> > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > - GBRG16_PISP_COMP1:\n> > fourcc: DRM_FORMAT_SGBRG16\n> > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > - BGGR16_PISP_COMP1:\n> > fourcc: DRM_FORMAT_SBGGR16\n> > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > - MONO_PISP_COMP1:\n> > fourcc: DRM_FORMAT_R16\n> > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> >\n> > See\n> > https://patchwork.libcamera.org/patch/19503/\n> >\n> > Would if be acceptable for DRM to include the above proposed modifiers for the\n> > purpose of defining the above presented libcamera formats ? There will be no\n> > graphic format associated with these modifiers as their purpose it not\n> > displaying images but rather exchange them between the components of the\n> > camera subsystem (and possibly be inspected by specialized test applications).\n> \n> Yeah I think libcamera using drm-fourcc formats and modifiers is\n> absolutely ok, and has my ack in principle. And for these users we're\n> ok with merging modifiers that the kernel doesn't use.\n> \n> I think it would be really good to formalize this by adding libcamera\n> to the officially listed users in the \"Open Source User Waiver\"\n> section in the drm_fourcc.h docs:\n> \n> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#open-source-user-waiver\n\nThe waiver states \"the usual requirement for an upstream in-kernel or\nopen source userspace user does not apply\". Strictly speaking, there is\nan open-source userspace user with libcamera. It's only on the kernel\nside that the new modifier my not get used.\n\nThis being said, I'm fine clarifying the documentation, clarity is\nalways good.\n\n> You might want to convert that into a list, it could get a bit\n> confusing. Then we can get that patch properly acked (by kernel and\n> libcamera folks) to record the community consensus.\n> \n> For the rpi modifiers themselves: They need to be properly documented,\n> least to exclude a screw-up like with the rpi modifiers we already\n> have, which unfortunately encode the buffer height (instead of just\n> the rounding algorithim to align the height to the right tile size) in\n> the modifiers, which breaks assumptions everywhere. For details see\n> https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057\n> \n> > ---\n> > include/uapi/drm/drm_fourcc.h | 5 +++++\n> > 1 file changed, 5 insertions(+)\n> >\n> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h\n> > index 00db00083175..09b182a959ad 100644\n> > --- a/include/uapi/drm/drm_fourcc.h\n> > +++ b/include/uapi/drm/drm_fourcc.h\n> > @@ -425,6 +425,7 @@ extern \"C\" {\n> > #define DRM_FORMAT_MOD_VENDOR_ARM 0x08\n> > #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09\n> > #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a\n> > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b\n> >\n> > /* add more to the end as needed */\n> >\n> > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)\n> > #define AMD_FMT_MOD_CLEAR(field) \\\n> > (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))\n> >\n> > +/* RPI (Raspberry Pi) modifiers */\n> > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)\n> > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)\n> > +\n> > #if defined(__cplusplus)\n> > }\n> > #endif","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D7F43BD160\n\tfor ;\n\tTue, 27 Feb 2024 13:08:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4942B62865;\n\tTue, 27 Feb 2024 14:08:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B490627FC\n\tfor ;\n\tTue, 27 Feb 2024 14:08:25 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2F67B8D4;\n\tTue, 27 Feb 2024 14:08:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Mot49NLQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709039293;\n\tbh=jfjwt1TTGVuxaHJOwlRf5YKl3EIMiknvSswb55ZrDko=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Mot49NLQPtdRqOFCOsFDxHVVBq0DeBV8mMOrPn6TFZEjOYWho2T1NSi4ei8+/yDOQ\n\tf04MNoL158x5mty9w2aNH58hIwvsAnoFJ5afXCZDaIu82gIbs8F/S5JQQ1NjqJCX86\n\t7t1JLOj7KiK0QXH8uALT4nxDWM7xw0jl1zSYV0Xs=","Date":"Tue, 27 Feb 2024 15:08:27 +0200","From":"Laurent Pinchart ","To":"Daniel Vetter ","Subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","Message-ID":"<20240227130827.GA5863@pendragon.ideasonboard.com>","References":"<20240226153854.99471-1-jacopo.mondi@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maarten Lankhorst ,\n\tMaxime Ripard , libcamera-devel@lists.libcamera.org, \n\tdri-devel@lists.freedesktop.org,\n\tThomas Zimmermann , \n\tJacopo Mondi ,\n\tDavid Airlie ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28754,"web_url":"https://patchwork.libcamera.org/comment/28754/","msgid":"<20240227131006.GB5863@pendragon.ideasonboard.com>","date":"2024-02-27T13:10:06","subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Feb 26, 2024 at 05:24:41PM +0100, Jacopo Mondi wrote:\n> On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote:\n> > On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi wrote:\n> > >\n> > > Add modifiers for the Raspberry Pi PiSP compressed formats.\n> > >\n> > > The compressed formats are documented at:\n> > > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst\n> > >\n> > > and in the PiSP datasheet:\n> > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > >\n> > > Background:\n> > > -----------\n> > >\n> > > The Raspberry Pi PiSP camera subsystem is on its way to upstream through the\n> > > Video4Linux2 subsystem:\n> > > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310\n> > >\n> > > The PiSP camera system is composed by a \"Front End\" and a \"Back End\".\n> > > The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory and\n> > > produce statistics, and the BackEnd is a memory-to-memory ISP that converts\n> > > images in a format usable by application.\n> > >\n> > > The \"FrontEnd\" is capable of encoding RAW Bayer images as received by the\n> > > image sensor in a 'compressed' format defined by Raspberry Pi and fully\n> > > documented in the PiSP manual:\n> > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > >\n> > > The compression scheme is documented in the in-review patch series for the BE\n> > > support at:\n> > > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n> > >\n> > > The \"BackEnd\" is capable of consuming images in the compressed format and\n> > > optionally user application might want to inspect those images for debugging\n> > > purposes.\n> > >\n> > > Why a DRM modifier\n> > > ------------------\n> > >\n> > > The PiSP support is entirely implemented in libcamera, with the support of an\n> > > hw-specific library called 'libpisp'.\n> > >\n> > > libcamera uses the fourcc codes defined by DRM to define its formats:\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml\n> > >\n> > > And to define a new libcamera format for the Raspberry Pi compressed ones we\n> > > need to associate the above proposed modifiers with a RAW Bayer format\n> > > identifier.\n> > >\n> > > In example:\n> > >\n> > > - RGGB16_PISP_COMP1:\n> > > fourcc: DRM_FORMAT_SRGGB16\n> > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > - GRBG16_PISP_COMP1:\n> > > fourcc: DRM_FORMAT_SGRBG16\n> > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > - GBRG16_PISP_COMP1:\n> > > fourcc: DRM_FORMAT_SGBRG16\n> > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > - BGGR16_PISP_COMP1:\n> > > fourcc: DRM_FORMAT_SBGGR16\n> > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > - MONO_PISP_COMP1:\n> > > fourcc: DRM_FORMAT_R16\n> > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > >\n> > > See\n> > > https://patchwork.libcamera.org/patch/19503/\n> > >\n> > > Would if be acceptable for DRM to include the above proposed modifiers for the\n> > > purpose of defining the above presented libcamera formats ? There will be no\n> > > graphic format associated with these modifiers as their purpose it not\n> > > displaying images but rather exchange them between the components of the\n> > > camera subsystem (and possibly be inspected by specialized test applications).\n> >\n> > Yeah I think libcamera using drm-fourcc formats and modifiers is\n> > absolutely ok, and has my ack in principle. And for these users we're\n> > ok with merging modifiers that the kernel doesn't use.\n> \n> Great!\n> \n> > I think it would be really good to formalize this by adding libcamera\n> > to the officially listed users in the \"Open Source User Waiver\"\n> > section in the drm_fourcc.h docs:\n> >\n> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#open-source-user-waiver\n> >\n> > You might want to convert that into a list, it could get a bit\n> > confusing. Then we can get that patch properly acked (by kernel and\n> > libcamera folks) to record the community consensus.\n> \n> I see your point, but I wonder if libcamera actually need to be part\n> of this list; we want in-kernel upstream driver and open-source\n\nFor the kernel side we'll use V4L2, so the DRM modifier won't have an\nin-kernel user.\n\nOn the userspace side, yes, there will be an open-source user with\nlibcamera.\n\n> userspace components. We allow binary 3A modules to be loaded by the\n> library, but I'm not sure they will ever need to modify the DRM 4cc list.\n> \n> Anyway, with other libcamera people ack, I can certainly do so!\n> \n> > For the rpi modifiers themselves: They need to be properly documented,\n> > least to exclude a screw-up like with the rpi modifiers we already\n> > have, which unfortunately encode the buffer height (instead of just\n> > the rounding algorithim to align the height to the right tile size) in\n> > the modifiers, which breaks assumptions everywhere. For details see\n> > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057\n> \n> I see. The formats are fully documented in the above linked datasheet,\n> and I've provided (with the help of RPi engineers, as I don't\n> understand the compression algorithm part :) a shorter description as\n> part of the V4L2 patch series that upstreams the BE driver\n> \n> https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n> \n> The only indication about the buffer's layout I see is\n> \n> Each scanline is padded to a multiple of 8 pixels wide, and each block of 8\n> horizontally-contiguous pixels is coded using 8 bytes.\n> \n> While the rest of the text describes the compression algorithm which\n> (afaiu) doesn't affect the buffer geometry.\n> \n> Would you be ok with me replicating the above description (or maybe\n> just reference the V4L2 documentation ?)\n> \n> > > ---\n> > > include/uapi/drm/drm_fourcc.h | 5 +++++\n> > > 1 file changed, 5 insertions(+)\n> > >\n> > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h\n> > > index 00db00083175..09b182a959ad 100644\n> > > --- a/include/uapi/drm/drm_fourcc.h\n> > > +++ b/include/uapi/drm/drm_fourcc.h\n> > > @@ -425,6 +425,7 @@ extern \"C\" {\n> > > #define DRM_FORMAT_MOD_VENDOR_ARM 0x08\n> > > #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09\n> > > #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a\n> > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b\n> > >\n> > > /* add more to the end as needed */\n> > >\n> > > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)\n> > > #define AMD_FMT_MOD_CLEAR(field) \\\n> > > (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))\n> > >\n> > > +/* RPI (Raspberry Pi) modifiers */\n> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)\n> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)\n> > > +\n> > > #if defined(__cplusplus)\n> > > }\n> > > #endif","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 90CEDBD160\n\tfor ;\n\tTue, 27 Feb 2024 13:10:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 454FC62864;\n\tTue, 27 Feb 2024 14:10:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD62E627FC\n\tfor ;\n\tTue, 27 Feb 2024 14:10:03 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8E7588D4;\n\tTue, 27 Feb 2024 14:09:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"p7317QAg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709039391;\n\tbh=v49QjxY4U9/AmYff5q8VBtbEYH5broHyCUF/VUdaeH0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p7317QAglfL4Zadd0nGP1KqIrIgVHHV4FMebEAI/P7Hld+8s81wldN1pU+0ivXoxH\n\tL5NTb9GyghuvTGduDvyt5b9o9iZiSikch+fS3sfW4L8L8Jv9Izp3clHh94swOfWUmp\n\tI6Zzk5VJF8M1Pe+3bip3Hd4SzOh+QsvTTT4zFBI8=","Date":"Tue, 27 Feb 2024 15:10:06 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","Message-ID":"<20240227131006.GB5863@pendragon.ideasonboard.com>","References":"<20240226153854.99471-1-jacopo.mondi@ideasonboard.com>\n\t\n\t<473ei7jyjevynhb7roinhdaj2hnmsog6owiognlyp5fpfc63pa@mdn73c5gu46r>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<473ei7jyjevynhb7roinhdaj2hnmsog6owiognlyp5fpfc63pa@mdn73c5gu46r>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Daniel Vetter ,\n\tMaarten Lankhorst ,\n\tdri-devel@lists.freedesktop.org, libcamera-devel@lists.libcamera.org, \n\tMaxime Ripard ,\n\tThomas Zimmermann , \n\tDavid Airlie ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28785,"web_url":"https://patchwork.libcamera.org/comment/28785/","msgid":"","date":"2024-02-28T10:41:57","subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Tue, Feb 27, 2024 at 03:08:27PM +0200, Laurent Pinchart wrote:\n> On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote:\n> > On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi wrote:\n> > >\n> > > Add modifiers for the Raspberry Pi PiSP compressed formats.\n> > >\n> > > The compressed formats are documented at:\n> > > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst\n> > >\n> > > and in the PiSP datasheet:\n> > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > >\n> > > Background:\n> > > -----------\n> > >\n> > > The Raspberry Pi PiSP camera subsystem is on its way to upstream through the\n> > > Video4Linux2 subsystem:\n> > > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310\n> > >\n> > > The PiSP camera system is composed by a \"Front End\" and a \"Back End\".\n> > > The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory and\n> > > produce statistics, and the BackEnd is a memory-to-memory ISP that converts\n> > > images in a format usable by application.\n> > >\n> > > The \"FrontEnd\" is capable of encoding RAW Bayer images as received by the\n> > > image sensor in a 'compressed' format defined by Raspberry Pi and fully\n> > > documented in the PiSP manual:\n> > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > >\n> > > The compression scheme is documented in the in-review patch series for the BE\n> > > support at:\n> > > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n> > >\n> > > The \"BackEnd\" is capable of consuming images in the compressed format and\n> > > optionally user application might want to inspect those images for debugging\n> > > purposes.\n> > >\n> > > Why a DRM modifier\n> > > ------------------\n> > >\n> > > The PiSP support is entirely implemented in libcamera, with the support of an\n> > > hw-specific library called 'libpisp'.\n> > >\n> > > libcamera uses the fourcc codes defined by DRM to define its formats:\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml\n> > >\n> > > And to define a new libcamera format for the Raspberry Pi compressed ones we\n> > > need to associate the above proposed modifiers with a RAW Bayer format\n> > > identifier.\n> > >\n> > > In example:\n> > >\n> > > - RGGB16_PISP_COMP1:\n> > > fourcc: DRM_FORMAT_SRGGB16\n>\n> An \"interesting\" issue here is that these formats currently live in\n> libcamera only, we haven't merged them in DRM \"yet\". This may be a\n> prerequisite ?\n>\n\nAh right! I didn't notice!\n\nI think there are two issues at play here, one to be clarified by the\nDRM maintainers, the other more technically involved with the\ndefinition of the Bayer formats themselves.\n\n- Does DRM want RAW Bayer formats to be listed here, as these are not\n typically 'graphic' formats. What's the DRM maintainers opinion here ?\n\n- The RAW Bayer pattern ordering: we have realized that the idea of\n defining RAW Bayer formats by encoding the components ordering in\n V4L2 was, in retrospective, a bad idea. The reason is that any\n change in the Bayer order along the capture pipeline (usually due to\n flip) requires reconfiguration of the formats along the whole\n pipeline, requiring a start/stop of the capture operations.\n\n For this reason, we have multiple times discussed the idea of only\n conveying the bit depth in the format definition and to communicate\n the components ordering with other out-of-band mechanism, that will\n likely be used to negotiate between the image sensor and the CSI-2\n receiver.\n\n With DRM the ideal would be something like\n\n DRM_FORMAT_RAW8 | FORMAT_MODIFIER_SBGGR\n DRM_FORMAT_RAW8 | FORMAT_MODIFIER_SGBRG\n ...\n\n DRM_FORMAT_RAW10 | FORMAT_MODIFIER_SBGGR\n DRM_FORMAT_RAW10 | FORMAT_MODIFIER_SGBRG\n ...\n\n This however prevents to use additional modifiers to convey, in\n example, packaging (ie MIPI CSI-2) or compressions (like done here\n for PISP_COMP1) as, as far as I understand it, multiple modifiers\n cannot be applied to a format.\n\n However, as I understand the argument of not repeating the V4L2\n mistakes here, the DRM formats are aimed to be used mostly to\n represent the formats of images as they get stored in application\n buffers (at least from a libcamera perspective). The drawback of\n having to reconfigure the capture pipeline due to a flip does not\n really apply here, actually, knowing the components ordering in the\n final image is very relevant for applications to know how to\n interpret the image data.\n\n TL;DR I'm not too concerned about having to propagate the components\n ordering along the pipeline, as the DRM formats are used only to\n describe the image data as presented to the applications and not\n along the pipeline as it happens for the V4L2 formats.\n\nOpinions anyone ?\n\nThanks\n j\n\n> > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > - GRBG16_PISP_COMP1:\n> > > fourcc: DRM_FORMAT_SGRBG16\n> > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > - GBRG16_PISP_COMP1:\n> > > fourcc: DRM_FORMAT_SGBRG16\n> > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > - BGGR16_PISP_COMP1:\n> > > fourcc: DRM_FORMAT_SBGGR16\n> > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > - MONO_PISP_COMP1:\n> > > fourcc: DRM_FORMAT_R16\n> > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > >\n> > > See\n> > > https://patchwork.libcamera.org/patch/19503/\n> > >\n> > > Would if be acceptable for DRM to include the above proposed modifiers for the\n> > > purpose of defining the above presented libcamera formats ? There will be no\n> > > graphic format associated with these modifiers as their purpose it not\n> > > displaying images but rather exchange them between the components of the\n> > > camera subsystem (and possibly be inspected by specialized test applications).\n> >\n> > Yeah I think libcamera using drm-fourcc formats and modifiers is\n> > absolutely ok, and has my ack in principle. And for these users we're\n> > ok with merging modifiers that the kernel doesn't use.\n> >\n> > I think it would be really good to formalize this by adding libcamera\n> > to the officially listed users in the \"Open Source User Waiver\"\n> > section in the drm_fourcc.h docs:\n> >\n> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#open-source-user-waiver\n>\n> The waiver states \"the usual requirement for an upstream in-kernel or\n> open source userspace user does not apply\". Strictly speaking, there is\n> an open-source userspace user with libcamera. It's only on the kernel\n> side that the new modifier my not get used.\n>\n> This being said, I'm fine clarifying the documentation, clarity is\n> always good.\n>\n> > You might want to convert that into a list, it could get a bit\n> > confusing. Then we can get that patch properly acked (by kernel and\n> > libcamera folks) to record the community consensus.\n> >\n> > For the rpi modifiers themselves: They need to be properly documented,\n> > least to exclude a screw-up like with the rpi modifiers we already\n> > have, which unfortunately encode the buffer height (instead of just\n> > the rounding algorithim to align the height to the right tile size) in\n> > the modifiers, which breaks assumptions everywhere. For details see\n> > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057\n> >\n> > > ---\n> > > include/uapi/drm/drm_fourcc.h | 5 +++++\n> > > 1 file changed, 5 insertions(+)\n> > >\n> > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h\n> > > index 00db00083175..09b182a959ad 100644\n> > > --- a/include/uapi/drm/drm_fourcc.h\n> > > +++ b/include/uapi/drm/drm_fourcc.h\n> > > @@ -425,6 +425,7 @@ extern \"C\" {\n> > > #define DRM_FORMAT_MOD_VENDOR_ARM 0x08\n> > > #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09\n> > > #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a\n> > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b\n> > >\n> > > /* add more to the end as needed */\n> > >\n> > > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)\n> > > #define AMD_FMT_MOD_CLEAR(field) \\\n> > > (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))\n> > >\n> > > +/* RPI (Raspberry Pi) modifiers */\n> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)\n> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)\n> > > +\n> > > #if defined(__cplusplus)\n> > > }\n> > > #endif\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8A146BD160\n\tfor ;\n\tWed, 28 Feb 2024 10:42:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4606462873;\n\tWed, 28 Feb 2024 11:42:02 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E468362865\n\tfor ;\n\tWed, 28 Feb 2024 11:42:00 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E26D8672;\n\tWed, 28 Feb 2024 11:41:47 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wEHtJQs0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709116908;\n\tbh=4zQ7UUI/up48qX1ZzKACFZftS9DX0OqT/16yMC0Lj2U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wEHtJQs0m/2p9t2vUU7dwG459ybuUc6OhxwzpLfCTMGIjkGa4+eokquNeidmc1xFk\n\tWGza3PLpMlj640vgMWHpky/mOyOdEVj8qrxkZwetRxQDAC/kHhA5GhyLjNkuP+pWhe\n\t2EdBujAu1L2oB9ORKCYQyzqpmw+bl2oKpk8Twfqk=","Date":"Wed, 28 Feb 2024 11:41:57 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","Message-ID":"","References":"<20240226153854.99471-1-jacopo.mondi@ideasonboard.com>\n\t\n\t<20240227130827.GA5863@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240227130827.GA5863@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Thomas Zimmermann ,\n\tMaarten Lankhorst ,\n\tMaxime Ripard , libcamera-devel@lists.libcamera.org, \n\tdri-devel@lists.freedesktop.org, Daniel Vetter ,\n\tJacopo Mondi ,\n\tDavid Airlie ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28789,"web_url":"https://patchwork.libcamera.org/comment/28789/","msgid":"<20240228111345.GK3419@pendragon.ideasonboard.com>","date":"2024-02-28T11:13:45","subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Feb 28, 2024 at 11:41:57AM +0100, Jacopo Mondi wrote:\n> On Tue, Feb 27, 2024 at 03:08:27PM +0200, Laurent Pinchart wrote:\n> > On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote:\n> > > On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi wrote:\n> > > >\n> > > > Add modifiers for the Raspberry Pi PiSP compressed formats.\n> > > >\n> > > > The compressed formats are documented at:\n> > > > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst\n> > > >\n> > > > and in the PiSP datasheet:\n> > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > >\n> > > > Background:\n> > > > -----------\n> > > >\n> > > > The Raspberry Pi PiSP camera subsystem is on its way to upstream through the\n> > > > Video4Linux2 subsystem:\n> > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310\n> > > >\n> > > > The PiSP camera system is composed by a \"Front End\" and a \"Back End\".\n> > > > The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory and\n> > > > produce statistics, and the BackEnd is a memory-to-memory ISP that converts\n> > > > images in a format usable by application.\n> > > >\n> > > > The \"FrontEnd\" is capable of encoding RAW Bayer images as received by the\n> > > > image sensor in a 'compressed' format defined by Raspberry Pi and fully\n> > > > documented in the PiSP manual:\n> > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > > >\n> > > > The compression scheme is documented in the in-review patch series for the BE\n> > > > support at:\n> > > > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n> > > >\n> > > > The \"BackEnd\" is capable of consuming images in the compressed format and\n> > > > optionally user application might want to inspect those images for debugging\n> > > > purposes.\n> > > >\n> > > > Why a DRM modifier\n> > > > ------------------\n> > > >\n> > > > The PiSP support is entirely implemented in libcamera, with the support of an\n> > > > hw-specific library called 'libpisp'.\n> > > >\n> > > > libcamera uses the fourcc codes defined by DRM to define its formats:\n> > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml\n> > > >\n> > > > And to define a new libcamera format for the Raspberry Pi compressed ones we\n> > > > need to associate the above proposed modifiers with a RAW Bayer format\n> > > > identifier.\n> > > >\n> > > > In example:\n> > > >\n> > > > - RGGB16_PISP_COMP1:\n> > > > fourcc: DRM_FORMAT_SRGGB16\n> >\n> > An \"interesting\" issue here is that these formats currently live in\n> > libcamera only, we haven't merged them in DRM \"yet\". This may be a\n> > prerequisite ?\n> >\n> \n> Ah right! I didn't notice!\n> \n> I think there are two issues at play here, one to be clarified by the\n> DRM maintainers, the other more technically involved with the\n> definition of the Bayer formats themselves.\n> \n> - Does DRM want RAW Bayer formats to be listed here, as these are not\n> typically 'graphic' formats. What's the DRM maintainers opinion here ?\n\nTo give some context, the \"historical mistake\" I keep referring to\nregarding V4L2 is the decision to combine the bit depth of raw formats\nwith the colour filter array (a.k.a. Bayer) pattern into a fourcc. I\nthink we should have defined raw pixel formats that only encode a bit\ndepth, and conveyed the CFA pattern out-of-band. This is especially the\ncase for media bus codes (formats on the buses between hardware\ndevices). The reasoning here is that most devices only care about the\nbit depth and not the CFA pattern. Adding a new CFA pattern (for\ninstance for RGB+Ir) for a camera sensor would currently require adding\na new media bus code (easy), using it in the sensor driver (obvious),\nand patching *every* CSI-2 receiver driver to pass it through. Userspace\nwould similarly need to grow support for the new format, even for\nuserspace code that only cares about capturing the raw data without\nprocessing the colour components. Having raw pixel formats that don't\nconvey the CFA pattern would simplify all this (and it's something I'm\nconsidering adding to the media bus codes).\npart is lots of churn\n\n> - The RAW Bayer pattern ordering: we have realized that the idea of\n> defining RAW Bayer formats by encoding the components ordering in\n> V4L2 was, in retrospective, a bad idea. The reason is that any\n> change in the Bayer order along the capture pipeline (usually due to\n> flip) requires reconfiguration of the formats along the whole\n> pipeline, requiring a start/stop of the capture operations.\n\nThat's also a reason, yes.\n\n> For this reason, we have multiple times discussed the idea of only\n> conveying the bit depth in the format definition and to communicate\n> the components ordering with other out-of-band mechanism, that will\n> likely be used to negotiate between the image sensor and the CSI-2\n> receiver.\n> \n> With DRM the ideal would be something like\n> \n> DRM_FORMAT_RAW8 | FORMAT_MODIFIER_SBGGR\n> DRM_FORMAT_RAW8 | FORMAT_MODIFIER_SGBRG\n> ...\n> \n> DRM_FORMAT_RAW10 | FORMAT_MODIFIER_SBGGR\n> DRM_FORMAT_RAW10 | FORMAT_MODIFIER_SGBRG\n> ...\n\nWhen I said out-of-band, I meant really out of band from a format point\nof view, so neither in the 4CC nor in the modifiers.\n\n> This however prevents to use additional modifiers to convey, in\n> example, packaging (ie MIPI CSI-2) or compressions (like done here\n> for PISP_COMP1) as, as far as I understand it, multiple modifiers\n> cannot be applied to a format.\n> \n> However, as I understand the argument of not repeating the V4L2\n> mistakes here, the DRM formats are aimed to be used mostly to\n> represent the formats of images as they get stored in application\n> buffers (at least from a libcamera perspective). The drawback of\n> having to reconfigure the capture pipeline due to a flip does not\n> really apply here, actually, knowing the components ordering in the\n> final image is very relevant for applications to know how to\n> interpret the image data.\n> \n> TL;DR I'm not too concerned about having to propagate the components\n> ordering along the pipeline, as the DRM formats are used only to\n> describe the image data as presented to the applications and not\n> along the pipeline as it happens for the V4L2 formats.\n> \n> Opinions anyone ?\n> \n> > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > - GRBG16_PISP_COMP1:\n> > > > fourcc: DRM_FORMAT_SGRBG16\n> > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > - GBRG16_PISP_COMP1:\n> > > > fourcc: DRM_FORMAT_SGBRG16\n> > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > - BGGR16_PISP_COMP1:\n> > > > fourcc: DRM_FORMAT_SBGGR16\n> > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > - MONO_PISP_COMP1:\n> > > > fourcc: DRM_FORMAT_R16\n> > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > >\n> > > > See\n> > > > https://patchwork.libcamera.org/patch/19503/\n> > > >\n> > > > Would if be acceptable for DRM to include the above proposed modifiers for the\n> > > > purpose of defining the above presented libcamera formats ? There will be no\n> > > > graphic format associated with these modifiers as their purpose it not\n> > > > displaying images but rather exchange them between the components of the\n> > > > camera subsystem (and possibly be inspected by specialized test applications).\n> > >\n> > > Yeah I think libcamera using drm-fourcc formats and modifiers is\n> > > absolutely ok, and has my ack in principle. And for these users we're\n> > > ok with merging modifiers that the kernel doesn't use.\n> > >\n> > > I think it would be really good to formalize this by adding libcamera\n> > > to the officially listed users in the \"Open Source User Waiver\"\n> > > section in the drm_fourcc.h docs:\n> > >\n> > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#open-source-user-waiver\n> >\n> > The waiver states \"the usual requirement for an upstream in-kernel or\n> > open source userspace user does not apply\". Strictly speaking, there is\n> > an open-source userspace user with libcamera. It's only on the kernel\n> > side that the new modifier my not get used.\n> >\n> > This being said, I'm fine clarifying the documentation, clarity is\n> > always good.\n> >\n> > > You might want to convert that into a list, it could get a bit\n> > > confusing. Then we can get that patch properly acked (by kernel and\n> > > libcamera folks) to record the community consensus.\n> > >\n> > > For the rpi modifiers themselves: They need to be properly documented,\n> > > least to exclude a screw-up like with the rpi modifiers we already\n> > > have, which unfortunately encode the buffer height (instead of just\n> > > the rounding algorithim to align the height to the right tile size) in\n> > > the modifiers, which breaks assumptions everywhere. For details see\n> > > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057\n> > >\n> > > > ---\n> > > > include/uapi/drm/drm_fourcc.h | 5 +++++\n> > > > 1 file changed, 5 insertions(+)\n> > > >\n> > > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h\n> > > > index 00db00083175..09b182a959ad 100644\n> > > > --- a/include/uapi/drm/drm_fourcc.h\n> > > > +++ b/include/uapi/drm/drm_fourcc.h\n> > > > @@ -425,6 +425,7 @@ extern \"C\" {\n> > > > #define DRM_FORMAT_MOD_VENDOR_ARM 0x08\n> > > > #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09\n> > > > #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a\n> > > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b\n> > > >\n> > > > /* add more to the end as needed */\n> > > >\n> > > > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)\n> > > > #define AMD_FMT_MOD_CLEAR(field) \\\n> > > > (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))\n> > > >\n> > > > +/* RPI (Raspberry Pi) modifiers */\n> > > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)\n> > > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)\n> > > > +\n> > > > #if defined(__cplusplus)\n> > > > }\n> > > > #endif","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8534FBD160\n\tfor ;\n\tWed, 28 Feb 2024 11:13:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C92A462868;\n\tWed, 28 Feb 2024 12:13:45 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8877261C94\n\tfor ;\n\tWed, 28 Feb 2024 12:13:44 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D1342B3;\n\tWed, 28 Feb 2024 12:13:31 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uxxkfH85\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709118811;\n\tbh=X265+GLtL8FxrQoXnFB6YbmIlSfVxB3qsx1cSvWW4Z8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uxxkfH85qfd9DDEovXOzW3B4+RU2z2ZIbZevqBh+UCguYTM5P5plSGbzGAwNchOSP\n\tPX8cMS1mBT8EWzQV0//ukT4PwuouGES2ZvVSF5PVcB/U2GTCBgr478usmLsRuwT2q4\n\tsahIo1pqo0QlW71vXK9u6PO0Fkve39Xix/nzvqWU=","Date":"Wed, 28 Feb 2024 13:13:45 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","Message-ID":"<20240228111345.GK3419@pendragon.ideasonboard.com>","References":"<20240226153854.99471-1-jacopo.mondi@ideasonboard.com>\n\t\n\t<20240227130827.GA5863@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Thomas Zimmermann ,\n\tMaarten Lankhorst ,\n\tMaxime Ripard , libcamera-devel@lists.libcamera.org, \n\tdri-devel@lists.freedesktop.org, Daniel Vetter ,\n\tDavid Airlie ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28800,"web_url":"https://patchwork.libcamera.org/comment/28800/","msgid":"","date":"2024-02-29T10:31:06","subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","submitter":{"id":187,"url":"https://patchwork.libcamera.org/api/people/187/","name":"Daniel Vetter","email":"daniel@ffwll.ch"},"content":"On Tue, Feb 27, 2024 at 03:10:06PM +0200, Laurent Pinchart wrote:\n> On Mon, Feb 26, 2024 at 05:24:41PM +0100, Jacopo Mondi wrote:\n> > On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote:\n> > > On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi wrote:\n> > > >\n> > > > Add modifiers for the Raspberry Pi PiSP compressed formats.\n> > > >\n> > > > The compressed formats are documented at:\n> > > > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst\n> > > >\n> > > > and in the PiSP datasheet:\n> > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > >\n> > > > Background:\n> > > > -----------\n> > > >\n> > > > The Raspberry Pi PiSP camera subsystem is on its way to upstream through the\n> > > > Video4Linux2 subsystem:\n> > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310\n> > > >\n> > > > The PiSP camera system is composed by a \"Front End\" and a \"Back End\".\n> > > > The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory and\n> > > > produce statistics, and the BackEnd is a memory-to-memory ISP that converts\n> > > > images in a format usable by application.\n> > > >\n> > > > The \"FrontEnd\" is capable of encoding RAW Bayer images as received by the\n> > > > image sensor in a 'compressed' format defined by Raspberry Pi and fully\n> > > > documented in the PiSP manual:\n> > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > > >\n> > > > The compression scheme is documented in the in-review patch series for the BE\n> > > > support at:\n> > > > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n> > > >\n> > > > The \"BackEnd\" is capable of consuming images in the compressed format and\n> > > > optionally user application might want to inspect those images for debugging\n> > > > purposes.\n> > > >\n> > > > Why a DRM modifier\n> > > > ------------------\n> > > >\n> > > > The PiSP support is entirely implemented in libcamera, with the support of an\n> > > > hw-specific library called 'libpisp'.\n> > > >\n> > > > libcamera uses the fourcc codes defined by DRM to define its formats:\n> > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml\n> > > >\n> > > > And to define a new libcamera format for the Raspberry Pi compressed ones we\n> > > > need to associate the above proposed modifiers with a RAW Bayer format\n> > > > identifier.\n> > > >\n> > > > In example:\n> > > >\n> > > > - RGGB16_PISP_COMP1:\n> > > > fourcc: DRM_FORMAT_SRGGB16\n> > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > - GRBG16_PISP_COMP1:\n> > > > fourcc: DRM_FORMAT_SGRBG16\n> > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > - GBRG16_PISP_COMP1:\n> > > > fourcc: DRM_FORMAT_SGBRG16\n> > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > - BGGR16_PISP_COMP1:\n> > > > fourcc: DRM_FORMAT_SBGGR16\n> > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > - MONO_PISP_COMP1:\n> > > > fourcc: DRM_FORMAT_R16\n> > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > >\n> > > > See\n> > > > https://patchwork.libcamera.org/patch/19503/\n> > > >\n> > > > Would if be acceptable for DRM to include the above proposed modifiers for the\n> > > > purpose of defining the above presented libcamera formats ? There will be no\n> > > > graphic format associated with these modifiers as their purpose it not\n> > > > displaying images but rather exchange them between the components of the\n> > > > camera subsystem (and possibly be inspected by specialized test applications).\n> > >\n> > > Yeah I think libcamera using drm-fourcc formats and modifiers is\n> > > absolutely ok, and has my ack in principle. And for these users we're\n> > > ok with merging modifiers that the kernel doesn't use.\n> > \n> > Great!\n> > \n> > > I think it would be really good to formalize this by adding libcamera\n> > > to the officially listed users in the \"Open Source User Waiver\"\n> > > section in the drm_fourcc.h docs:\n> > >\n> > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#open-source-user-waiver\n> > >\n> > > You might want to convert that into a list, it could get a bit\n> > > confusing. Then we can get that patch properly acked (by kernel and\n> > > libcamera folks) to record the community consensus.\n> > \n> > I see your point, but I wonder if libcamera actually need to be part\n> > of this list; we want in-kernel upstream driver and open-source\n> \n> For the kernel side we'll use V4L2, so the DRM modifier won't have an\n> in-kernel user.\n\nAlso if you e.g. use a gpu or accel thing for post-processing, which would\nbe a drm driver, then you still don't have an in-kernel user for these\nbecause the userspace driver handles that all directly and just sends\ncommand buffers to the kernel.\n\nSo I think adding libcamera explicitly would be really good.\n\nAlso if you convert the list to a proper list it might be good to link to\nthe relevant gl/vk extensions directly instead of just vaguely hinting at\nthem.\n\n> On the userspace side, yes, there will be an open-source user with\n> libcamera.\n> \n> > userspace components. We allow binary 3A modules to be loaded by the\n> > library, but I'm not sure they will ever need to modify the DRM 4cc list.\n> > \n> > Anyway, with other libcamera people ack, I can certainly do so!\n> > \n> > > For the rpi modifiers themselves: They need to be properly documented,\n> > > least to exclude a screw-up like with the rpi modifiers we already\n> > > have, which unfortunately encode the buffer height (instead of just\n> > > the rounding algorithim to align the height to the right tile size) in\n> > > the modifiers, which breaks assumptions everywhere. For details see\n> > > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057\n> > \n> > I see. The formats are fully documented in the above linked datasheet,\n> > and I've provided (with the help of RPi engineers, as I don't\n> > understand the compression algorithm part :) a shorter description as\n> > part of the V4L2 patch series that upstreams the BE driver\n\nI think linking to the datasheet in the comment above the modifiers is ok\ntoo, if we don't have anything better. Just not entirely undocumented\nmagic values, because those result in giantic confusion all around.\n\nAlso for the namespace, I'm assuming this format is actually rpi IP, and\nnot something they've lincensed from someone else? Because the point here\nis interop, so if it's a licensed format we need to put the modifiers\nunder the name of the original IP vendor, or people will create duplicated\nmodifiers and the entire interop dream is just a dream.\n-Sima\n\n\n> > \n> > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n> > \n> > The only indication about the buffer's layout I see is\n> > \n> > Each scanline is padded to a multiple of 8 pixels wide, and each block of 8\n> > horizontally-contiguous pixels is coded using 8 bytes.\n> > \n> > While the rest of the text describes the compression algorithm which\n> > (afaiu) doesn't affect the buffer geometry.\n> > \n> > Would you be ok with me replicating the above description (or maybe\n> > just reference the V4L2 documentation ?)\n> > \n> > > > ---\n> > > > include/uapi/drm/drm_fourcc.h | 5 +++++\n> > > > 1 file changed, 5 insertions(+)\n> > > >\n> > > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h\n> > > > index 00db00083175..09b182a959ad 100644\n> > > > --- a/include/uapi/drm/drm_fourcc.h\n> > > > +++ b/include/uapi/drm/drm_fourcc.h\n> > > > @@ -425,6 +425,7 @@ extern \"C\" {\n> > > > #define DRM_FORMAT_MOD_VENDOR_ARM 0x08\n> > > > #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09\n> > > > #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a\n> > > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b\n> > > >\n> > > > /* add more to the end as needed */\n> > > >\n> > > > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)\n> > > > #define AMD_FMT_MOD_CLEAR(field) \\\n> > > > (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))\n> > > >\n> > > > +/* RPI (Raspberry Pi) modifiers */\n> > > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)\n> > > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)\n> > > > +\n> > > > #if defined(__cplusplus)\n> > > > }\n> > > > #endif\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3E351BD160\n\tfor ;\n\tThu, 29 Feb 2024 10:31:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68ACC6286D;\n\tThu, 29 Feb 2024 11:31:11 +0100 (CET)","from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com\n\t[IPv6:2a00:1450:4864:20::42f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF14A61C96\n\tfor ;\n\tThu, 29 Feb 2024 11:31:09 +0100 (CET)","by mail-wr1-x42f.google.com with SMTP id\n\tffacd0b85a97d-33df8203a08so215736f8f.0\n\tfor ;\n\tThu, 29 Feb 2024 02:31:09 -0800 (PST)","from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa])\n\tby smtp.gmail.com with ESMTPSA id\n\tfc18-20020a05600c525200b00412aeb77bbcsm1655397wmb.19.2024.02.29.02.31.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Feb 2024 02:31:08 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ffwll.ch header.i=@ffwll.ch header.b=\"bszxAmsv\";\n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ffwll.ch; s=google; t=1709202669; x=1709807469;\n\tdarn=lists.libcamera.org; \n\th=in-reply-to:content-disposition:mime-version:references:message-id\n\t:subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; \n\tbh=oCCRYUVkzEW74bA3wPKxqIN5m8hMZvpa6nGmDbuITfY=;\n\tb=bszxAmsv6f13Pb0xIXcI6tUqyZl3RRPeK20g0k+CGu91ktua/ZGMFoBs+v2Cv7+9DQ\n\to64jCPEiUH2I4Y8unfyQpU03sS/rC/WQEx8Li8Q1PmqyA37jryqnR9rZnA1py5hD1bls\n\tqDgFQVkqftJzBZKZRDBAU99aOkPsa2odfldC4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1709202669; x=1709807469;\n\th=in-reply-to:content-disposition:mime-version:references:message-id\n\t:subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=oCCRYUVkzEW74bA3wPKxqIN5m8hMZvpa6nGmDbuITfY=;\n\tb=hdN1Fo4+olAGnErowf4j4beqlZLBdr84poShx9zlEDM5X00fJWyyPtwpbsMiY9XGCc\n\tchCFihcl27LP1lY4Nn0LkGDcos0DDiA5OrZxHt+4kAsLTfLoOu1lbVPwGIFxeAb7u7l/\n\t9r4QMH/MO+O7pzUcLC5kc16M04cUPjbr56NgmLOzqYiXgcZrwKJSsSy/qzwk/gmUFkVP\n\t9TXsFULlY4fxMssBEy4eml196KLIfzVfDLvZ68xxly2SKo8anlqvQNFcFNGZJhfIv1ru\n\tcdKC/Y34zgeXtcdwDc7+/ZcxGKHm1SxxJBIH3RUpHezpNISQuz6hMxL23GEV3iPO0ztE\n\tlRmA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVDXzcsNk1dbCbmgGNL0aTmAWZfDgIL+EWaXELa262e+EU75m+PoispiFhGoqnBqrjaLH30u3DP0NaoAhqvZCWHrHZkSpoggbb86ivyTMS5bx+A9g==","X-Gm-Message-State":"AOJu0YxCYzzmBJlrLnoUMPlVeZ6B98p7ZiD3XNwzf/YQ83zRKnHvg36L\n\tHTmHf33+WgZIgXODBe9K1rS+KljXNUPee/WIBUFBo5ulKDCw0Y2hrX5Lzl9/IjM=","X-Google-Smtp-Source":"AGHT+IGZGcWyleP0w0O0pOg+nQfRGNOlKJCa4eGypGpJ0HUnHG9IG5DW/Jkj2kEDzvuJIVi/RO7jWA==","X-Received":"by 2002:a05:600c:3b82:b0:412:a71e:98a3 with SMTP id\n\tn2-20020a05600c3b8200b00412a71e98a3mr1345088wms.3.1709202669001; \n\tThu, 29 Feb 2024 02:31:09 -0800 (PST)","Date":"Thu, 29 Feb 2024 11:31:06 +0100","From":"Daniel Vetter ","To":"Laurent Pinchart ","Subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","Message-ID":"","References":"<20240226153854.99471-1-jacopo.mondi@ideasonboard.com>\n\t\n\t<473ei7jyjevynhb7roinhdaj2hnmsog6owiognlyp5fpfc63pa@mdn73c5gu46r>\n\t<20240227131006.GB5863@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240227131006.GB5863@pendragon.ideasonboard.com>","X-Operating-System":"Linux phenom 6.6.11-amd64 ","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Thomas Zimmermann ,\n\tMaarten Lankhorst ,\n\tdri-devel@lists.freedesktop.org, libcamera-devel@lists.libcamera.org, \n\tMaxime Ripard , Daniel Vetter , \n\tJacopo Mondi ,\n\tDavid Airlie ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28801,"web_url":"https://patchwork.libcamera.org/comment/28801/","msgid":"","date":"2024-02-29T10:39:24","subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","submitter":{"id":187,"url":"https://patchwork.libcamera.org/api/people/187/","name":"Daniel Vetter","email":"daniel@ffwll.ch"},"content":"On Wed, Feb 28, 2024 at 01:13:45PM +0200, Laurent Pinchart wrote:\n> On Wed, Feb 28, 2024 at 11:41:57AM +0100, Jacopo Mondi wrote:\n> > On Tue, Feb 27, 2024 at 03:08:27PM +0200, Laurent Pinchart wrote:\n> > > On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote:\n> > > > On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi wrote:\n> > > > >\n> > > > > Add modifiers for the Raspberry Pi PiSP compressed formats.\n> > > > >\n> > > > > The compressed formats are documented at:\n> > > > > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst\n> > > > >\n> > > > > and in the PiSP datasheet:\n> > > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > >\n> > > > > Background:\n> > > > > -----------\n> > > > >\n> > > > > The Raspberry Pi PiSP camera subsystem is on its way to upstream through the\n> > > > > Video4Linux2 subsystem:\n> > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310\n> > > > >\n> > > > > The PiSP camera system is composed by a \"Front End\" and a \"Back End\".\n> > > > > The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory and\n> > > > > produce statistics, and the BackEnd is a memory-to-memory ISP that converts\n> > > > > images in a format usable by application.\n> > > > >\n> > > > > The \"FrontEnd\" is capable of encoding RAW Bayer images as received by the\n> > > > > image sensor in a 'compressed' format defined by Raspberry Pi and fully\n> > > > > documented in the PiSP manual:\n> > > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > > > >\n> > > > > The compression scheme is documented in the in-review patch series for the BE\n> > > > > support at:\n> > > > > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n> > > > >\n> > > > > The \"BackEnd\" is capable of consuming images in the compressed format and\n> > > > > optionally user application might want to inspect those images for debugging\n> > > > > purposes.\n> > > > >\n> > > > > Why a DRM modifier\n> > > > > ------------------\n> > > > >\n> > > > > The PiSP support is entirely implemented in libcamera, with the support of an\n> > > > > hw-specific library called 'libpisp'.\n> > > > >\n> > > > > libcamera uses the fourcc codes defined by DRM to define its formats:\n> > > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml\n> > > > >\n> > > > > And to define a new libcamera format for the Raspberry Pi compressed ones we\n> > > > > need to associate the above proposed modifiers with a RAW Bayer format\n> > > > > identifier.\n> > > > >\n> > > > > In example:\n> > > > >\n> > > > > - RGGB16_PISP_COMP1:\n> > > > > fourcc: DRM_FORMAT_SRGGB16\n> > >\n> > > An \"interesting\" issue here is that these formats currently live in\n> > > libcamera only, we haven't merged them in DRM \"yet\". This may be a\n> > > prerequisite ?\n> > >\n> > \n> > Ah right! I didn't notice!\n> > \n> > I think there are two issues at play here, one to be clarified by the\n> > DRM maintainers, the other more technically involved with the\n> > definition of the Bayer formats themselves.\n> > \n> > - Does DRM want RAW Bayer formats to be listed here, as these are not\n> > typically 'graphic' formats. What's the DRM maintainers opinion here ?\n> \n> To give some context, the \"historical mistake\" I keep referring to\n> regarding V4L2 is the decision to combine the bit depth of raw formats\n> with the colour filter array (a.k.a. Bayer) pattern into a fourcc. I\n> think we should have defined raw pixel formats that only encode a bit\n> depth, and conveyed the CFA pattern out-of-band. This is especially the\n> case for media bus codes (formats on the buses between hardware\n> devices). The reasoning here is that most devices only care about the\n> bit depth and not the CFA pattern. Adding a new CFA pattern (for\n> instance for RGB+Ir) for a camera sensor would currently require adding\n> a new media bus code (easy), using it in the sensor driver (obvious),\n> and patching *every* CSI-2 receiver driver to pass it through. Userspace\n> would similarly need to grow support for the new format, even for\n> userspace code that only cares about capturing the raw data without\n> processing the colour components. Having raw pixel formats that don't\n> convey the CFA pattern would simplify all this (and it's something I'm\n> considering adding to the media bus codes).\n> part is lots of churn\n\nSo both drm fourcc and modifer formats are meant to be entirely opaque and\ncomplete. Which means if you take them from one driver (whether userspace\nor kernel driver shouldn't matter) and pass it to another, together with\nthe height, width (in pixels) and the pitch (in linearized bytes, people\ngot confused and wanted to do tile row pitch here and create and absolute\nmess) must _fully_ describe the buffer for the other side.\n\nWhich would mean the bayer pattern would need to be in the modifier. Same\nway we have BGRA and RGBA and all these swizzles of the same stuff.\n\nBut also if these are purely internal to libcamera I guess we can make\nexceptions, after all we do have _R8 and _RG8 formats too, and those have\nnothing to do with red/green colors - it's simply historical naming for\nthe first/second color channel.\n\n> > - The RAW Bayer pattern ordering: we have realized that the idea of\n> > defining RAW Bayer formats by encoding the components ordering in\n> > V4L2 was, in retrospective, a bad idea. The reason is that any\n> > change in the Bayer order along the capture pipeline (usually due to\n> > flip) requires reconfiguration of the formats along the whole\n> > pipeline, requiring a start/stop of the capture operations.\n> \n> That's also a reason, yes.\n> \n> > For this reason, we have multiple times discussed the idea of only\n> > conveying the bit depth in the format definition and to communicate\n> > the components ordering with other out-of-band mechanism, that will\n> > likely be used to negotiate between the image sensor and the CSI-2\n> > receiver.\n> > \n> > With DRM the ideal would be something like\n> > \n> > DRM_FORMAT_RAW8 | FORMAT_MODIFIER_SBGGR\n> > DRM_FORMAT_RAW8 | FORMAT_MODIFIER_SGBRG\n> > ...\n> > \n> > DRM_FORMAT_RAW10 | FORMAT_MODIFIER_SBGGR\n> > DRM_FORMAT_RAW10 | FORMAT_MODIFIER_SGBRG\n> > ...\n> \n> When I said out-of-band, I meant really out of band from a format point\n> of view, so neither in the 4CC nor in the modifiers.\n\nSee above, but drm fourcc don't have flags. Yes we have the\nbig/little-endian flag, and it's absolute lolz and mostly doesn't work.\n\nAlso _RAW8 exist already, it's R8, again see above for the historical\nreasons why gl/vk call them like that.\n\n> > This however prevents to use additional modifiers to convey, in\n> > example, packaging (ie MIPI CSI-2) or compressions (like done here\n> > for PISP_COMP1) as, as far as I understand it, multiple modifiers\n> > cannot be applied to a format.\n> > \n> > However, as I understand the argument of not repeating the V4L2\n> > mistakes here, the DRM formats are aimed to be used mostly to\n> > represent the formats of images as they get stored in application\n> > buffers (at least from a libcamera perspective). The drawback of\n> > having to reconfigure the capture pipeline due to a flip does not\n> > really apply here, actually, knowing the components ordering in the\n> > final image is very relevant for applications to know how to\n> > interpret the image data.\n> > \n> > TL;DR I'm not too concerned about having to propagate the components\n> > ordering along the pipeline, as the DRM formats are used only to\n> > describe the image data as presented to the applications and not\n> > along the pipeline as it happens for the V4L2 formats.\n> > \n> > Opinions anyone ?\n> > \n> > > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > > - GRBG16_PISP_COMP1:\n> > > > > fourcc: DRM_FORMAT_SGRBG16\n> > > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > > - GBRG16_PISP_COMP1:\n> > > > > fourcc: DRM_FORMAT_SGBRG16\n> > > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > > - BGGR16_PISP_COMP1:\n> > > > > fourcc: DRM_FORMAT_SBGGR16\n> > > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > > - MONO_PISP_COMP1:\n> > > > > fourcc: DRM_FORMAT_R16\n> > > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > >\n> > > > > See\n> > > > > https://patchwork.libcamera.org/patch/19503/\n> > > > >\n> > > > > Would if be acceptable for DRM to include the above proposed modifiers for the\n> > > > > purpose of defining the above presented libcamera formats ? There will be no\n> > > > > graphic format associated with these modifiers as their purpose it not\n> > > > > displaying images but rather exchange them between the components of the\n> > > > > camera subsystem (and possibly be inspected by specialized test applications).\n> > > >\n> > > > Yeah I think libcamera using drm-fourcc formats and modifiers is\n> > > > absolutely ok, and has my ack in principle. And for these users we're\n> > > > ok with merging modifiers that the kernel doesn't use.\n> > > >\n> > > > I think it would be really good to formalize this by adding libcamera\n> > > > to the officially listed users in the \"Open Source User Waiver\"\n> > > > section in the drm_fourcc.h docs:\n> > > >\n> > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#open-source-user-waiver\n> > >\n> > > The waiver states \"the usual requirement for an upstream in-kernel or\n> > > open source userspace user does not apply\". Strictly speaking, there is\n> > > an open-source userspace user with libcamera. It's only on the kernel\n> > > side that the new modifier my not get used.\n\nSo the usual requirement here means for merging a drm driver into the\nupstream drm subsystem. We might want to link to that section explicitly:\n\nhttps://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements\n\nAnd yes this exception is for proprietary gl/vk implementations, which\nwould not apply to upstream libcamera.\n\nBut my understanding is that libcamera does allow for proprierty\nextensions (out-of-tree), so personally I fell like adding libcamera\nuserspace to this exception list makes sense. I think allowing this would\nhelp to push libcamera towards an actual industry standard that hopefully\neveryone can and will use.\n\nBut that's just my take, I'm happy to go with whatever libcamera people\nwant here.\n\n> > > This being said, I'm fine clarifying the documentation, clarity is\n> > > always good.\n\nAbsolutely :-)\n-Sima\n\n> > >\n> > > > You might want to convert that into a list, it could get a bit\n> > > > confusing. Then we can get that patch properly acked (by kernel and\n> > > > libcamera folks) to record the community consensus.\n> > > >\n> > > > For the rpi modifiers themselves: They need to be properly documented,\n> > > > least to exclude a screw-up like with the rpi modifiers we already\n> > > > have, which unfortunately encode the buffer height (instead of just\n> > > > the rounding algorithim to align the height to the right tile size) in\n> > > > the modifiers, which breaks assumptions everywhere. For details see\n> > > > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057\n> > > >\n> > > > > ---\n> > > > > include/uapi/drm/drm_fourcc.h | 5 +++++\n> > > > > 1 file changed, 5 insertions(+)\n> > > > >\n> > > > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h\n> > > > > index 00db00083175..09b182a959ad 100644\n> > > > > --- a/include/uapi/drm/drm_fourcc.h\n> > > > > +++ b/include/uapi/drm/drm_fourcc.h\n> > > > > @@ -425,6 +425,7 @@ extern \"C\" {\n> > > > > #define DRM_FORMAT_MOD_VENDOR_ARM 0x08\n> > > > > #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09\n> > > > > #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a\n> > > > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b\n> > > > >\n> > > > > /* add more to the end as needed */\n> > > > >\n> > > > > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)\n> > > > > #define AMD_FMT_MOD_CLEAR(field) \\\n> > > > > (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))\n> > > > >\n> > > > > +/* RPI (Raspberry Pi) modifiers */\n> > > > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)\n> > > > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)\n> > > > > +\n> > > > > #if defined(__cplusplus)\n> > > > > }\n> > > > > #endif\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D0187BE080\n\tfor ;\n\tThu, 29 Feb 2024 10:39:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31ED461C96;\n\tThu, 29 Feb 2024 11:39:30 +0100 (CET)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 489C261C96\n\tfor ;\n\tThu, 29 Feb 2024 11:39:28 +0100 (CET)","by mail-lj1-x22f.google.com with SMTP id\n\t38308e7fff4ca-2d2c8c1b76cso1697411fa.1\n\tfor ;\n\tThu, 29 Feb 2024 02:39:28 -0800 (PST)","from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa])\n\tby smtp.gmail.com with ESMTPSA id\n\tdx14-20020a05600c63ce00b004129f28e2cdsm4758618wmb.3.2024.02.29.02.39.26\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Feb 2024 02:39:26 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ffwll.ch header.i=@ffwll.ch header.b=\"HU2D7KYB\";\n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ffwll.ch; s=google; t=1709203167; x=1709807967;\n\tdarn=lists.libcamera.org; \n\th=in-reply-to:content-disposition:mime-version:references:message-id\n\t:subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; \n\tbh=TWM/OCTvKguUbfTK3xbthPcnGygmFUOk/8tkxLxPjOQ=;\n\tb=HU2D7KYB+sRjryR39aQ4wNNafjAnzx+tr4h54w2JAd0l7zAaASJW0l7XKgdPRZvvYy\n\tGSdqzxH43YW64kwiJIadiAGKskYWpeDaN86pMLNdDMglwnxEopNPOebYU26DfXGRXOot\n\toBJ1woYkqoiOkvSjqkxZJvW2Y5ZsFjAfYi2/E=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1709203167; x=1709807967;\n\th=in-reply-to:content-disposition:mime-version:references:message-id\n\t:subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=TWM/OCTvKguUbfTK3xbthPcnGygmFUOk/8tkxLxPjOQ=;\n\tb=OLP98auXYtAfUun7X3+NVPVk3OFWqkic3mTpziNXMCci1CNtfWc0ADhn/EJ82ftw5q\n\tiC70rksjM2T9UOEr8F/nCo5PdDU14y59Vtd5HYuRu7FK/I8Ibb0CXo9sRpRAmlK5TM/V\n\tkiXNXTixKiF3z7Dgt7SrYn4X6lZB3OHXLEwFPOU+/2kVzK1UtY5Dd2oGGY7f2iz4HDFW\n\tXbPWdGTKL3qaHa1kOIH1yeEhmAxzuDl8cCb0XB57j4iykNVh+LpovJO1+k2mK9kCrbCV\n\txFhq2bPEHcPWcgyWy7XQ0jbcfs67azuad95h85ArQ95vr/edy6Zy/a6U+LHjfQIskNY/\n\tWekQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXyHl1+ruYmo9ChBWp6C04fp3hAdtjPWFVkE8nXC1QE0LEVG+51k2bC2u0/MWrH89bGwmJK3jSw9u5Owc+2FE/sq0oGDGxbPgT54UrBwkwXo5hdbA==","X-Gm-Message-State":"AOJu0YyY730b6rmMja9pN+jKAkoRaKUsYUv7e1sMwDu5CDbXJ0yev7bS\n\tn0E5mHAjG2Ut3VzmqlUiVBkC4pk/EA/hjGs6RDP1UfHE2aohg33+rvu4H3WSZWY=","X-Google-Smtp-Source":"AGHT+IG1qi9mb2nvTpTU3i05j5epsYRlb9+JM5P+OjFdNZf0OWHnCeOXX9TaHVDFNo5SmBdY5+A+qQ==","X-Received":"by 2002:a05:651c:2123:b0:2d3:17e6:3b3b with SMTP id\n\ta35-20020a05651c212300b002d317e63b3bmr98019ljq.0.1709203167145; \n\tThu, 29 Feb 2024 02:39:27 -0800 (PST)","Date":"Thu, 29 Feb 2024 11:39:24 +0100","From":"Daniel Vetter ","To":"Laurent Pinchart ","Subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","Message-ID":"","References":"<20240226153854.99471-1-jacopo.mondi@ideasonboard.com>\n\t\n\t<20240227130827.GA5863@pendragon.ideasonboard.com>\n\t\n\t<20240228111345.GK3419@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240228111345.GK3419@pendragon.ideasonboard.com>","X-Operating-System":"Linux phenom 6.6.11-amd64 ","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Thomas Zimmermann ,\n\tMaarten Lankhorst ,\n\tMaxime Ripard , libcamera-devel@lists.libcamera.org, \n\tdri-devel@lists.freedesktop.org, Daniel Vetter ,\n\tJacopo Mondi ,\n\tDavid Airlie ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28802,"web_url":"https://patchwork.libcamera.org/comment/28802/","msgid":"<20240229105016.GC30889@pendragon.ideasonboard.com>","date":"2024-02-29T10:50:16","subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Feb 29, 2024 at 11:39:24AM +0100, Daniel Vetter wrote:\n> On Wed, Feb 28, 2024 at 01:13:45PM +0200, Laurent Pinchart wrote:\n> > On Wed, Feb 28, 2024 at 11:41:57AM +0100, Jacopo Mondi wrote:\n> > > On Tue, Feb 27, 2024 at 03:08:27PM +0200, Laurent Pinchart wrote:\n> > > > On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote:\n> > > > > On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi wrote:\n> > > > > >\n> > > > > > Add modifiers for the Raspberry Pi PiSP compressed formats.\n> > > > > >\n> > > > > > The compressed formats are documented at:\n> > > > > > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst\n> > > > > >\n> > > > > > and in the PiSP datasheet:\n> > > > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > >\n> > > > > > Background:\n> > > > > > -----------\n> > > > > >\n> > > > > > The Raspberry Pi PiSP camera subsystem is on its way to upstream through the\n> > > > > > Video4Linux2 subsystem:\n> > > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310\n> > > > > >\n> > > > > > The PiSP camera system is composed by a \"Front End\" and a \"Back End\".\n> > > > > > The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory and\n> > > > > > produce statistics, and the BackEnd is a memory-to-memory ISP that converts\n> > > > > > images in a format usable by application.\n> > > > > >\n> > > > > > The \"FrontEnd\" is capable of encoding RAW Bayer images as received by the\n> > > > > > image sensor in a 'compressed' format defined by Raspberry Pi and fully\n> > > > > > documented in the PiSP manual:\n> > > > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf\n> > > > > >\n> > > > > > The compression scheme is documented in the in-review patch series for the BE\n> > > > > > support at:\n> > > > > > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mondi@ideasonboard.com/\n> > > > > >\n> > > > > > The \"BackEnd\" is capable of consuming images in the compressed format and\n> > > > > > optionally user application might want to inspect those images for debugging\n> > > > > > purposes.\n> > > > > >\n> > > > > > Why a DRM modifier\n> > > > > > ------------------\n> > > > > >\n> > > > > > The PiSP support is entirely implemented in libcamera, with the support of an\n> > > > > > hw-specific library called 'libpisp'.\n> > > > > >\n> > > > > > libcamera uses the fourcc codes defined by DRM to define its formats:\n> > > > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml\n> > > > > >\n> > > > > > And to define a new libcamera format for the Raspberry Pi compressed ones we\n> > > > > > need to associate the above proposed modifiers with a RAW Bayer format\n> > > > > > identifier.\n> > > > > >\n> > > > > > In example:\n> > > > > >\n> > > > > > - RGGB16_PISP_COMP1:\n> > > > > > fourcc: DRM_FORMAT_SRGGB16\n> > > >\n> > > > An \"interesting\" issue here is that these formats currently live in\n> > > > libcamera only, we haven't merged them in DRM \"yet\". This may be a\n> > > > prerequisite ?\n> > > >\n> > > \n> > > Ah right! I didn't notice!\n> > > \n> > > I think there are two issues at play here, one to be clarified by the\n> > > DRM maintainers, the other more technically involved with the\n> > > definition of the Bayer formats themselves.\n> > > \n> > > - Does DRM want RAW Bayer formats to be listed here, as these are not\n> > > typically 'graphic' formats. What's the DRM maintainers opinion here ?\n> > \n> > To give some context, the \"historical mistake\" I keep referring to\n> > regarding V4L2 is the decision to combine the bit depth of raw formats\n> > with the colour filter array (a.k.a. Bayer) pattern into a fourcc. I\n> > think we should have defined raw pixel formats that only encode a bit\n> > depth, and conveyed the CFA pattern out-of-band. This is especially the\n> > case for media bus codes (formats on the buses between hardware\n> > devices). The reasoning here is that most devices only care about the\n> > bit depth and not the CFA pattern. Adding a new CFA pattern (for\n> > instance for RGB+Ir) for a camera sensor would currently require adding\n> > a new media bus code (easy), using it in the sensor driver (obvious),\n> > and patching *every* CSI-2 receiver driver to pass it through. Userspace\n> > would similarly need to grow support for the new format, even for\n> > userspace code that only cares about capturing the raw data without\n> > processing the colour components. Having raw pixel formats that don't\n> > convey the CFA pattern would simplify all this (and it's something I'm\n> > considering adding to the media bus codes).\n> > part is lots of churn\n> \n> So both drm fourcc and modifer formats are meant to be entirely opaque and\n> complete. Which means if you take them from one driver (whether userspace\n> or kernel driver shouldn't matter) and pass it to another, together with\n> the height, width (in pixels) and the pitch (in linearized bytes, people\n> got confused and wanted to do tile row pitch here and create and absolute\n> mess) must _fully_ describe the buffer for the other side.\n\nHow full does fully mean ? :-) For instance, one thing that is not\nconveyed by the format today is colourspace information. It is\ncommunicated out-of-band, and if the source and sink have different\nconfigurations, you'll get sub-optimal results. Same thing for\nmonochrome formats, R8 could store visible light data, infrared data or\ndepth data.\n\nGranted, when it comes to the colour filter array pattern the\n\"suboptimal\" results will be quite significantly more suboptimal than\nwhat you'll get from mismatches in colourspaces.\n\n> Which would mean the bayer pattern would need to be in the modifier. Same\n> way we have BGRA and RGBA and all these swizzles of the same stuff.\n> \n> But also if these are purely internal to libcamera I guess we can make\n> exceptions, after all we do have _R8 and _RG8 formats too, and those have\n> nothing to do with red/green colors - it's simply historical naming for\n> the first/second color channel.\n> \n> > > - The RAW Bayer pattern ordering: we have realized that the idea of\n> > > defining RAW Bayer formats by encoding the components ordering in\n> > > V4L2 was, in retrospective, a bad idea. The reason is that any\n> > > change in the Bayer order along the capture pipeline (usually due to\n> > > flip) requires reconfiguration of the formats along the whole\n> > > pipeline, requiring a start/stop of the capture operations.\n> > \n> > That's also a reason, yes.\n> > \n> > > For this reason, we have multiple times discussed the idea of only\n> > > conveying the bit depth in the format definition and to communicate\n> > > the components ordering with other out-of-band mechanism, that will\n> > > likely be used to negotiate between the image sensor and the CSI-2\n> > > receiver.\n> > > \n> > > With DRM the ideal would be something like\n> > > \n> > > DRM_FORMAT_RAW8 | FORMAT_MODIFIER_SBGGR\n> > > DRM_FORMAT_RAW8 | FORMAT_MODIFIER_SGBRG\n> > > ...\n> > > \n> > > DRM_FORMAT_RAW10 | FORMAT_MODIFIER_SBGGR\n> > > DRM_FORMAT_RAW10 | FORMAT_MODIFIER_SGBRG\n> > > ...\n> > \n> > When I said out-of-band, I meant really out of band from a format point\n> > of view, so neither in the 4CC nor in the modifiers.\n> \n> See above, but drm fourcc don't have flags. Yes we have the\n> big/little-endian flag, and it's absolute lolz and mostly doesn't work.\n> \n> Also _RAW8 exist already, it's R8, again see above for the historical\n> reasons why gl/vk call them like that.\n> \n> > > This however prevents to use additional modifiers to convey, in\n> > > example, packaging (ie MIPI CSI-2) or compressions (like done here\n> > > for PISP_COMP1) as, as far as I understand it, multiple modifiers\n> > > cannot be applied to a format.\n> > > \n> > > However, as I understand the argument of not repeating the V4L2\n> > > mistakes here, the DRM formats are aimed to be used mostly to\n> > > represent the formats of images as they get stored in application\n> > > buffers (at least from a libcamera perspective). The drawback of\n> > > having to reconfigure the capture pipeline due to a flip does not\n> > > really apply here, actually, knowing the components ordering in the\n> > > final image is very relevant for applications to know how to\n> > > interpret the image data.\n> > > \n> > > TL;DR I'm not too concerned about having to propagate the components\n> > > ordering along the pipeline, as the DRM formats are used only to\n> > > describe the image data as presented to the applications and not\n> > > along the pipeline as it happens for the V4L2 formats.\n> > > \n> > > Opinions anyone ?\n> > > \n> > > > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > > > - GRBG16_PISP_COMP1:\n> > > > > > fourcc: DRM_FORMAT_SGRBG16\n> > > > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > > > - GBRG16_PISP_COMP1:\n> > > > > > fourcc: DRM_FORMAT_SGBRG16\n> > > > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > > > - BGGR16_PISP_COMP1:\n> > > > > > fourcc: DRM_FORMAT_SBGGR16\n> > > > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > > > - MONO_PISP_COMP1:\n> > > > > > fourcc: DRM_FORMAT_R16\n> > > > > > mod: PISP_FORMAT_MOD_COMPRESS_MODE1\n> > > > > >\n> > > > > > See\n> > > > > > https://patchwork.libcamera.org/patch/19503/\n> > > > > >\n> > > > > > Would if be acceptable for DRM to include the above proposed modifiers for the\n> > > > > > purpose of defining the above presented libcamera formats ? There will be no\n> > > > > > graphic format associated with these modifiers as their purpose it not\n> > > > > > displaying images but rather exchange them between the components of the\n> > > > > > camera subsystem (and possibly be inspected by specialized test applications).\n> > > > >\n> > > > > Yeah I think libcamera using drm-fourcc formats and modifiers is\n> > > > > absolutely ok, and has my ack in principle. And for these users we're\n> > > > > ok with merging modifiers that the kernel doesn't use.\n> > > > >\n> > > > > I think it would be really good to formalize this by adding libcamera\n> > > > > to the officially listed users in the \"Open Source User Waiver\"\n> > > > > section in the drm_fourcc.h docs:\n> > > > >\n> > > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#open-source-user-waiver\n> > > >\n> > > > The waiver states \"the usual requirement for an upstream in-kernel or\n> > > > open source userspace user does not apply\". Strictly speaking, there is\n> > > > an open-source userspace user with libcamera. It's only on the kernel\n> > > > side that the new modifier my not get used.\n> \n> So the usual requirement here means for merging a drm driver into the\n> upstream drm subsystem. We might want to link to that section explicitly:\n> \n> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements\n> \n> And yes this exception is for proprietary gl/vk implementations, which\n> would not apply to upstream libcamera.\n> \n> But my understanding is that libcamera does allow for proprierty\n> extensions (out-of-tree),\n\nFor the sake of completeness, those extensions are not allowed to define\nnew pixel formats at the moment. I don't envision changing that, but I\ncan't fully rule it out either.\n\n> so personally I fell like adding libcamera\n> userspace to this exception list makes sense. I think allowing this would\n> help to push libcamera towards an actual industry standard that hopefully\n> everyone can and will use.\n> \n> But that's just my take, I'm happy to go with whatever libcamera people\n> want here.\n> \n> > > > This being said, I'm fine clarifying the documentation, clarity is\n> > > > always good.\n> \n> Absolutely :-)\n> \n> > > >\n> > > > > You might want to convert that into a list, it could get a bit\n> > > > > confusing. Then we can get that patch properly acked (by kernel and\n> > > > > libcamera folks) to record the community consensus.\n> > > > >\n> > > > > For the rpi modifiers themselves: They need to be properly documented,\n> > > > > least to exclude a screw-up like with the rpi modifiers we already\n> > > > > have, which unfortunately encode the buffer height (instead of just\n> > > > > the rounding algorithim to align the height to the right tile size) in\n> > > > > the modifiers, which breaks assumptions everywhere. For details see\n> > > > > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057\n> > > > >\n> > > > > > ---\n> > > > > > include/uapi/drm/drm_fourcc.h | 5 +++++\n> > > > > > 1 file changed, 5 insertions(+)\n> > > > > >\n> > > > > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h\n> > > > > > index 00db00083175..09b182a959ad 100644\n> > > > > > --- a/include/uapi/drm/drm_fourcc.h\n> > > > > > +++ b/include/uapi/drm/drm_fourcc.h\n> > > > > > @@ -425,6 +425,7 @@ extern \"C\" {\n> > > > > > #define DRM_FORMAT_MOD_VENDOR_ARM 0x08\n> > > > > > #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09\n> > > > > > #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a\n> > > > > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b\n> > > > > >\n> > > > > > /* add more to the end as needed */\n> > > > > >\n> > > > > > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)\n> > > > > > #define AMD_FMT_MOD_CLEAR(field) \\\n> > > > > > (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))\n> > > > > >\n> > > > > > +/* RPI (Raspberry Pi) modifiers */\n> > > > > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)\n> > > > > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)\n> > > > > > +\n> > > > > > #if defined(__cplusplus)\n> > > > > > }\n> > > > > > #endif","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 68E0EBD160\n\tfor ;\n\tThu, 29 Feb 2024 10:50:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79F2F6286D;\n\tThu, 29 Feb 2024 11:50:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F4A461C96\n\tfor ;\n\tThu, 29 Feb 2024 11:50:15 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4BBFC673;\n\tThu, 29 Feb 2024 11:50:01 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"M2Xifn5x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709203801;\n\tbh=T6HDp07g53FgkZxh43BprMdnvxBuAQft+ClHktdNyrk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M2Xifn5xziofhKV2CCHALmh4sFy0ETSjFot9Niwwqr8oKiNCB9cfDAc1T4opZkjWG\n\tQc2rkGVWkLnN6IXTd2oJ/8L+0JKT0PFv704OG4kwRhRrcsWzwIak6vIczISvmMUS4a\n\tdmNfCO/KRp3+SzhYIoiFTsXvz7ogjIlkPMm3cC5U=","Date":"Thu, 29 Feb 2024 12:50:16 +0200","From":"Laurent Pinchart ","To":"Daniel Vetter ","Subject":"Re: [RFC] drm/fourcc: Add RPI modifiers","Message-ID":"<20240229105016.GC30889@pendragon.ideasonboard.com>","References":"<20240226153854.99471-1-jacopo.mondi@ideasonboard.com>\n\t\n\t<20240227130827.GA5863@pendragon.ideasonboard.com>\n\t\n\t<20240228111345.GK3419@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maarten Lankhorst ,\n\tMaxime Ripard , libcamera-devel@lists.libcamera.org, \n\tdri-devel@lists.freedesktop.org,\n\tThomas Zimmermann , \n\tJacopo Mondi ,\n\tDavid Airlie ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]