[{"id":34807,"web_url":"https://patchwork.libcamera.org/comment/34807/","msgid":"<20250707130449.GB14963@pendragon.ideasonboard.com>","date":"2025-07-07T13:04:49","subject":"Re: [PATCH v3 6/9] pipeline: rkisp1: Add error log when parameter\n\tqueuing fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jul 07, 2025 at 10:55:09AM +0200, Stefan Klug wrote:\n> Add a error statement when quieing of the parameter buffer fails for\n\ns/quieing/\n\n> whatever reason.\n\nCommit messages need to explain first and foremost *why* a change is\nneeded.\n\n> Signed-off-by: Stefan Klug \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Paul Elder \n> \n> ---\n> \n> Changes in v3:\n> - Collected tags\n> - Removed hint regarding unsupported parameter types as this will be\n> handled using the now upstreamed RKISP1_CID_SUPPORTED_PARAMS_BLOCKS.\n> \n> Changes in v2:\n> - Also print the error code in case of failure\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 675f0a7490a6..d5bb8aced3c8 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -411,7 +411,14 @@ void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused\n> \t\treturn;\n> \n> \tinfo->paramBuffer->_d()->metadata().planes()[0].bytesused = bytesused;\n> -\tpipe->param_->queueBuffer(info->paramBuffer);\n> +\n> +\tint ret = pipe->param_->queueBuffer(info->paramBuffer);\n> +\tif (ret < 0) {\n> +\t\tLOG(RkISP1, Error) << \"Failed to queue parameter buffer: \"\n> +\t\t\t\t << strerror(-ret);\n> +\t\treturn;\n\nThis seems very fatal, if we can't queue the params buffer, everything\nwill fall apart. I think we should in that case stop the camera and\nreturn all requests as cancelled.\n\n> +\t}\n> +\n> \tpipe->stat_->queueBuffer(info->statBuffer);\n\nIn particular, I wonder why the check is needed for the param_ device\nbut not the stat_ device.\n\n> \n> \tif (info->mainPathBuffer)","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 B68D2C3237\n\tfor ;\n\tMon, 7 Jul 2025 13:05:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72B3668EB9;\n\tMon, 7 Jul 2025 15:05:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD7176151E\n\tfor ;\n\tMon, 7 Jul 2025 15:05:17 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 6A9C0664;\n\tMon, 7 Jul 2025 15:04:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rlHj1b1T\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751893490;\n\tbh=q7DEQzm1+WiAp1mY/euHIbr8GXz6x7xX5mnzdgwUF6U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rlHj1b1THxwheH3eeRp9eetDVU+aTWsTtcXqvbBLN828UpXOdddFxzy/ysZeYddv0\n\tf1eLOnDZLOoZaonpl3u2D3oi9qXHbQugzusmuPgsOkO2pWgUQm29I63HPoZskOtVF1\n\tGVcau+B5LshpBbX6hsRGOpgvI/VxoJIHue0MoIAM=","Date":"Mon, 7 Jul 2025 16:04:49 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tPaul Elder ","Subject":"Re: [PATCH v3 6/9] pipeline: rkisp1: Add error log when parameter\n\tqueuing fails","Message-ID":"<20250707130449.GB14963@pendragon.ideasonboard.com>","References":"<20250707085520.39777-1-stefan.klug@ideasonboard.com>\n\t<20250707085520.39777-7-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250707085520.39777-7-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34810,"web_url":"https://patchwork.libcamera.org/comment/34810/","msgid":"<175189612017.40598.7034541331228445062@localhost>","date":"2025-07-07T13:48:40","subject":"Re: [PATCH v3 6/9] pipeline: rkisp1: Add error log when parameter\n\tqueuing fails","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThanks for the comments.\n\nQuoting Laurent Pinchart (2025-07-07 15:04:49)\n> On Mon, Jul 07, 2025 at 10:55:09AM +0200, Stefan Klug wrote:\n> > Add a error statement when quieing of the parameter buffer fails for\n> \n> s/quieing/\n> \n> > whatever reason.\n> \n> Commit messages need to explain first and foremost *why* a change is\n> needed.\n\nDo we really need a reason to log an error when something fails? If we\nonly look at upstream and proper setups, this error condition will\npossibly never occur again. I can't tell. But I wish I would have had\nthis error message when I hit the error in first place. My take on that\nis that every call to the outside should be error checked and if\nsomething unexpected happens we should log it. That maybe a bit on the\ndefensive side but in this case I was actually bitten.\n\n> \n> > Signed-off-by: Stefan Klug \n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Paul Elder \n> > \n> > ---\n> > \n> > Changes in v3:\n> > - Collected tags\n> > - Removed hint regarding unsupported parameter types as this will be\n> > handled using the now upstreamed RKISP1_CID_SUPPORTED_PARAMS_BLOCKS.\n> > \n> > Changes in v2:\n> > - Also print the error code in case of failure\n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-\n> > 1 file changed, 8 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 675f0a7490a6..d5bb8aced3c8 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -411,7 +411,14 @@ void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused\n> > return;\n> > \n> > info->paramBuffer->_d()->metadata().planes()[0].bytesused = bytesused;\n> > - pipe->param_->queueBuffer(info->paramBuffer);\n> > +\n> > + int ret = pipe->param_->queueBuffer(info->paramBuffer);\n> > + if (ret < 0) {\n> > + LOG(RkISP1, Error) << \"Failed to queue parameter buffer: \"\n> > + << strerror(-ret);\n> > + return;\n> \n> This seems very fatal, if we can't queue the params buffer, everything\n> will fall apart. I think we should in that case stop the camera and\n> return all requests as cancelled.\n\nIs there an easy way to do this? We could also replace Error by Fatal\nand exit out as this is really an unexpected condition.\n\n> \n> > + }\n> > +\n> > pipe->stat_->queueBuffer(info->statBuffer);\n> \n> In particular, I wonder why the check is needed for the param_ device\n> but not the stat_ device.\n\nIt should actually be added. It just didn't go wrong in the work I did\nand therefore wasn't added.\n\nBest regards,\nStefan\n\n> \n> > \n> > if (info->mainPathBuffer)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7B2B5C0DA4\n\tfor ;\n\tMon, 7 Jul 2025 13:48:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3F3B568EB6;\n\tMon, 7 Jul 2025 15:48:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B8096151E\n\tfor ;\n\tMon, 7 Jul 2025 15:48:44 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c79f:85df:e7f5:4c31])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 3D22D89A;\n\tMon, 7 Jul 2025 15:48:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hYCwcekS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751896097;\n\tbh=UCca9LX7OCAI163ZRzv1caFh0PWCUnd1XwDp0Sg1tP8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=hYCwcekSWTAUe5j89V7zPdi0TY8wLGX8kZJs+stJuZFdc6fxgstWjj/Whnx7xk/yk\n\tUlFjA5T1CV6qUcdAkdxqmGf/IeQFl/j52Ril34kWTtH3xmBzroVjkb4vKiMmpOu/7r\n\tZazTo95wMaMd9p8oNDWx8WaBd4Z5SJUQE+Z0VsSw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250707130449.GB14963@pendragon.ideasonboard.com>","References":"<20250707085520.39777-1-stefan.klug@ideasonboard.com>\n\t<20250707085520.39777-7-stefan.klug@ideasonboard.com>\n\t<20250707130449.GB14963@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v3 6/9] pipeline: rkisp1: Add error log when parameter\n\tqueuing fails","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tPaul Elder ","To":"Laurent Pinchart ","Date":"Mon, 07 Jul 2025 15:48:40 +0200","Message-ID":"<175189612017.40598.7034541331228445062@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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":35044,"web_url":"https://patchwork.libcamera.org/comment/35044/","msgid":"<20250723155002.GA14576@pendragon.ideasonboard.com>","date":"2025-07-23T15:50:02","subject":"Re: [PATCH v3 6/9] pipeline: rkisp1: Add error log when parameter\n\tqueuing fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jul 07, 2025 at 03:48:40PM +0200, Stefan Klug wrote:\n> Quoting Laurent Pinchart (2025-07-07 15:04:49)\n> > On Mon, Jul 07, 2025 at 10:55:09AM +0200, Stefan Klug wrote:\n> > > Add a error statement when quieing of the parameter buffer fails for\n> > \n> > s/quieing/\n> > \n> > > whatever reason.\n> > \n> > Commit messages need to explain first and foremost *why* a change is\n> > needed.\n> \n> Do we really need a reason to log an error when something fails? If we\n> only look at upstream and proper setups, this error condition will\n> possibly never occur again. I can't tell. But I wish I would have had\n> this error message when I hit the error in first place. My take on that\n> is that every call to the outside should be error checked and if\n> something unexpected happens we should log it. That maybe a bit on the\n> defensive side but in this case I was actually bitten.\n\nI'm not pushing back against this patch, the comment was about the fact\ncommit messages need to explain the rationale for a change. What may be\nevident to you could not be so for someone else.\n\n> > > Signed-off-by: Stefan Klug \n> > > Reviewed-by: Kieran Bingham \n> > > Reviewed-by: Paul Elder \n> > > \n> > > ---\n> > > \n> > > Changes in v3:\n> > > - Collected tags\n> > > - Removed hint regarding unsupported parameter types as this will be\n> > > handled using the now upstreamed RKISP1_CID_SUPPORTED_PARAMS_BLOCKS.\n> > > \n> > > Changes in v2:\n> > > - Also print the error code in case of failure\n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-\n> > > 1 file changed, 8 insertions(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 675f0a7490a6..d5bb8aced3c8 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -411,7 +411,14 @@ void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused\n> > > return;\n> > > \n> > > info->paramBuffer->_d()->metadata().planes()[0].bytesused = bytesused;\n> > > - pipe->param_->queueBuffer(info->paramBuffer);\n> > > +\n> > > + int ret = pipe->param_->queueBuffer(info->paramBuffer);\n> > > + if (ret < 0) {\n> > > + LOG(RkISP1, Error) << \"Failed to queue parameter buffer: \"\n> > > + << strerror(-ret);\n> > > + return;\n> > \n> > This seems very fatal, if we can't queue the params buffer, everything\n> > will fall apart. I think we should in that case stop the camera and\n> > return all requests as cancelled.\n> \n> Is there an easy way to do this? We could also replace Error by Fatal\n> and exit out as this is really an unexpected condition.\n\nIt should be done on top, I don't know how easy or difficult it would\nbe. If it's difficult, we need helpers to make it easy :-)\n\n> > > + }\n> > > +\n> > > pipe->stat_->queueBuffer(info->statBuffer);\n> > \n> > In particular, I wonder why the check is needed for the param_ device\n> > but not the stat_ device.\n> \n> It should actually be added. It just didn't go wrong in the work I did\n> and therefore wasn't added.\n> \n> > > \n> > > if (info->mainPathBuffer)","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 651C2BDCC1\n\tfor ;\n\tWed, 23 Jul 2025 15:50:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F2A7369056;\n\tWed, 23 Jul 2025 17:50:06 +0200 (CEST)","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 13F7E614ED\n\tfor ;\n\tWed, 23 Jul 2025 17:50:05 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 447FFE92;\n\tWed, 23 Jul 2025 17:49:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DGGAf4ja\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753285766;\n\tbh=qp3zEMs/fi7+7LHnu4zTsbGUAR9mk+cSx3UcXM6PgmE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DGGAf4jaiBGpfWx7QTvNwL9KaAojplUejd/SFSKBt80kUeWp2CtrfyOd0pBu55NC4\n\tfNakNAQVDd1lSmXV+jgs4rMQs2yMZlpXLgtelZP5KSq2ztb2dMwe9sAFN6SwGM2taa\n\tABDSmpbWa2P9ssgo6TzERH4GEkTlnQbJcgMieXmQ=","Date":"Wed, 23 Jul 2025 18:50:02 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tPaul Elder ","Subject":"Re: [PATCH v3 6/9] pipeline: rkisp1: Add error log when parameter\n\tqueuing fails","Message-ID":"<20250723155002.GA14576@pendragon.ideasonboard.com>","References":"<20250707085520.39777-1-stefan.klug@ideasonboard.com>\n\t<20250707085520.39777-7-stefan.klug@ideasonboard.com>\n\t<20250707130449.GB14963@pendragon.ideasonboard.com>\n\t<175189612017.40598.7034541331228445062@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<175189612017.40598.7034541331228445062@localhost>","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\" "}}]