[{"id":31316,"web_url":"https://patchwork.libcamera.org/comment/31316/","msgid":"","date":"2024-09-23T18:09:51","subject":"Re: [PATCH 1/1] libcamera: Add request API support for media\n\tcontroller device","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Hi,\n\nLe lundi 23 septembre 2024 à 06:02 +0000, Harvey Yang a écrit :\n> From: Han-Lin Chen \n> \n> This patch adds `allocateRequests` with MEDIA_IOC_REQUEST_ALLOC\n> , `queueRequest` with MEDIA_REQUEST_IOC_QUEUE, and `reInitRequest`\n> with MEDIA_REQUEST_IOC_REINIT.\n> \n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> ---\n> include/libcamera/internal/media_device.h | 4 ++\n> src/libcamera/media_device.cpp | 87 +++++++++++++++++++++++\n> 2 files changed, 91 insertions(+)\n> \n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index e412d3a0..8f54dd12 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -53,6 +53,10 @@ public:\n> \tMediaLink *link(const MediaPad *source, const MediaPad *sink);\n> \tint disableLinks();\n> \n> +\tint allocateRequests(unsigned int count, std::vector &requests);\n> +\tint queueRequest(int requestFd);\n> +\tint reInitRequest(int requestFd);\n> +\n> \tSignal<> disconnected;\n> \n> protected:\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index bd054552..47b75809 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -836,4 +837,90 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Allocate \\a count requests with ioctl\n> + * \\param[in] count The number of requests to be allocated\n> + * \\param[out] requests The request file descriptors to be returned\n> + *\n> + * This function returns request file descriptors if the MediaDevice supports\n> + * requests. The file descriptors can then be queued and re-inited afterwards.\n> + *\n> + * \\sa queueRequest(int requestFd)\n> + * \\sa reInitRequest(int requestFd)\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int MediaDevice::allocateRequests(unsigned int count, std::vector &requests)\n> +{\n> +\tfor (unsigned int i = 0; i < count; i++) {\n> +\t\tint fd;\n> +\t\tint ret = ioctl(fd_.get(), MEDIA_IOC_REQUEST_ALLOC, &fd);\n> +\t\tif (ret) {\n> +\t\t\tLOG(MediaDevice, Error) << \"Allocate request failed \"\n> +\t\t\t\t\t\t<< strerror(-ret);\n> +\t\t\treturn -EBUSY;\n> +\t\t}\n> +\t\trequests.emplace_back(fd);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n\nWouldn't if be nicer if the request was an object ?\n\n> +\n> +/**\n> + * \\brief Queue a request with ioctl\n> + * \\param[in] requestFd The request file descriptor\n> + *\n> + * This function queues a request that was allocated before.\n> + *\n> + * \\sa allocateRequests(unsigned int count, std::vector &requests)\n> + * \\sa reInitRequest(int requestFd)\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int MediaDevice::queueRequest(int requestFd)\n> +{\n> +\tint ret = ioctl(requestFd, MEDIA_REQUEST_IOC_QUEUE, NULL);\n> +\tif (ret)\n> +\t\tLOG(MediaDevice, Error) << \"QueueRequest fd \" << requestFd\n> +\t\t\t\t\t<< \"failed: \" << strerror(-ret);\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * \\brief Re-init a request with ioctl\n> + * \\param[in] requestFd The request file descriptor\n> + *\n> + * This function recycles a request that was allocated and queued before. It\n> + * polls on \\a requestFd to ensure the request is completed, and reinits the\n> + * request.\n> + *\n> + * \\sa allocateRequests(unsigned int count, std::vector &requests)\n> + * \\sa queueRequest(int requestFd)\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int MediaDevice::reInitRequest(int requestFd)\n> +{\n> +\tstruct pollfd pfd;\n> +\n> +\tpfd.fd = requestFd;\n> +\tpfd.events = POLLPRI;\n> +\n> +\tint ret = TEMP_FAILURE_RETRY(poll(&pfd, 1, 300));\n\nShouldn't we split polling and re-init ? Polling is a very important feature of\nRequests, since it can be used as a synchronization point, avoiding numerous\nindependent queue polling. Its also nicer if callers can control (or avoid) the\ntimeout.\n\nNicolas\n\n> +\tif (ret < 0)\n> +\t\tLOG(MediaDevice, Error) << \"The request \" << requestFd\n> +\t\t\t\t\t<< \" polled failed: \" << strerror(-ret);\n> +\telse if (ret == 0)\n> +\t\tLOG(MediaDevice, Error) << \"The request \" << requestFd\n> +\t\t\t\t\t<< \" polled timeout: \" << strerror(-ret);\n> +\n> +\tret = ::ioctl(requestFd, MEDIA_REQUEST_IOC_REINIT, NULL);\n> +\tif (ret)\n> +\t\tLOG(MediaDevice, Error) << \"The request \" << requestFd\n> +\t\t\t\t\t<< \" is queued but not yet completed: \"\n> +\t\t\t\t\t<< strerror(-ret);\n> +\n> +\treturn ret;\n> +}\n> +\n> } /* namespace libcamera */","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 251ACC3257\n\tfor ;\n\tMon, 23 Sep 2024 18:09:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2183D6350B;\n\tMon, 23 Sep 2024 20:09:56 +0200 (CEST)","from mail-oo1-xc31.google.com (mail-oo1-xc31.google.com\n\t[IPv6:2607:f8b0:4864:20::c31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D2236037E\n\tfor ;\n\tMon, 23 Sep 2024 20:09:54 +0200 (CEST)","by mail-oo1-xc31.google.com with SMTP id\n\t006d021491bc7-5d5c7f24372so2728559eaf.0\n\tfor ;\n\tMon, 23 Sep 2024 11:09:54 -0700 (PDT)","from nicolas-tpx395.lan ([2606:6d00:15:862e::580])\n\tby smtp.gmail.com with ESMTPSA id\n\t6a1803df08f44-6c75e586bd6sm49722966d6.135.2024.09.23.11.09.52\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 23 Sep 2024 11:09:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20230601.gappssmtp.com\n\theader.i=@ndufresne-ca.20230601.gappssmtp.com\n\theader.b=\"kx5fgpTu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20230601.gappssmtp.com; s=20230601; t=1727114993;\n\tx=1727719793; darn=lists.libcamera.org; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=FdaJnsOAvBnOXxD28lMmiT6wReQZJUDGFd22oSvfOmg=;\n\tb=kx5fgpTuR0dCG/otINBYuJCAfUNQNFRy8IUqFalyHyS6E/Oo4DWhqk6VK017jYoYGk\n\tEicSE2swGNs16+3vHmAOwcRCN1alvyskgPWzghoQLBYh2TtgihaVy/ddVR+6ic3jGRtT\n\tUFHt9LktJTL0XHu/oXJNK/zkje4Vh/UMukPf3qQ4NVtzfOCWppG6SrQlf+kKGek0LAqC\n\tbkouX4V+nOjS8Q8TR9WUXUmypjteIY9Xw5jywJ0JAgOCBtXYsuW5emjw5ISoj/t16x1j\n\tUmwzOLN6l0aUUYs/mp4ql0pbr1npLluVhmwVC9Yqrha6wblAZuzupLV6uCcdzi0W4jCf\n\tOmRw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727114993; x=1727719793;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=FdaJnsOAvBnOXxD28lMmiT6wReQZJUDGFd22oSvfOmg=;\n\tb=hRCFRr6HgZnXXxYVONgZ7xdOHsw+hgWbgOw1qP2VowXdixs19wUNRmlNQZfQVDaxUg\n\tImD3d8hhooCGV5bEwlMv+lTaE6lP9gTtCMNnp9kzG7HLp5Ac5Ria01p8m4XrVQiywJrH\n\tw6RWpuBi8k4PLZivsLCAsxhLi7othGn4jTgSLKzYIb3kmE7fhj7i1Jo1hU4mtBwnZxMl\n\tbQUuL87lqIpBrnzkAo68agpbHL5xOTX/80iAxEiaZCm9hfJbE8ZEtXyOXjPn4Ac5q2Xj\n\tR5E2hgHyNMTbUJWPDdiT1D8ihykyjEgISQPLNRowImz/qElwG3j3lSEm0cyasYkswuwg\n\tDAeA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUFgrOMJQ0cj5j2nXZsRa+MIboud2jr772aCzccFEbMj6xIrPndCDkaexSYcLH3ekpRr7GpqsY2uxPffdzLzZI=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YyeOZu1OwuBTrqwVhh/8FDqEgdcpxlrDDKqRNITwvzlDiyqLTGg\n\tXZJX7tYLXh9rEB5sNf2gwnXx9diI4Npns4E7vqoBwqQmvVOJhqu+w+wxr8dbESg=","X-Google-Smtp-Source":"AGHT+IE599m9bZtuJVbdpPT0K5ifCqpnEDPNKn0rUjzTU5H+1ofUgmVD02y9ayc2gubEnguLGROqWQ==","X-Received":"by 2002:a05:6359:4583:b0:1aa:b9f2:a0d2 with SMTP id\n\te5c5f4694b2df-1bc99d18c91mr343733955d.10.1727114992710; \n\tMon, 23 Sep 2024 11:09:52 -0700 (PDT)","Message-ID":"","Subject":"Re: [PATCH 1/1] libcamera: Add request API support for media\n\tcontroller device","From":"Nicolas Dufresne ","To":"Harvey Yang , \n\tlibcamera-devel@lists.libcamera.org","Cc":"Han-Lin Chen ","Date":"Mon, 23 Sep 2024 14:09:51 -0400","In-Reply-To":"<20240923060551.1849065-2-chenghaoyang@google.com>","References":"<20240923060551.1849065-1-chenghaoyang@google.com>\n\t<20240923060551.1849065-2-chenghaoyang@google.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.52.4 (3.52.4-1.fc40) ","MIME-Version":"1.0","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":31342,"web_url":"https://patchwork.libcamera.org/comment/31342/","msgid":"<20240924194305.GG7165@pendragon.ideasonboard.com>","date":"2024-09-24T19:43:05","subject":"Re: [PATCH 1/1] libcamera: Add request API support for media\n\tcontroller device","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Sep 23, 2024 at 02:09:51PM -0400, Nicolas Dufresne wrote:\n> Le lundi 23 septembre 2024 à 06:02 +0000, Harvey Yang a écrit :\n> > From: Han-Lin Chen \n> > \n> > This patch adds `allocateRequests` with MEDIA_IOC_REQUEST_ALLOC\n> > , `queueRequest` with MEDIA_REQUEST_IOC_QUEUE, and `reInitRequest`\n> > with MEDIA_REQUEST_IOC_REINIT.\n> > \n> > Signed-off-by: Han-Lin Chen \n> > Co-developed-by: Harvey Yang \n> > ---\n> > include/libcamera/internal/media_device.h | 4 ++\n> > src/libcamera/media_device.cpp | 87 +++++++++++++++++++++++\n> > 2 files changed, 91 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> > index e412d3a0..8f54dd12 100644\n> > --- a/include/libcamera/internal/media_device.h\n> > +++ b/include/libcamera/internal/media_device.h\n> > @@ -53,6 +53,10 @@ public:\n> > \tMediaLink *link(const MediaPad *source, const MediaPad *sink);\n> > \tint disableLinks();\n> > \n> > +\tint allocateRequests(unsigned int count, std::vector &requests);\n> > +\tint queueRequest(int requestFd);\n> > +\tint reInitRequest(int requestFd);\n> > +\n> > \tSignal<> disconnected;\n> > \n> > protected:\n> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> > index bd054552..47b75809 100644\n> > --- a/src/libcamera/media_device.cpp\n> > +++ b/src/libcamera/media_device.cpp\n> > @@ -9,6 +9,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -836,4 +837,90 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)\n> > \treturn 0;\n> > }\n> > \n> > +/**\n> > + * \\brief Allocate \\a count requests with ioctl\n> > + * \\param[in] count The number of requests to be allocated\n> > + * \\param[out] requests The request file descriptors to be returned\n> > + *\n> > + * This function returns request file descriptors if the MediaDevice supports\n> > + * requests. The file descriptors can then be queued and re-inited afterwards.\n> > + *\n> > + * \\sa queueRequest(int requestFd)\n> > + * \\sa reInitRequest(int requestFd)\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int MediaDevice::allocateRequests(unsigned int count, std::vector &requests)\n\nI think I would limit this function to allocating a single request, and\nhandle the loop in the caller. This will give us a nicer function\nsignature, and will allow the caller to decide what to do if only a\nsubset of the requests can be allocated.\n\n> > +{\n> > +\tfor (unsigned int i = 0; i < count; i++) {\n> > +\t\tint fd;\n> > +\t\tint ret = ioctl(fd_.get(), MEDIA_IOC_REQUEST_ALLOC, &fd);\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(MediaDevice, Error) << \"Allocate request failed \"\n> > +\t\t\t\t\t\t<< strerror(-ret);\n> > +\t\t\treturn -EBUSY;\n> > +\t\t}\n> > +\t\trequests.emplace_back(fd);\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> \n> Wouldn't if be nicer if the request was an object ?\n\nI agree, I think that would be nicer. The reInitRequest() function would\nthen become a member function of the MediaRequest class, and would be\ncalled reinit().\n\n> > +\n> > +/**\n> > + * \\brief Queue a request with ioctl\n\n\"with ioctl\" is an implementation detail. You can write \"Queue a request\nto the media device\".\n\n> > + * \\param[in] requestFd The request file descriptor\n\nThis function should take a MediaRequest reference.\n\n> > + *\n> > + * This function queues a request that was allocated before.\n\nIf you pass a request reference, the \"that was allocated before\" becomes\nimplicit, you can drop it.\n\n> > + *\n> > + * \\sa allocateRequests(unsigned int count, std::vector &requests)\n> > + * \\sa reInitRequest(int requestFd)\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int MediaDevice::queueRequest(int requestFd)\n> > +{\n> > +\tint ret = ioctl(requestFd, MEDIA_REQUEST_IOC_QUEUE, NULL);\n> > +\tif (ret)\n> > +\t\tLOG(MediaDevice, Error) << \"QueueRequest fd \" << requestFd\n> > +\t\t\t\t\t<< \"failed: \" << strerror(-ret);\n> > +\treturn ret;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Re-init a request with ioctl\n> > + * \\param[in] requestFd The request file descriptor\n> > + *\n> > + * This function recycles a request that was allocated and queued before. It\n> > + * polls on \\a requestFd to ensure the request is completed, and reinits the\n> > + * request.\n> > + *\n> > + * \\sa allocateRequests(unsigned int count, std::vector &requests)\n> > + * \\sa queueRequest(int requestFd)\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int MediaDevice::reInitRequest(int requestFd)\n> > +{\n> > +\tstruct pollfd pfd;\n> > +\n> > +\tpfd.fd = requestFd;\n> > +\tpfd.events = POLLPRI;\n> > +\n> > +\tint ret = TEMP_FAILURE_RETRY(poll(&pfd, 1, 300));\n\nI didn't know about TEMP_FAILURE_RETRY. Shouldn't we wrap fds in a class\nthat provides posix function wrappers with EINTR handling, instead of\nspringkling it manually through the code ?\n\n> \n> Shouldn't we split polling and re-init ? Polling is a very important feature of\n> Requests, since it can be used as a synchronization point, avoiding numerous\n> independent queue polling. Its also nicer if callers can control (or avoid) the\n> timeout.\n\nAgreed, and polling with a timeout is a bad idea. You'll block the\ncamera thread for up to 300ms, that's way too long. Don't block,\nimplement the behaviour you want in an event-driven way.\n\n> > +\tif (ret < 0)\n> > +\t\tLOG(MediaDevice, Error) << \"The request \" << requestFd\n> > +\t\t\t\t\t<< \" polled failed: \" << strerror(-ret);\n> > +\telse if (ret == 0)\n> > +\t\tLOG(MediaDevice, Error) << \"The request \" << requestFd\n> > +\t\t\t\t\t<< \" polled timeout: \" << strerror(-ret);\n> > +\n> > +\tret = ::ioctl(requestFd, MEDIA_REQUEST_IOC_REINIT, NULL);\n> > +\tif (ret)\n> > +\t\tLOG(MediaDevice, Error) << \"The request \" << requestFd\n> > +\t\t\t\t\t<< \" is queued but not yet completed: \"\n\nIs this the right message ?\n\n> > +\t\t\t\t\t<< strerror(-ret);\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > } /* namespace libcamera */\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CB0EEC0F1B\n\tfor ;\n\tTue, 24 Sep 2024 19:43:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BACC163502;\n\tTue, 24 Sep 2024 21:43:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07A6F63500\n\tfor ;\n\tTue, 24 Sep 2024 21:43:38 +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 ESMTPSA id A478A827;\n\tTue, 24 Sep 2024 21:42:10 +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=\"wFiylwr+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727206930;\n\tbh=rMyDjnsgHMjvUSZWY/KUd6axFLo/Wl7bWIAxBbnpzWc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wFiylwr+PRX7LmnV/XEiBsqUs2dmyNBwIpiKeGGd8sKavJpVjeHKCXYac2L6Gbpi+\n\t8PuVuWIkePiZjKV6N4KcFLK1+7gtm6t6C8/xy4na7GFeAVZy9c6H6ZDCbs280APdI+\n\tFfqnOpjKxhJPxWqJTWjf5TLX/QTYEfreVMGoqjEY=","Date":"Tue, 24 Sep 2024 22:43:05 +0300","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH 1/1] libcamera: Add request API support for media\n\tcontroller device","Message-ID":"<20240924194305.GG7165@pendragon.ideasonboard.com>","References":"<20240923060551.1849065-1-chenghaoyang@google.com>\n\t<20240923060551.1849065-2-chenghaoyang@google.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","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":32060,"web_url":"https://patchwork.libcamera.org/comment/32060/","msgid":"","date":"2024-11-06T17:37:18","subject":"Re: [PATCH 1/1] libcamera: Add request API support for media\n\tcontroller device","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Nicolas and Laurent,\n\nOn Wed, Sep 25, 2024 at 3:43 AM Laurent Pinchart\n wrote:\n>\n> Hello,\n>\n> On Mon, Sep 23, 2024 at 02:09:51PM -0400, Nicolas Dufresne wrote:\n> > Le lundi 23 septembre 2024 à 06:02 +0000, Harvey Yang a écrit :\n> > > From: Han-Lin Chen \n> > >\n> > > This patch adds `allocateRequests` with MEDIA_IOC_REQUEST_ALLOC\n> > > , `queueRequest` with MEDIA_REQUEST_IOC_QUEUE, and `reInitRequest`\n> > > with MEDIA_REQUEST_IOC_REINIT.\n> > >\n> > > Signed-off-by: Han-Lin Chen \n> > > Co-developed-by: Harvey Yang \n> > > ---\n> > > include/libcamera/internal/media_device.h | 4 ++\n> > > src/libcamera/media_device.cpp | 87 +++++++++++++++++++++++\n> > > 2 files changed, 91 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> > > index e412d3a0..8f54dd12 100644\n> > > --- a/include/libcamera/internal/media_device.h\n> > > +++ b/include/libcamera/internal/media_device.h\n> > > @@ -53,6 +53,10 @@ public:\n> > > MediaLink *link(const MediaPad *source, const MediaPad *sink);\n> > > int disableLinks();\n> > >\n> > > + int allocateRequests(unsigned int count, std::vector &requests);\n> > > + int queueRequest(int requestFd);\n> > > + int reInitRequest(int requestFd);\n> > > +\n> > > Signal<> disconnected;\n> > >\n> > > protected:\n> > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> > > index bd054552..47b75809 100644\n> > > --- a/src/libcamera/media_device.cpp\n> > > +++ b/src/libcamera/media_device.cpp\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -836,4 +837,90 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)\n> > > return 0;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Allocate \\a count requests with ioctl\n> > > + * \\param[in] count The number of requests to be allocated\n> > > + * \\param[out] requests The request file descriptors to be returned\n> > > + *\n> > > + * This function returns request file descriptors if the MediaDevice supports\n> > > + * requests. The file descriptors can then be queued and re-inited afterwards.\n> > > + *\n> > > + * \\sa queueRequest(int requestFd)\n> > > + * \\sa reInitRequest(int requestFd)\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int MediaDevice::allocateRequests(unsigned int count, std::vector &requests)\n>\n> I think I would limit this function to allocating a single request, and\n> handle the loop in the caller. This will give us a nicer function\n> signature, and will allow the caller to decide what to do if only a\n> subset of the requests can be allocated.\n\nDone\n\n>\n> > > +{\n> > > + for (unsigned int i = 0; i < count; i++) {\n> > > + int fd;\n> > > + int ret = ioctl(fd_.get(), MEDIA_IOC_REQUEST_ALLOC, &fd);\n> > > + if (ret) {\n> > > + LOG(MediaDevice, Error) << \"Allocate request failed \"\n> > > + << strerror(-ret);\n> > > + return -EBUSY;\n> > > + }\n> > > + requests.emplace_back(fd);\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> >\n> > Wouldn't if be nicer if the request was an object ?\n>\n> I agree, I think that would be nicer. The reInitRequest() function would\n> then become a member function of the MediaRequest class, and would be\n> called reinit().\n\nAdded `MediaDevice::Request`. Please take a look.\n\n>\n> > > +\n> > > +/**\n> > > + * \\brief Queue a request with ioctl\n>\n> \"with ioctl\" is an implementation detail. You can write \"Queue a request\n> to the media device\".\n\nDone\n\n>\n> > > + * \\param[in] requestFd The request file descriptor\n>\n> This function should take a MediaRequest reference.\n\nI make it a MediaDevice::Request's member function, so that\nit doesn't take an input.\n\n>\n> > > + *\n> > > + * This function queues a request that was allocated before.\n>\n> If you pass a request reference, the \"that was allocated before\" becomes\n> implicit, you can drop it.\n\nDone\n\n>\n> > > + *\n> > > + * \\sa allocateRequests(unsigned int count, std::vector &requests)\n> > > + * \\sa reInitRequest(int requestFd)\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int MediaDevice::queueRequest(int requestFd)\n> > > +{\n> > > + int ret = ioctl(requestFd, MEDIA_REQUEST_IOC_QUEUE, NULL);\n> > > + if (ret)\n> > > + LOG(MediaDevice, Error) << \"QueueRequest fd \" << requestFd\n> > > + << \"failed: \" << strerror(-ret);\n> > > + return ret;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Re-init a request with ioctl\n> > > + * \\param[in] requestFd The request file descriptor\n> > > + *\n> > > + * This function recycles a request that was allocated and queued before. It\n> > > + * polls on \\a requestFd to ensure the request is completed, and reinits the\n> > > + * request.\n> > > + *\n> > > + * \\sa allocateRequests(unsigned int count, std::vector &requests)\n> > > + * \\sa queueRequest(int requestFd)\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int MediaDevice::reInitRequest(int requestFd)\n> > > +{\n> > > + struct pollfd pfd;\n> > > +\n> > > + pfd.fd = requestFd;\n> > > + pfd.events = POLLPRI;\n> > > +\n> > > + int ret = TEMP_FAILURE_RETRY(poll(&pfd, 1, 300));\n>\n> I didn't know about TEMP_FAILURE_RETRY. Shouldn't we wrap fds in a class\n> that provides posix function wrappers with EINTR handling, instead of\n> springkling it manually through the code ?\n\nRemoved\n\n>\n> >\n> > Shouldn't we split polling and re-init ? Polling is a very important feature of\n> > Requests, since it can be used as a synchronization point, avoiding numerous\n> > independent queue polling. Its also nicer if callers can control (or avoid) the\n> > timeout.\n>\n> Agreed, and polling with a timeout is a bad idea. You'll block the\n> camera thread for up to 300ms, that's way too long. Don't block,\n> implement the behaviour you want in an event-driven way.\n\nUsed EventNotifier instead.\n\n>\n> > > + if (ret < 0)\n> > > + LOG(MediaDevice, Error) << \"The request \" << requestFd\n> > > + << \" polled failed: \" << strerror(-ret);\n> > > + else if (ret == 0)\n> > > + LOG(MediaDevice, Error) << \"The request \" << requestFd\n> > > + << \" polled timeout: \" << strerror(-ret);\n> > > +\n> > > + ret = ::ioctl(requestFd, MEDIA_REQUEST_IOC_REINIT, NULL);\n> > > + if (ret)\n> > > + LOG(MediaDevice, Error) << \"The request \" << requestFd\n> > > + << \" is queued but not yet completed: \"\n>\n> Is this the right message ?\n\nI think so:\nhttps://www.kernel.org/doc/html/v5.8/userspace-api/media/mediactl/media-request-ioc-reinit.html\nIt states the error EBUSY to be the case.\nWDYT?\n\n\n>\n> > > + << strerror(-ret);\n> > > +\n> > > + return ret;\n> > > +}\n> > > +\n> > > } /* namespace libcamera */\n> >\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 904DFBDB13\n\tfor ;\n\tWed, 6 Nov 2024 17:37:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 48CF765435;\n\tWed, 6 Nov 2024 18:37:32 +0100 (CET)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 764B165434\n\tfor ;\n\tWed, 6 Nov 2024 18:37:30 +0100 (CET)","by mail-lj1-x22c.google.com with SMTP id\n\t38308e7fff4ca-2fb3da341c9so328541fa.2\n\tfor ;\n\tWed, 06 Nov 2024 09:37:30 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"EvTdk6/u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1730914650; x=1731519450;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=4c6ip0bO/r4vihFEfTuwIsedz3QYL7wd54KEjAvaXC4=;\n\tb=EvTdk6/uJhCPBk7pksB1nlfr5XkbCaMHzbD1Q7cTVEajSbd+jJxAeNBICNmNdq4NLo\n\tYasw3eglr43XK4+KRobQ/0apq/xefV6ao0Uk+X+4c7AH7KBjk/CjRacJW3vmJZ0XocVR\n\tXB491ZRir+evqI2kfqThOBzdMweXEQMbr69tg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730914650; x=1731519450;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=4c6ip0bO/r4vihFEfTuwIsedz3QYL7wd54KEjAvaXC4=;\n\tb=U2YFfPdgnX4w0MB0QjzbNDcDZ+1LY3oh+DigpdhQwprGutkkCqsDrBz9DiLWnKf5gD\n\tpLYhCZDzAtzzm9ZB27zBN0PXOAFa0iDV0RaVWU83Fc/N6OsEsNlxAga5AUK74z0eTyXH\n\tbNxQeCSuQU0Xf16F2rvLI3P53QfNXxru2OWid4zZINATBrou9dcnR2wmjctMfZKjJUE5\n\trQHM6i/o9fOD8QK1GR2esjxpZh2GjsiWPpFTH6EMYvKm7jloGGFNk4wGAfFRQGBmjnu4\n\tjesdR4IDnjat2w7fwBPrM5ZH6ARhou49k0XWHBz99Bxy6y7RGSb6NWyMJ1klxvKHWghS\n\tXubw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXG3IJMviXUxn7fYqSAgh9jxGqVjI7pTdX0k8RrvWqodkbB+le7+AO1tvKp7+u8lJNGYa3i+fTVJuXj6EJuLns=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yw40hVWyogcDeU8yotPBcm1T2Lhbi/5Aq9g2BYukFFOVvKudC8n\n\tnh3FA884fr4QBJ0etZAqABGkPFtOGOG4VtZrFBXupjj2I3/3QxwCnZQDOjQ0bvbuqZ1s/XS0f/I\n\tKB2mwAKV3m2UP4BeP/iwz8isyIt6it0UOoqG/","X-Google-Smtp-Source":"AGHT+IFUoDjR5bSdE+S+L6sFyGLpJq7OH8I59awd7NDbCS+Yo/uh6KTvf1S+B3u4lG1iZVoNrWnNoPT8a8z2L6khraU=","X-Received":"by 2002:a2e:a543:0:b0:2fa:cac0:2a14 with SMTP id\n\t38308e7fff4ca-2fedb7723edmr108034251fa.11.1730914649571;\n\tWed, 06 Nov 2024 09:37:29 -0800 (PST)","MIME-Version":"1.0","References":"<20240923060551.1849065-1-chenghaoyang@google.com>\n\t<20240923060551.1849065-2-chenghaoyang@google.com>\n\t\n\t<20240924194305.GG7165@pendragon.ideasonboard.com>","In-Reply-To":"<20240924194305.GG7165@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Thu, 7 Nov 2024 01:37:18 +0800","Message-ID":"","Subject":"Re: [PATCH 1/1] libcamera: Add request API support for media\n\tcontroller device","To":"Laurent Pinchart ","Cc":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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\" "}}]