[{"id":26180,"web_url":"https://patchwork.libcamera.org/comment/26180/","msgid":"<20230104131229.uwwchptzslflzyjr@uno.localdomain>","date":"2023-01-04T13:12:29","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David\n   sorry to revive this, but I'm carrying this series in my tree and I\njust found a bug in this patches\n\n\nOn Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:\n> From: David Plowman <david.plowman@raspberrypi.com>\n>\n> This makes it easier to perform transformations on Bayer type mbus\n> codes by converting them to a BayerFormat, doing the transform, and\n> then converting them back again.\n>\n> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  include/libcamera/internal/bayer_format.h |  1 +\n>  src/libcamera/bayer_format.cpp            | 11 +++++++++++\n>  2 files changed, 12 insertions(+)\n>\n> diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> index 78ba3969913d..3601dccb7228 100644\n> --- a/include/libcamera/internal/bayer_format.h\n> +++ b/include/libcamera/internal/bayer_format.h\n> @@ -47,6 +47,7 @@ public:\n>  \t}\n>\n>  \tstatic const BayerFormat &fromMbusCode(unsigned int mbusCode);\n> +\tunsigned int toMbusCode() const;\n>  \tbool isValid() const { return bitDepth != 0; }\n>\n>  \tstd::string toString() const;\n> diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> index f27cc1662d25..fdbc4af1dcc0 100644\n> --- a/src/libcamera/bayer_format.cpp\n> +++ b/src/libcamera/bayer_format.cpp\n> @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)\n>  \t\treturn it->second;\n>  }\n>\n> +/**\n> + * \\brief Retrieve the media bus code corresponding this this BayerFormat\n> + * \\return The corresponding media bus code, or zero if none was found\n> + */\n> +unsigned int BayerFormat::toMbusCode() const\n> +{\n> +\tauto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),\n> +\t\t\t       [this](const auto &i) { return i.second == *this; });\n> +\treturn it != mbusCodeToBayer.end() ? it->first : 0;\n\nI'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10\nIt doesn't register flips, so the bayer pattern should not be changed\n\nThe code you added in the next patch for the CameraSensor class goes\nas\n\n\tfor (const auto &format : formats) {\n\t\tunsigned int mbusCode = format.first;\n\t\tBayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n\n\t\tif (bayerFormat.isValid())\n\t\t\tmbusCode = bayerFormat.transform(transform).toMbusCode();\n\n\t\tif (mbusCode)\n\t\t\tformats_[mbusCode] = std::move(format.second);\n\t}\n\n\nThe first\n\n\t\tBayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n\nreturns  { BayerFormat::BGGR, 10, BayerFormat::Packing::None }\n\nwe go throgh a transform() which does nothing as it receives Indentity\nand then call toMbusCode() on it\n\nThe above implementation of toMbusCode() walks the mbusCodeToBayer map\nlooking for a matching bayer. Look at this table section\n\n\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n\t{ MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n\nSo here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and\nMEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same\n{ BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly\nget my original 0x3007 code transformed to 0x3003!\n\nI'm afraid the fact mbus_code-to-BayerFormat relationship is not\nunique is a blocker for this patch\n\n\n\n\n\n> +}\n> +\n>  /**\n>   * \\fn BayerFormat::isValid()\n>   * \\brief Return whether a BayerFormat is valid\n> --\n> 2.38.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 94ABEC322E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  4 Jan 2023 13:12:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F18DA625DE;\n\tWed,  4 Jan 2023 14:12:33 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C620603C5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  4 Jan 2023 14:12:31 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id C4F2A240011;\n\tWed,  4 Jan 2023 13:12:30 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1672837954;\n\tbh=61T1sIfUZJzS6CwS1RsUH2YXXgXeDVHiy0h67x3WNBs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=S0luPoARKig5X8dclJqPDQ+FiUHw3fa6vW+eVbL127WZzFaVOehNZv7PiBneY8NPG\n\t5b4C5XHtiensRfDOS46h2bDDIZWp2V1Nr8TjoHj07qzVaQlGQGd82R6VFcLLoddBH/\n\tFG8CSWYucP/RqwKKvWWhevKI1HL/cEkgnNpIkvUrH8+VjrbJU/YSk/Hr0+D4G1lvBn\n\tWYhFmUcUbYOdKjNZN6wLoT+xhZwCB0ZE2tgj1gBtolDzf0T5tm6R1kw/SQTsO/nQww\n\thMmkntsJXfKD9ILuGTR1V+FqjQsgNa551emHbowxp1Yq87AC+RiAFYr8WE9hNTVAPB\n\t402VBsPiUaflQ==","Date":"Wed, 4 Jan 2023 14:12:29 +0100","To":"libcamera-devel@lists.libcamera.org","Message-ID":"<20230104131229.uwwchptzslflzyjr@uno.localdomain>","References":"<20221124121220.47000-1-jacopo@jmondi.org>\n\t<20221124121220.47000-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221124121220.47000-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo@jmondi.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26192,"web_url":"https://patchwork.libcamera.org/comment/26192/","msgid":"<CAHW6GYL26Ws3fPWWp0P60-PMn4nz-5Onr8ba0V0-YL3iGwT1Ug@mail.gmail.com>","date":"2023-01-06T16:20:56","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@jmondi.org> wrote:\n>\n> Hi David\n>    sorry to revive this, but I'm carrying this series in my tree and I\n> just found a bug in this patches\n>\n>\n> On Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:\n> > From: David Plowman <david.plowman@raspberrypi.com>\n> >\n> > This makes it easier to perform transformations on Bayer type mbus\n> > codes by converting them to a BayerFormat, doing the transform, and\n> > then converting them back again.\n> >\n> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  include/libcamera/internal/bayer_format.h |  1 +\n> >  src/libcamera/bayer_format.cpp            | 11 +++++++++++\n> >  2 files changed, 12 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> > index 78ba3969913d..3601dccb7228 100644\n> > --- a/include/libcamera/internal/bayer_format.h\n> > +++ b/include/libcamera/internal/bayer_format.h\n> > @@ -47,6 +47,7 @@ public:\n> >       }\n> >\n> >       static const BayerFormat &fromMbusCode(unsigned int mbusCode);\n> > +     unsigned int toMbusCode() const;\n> >       bool isValid() const { return bitDepth != 0; }\n> >\n> >       std::string toString() const;\n> > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> > index f27cc1662d25..fdbc4af1dcc0 100644\n> > --- a/src/libcamera/bayer_format.cpp\n> > +++ b/src/libcamera/bayer_format.cpp\n> > @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)\n> >               return it->second;\n> >  }\n> >\n> > +/**\n> > + * \\brief Retrieve the media bus code corresponding this this BayerFormat\n> > + * \\return The corresponding media bus code, or zero if none was found\n> > + */\n> > +unsigned int BayerFormat::toMbusCode() const\n> > +{\n> > +     auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),\n> > +                            [this](const auto &i) { return i.second == *this; });\n> > +     return it != mbusCodeToBayer.end() ? it->first : 0;\n>\n> I'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10\n> It doesn't register flips, so the bayer pattern should not be changed\n>\n> The code you added in the next patch for the CameraSensor class goes\n> as\n>\n>         for (const auto &format : formats) {\n>                 unsigned int mbusCode = format.first;\n>                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n>\n>                 if (bayerFormat.isValid())\n>                         mbusCode = bayerFormat.transform(transform).toMbusCode();\n>\n>                 if (mbusCode)\n>                         formats_[mbusCode] = std::move(format.second);\n>         }\n>\n>\n> The first\n>\n>                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n>\n> returns  { BayerFormat::BGGR, 10, BayerFormat::Packing::None }\n>\n> we go throgh a transform() which does nothing as it receives Indentity\n> and then call toMbusCode() on it\n>\n> The above implementation of toMbusCode() walks the mbusCodeToBayer map\n> looking for a matching bayer. Look at this table section\n>\n>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n>         { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n>\n> So here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and\n> MEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same\n> { BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly\n> get my original 0x3007 code transformed to 0x3003!\n>\n> I'm afraid the fact mbus_code-to-BayerFormat relationship is not\n> unique is a blocker for this patch\n\nOh dear! Thanks for discovering this, I had no idea that these formats existed.\n\nI wonder if the correct solution is to have 4 extra packing formats,\nalongside the existing CSI2 and IPU3 ones. What do you think?\n\nDavid\n\n>\n>\n>\n>\n>\n> > +}\n> > +\n> >  /**\n> >   * \\fn BayerFormat::isValid()\n> >   * \\brief Return whether a BayerFormat is valid\n> > --\n> > 2.38.1\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 45347BD78E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  6 Jan 2023 16:21:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93C85625CF;\n\tFri,  6 Jan 2023 17:21:11 +0100 (CET)","from mail-oa1-x31.google.com (mail-oa1-x31.google.com\n\t[IPv6:2001:4860:4864:20::31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 30ABD625CE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  6 Jan 2023 17:21:09 +0100 (CET)","by mail-oa1-x31.google.com with SMTP id\n\t586e51a60fabf-1433ef3b61fso1973582fac.10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 06 Jan 2023 08:21:09 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673022071;\n\tbh=kNi42mIuJ1E+yXRFbxjQV8X+2XNT6BpFbb+u171m8oc=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=nbGST+gZtT1tNlS6lX71HaP502tiryemhO07wxfnn/RU564Sx617byklqAh3p49mc\n\t5vs/QgCeB4k5pSGwRLnPwtP1RVZqdlpSxYnDHuHoAp5i7zX6rJprFJbY4eSAX5wClg\n\t8q2HvUD6SsGCkn328UeaHkBUAWrYXcLedWsBHvQNuRd+/ilA94IKcSK5APfSrDB5SR\n\tg25X17uT2uGfWjfzly66cztHIkTsWOmd0Z2Rf44h/GeOMr5kGCq7OndAAWYKiBpPJV\n\t61H6Foha9mUN0hb2t77WzJOOfxG5pskwrTFLlGBTpN6T85SCWDJlYP+3DEt+/c5LBD\n\tkTPi41PuxGJlA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=JmLIrjne3ElvOAx8EPZQPpPF6VCxTfMWVCx5raozLjg=;\n\tb=Asp3j4Q2b5uoZGcWF+NXmpj93YDiiBD1qriYawUUVqv/nQx/Pmd3HBmInAJlJXqwhp\n\tBKzE4kPDeO7AUdYvRQdA7esp9X8gCCrBl+wmiSHJt7wwe31GhWqCX0Sq1gAlYZ1nwGlN\n\tCo9s/QF7n+Y6Rv2hRpdAhNenC+9yK5phi8okXLacAHqzFk6U3oyAAlt07kf5DsYMBYp3\n\tXD2YcpWIY81ILHMcx7ToLLCKNQnsRVmxYwrhmbcG7Jgxg2WijXmvpGKClZK3mH/Rl3ZH\n\t+DyyT+hxlmthH9XNj+w6HYXgK+yfNXZc5Oc5RIu/YG3VIyLYZzgZpZmG4mXaChdg+2QI\n\tI7Cw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Asp3j4Q2\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=JmLIrjne3ElvOAx8EPZQPpPF6VCxTfMWVCx5raozLjg=;\n\tb=Pag18bk520ivlQVCCRSju6Kxp4v8bVO1UClOvm4NEpDjo75T5/DEEEMFrrDmrvwGa1\n\tKBI02tZAR6V/W0IMLCy3012d3JCOBED+TGIRQly0XaXCsGML/5sHpeAmU/8D0vXitH3R\n\tKpjbmkKvEgb2/MTNFZPnoNbc/7mSNpXaY411BH6bItQb2IQZNrm+dj95MD0EKFsXYAE0\n\tgDqEWFXU83+j0iCRjk1HB2zd5sIxKh9MEpEsh9EW5qmykDOohmVi2aoDyklKAAz09d0o\n\tm6ZqcM+VPDxLIZrZEZlifiWKbAibIlPlLqR0G2Qo/h3S5r0obv9QJVDJk0Bj7LQ3pzU6\n\tx6wA==","X-Gm-Message-State":"AFqh2krP2JyE6njoQfJ7ScKtLwYytZCHszyuNFH7rU8a/3AhLC1qVlKb\n\t5rHMyyOBurnhKaBsesO4G2G+KOqfBXPJ+0EvjNwdpA==","X-Google-Smtp-Source":"AMrXdXs68qAuT9WaMFcEV32QX27yP6UriroJkSkky14pTYFshor4glHlIkFnC2729b1OYfzYRkzGO7Vy/PbFSiN4cM8=","X-Received":"by 2002:a05:6870:e2c9:b0:150:a02f:1505 with SMTP id\n\tw9-20020a056870e2c900b00150a02f1505mr1231941oad.246.1673022067681;\n\tFri, 06 Jan 2023 08:21:07 -0800 (PST)","MIME-Version":"1.0","References":"<20221124121220.47000-1-jacopo@jmondi.org>\n\t<20221124121220.47000-2-jacopo@jmondi.org>\n\t<20230104131229.uwwchptzslflzyjr@uno.localdomain>","In-Reply-To":"<20230104131229.uwwchptzslflzyjr@uno.localdomain>","Date":"Fri, 6 Jan 2023 16:20:56 +0000","Message-ID":"<CAHW6GYL26Ws3fPWWp0P60-PMn4nz-5Onr8ba0V0-YL3iGwT1Ug@mail.gmail.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26193,"web_url":"https://patchwork.libcamera.org/comment/26193/","msgid":"<CAPY8ntCy54XsgdOgG1hAXrFVSTnEaDR_x+UP7Gif6Lk2W0mn=A@mail.gmail.com>","date":"2023-01-06T17:28:26","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi David and Jacopo\n\nOn Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel\n<libcamera-devel@lists.libcamera.org> wrote:\n>\n> Hi Jacopo\n>\n> On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@jmondi.org> wrote:\n> >\n> > Hi David\n> >    sorry to revive this, but I'm carrying this series in my tree and I\n> > just found a bug in this patches\n> >\n> >\n> > On Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:\n> > > From: David Plowman <david.plowman@raspberrypi.com>\n> > >\n> > > This makes it easier to perform transformations on Bayer type mbus\n> > > codes by converting them to a BayerFormat, doing the transform, and\n> > > then converting them back again.\n> > >\n> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  include/libcamera/internal/bayer_format.h |  1 +\n> > >  src/libcamera/bayer_format.cpp            | 11 +++++++++++\n> > >  2 files changed, 12 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> > > index 78ba3969913d..3601dccb7228 100644\n> > > --- a/include/libcamera/internal/bayer_format.h\n> > > +++ b/include/libcamera/internal/bayer_format.h\n> > > @@ -47,6 +47,7 @@ public:\n> > >       }\n> > >\n> > >       static const BayerFormat &fromMbusCode(unsigned int mbusCode);\n> > > +     unsigned int toMbusCode() const;\n> > >       bool isValid() const { return bitDepth != 0; }\n> > >\n> > >       std::string toString() const;\n> > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> > > index f27cc1662d25..fdbc4af1dcc0 100644\n> > > --- a/src/libcamera/bayer_format.cpp\n> > > +++ b/src/libcamera/bayer_format.cpp\n> > > @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)\n> > >               return it->second;\n> > >  }\n> > >\n> > > +/**\n> > > + * \\brief Retrieve the media bus code corresponding this this BayerFormat\n> > > + * \\return The corresponding media bus code, or zero if none was found\n> > > + */\n> > > +unsigned int BayerFormat::toMbusCode() const\n> > > +{\n> > > +     auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),\n> > > +                            [this](const auto &i) { return i.second == *this; });\n> > > +     return it != mbusCodeToBayer.end() ? it->first : 0;\n> >\n> > I'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10\n> > It doesn't register flips, so the bayer pattern should not be changed\n> >\n> > The code you added in the next patch for the CameraSensor class goes\n> > as\n> >\n> >         for (const auto &format : formats) {\n> >                 unsigned int mbusCode = format.first;\n> >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> >\n> >                 if (bayerFormat.isValid())\n> >                         mbusCode = bayerFormat.transform(transform).toMbusCode();\n> >\n> >                 if (mbusCode)\n> >                         formats_[mbusCode] = std::move(format.second);\n> >         }\n> >\n> >\n> > The first\n> >\n> >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> >\n> > returns  { BayerFormat::BGGR, 10, BayerFormat::Packing::None }\n> >\n> > we go throgh a transform() which does nothing as it receives Indentity\n> > and then call toMbusCode() on it\n> >\n> > The above implementation of toMbusCode() walks the mbusCodeToBayer map\n> > looking for a matching bayer. Look at this table section\n> >\n> >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> >         { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> >\n> > So here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and\n> > MEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same\n> > { BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly\n> > get my original 0x3007 code transformed to 0x3003!\n> >\n> > I'm afraid the fact mbus_code-to-BayerFormat relationship is not\n> > unique is a blocker for this patch\n>\n> Oh dear! Thanks for discovering this, I had no idea that these formats existed.\n\nWhy do those formats exist, and why only the BGGR ordering versions?\nA partial implementation of something, and presumably on a parallel\ninterface as formats such as YUYV are mandated to use the 1X16\nversions (not 2X8) on CSI.\n\nAre they actually relevant, or legacy cruft?\n\n  Dave\n\n> I wonder if the correct solution is to have 4 extra packing formats,\n> alongside the existing CSI2 and IPU3 ones. What do you think?\n>\n> David\n>\n> >\n> >\n> >\n> >\n> >\n> > > +}\n> > > +\n> > >  /**\n> > >   * \\fn BayerFormat::isValid()\n> > >   * \\brief Return whether a BayerFormat is valid\n> > > --\n> > > 2.38.1\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 750B5C322E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  6 Jan 2023 17:28:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDD17625DE;\n\tFri,  6 Jan 2023 18:28:44 +0100 (CET)","from mail-vs1-xe2d.google.com (mail-vs1-xe2d.google.com\n\t[IPv6:2607:f8b0:4864:20::e2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF87A625CE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  6 Jan 2023 18:28:43 +0100 (CET)","by mail-vs1-xe2d.google.com with SMTP id l184so2173365vsc.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 06 Jan 2023 09:28:43 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673026124;\n\tbh=S+hMfN22HwlgLGHauC2aUqyaafr/ElmFcEwEYMwglZ0=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=StLqrDSfNApVFkasqxvna43WJC1yrAB/MUG61Q9PIZjhauTz0s2kp1yz18NTpx/u1\n\tzBaSMAw6rjTRuqf47vvIA8pFTP3dUuqiQfnN8X4jl8M5t4lkDcrX4gzXuZCjqd28Ad\n\tLnsbBosD8mcjvZ5UQfAlYuY1AYWS5YiIAtSgPmkZyCyPUF3mcYqF5Rcn9ln/I4BHaR\n\tAAynCG2mfx/3bLqgzI79c0Obikejgt2pfm9OX1mutITRwPSYjY6o1LnuET8o5Do3yC\n\tNE6/VSCxiAG1kG9Upr6kJuBptKGDLg4oqzRVI9YXA4evvp18UXsvUKWbAQxxfycJrm\n\t6WrWeUvsHaXvA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=2Mi6xbrSMEJwm10wb5kI895FvpvnCepKR5PKrNE+SOM=;\n\tb=fhKHFS+LRjlj8Rawc01Sp17UZLbCfti1LvRkgFy6KVcfwPT/SA39w7hjXAHHSZAgRo\n\te61agZ49oVgZUljVtCdz+rhiNwCxrf0EguQ+UDpi5nopNwzA2R+x2m2r1XMl3/qdJKha\n\tCiE058TAbb0/RoPtwr7zWY7K4bW3txYt9ElJZTcGp7cX01SLYkBuGylf/R/a60HhBQVT\n\tdtryAwFBRQud1SYuSykLTLxkxbLQV2h5zx2u83wZ/J0yA2pzY20tosRNHqsM6FIV1/I4\n\tP5HpLcmgU2WTlchG+eaY2qw+iVpxhWoiSpwTS0xfSpBjFT+H2X8SWdyOXDjqjDRlavMD\n\tIR0Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"fhKHFS+L\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=2Mi6xbrSMEJwm10wb5kI895FvpvnCepKR5PKrNE+SOM=;\n\tb=d2Cp+uqixCAftNrMVdovrqJksNOmw4qNuVON1NQZzWdvT8ksuxCb5lZ8NcMYXPIWlN\n\tRLZWEECs2So2cBagpWEwrnTwVK9aCAbH1seXvcZYCfajwF8zn8QWa4IoGevFIb6RuyEX\n\tjwlYiE1i+DCnocTRrNtPG1mJIzRbWiFuMgpGfj1Wlz8tFrFuBNt2b1aylDjfvyOIdgZk\n\tBP1m1E6ZuSAkuuqLOVc1FmeeXsUJUVoWK69WqXNep+aubr/xd5/fQonG/a+0JnP8hi/z\n\tagzkZYbLMTl0e9b1DUtvETCKC5g9AhPhS/p+Qb992IYqKiia5ul3q+JOYsLyJYCkWTgN\n\twtnA==","X-Gm-Message-State":"AFqh2kp1lDrbofwJn5euAUB7b479qT+xGjAmaSXrV3KzKIT8jBkpa4Rl\n\tUaTan3WwClVZmq59vm87bN9Fwx1+EpConGzex8zhL57JfZEbLw==","X-Google-Smtp-Source":"AMrXdXtfyVMeofThB288XWzAsNvjHs7qXcFktGc/5L/ZjqJDewkNosZQJwQaGULnTGzedgXU9H1QaFviPdOnJuCppFs=","X-Received":"by 2002:a05:6102:532:b0:3ca:ef2e:c0b6 with SMTP id\n\tm18-20020a056102053200b003caef2ec0b6mr4754544vsa.32.1673026122482;\n\tFri, 06 Jan 2023 09:28:42 -0800 (PST)","MIME-Version":"1.0","References":"<20221124121220.47000-1-jacopo@jmondi.org>\n\t<20221124121220.47000-2-jacopo@jmondi.org>\n\t<20230104131229.uwwchptzslflzyjr@uno.localdomain>\n\t<CAHW6GYL26Ws3fPWWp0P60-PMn4nz-5Onr8ba0V0-YL3iGwT1Ug@mail.gmail.com>","In-Reply-To":"<CAHW6GYL26Ws3fPWWp0P60-PMn4nz-5Onr8ba0V0-YL3iGwT1Ug@mail.gmail.com>","Date":"Fri, 6 Jan 2023 17:28:26 +0000","Message-ID":"<CAPY8ntCy54XsgdOgG1hAXrFVSTnEaDR_x+UP7Gif6Lk2W0mn=A@mail.gmail.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Dave Stevenson via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Dave Stevenson <dave.stevenson@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26205,"web_url":"https://patchwork.libcamera.org/comment/26205/","msgid":"<CAHW6GY+amBOp2H6wjm3fbYUrmvwkSDVbMSEyEj-kwyfZpOkJAQ@mail.gmail.com>","date":"2023-01-10T11:59:19","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again\n\nI was thinking about this a bit more. One thing I wanted to clarify\nwas what media bus formats really mean. For example, if I have\nMEDIA_BUS_FMT_SBGGR10_1X10, presumably that means Bayer BGGR with CSI2\npacking. Does that sound right?\n\nUnder those circumstances it seems reasonable to me that the\nmbusCodeToBayer table (bayer_format.cpp) should specify Packing::CSI2P\nfor most of the formats there, but with new values (e.g.\nPacking::PADHI_BE) for the ones now under discussion.\n\nI don't think such a change would bother Raspberry Pi (it might well\n\"just work\", or if not require only very minor tweaks), but someone\nelse would have to comment on ipu3 or rkisp1.\n\nDave also mentions that all the non-BGGR versions are missing (it\nseems a bit disappointing that they weren't added all at the same\ntime), but for the moment I suggest we ignore that.\n\nI also note there's a potential problem with the \"ALAW8\" and \"DPCM8\"\nformats. I don't know if folks think those can be handled by more\n\"packing\" formats, or whether we'd rather have a new \"modifiers\"\nfield?\n\nThoughts?\n\nDavid\n\nOn Fri, 6 Jan 2023 at 17:28, Dave Stevenson\n<dave.stevenson@raspberrypi.com> wrote:\n>\n> Hi David and Jacopo\n>\n> On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel\n> <libcamera-devel@lists.libcamera.org> wrote:\n> >\n> > Hi Jacopo\n> >\n> > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@jmondi.org> wrote:\n> > >\n> > > Hi David\n> > >    sorry to revive this, but I'm carrying this series in my tree and I\n> > > just found a bug in this patches\n> > >\n> > >\n> > > On Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:\n> > > > From: David Plowman <david.plowman@raspberrypi.com>\n> > > >\n> > > > This makes it easier to perform transformations on Bayer type mbus\n> > > > codes by converting them to a BayerFormat, doing the transform, and\n> > > > then converting them back again.\n> > > >\n> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > > ---\n> > > >  include/libcamera/internal/bayer_format.h |  1 +\n> > > >  src/libcamera/bayer_format.cpp            | 11 +++++++++++\n> > > >  2 files changed, 12 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> > > > index 78ba3969913d..3601dccb7228 100644\n> > > > --- a/include/libcamera/internal/bayer_format.h\n> > > > +++ b/include/libcamera/internal/bayer_format.h\n> > > > @@ -47,6 +47,7 @@ public:\n> > > >       }\n> > > >\n> > > >       static const BayerFormat &fromMbusCode(unsigned int mbusCode);\n> > > > +     unsigned int toMbusCode() const;\n> > > >       bool isValid() const { return bitDepth != 0; }\n> > > >\n> > > >       std::string toString() const;\n> > > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> > > > index f27cc1662d25..fdbc4af1dcc0 100644\n> > > > --- a/src/libcamera/bayer_format.cpp\n> > > > +++ b/src/libcamera/bayer_format.cpp\n> > > > @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)\n> > > >               return it->second;\n> > > >  }\n> > > >\n> > > > +/**\n> > > > + * \\brief Retrieve the media bus code corresponding this this BayerFormat\n> > > > + * \\return The corresponding media bus code, or zero if none was found\n> > > > + */\n> > > > +unsigned int BayerFormat::toMbusCode() const\n> > > > +{\n> > > > +     auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),\n> > > > +                            [this](const auto &i) { return i.second == *this; });\n> > > > +     return it != mbusCodeToBayer.end() ? it->first : 0;\n> > >\n> > > I'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10\n> > > It doesn't register flips, so the bayer pattern should not be changed\n> > >\n> > > The code you added in the next patch for the CameraSensor class goes\n> > > as\n> > >\n> > >         for (const auto &format : formats) {\n> > >                 unsigned int mbusCode = format.first;\n> > >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > >\n> > >                 if (bayerFormat.isValid())\n> > >                         mbusCode = bayerFormat.transform(transform).toMbusCode();\n> > >\n> > >                 if (mbusCode)\n> > >                         formats_[mbusCode] = std::move(format.second);\n> > >         }\n> > >\n> > >\n> > > The first\n> > >\n> > >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > >\n> > > returns  { BayerFormat::BGGR, 10, BayerFormat::Packing::None }\n> > >\n> > > we go throgh a transform() which does nothing as it receives Indentity\n> > > and then call toMbusCode() on it\n> > >\n> > > The above implementation of toMbusCode() walks the mbusCodeToBayer map\n> > > looking for a matching bayer. Look at this table section\n> > >\n> > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > >         { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > >\n> > > So here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and\n> > > MEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same\n> > > { BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly\n> > > get my original 0x3007 code transformed to 0x3003!\n> > >\n> > > I'm afraid the fact mbus_code-to-BayerFormat relationship is not\n> > > unique is a blocker for this patch\n> >\n> > Oh dear! Thanks for discovering this, I had no idea that these formats existed.\n>\n> Why do those formats exist, and why only the BGGR ordering versions?\n> A partial implementation of something, and presumably on a parallel\n> interface as formats such as YUYV are mandated to use the 1X16\n> versions (not 2X8) on CSI.\n>\n> Are they actually relevant, or legacy cruft?\n>\n>   Dave\n>\n> > I wonder if the correct solution is to have 4 extra packing formats,\n> > alongside the existing CSI2 and IPU3 ones. What do you think?\n> >\n> > David\n> >\n> > >\n> > >\n> > >\n> > >\n> > >\n> > > > +}\n> > > > +\n> > > >  /**\n> > > >   * \\fn BayerFormat::isValid()\n> > > >   * \\brief Return whether a BayerFormat is valid\n> > > > --\n> > > > 2.38.1\n> > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 879D9C322E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 10 Jan 2023 11:59:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C753E625DE;\n\tTue, 10 Jan 2023 12:59:33 +0100 (CET)","from mail-oa1-x32.google.com (mail-oa1-x32.google.com\n\t[IPv6:2001:4860:4864:20::32])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6688F61F07\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 10 Jan 2023 12:59:31 +0100 (CET)","by mail-oa1-x32.google.com with SMTP id\n\t586e51a60fabf-142b72a728fso11857203fac.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 10 Jan 2023 03:59:31 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673351973;\n\tbh=LSm4PGKfHU2HANKu5QuQe6ysPrTniijv8lI3Hjl02bQ=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=YpQl28IDz2Mn4BYlCZVkFww60eFplWVRLOC9YM7MyZAqsXH3rYHbNs4/Tzl+kw0yX\n\tw17x+1qezKZvxkj2yI1aDqaJ28jrQpWUpdF/XdghJ8Igv/z3OXCFAty7IjZi2ZonBQ\n\tp9GI1cQo097vrtP0GA28RnVpi8NLyIi8eNuS5+TXxFgHkBnfCZjbhZQo4RhiFquzUK\n\tzhkImBp5Ou2Yb4dkmuYGyEmmonK57myBH2QR022vwyBz7gyZcD/Bs8PgIueqeGVzib\n\tkRkyskToLmuWDyghOxOWfHUcGLhfv7YEyKtjF332BtmWYxSY1G9euHzpMHymJh++iM\n\tvU+oLlcMcBQhg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=TM6QUcYDU2lBRmmtslS9Hmc5zE1UEGyjCl1af0SUrBI=;\n\tb=QCoFZB6LkYSbfp6K4MjlDNK1yVkAae9XE50++GgP9mzq4Ne+JFVYGgn8Kwcjk2Cmuh\n\t8XFyO6uR/rChd6iXfJKh1cHj36NyztWz1kecfqooacCKgO9pPAxz474rZOkBPGB7UM7k\n\t75FygjYMjPuHG0Ju2QnvebHr1f+EJm5nbGDiGfRJ62wDsW7jOE+dQ+TNGHHoc/QStRDa\n\tIhduzmA2x+xslDDSFr9hIYO+84lMbdHBqw4HI74Duhd39OrexXcXR7CTwRb3PziM5qNg\n\tcYwlWZKsteasLFJOGX3htbas4mYLr7eXVan4y/1VBSIE0qu+JzKsmYPtUsOwLYD7Ze9c\n\tE3Mw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"QCoFZB6L\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=TM6QUcYDU2lBRmmtslS9Hmc5zE1UEGyjCl1af0SUrBI=;\n\tb=jps+HgCG7n98i0o4zHaIPlyRuE7lRGxP+AfChPMgjmbQE1xrLoHVYr8KLMJQAoPhHM\n\tRPltHB0/SRuIRdteLJBdRxi5QqFIQM5Gnz5HDTW9SYuKQmy1/hXnYWCjcLq2HKhN8R7v\n\toYTkxwgvf/dMRgSO8w1e58/ejUbG8jcuyh/tQzpSaDKFaWVpkARF1uojLSXysI9djPFH\n\tlCmplREZr39Bj/tD96rugGGXht8KnuxCJcL2jZmpETHQ5IP86Js53onXnjHrFPn6uAVA\n\tFH6k/LgmFsXO6lOQRYNKz49Cj/wKmeyaP1DwuTekjn1M7I/2J3dyYjQuOHHikJ+YVlcv\n\th/dw==","X-Gm-Message-State":"AFqh2kq6Dbp8QCw/zWaCxi0UKQrX4DJcM4dCzdVkyos//MktLIRLyoix\n\tbh7F0NmM9GRR/nUvsAdejgPT99Fcb25qnLZRLDM7har6nHMtSQ==","X-Google-Smtp-Source":"AMrXdXvhXl2JNEhY/KpTqSVgRJsdKHLPTnM8tsF53ApZ9ap74jZTRF9YW5KASUNjlNtEcwRvO2HmT5ZoVOAmJmA5U34=","X-Received":"by 2002:a05:6870:494f:b0:150:2429:9851 with SMTP id\n\tfl15-20020a056870494f00b0015024299851mr3450362oab.20.1673351969925;\n\tTue, 10 Jan 2023 03:59:29 -0800 (PST)","MIME-Version":"1.0","References":"<20221124121220.47000-1-jacopo@jmondi.org>\n\t<20221124121220.47000-2-jacopo@jmondi.org>\n\t<20230104131229.uwwchptzslflzyjr@uno.localdomain>\n\t<CAHW6GYL26Ws3fPWWp0P60-PMn4nz-5Onr8ba0V0-YL3iGwT1Ug@mail.gmail.com>\n\t<CAPY8ntCy54XsgdOgG1hAXrFVSTnEaDR_x+UP7Gif6Lk2W0mn=A@mail.gmail.com>","In-Reply-To":"<CAPY8ntCy54XsgdOgG1hAXrFVSTnEaDR_x+UP7Gif6Lk2W0mn=A@mail.gmail.com>","Date":"Tue, 10 Jan 2023 11:59:19 +0000","Message-ID":"<CAHW6GY+amBOp2H6wjm3fbYUrmvwkSDVbMSEyEj-kwyfZpOkJAQ@mail.gmail.com>","To":"Dave Stevenson <dave.stevenson@raspberrypi.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26206,"web_url":"https://patchwork.libcamera.org/comment/26206/","msgid":"<20230110123823.lcxhm7plu52xbhyb@uno.localdomain>","date":"2023-01-10T12:38:23","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Tue, Jan 10, 2023 at 11:59:19AM +0000, David Plowman wrote:\n> Hi again\n>\n> I was thinking about this a bit more. One thing I wanted to clarify\n> was what media bus formats really mean. For example, if I have\n> MEDIA_BUS_FMT_SBGGR10_1X10, presumably that means Bayer BGGR with CSI2\n> packing. Does that sound right?\n\nNot per my understanding\n\nThe \"CSI-2 packaged\" Bayer format are described by the 'P' variant of\nthe canonical raw Bayer formats packaged in 5 bytes according to the\nCSI-2 specifications.\n\nIn example section \"11.4.4 RAW10\" specifies\nThe transmission of 10-bit Raw data is accomplished by packing the\n10-bit pixel data to look like 8-bit data format.\n\nWhich means the first 4 bytes will contain the 8 MSB and the last\nbyte contains the 2 LSB of each pixel.\n\nV4L2 defines pixel formats for CSI-2 packaged bayer formats, but not\nmedia bus codes as far as I can tell:\nhttps://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10p.html\n\nThis surprises me as I would be expecting that being the formats\ndefined by the CSI-2 specs that's actually what it is sent on the wire\nbut as far as I can tell sensors I've seen do not send RAW10 that way?\n\nAm I confused ?\n\n\n>\n> Under those circumstances it seems reasonable to me that the\n> mbusCodeToBayer table (bayer_format.cpp) should specify Packing::CSI2P\n> for most of the formats there, but with new values (e.g.\n> Packing::PADHI_BE) for the ones now under discussion.\n>\n> I don't think such a change would bother Raspberry Pi (it might well\n> \"just work\", or if not require only very minor tweaks), but someone\n> else would have to comment on ipu3 or rkisp1.\n>\n\nPlease note that 2X8 formats are for parallel busses only, and the\nPAD.. only apply to them. I wonder if we do care about them for real..\n\n> Dave also mentions that all the non-BGGR versions are missing (it\n> seems a bit disappointing that they weren't added all at the same\n> time), but for the moment I suggest we ignore that.\n>\n> I also note there's a potential problem with the \"ALAW8\" and \"DPCM8\"\n> formats. I don't know if folks think those can be handled by more\n> \"packing\" formats, or whether we'd rather have a new \"modifiers\"\n> field?\n\nALAW and DPCM8 could be described as \"compressions\" more than\npackaging ? Reading\nhttps://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10alaw8.html\nit seems to imply a 10-bit pixel gets compressed to an 8-bit sample,\nso that impacts the buffer and line sizes ?\n\n>\n> Thoughts?\n\nYes, and all of them rather confused :)\n\n>\n> David\n>\n> On Fri, 6 Jan 2023 at 17:28, Dave Stevenson\n> <dave.stevenson@raspberrypi.com> wrote:\n> >\n> > Hi David and Jacopo\n> >\n> > On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel\n> > <libcamera-devel@lists.libcamera.org> wrote:\n> > >\n> > > Hi Jacopo\n> > >\n> > > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@jmondi.org> wrote:\n> > > >\n> > > > Hi David\n> > > >    sorry to revive this, but I'm carrying this series in my tree and I\n> > > > just found a bug in this patches\n> > > >\n> > > >\n> > > > On Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:\n> > > > > From: David Plowman <david.plowman@raspberrypi.com>\n> > > > >\n> > > > > This makes it easier to perform transformations on Bayer type mbus\n> > > > > codes by converting them to a BayerFormat, doing the transform, and\n> > > > > then converting them back again.\n> > > > >\n> > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > > > ---\n> > > > >  include/libcamera/internal/bayer_format.h |  1 +\n> > > > >  src/libcamera/bayer_format.cpp            | 11 +++++++++++\n> > > > >  2 files changed, 12 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> > > > > index 78ba3969913d..3601dccb7228 100644\n> > > > > --- a/include/libcamera/internal/bayer_format.h\n> > > > > +++ b/include/libcamera/internal/bayer_format.h\n> > > > > @@ -47,6 +47,7 @@ public:\n> > > > >       }\n> > > > >\n> > > > >       static const BayerFormat &fromMbusCode(unsigned int mbusCode);\n> > > > > +     unsigned int toMbusCode() const;\n> > > > >       bool isValid() const { return bitDepth != 0; }\n> > > > >\n> > > > >       std::string toString() const;\n> > > > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> > > > > index f27cc1662d25..fdbc4af1dcc0 100644\n> > > > > --- a/src/libcamera/bayer_format.cpp\n> > > > > +++ b/src/libcamera/bayer_format.cpp\n> > > > > @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)\n> > > > >               return it->second;\n> > > > >  }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Retrieve the media bus code corresponding this this BayerFormat\n> > > > > + * \\return The corresponding media bus code, or zero if none was found\n> > > > > + */\n> > > > > +unsigned int BayerFormat::toMbusCode() const\n> > > > > +{\n> > > > > +     auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),\n> > > > > +                            [this](const auto &i) { return i.second == *this; });\n> > > > > +     return it != mbusCodeToBayer.end() ? it->first : 0;\n> > > >\n> > > > I'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10\n> > > > It doesn't register flips, so the bayer pattern should not be changed\n> > > >\n> > > > The code you added in the next patch for the CameraSensor class goes\n> > > > as\n> > > >\n> > > >         for (const auto &format : formats) {\n> > > >                 unsigned int mbusCode = format.first;\n> > > >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > >\n> > > >                 if (bayerFormat.isValid())\n> > > >                         mbusCode = bayerFormat.transform(transform).toMbusCode();\n> > > >\n> > > >                 if (mbusCode)\n> > > >                         formats_[mbusCode] = std::move(format.second);\n> > > >         }\n> > > >\n> > > >\n> > > > The first\n> > > >\n> > > >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > >\n> > > > returns  { BayerFormat::BGGR, 10, BayerFormat::Packing::None }\n> > > >\n> > > > we go throgh a transform() which does nothing as it receives Indentity\n> > > > and then call toMbusCode() on it\n> > > >\n> > > > The above implementation of toMbusCode() walks the mbusCodeToBayer map\n> > > > looking for a matching bayer. Look at this table section\n> > > >\n> > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > >         { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > >\n> > > > So here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and\n> > > > MEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same\n> > > > { BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly\n> > > > get my original 0x3007 code transformed to 0x3003!\n> > > >\n> > > > I'm afraid the fact mbus_code-to-BayerFormat relationship is not\n> > > > unique is a blocker for this patch\n> > >\n> > > Oh dear! Thanks for discovering this, I had no idea that these formats existed.\n> >\n> > Why do those formats exist, and why only the BGGR ordering versions?\n> > A partial implementation of something, and presumably on a parallel\n> > interface as formats such as YUYV are mandated to use the 1X16\n> > versions (not 2X8) on CSI.\n> >\n> > Are they actually relevant, or legacy cruft?\n> >\n> >   Dave\n> >\n> > > I wonder if the correct solution is to have 4 extra packing formats,\n> > > alongside the existing CSI2 and IPU3 ones. What do you think?\n> > >\n> > > David\n> > >\n> > > >\n> > > >\n> > > >\n> > > >\n> > > >\n> > > > > +}\n> > > > > +\n> > > > >  /**\n> > > > >   * \\fn BayerFormat::isValid()\n> > > > >   * \\brief Return whether a BayerFormat is valid\n> > > > > --\n> > > > > 2.38.1\n> > > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3DD7CC3237\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 10 Jan 2023 12:38:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 843C4625DF;\n\tTue, 10 Jan 2023 13:38:27 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFFE7625CF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 10 Jan 2023 13:38:25 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id E170620005;\n\tTue, 10 Jan 2023 12:38:24 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673354307;\n\tbh=y/TxMOaVMg6YSJao+U2LG+4AkQD5cLRX5HDjBwvFJaM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=X3l9vmtn8JlcR40ROVE+YGUsvRlpfaFV2A6k9m6+Ewo2d9APm/sUev8X0ad6P9ogg\n\t50rSHtq0lxNHtpCQmygDF8US9yI3QW88ZhnBEMtLYvsRzMlAgw9hqAGy60Wfb4+W/U\n\tL6ucT0E3XOOMYnlgixn8XJH9Y3hBltmAuyG6ESIDG+Rw1GIHKMzlkLLxIiS0q65wmN\n\trCCX0n2E4mS13mavz4JH3dOsJHNSFy1/UcLTRabw9lOgwgW4t5H2FcqfPMP1zKhehf\n\tIge7QhzQ8Sm9t2mVD8gFR7NfDR/FF4gY2U7jrNZIjZxikBS6/LaAgUHwXX7ZtsI7QL\n\tMqYYpNKpb5jKw==","Date":"Tue, 10 Jan 2023 13:38:23 +0100","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<20230110123823.lcxhm7plu52xbhyb@uno.localdomain>","References":"<20221124121220.47000-1-jacopo@jmondi.org>\n\t<20221124121220.47000-2-jacopo@jmondi.org>\n\t<20230104131229.uwwchptzslflzyjr@uno.localdomain>\n\t<CAHW6GYL26Ws3fPWWp0P60-PMn4nz-5Onr8ba0V0-YL3iGwT1Ug@mail.gmail.com>\n\t<CAPY8ntCy54XsgdOgG1hAXrFVSTnEaDR_x+UP7Gif6Lk2W0mn=A@mail.gmail.com>\n\t<CAHW6GY+amBOp2H6wjm3fbYUrmvwkSDVbMSEyEj-kwyfZpOkJAQ@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAHW6GY+amBOp2H6wjm3fbYUrmvwkSDVbMSEyEj-kwyfZpOkJAQ@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26207,"web_url":"https://patchwork.libcamera.org/comment/26207/","msgid":"<CAHW6GY+33J-RV+FdsRFyenhcHonL=R5u=9hwA8NBUJ2MKQR12Q@mail.gmail.com>","date":"2023-01-10T14:47:10","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Tue, 10 Jan 2023 at 12:38, Jacopo Mondi <jacopo@jmondi.org> wrote:\n>\n> Hi David,\n>\n> On Tue, Jan 10, 2023 at 11:59:19AM +0000, David Plowman wrote:\n> > Hi again\n> >\n> > I was thinking about this a bit more. One thing I wanted to clarify\n> > was what media bus formats really mean. For example, if I have\n> > MEDIA_BUS_FMT_SBGGR10_1X10, presumably that means Bayer BGGR with CSI2\n> > packing. Does that sound right?\n>\n> Not per my understanding\n>\n> The \"CSI-2 packaged\" Bayer format are described by the 'P' variant of\n> the canonical raw Bayer formats packaged in 5 bytes according to the\n> CSI-2 specifications.\n>\n> In example section \"11.4.4 RAW10\" specifies\n> The transmission of 10-bit Raw data is accomplished by packing the\n> 10-bit pixel data to look like 8-bit data format.\n>\n> Which means the first 4 bytes will contain the 8 MSB and the last\n> byte contains the 2 LSB of each pixel.\n>\n> V4L2 defines pixel formats for CSI-2 packaged bayer formats, but not\n> media bus codes as far as I can tell:\n> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10p.html\n>\n> This surprises me as I would be expecting that being the formats\n> defined by the CSI-2 specs that's actually what it is sent on the wire\n> but as far as I can tell sensors I've seen do not send RAW10 that way?\n>\n> Am I confused ?\n\nWell, I'm pretty baffled by the whole thing too.\n\nWe're definitely using MEDIA_BUS_FMT_SBGGR10_1X10 to mean CSI-2 packed\n10-bit BGGR, and looking at all the media bus formats, I can't see\nthere's anything else. So it must be true. ??\n\n>\n>\n> >\n> > Under those circumstances it seems reasonable to me that the\n> > mbusCodeToBayer table (bayer_format.cpp) should specify Packing::CSI2P\n> > for most of the formats there, but with new values (e.g.\n> > Packing::PADHI_BE) for the ones now under discussion.\n> >\n> > I don't think such a change would bother Raspberry Pi (it might well\n> > \"just work\", or if not require only very minor tweaks), but someone\n> > else would have to comment on ipu3 or rkisp1.\n> >\n>\n> Please note that 2X8 formats are for parallel busses only, and the\n> PAD.. only apply to them. I wonder if we do care about them for real..\n\nThat's a fair point, though I do feel there's something wrong when we\ncan't get back the same \"kind\" of mbus format as we put in. If it's\nnot too difficult to sort out, we probably should. ??\n\n>\n> > Dave also mentions that all the non-BGGR versions are missing (it\n> > seems a bit disappointing that they weren't added all at the same\n> > time), but for the moment I suggest we ignore that.\n> >\n> > I also note there's a potential problem with the \"ALAW8\" and \"DPCM8\"\n> > formats. I don't know if folks think those can be handled by more\n> > \"packing\" formats, or whether we'd rather have a new \"modifiers\"\n> > field?\n>\n> ALAW and DPCM8 could be described as \"compressions\" more than\n> packaging ? Reading\n> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10alaw8.html\n> it seems to imply a 10-bit pixel gets compressed to an 8-bit sample,\n> so that impacts the buffer and line sizes ?\n\nI think these would actually work if we treated them as \"packing\" too.\n\"compression\" is just a fancier form of packing, kind of. Or maybe we\nshould rename the \"packing\" field to \"modifier\", that might feel less\nstrange.\n\n>\n> >\n> > Thoughts?\n>\n> Yes, and all of them rather confused :)\n\nDitto.\n\nI would be happy to make up a PR to attempt to improve some of this,\nbut we'd have to find someone else to worry about non-Pi platforms.\n\nDavid\n\n>\n> >\n> > David\n> >\n> > On Fri, 6 Jan 2023 at 17:28, Dave Stevenson\n> > <dave.stevenson@raspberrypi.com> wrote:\n> > >\n> > > Hi David and Jacopo\n> > >\n> > > On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel\n> > > <libcamera-devel@lists.libcamera.org> wrote:\n> > > >\n> > > > Hi Jacopo\n> > > >\n> > > > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@jmondi.org> wrote:\n> > > > >\n> > > > > Hi David\n> > > > >    sorry to revive this, but I'm carrying this series in my tree and I\n> > > > > just found a bug in this patches\n> > > > >\n> > > > >\n> > > > > On Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:\n> > > > > > From: David Plowman <david.plowman@raspberrypi.com>\n> > > > > >\n> > > > > > This makes it easier to perform transformations on Bayer type mbus\n> > > > > > codes by converting them to a BayerFormat, doing the transform, and\n> > > > > > then converting them back again.\n> > > > > >\n> > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > > > > ---\n> > > > > >  include/libcamera/internal/bayer_format.h |  1 +\n> > > > > >  src/libcamera/bayer_format.cpp            | 11 +++++++++++\n> > > > > >  2 files changed, 12 insertions(+)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> > > > > > index 78ba3969913d..3601dccb7228 100644\n> > > > > > --- a/include/libcamera/internal/bayer_format.h\n> > > > > > +++ b/include/libcamera/internal/bayer_format.h\n> > > > > > @@ -47,6 +47,7 @@ public:\n> > > > > >       }\n> > > > > >\n> > > > > >       static const BayerFormat &fromMbusCode(unsigned int mbusCode);\n> > > > > > +     unsigned int toMbusCode() const;\n> > > > > >       bool isValid() const { return bitDepth != 0; }\n> > > > > >\n> > > > > >       std::string toString() const;\n> > > > > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> > > > > > index f27cc1662d25..fdbc4af1dcc0 100644\n> > > > > > --- a/src/libcamera/bayer_format.cpp\n> > > > > > +++ b/src/libcamera/bayer_format.cpp\n> > > > > > @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)\n> > > > > >               return it->second;\n> > > > > >  }\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\brief Retrieve the media bus code corresponding this this BayerFormat\n> > > > > > + * \\return The corresponding media bus code, or zero if none was found\n> > > > > > + */\n> > > > > > +unsigned int BayerFormat::toMbusCode() const\n> > > > > > +{\n> > > > > > +     auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),\n> > > > > > +                            [this](const auto &i) { return i.second == *this; });\n> > > > > > +     return it != mbusCodeToBayer.end() ? it->first : 0;\n> > > > >\n> > > > > I'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10\n> > > > > It doesn't register flips, so the bayer pattern should not be changed\n> > > > >\n> > > > > The code you added in the next patch for the CameraSensor class goes\n> > > > > as\n> > > > >\n> > > > >         for (const auto &format : formats) {\n> > > > >                 unsigned int mbusCode = format.first;\n> > > > >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > > >\n> > > > >                 if (bayerFormat.isValid())\n> > > > >                         mbusCode = bayerFormat.transform(transform).toMbusCode();\n> > > > >\n> > > > >                 if (mbusCode)\n> > > > >                         formats_[mbusCode] = std::move(format.second);\n> > > > >         }\n> > > > >\n> > > > >\n> > > > > The first\n> > > > >\n> > > > >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > > >\n> > > > > returns  { BayerFormat::BGGR, 10, BayerFormat::Packing::None }\n> > > > >\n> > > > > we go throgh a transform() which does nothing as it receives Indentity\n> > > > > and then call toMbusCode() on it\n> > > > >\n> > > > > The above implementation of toMbusCode() walks the mbusCodeToBayer map\n> > > > > looking for a matching bayer. Look at this table section\n> > > > >\n> > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > >         { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > >\n> > > > > So here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and\n> > > > > MEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same\n> > > > > { BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly\n> > > > > get my original 0x3007 code transformed to 0x3003!\n> > > > >\n> > > > > I'm afraid the fact mbus_code-to-BayerFormat relationship is not\n> > > > > unique is a blocker for this patch\n> > > >\n> > > > Oh dear! Thanks for discovering this, I had no idea that these formats existed.\n> > >\n> > > Why do those formats exist, and why only the BGGR ordering versions?\n> > > A partial implementation of something, and presumably on a parallel\n> > > interface as formats such as YUYV are mandated to use the 1X16\n> > > versions (not 2X8) on CSI.\n> > >\n> > > Are they actually relevant, or legacy cruft?\n> > >\n> > >   Dave\n> > >\n> > > > I wonder if the correct solution is to have 4 extra packing formats,\n> > > > alongside the existing CSI2 and IPU3 ones. What do you think?\n> > > >\n> > > > David\n> > > >\n> > > > >\n> > > > >\n> > > > >\n> > > > >\n> > > > >\n> > > > > > +}\n> > > > > > +\n> > > > > >  /**\n> > > > > >   * \\fn BayerFormat::isValid()\n> > > > > >   * \\brief Return whether a BayerFormat is valid\n> > > > > > --\n> > > > > > 2.38.1\n> > > > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 828B8C322E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 10 Jan 2023 14:47:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3E00625DE;\n\tTue, 10 Jan 2023 15:47:23 +0100 (CET)","from mail-oi1-x235.google.com (mail-oi1-x235.google.com\n\t[IPv6:2607:f8b0:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 86CC861F07\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 10 Jan 2023 15:47:22 +0100 (CET)","by mail-oi1-x235.google.com with SMTP id s66so9571109oib.7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 10 Jan 2023 06:47:22 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673362043;\n\tbh=tbHBkKSIHJD8GCCXPzmdj92QmGqVgFeXHEWMJusvnF8=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Vt2/TRgVEhJP9Cn03BLZo6MCp+/bF72B+gP49wsmDYAvyB01Uf6OdSZ9dsfQGtvNp\n\tof9sQnda3xY8X1mi1FQV4iB5YD6NOwId71uJuR6GaRJcI5/9+x928+DLC4JoPWdi4h\n\trmA6/8T2I1mU98oiqbBeTHzYpKAwz2K2KMLWnfjxY0YpS+YN2LIRYJ3iDlB/kBrz4I\n\t4zXXL/hL1LYGH0GjlG/I/s7vlZvD6LH55u/Ih9eFTmozpGrsVDqaFqb3pw/rbLIiK0\n\th4q+ak+8tSDf7HlkKNDWOvnvEEJfahS06aSi+BJJ5UeVYtqonkG6uD/Es0/OPM4cGU\n\t22FsdJO1PHKrg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=1qNuW66wCduEgxqQ2czBQ/BK7NAJvYYRU3fTxHVlWA8=;\n\tb=almqIEOIBpqGpZNClCyLjTGFzsb/SpjTxZMarBZfNWw/+Yq0NKcNwYDmcBBd5R8uk4\n\ttsLVkvwD9E9tTpbZI60hSpQpahMKVFy0BvPbEAp+bJcLH77KW2GQ+4KVuUWmQgC+v+ln\n\tfqWWekEZhjlaZTCTOvfoIvIkQgGH5ISPAvQuRk2iFJqiTm2xdtCfMg6HUugGqzWfay9t\n\tPSMqhwXtrZgOa/YVj2qJoeAaWEGS8JWVon/fhrxLk1S+jtLJZIxa3DKRW0/PAinS2PLe\n\tZpfMdEbEC+P1q5mHaA9z3Wb32E6m/XQJ+vZrptZ9fd4O0dDQsY8mJPJsTVFBop8BCm57\n\tkQig=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"almqIEOI\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=1qNuW66wCduEgxqQ2czBQ/BK7NAJvYYRU3fTxHVlWA8=;\n\tb=pYafTl9+kUHnofyt3sa9IrTu4QifdHlJ8h97e6IVyLKpWw/nM2H+f6QEspkl6MSHL2\n\tglX3/zRJmwpwVNIX3/Bfd0+CdM4pASxVE22upq0kW3X8ZrnF21K2CpIVfG73IvinHizA\n\t9AWUjpuSe+3HXdYpZObRYVyAOhxLrvcDBKf6lYsy53jy0AiAauifoUb6HtaJo8Q6w7E8\n\tx7HWqZh0yPRplAWDPKZ0E8GT9QywqgtTt1eWWbgZ4nsO2uBVfLdQibZIGjOBvobpacdl\n\tKaUXhg4cFf8zjHfzUiSE7afhKGy8/V8WjQIPj07qKI+YLl4r7uUD3ecX7uVWAGArN0dW\n\tcciA==","X-Gm-Message-State":"AFqh2kq2RQ2s6biQiTXevYy0IgBgPE06QoqqVakqq84U1qzCH5UlEHiD\n\tYmUeFG/+M+iSeRRtmFERnN9gVTcAwvX6D9l+4gzqJw==","X-Google-Smtp-Source":"AMrXdXs5RnlDQvLl6RH9iFgZscGK3mdibvpyg3iEg8nO8p6UrbiHAqQ8VRedu5B1prZpRghu/aOQJvRjugpbOrBUyjE=","X-Received":"by 2002:aca:910:0:b0:354:4cfe:aaad with SMTP id\n\t16-20020aca0910000000b003544cfeaaadmr3002630oij.246.1673362041027;\n\tTue, 10 Jan 2023 06:47:21 -0800 (PST)","MIME-Version":"1.0","References":"<20221124121220.47000-1-jacopo@jmondi.org>\n\t<20221124121220.47000-2-jacopo@jmondi.org>\n\t<20230104131229.uwwchptzslflzyjr@uno.localdomain>\n\t<CAHW6GYL26Ws3fPWWp0P60-PMn4nz-5Onr8ba0V0-YL3iGwT1Ug@mail.gmail.com>\n\t<CAPY8ntCy54XsgdOgG1hAXrFVSTnEaDR_x+UP7Gif6Lk2W0mn=A@mail.gmail.com>\n\t<CAHW6GY+amBOp2H6wjm3fbYUrmvwkSDVbMSEyEj-kwyfZpOkJAQ@mail.gmail.com>\n\t<20230110123823.lcxhm7plu52xbhyb@uno.localdomain>","In-Reply-To":"<20230110123823.lcxhm7plu52xbhyb@uno.localdomain>","Date":"Tue, 10 Jan 2023 14:47:10 +0000","Message-ID":"<CAHW6GY+33J-RV+FdsRFyenhcHonL=R5u=9hwA8NBUJ2MKQR12Q@mail.gmail.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26208,"web_url":"https://patchwork.libcamera.org/comment/26208/","msgid":"<CAPY8ntDUvwbaRbF1a5NMC3bWGbCbCMK3S4Tbj-f66uxAi+eRyA@mail.gmail.com>","date":"2023-01-11T13:45:13","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi\n\nOn Tue, 10 Jan 2023 at 14:47, David Plowman\n<david.plowman@raspberrypi.com> wrote:\n>\n> Hi Jacopo\n>\n> On Tue, 10 Jan 2023 at 12:38, Jacopo Mondi <jacopo@jmondi.org> wrote:\n> >\n> > Hi David,\n> >\n> > On Tue, Jan 10, 2023 at 11:59:19AM +0000, David Plowman wrote:\n> > > Hi again\n> > >\n> > > I was thinking about this a bit more. One thing I wanted to clarify\n> > > was what media bus formats really mean. For example, if I have\n> > > MEDIA_BUS_FMT_SBGGR10_1X10, presumably that means Bayer BGGR with CSI2\n> > > packing. Does that sound right?\n> >\n> > Not per my understanding\n> >\n> > The \"CSI-2 packaged\" Bayer format are described by the 'P' variant of\n> > the canonical raw Bayer formats packaged in 5 bytes according to the\n> > CSI-2 specifications.\n> >\n> > In example section \"11.4.4 RAW10\" specifies\n> > The transmission of 10-bit Raw data is accomplished by packing the\n> > 10-bit pixel data to look like 8-bit data format.\n> >\n> > Which means the first 4 bytes will contain the 8 MSB and the last\n> > byte contains the 2 LSB of each pixel.\n> >\n> > V4L2 defines pixel formats for CSI-2 packaged bayer formats, but not\n> > media bus codes as far as I can tell:\n> > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10p.html\n> >\n> > This surprises me as I would be expecting that being the formats\n> > defined by the CSI-2 specs that's actually what it is sent on the wire\n> > but as far as I can tell sensors I've seen do not send RAW10 that way?\n> >\n> > Am I confused ?\n>\n> Well, I'm pretty baffled by the whole thing too.\n>\n> We're definitely using MEDIA_BUS_FMT_SBGGR10_1X10 to mean CSI-2 packed\n> 10-bit BGGR, and looking at all the media bus formats, I can't see\n> there's anything else. So it must be true. ??\n\nIt is all rather ugly when it comes to CSI2 as it doesn't define the\nbus format in any sensible shape or form.\n\nThe 1X10, 2X8, 1X16 stuff is all related to a parallel bus taking a\nbus width and number of transfers to pass the pixel value. To\naccurately represent CSI2 it would therefore be representing the\nnumber of data lanes as well as the format, and that is not\ninformation that userspace should be worrying about.\n\nFundamentally for CSI-2, there is no padding in the datastream,\ntherefore 1X.. is the closest representation. Any unpacking into a 16\nbit representation (ie BGGR10 vs BGGR10P) is done in the receiver.\n\nRPi doesn't have a parallel camera interface so we really don't care,\nbut libcamera doesn't have that constraint.\n\nEven on a parallel bus MEDIA_BUS_FMT's don't fully work, as the\nphysical bus isn't necessarily a 1:1 mapping.\nLess so with Bayer data, but there's an ongoing discussion for DPI\ndisplays where you might have a display that wants RGB666. The panel\ncould legitimately advertise MEDIA_BUS_FMT_RGB666_1X18, but the wiring\nto the host could be packed onto a 24 bit bus so wants\nMEDIA_BUS_FMT_RGB888_1X24 (with 6 wires not connected), or\nMEDIA_BUS_FMT_RGB666_1X24_CPADHI, or a number of others. Two different\nformats needed on opposite ends of the same link :-(\n\nYes with any 2X.. formats it matters, but for any 1X.. format, it\nreally doesn't.\n\n> >\n> >\n> > >\n> > > Under those circumstances it seems reasonable to me that the\n> > > mbusCodeToBayer table (bayer_format.cpp) should specify Packing::CSI2P\n> > > for most of the formats there, but with new values (e.g.\n> > > Packing::PADHI_BE) for the ones now under discussion.\n> > >\n> > > I don't think such a change would bother Raspberry Pi (it might well\n> > > \"just work\", or if not require only very minor tweaks), but someone\n> > > else would have to comment on ipu3 or rkisp1.\n> > >\n> >\n> > Please note that 2X8 formats are for parallel busses only, and the\n> > PAD.. only apply to them. I wonder if we do care about them for real..\n>\n> That's a fair point, though I do feel there's something wrong when we\n> can't get back the same \"kind\" of mbus format as we put in. If it's\n> not too difficult to sort out, we probably should. ??\n\nThe only source for MEDIA_BUS_FMT_SBGGR10_2X8_PAD* is the Sharp\nRJ54N1CB0C, and it also supports MEDIA_BUS_FMT_SBGGR10_1X10.\nThat means you haven't a hope of writing generic code to be able to\nreverse the conversion without defining it as packing or similar.\n\ndrivers/gpu/ipu-v3/ipu-csi.c and\ndrivers/media/platform/intel/pxa_camera have support for receiving\nthese formats.\n\nAre i.MX5, i.MX6Q or Intel PXA27x Quick Capture Interface supported by\nlibcamera? If not, then drop the mappings, and worry about it when\nsomeone tries to interface a RJ54N1CB0C with libcamera.\n\n> >\n> > > Dave also mentions that all the non-BGGR versions are missing (it\n> > > seems a bit disappointing that they weren't added all at the same\n> > > time), but for the moment I suggest we ignore that.\n> > >\n> > > I also note there's a potential problem with the \"ALAW8\" and \"DPCM8\"\n> > > formats. I don't know if folks think those can be handled by more\n> > > \"packing\" formats, or whether we'd rather have a new \"modifiers\"\n> > > field?\n> >\n> > ALAW and DPCM8 could be described as \"compressions\" more than\n> > packaging ? Reading\n> > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10alaw8.html\n> > it seems to imply a 10-bit pixel gets compressed to an 8-bit sample,\n> > so that impacts the buffer and line sizes ?\n>\n> I think these would actually work if we treated them as \"packing\" too.\n> \"compression\" is just a fancier form of packing, kind of. Or maybe we\n> should rename the \"packing\" field to \"modifier\", that might feel less\n> strange.\n\nI'll agree that ALAW and DPCM would be packings as they affect how the\ndata has to be interpreted. They also map through to V4L2 formats.\n\nWhether anyone other than vimc actually still uses them is a totally\ndifferent question.\nI see et8ek8 (wow, blast from the past) has a mode using DPCM 10 to 8,\nand CCS supports it, but most users these days care about image\nquality more than bandwidth.\n\nI don't see reference to ALAW in the CSI-2 spec, so I wonder where\nthat one came from.\n\n  Dave\n\n> >\n> > >\n> > > Thoughts?\n> >\n> > Yes, and all of them rather confused :)\n>\n> Ditto.\n>\n> I would be happy to make up a PR to attempt to improve some of this,\n> but we'd have to find someone else to worry about non-Pi platforms.\n>\n> David\n>\n> >\n> > >\n> > > David\n> > >\n> > > On Fri, 6 Jan 2023 at 17:28, Dave Stevenson\n> > > <dave.stevenson@raspberrypi.com> wrote:\n> > > >\n> > > > Hi David and Jacopo\n> > > >\n> > > > On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel\n> > > > <libcamera-devel@lists.libcamera.org> wrote:\n> > > > >\n> > > > > Hi Jacopo\n> > > > >\n> > > > > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@jmondi.org> wrote:\n> > > > > >\n> > > > > > Hi David\n> > > > > >    sorry to revive this, but I'm carrying this series in my tree and I\n> > > > > > just found a bug in this patches\n> > > > > >\n> > > > > >\n> > > > > > On Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:\n> > > > > > > From: David Plowman <david.plowman@raspberrypi.com>\n> > > > > > >\n> > > > > > > This makes it easier to perform transformations on Bayer type mbus\n> > > > > > > codes by converting them to a BayerFormat, doing the transform, and\n> > > > > > > then converting them back again.\n> > > > > > >\n> > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > > > > > ---\n> > > > > > >  include/libcamera/internal/bayer_format.h |  1 +\n> > > > > > >  src/libcamera/bayer_format.cpp            | 11 +++++++++++\n> > > > > > >  2 files changed, 12 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> > > > > > > index 78ba3969913d..3601dccb7228 100644\n> > > > > > > --- a/include/libcamera/internal/bayer_format.h\n> > > > > > > +++ b/include/libcamera/internal/bayer_format.h\n> > > > > > > @@ -47,6 +47,7 @@ public:\n> > > > > > >       }\n> > > > > > >\n> > > > > > >       static const BayerFormat &fromMbusCode(unsigned int mbusCode);\n> > > > > > > +     unsigned int toMbusCode() const;\n> > > > > > >       bool isValid() const { return bitDepth != 0; }\n> > > > > > >\n> > > > > > >       std::string toString() const;\n> > > > > > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> > > > > > > index f27cc1662d25..fdbc4af1dcc0 100644\n> > > > > > > --- a/src/libcamera/bayer_format.cpp\n> > > > > > > +++ b/src/libcamera/bayer_format.cpp\n> > > > > > > @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)\n> > > > > > >               return it->second;\n> > > > > > >  }\n> > > > > > >\n> > > > > > > +/**\n> > > > > > > + * \\brief Retrieve the media bus code corresponding this this BayerFormat\n> > > > > > > + * \\return The corresponding media bus code, or zero if none was found\n> > > > > > > + */\n> > > > > > > +unsigned int BayerFormat::toMbusCode() const\n> > > > > > > +{\n> > > > > > > +     auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),\n> > > > > > > +                            [this](const auto &i) { return i.second == *this; });\n> > > > > > > +     return it != mbusCodeToBayer.end() ? it->first : 0;\n> > > > > >\n> > > > > > I'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10\n> > > > > > It doesn't register flips, so the bayer pattern should not be changed\n> > > > > >\n> > > > > > The code you added in the next patch for the CameraSensor class goes\n> > > > > > as\n> > > > > >\n> > > > > >         for (const auto &format : formats) {\n> > > > > >                 unsigned int mbusCode = format.first;\n> > > > > >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > > > >\n> > > > > >                 if (bayerFormat.isValid())\n> > > > > >                         mbusCode = bayerFormat.transform(transform).toMbusCode();\n> > > > > >\n> > > > > >                 if (mbusCode)\n> > > > > >                         formats_[mbusCode] = std::move(format.second);\n> > > > > >         }\n> > > > > >\n> > > > > >\n> > > > > > The first\n> > > > > >\n> > > > > >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > > > >\n> > > > > > returns  { BayerFormat::BGGR, 10, BayerFormat::Packing::None }\n> > > > > >\n> > > > > > we go throgh a transform() which does nothing as it receives Indentity\n> > > > > > and then call toMbusCode() on it\n> > > > > >\n> > > > > > The above implementation of toMbusCode() walks the mbusCodeToBayer map\n> > > > > > looking for a matching bayer. Look at this table section\n> > > > > >\n> > > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > > >         { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > > >\n> > > > > > So here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and\n> > > > > > MEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same\n> > > > > > { BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly\n> > > > > > get my original 0x3007 code transformed to 0x3003!\n> > > > > >\n> > > > > > I'm afraid the fact mbus_code-to-BayerFormat relationship is not\n> > > > > > unique is a blocker for this patch\n> > > > >\n> > > > > Oh dear! Thanks for discovering this, I had no idea that these formats existed.\n> > > >\n> > > > Why do those formats exist, and why only the BGGR ordering versions?\n> > > > A partial implementation of something, and presumably on a parallel\n> > > > interface as formats such as YUYV are mandated to use the 1X16\n> > > > versions (not 2X8) on CSI.\n> > > >\n> > > > Are they actually relevant, or legacy cruft?\n> > > >\n> > > >   Dave\n> > > >\n> > > > > I wonder if the correct solution is to have 4 extra packing formats,\n> > > > > alongside the existing CSI2 and IPU3 ones. What do you think?\n> > > > >\n> > > > > David\n> > > > >\n> > > > > >\n> > > > > >\n> > > > > >\n> > > > > >\n> > > > > >\n> > > > > > > +}\n> > > > > > > +\n> > > > > > >  /**\n> > > > > > >   * \\fn BayerFormat::isValid()\n> > > > > > >   * \\brief Return whether a BayerFormat is valid\n> > > > > > > --\n> > > > > > > 2.38.1\n> > > > > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 94FF6C3240\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 11 Jan 2023 13:45:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0E1D625CF;\n\tWed, 11 Jan 2023 14:45:31 +0100 (CET)","from mail-vk1-xa2a.google.com (mail-vk1-xa2a.google.com\n\t[IPv6:2607:f8b0:4864:20::a2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B9CE61F06\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 11 Jan 2023 14:45:30 +0100 (CET)","by mail-vk1-xa2a.google.com with SMTP id i82so6976795vki.8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 11 Jan 2023 05:45:30 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673444732;\n\tbh=QwzGdowoTdZweEMVqfHezJ0Z0ut4vh3BLsv/9AN2YXM=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=P/1mld2GSu7LNrE36Oqq2J7CvqM1nTcocqDZP+XTLjXEMnM+wXin4jgWJ0k3KxJhc\n\trvbRkf4L5o2oyeDx822IqhqjZKC6UpBfrxOFVQisWeHhJ+fbcIzVKwYKWor9wcl1iH\n\tLhwW6BG6R/S4rD/0SFztDCkN1RfiZO39foEQZOOnLu+uuxjoijFtU/Uo9J6koT9t2L\n\tz7++CN1RLQNFSx2GjpWWqIpzwUp1sJWrMwDpZzybEYBBkbHDBl2i3T16+QscUAWqgh\n\tKvs/SOoH2k6AdOh5zt/R0vvkdGeJms9nPfp+NztzNwhzKtJJt0E0O1ly7+2aBiJWKA\n\toVKkqBcwAO7AQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=1vZ2371tnBdoogVyJl1v54NNmXfOuoW1G7Qa1LNGnVk=;\n\tb=pwQ+l97BTXaI/D4hO2OJYU2BmpKYcGjERba+cpYt5XYXXj3/hSQ+daEaDlFm8Ngrlz\n\tzjRs7ORZkHub0wqKzq4NVNmK+OEpGrL5nZnpbr5nIE5Vyzkx+BV7Y+Z33lQYY6tHkK8H\n\tg6xBIXW7KkqYJNBtt371ABSRbJ1vMatuycEy+RJ0f9tHv1fl4DpoBpnnjgndjRdHnwEV\n\t5hWXydljpIJYssMViCZxfrJ6/YXSSQBd1MvSDlER34mq6I3EA0YGGUZkJoVyydmakrgc\n\tiQspwgkcHxwLCpn+ZS25F8MEsV3F2zWtSVHtREVmdxtj+LnibPdtmV1+o94EbYN3Nyax\n\t7vBA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"pwQ+l97B\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=1vZ2371tnBdoogVyJl1v54NNmXfOuoW1G7Qa1LNGnVk=;\n\tb=LUSzcuT1gAqMCJYWba2dvVz8GriaxwTYYC1J+vDXsPt36emshJnYlFVcE3R1xyg0Rm\n\tvYe8E2NpXKbOy6MyfjFfdbBznWtHjZFCZCS1LXV4vs+mIqGeG7IyOb96Ob8VNShAZxI1\n\tYAOkR8q4/30KumzicLyhnurU22Nrheuy3xwK/Ec9PIjuDHsPMdzWGHro+x07/OI/KLyB\n\tupulIBCi5CQb+3+LetEZ9WQNzCDn3bsCHAJfJKT/WEsCogIONwxW6+5u3dVa2b2/qufk\n\tO//8z/TMvw7P+9178rvQraSXNBlqii6UAO88FmhP33p5s0Y7EeiArItvAF7MILkWpEOA\n\tAXaA==","X-Gm-Message-State":"AFqh2krSPSh/xbltiUE78WyJuehQsGSw3FBBJS3NlRrJpUFwn5v7SolU\n\tX5cT3rYwvhfbLzg6aLwD2+9NjGaAYdlrixioOz2unL7eJTnZ+q5P","X-Google-Smtp-Source":"AMrXdXvy4AAci4oP34cmG3KORMd3f8lHdLO/1EELXD4jFbA3pO0QzaZIq3+kZDoLVgc4I3bJX76MNIKWQ91+YniagwE=","X-Received":"by 2002:a1f:1243:0:b0:3da:e97f:4dd7 with SMTP id\n\t64-20020a1f1243000000b003dae97f4dd7mr716925vks.36.1673444728914;\n\tWed, 11 Jan 2023 05:45:28 -0800 (PST)","MIME-Version":"1.0","References":"<20221124121220.47000-1-jacopo@jmondi.org>\n\t<20221124121220.47000-2-jacopo@jmondi.org>\n\t<20230104131229.uwwchptzslflzyjr@uno.localdomain>\n\t<CAHW6GYL26Ws3fPWWp0P60-PMn4nz-5Onr8ba0V0-YL3iGwT1Ug@mail.gmail.com>\n\t<CAPY8ntCy54XsgdOgG1hAXrFVSTnEaDR_x+UP7Gif6Lk2W0mn=A@mail.gmail.com>\n\t<CAHW6GY+amBOp2H6wjm3fbYUrmvwkSDVbMSEyEj-kwyfZpOkJAQ@mail.gmail.com>\n\t<20230110123823.lcxhm7plu52xbhyb@uno.localdomain>\n\t<CAHW6GY+33J-RV+FdsRFyenhcHonL=R5u=9hwA8NBUJ2MKQR12Q@mail.gmail.com>","In-Reply-To":"<CAHW6GY+33J-RV+FdsRFyenhcHonL=R5u=9hwA8NBUJ2MKQR12Q@mail.gmail.com>","Date":"Wed, 11 Jan 2023 13:45:13 +0000","Message-ID":"<CAPY8ntDUvwbaRbF1a5NMC3bWGbCbCMK3S4Tbj-f66uxAi+eRyA@mail.gmail.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Dave Stevenson via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Dave Stevenson <dave.stevenson@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26209,"web_url":"https://patchwork.libcamera.org/comment/26209/","msgid":"<20230111154719.w7ekg7u26e6cv7it@uno.localdomain>","date":"2023-01-11T15:47:19","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi\n\nOn Wed, Jan 11, 2023 at 01:45:13PM +0000, Dave Stevenson wrote:\n> Hi\n>\n> On Tue, 10 Jan 2023 at 14:47, David Plowman\n> <david.plowman@raspberrypi.com> wrote:\n> >\n> > Hi Jacopo\n> >\n> > On Tue, 10 Jan 2023 at 12:38, Jacopo Mondi <jacopo@jmondi.org> wrote:\n> > >\n> > > Hi David,\n> > >\n> > > On Tue, Jan 10, 2023 at 11:59:19AM +0000, David Plowman wrote:\n> > > > Hi again\n> > > >\n> > > > I was thinking about this a bit more. One thing I wanted to clarify\n> > > > was what media bus formats really mean. For example, if I have\n> > > > MEDIA_BUS_FMT_SBGGR10_1X10, presumably that means Bayer BGGR with CSI2\n> > > > packing. Does that sound right?\n> > >\n> > > Not per my understanding\n> > >\n> > > The \"CSI-2 packaged\" Bayer format are described by the 'P' variant of\n> > > the canonical raw Bayer formats packaged in 5 bytes according to the\n> > > CSI-2 specifications.\n> > >\n> > > In example section \"11.4.4 RAW10\" specifies\n> > > The transmission of 10-bit Raw data is accomplished by packing the\n> > > 10-bit pixel data to look like 8-bit data format.\n> > >\n> > > Which means the first 4 bytes will contain the 8 MSB and the last\n> > > byte contains the 2 LSB of each pixel.\n> > >\n> > > V4L2 defines pixel formats for CSI-2 packaged bayer formats, but not\n> > > media bus codes as far as I can tell:\n> > > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10p.html\n> > >\n> > > This surprises me as I would be expecting that being the formats\n> > > defined by the CSI-2 specs that's actually what it is sent on the wire\n> > > but as far as I can tell sensors I've seen do not send RAW10 that way?\n> > >\n> > > Am I confused ?\n> >\n> > Well, I'm pretty baffled by the whole thing too.\n> >\n> > We're definitely using MEDIA_BUS_FMT_SBGGR10_1X10 to mean CSI-2 packed\n> > 10-bit BGGR, and looking at all the media bus formats, I can't see\n> > there's anything else. So it must be true. ??\n>\n\nI interpret this as a confirmation that 10 bits RAW bayer is sent as\n\n        +------+------+------+------+------+\n        | MSB0 | MSB1 | MSB2 | MSB3 | LSBs |\n        +------+------+------+------+------+\n\nby sensors. That's what the CSI-2 specification say, so I shouldn't be\nsurprised (with a little thinking, it makes sense to arrange data in\n8-bits samples for CSI-2 as, at least on D-PHY, it's a multiple (or a\ndivider) of the number of data lanes...)\n\nIf that's the case, yes, I guess MEDIA_BUS_FMT_SBGGR10_1X10 implies\nthe CSI-2 packaging as you originally suggested,\n\n\n> It is all rather ugly when it comes to CSI2 as it doesn't define the\n> bus format in any sensible shape or form.\n>\n> The 1X10, 2X8, 1X16 stuff is all related to a parallel bus taking a\n> bus width and number of transfers to pass the pixel value. To\n> accurately represent CSI2 it would therefore be representing the\n> number of data lanes as well as the format, and that is not\n> information that userspace should be worrying about.\n>\n> Fundamentally for CSI-2, there is no padding in the datastream,\n> therefore 1X.. is the closest representation. Any unpacking into a 16\n> bit representation (ie BGGR10 vs BGGR10P) is done in the receiver.\n> RPi doesn't have a parallel camera interface so we really don't care,\n> but libcamera doesn't have that constraint.\n>\n\nThat \"constraint\" or that \"privilege\" ? :)\n\n> Even on a parallel bus MEDIA_BUS_FMT's don't fully work, as the\n> physical bus isn't necessarily a 1:1 mapping.\n> Less so with Bayer data, but there's an ongoing discussion for DPI\n> displays where you might have a display that wants RGB666. The panel\n> could legitimately advertise MEDIA_BUS_FMT_RGB666_1X18, but the wiring\n> to the host could be packed onto a 24 bit bus so wants\n> MEDIA_BUS_FMT_RGB888_1X24 (with 6 wires not connected), or\n> MEDIA_BUS_FMT_RGB666_1X24_CPADHI, or a number of others. Two different\n> formats needed on opposite ends of the same link :-(\n>\n> Yes with any 2X.. formats it matters, but for any 1X.. format, it\n> really doesn't.\n>\n> > >\n> > >\n> > > >\n> > > > Under those circumstances it seems reasonable to me that the\n> > > > mbusCodeToBayer table (bayer_format.cpp) should specify Packing::CSI2P\n> > > > for most of the formats there, but with new values (e.g.\n> > > > Packing::PADHI_BE) for the ones now under discussion.\n> > > >\n> > > > I don't think such a change would bother Raspberry Pi (it might well\n> > > > \"just work\", or if not require only very minor tweaks), but someone\n> > > > else would have to comment on ipu3 or rkisp1.\n> > > >\n> > >\n> > > Please note that 2X8 formats are for parallel busses only, and the\n> > > PAD.. only apply to them. I wonder if we do care about them for real..\n> >\n> > That's a fair point, though I do feel there's something wrong when we\n> > can't get back the same \"kind\" of mbus format as we put in. If it's\n> > not too difficult to sort out, we probably should. ??\n>\n> The only source for MEDIA_BUS_FMT_SBGGR10_2X8_PAD* is the Sharp\n> RJ54N1CB0C, and it also supports MEDIA_BUS_FMT_SBGGR10_1X10.\n> That means you haven't a hope of writing generic code to be able to\n> reverse the conversion without defining it as packing or similar.\n>\n> drivers/gpu/ipu-v3/ipu-csi.c and\n> drivers/media/platform/intel/pxa_camera have support for receiving\n> these formats.\n>\n> Are i.MX5, i.MX6Q or Intel PXA27x Quick Capture Interface supported by\n> libcamera? If not, then drop the mappings, and worry about it when\n> someone tries to interface a RJ54N1CB0C with libcamera.\n>\n\nThese are all fair points and I agree David's patch could be made to\nwork if we guarantee the mbusCodeToBayer table is a 1:1 mapping. One\nBayerFormat for one media bus code and you can get from one to another\nwithout issues.\n\nimx6q is supported by the simple pipeline handler, but I guess the\ndecision is more about how to support parallel RAW sensor that produce\nMEDIA_BUS_FMT_...10_2X8_PAD.. formats than the platform.\n\nAs David suggested it could be done by adding a dedicated Packing::\nand transform all the existing Packing::None in Packing::CSI2 in\nmbusCodeToBayer[].\n\nBut there's one point that worries me: we have two V4L2_PIX_FMT_ for\neach raw format, as bayer data might be stored into memory:\n\n- as they're received from the CSI-2 bus (the P format)\n- re-pacakged by the CSI-2 receiver in 10 bits samples (the non-P format)\n\nLooking at BayerFormat::fromV4L2PixelFormat() I wonder what happens if:\n(code from your validate() implementation)\n\nfourcc = V4L2_PIX_FMT_SGRBG10P;\nBayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);\n        this returns:\n        { MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::Packing::CSI2 } },\n\nfourcc = bayer.toV4L2PixelFormat();\n        this fails as there's no entry with Packing:CSI2 in the\n        mbusCodeToBayer[] table\n\nIf we switch all the entries to use Packing::CSI2 we would have the\nsame problem if the video device reports the re-packaged pix format:\n\nfourcc = V4L2_PIX_FMT_SGRBG10;\nBayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);\n        this returns:\n        { MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::Packing::None } },\n\nfourcc = bayer.toV4L2PixelFormat();\n        this fails as there's no entry with Packing:None in the\n        mbusCodeToBayer[] table\n\nDoes you pipeline handler ever select the P format variants from the\nunicam video device ?\n\n> > >\n> > > > Dave also mentions that all the non-BGGR versions are missing (it\n> > > > seems a bit disappointing that they weren't added all at the same\n> > > > time), but for the moment I suggest we ignore that.\n> > > >\n> > > > I also note there's a potential problem with the \"ALAW8\" and \"DPCM8\"\n> > > > formats. I don't know if folks think those can be handled by more\n> > > > \"packing\" formats, or whether we'd rather have a new \"modifiers\"\n> > > > field?\n> > >\n> > > ALAW and DPCM8 could be described as \"compressions\" more than\n> > > packaging ? Reading\n> > > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10alaw8.html\n> > > it seems to imply a 10-bit pixel gets compressed to an 8-bit sample,\n> > > so that impacts the buffer and line sizes ?\n> >\n> > I think these would actually work if we treated them as \"packing\" too.\n> > \"compression\" is just a fancier form of packing, kind of. Or maybe we\n> > should rename the \"packing\" field to \"modifier\", that might feel less\n> > strange.\n>\n> I'll agree that ALAW and DPCM would be packings as they affect how the\n> data has to be interpreted. They also map through to V4L2 formats.\n>\n> Whether anyone other than vimc actually still uses them is a totally\n> different question.\n> I see et8ek8 (wow, blast from the past) has a mode using DPCM 10 to 8,\n> and CCS supports it, but most users these days care about image\n> quality more than bandwidth.\n>\n> I don't see reference to ALAW in the CSI-2 spec, so I wonder where\n> that one came from.\n\nNo idea but those concerning to me that the 4 _2X8_PAD variants.\n\n>\n>   Dave\n>\n> > >\n> > > >\n> > > > Thoughts?\n> > >\n> > > Yes, and all of them rather confused :)\n> >\n> > Ditto.\n> >\n> > I would be happy to make up a PR to attempt to improve some of this,\n> > but we'd have to find someone else to worry about non-Pi platforms.\n> >\n> > David\n> >\n> > >\n> > > >\n> > > > David\n> > > >\n> > > > On Fri, 6 Jan 2023 at 17:28, Dave Stevenson\n> > > > <dave.stevenson@raspberrypi.com> wrote:\n> > > > >\n> > > > > Hi David and Jacopo\n> > > > >\n> > > > > On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel\n> > > > > <libcamera-devel@lists.libcamera.org> wrote:\n> > > > > >\n> > > > > > Hi Jacopo\n> > > > > >\n> > > > > > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@jmondi.org> wrote:\n> > > > > > >\n> > > > > > > Hi David\n> > > > > > >    sorry to revive this, but I'm carrying this series in my tree and I\n> > > > > > > just found a bug in this patches\n> > > > > > >\n> > > > > > >\n> > > > > > > On Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:\n> > > > > > > > From: David Plowman <david.plowman@raspberrypi.com>\n> > > > > > > >\n> > > > > > > > This makes it easier to perform transformations on Bayer type mbus\n> > > > > > > > codes by converting them to a BayerFormat, doing the transform, and\n> > > > > > > > then converting them back again.\n> > > > > > > >\n> > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > > > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > > > > > > ---\n> > > > > > > >  include/libcamera/internal/bayer_format.h |  1 +\n> > > > > > > >  src/libcamera/bayer_format.cpp            | 11 +++++++++++\n> > > > > > > >  2 files changed, 12 insertions(+)\n> > > > > > > >\n> > > > > > > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> > > > > > > > index 78ba3969913d..3601dccb7228 100644\n> > > > > > > > --- a/include/libcamera/internal/bayer_format.h\n> > > > > > > > +++ b/include/libcamera/internal/bayer_format.h\n> > > > > > > > @@ -47,6 +47,7 @@ public:\n> > > > > > > >       }\n> > > > > > > >\n> > > > > > > >       static const BayerFormat &fromMbusCode(unsigned int mbusCode);\n> > > > > > > > +     unsigned int toMbusCode() const;\n> > > > > > > >       bool isValid() const { return bitDepth != 0; }\n> > > > > > > >\n> > > > > > > >       std::string toString() const;\n> > > > > > > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> > > > > > > > index f27cc1662d25..fdbc4af1dcc0 100644\n> > > > > > > > --- a/src/libcamera/bayer_format.cpp\n> > > > > > > > +++ b/src/libcamera/bayer_format.cpp\n> > > > > > > > @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)\n> > > > > > > >               return it->second;\n> > > > > > > >  }\n> > > > > > > >\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Retrieve the media bus code corresponding this this BayerFormat\n> > > > > > > > + * \\return The corresponding media bus code, or zero if none was found\n> > > > > > > > + */\n> > > > > > > > +unsigned int BayerFormat::toMbusCode() const\n> > > > > > > > +{\n> > > > > > > > +     auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),\n> > > > > > > > +                            [this](const auto &i) { return i.second == *this; });\n> > > > > > > > +     return it != mbusCodeToBayer.end() ? it->first : 0;\n> > > > > > >\n> > > > > > > I'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10\n> > > > > > > It doesn't register flips, so the bayer pattern should not be changed\n> > > > > > >\n> > > > > > > The code you added in the next patch for the CameraSensor class goes\n> > > > > > > as\n> > > > > > >\n> > > > > > >         for (const auto &format : formats) {\n> > > > > > >                 unsigned int mbusCode = format.first;\n> > > > > > >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > > > > >\n> > > > > > >                 if (bayerFormat.isValid())\n> > > > > > >                         mbusCode = bayerFormat.transform(transform).toMbusCode();\n> > > > > > >\n> > > > > > >                 if (mbusCode)\n> > > > > > >                         formats_[mbusCode] = std::move(format.second);\n> > > > > > >         }\n> > > > > > >\n> > > > > > >\n> > > > > > > The first\n> > > > > > >\n> > > > > > >                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > > > > >\n> > > > > > > returns  { BayerFormat::BGGR, 10, BayerFormat::Packing::None }\n> > > > > > >\n> > > > > > > we go throgh a transform() which does nothing as it receives Indentity\n> > > > > > > and then call toMbusCode() on it\n> > > > > > >\n> > > > > > > The above implementation of toMbusCode() walks the mbusCodeToBayer map\n> > > > > > > looking for a matching bayer. Look at this table section\n> > > > > > >\n> > > > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > > > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > > > >         { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },\n> > > > > > >\n> > > > > > > So here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and\n> > > > > > > MEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same\n> > > > > > > { BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly\n> > > > > > > get my original 0x3007 code transformed to 0x3003!\n> > > > > > >\n> > > > > > > I'm afraid the fact mbus_code-to-BayerFormat relationship is not\n> > > > > > > unique is a blocker for this patch\n> > > > > >\n> > > > > > Oh dear! Thanks for discovering this, I had no idea that these formats existed.\n> > > > >\n> > > > > Why do those formats exist, and why only the BGGR ordering versions?\n> > > > > A partial implementation of something, and presumably on a parallel\n> > > > > interface as formats such as YUYV are mandated to use the 1X16\n> > > > > versions (not 2X8) on CSI.\n> > > > >\n> > > > > Are they actually relevant, or legacy cruft?\n> > > > >\n> > > > >   Dave\n> > > > >\n> > > > > > I wonder if the correct solution is to have 4 extra packing formats,\n> > > > > > alongside the existing CSI2 and IPU3 ones. What do you think?\n> > > > > >\n> > > > > > David\n> > > > > >\n> > > > > > >\n> > > > > > >\n> > > > > > >\n> > > > > > >\n> > > > > > >\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > >  /**\n> > > > > > > >   * \\fn BayerFormat::isValid()\n> > > > > > > >   * \\brief Return whether a BayerFormat is valid\n> > > > > > > > --\n> > > > > > > > 2.38.1\n> > > > > > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 E9544C3292\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 11 Jan 2023 15:47:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3F99861F06;\n\tWed, 11 Jan 2023 16:47:24 +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 8D81C61F06\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 11 Jan 2023 16:47:22 +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 12AD94DD;\n\tWed, 11 Jan 2023 16:47:22 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673452044;\n\tbh=PosqR1i8RKX8lS7kqqjtJHO8U/bbF00tN9F+KCHDYok=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=xlQB+oh0Nd6Qc+tCDVj6j2DTFoIammynTzQwAzfZdRVDwAOWM+EtAC6e+px1rsjmD\n\t1nPwOs+p6xa9JWrqCXbW0sM4e/QL39fjf4iozTSSuUTOyHUv7PiZPKDAC3aCml2ged\n\tXknCswPePjeJxZMzYY8BRChdyTKdayEjyp1EourAZ5Ed7HbImzaUwaVSldIBNa2akC\n\txWA4kWM1FgiNKTGozt42xujYxzNJdlgIKbCD2KDHoUix3REt0dJ8ambKYUYBscJxR5\n\tRRPKXNriu21pBsHA0tZOKM0Q4ALr+gveuWDd/sWokXWnSMBK73xeoC7usUSy5VhDhU\n\tmcynajiNkamog==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1673452042;\n\tbh=PosqR1i8RKX8lS7kqqjtJHO8U/bbF00tN9F+KCHDYok=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FtM+Zl4tX1VmhNn8jwW5U2+QxFMtj4mv/tqFy2xQNq5BPxghG8flIdfDd2DfCHmeD\n\tNrgzfTQ62W+6poLkRNPfBWay/Nww1NwWXp2guIXxGXZjoD/4BeDcK3FUY/CHOSsLvF\n\tY2f3RtH0DGyjUJcK4/azc+Jm6ikVMYJf59n9+ea8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FtM+Zl4t\"; dkim-atps=neutral","Date":"Wed, 11 Jan 2023 16:47:19 +0100","To":"Dave Stevenson <dave.stevenson@raspberrypi.com>","Message-ID":"<20230111154719.w7ekg7u26e6cv7it@uno.localdomain>","References":"<20221124121220.47000-1-jacopo@jmondi.org>\n\t<20221124121220.47000-2-jacopo@jmondi.org>\n\t<20230104131229.uwwchptzslflzyjr@uno.localdomain>\n\t<CAHW6GYL26Ws3fPWWp0P60-PMn4nz-5Onr8ba0V0-YL3iGwT1Ug@mail.gmail.com>\n\t<CAPY8ntCy54XsgdOgG1hAXrFVSTnEaDR_x+UP7Gif6Lk2W0mn=A@mail.gmail.com>\n\t<CAHW6GY+amBOp2H6wjm3fbYUrmvwkSDVbMSEyEj-kwyfZpOkJAQ@mail.gmail.com>\n\t<20230110123823.lcxhm7plu52xbhyb@uno.localdomain>\n\t<CAHW6GY+33J-RV+FdsRFyenhcHonL=R5u=9hwA8NBUJ2MKQR12Q@mail.gmail.com>\n\t<CAPY8ntDUvwbaRbF1a5NMC3bWGbCbCMK3S4Tbj-f66uxAi+eRyA@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAPY8ntDUvwbaRbF1a5NMC3bWGbCbCMK3S4Tbj-f66uxAi+eRyA@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add\n\ttoMbusCode method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]