[{"id":22848,"web_url":"https://patchwork.libcamera.org/comment/22848/","msgid":"<165174338949.1423360.2114054147343558842@Monstersaurus>","date":"2022-05-05T09:36:29","subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nQuoting Naushir Patuck via libcamera-devel (2022-05-05 09:48:24)\n> Only enable/reset the watchdog timer when there are buffers queued in the V4L2\n> device. Otherwise, we may trigger spurious warnings when the watchdog times out\n> even if there are no buffers queued in the device.\n\naha yes - I can see how that was tripping up on python interactive\nsessions or otherwise underflowing when it's not at all a fault of the\nV4L2VideoDevice.\n\n\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++-------\n> 1 file changed, 15 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 5b4637b1a39e..430715afd554 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> return ret;\n> }\n> \n> - if (queuedBuffers_.empty())\n> + if (queuedBuffers_.empty()) {\n> fdBufferNotifier_->setEnabled(true);\n> + if (watchdogDuration_)\n> + watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> + }\n\nI guess this could also be set outside of the if (queuedBuffer_.empty())\n- but it would artificially delay the watchdog every time a buffer was\nqueued. In the event that more than one buffer is required to be queued\n(to satisfy internal requirements perhaps?) that might actually be\nbeneficial ... But I think either way is fine.\n\n> \n> queuedBuffers_[buf.index] = buffer;\n> \n> @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> return nullptr;\n> }\n> \n> - if (watchdogDuration_.count())\n> - watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> -\n> cache_->put(buf.index);\n> \n> FrameBuffer *buffer = it->second;\n> queuedBuffers_.erase(it);\n> \n> - if (queuedBuffers_.empty())\n> + if (queuedBuffers_.empty()) {\n> fdBufferNotifier_->setEnabled(false);\n> + watchdog_.stop();\n> + } else if (watchdogDuration_) {\n> + /*\n> + * Restart the watchdog timer if there are buffers still queued\n> + * in the device.\n> + */\n> + watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n\nWhy do we need/have all these casts? Is either watchdogDuration_ not a\nduration? or is watchdog_.start() not accepting a duration? Either of\nthose would remove a lot of casting surely?\n\nwatchdogDuration_ is a utils::Duration which stores std::nano, and I see\nTimer start takes a std::chrono::milliseconds. I think it would make\nsense to add a 'void start(std::chrono::duration)' to the timer class\nand simplify these casts. But that doesn't have to be fixed in this\npatch.\n\nWould you like to do that as a patch on top? If you don't want to let me\nknow and I'll handle it after this patch is merged.\n\n\nReviewed-by: Kieran Bingham \n\n\n> + }\n> \n> buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR\n> ? FrameMetadata::FrameError\n> @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn()\n> }\n> \n> state_ = State::Streaming;\n> - if (watchdogDuration_.count())\n> + if (watchdogDuration_ && !queuedBuffers_.empty())\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> \n> return 0;\n> @@ -1924,7 +1932,7 @@ void V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout)\n> watchdogDuration_ = timeout;\n> \n> watchdog_.stop();\n> - if (watchdogDuration_.count() && state_ == State::Streaming)\n> + if (watchdogDuration_ && state_ == State::Streaming && !queuedBuffers_.empty())\n> watchdog_.start(std::chrono::duration_cast(timeout));\n> }\n> \n> -- \n> 2.25.1\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 43097C3256\n\tfor ;\n\tThu, 5 May 2022 09:36:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 84C6B65647;\n\tThu, 5 May 2022 11:36:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB82B604A2\n\tfor ;\n\tThu, 5 May 2022 11:36:32 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4A945492;\n\tThu, 5 May 2022 11:36:32 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651743393;\n\tbh=L7/WmvBWW5nH4eKFOeCq6RgnISZtLfvMaSZv3NaIRPw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=Yeh0vuTabs5rUGn3NoC57RKAUP79E7R8UxPPXReEgA1raGNH7/8pYIVP2FFFBIMfp\n\tdRNqgcNEWfFuWzmdtfkJrcgVK3ldVFt05y92GMliwZd0T8g+AhP8Nk6e8ne0iZp84J\n\tnEGlGlDxpeJre24KLggROVYdusQOwsvSAlcilDSqSTOiUeV9T/QRo/9cVzytcNReN8\n\t6MzQOCutII2mTtRCegWAHQuh3FDpv1+UdUrBJS0p2f7hFKcBudwoZnoYI/LZve+eih\n\thWwMpKxp53Ux9vNhomguehYUGQXvpNRGYiqrl4fZEljZjTKwt/EcOH47kREEkbCQqi\n\tHH0bbDa/ZwZDw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1651743392;\n\tbh=L7/WmvBWW5nH4eKFOeCq6RgnISZtLfvMaSZv3NaIRPw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=JhJgkjMdBHmTztujrg9lN0/n1SRAH3JIP8voxhub7XNx/eX1mK39NiaKfbkbfrv2B\n\tunNk5PDKPBCOiXaTQly1nPOEY+eNmF+hz3RtIxtoIXQRw5zq/OSZhWqL9nM0G54Qcf\n\txSBUIWNfH2aBBFJwTUmYMpHaaj50nw/YBCstsQYo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JhJgkjMd\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220505084824.4104296-1-naush@raspberrypi.com>","References":"<20220505084824.4104296-1-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 05 May 2022 10:36:29 +0100","Message-ID":"<165174338949.1423360.2114054147343558842@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22849,"web_url":"https://patchwork.libcamera.org/comment/22849/","msgid":"","date":"2022-05-05T10:15:44","subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for the feedback!\n\nOn Thu, 5 May 2022 at 10:36, Kieran Bingham \nwrote:\n\n> Hi Naush,\n>\n> Quoting Naushir Patuck via libcamera-devel (2022-05-05 09:48:24)\n> > Only enable/reset the watchdog timer when there are buffers queued in\n> the V4L2\n> > device. Otherwise, we may trigger spurious warnings when the watchdog\n> times out\n> > even if there are no buffers queued in the device.\n>\n> aha yes - I can see how that was tripping up on python interactive\n> sessions or otherwise underflowing when it's not at all a fault of the\n> V4L2VideoDevice.\n>\n>\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++-------\n> > 1 file changed, 15 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp\n> b/src/libcamera/v4l2_videodevice.cpp\n> > index 5b4637b1a39e..430715afd554 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer\n> *buffer)\n> > return ret;\n> > }\n> >\n> > - if (queuedBuffers_.empty())\n> > + if (queuedBuffers_.empty()) {\n> > fdBufferNotifier_->setEnabled(true);\n> > + if (watchdogDuration_)\n> > +\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > + }\n>\n> I guess this could also be set outside of the if (queuedBuffer_.empty())\n> - but it would artificially delay the watchdog every time a buffer was\n> queued. In the event that more than one buffer is required to be queued\n> (to satisfy internal requirements perhaps?) that might actually be\n> beneficial ... But I think either way is fine.\n>\n\nIt could... I thought it might be ever so slightly more logically correct\nif this\nwas inside the if() block. I'll keep it in there unless there are further\nobjections.\n\n\n>\n> >\n> > queuedBuffers_[buf.index] = buffer;\n> >\n> > @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > return nullptr;\n> > }\n> >\n> > - if (watchdogDuration_.count())\n> > -\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > -\n> > cache_->put(buf.index);\n> >\n> > FrameBuffer *buffer = it->second;\n> > queuedBuffers_.erase(it);\n> >\n> > - if (queuedBuffers_.empty())\n> > + if (queuedBuffers_.empty()) {\n> > fdBufferNotifier_->setEnabled(false);\n> > + watchdog_.stop();\n> > + } else if (watchdogDuration_) {\n> > + /*\n> > + * Restart the watchdog timer if there are buffers still\n> queued\n> > + * in the device.\n> > + */\n> > +\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n>\n> Why do we need/have all these casts? Is either watchdogDuration_ not a\n> duration? or is watchdog_.start() not accepting a duration? Either of\n> those would remove a lot of casting surely?\n>\n\nThis is a bit of a minor annoyance because of how I defined the underlying\ntype\nof utils::Duration to a double. The std::chrono library forbids implicit\ncasts\nfrom integer to float types.\n\n\n> watchdogDuration_ is a utils::Duration which stores std::nano, and I see\n> Timer start takes a std::chrono::milliseconds. I think it would make\n> sense to add a 'void start(std::chrono::duration)' to the timer class\n> and simplify these casts. But that doesn't have to be fixed in this\n> patch.\n>\n> Would you like to do that as a patch on top? If you don't want to let me\n> know and I'll handle it after this patch is merged.\n>\n\nWhat we've talked about doing is to switch utils::Duration to use an integer\nunderlying type, which would allow implicit conversions and remove the ugly\ncasts like the above. I believe Laurent was still thinking this through as\nthere\nwere also plans to make utils::Duration part of the public API...\n\nRegards,\nNaush\n\n\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > + }\n> >\n> > buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR\n> > ? FrameMetadata::FrameError\n> > @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn()\n> > }\n> >\n> > state_ = State::Streaming;\n> > - if (watchdogDuration_.count())\n> > + if (watchdogDuration_ && !queuedBuffers_.empty())\n> >\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> >\n> > return 0;\n> > @@ -1924,7 +1932,7 @@ void\n> V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout)\n> > watchdogDuration_ = timeout;\n> >\n> > watchdog_.stop();\n> > - if (watchdogDuration_.count() && state_ == State::Streaming)\n> > + if (watchdogDuration_ && state_ == State::Streaming &&\n> !queuedBuffers_.empty())\n> >\n> watchdog_.start(std::chrono::duration_cast(timeout));\n> > }\n> >\n> > --\n> > 2.25.1\n> >\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 0DE01C0F2A\n\tfor ;\n\tThu, 5 May 2022 10:16:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 55A7860424;\n\tThu, 5 May 2022 12:16:03 +0200 (CEST)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 54DA8603AB\n\tfor ;\n\tThu, 5 May 2022 12:16:01 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id w19so6656325lfu.11\n\tfor ;\n\tThu, 05 May 2022 03:16:01 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651745763;\n\tbh=NUedEZWxWGefffL0Nylka3mlrJZc9DLBm8h5JPlAUHM=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=fAPv6VSy8XpyjxnRfu+nxXlYTPlXYh7dDUmQ8Z7CmdupYgP0x8PEfXmiFHn4oqd5H\n\tn6qUV06OXN6B6yMKSxSiyrNp1JqVSM9THCjaBKhdG/t135s9yHXUhE7rgrpp3xv/hF\n\tUxYl0rZ6fWuOCnEmM+yVa3kP9ks0+zBOcodZ4wPnqXoKl79abDBPZegNLGIR35qM49\n\tN2OUFZImAW+gB6Y++NP/ATQVHgTMvabgLbyp8sB6Kdff9Aj3qrcl9aZJx+jj1DPdz2\n\th5kC6L5goNFYTp7iGB4BY3VW0LBkKfAp4TlpympDAvnaMw8ugAoO2X3l70hJ6Hlv6n\n\tTbH21vq8GbJXA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=zXm7hDIcJ6ri+u1A6RxCY/x9PIqT858HUAONvpfa9po=;\n\tb=QRx0Xwz9HYU15nUqPFopRD0yBkr7PwjZkuTkdqtkbu6KaZXzFe6WpB9xJfuGge+6Al\n\tcMtiIUUpkabnwHfa63V6/Bdg76RJfQoqzFm9xPRdMLUpncNnfSvzoxR7vDySyhJx/P36\n\t/mY5QMo+Q54OuAXrvRmsbJEqa6l8ep94X9rW7zr6udg/WA9KSy3vHvARKxEtYA3pHGoU\n\tTa+hGnmJ8fyeHfBFU8qa48hT2Gg4H0yWtrz1O1bNVaLjOfJS9ZOoWUomKL+xrs9YyezS\n\tFpUmREX6GnPcG95tSknFUewGi63Qm6NW8wuLUascBodSOEsCmWv/WO6Y1q9nxE0jKDeI\n\tokJg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"QRx0Xwz9\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=zXm7hDIcJ6ri+u1A6RxCY/x9PIqT858HUAONvpfa9po=;\n\tb=tg2GJGONFI9onoJCgTjPk8wEbsAMZiS8iknyxLu7Km/Jx0VfqmtYtq06LpBIDnY8Fa\n\tJhmiJW3CzqnkoqN0AGa4HPYwndsUkp87aJ56wUCt6gX4O/HAjSd4rQAd+Me12djShy0Z\n\tv7x914VYP078DAjHPL5Vl5pL0JfZcrFGl71uYLG95wN0GaHXXX+G4dMv3J6hcpjvZGAe\n\t6tk++Vhl+ZVTjar+cKADmzc2ALS9ytAJCY+BeclRD2hFR2L9dDIBtPXzsh2/5FI3x2be\n\tP8GbPWbDmnSDSs0Be1/hwdIaI/MonYWbTaybYYKTqUmFJFGZQGcwplfKlEGyja402e29\n\tUxUw==","X-Gm-Message-State":"AOAM533m8PNASjBcIxA6yryUp28LVbjiD4mOr8AW6IqnarrBsWwlfiiM\n\tfWjU6W6N+tPqvpQ94IPs8FT5KzOJSNnPdR347hRLvA==","X-Google-Smtp-Source":"ABdhPJxGzXtnJKQmmzak5AGSyjj3kTN8rQOAOlXGKPUKeNo0/WcMK1+PzUCORajHENZQZJmJ572hCuSHsBDtTAiUcPs=","X-Received":"by 2002:ac2:596f:0:b0:472:5c36:17f5 with SMTP id\n\th15-20020ac2596f000000b004725c3617f5mr15023384lfp.142.1651745760330;\n\tThu, 05 May 2022 03:16:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20220505084824.4104296-1-naush@raspberrypi.com>\n\t<165174338949.1423360.2114054147343558842@Monstersaurus>","In-Reply-To":"<165174338949.1423360.2114054147343558842@Monstersaurus>","Date":"Thu, 5 May 2022 11:15:44 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"0000000000004de1e505de4107e1\"","Subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22850,"web_url":"https://patchwork.libcamera.org/comment/22850/","msgid":"<165174648332.1423360.10796743332517265418@Monstersaurus>","date":"2022-05-05T10:28:03","subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2022-05-05 11:15:44)\n> Hi Kieran,\n> \n> Thank you for the feedback!\n> \n> On Thu, 5 May 2022 at 10:36, Kieran Bingham \n> wrote:\n> \n> > Hi Naush,\n> >\n> > Quoting Naushir Patuck via libcamera-devel (2022-05-05 09:48:24)\n> > > Only enable/reset the watchdog timer when there are buffers queued in\n> > the V4L2\n> > > device. Otherwise, we may trigger spurious warnings when the watchdog\n> > times out\n> > > even if there are no buffers queued in the device.\n> >\n> > aha yes - I can see how that was tripping up on python interactive\n> > sessions or otherwise underflowing when it's not at all a fault of the\n> > V4L2VideoDevice.\n> >\n> >\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++-------\n> > > 1 file changed, 15 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp\n> > b/src/libcamera/v4l2_videodevice.cpp\n> > > index 5b4637b1a39e..430715afd554 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer\n> > *buffer)\n> > > return ret;\n> > > }\n> > >\n> > > - if (queuedBuffers_.empty())\n> > > + if (queuedBuffers_.empty()) {\n> > > fdBufferNotifier_->setEnabled(true);\n> > > + if (watchdogDuration_)\n> > > +\n> > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > > + }\n> >\n> > I guess this could also be set outside of the if (queuedBuffer_.empty())\n> > - but it would artificially delay the watchdog every time a buffer was\n> > queued. In the event that more than one buffer is required to be queued\n> > (to satisfy internal requirements perhaps?) that might actually be\n> > beneficial ... But I think either way is fine.\n> >\n> \n> It could... I thought it might be ever so slightly more logically correct\n> if this\n> was inside the if() block. I'll keep it in there unless there are further\n> objections.\n> \n> \n> >\n> > >\n> > > queuedBuffers_[buf.index] = buffer;\n> > >\n> > > @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > return nullptr;\n> > > }\n> > >\n> > > - if (watchdogDuration_.count())\n> > > -\n> > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > > -\n> > > cache_->put(buf.index);\n> > >\n> > > FrameBuffer *buffer = it->second;\n> > > queuedBuffers_.erase(it);\n> > >\n> > > - if (queuedBuffers_.empty())\n> > > + if (queuedBuffers_.empty()) {\n> > > fdBufferNotifier_->setEnabled(false);\n> > > + watchdog_.stop();\n> > > + } else if (watchdogDuration_) {\n> > > + /*\n> > > + * Restart the watchdog timer if there are buffers still\n> > queued\n> > > + * in the device.\n> > > + */\n> > > +\n> > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> >\n> > Why do we need/have all these casts? Is either watchdogDuration_ not a\n> > duration? or is watchdog_.start() not accepting a duration? Either of\n> > those would remove a lot of casting surely?\n> >\n> \n> This is a bit of a minor annoyance because of how I defined the underlying\n> type\n> of utils::Duration to a double. The std::chrono library forbids implicit\n> casts\n> from integer to float types.\n> \n> \n> > watchdogDuration_ is a utils::Duration which stores std::nano, and I see\n> > Timer start takes a std::chrono::milliseconds. I think it would make\n> > sense to add a 'void start(std::chrono::duration)' to the timer class\n> > and simplify these casts. But that doesn't have to be fixed in this\n> > patch.\n> >\n> > Would you like to do that as a patch on top? If you don't want to let me\n> > know and I'll handle it after this patch is merged.\n> >\n> \n> What we've talked about doing is to switch utils::Duration to use an integer\n> underlying type, which would allow implicit conversions and remove the ugly\n> casts like the above. I believe Laurent was still thinking this through as\n> there\n> were also plans to make utils::Duration part of the public API...\n\nAyeee - I see - not as simple and obvious as I thought it should be\n(which explains why it isn't so ...)\n\nWell - I don't think it should affect this patch anyway.\n\n--\nKieran\n\n\n> \n> Regards,\n> Naush\n> \n> \n> >\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\n> > > + }\n> > >\n> > > buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR\n> > > ? FrameMetadata::FrameError\n> > > @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn()\n> > > }\n> > >\n> > > state_ = State::Streaming;\n> > > - if (watchdogDuration_.count())\n> > > + if (watchdogDuration_ && !queuedBuffers_.empty())\n> > >\n> > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > >\n> > > return 0;\n> > > @@ -1924,7 +1932,7 @@ void\n> > V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout)\n> > > watchdogDuration_ = timeout;\n> > >\n> > > watchdog_.stop();\n> > > - if (watchdogDuration_.count() && state_ == State::Streaming)\n> > > + if (watchdogDuration_ && state_ == State::Streaming &&\n> > !queuedBuffers_.empty())\n> > >\n> > watchdog_.start(std::chrono::duration_cast(timeout));\n> > > }\n> > >\n> > > --\n> > > 2.25.1\n> > >\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 4C5CCC3256\n\tfor ;\n\tThu, 5 May 2022 10:28:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A1ED560424;\n\tThu, 5 May 2022 12:28:08 +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 BA63F603AB\n\tfor ;\n\tThu, 5 May 2022 12:28:06 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3CA9A492;\n\tThu, 5 May 2022 12:28:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651746488;\n\tbh=oKecYEbtck0UGgP4B4OrxKL8bde7KxZxsJU3mvMK17w=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=viqQ+SKXKOYHkXDiguwmaWRB+HRmRwKzv/2SL7Ss/LXfp6pF1qdTmNoCJwG8HKo1t\n\tQ3O5q3T/CF06MrXnEAbupdSUrx5slsbitFGq6aBoDAnRmctqF56x5pmVVd9p+cSzbF\n\t19N7IrkUSBq/VJT+rPKmHw5rZGnXfVH70eoE5J+CjJZzUS2ZlkrcvefuqOFGe9Tejh\n\tEvTTr5lxj+5Lq+CvqhyaWkUsT/KBk7NmmssZOQxuyAZvJmkLxj7MkFmOrTQI2aQo4N\n\tqTkOmJWvcp7Oo8pnIVfTg3bTGGjyNUcakyxeYqkGUK+H1gnzsiiYwxCKeN/x/OoPyK\n\tDysmh55DqhFog==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1651746486;\n\tbh=oKecYEbtck0UGgP4B4OrxKL8bde7KxZxsJU3mvMK17w=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=FTO1Z0fBF5Nvc++2gAq/ROFvbvs/Er/uwqvCgQRtFKPxfIK2KVxvWQvz9pql1NV43\n\tLT5dq7dD/2bIk9HQNfOvVv3UYFGm7aacrQMg7sllH4CBR9ctovbqA2wvQbZ+p6lAfW\n\td7wHZesxlgVmDPzxpBrJ3oR+EKEAqHZE7wkXmPU8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FTO1Z0fB\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220505084824.4104296-1-naush@raspberrypi.com>\n\t<165174338949.1423360.2114054147343558842@Monstersaurus>\n\t","To":"Naushir Patuck ","Date":"Thu, 05 May 2022 11:28:03 +0100","Message-ID":"<165174648332.1423360.10796743332517265418@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22851,"web_url":"https://patchwork.libcamera.org/comment/22851/","msgid":"","date":"2022-05-05T10:42:47","subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for fixing this!\n\nOn Thu, 5 May 2022 at 09:48, Naushir Patuck via libcamera-devel\n wrote:\n>\n> Only enable/reset the watchdog timer when there are buffers queued in the V4L2\n> device. Otherwise, we may trigger spurious warnings when the watchdog times out\n> even if there are no buffers queued in the device.\n>\n> Signed-off-by: Naushir Patuck \n\nWorks for me, and on the Picamera2 auto test machine:\n\nTested-by: David Plowman \n\nThanks!\nDavid\n\n> ---\n> src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++-------\n> 1 file changed, 15 insertions(+), 7 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 5b4637b1a39e..430715afd554 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> return ret;\n> }\n>\n> - if (queuedBuffers_.empty())\n> + if (queuedBuffers_.empty()) {\n> fdBufferNotifier_->setEnabled(true);\n> + if (watchdogDuration_)\n> + watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> + }\n>\n> queuedBuffers_[buf.index] = buffer;\n>\n> @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> return nullptr;\n> }\n>\n> - if (watchdogDuration_.count())\n> - watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> -\n> cache_->put(buf.index);\n>\n> FrameBuffer *buffer = it->second;\n> queuedBuffers_.erase(it);\n>\n> - if (queuedBuffers_.empty())\n> + if (queuedBuffers_.empty()) {\n> fdBufferNotifier_->setEnabled(false);\n> + watchdog_.stop();\n> + } else if (watchdogDuration_) {\n> + /*\n> + * Restart the watchdog timer if there are buffers still queued\n> + * in the device.\n> + */\n> + watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> + }\n>\n> buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR\n> ? FrameMetadata::FrameError\n> @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn()\n> }\n>\n> state_ = State::Streaming;\n> - if (watchdogDuration_.count())\n> + if (watchdogDuration_ && !queuedBuffers_.empty())\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n>\n> return 0;\n> @@ -1924,7 +1932,7 @@ void V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout)\n> watchdogDuration_ = timeout;\n>\n> watchdog_.stop();\n> - if (watchdogDuration_.count() && state_ == State::Streaming)\n> + if (watchdogDuration_ && state_ == State::Streaming && !queuedBuffers_.empty())\n> watchdog_.start(std::chrono::duration_cast(timeout));\n> }\n>\n> --\n> 2.25.1\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 CA170C0F2A\n\tfor ;\n\tThu, 5 May 2022 10:42:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B62765645;\n\tThu, 5 May 2022 12:42:59 +0200 (CEST)","from mail-wm1-x332.google.com (mail-wm1-x332.google.com\n\t[IPv6:2a00:1450:4864:20::332])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BFE8260424\n\tfor ;\n\tThu, 5 May 2022 12:42:58 +0200 (CEST)","by mail-wm1-x332.google.com with SMTP id\n\tr1-20020a1c2b01000000b00394398c5d51so2382626wmr.2\n\tfor ;\n\tThu, 05 May 2022 03:42:58 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651747379;\n\tbh=kEC9hvZgOHi9VYbYuqSdl30rpGOgsQncLFk6ovUd9Ng=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=M8adqzRPfNnXjIKYDiM4tJm2i5cBGjnYUL3NEADGGdqJgl/fcEik1vVMbjhXGozqZ\n\tOXfkBr1DCv3NG2DAblg0YJLXmyVEmIWO6L0bDwGae4qwLB8ANhMElYB09OIJGXvt2l\n\tv1uGNi/LL6F6zUAVmzMVHyWFMhCLka6bng0Qw4BzDbaHsXENLwB4WX+Z+d6H+AcjuI\n\t+yWhQOYyFFd8kgLBVEfeqCNJadqylRHq8zSdLTe+3byf4rmn7FVPB41CoP7ECHwsKz\n\t5gNKU7YkhWNeMWGAFkVM1Hzu/9unAmgPnTFPSGC3RpdZPAhbEAXthNc0KuXLPZzsd0\n\tGYajdnIOBoC7g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=nJJFjiClVXkSUVt+tbG1uGywcxJP0EOuAW89K1PqkyU=;\n\tb=OZjOWIIaQmzqnDpTDQ8tOUV5v6i4EOQZU2rDgDr1ocjR0hgxnRZu3DQDSE8EFBMvmf\n\t+rgDQYBpaLvl6uqtYbBB5qdN4yHIHyh2rqBddhUN++AgmbV/3xxsJG/y1XQj/gU2pZzT\n\tbE39m9l6pziQ5m//jDA58aBZVVypN2SF5E84SwSus7+VDl59K3+9tfpAk+j37/Nt+eQa\n\tohc4zDDoxM9zJ1ght00/FbT8mJsPFlnpuaw7MPOGj2OyzSSlL4PtoKw0jBsxOU7HQfka\n\tL12SADFxhuoz/FEIZW8BzcwzWBgwd8mOIUoK1Rkjt2ooCEU/pw+TqNysc3g5QcNUW/Fh\n\tLdhQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"OZjOWIIa\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=nJJFjiClVXkSUVt+tbG1uGywcxJP0EOuAW89K1PqkyU=;\n\tb=fA80brh5aLsZOAsPHFjHh9IiyXl62sW8cv4wMGAgyTvOznBrkUMmt3psbJ9HO91kr/\n\t8p+f8dgXRgrwXpDN0yCeA9oGUXL64jEEZXLhaVtKZ1jA0z0d6YsYgSz3VvLN3R+jBGv0\n\tDD2OgQFnLO5nJKyduJ456rKCjaCDW3lSblM8o6f6a+Epntx/Y7iyjEHOHo7Ku48ceuCr\n\t6cbKDhI1lT/cv7Zb4jyQ+RuzGlZEj9dCdiT6+qDP7LbB/yg5B+1B493GT4OQB74Q6v1s\n\t4N3OWtm5gl8i0oKBkisGRdnAzl1r7HmbVawVGoZvNVeFBdBpah2XaWplkmpiU8jT2Poi\n\tEpPg==","X-Gm-Message-State":"AOAM530sQkbBLnWJtXMA5tyL7xaCgNLO5R/3+BinV8b3H8nuT5ReLnUb\n\t4PCqqOhwc+wxi3bGQQwtXOcLyMWCfW3ZMSbwr7fPHJTGhOsVPQ==","X-Google-Smtp-Source":"ABdhPJyDtRJEbNRk7VMmPWQjbcPVpcJXD57Vk667Ktum+q5H1GP3JM1Ja4pHAnoJCPXK8IB8/HiJO285jh6r2zPKlJ0=","X-Received":"by 2002:a05:600c:1c84:b0:394:5de0:245e with SMTP id\n\tk4-20020a05600c1c8400b003945de0245emr4137558wms.32.1651747378457;\n\tThu, 05 May 2022 03:42:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20220505084824.4104296-1-naush@raspberrypi.com>","In-Reply-To":"<20220505084824.4104296-1-naush@raspberrypi.com>","Date":"Thu, 5 May 2022 11:42:47 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22922,"web_url":"https://patchwork.libcamera.org/comment/22922/","msgid":"","date":"2022-05-10T07:58:39","subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi,\n\nOn Thu, 5 May 2022 at 09:48, Naushir Patuck wrote:\n\n> Only enable/reset the watchdog timer when there are buffers queued in the\n> V4L2\n> device. Otherwise, we may trigger spurious warnings when the watchdog\n> times out\n> even if there are no buffers queued in the device.\n>\n> Signed-off-by: Naushir Patuck \n>\n\nGentle ping. This needs one more R-B tag to get merged.\n\nRegards,\nNaush\n\n\n> ---\n> src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++-------\n> 1 file changed, 15 insertions(+), 7 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_videodevice.cpp\n> b/src/libcamera/v4l2_videodevice.cpp\n> index 5b4637b1a39e..430715afd554 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer\n> *buffer)\n> return ret;\n> }\n>\n> - if (queuedBuffers_.empty())\n> + if (queuedBuffers_.empty()) {\n> fdBufferNotifier_->setEnabled(true);\n> + if (watchdogDuration_)\n> +\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> + }\n>\n> queuedBuffers_[buf.index] = buffer;\n>\n> @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> return nullptr;\n> }\n>\n> - if (watchdogDuration_.count())\n> -\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> -\n> cache_->put(buf.index);\n>\n> FrameBuffer *buffer = it->second;\n> queuedBuffers_.erase(it);\n>\n> - if (queuedBuffers_.empty())\n> + if (queuedBuffers_.empty()) {\n> fdBufferNotifier_->setEnabled(false);\n> + watchdog_.stop();\n> + } else if (watchdogDuration_) {\n> + /*\n> + * Restart the watchdog timer if there are buffers still\n> queued\n> + * in the device.\n> + */\n> +\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> + }\n>\n> buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR\n> ? FrameMetadata::FrameError\n> @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn()\n> }\n>\n> state_ = State::Streaming;\n> - if (watchdogDuration_.count())\n> + if (watchdogDuration_ && !queuedBuffers_.empty())\n>\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n>\n> return 0;\n> @@ -1924,7 +1932,7 @@ void\n> V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout)\n> watchdogDuration_ = timeout;\n>\n> watchdog_.stop();\n> - if (watchdogDuration_.count() && state_ == State::Streaming)\n> + if (watchdogDuration_ && state_ == State::Streaming &&\n> !queuedBuffers_.empty())\n>\n> watchdog_.start(std::chrono::duration_cast(timeout));\n> }\n>\n> --\n> 2.25.1\n>\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 EAD18C3256\n\tfor ;\n\tTue, 10 May 2022 07:58:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3304A65646;\n\tTue, 10 May 2022 09:58:59 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA8D56563E\n\tfor ;\n\tTue, 10 May 2022 09:58:56 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id g16so19898153lja.3\n\tfor ;\n\tTue, 10 May 2022 00:58:56 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652169539;\n\tbh=Lyhnl8GHOJxM/GXbEgWIdEyLGXp1o5T4XiV3d0VmcRc=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=ei599a1A81+O8rZmvkXJyLWpb38SIYHSFQrssIXGWk+Dofl/eXqmt0gqAmaxZVa7R\n\tsJ9fK9U4roVTQldgv7s+sThrFAwgAF3JtP4Z++6eRJAXHesdCWVk+1F/svIP0wjv7A\n\tQwHCvIoSJce8TFPZo/9XNeNOZn7IwBV3M+Y4iJ/OiqHmlOB3JUJV0O49eovzmlN4CK\n\tqCuDEpMYbgEXmP0wVDqX2i9FfE8nI87D6ORIpPnCpzGcvmiLqEo3QYBUJJJYUSDvAB\n\tC9b0pcFM7SgAv6vjBECeHpHqpK8u/tWsgD+XjZFKiUmc2rGhkNeQkADPb8a0D+EdRs\n\twHNUyYaEQ401A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=TrcCKfyZ6JAIeyhRaVsOltoGNO8gnTXNiEI7qAFPxXo=;\n\tb=o7eP93u/JcRHUG2frAjV1C51DOvPkTtKNOuL1RSyv5G9skDhn4AZpAadraImMK7v+L\n\tgrivtX6cjhRWO+IxtDEnBdGQVHV42XkFmOEB4zUTenm8ure5hjBGvK5GGEiFfcYDLTK4\n\tZWjkYSJEkj2YxO6iHczmaadIgWUBnCui982Un37mZQ5/ifO4fkLfDingJEy6/joDwGYv\n\tnWDij4VwOj1ZCac73fndWakN1EZ503R73uxS4r1155qYVc+d+K/euccCZgXh+gp1mKo9\n\tdqgu8leUAHwiYTimopM0bL5jXWkZRSalkc7mY0URb3HLgSXH+euqmleWiOiDvFYDgOOA\n\t9uqw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"o7eP93u/\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=TrcCKfyZ6JAIeyhRaVsOltoGNO8gnTXNiEI7qAFPxXo=;\n\tb=H/kvkMl+PWYTtG/E1oynHlqW+ydllirJnzKbOE+gc8wzX8NarwF1Kk8QdU+ZYbegMj\n\t8sQIr0DMUJYChGQTIEeIVYTsqsFWxpudxRm9K7FVG3+0dk1cnk4CPZ9SWfdXKHIA9Tor\n\t3MnS4uC8iS15cSnlxeVKy7soNhZEpYv2/QXG+EQOQvcVuLqKDSDzNrKIQyxQ4PpGxSnR\n\tyveQk3TOuD4qhXtLQp9WgBC+gfjUqfJJUgGEGyq+gaDR9Rnrz227+9iyPAGRC15QTE15\n\tcl5mnK0EffFEYDssyFmFgwSScW4xdzGzDFQL94AdjCJdy/VKgHquszUdFMgztWStX7ql\n\tBqzQ==","X-Gm-Message-State":"AOAM531I0GtOL1AC5WtGe+D9GTlKHmw+HQUROuJoCwfpf66zHa0hoHSz\n\tAdSvEwUnic1XmrCB1c6oBUVh3lD1fVTaJ+T6sYr1GsoBh+I=","X-Google-Smtp-Source":"ABdhPJxYgj4YKVBZCOzCNSuBQB5WQ4HHf/tTlEL8zKhFJIUQg6xPGeesCycwa6HvPZe+kqVo9qQ1c0UcMXT30mqRC70=","X-Received":"by 2002:a2e:bf01:0:b0:247:dfe7:62dc with SMTP id\n\tc1-20020a2ebf01000000b00247dfe762dcmr13351418ljr.365.1652169535489;\n\tTue, 10 May 2022 00:58:55 -0700 (PDT)","MIME-Version":"1.0","References":"<20220505084824.4104296-1-naush@raspberrypi.com>","In-Reply-To":"<20220505084824.4104296-1-naush@raspberrypi.com>","Date":"Tue, 10 May 2022 08:58:39 +0100","Message-ID":"","To":"libcamera devel ","Content-Type":"multipart/alternative; boundary=\"000000000000459bd805dea3b20d\"","Subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22938,"web_url":"https://patchwork.libcamera.org/comment/22938/","msgid":"<165219076743.2416244.5564013526764560516@Monstersaurus>","date":"2022-05-10T13:52:47","subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-05-10 08:58:39)\n> Hi,\n> \n> On Thu, 5 May 2022 at 09:48, Naushir Patuck wrote:\n> \n> > Only enable/reset the watchdog timer when there are buffers queued in the\n> > V4L2\n> > device. Otherwise, we may trigger spurious warnings when the watchdog\n> > times out\n> > even if there are no buffers queued in the device.\n> >\n> > Signed-off-by: Naushir Patuck \n> >\n> \n> Gentle ping. This needs one more R-B tag to get merged.\n\nDavid, can we upgrade your Tested-by: to a Reviewed-by: ? (Or maybe TB\nis sufficient?)\n\n\n> \n> Regards,\n> Naush\n> \n> \n> > ---\n> > src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++-------\n> > 1 file changed, 15 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp\n> > b/src/libcamera/v4l2_videodevice.cpp\n> > index 5b4637b1a39e..430715afd554 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer\n> > *buffer)\n> > return ret;\n> > }\n> >\n> > - if (queuedBuffers_.empty())\n> > + if (queuedBuffers_.empty()) {\n> > fdBufferNotifier_->setEnabled(true);\n> > + if (watchdogDuration_)\n> > +\n> > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > + }\n> >\n> > queuedBuffers_[buf.index] = buffer;\n> >\n> > @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > return nullptr;\n> > }\n> >\n> > - if (watchdogDuration_.count())\n> > -\n> > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > -\n> > cache_->put(buf.index);\n> >\n> > FrameBuffer *buffer = it->second;\n> > queuedBuffers_.erase(it);\n> >\n> > - if (queuedBuffers_.empty())\n> > + if (queuedBuffers_.empty()) {\n> > fdBufferNotifier_->setEnabled(false);\n> > + watchdog_.stop();\n> > + } else if (watchdogDuration_) {\n> > + /*\n> > + * Restart the watchdog timer if there are buffers still\n> > queued\n> > + * in the device.\n> > + */\n> > +\n> > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > + }\n> >\n> > buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR\n> > ? FrameMetadata::FrameError\n> > @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn()\n> > }\n> >\n> > state_ = State::Streaming;\n> > - if (watchdogDuration_.count())\n> > + if (watchdogDuration_ && !queuedBuffers_.empty())\n> >\n> > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> >\n> > return 0;\n> > @@ -1924,7 +1932,7 @@ void\n> > V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout)\n> > watchdogDuration_ = timeout;\n> >\n> > watchdog_.stop();\n> > - if (watchdogDuration_.count() && state_ == State::Streaming)\n> > + if (watchdogDuration_ && state_ == State::Streaming &&\n> > !queuedBuffers_.empty())\n> >\n> > watchdog_.start(std::chrono::duration_cast(timeout));\n> > }\n> >\n> > --\n> > 2.25.1\n> >\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 70A0FC0F2A\n\tfor ;\n\tTue, 10 May 2022 13:52:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEF1365646;\n\tTue, 10 May 2022 15:52:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F44D65643\n\tfor ;\n\tTue, 10 May 2022 15:52:50 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D176E824;\n\tTue, 10 May 2022 15:52:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652190771;\n\tbh=Q9JldWKFl8rcWsYaR6HCk5LqqmSksNiwmAj9TIw6+CE=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=0PX1cbIUkd3+3xrrG7L18Z5levEhSMHAr56j2kTeufZKGLztp5lRkGn2G/nF7F2LM\n\twPqJ1VMMW24YHJzdrw9IPPWdE1i3QKVQZ7PhqP/vlQ2X2yFUji57yg4IhOFkyYBpeQ\n\tFGF/JDtaBBieNoHy7Qetb9IFsEKYG67TeHnEa0eNcwYWOglTbKL3CsyDKvdIHB5ws7\n\t0WRuA4iBb/nMSqDaWVhq0by3/EEW5SP+C8+YU9r2jNBNW5prQfw1YJW2NWWP9Dt+X4\n\tOd2Oxckg1aKDE22I6Zrb4QVDT97Hle5MReHa8ptzjxM1OrXBpBMk+lKiw75PITogym\n\tgr+nuzomtr9jg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652190769;\n\tbh=Q9JldWKFl8rcWsYaR6HCk5LqqmSksNiwmAj9TIw6+CE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=ijez6kCo98Z6+GqBFACKxXsnJYhifgVSctZ5cx1Ekm1sTfZy9jsFUv6MjFkdHRjiF\n\tD+wqBhP+d9xviRpQ3WrgtF0p7j+WPM1R6A/RsYD6C+wzTTMC9RyQRK/uaanaW2ZOdO\n\tXeicReNkfQlfRy0i2NFb+vHgDysfTIqdwW1/TKjQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ijez6kCo\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220505084824.4104296-1-naush@raspberrypi.com>\n\t","To":"Naushir Patuck ,\n\tlibcamera devel ,\n\tDavid Plowman , ","Date":"Tue, 10 May 2022 14:52:47 +0100","Message-ID":"<165219076743.2416244.5564013526764560516@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22946,"web_url":"https://patchwork.libcamera.org/comment/22946/","msgid":"","date":"2022-05-10T15:44:28","subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Thu, May 05, 2022 at 09:48:24AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Only enable/reset the watchdog timer when there are buffers queued in the V4L2\n> device. Otherwise, we may trigger spurious warnings when the watchdog times out\n> even if there are no buffers queued in the device.\n> \n> Signed-off-by: Naushir Patuck \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++-------\n> 1 file changed, 15 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 5b4637b1a39e..430715afd554 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> \t\treturn ret;\n> \t}\n> \n> -\tif (queuedBuffers_.empty())\n> +\tif (queuedBuffers_.empty()) {\n> \t\tfdBufferNotifier_->setEnabled(true);\n> +\t\tif (watchdogDuration_)\n> +\t\t\twatchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> +\t}\n> \n> \tqueuedBuffers_[buf.index] = buffer;\n> \n> @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \t\treturn nullptr;\n> \t}\n> \n> -\tif (watchdogDuration_.count())\n> -\t\twatchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> -\n> \tcache_->put(buf.index);\n> \n> \tFrameBuffer *buffer = it->second;\n> \tqueuedBuffers_.erase(it);\n> \n> -\tif (queuedBuffers_.empty())\n> +\tif (queuedBuffers_.empty()) {\n> \t\tfdBufferNotifier_->setEnabled(false);\n> +\t\twatchdog_.stop();\n> +\t} else if (watchdogDuration_) {\n> +\t\t/*\n> +\t\t * Restart the watchdog timer if there are buffers still queued\n> +\t\t * in the device.\n> +\t\t */\n> +\t\twatchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> +\t}\n> \n> \tbuffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR\n> \t\t\t\t ? FrameMetadata::FrameError\n> @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn()\n> \t}\n> \n> \tstate_ = State::Streaming;\n> -\tif (watchdogDuration_.count())\n> +\tif (watchdogDuration_ && !queuedBuffers_.empty())\n> \t\twatchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> \n> \treturn 0;\n> @@ -1924,7 +1932,7 @@ void V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout)\n> \twatchdogDuration_ = timeout;\n> \n> \twatchdog_.stop();\n> -\tif (watchdogDuration_.count() && state_ == State::Streaming)\n> +\tif (watchdogDuration_ && state_ == State::Streaming && !queuedBuffers_.empty())\n> \t\twatchdog_.start(std::chrono::duration_cast(timeout));\n> }\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 2B25CC0F2A\n\tfor ;\n\tTue, 10 May 2022 15:44:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C3FF65646;\n\tTue, 10 May 2022 17:44:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57A6665643\n\tfor ;\n\tTue, 10 May 2022 17:44:34 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BF55FBA9;\n\tTue, 10 May 2022 17:44:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652197476;\n\tbh=QONH8KKoUJyWmA+bCfr6Ok5hWGxNKL7VuZOiGDrXJho=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=HVqYN1W+ETc7Jsv3xkmY1lfNdSWdTYFbdrivWCaj7iX3h2TQrY0V94r8yZcZqgXg/\n\t4mlF711Sx0xyoKtGXESyQoCf2sSdrCLvcROLHxbYR8uRT5Zv2AT28p+2r9112w1bYG\n\t9HJeJE9K2BvJMwj2kF2qmO7l/Qj3oWIkpmED6WAyDdSxy5OH4g3c+JtSNmONRbkpL5\n\t5r+B8g5xrwpi9PzBG9kl1S35Rbs9C9Ht3prj7nECF/tveA1MtiXJbPIeAefssK8Iea\n\tZx3ysnGh6t2S80F98CZJnH3RUG3AdCWElj1+sGsTDIRMTFK9EJHyTFVEEPoBC8Q7T4\n\t5TbiL8pKiRYMQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652197474;\n\tbh=QONH8KKoUJyWmA+bCfr6Ok5hWGxNKL7VuZOiGDrXJho=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KEenJtZF0yJ4d0NcsJyE9jZJGnpJSa1iBkV0U62n4FEESAEh+tOIhra+yZsAL7CT8\n\thEYXCJ7KdxM6BND8i3xJluzFxLRCiQabOnj/0jK5e1sbe4YcvyQhnBUlztrC6d+Zl7\n\t8WjaKpQegZ0+PaZQGQIrB5k5LEXXyoh3XivWhevE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"KEenJtZF\"; dkim-atps=neutral","Date":"Tue, 10 May 2022 18:44:28 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20220505084824.4104296-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220505084824.4104296-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1] v4l2_videodevice: Disable the\n\twatchdog timer when no buffers are queued","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]