[{"id":32020,"web_url":"https://patchwork.libcamera.org/comment/32020/","msgid":"<20241104235314.GI14276@pendragon.ideasonboard.com>","date":"2024-11-04T23:53:14","subject":"Re: [PATCH 8/8] libcamera: Add new atomisp pipeline handler","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\n(CC'ing Sakari)\n\nThank you for the patch.\n\nA few high-level questions first.\n\nOn Sun, Nov 03, 2024 at 04:22:05PM +0100, Hans de Goede wrote:\n> Add a basic atomisp pipeline handler which supports configuring\n> the pipeline, capturing frames and selecting front/back sensor.\n> \n> The atomisp ISP needs some extra lines/columns when debayering and also\n> has some max resolution limitations, this causes the available output\n> resolutions to differ from the sensor resolutions.\n> \n> The atomisp driver's Android heritage means that it mostly works as a non\n> media-controller centric v4l2 device, primarily controlled through its\n> /dev/video# node.\n\nCould that be fixed on the kernel side (assuming someone would be able\nto do the work of course) ?\n\n> The driver takes care of setting up the pipeline itself\n> propagating try / set fmt calls down from its single /dev/video# node to\n> the selected sensor taking the necessary padding, etc. into account.\n> \n> Therefor things like getting the list of support formats / sizes and\n> setFmt() calls are all done on the /dev/video# node instead of on subdevs,\n> this avoids having to duplicate the padding, etc. logic in the pipeline\n> handler.\n> \n> Since the statistics buffers which we get from the ISP2 are not documented\n\nCould the stats format be reverse-engineered ? Or alternatively, could\nIntel provide documentation (waving at Sakari) ?\n\n> this uses the swstats_cpu and simple-IPA from the swisp. At the moment only\n> aec/agc is supported.\n> \n> awb support will be added in a follow-up patch.\n> \n> Signed-off-by: Hans de Goede \n\n[snip]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 955CDBDB1C\n\tfor ;\n\tMon, 4 Nov 2024 23:53:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58DE5653E4;\n\tTue, 5 Nov 2024 00:53: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 1C33B65399\n\tfor ;\n\tTue, 5 Nov 2024 00:53:22 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 70586475;\n\tTue, 5 Nov 2024 00:53:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"n0cgL2nk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730764395;\n\tbh=UvpMzWa91tsyhK/Gkzh+AZ1aD/eiwcUyuvei5TrKhJE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n0cgL2nkobKGjSQRa/BrmJkRNousoGAq0a2+UUNlnQjiWbDMbZ5Wb24dcUhe9ZEXw\n\twvUQsdj7Z4JaTa1xyDOS68KdFMPUb/3UC+LvvdaSIfIrsW2HW+aFOjLoN42vXHIcqW\n\tKD7+y9OQ8rVh2caBFWQEHzqBeWRipiwCGVUuknZU=","Date":"Tue, 5 Nov 2024 01:53:14 +0200","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard , Sakari Ailus ","Subject":"Re: [PATCH 8/8] libcamera: Add new atomisp pipeline handler","Message-ID":"<20241104235314.GI14276@pendragon.ideasonboard.com>","References":"<20241103152205.29219-1-hdegoede@redhat.com>\n\t<20241103152205.29219-9-hdegoede@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241103152205.29219-9-hdegoede@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32050,"web_url":"https://patchwork.libcamera.org/comment/32050/","msgid":"","date":"2024-11-06T13:25:31","subject":"Re: [PATCH 8/8] libcamera: Add new atomisp pipeline handler","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Laurent,\n\nOn 5-Nov-24 12:53 AM, Laurent Pinchart wrote:\n> Hi Hans,\n> \n> (CC'ing Sakari)\n> \n> Thank you for the patch.\n> \n> A few high-level questions first.\n> \n> On Sun, Nov 03, 2024 at 04:22:05PM +0100, Hans de Goede wrote:\n>> Add a basic atomisp pipeline handler which supports configuring\n>> the pipeline, capturing frames and selecting front/back sensor.\n>>\n>> The atomisp ISP needs some extra lines/columns when debayering and also\n>> has some max resolution limitations, this causes the available output\n>> resolutions to differ from the sensor resolutions.\n>>\n>> The atomisp driver's Android heritage means that it mostly works as a non\n>> media-controller centric v4l2 device, primarily controlled through its\n>> /dev/video# node.\n> \n> Could that be fixed on the kernel side (assuming someone would be able\n> to do the work of course) ?\n\nYes, note that the current kernel driver already uses the media-controller\nand has separate subdevs for the ISP, CSI receivers, sensors and VCM,\nsee e.g. the 2 attached pngs for 2 different setups (generated by dot).\n\nAnd the atomisp pipeline handler e.g. already configures mc-links to\nselect which sensor to use.\n\nSo we are already part way there. The thing which currently is not\nvery mc-centric is that a single set_fmt call is made on /dev/video#\nafter setting the mc-links and then that configures the fmts\non all the subdevs taking the special resolution-padding requirements\nof the ISP into account.\n\nCurrently atomisp kernel code already allocates and initializes\na bunch of ISP contexts at this set_fmt call time (rather then\nat request-buffers time) and more importantly it selects which\npipeline program (since the ISP is not fixed function) to run on\nthe ISP at this time. Changing that is very much no trivial.\n\nI guess we could keep allocating those at that time and have\na flag (ioctl / v4l2-ctrl?) to skip the propagating of the fmts\nto the subdevs and instead having the pipeline handler set\nthe subdev fmts itself, but I do not see much added value in that\natm.\n\n>> The driver takes care of setting up the pipeline itself\n>> propagating try / set fmt calls down from its single /dev/video# node to\n>> the selected sensor taking the necessary padding, etc. into account.\n>>\n>> Therefor things like getting the list of support formats / sizes and\n>> setFmt() calls are all done on the /dev/video# node instead of on subdevs,\n>> this avoids having to duplicate the padding, etc. logic in the pipeline\n>> handler.\n>>\n>> Since the statistics buffers which we get from the ISP2 are not documented\n> \n> Could the stats format be reverse-engineered ? Or alternatively, could\n> Intel provide documentation (waving at Sakari) ?\n\nI have asked Salari about this already, but with these kinda things\nit is going to take a while to get an official yes / no answer.\n\n>> this uses the swstats_cpu and simple-IPA from the swisp. At the moment only\n>> aec/agc is supported.\n>>\n>> awb support will be added in a follow-up patch.\n\nRegards,\n\nHans","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8270FBE173\n\tfor ;\n\tWed, 6 Nov 2024 13:25:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7AAA65429;\n\tWed, 6 Nov 2024 14:25:39 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D3E0653C5\n\tfor ;\n\tWed, 6 Nov 2024 14:25:38 +0100 (CET)","from mail-ej1-f69.google.com (mail-ej1-f69.google.com\n\t[209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-7-XdlFfwFmPZWDoQtnNLhdAw-1; Wed, 06 Nov 2024 08:25:35 -0500","by mail-ej1-f69.google.com with SMTP id\n\ta640c23a62f3a-a9a22a62e80so136930466b.1\n\tfor ;\n\tWed, 06 Nov 2024 05:25:35 -0800 (PST)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a9eb17d7420sm284271366b.109.2024.11.06.05.25.31\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 06 Nov 2024 05:25:32 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"hNP7KHtf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1730899537;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=0NbXKY7VBhavBOerfNpFFVBS3tFX3IONNLCF+r3dcSE=;\n\tb=hNP7KHtfO0z5MOY/3cylncIylwl4b02QDMmsGU/U/CQ5MJkYJDwo01CuiNhEKkIZrzumE8\n\t/OWSri40m2LSUpZ0+66pV1pAo49aVZHDEIjC/8uY11zlKRrsjFj30QMDcmqCxp9vmGyHAj\n\tmhVG7+fSjNfCmyBPe4aDIRr/rsSfskw=","X-MC-Unique":"XdlFfwFmPZWDoQtnNLhdAw-1","X-Mimecast-MFC-AGG-ID":"XdlFfwFmPZWDoQtnNLhdAw","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730899534; x=1731504334;\n\th=in-reply-to:from:content-language:references:cc:to:subject\n\t:user-agent:mime-version:date:message-id:x-gm-message-state:from:to\n\t:cc:subject:date:message-id:reply-to;\n\tbh=QyRa0d9ftmYcDfpai0T5K8ztCAoKcsXYT+LoGsC08SE=;\n\tb=imZYCCHtK/F6K8iC8nnqgBdYDAEkfJEM3Wzdb4bLi5FedHUjYtZYBa35OgTmVfFDth\n\tfT6BTBUyeXEj2OtmwW6x+tkYN/KNGT5ts94du/4Fk9X3LOv5tf1Uxp3wBYthomF+2jCh\n\tLx/ZJ6RwxxhPCbiSE9k8a2TMgxcKceuQe2hSyzAvDW7CYUIUg7ulivnLwwWmWcBzPgUZ\n\tjqYDq5U9B0HzknIutJKhAexghuDuuJgoBqHUJkXvzrXu4nGRj9g5Kq0yp1sRHYWruAOt\n\tQgqfgTDWUAK6Ob5jJnOZfhUoe70oFA26v7JhcXWDLUgjp9hAkHnKFYLLNMVsvNobxuOa\n\tZAuw==","X-Gm-Message-State":"AOJu0YyzAAud8Lp3RNdFB1DoCq03y8Ob6CQgn98BwR67TEPFto+2Uhwk\n\t6zd/Uv4hZ+XrUrOOlhE6945vhj32taIVP04V0/R7NDXlWGccO6eaVF5BEKLCuH+Fdk3NFdEGZiV\n\tKJpJbHrsvRynTsK70dkMM13ULnzXaPYokM7WRkGI24nrK0Z8Z2K6IU6eqf6lVLq9lthJkqzg=","X-Received":["by 2002:a17:907:9495:b0:a9a:161:8da4 with SMTP id\n\ta640c23a62f3a-a9de619f1b6mr3796173566b.55.1730899534001; \n\tWed, 06 Nov 2024 05:25:34 -0800 (PST)","by 2002:a17:907:9495:b0:a9a:161:8da4 with SMTP id\n\ta640c23a62f3a-a9de619f1b6mr3796165966b.55.1730899532658; \n\tWed, 06 Nov 2024 05:25:32 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHFCHpSw374omWL5TO6TeDYAyI9nuvDOpTpJ4Gy4sR1MSTDq69Wij5bLzyz3KdSdysHfATEzQ==","Message-ID":"","Date":"Wed, 6 Nov 2024 14:25:31 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 8/8] libcamera: Add new atomisp pipeline handler","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard , Sakari Ailus ","References":"<20241103152205.29219-1-hdegoede@redhat.com>\n\t<20241103152205.29219-9-hdegoede@redhat.com>\n\t<20241104235314.GI14276@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20241104235314.GI14276@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"16eaFNKcjN4TgLn74kltnVRMXw_gbyYZyDOpkVGVttM_1730899534","X-Mimecast-Originator":"redhat.com","Content-Type":"multipart/mixed;\n\tboundary=\"------------SsrK8G0M8OPKG75EYz1ZB2sJ\"","Content-Language":"en-US, nl","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32051,"web_url":"https://patchwork.libcamera.org/comment/32051/","msgid":"<20241106134006.GH9369@pendragon.ideasonboard.com>","date":"2024-11-06T13:40:06","subject":"Re: [PATCH 8/8] libcamera: Add new atomisp pipeline handler","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nOn Wed, Nov 06, 2024 at 02:25:31PM +0100, Hans de Goede wrote:\n> On 5-Nov-24 12:53 AM, Laurent Pinchart wrote:\n> > Hi Hans,\n> > \n> > (CC'ing Sakari)\n> > \n> > Thank you for the patch.\n> > \n> > A few high-level questions first.\n> > \n> > On Sun, Nov 03, 2024 at 04:22:05PM +0100, Hans de Goede wrote:\n> >> Add a basic atomisp pipeline handler which supports configuring\n> >> the pipeline, capturing frames and selecting front/back sensor.\n> >>\n> >> The atomisp ISP needs some extra lines/columns when debayering and also\n> >> has some max resolution limitations, this causes the available output\n> >> resolutions to differ from the sensor resolutions.\n> >>\n> >> The atomisp driver's Android heritage means that it mostly works as a non\n> >> media-controller centric v4l2 device, primarily controlled through its\n> >> /dev/video# node.\n> > \n> > Could that be fixed on the kernel side (assuming someone would be able\n> > to do the work of course) ?\n> \n> Yes, note that the current kernel driver already uses the media-controller\n> and has separate subdevs for the ISP, CSI receivers, sensors and VCM,\n> see e.g. the 2 attached pngs for 2 different setups (generated by dot).\n> \n> And the atomisp pipeline handler e.g. already configures mc-links to\n> select which sensor to use.\n> \n> So we are already part way there.\n\nAh nice :-)\n\n> The thing which currently is not\n> very mc-centric is that a single set_fmt call is made on /dev/video#\n> after setting the mc-links and then that configures the fmts\n> on all the subdevs taking the special resolution-padding requirements\n> of the ISP into account.\n> \n> Currently atomisp kernel code already allocates and initializes\n> a bunch of ISP contexts at this set_fmt call time (rather then\n> at request-buffers time) and more importantly it selects which\n> pipeline program (since the ISP is not fixed function) to run on\n> the ISP at this time. Changing that is very much no trivial.\n\nI see there's quite a bit of untangling that would need to be done\nindeed.\n\nSpeaking of this, how do you plan to handle side-by-side development in\nlibcamera and in the driver ? I don't see how we could ensure backward\ncompatibility in any clean way on either side, would it be fine to tell\nusers they will always have to use the latest version on both sides ?\n\n> I guess we could keep allocating those at that time and have\n> a flag (ioctl / v4l2-ctrl?) to skip the propagating of the fmts\n> to the subdevs and instead having the pipeline handler set\n> the subdev fmts itself, but I do not see much added value in that\n> atm.\n\nBy itself it doesn't add a lot of value indeed, but it would still\nprepare for the future.\n\nAnother thing that would need to be looked at is replacing the ISP\nparameters ioctl API with a parameters buffer. That will be useful to\nset the white balance gains.\n\n> >> The driver takes care of setting up the pipeline itself\n> >> propagating try / set fmt calls down from its single /dev/video# node to\n> >> the selected sensor taking the necessary padding, etc. into account.\n> >>\n> >> Therefor things like getting the list of support formats / sizes and\n> >> setFmt() calls are all done on the /dev/video# node instead of on subdevs,\n> >> this avoids having to duplicate the padding, etc. logic in the pipeline\n> >> handler.\n> >>\n> >> Since the statistics buffers which we get from the ISP2 are not documented\n> > \n> > Could the stats format be reverse-engineered ? Or alternatively, could\n> > Intel provide documentation (waving at Sakari) ?\n> \n> I have asked Salari about this already, but with these kinda things\n> it is going to take a while to get an official yes / no answer.\n> \n> >> this uses the swstats_cpu and simple-IPA from the swisp. At the moment only\n> >> aec/agc is supported.\n> >>\n> >> awb support will be added in a follow-up patch.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5931BBDB13\n\tfor ;\n\tWed, 6 Nov 2024 13:40:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E22F6542B;\n\tWed, 6 Nov 2024 14:40:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5277653C5\n\tfor ;\n\tWed, 6 Nov 2024 14:40:13 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1B203A44;\n\tWed, 6 Nov 2024 14:40:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cZsxW6E3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730900405;\n\tbh=mFPzx2UuHlxNZX75PlX9Dja/rf2R9d4JEi8W3lG7k8Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cZsxW6E3s7sgsHaU1MtCRQQLuAv64wuPyJHTTFHt46oQFtuq2BQ1mhlzIfbBEsBi6\n\trcmAMhvEKYMlUyrLUTqG6MnTMgGgjUBPcnXM2vwfqOUkLggMy88zaapZEQnlaRdzne\n\tfvagZ+KoQyRgZOftR6pas48i1qViutjplrlBs/y8=","Date":"Wed, 6 Nov 2024 15:40:06 +0200","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard , Sakari Ailus ","Subject":"Re: [PATCH 8/8] libcamera: Add new atomisp pipeline handler","Message-ID":"<20241106134006.GH9369@pendragon.ideasonboard.com>","References":"<20241103152205.29219-1-hdegoede@redhat.com>\n\t<20241103152205.29219-9-hdegoede@redhat.com>\n\t<20241104235314.GI14276@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32057,"web_url":"https://patchwork.libcamera.org/comment/32057/","msgid":"","date":"2024-11-06T14:17:19","subject":"Re: [PATCH 8/8] libcamera: Add new atomisp pipeline handler","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Laurent,\n\nOn 6-Nov-24 2:40 PM, Laurent Pinchart wrote:\n> Hi Hans,\n> \n> On Wed, Nov 06, 2024 at 02:25:31PM +0100, Hans de Goede wrote:\n>> On 5-Nov-24 12:53 AM, Laurent Pinchart wrote:\n>>> Hi Hans,\n>>>\n>>> (CC'ing Sakari)\n>>>\n>>> Thank you for the patch.\n>>>\n>>> A few high-level questions first.\n>>>\n>>> On Sun, Nov 03, 2024 at 04:22:05PM +0100, Hans de Goede wrote:\n>>>> Add a basic atomisp pipeline handler which supports configuring\n>>>> the pipeline, capturing frames and selecting front/back sensor.\n>>>>\n>>>> The atomisp ISP needs some extra lines/columns when debayering and also\n>>>> has some max resolution limitations, this causes the available output\n>>>> resolutions to differ from the sensor resolutions.\n>>>>\n>>>> The atomisp driver's Android heritage means that it mostly works as a non\n>>>> media-controller centric v4l2 device, primarily controlled through its\n>>>> /dev/video# node.\n>>>\n>>> Could that be fixed on the kernel side (assuming someone would be able\n>>> to do the work of course) ?\n>>\n>> Yes, note that the current kernel driver already uses the media-controller\n>> and has separate subdevs for the ISP, CSI receivers, sensors and VCM,\n>> see e.g. the 2 attached pngs for 2 different setups (generated by dot).\n>>\n>> And the atomisp pipeline handler e.g. already configures mc-links to\n>> select which sensor to use.\n>>\n>> So we are already part way there.\n> \n> Ah nice :-)\n> \n>> The thing which currently is not\n>> very mc-centric is that a single set_fmt call is made on /dev/video#\n>> after setting the mc-links and then that configures the fmts\n>> on all the subdevs taking the special resolution-padding requirements\n>> of the ISP into account.\n>>\n>> Currently atomisp kernel code already allocates and initializes\n>> a bunch of ISP contexts at this set_fmt call time (rather then\n>> at request-buffers time) and more importantly it selects which\n>> pipeline program (since the ISP is not fixed function) to run on\n>> the ISP at this time. Changing that is very much no trivial.\n> \n> I see there's quite a bit of untangling that would need to be done\n> indeed.\n> \n> Speaking of this, how do you plan to handle side-by-side development in\n> libcamera and in the driver ?\n\nThat is a good question my answer is: \"carefully\".\n\nI hope to be able to make time to get any review comments on\nthe atomisp pipeline handler addressed and post new version\nregularly so that this can get merged in a reasonable time frame.\n\nThen I would like to enable this for Fedora 42 (code complete\ndeadline 18 Feb 2025, final freeze April 1st 2025) and once enabled\nthere some form of compatibility will need to be kept.\n\nAs mentioned in the other thread IMHO it is import to start shipping\nthis to end users in a somewhat usable form to hopefully build\na community around this and get more contributors.\n\nSo lets say that we start with swstats and then manage to switch\nto ISP 3A stats, then I will likely keep the swstats support around\nbehind some flag for testing / comparison at least for a while.\n\nAnd I would e.g. make the pipeline handler detect an older kernel\ndriver and auto switch to the swstats in that case for say approx.\n6 months (so keep swstats support around at least that long).\n\nSo basically my idea would be to not pin ourselves to providing\na stable ABI / compatibility in either direction (kernel > libcamera,\nlibcamera > kernel) forever. But I also don't want things to break\nif the 2 are not upgraded at exactly the same time.\n\nLikewise if the kernel gets a new /dev/video node for stats buffers\nand that is not used by userspace then it should behave as before\nand allocate stats buffers itself and just cycle through those\nas it does now, since I don't think the ISP can work without them.\n\n> I don't see how we could ensure backward\n> compatibility in any clean way on either side, would it be fine to tell\n> users they will always have to use the latest version on both sides ?\n\nSee above. My compromise would be no long term ABI guarantees\n(because staging driver) but add in some leeway by not braking things\nif they get a bit out of sink.\n\n>> I guess we could keep allocating those at that time and have\n>> a flag (ioctl / v4l2-ctrl?) to skip the propagating of the fmts\n>> to the subdevs and instead having the pipeline handler set\n>> the subdev fmts itself, but I do not see much added value in that\n>> atm.\n> \n> By itself it doesn't add a lot of value indeed, but it would still\n> prepare for the future.\n> \n> Another thing that would need to be looked at is replacing the ISP\n> parameters ioctl API with a parameters buffer. That will be useful to\n> set the white balance gains.\n\nInteresting. I have not had time look into this yet. But I think that\ncurrently there is some custom ioctl which passes params including the\nwhite balance gains.\n\nI definitely do not plan to use any of the still existing (a lot have\nbeen removed already) custom atomisp IOCTLs. I was actually thinking\nabout having v4l2-controls on the ISP subdev for the whitebalance\ngains. But if other ISPs are using parameter buffers for this then\nthat sounds good.\n\nDo the go through another /dev/video# node, or ... ? Are there any\ndocs / example code for this ?\n\nIt would be good to replace the custom ioctl used for the atomisp\nparams which contain the gains with a parameter buffer mechanism.\n\nNote that AFAICT the atomisp has multiple parameter buffer types\n(currently separate ioctls). Like e.g. separate buffers to pass parameters\nrelated to special optional features like digital image stabilization.\n\nRegards,\n\nHans","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 11241BE173\n\tfor ;\n\tWed, 6 Nov 2024 14:17:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42EEC65430;\n\tWed, 6 Nov 2024 15:17:30 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A7DA653C5\n\tfor ;\n\tWed, 6 Nov 2024 15:17:28 +0100 (CET)","from mail-lf1-f70.google.com (mail-lf1-f70.google.com\n\t[209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-436-_ZP73jjGMxamdquyVaO0kw-1; Wed, 06 Nov 2024 09:17:22 -0500","by mail-lf1-f70.google.com with SMTP id\n\t2adb3069b0e04-539ebb5a10cso6310061e87.3\n\tfor ;\n\tWed, 06 Nov 2024 06:17:22 -0800 (PST)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a9eb17f9806sm286615466b.157.2024.11.06.06.17.20\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 06 Nov 2024 06:17:20 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"LWWszzgH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1730902647;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=ctU5x8nHLtGlxODXmBYIZTUasHSnAJEteh2uNqncwpM=;\n\tb=LWWszzgHbIDI99tQ9luFpLIzu6+qeLTb/YS3QY/K7IQ8hUridFjmevL9dLktw7/iqwqTq4\n\tCDHL4x0LVd0AXJTQHLTL67Z4dNoua9TSQ6H+7DsdNCBSfJmc1VoCH66+75Rv22G6VDbte9\n\tgNfcOKgEwvu/L6HESj4+cYWZzZ3P1BQ=","X-MC-Unique":"_ZP73jjGMxamdquyVaO0kw-1","X-Mimecast-MFC-AGG-ID":"_ZP73jjGMxamdquyVaO0kw","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730902641; x=1731507441;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=ctU5x8nHLtGlxODXmBYIZTUasHSnAJEteh2uNqncwpM=;\n\tb=KhsZNBbbH8W6Isd9OPn1HJCWTG0KDzCWnAR9TXOtyfkA/PHI3PAK+B9GWB4mCwL4WJ\n\tfW+tOQDjFLQgpe+CzudkjIfb5PpyFkIhj1grziS2JLEkaO/3G57ePsctTJHNAdr4qGqY\n\tvjx9ACTEWOScBIxuGP35cCuOUwC92Bfp8eogFg63IJB9iJkgZ4lnfrVYtFePM0yYW3ZA\n\tfozMShom2Agb47crJq/EgmzR03Q2n0UVZA6OLlGlImKaGDNKFyVwN2qQOGPm1RySyp6C\n\tIA+pFY/Dc1bPC5NYJ2JJxijqzF3rWEepClhk04SHg/783OzCeMuLvjC507DsC4OgpPI3\n\tn8gQ==","X-Gm-Message-State":"AOJu0YyiOsTvZmeFCn8gIqqjGg4diV2MJoci4qgE7mu8mfL1q/b7oi8g\n\ttEtIkEEeahjrHQtNpBICaNf6w/A/K84wVNl+2TxJNwBuDOaYi0h23gihVxoWEwp2GKd3ZiG+ZvB\n\tABoiCkkQWKW+Ek7DsrlvstoNo0MN2e5P1AzWM0r7maxJuL2w/+GdZKW5LrFgDyTauQqKWiMo=","X-Received":["by 2002:a05:6512:3a8b:b0:539:89a8:600f with SMTP id\n\t2adb3069b0e04-53d65de5298mr15801547e87.23.1730902641240; \n\tWed, 06 Nov 2024 06:17:21 -0800 (PST)","by 2002:a05:6512:3a8b:b0:539:89a8:600f with SMTP id\n\t2adb3069b0e04-53d65de5298mr15801510e87.23.1730902640702; \n\tWed, 06 Nov 2024 06:17:20 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFCi5QGczcNQTxzMg90ph0k0KSuZufzzbK504vDqGbZ7K2NTKVhauP8azdiGN3TsHZEh3oAuw==","Message-ID":"","Date":"Wed, 6 Nov 2024 15:17:19 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 8/8] libcamera: Add new atomisp pipeline handler","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard , Sakari Ailus ","References":"<20241103152205.29219-1-hdegoede@redhat.com>\n\t<20241103152205.29219-9-hdegoede@redhat.com>\n\t<20241104235314.GI14276@pendragon.ideasonboard.com>\n\t\n\t<20241106134006.GH9369@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20241106134006.GH9369@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"LnTfVubmRf5oGCK2wsQU5edQZ2JInTAScgk46v-fXf8_1730902641","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]