[{"id":30883,"web_url":"https://patchwork.libcamera.org/comment/30883/","msgid":"<CAEB1ahsg4D771B0QjWnjkxJr5KFPHZvVZo8ivJJfFhaevVyaQw@mail.gmail.com>","date":"2024-08-21T16:55:45","subject":"Re: [PATCH 3/5] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Hans,\n\nThanks for the patch. Actually in mtkisp7, we did something very similar\n[1].\n(And I just caught a bug by checking your changes. Thanks :)\n\n[1]:\nhttps://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900\n\nOn Tue, Aug 20, 2024 at 9:50 PM Hans de Goede <hdegoede@redhat.com> wrote:\n\n> ATM libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes\n> for a pipeline open after enumerating the camera.\n>\n> This is a problem for uvcvideo and atomisp /dev/video# nodes as well as\n> for VCM /dev/v4l2_subdev# nodes. Keeping these open stops the underlying\n> hw-devices from being able to enter runtime-suspend causing significant\n> unnecessary power-usage.\n>\n> Add a stub acquireDevice() method to the PipelineHandler class which\n> pipeline handlers can override to delay opening /dev nodes until\n> the camera is acquired.\n>\n> Note pipeline handlers typically also will need access to /dev nodes\n> for their CameraConfig::validate() implementation for tryFormat() calls.\n> Making sure that /dev nodes are opened as necessary from validate() is\n> left up to the pipeline handler implementation.\n>\n> Signed-off-by: Hans de Goede <hdegoede@redhat.com>\n> ---\n>  include/libcamera/internal/pipeline_handler.h |  3 +-\n>  src/libcamera/camera.cpp                      |  2 +-\n>  src/libcamera/pipeline_handler.cpp            | 43 +++++++++++++++----\n>  3 files changed, 37 insertions(+), 11 deletions(-)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h\n> b/include/libcamera/internal/pipeline_handler.h\n> index cad5812f..597f7c3e 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -45,7 +45,7 @@ public:\n>         MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,\n>                                         const DeviceMatch &dm);\n>\n> -       bool acquire();\n> +       bool acquire(Camera *camera);\n>         void release(Camera *camera);\n>\n>         virtual std::unique_ptr<CameraConfiguration>\n> generateConfiguration(Camera *camera,\n> @@ -79,6 +79,7 @@ protected:\n>         virtual int queueRequestDevice(Camera *camera, Request *request) =\n> 0;\n>         virtual void stopDevice(Camera *camera) = 0;\n>\n> +       virtual bool acquireDevice(Camera *camera);\n>         virtual void releaseDevice(Camera *camera);\n>\n>         CameraManager *manager_;\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 382a68f7..4e393f89 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -995,7 +995,7 @@ int Camera::acquire()\n>         if (ret < 0)\n>                 return ret == -EACCES ? -EBUSY : ret;\n>\n> -       if (!d->pipe_->acquire()) {\n> +       if (!d->pipe_->acquire(this)) {\n>                 LOG(Camera, Info)\n>                         << \"Pipeline handler in use by another process\";\n>                 return -EBUSY;\n> diff --git a/src/libcamera/pipeline_handler.cpp\n> b/src/libcamera/pipeline_handler.cpp\n> index 1fc22d6a..e1342306 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -163,20 +163,22 @@ MediaDevice\n> *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n>   * has already acquired it\n>   * \\sa release()\n>   */\n> -bool PipelineHandler::acquire()\n> +bool PipelineHandler::acquire(Camera *camera)\n>  {\n>         MutexLocker locker(lock_);\n>\n> -       if (useCount_) {\n> -               ++useCount_;\n> -               return true;\n> +       if (useCount_ == 0) {\n> +               for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {\n> +                       if (!media->lock()) {\n> +                               unlockMediaDevices();\n> +                               return false;\n> +                       }\n> +               }\n>         }\n>\n> -       for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {\n> -               if (!media->lock()) {\n> -                       unlockMediaDevices();\n> -                       return false;\n> -               }\n> +       if (!acquireDevice(camera)) {\n> +               unlockMediaDevices();\n>\n\nI think we should only unlock media devices if it's the first acquire\nbeing called, as there might be other ongoing usages on other\ncameras [2]. Therefore:\n```\n                    if (useCount_ == 0)\n                            unlockMediaDevices();\n```\n\n[2]:\nhttps://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900/7/src/libcamera/pipeline_handler.cpp#181\n\n+               return false;\n>         }\n>\n>         ++useCount_;\n> @@ -213,12 +215,35 @@ void PipelineHandler::release(Camera *camera)\n>         --useCount_;\n>  }\n>\n> +/**\n> + * \\brief Acquire resources associated with this camera\n> + * \\param[in] camera The camera for which to acquire resources\n> + *\n> + * Pipeline handlers may override this in order to get resources\n> + * such as open /dev nodes are allocate buffers when a camera is\n> + * acquired.\n> + *\n> + * Note this is called once for every camera which is acquired,\n> + * so if there are shared resources the pipeline handler must take\n> + * care to not release them until releaseDevice() has been called for\n> + * all previously acquired cameras.\n> + *\n> + * \\return True on success, false on failure.\n> + * \\sa releaseDevice()\n> + */\n> +bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)\n> +{\n> +       return true;\n> +}\n> +\n>  /**\n>   * \\brief Release resources associated with this camera\n>   * \\param[in] camera The camera for which to release resources\n>   *\n>   * Pipeline handlers may override this in order to perform cleanup\n> operations\n>   * when a camera is released, such as freeing memory.\n> + *\n> + * \\sa acquireDevice()\n>   */\n>  void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)\n>  {\n> --\n> 2.46.0\n>\n>\nBR,\nHarvey","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 4DA68BDE17\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 21 Aug 2024 16:56:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2AF0A633CF;\n\tWed, 21 Aug 2024 18:55:59 +0200 (CEST)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 862F163394\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 21 Aug 2024 18:55:57 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id\n\t38308e7fff4ca-2f3e071eb64so51326231fa.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 21 Aug 2024 09:55:57 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"n/J+JLde\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1724259357; x=1724864157;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=yiPe/biaIz5Q+v0KXPDPggQSeL/CHQSHSQ1b30d/u+Y=;\n\tb=n/J+JLdeUsTfoDVPffihFN8upas/0pKf9tSBQFtjQD+4yzUSK9rN42TdUCB/pogjoa\n\txWXg520qFqJMqvm1t4qI5XlU1AeyHtzropHgT8VEdnP6sClaZtwt22vbMgE43gqy3gWl\n\tqBS8Yn2DGGgiRr0gWq2mKAN83FthZDwDgXdYc=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724259357; x=1724864157;\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=yiPe/biaIz5Q+v0KXPDPggQSeL/CHQSHSQ1b30d/u+Y=;\n\tb=Ucr664scSDPPSl9ImvwAo93wpqLsgqQ86e7pdZdAgj1mlUF8g6s8Kz7T4YNxjeBGty\n\tlXwvqfNsrJZJZHQzRdVv4Ky/FgpbhKeAfWzgiPS4j4hmZ2kS3WZ/Dc23EdVgzWwc1rBY\n\tpvhJDATuUt+jH6DuKm7kOS/k7+v4Y2lYjI0Jwn4oa/gIYhWktklgQPTZd7XbCicJU0p0\n\tIk6/uvvSXAst8X9mjt7wLH3Ip5Jugr6EKglEMoVyT6dIyuKlLdukx72yr2C4kL/oqztd\n\tMQC8wuox6gPsAKrpPWNgCLrBNE2oGeQovbx46JGfnS0YX+NE+8+YDFVCqFKahqK7V50w\n\tLbCQ==","X-Gm-Message-State":"AOJu0YyaMccRyL7O9xOTKe2y4ZOUr4b7R3JWa8LudfrAWSd1sR5VdP23\n\tp6k9fmH/nwZkIaMqgZFtVO9+t1WIoS6jtF1WLk/jD79Voyk1DcJk8oE2dgqNaJbbZG8U5kPLTdo\n\tbPH1xvRBnd4wyEjNXNbXhYNlOg0nn1r+iEuH+","X-Google-Smtp-Source":"AGHT+IGVKze6fNUFJC5m6pI3LpjMwG6G7T5NoTfCKiB5DWTZjwKjPJCeeg/73GilBeZKtzgosPatt/04+nAv3uFDlCc=","X-Received":"by 2002:a2e:b896:0:b0:2f3:c384:71ee with SMTP id\n\t38308e7fff4ca-2f3f8948c2cmr33031961fa.33.1724259356398;\n\tWed, 21 Aug 2024 09:55:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-4-hdegoede@redhat.com>","In-Reply-To":"<20240820195016.16028-4-hdegoede@redhat.com>","From":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Date":"Wed, 21 Aug 2024 18:55:45 +0200","Message-ID":"<CAEB1ahsg4D771B0QjWnjkxJr5KFPHZvVZo8ivJJfFhaevVyaQw@mail.gmail.com>","Subject":"Re: [PATCH 3/5] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","To":"Hans de Goede <hdegoede@redhat.com>","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard <mripard@redhat.com>, \n\tMilan Zamazal <mzamazal@redhat.com>","Content-Type":"multipart/alternative; boundary=\"000000000000706a1b0620346a74\"","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":30893,"web_url":"https://patchwork.libcamera.org/comment/30893/","msgid":"<20240825004957.GB17238@pendragon.ideasonboard.com>","date":"2024-08-25T00:49:57","subject":"Re: [PATCH 3/5] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Aug 21, 2024 at 06:55:45PM +0200, Cheng-Hao Yang wrote:\n> Hi Hans,\n> \n> Thanks for the patch. Actually in mtkisp7, we did something very similar\n> [1].\n> (And I just caught a bug by checking your changes. Thanks :)\n> \n> [1]:\n> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900\n> \n> On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede <hdegoede@redhat.com> wrote:\n> \n> > ATM libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes\n> > for a pipeline open after enumerating the camera.\n> >\n> > This is a problem for uvcvideo and atomisp /dev/video# nodes as well as\n> > for VCM /dev/v4l2_subdev# nodes. Keeping these open stops the underlying\n> > hw-devices from being able to enter runtime-suspend causing significant\n> > unnecessary power-usage.\n> >\n> > Add a stub acquireDevice() method to the PipelineHandler class which\n> > pipeline handlers can override to delay opening /dev nodes until\n> > the camera is acquired.\n> >\n> > Note pipeline handlers typically also will need access to /dev nodes\n> > for their CameraConfig::validate() implementation for tryFormat() calls.\n> > Making sure that /dev nodes are opened as necessary from validate() is\n> > left up to the pipeline handler implementation.\n> >\n> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>\n> > ---\n> >  include/libcamera/internal/pipeline_handler.h |  3 +-\n> >  src/libcamera/camera.cpp                      |  2 +-\n> >  src/libcamera/pipeline_handler.cpp            | 43 +++++++++++++++----\n> >  3 files changed, 37 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h\n> > b/include/libcamera/internal/pipeline_handler.h\n> > index cad5812f..597f7c3e 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -45,7 +45,7 @@ public:\n> >         MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,\n> >                                         const DeviceMatch &dm);\n> >\n> > -       bool acquire();\n> > +       bool acquire(Camera *camera);\n> >         void release(Camera *camera);\n> >\n> >         virtual std::unique_ptr<CameraConfiguration>\n> > generateConfiguration(Camera *camera,\n> > @@ -79,6 +79,7 @@ protected:\n> >         virtual int queueRequestDevice(Camera *camera, Request *request) =\n> > 0;\n> >         virtual void stopDevice(Camera *camera) = 0;\n> >\n> > +       virtual bool acquireDevice(Camera *camera);\n> >         virtual void releaseDevice(Camera *camera);\n> >\n> >         CameraManager *manager_;\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 382a68f7..4e393f89 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -995,7 +995,7 @@ int Camera::acquire()\n> >         if (ret < 0)\n> >                 return ret == -EACCES ? -EBUSY : ret;\n> >\n> > -       if (!d->pipe_->acquire()) {\n> > +       if (!d->pipe_->acquire(this)) {\n> >                 LOG(Camera, Info)\n> >                         << \"Pipeline handler in use by another process\";\n> >                 return -EBUSY;\n> > diff --git a/src/libcamera/pipeline_handler.cpp\n> > b/src/libcamera/pipeline_handler.cpp\n> > index 1fc22d6a..e1342306 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -163,20 +163,22 @@ MediaDevice\n> > *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> >   * has already acquired it\n> >   * \\sa release()\n> >   */\n> > -bool PipelineHandler::acquire()\n> > +bool PipelineHandler::acquire(Camera *camera)\n> >  {\n> >         MutexLocker locker(lock_);\n> >\n> > -       if (useCount_) {\n> > -               ++useCount_;\n> > -               return true;\n> > +       if (useCount_ == 0) {\n> > +               for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {\n> > +                       if (!media->lock()) {\n> > +                               unlockMediaDevices();\n> > +                               return false;\n> > +                       }\n> > +               }\n> >         }\n> >\n> > -       for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {\n> > -               if (!media->lock()) {\n> > -                       unlockMediaDevices();\n> > -                       return false;\n> > -               }\n> > +       if (!acquireDevice(camera)) {\n> > +               unlockMediaDevices();\n> >\n> \n> I think we should only unlock media devices if it's the first acquire\n> being called, as there might be other ongoing usages on other\n> cameras [2]. Therefore:\n> ```\n>                     if (useCount_ == 0)\n>                             unlockMediaDevices();\n> ```\n> \n> [2]:\n> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900/7/src/libcamera/pipeline_handler.cpp#181\n> \n> +               return false;\n> >         }\n> >\n> >         ++useCount_;\n> > @@ -213,12 +215,35 @@ void PipelineHandler::release(Camera *camera)\n> >         --useCount_;\n> >  }\n> >\n> > +/**\n> > + * \\brief Acquire resources associated with this camera\n> > + * \\param[in] camera The camera for which to acquire resources\n> > + *\n> > + * Pipeline handlers may override this in order to get resources\n> > + * such as open /dev nodes are allocate buffers when a camera is\n\ns@open /dev nodes@opening devices/\ns/are allocate/and allocating/\n\nAs discussed previously, I think we need to improve the\napplication-facing API to make resource and power management more\nexplicit. Camera::acquire() was never meant for this purpose. I\nunderstand we need a short term solution to the power consumption issue\nfor UVC cameras without an enormous amount of yak shaving, and I'm fine\nwith this patch series as an interim measure, but I don't want to see\nacquireDevice() being used in random ways by pipeline handlers other\nthan UVC before we address the more general problem. This should be\ndocumented here.\n\n> > + * acquired.\n\nPlease reflow these paragraphs to 80 columns.\n\n> > + *\n> > + * Note this is called once for every camera which is acquired,\n\ns/which/that/\n\n> > + * so if there are shared resources the pipeline handler must take\n\ns/shared resources/resources shared by multiple cameras/\n\n> > + * care to not release them until releaseDevice() has been called for\n> > + * all previously acquired cameras.\n\nDoesn't this paragraph belong to releaseDevice() ?\n\n> > + *\n> > + * \\return True on success, false on failure.\n> > + * \\sa releaseDevice()\n> > + */\n> > +bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)\n> > +{\n> > +       return true;\n> > +}\n> > +\n> >  /**\n> >   * \\brief Release resources associated with this camera\n> >   * \\param[in] camera The camera for which to release resources\n> >   *\n> >   * Pipeline handlers may override this in order to perform cleanup\n> > operations\n> >   * when a camera is released, such as freeing memory.\n> > + *\n> > + * \\sa acquireDevice()\n> >   */\n> >  void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)\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 DCD0CBDE17\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 25 Aug 2024 00:50:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEB8E633CF;\n\tSun, 25 Aug 2024 02:50:03 +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 16D2E61902\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 25 Aug 2024 02:50:02 +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 9856D480;\n\tSun, 25 Aug 2024 02:48:56 +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=\"UxJ9ftLw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724546936;\n\tbh=Xy6VDPPV5cP/MYVrfReJPx5ojAgNar98jRSlIeaiCfo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UxJ9ftLw2LlfEAVsewvYXYPzzWDAoQMeE9lKUOTf7Bv6iP8gafhOIOsiOHZMCxHHs\n\t+irjRIdhU27xR4T/mVsXvy6Q7geLM3PhtdkxIad4dPtLsCNDhVl2R9C1VL8OI4uCqb\n\tPlsPUTBFrDe49AkjJ4RsoVtm4YZqNv+UC5ESgkwg=","Date":"Sun, 25 Aug 2024 03:49:57 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Cc":"Hans de Goede <hdegoede@redhat.com>, libcamera-devel@lists.libcamera.org,\n\tMaxime Ripard <mripard@redhat.com>, Milan Zamazal <mzamazal@redhat.com>","Subject":"Re: [PATCH 3/5] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","Message-ID":"<20240825004957.GB17238@pendragon.ideasonboard.com>","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-4-hdegoede@redhat.com>\n\t<CAEB1ahsg4D771B0QjWnjkxJr5KFPHZvVZo8ivJJfFhaevVyaQw@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<CAEB1ahsg4D771B0QjWnjkxJr5KFPHZvVZo8ivJJfFhaevVyaQw@mail.gmail.com>","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":30922,"web_url":"https://patchwork.libcamera.org/comment/30922/","msgid":"<79188a4f-0409-4581-869f-07aa20d94110@redhat.com>","date":"2024-08-27T14:11:47","subject":"Re: [PATCH 3/5] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 8/21/24 6:55 PM, Cheng-Hao Yang wrote:\n> Hi Hans,\n> \n> Thanks for the patch. Actually in mtkisp7, we did something very similar [1].\n> (And I just caught a bug by checking your changes. Thanks :)\n> \n> [1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900 <https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900>\n> \n> On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:\n> \n>     ATM libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes\n>     for a pipeline open after enumerating the camera.\n> \n>     This is a problem for uvcvideo and atomisp /dev/video# nodes as well as\n>     for VCM /dev/v4l2_subdev# nodes. Keeping these open stops the underlying\n>     hw-devices from being able to enter runtime-suspend causing significant\n>     unnecessary power-usage.\n> \n>     Add a stub acquireDevice() method to the PipelineHandler class which\n>     pipeline handlers can override to delay opening /dev nodes until\n>     the camera is acquired.\n> \n>     Note pipeline handlers typically also will need access to /dev nodes\n>     for their CameraConfig::validate() implementation for tryFormat() calls.\n>     Making sure that /dev nodes are opened as necessary from validate() is\n>     left up to the pipeline handler implementation.\n> \n>     Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>\n>     ---\n>      include/libcamera/internal/pipeline_handler.h |  3 +-\n>      src/libcamera/camera.cpp                      |  2 +-\n>      src/libcamera/pipeline_handler.cpp            | 43 +++++++++++++++----\n>      3 files changed, 37 insertions(+), 11 deletions(-)\n> \n>     diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>     index cad5812f..597f7c3e 100644\n>     --- a/include/libcamera/internal/pipeline_handler.h\n>     +++ b/include/libcamera/internal/pipeline_handler.h\n>     @@ -45,7 +45,7 @@ public:\n>             MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,\n>                                             const DeviceMatch &dm);\n> \n>     -       bool acquire();\n>     +       bool acquire(Camera *camera);\n>             void release(Camera *camera);\n> \n>             virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,\n>     @@ -79,6 +79,7 @@ protected:\n>             virtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n>             virtual void stopDevice(Camera *camera) = 0;\n> \n>     +       virtual bool acquireDevice(Camera *camera);\n>             virtual void releaseDevice(Camera *camera);\n> \n>             CameraManager *manager_;\n>     diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>     index 382a68f7..4e393f89 100644\n>     --- a/src/libcamera/camera.cpp\n>     +++ b/src/libcamera/camera.cpp\n>     @@ -995,7 +995,7 @@ int Camera::acquire()\n>             if (ret < 0)\n>                     return ret == -EACCES ? -EBUSY : ret;\n> \n>     -       if (!d->pipe_->acquire()) {\n>     +       if (!d->pipe_->acquire(this)) {\n>                     LOG(Camera, Info)\n>                             << \"Pipeline handler in use by another process\";\n>                     return -EBUSY;\n>     diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>     index 1fc22d6a..e1342306 100644\n>     --- a/src/libcamera/pipeline_handler.cpp\n>     +++ b/src/libcamera/pipeline_handler.cpp\n>     @@ -163,20 +163,22 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n>       * has already acquired it\n>       * \\sa release()\n>       */\n>     -bool PipelineHandler::acquire()\n>     +bool PipelineHandler::acquire(Camera *camera)\n>      {\n>             MutexLocker locker(lock_);\n> \n>     -       if (useCount_) {\n>     -               ++useCount_;\n>     -               return true;\n>     +       if (useCount_ == 0) {\n>     +               for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {\n>     +                       if (!media->lock()) {\n>     +                               unlockMediaDevices();\n>     +                               return false;\n>     +                       }\n>     +               }\n>             }\n> \n>     -       for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {\n>     -               if (!media->lock()) {\n>     -                       unlockMediaDevices();\n>     -                       return false;\n>     -               }\n>     +       if (!acquireDevice(camera)) {\n>     +               unlockMediaDevices();\n> \n>  \n> I think we should only unlock media devices if it's the first acquire\n> being called, as there might be other ongoing usages on other\n> cameras [2]. Therefore:\n> ```\n>                     if (useCount_ == 0)\n>                             unlockMediaDevices();\n> ```\n\nGood point, fixed for v2.\n\nRegards,\n\nHans","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 EB7A2C323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 27 Aug 2024 14:11:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E531D63460;\n\tTue, 27 Aug 2024 16:11:54 +0200 (CEST)","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 A054961901\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 27 Aug 2024 16:11:53 +0200 (CEST)","from mail-lj1-f197.google.com (mail-lj1-f197.google.com\n\t[209.85.208.197]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-160-y2FB4cIMPC-f6eqiCYG2NQ-1; Tue, 27 Aug 2024 10:11:50 -0400","by mail-lj1-f197.google.com with SMTP id\n\t38308e7fff4ca-2f3f61b42c2so61069231fa.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 27 Aug 2024 07:11:49 -0700 (PDT)","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-a86e591c015sm113563966b.191.2024.08.27.07.11.47\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 27 Aug 2024 07:11:47 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"R5oTI0Cw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1724767912;\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=5/xqEtCe1nnXRAoeVTUMJ7pSJtUCesymYvRgp5cuCjU=;\n\tb=R5oTI0CwZCqEkpjjvuJrO/QMdAwaMioBnj7YmDMVYbH6Zqvrx84dB1PGBXO3Sygagtwe0L\n\t4pr0DOgw9VaUlgu9i2FGYDTXYdR3G81lsqPnb/YdrbzmjlB/B1Wj6qz6f82AhFfRR0VL92\n\tqdHZlX46dDf8Wrk510Dpl4VA9crRhKI=","X-MC-Unique":"y2FB4cIMPC-f6eqiCYG2NQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724767909; x=1725372709;\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=5/xqEtCe1nnXRAoeVTUMJ7pSJtUCesymYvRgp5cuCjU=;\n\tb=dJ87H7LfeMOe1Oh6ej9Bem38qYdgaT4wJxMWc1PWVbpwfZ5E6zWRHgrS2ub68EiFxy\n\tkKnI6BTjQFDiEef+yfY1YiB+4dkdlDi0qVb1sATiwuZ4V1pT8uhWoFZ4iK29130hKaOi\n\t/H/c3zNHm79OaY8zUWUBFazJDqUqXu/xchgCC1JzGNY+VVrzZUkvEDoxmCK5pd3WSu74\n\tYF3F2yzTqOdVX9W4r3+O71v3LALO0DORzrESkKW2NocXyCW7FrVrCSarGKHpKy98T0Vk\n\tUSBym61GbHbUCKUFzLR3YKX4c4RbmzGPg9ortqB/iKJ9JykjdE7lJcI/zZyTdpKiMFwx\n\tc+9Q==","X-Gm-Message-State":"AOJu0YykpMnG8X5tA7wGP26V9bsftaHCoFH+yqCaBsL7IyMz/gbi9llp\n\tpQkTMzzEZL+ZFLgtJGzDuH4qcPfR/zSNf69gP0nY/EHid77EFC6lc4ZH2lke1nT3P3h7wTOjjNV\n\tcrelzE5OK8KyNjrK5E5NpgaSTMIyL81nqzz5DdFAZqytK9kgIc4DHyAzvCgv8LoGgYWcrwbY=","X-Received":["by 2002:a2e:bc17:0:b0:2f5:966:c22e with SMTP id\n\t38308e7fff4ca-2f50966c499mr67599271fa.11.1724767908615; \n\tTue, 27 Aug 2024 07:11:48 -0700 (PDT)","by 2002:a2e:bc17:0:b0:2f5:966:c22e with SMTP id\n\t38308e7fff4ca-2f50966c499mr67598961fa.11.1724767907979; \n\tTue, 27 Aug 2024 07:11:47 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IELBAsQUDkUYdgIyHwefV93RYc3KsL9Nu1w/2o43bJWeip81uZM+km/cFG5khEDxXJLGVYTSw==","Message-ID":"<79188a4f-0409-4581-869f-07aa20d94110@redhat.com>","Date":"Tue, 27 Aug 2024 16:11:47 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 3/5] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","To":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard <mripard@redhat.com>, \n\tMilan Zamazal <mzamazal@redhat.com>","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-4-hdegoede@redhat.com>\n\t<CAEB1ahsg4D771B0QjWnjkxJr5KFPHZvVZo8ivJJfFhaevVyaQw@mail.gmail.com>","From":"Hans de Goede <hdegoede@redhat.com>","In-Reply-To":"<CAEB1ahsg4D771B0QjWnjkxJr5KFPHZvVZo8ivJJfFhaevVyaQw@mail.gmail.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":30924,"web_url":"https://patchwork.libcamera.org/comment/30924/","msgid":"<c0182c39-d0dc-4446-aea2-2326dd9d8f1c@redhat.com>","date":"2024-08-27T14:25:00","subject":"Re: [PATCH 3/5] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 8/25/24 2:49 AM, Laurent Pinchart wrote:\n\n<snip>\n\n>>> @@ -213,12 +215,35 @@ void PipelineHandler::release(Camera *camera)\n>>>         --useCount_;\n>>>  }\n>>>\n>>> +/**\n>>> + * \\brief Acquire resources associated with this camera\n>>> + * \\param[in] camera The camera for which to acquire resources\n>>> + *\n>>> + * Pipeline handlers may override this in order to get resources\n>>> + * such as open /dev nodes are allocate buffers when a camera is\n> \n> s@open /dev nodes@opening devices/\n> s/are allocate/and allocating/\n\nAck, fixed for v2.\n\n> As discussed previously, I think we need to improve the\n> application-facing API to make resource and power management more\n> explicit. Camera::acquire() was never meant for this purpose. I\n> understand we need a short term solution to the power consumption issue\n> for UVC cameras without an enormous amount of yak shaving\n\nAck, I have posted a RFC with the application-facing API changes we discussed\nfor this:\n\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2024-August/044384.html\n\n> and I'm fine\n> with this patch series as an interim measure, but I don't want to see\n> acquireDevice() being used in random ways by pipeline handlers other\n> than UVC before we address the more general problem. This should be\n> documented here.\n\nOk I have added the following block to the docbook comment to address\nthis for v2:\n\n * This is used by the uvcvideo pipeline handler to delay opening /dev/video#\n * until the camera is acquired to avoid excess power consumption. The delayed\n * opening of /dev/video# is a special case because the kernel uvcvideo driver\n * powers on the USB device as soon as /dev/video# is opened. This behavior\n * must *not* be copied by other drivers.\n\n>>> + * acquired.\n> \n> Please reflow these paragraphs to 80 columns.\n\nAck, done for v2.\n\n> \n>>> + *\n>>> + * Note this is called once for every camera which is acquired,\n> \n> s/which/that/\n> \n>>> + * so if there are shared resources the pipeline handler must take\n> \n> s/shared resources/resources shared by multiple cameras/\n> \n>>> + * care to not release them until releaseDevice() has been called for\n>>> + * all previously acquired cameras.\n> \n> Doesn't this paragraph belong to releaseDevice() ?\n\nthat is a better place yes, I have moved it for v2.\n\nRegards,\n\nHans","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 A9AF2C324C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 27 Aug 2024 14:25:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6DBD63461;\n\tTue, 27 Aug 2024 16:25:07 +0200 (CEST)","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 52A2D61901\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 27 Aug 2024 16:25:06 +0200 (CEST)","from mail-lf1-f69.google.com (mail-lf1-f69.google.com\n\t[209.85.167.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-396-bQrP9I-nMJuwD3cBaZ2tMQ-1; Tue, 27 Aug 2024 10:25:03 -0400","by mail-lf1-f69.google.com with SMTP id\n\t2adb3069b0e04-533500041b1so6129447e87.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 27 Aug 2024 07:25:03 -0700 (PDT)","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-a86e594a5e4sm115455166b.202.2024.08.27.07.25.00\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 27 Aug 2024 07:25:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"avs1PYtD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1724768705;\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=bm4CIw7uTGaXqPcZw/0xeXEHhzQBYDEKxHNOxvmZkB0=;\n\tb=avs1PYtDMPoAw9cTS3phmOh9wA7AyOA0ehBe16p80YieEX2JP+u3pVhLPmM4OTl6qP9R8D\n\tCLEEI0qL2Qqd/xUACiPsEw+vIebDyqEWyQyM1c5XfdAIFEjqkXoVv4/mAzyRTLOnMpo5MV\n\tKIKS4byvI4Fcmpqp6q+DHywZ3eOMc8o=","X-MC-Unique":"bQrP9I-nMJuwD3cBaZ2tMQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724768702; x=1725373502;\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=bm4CIw7uTGaXqPcZw/0xeXEHhzQBYDEKxHNOxvmZkB0=;\n\tb=NcDZF7wG4o5lpEeL9+yQwbE7mOevZ8hTeawXIL5BKOasTqpK9fR03IdaGlv/HtkMNh\n\tbZA2YeTxgnPk/WLTcTcz4pcLquvOZynoXEJ2JX/dqrhSFNSHD8zaVPuoAaiIPzkaohbm\n\tSxyuD+pLI04UYYbI4xnPXeB0KtkyZtMzw3udi0R8YPvcP8DnxSp6jRa/PSdGXyH/U02t\n\t5ZR6g05nFNr+SSvaqQJgxwNT1cWpXj403h4xFe12HDzl82KQ+3SmGQIUGMBeLrkXWup6\n\tYzpjoIneXLAczPb4JgQF0xladJKETmSrQb6y4qPkUq6xMrxfKzaY8moSWLCkRu7eDlh4\n\tZB9Q==","X-Gm-Message-State":"AOJu0YwK7GSerQ4+5CgsbNOGZNRc2hKv+XA8ro0aj63K1NyjEG9groqj\n\tHEaiIrAkacQgUsEjBL/aV11SCI/ddBx/GfyhF8QfMqE5xR3TfA3qSfj70EB7VbMjkDDaH1c1NIb\n\tMcH3WCgAj4jawMXn+dTHlskX8O4S6yesuvYzYKROPdYjSLIFEH1BjkXQkHD5C81cYunQAUqI=","X-Received":["by 2002:a05:6512:1596:b0:52c:deb9:904b with SMTP id\n\t2adb3069b0e04-5343887e17bmr12093845e87.38.1724768701975; \n\tTue, 27 Aug 2024 07:25:01 -0700 (PDT)","by 2002:a05:6512:1596:b0:52c:deb9:904b with SMTP id\n\t2adb3069b0e04-5343887e17bmr12093812e87.38.1724768701402; \n\tTue, 27 Aug 2024 07:25:01 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFWFHD/U9MYIZxia6mp3q4ktTAecXnMEpfYjQwHrglh/rR0goMwoX11bOlkm7VUTDL4Q4IHjA==","Message-ID":"<c0182c39-d0dc-4446-aea2-2326dd9d8f1c@redhat.com>","Date":"Tue, 27 Aug 2024 16:25:00 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 3/5] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tCheng-Hao Yang <chenghaoyang@chromium.org>","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard <mripard@redhat.com>, \n\tMilan Zamazal <mzamazal@redhat.com>","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-4-hdegoede@redhat.com>\n\t<CAEB1ahsg4D771B0QjWnjkxJr5KFPHZvVZo8ivJJfFhaevVyaQw@mail.gmail.com>\n\t<20240825004957.GB17238@pendragon.ideasonboard.com>","From":"Hans de Goede <hdegoede@redhat.com>","In-Reply-To":"<20240825004957.GB17238@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","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":"<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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":30963,"web_url":"https://patchwork.libcamera.org/comment/30963/","msgid":"<20240829210631.GF15799@pendragon.ideasonboard.com>","date":"2024-08-29T21:06:31","subject":"Re: [PATCH 3/5] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nOn Tue, Aug 27, 2024 at 04:25:00PM +0200, Hans de Goede wrote:\n> On 8/25/24 2:49 AM, Laurent Pinchart wrote:\n> \n> <snip>\n> \n> >>> @@ -213,12 +215,35 @@ void PipelineHandler::release(Camera *camera)\n> >>>         --useCount_;\n> >>>  }\n> >>>\n> >>> +/**\n> >>> + * \\brief Acquire resources associated with this camera\n> >>> + * \\param[in] camera The camera for which to acquire resources\n> >>> + *\n> >>> + * Pipeline handlers may override this in order to get resources\n> >>> + * such as open /dev nodes are allocate buffers when a camera is\n> > \n> > s@open /dev nodes@opening devices/\n> > s/are allocate/and allocating/\n> \n> Ack, fixed for v2.\n> \n> > As discussed previously, I think we need to improve the\n> > application-facing API to make resource and power management more\n> > explicit. Camera::acquire() was never meant for this purpose. I\n> > understand we need a short term solution to the power consumption issue\n> > for UVC cameras without an enormous amount of yak shaving\n> \n> Ack, I have posted a RFC with the application-facing API changes we discussed\n> for this:\n> \n> https://lists.libcamera.org/pipermail/libcamera-devel/2024-August/044384.html\n\nThank you for that. I really appreciate your efforts. I'll reply to the\nRFC after reviewing v2 of this series.\n\n> > and I'm fine\n> > with this patch series as an interim measure, but I don't want to see\n> > acquireDevice() being used in random ways by pipeline handlers other\n> > than UVC before we address the more general problem. This should be\n> > documented here.\n> \n> Ok I have added the following block to the docbook comment to address\n> this for v2:\n> \n>  * This is used by the uvcvideo pipeline handler to delay opening /dev/video#\n>  * until the camera is acquired to avoid excess power consumption. The delayed\n>  * opening of /dev/video# is a special case because the kernel uvcvideo driver\n>  * powers on the USB device as soon as /dev/video# is opened. This behavior\n>  * must *not* be copied by other drivers.\n> \n> >>> + * acquired.\n> > \n> > Please reflow these paragraphs to 80 columns.\n> \n> Ack, done for v2.\n> \n> >>> + *\n> >>> + * Note this is called once for every camera which is acquired,\n> > \n> > s/which/that/\n> > \n> >>> + * so if there are shared resources the pipeline handler must take\n> > \n> > s/shared resources/resources shared by multiple cameras/\n> > \n> >>> + * care to not release them until releaseDevice() has been called for\n> >>> + * all previously acquired cameras.\n> > \n> > Doesn't this paragraph belong to releaseDevice() ?\n> \n> that is a better place yes, I have moved it for v2.","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 7C97AC324C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 29 Aug 2024 21:07:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 688C063469;\n\tThu, 29 Aug 2024 23:07:05 +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 BC6F463456\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 29 Aug 2024 23:07:03 +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 DD9E4226;\n\tThu, 29 Aug 2024 23:05:54 +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=\"wbaXYpeu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724965555;\n\tbh=FvU9ylIYkzbOFIYIa9xqF2vEM4AWFOX8n+sIYFPuMiM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wbaXYpeuWijW8HY12e1qkC36R6VHEDd2bAHsY0jn+OvU5VExjnN8IhghIeYpR7LAW\n\taZAcMS3DQthZKH0edI5eht8nLCUJaCPkQUc8F2M3lJmIJHIgz2SjCl3I4jOO6NIPXU\n\tnPjDh1iTdGge5SuxnuuolqD7ROhLZn5CIMZi0hzc=","Date":"Fri, 30 Aug 2024 00:06:31 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Hans de Goede <hdegoede@redhat.com>","Cc":"Cheng-Hao Yang <chenghaoyang@chromium.org>,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard <mripard@redhat.com>, \n\tMilan Zamazal <mzamazal@redhat.com>","Subject":"Re: [PATCH 3/5] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","Message-ID":"<20240829210631.GF15799@pendragon.ideasonboard.com>","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-4-hdegoede@redhat.com>\n\t<CAEB1ahsg4D771B0QjWnjkxJr5KFPHZvVZo8ivJJfFhaevVyaQw@mail.gmail.com>\n\t<20240825004957.GB17238@pendragon.ideasonboard.com>\n\t<c0182c39-d0dc-4446-aea2-2326dd9d8f1c@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<c0182c39-d0dc-4446-aea2-2326dd9d8f1c@redhat.com>","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]