[{"id":3689,"web_url":"https://patchwork.libcamera.org/comment/3689/","msgid":"","date":"2020-02-12T11:47:42","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Fix V4L2\n\tbuffer cache entry availabity check","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi,\n\nWould anybody - hopefully Niklas :) - be able to do a quick review of\nthe below patch please? It does seem we need it (or similar\nalternative) for good performance on our platform.\n\nRegards,\nNaush\n\nOn Thu, 30 Jan 2020 at 15:43, wrote:\n>\n> From: Naushir Patuck \n>\n> The logic for keeping track of the cache holding dmabuf file descriptors\n> did not distinguish between slots that are occuiped but free and slots\n> that are not occupied. In devices using imported dmabufs this would\n> result in ping-ponging between the top two cache entries - and that in\n> turn would result in unnecessary cache missies.\n>\n> Fix this by tracking if an entry is occupied separately from if an entry\n> is free.\n>\n> Fixes: 9e71540ebbde (\"libcamera: v4l2_videodevice: Add V4L2BufferCache to deal with index mapping\")\n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/include/v4l2_videodevice.h | 1 +\n> src/libcamera/v4l2_videodevice.cpp | 6 +++---\n> 2 files changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index e4d35ab..a77b62e 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -125,6 +125,7 @@ private:\n> bool operator==(const FrameBuffer &buffer);\n>\n> bool free;\n> + bool occupied;\n>\n> private:\n> struct Plane {\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 37b8d33..b887a38 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -212,7 +212,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n> if (!entry.free)\n> continue;\n>\n> - if (use < 0)\n> + if (use < 0 && !entry.occupied)\n> use = index;\n>\n> /* Try to find a cache hit by comparing the planes. */\n> @@ -245,12 +245,12 @@ void V4L2BufferCache::put(unsigned int index)\n> }\n>\n> V4L2BufferCache::Entry::Entry()\n> - : free(true)\n> + : free(true), occupied(false)\n> {\n> }\n>\n> V4L2BufferCache::Entry::Entry(bool free, const FrameBuffer &buffer)\n> - : free(free)\n> + : free(free), occupied(true)\n> {\n> for (const FrameBuffer::Plane &plane : buffer.planes())\n> planes_.emplace_back(plane);\n> --\n> 2.17.1\n>","headers":{"Return-Path":"","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D942B6043B\n\tfor ;\n\tWed, 12 Feb 2020 12:47:58 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id r19so1962458ljg.3\n\tfor ;\n\tWed, 12 Feb 2020 03:47:58 -0800 (PST)"],"DKIM-Signature":"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=3WbfsyNpfb5b23YI8snR7GrSjfIf567bw8Mjra5Ghto=;\n\tb=I7PKSMZBUmEwUKoXg2hvGC4LfLgwlUd3mOxwYO4g5i3RiBbunzf6DoYEs+SWBveZe6\n\tS2bFTQ49+vb7uAZkp30Oak3l+nlsRZYf6GuyS57S5Zz2IgTl7KdxRdRzmlVLzcuN1zuQ\n\tWugWel56jlvEfo2NMolweUAPSzS80Xtnwgqi4Gj8lnvi0gEWA1G0f6RQRwAw/AMMEPqh\n\tFXQnpsrdoEep8j/I7FR/1eOS2Y2TZXN78I0zHhpVRw1NC7uIF+hpsHG04GLXucRL/3tZ\n\t8IgFxTCV4dueyEOd8FPa1FUjYan9KopWsuQ2DiJj4+uvPZXTmTG6i14t/qnQ/dP4YoM6\n\tywFg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=3WbfsyNpfb5b23YI8snR7GrSjfIf567bw8Mjra5Ghto=;\n\tb=HMvpRxi6ikWUfC4Bv7xEdOJjmbxOyLehLCIFeMJi0n4Gowr0TpkfvsiO3k/AtLs/mR\n\twlgdQ495LbG03Jk+E21EgSNRkL3MtFzzPo3CiAHSf8021WLrPM7pxZqOJ1i+qRgvxtcw\n\tz37TweBd2ezYgU02vOdYNBk6TURj4lhGQtt+uJuKxhIPLwcokmJ/e75wzPlTZoqXDiky\n\tZyFolrnNguyzTULcto+4QLxeWLsVzK5OSics7piJilV9ECNLx7gmZXHjtY8U+Bkl+MO7\n\t10GWrXHb9MGW3zw7LQ6/brLFFIZ1d54mZTfomJTUFkK9WaG8BVzy9XR6prgdGOASShgG\n\tcUJw==","X-Gm-Message-State":"APjAAAWEmpDS0yvU29xhnnVYiWnumL+v+AejYhFw3kTrVeG1FfeK/K1c\n\tCMcmpXe0gy8QlBS0rMvUNbgpxmTdeWy50/tum+i9noa6otO1rA==","X-Google-Smtp-Source":"APXvYqzNhV9o5hbe7qJvsgsgtCPwlp5CCx68XzWXf0oZ8kTK5aLy+cnnnvVKYSQibQ1iZYtdjfc9EipZ2kB3IMFFLnI=","X-Received":"by 2002:a2e:884c:: with SMTP id z12mr7667640ljj.55.1581508077701;\n\tWed, 12 Feb 2020 03:47:57 -0800 (PST)","MIME-Version":"1.0","References":"<20200130154137.10315-1-naush@raspberrypi.com>","In-Reply-To":"<20200130154137.10315-1-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Wed, 12 Feb 2020 11:47:42 +0000","Message-ID":"","To":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Fix V4L2\n\tbuffer cache entry availabity check","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","X-List-Received-Date":"Wed, 12 Feb 2020 11:47:59 -0000"}},{"id":3690,"web_url":"https://patchwork.libcamera.org/comment/3690/","msgid":"<20200212150729.GB3013231@oden.dyn.berto.se>","date":"2020-02-12T15:07:29","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Fix V4L2\n\tbuffer cache entry availabity check","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naushir,\n\nOn 2020-02-12 11:47:42 +0000, Naushir Patuck wrote:\n> Hi,\n> \n> Would anybody - hopefully Niklas :) - be able to do a quick review of\n> the below patch please? It does seem we need it (or similar\n> alternative) for good performance on our platform.\n\nI had a quick look and I agree this fix solves a real issue. However I'm \nconcerned it introduce another one. If more imported buffers are used \nthen there are entries in the cache there is no eviction of the oldest \noccupied entry not in use.\n\nThis is my fault as I have it on my todo list to write a proper test for \nthe V4L2BufferCache and address any faults like the one you address in \nthis change. If you don't beat me to it I hope to do so beginning next \nweek.\n\n> \n> Regards,\n> Naush\n> \n> On Thu, 30 Jan 2020 at 15:43, wrote:\n> >\n> > From: Naushir Patuck \n> >\n> > The logic for keeping track of the cache holding dmabuf file descriptors\n> > did not distinguish between slots that are occuiped but free and slots\n> > that are not occupied. In devices using imported dmabufs this would\n> > result in ping-ponging between the top two cache entries - and that in\n> > turn would result in unnecessary cache missies.\n> >\n> > Fix this by tracking if an entry is occupied separately from if an entry\n> > is free.\n> >\n> > Fixes: 9e71540ebbde (\"libcamera: v4l2_videodevice: Add V4L2BufferCache to deal with index mapping\")\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/include/v4l2_videodevice.h | 1 +\n> > src/libcamera/v4l2_videodevice.cpp | 6 +++---\n> > 2 files changed, 4 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> > index e4d35ab..a77b62e 100644\n> > --- a/src/libcamera/include/v4l2_videodevice.h\n> > +++ b/src/libcamera/include/v4l2_videodevice.h\n> > @@ -125,6 +125,7 @@ private:\n> > bool operator==(const FrameBuffer &buffer);\n> >\n> > bool free;\n> > + bool occupied;\n> >\n> > private:\n> > struct Plane {\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 37b8d33..b887a38 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -212,7 +212,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n> > if (!entry.free)\n> > continue;\n> >\n> > - if (use < 0)\n> > + if (use < 0 && !entry.occupied)\n> > use = index;\n> >\n> > /* Try to find a cache hit by comparing the planes. */\n> > @@ -245,12 +245,12 @@ void V4L2BufferCache::put(unsigned int index)\n> > }\n> >\n> > V4L2BufferCache::Entry::Entry()\n> > - : free(true)\n> > + : free(true), occupied(false)\n> > {\n> > }\n> >\n> > V4L2BufferCache::Entry::Entry(bool free, const FrameBuffer &buffer)\n> > - : free(free)\n> > + : free(free), occupied(true)\n> > {\n> > for (const FrameBuffer::Plane &plane : buffer.planes())\n> > planes_.emplace_back(plane);\n> > --\n> > 2.17.1\n> >\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AB9E6043B\n\tfor ;\n\tWed, 12 Feb 2020 16:07:32 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id x7so2724740ljc.1\n\tfor ;\n\tWed, 12 Feb 2020 07:07:32 -0800 (PST)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\td25sm457624ljj.51.2020.02.12.07.07.29\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 12 Feb 2020 07:07:29 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=cXuCz8AA+4R3ovRCGzNS+HNe1B8lKK20h8HHfVWMUeA=;\n\tb=gOUd3T/Cd2BrNDNgb5WZ5JnEj6izycnLrcxbpG4ZeASibROwqpcU9AkWqklteJgLkp\n\tvTe3XX/KG0R7h9UYMvNsEtNVMEaPicY24Pla6O3yaqan1rxawlAM7zcQ2SKcSWNdM6t6\n\tHNLe/hSfH9Meva7x19io7laCt9O0MgoR9edhTwgzbSNbohCNbJ1tCxCS9xo5TsAPWPv0\n\t1qZM1AhoYu3a9YN298qDxUADbdMkdl325iKvxCC9Wtpuisn9T6+at8NrdpZAsKsJO4Tb\n\td/3cbtLotFGvOh2IbZuM9c5PPy5pY4fXOJhmOZvP9M7NEE5ZIr0AYhcMihWIkwAw78ak\n\tX4mw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=cXuCz8AA+4R3ovRCGzNS+HNe1B8lKK20h8HHfVWMUeA=;\n\tb=JrnCXTk3lX2664AVomY/tAkgwJcDdqcUjnHvFILwc1wcZfsqUXYLzBWR1XXz6J2crM\n\tK9POuvED8N8vyM1Ca264wiTfaqhhGJEn5OQeh+nbGI8mLxGRe83M2ZX1n1exHBujLHj4\n\tveu1Y/IAUR6I1WlrKnj3ZV2otjfuE4cpSD2EFGqSto/k1XN2umVNN/M5lXAl0XGb1ES7\n\tmdsUjic7vPBoqVRKbXcHd4CRy1LGGSUv1RgvBbU1CIdAB6Xm4Gbd8tL0ZEebkYiv3UgS\n\tGIIJ/UzI85FiIzSdcQnRHoqq6yi9G5amA4GCL/kUpAIVS3zMnRH6be4zqamT3w5lV6x+\n\tX0aQ==","X-Gm-Message-State":"APjAAAXrINWc74EIpvDuI8aB7OPr2RhAfNlp0XiKmtlHeAVVSe1GnZTo\n\t6UhETuj1EjTS7Fi1FlqsKMhbhA==","X-Google-Smtp-Source":"APXvYqz5UhH0Tad7JAq1GxugUnr6FI6UB5xlJT0QeeDQkt1aJxafp2s8YU7VgOtn+y7bE5CsGs/b3Q==","X-Received":"by 2002:a2e:b8d0:: with SMTP id s16mr7507514ljp.32.1581520051106;\n\tWed, 12 Feb 2020 07:07:31 -0800 (PST)","Date":"Wed, 12 Feb 2020 16:07:29 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200212150729.GB3013231@oden.dyn.berto.se>","References":"<20200130154137.10315-1-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Fix V4L2\n\tbuffer cache entry availabity check","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","X-List-Received-Date":"Wed, 12 Feb 2020 15:07:32 -0000"}},{"id":3691,"web_url":"https://patchwork.libcamera.org/comment/3691/","msgid":"","date":"2020-02-12T15:43:42","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Fix V4L2\n\tbuffer cache entry availabity check","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Niklas,\n\nOn Wed, 12 Feb 2020 at 15:07, Niklas Söderlund\n wrote:\n>\n> Hi Naushir,\n>\n> On 2020-02-12 11:47:42 +0000, Naushir Patuck wrote:\n> > Hi,\n> >\n> > Would anybody - hopefully Niklas :) - be able to do a quick review of\n> > the below patch please? It does seem we need it (or similar\n> > alternative) for good performance on our platform.\n>\n> I had a quick look and I agree this fix solves a real issue. However I'm\n> concerned it introduce another one. If more imported buffers are used\n> then there are entries in the cache there is no eviction of the oldest\n> occupied entry not in use.\n>\n\nYes, this was my thoughts on the fix as well. However, I noticed that\nthe cache size is always initialised to the number of buffers\nallocated, so we would always have the same number of entries as\nbuffers - maybe my understanding is wrong?\n\n\n\n> This is my fault as I have it on my todo list to write a proper test for\n> the V4L2BufferCache and address any faults like the one you address in\n> this change. If you don't beat me to it I hope to do so beginning next\n> week.\n>\n\nNot a problem. I am running locally with the patch below, so this is\nnot impacting me in any way.\n\n> >\n> > Regards,\n> > Naush\n> >\n> > On Thu, 30 Jan 2020 at 15:43, wrote:\n> > >\n> > > From: Naushir Patuck \n> > >\n> > > The logic for keeping track of the cache holding dmabuf file descriptors\n> > > did not distinguish between slots that are occuiped but free and slots\n> > > that are not occupied. In devices using imported dmabufs this would\n> > > result in ping-ponging between the top two cache entries - and that in\n> > > turn would result in unnecessary cache missies.\n> > >\n> > > Fix this by tracking if an entry is occupied separately from if an entry\n> > > is free.\n> > >\n> > > Fixes: 9e71540ebbde (\"libcamera: v4l2_videodevice: Add V4L2BufferCache to deal with index mapping\")\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/libcamera/include/v4l2_videodevice.h | 1 +\n> > > src/libcamera/v4l2_videodevice.cpp | 6 +++---\n> > > 2 files changed, 4 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> > > index e4d35ab..a77b62e 100644\n> > > --- a/src/libcamera/include/v4l2_videodevice.h\n> > > +++ b/src/libcamera/include/v4l2_videodevice.h\n> > > @@ -125,6 +125,7 @@ private:\n> > > bool operator==(const FrameBuffer &buffer);\n> > >\n> > > bool free;\n> > > + bool occupied;\n> > >\n> > > private:\n> > > struct Plane {\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 37b8d33..b887a38 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -212,7 +212,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n> > > if (!entry.free)\n> > > continue;\n> > >\n> > > - if (use < 0)\n> > > + if (use < 0 && !entry.occupied)\n> > > use = index;\n> > >\n> > > /* Try to find a cache hit by comparing the planes. */\n> > > @@ -245,12 +245,12 @@ void V4L2BufferCache::put(unsigned int index)\n> > > }\n> > >\n> > > V4L2BufferCache::Entry::Entry()\n> > > - : free(true)\n> > > + : free(true), occupied(false)\n> > > {\n> > > }\n> > >\n> > > V4L2BufferCache::Entry::Entry(bool free, const FrameBuffer &buffer)\n> > > - : free(free)\n> > > + : free(free), occupied(true)\n> > > {\n> > > for (const FrameBuffer::Plane &plane : buffer.planes())\n> > > planes_.emplace_back(plane);\n> > > --\n> > > 2.17.1\n> > >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\n\nBest regards,\nNaush","headers":{"Return-Path":"","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 770936043B\n\tfor ;\n\tWed, 12 Feb 2020 16:43:58 +0100 (CET)","by mail-lf1-x142.google.com with SMTP id z26so1902837lfg.13\n\tfor ;\n\tWed, 12 Feb 2020 07:43:58 -0800 (PST)"],"DKIM-Signature":"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:content-transfer-encoding;\n\tbh=jMghYy3q95m8l/ZMPcfkrw8WXYb10Hcd/UnB2XZxVbg=;\n\tb=PaZQvJkg7x5+E71fuZEdAQhmALoo1HdY58xPbOAVQWfUkc2rHNUBBbc8bndEGJamTd\n\t6C4vMoO/wFsYtMjFrU4sQz9KDh9z+b3N13IXYOtrlrlm8ji4lzmjx3lahzbKAnKhXnog\n\t/jNMstErvJor31Y1+GYhw6t1iGv0TvUtByoPJf5hx5VVRYNYJP9sws4rd/rXAo1uVzib\n\tX8u7LXKoAiXqzSjn64TLAXuwKE7rhayKkf5wEnj7b9ddPMY7UEzmk7wnZIb5y2WvF25j\n\tYckB0VTzHsNoyNbcLLzpwM3+OTVUlG34Yymxvs66ALPsWKunPlcBqeR/MotZ5xfpSiOH\n\tdjcg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=jMghYy3q95m8l/ZMPcfkrw8WXYb10Hcd/UnB2XZxVbg=;\n\tb=VBZvL4WuF32oxOoYY17B3nvSTD1ZsffqzgOBxkLMberm6ny97cRHFoikvznIBB9UJW\n\teNl32x8X4fF4m8TU66C7m08eSTkHSlQOSUVaWzHzBu4XgVHRBF2wzu+r+xAOZsS9Ldtr\n\tISI5amh30b162Un8bo39HUAu7F+mXteN67xXEkJen5jt0E0kH2xu4DGP9uS9FZegm/2/\n\tGPMqNtoaa422B65Cp9FAkeqxv6NqWkQr017NV6W7/bn8tln8JUMtzv+n7akC9XI9eEzg\n\taxw6MPUSoo2kphczkyaO8iC6lbIGusXtT6VLcU72UphvXo4DE83HII6NZKcVWowj0Vb4\n\tEgEA==","X-Gm-Message-State":"APjAAAWYf96y+RSthZ4kYD7wzKW38fW6VsneECZivt45HUNAjABoNBep\n\txILL/dSiAIrfCQY1gyICbGeJdxQ64ILpPeS1w+dbhojqZtY=","X-Google-Smtp-Source":"APXvYqyOZEaqFpuwi0VGpq9d/EeR/QoYF8GFhrR+X9IL+8Dewy1xyn6V4pkJiaDjXimSWXScSpqkYI/Lmr9wLtVx+9Y=","X-Received":"by 2002:a19:4a92:: with SMTP id\n\tx140mr7126582lfa.29.1581522237689; \n\tWed, 12 Feb 2020 07:43:57 -0800 (PST)","MIME-Version":"1.0","References":"<20200130154137.10315-1-naush@raspberrypi.com>\n\t\n\t<20200212150729.GB3013231@oden.dyn.berto.se>","In-Reply-To":"<20200212150729.GB3013231@oden.dyn.berto.se>","From":"Naushir Patuck ","Date":"Wed, 12 Feb 2020 15:43:42 +0000","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Fix V4L2\n\tbuffer cache entry availabity check","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","X-List-Received-Date":"Wed, 12 Feb 2020 15:43:58 -0000"}},{"id":3769,"web_url":"https://patchwork.libcamera.org/comment/3769/","msgid":"<20200216162202.GM3013231@oden.dyn.berto.se>","date":"2020-02-16T16:22:02","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Fix V4L2\n\tbuffer cache entry availabity check","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naushir,\n\nOn 2020-02-12 15:43:42 +0000, Naushir Patuck wrote:\n> Hi Niklas,\n> \n> On Wed, 12 Feb 2020 at 15:07, Niklas Söderlund\n> wrote:\n> >\n> > Hi Naushir,\n> >\n> > On 2020-02-12 11:47:42 +0000, Naushir Patuck wrote:\n> > > Hi,\n> > >\n> > > Would anybody - hopefully Niklas :) - be able to do a quick review of\n> > > the below patch please? It does seem we need it (or similar\n> > > alternative) for good performance on our platform.\n> >\n> > I had a quick look and I agree this fix solves a real issue. However I'm\n> > concerned it introduce another one. If more imported buffers are used\n> > then there are entries in the cache there is no eviction of the oldest\n> > occupied entry not in use.\n> >\n> \n> Yes, this was my thoughts on the fix as well. However, I noticed that\n> the cache size is always initialised to the number of buffers\n> allocated, so we would always have the same number of entries as\n> buffers - maybe my understanding is wrong?\n\nThat is mostly correct, in the Android HAL code we operate using \nexternally provided buffers and we do not know how many they will use \nand if it will reflect the number provided at stream configuration time.\n\nI just sent out a patch that I hope will solve your issues as well as \naddress the Android HAL needs [1]. It would be super if you could test \nit and verify it solves your problem.\n\n1. [PATCH 0/2] libcamera: V4L2BufferCache: Improve cache eviction \n strategy\n\n> \n> \n> \n> > This is my fault as I have it on my todo list to write a proper test for\n> > the V4L2BufferCache and address any faults like the one you address in\n> > this change. If you don't beat me to it I hope to do so beginning next\n> > week.\n> >\n> \n> Not a problem. I am running locally with the patch below, so this is\n> not impacting me in any way.\n> \n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > > On Thu, 30 Jan 2020 at 15:43, wrote:\n> > > >\n> > > > From: Naushir Patuck \n> > > >\n> > > > The logic for keeping track of the cache holding dmabuf file descriptors\n> > > > did not distinguish between slots that are occuiped but free and slots\n> > > > that are not occupied. In devices using imported dmabufs this would\n> > > > result in ping-ponging between the top two cache entries - and that in\n> > > > turn would result in unnecessary cache missies.\n> > > >\n> > > > Fix this by tracking if an entry is occupied separately from if an entry\n> > > > is free.\n> > > >\n> > > > Fixes: 9e71540ebbde (\"libcamera: v4l2_videodevice: Add V4L2BufferCache to deal with index mapping\")\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > src/libcamera/include/v4l2_videodevice.h | 1 +\n> > > > src/libcamera/v4l2_videodevice.cpp | 6 +++---\n> > > > 2 files changed, 4 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> > > > index e4d35ab..a77b62e 100644\n> > > > --- a/src/libcamera/include/v4l2_videodevice.h\n> > > > +++ b/src/libcamera/include/v4l2_videodevice.h\n> > > > @@ -125,6 +125,7 @@ private:\n> > > > bool operator==(const FrameBuffer &buffer);\n> > > >\n> > > > bool free;\n> > > > + bool occupied;\n> > > >\n> > > > private:\n> > > > struct Plane {\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index 37b8d33..b887a38 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -212,7 +212,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n> > > > if (!entry.free)\n> > > > continue;\n> > > >\n> > > > - if (use < 0)\n> > > > + if (use < 0 && !entry.occupied)\n> > > > use = index;\n> > > >\n> > > > /* Try to find a cache hit by comparing the planes. */\n> > > > @@ -245,12 +245,12 @@ void V4L2BufferCache::put(unsigned int index)\n> > > > }\n> > > >\n> > > > V4L2BufferCache::Entry::Entry()\n> > > > - : free(true)\n> > > > + : free(true), occupied(false)\n> > > > {\n> > > > }\n> > > >\n> > > > V4L2BufferCache::Entry::Entry(bool free, const FrameBuffer &buffer)\n> > > > - : free(free)\n> > > > + : free(free), occupied(true)\n> > > > {\n> > > > for (const FrameBuffer::Plane &plane : buffer.planes())\n> > > > planes_.emplace_back(plane);\n> > > > --\n> > > > 2.17.1\n> > > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> \n> Best regards,\n> Naush","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 096A760438\n\tfor ;\n\tSun, 16 Feb 2020 17:22:05 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id v17so16041024ljg.4\n\tfor ;\n\tSun, 16 Feb 2020 08:22:04 -0800 (PST)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tn3sm6944477ljc.100.2020.02.16.08.22.02\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 16 Feb 2020 08:22:02 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=hJcmQObQyWaqBGEOczysXYmZgb3+LUnbwB9XrmKBMVs=;\n\tb=zX14V3qaku2THGVlswFDadtpdIPJmuwS+BOdCElrg2cG480UyxXxjFStM3+qobysnm\n\tWAF2VpjtaEpCfbzoHaJBn9VfQSPN1yWFwHprzyfDUkzjP0Dk3jz59hMoBBwaHf5T0HAG\n\tnXZV6qoIr9mfbzFBA1gPy2DnBwgNWV62WplzsR1Sr3557A7Fzo3An8OJU1m3zhtEvRoq\n\tTOUafSihCDBfIJ8/Or/UGor4KA5itLDO46bSl8RzxpXgyMNFuvHLbD/r5Kh4VET95tQG\n\trDu/2gMIUBwOCKohzvkawZ4R9M32nOgQr36N4+UnURtJthYA/tAP6tqBnI6zQwMKY99t\n\ttwkg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=hJcmQObQyWaqBGEOczysXYmZgb3+LUnbwB9XrmKBMVs=;\n\tb=Yw33qztwWzOn/ryFDjXVZPZSHzhrIdT6v3w8gvQ9OSLxFkrGDVRFvjOxA3LaAshkp7\n\tSp6BklZRxzwgzEWcjTQun/5rkytud8uFOr0lddPygHRZmVo00lnOAMtux8+ucdQk337S\n\tin+U5h/RNS6dX7d7/3bwU3kwW0n5UP0Z8EmueYq/ekt74GSSJnqoniIAJyx58hr093Np\n\tQ+zTOm1Nr4tgL10UNdKxXADON8Dk+I9IaFlTVWuI4K8qwwKMXNQQo8hPRYeg6pAlcmro\n\tzH7xs+qFyDXTgLmj5986Nq0OxRe22ard4GPAHL1/O1Ny++WCXPCZsX1jJAXAIi3jL98n\n\tJiFA==","X-Gm-Message-State":"APjAAAUgi+Ut/BtHOfPOAgVPaaYLlHy+qkVTqabqHbF4Yr77sAyVu5Dy\n\tq6yBiqKDJmsG6cOQ4VeYkwhbyzcEGEI=","X-Google-Smtp-Source":"APXvYqxO7lHc9M3uf8XxzgDfEBTWnnHn43VHpYDLwZoBJhtnULRaWvfJIPgrbIOI7jFnDkt7/kkuJw==","X-Received":"by 2002:a2e:86d6:: with SMTP id n22mr7071765ljj.77.1581870123282;\n\tSun, 16 Feb 2020 08:22:03 -0800 (PST)","Date":"Sun, 16 Feb 2020 17:22:02 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200216162202.GM3013231@oden.dyn.berto.se>","References":"<20200130154137.10315-1-naush@raspberrypi.com>\n\t\n\t<20200212150729.GB3013231@oden.dyn.berto.se>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Fix V4L2\n\tbuffer cache entry availabity check","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","X-List-Received-Date":"Sun, 16 Feb 2020 16:22:05 -0000"}}]