[{"id":1583,"web_url":"https://patchwork.libcamera.org/comment/1583/","msgid":"<20190511125248.GB4946@pendragon.ideasonboard.com>","date":"2019-05-11T12:52:48","subject":"Re: [libcamera-devel] [PATCH v3 01/11] libcamera: Always check\n\treturn value of MediaDevice::acquire()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sat, May 11, 2019 at 11:18:57AM +0200, Niklas Söderlund wrote:\n> In preparation for adding more responsibility to MediaDevice::acquire()\n> remove unneeded calls to acquire() and release(), and make sure all\n> needed calls to acquire() are checked and acted on.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 ++++------\n> src/libcamera/pipeline/uvcvideo.cpp | 3 ++-\n> src/libcamera/pipeline/vimc.cpp | 3 ++-\n> test/media_device/media_device_link_test.cpp | 6 +++++-\n> test/v4l2_device/v4l2_device_test.cpp | 4 ----\n> test/v4l2_subdevice/v4l2_subdevice_test.cpp | 7 -------\n> 6 files changed, 13 insertions(+), 20 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 0041662514e1d7ca..75e878afad4e67a9 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -614,21 +614,19 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> \timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n> \timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n> \n> -\t/*\n> -\t * It is safe to acquire both media devices at this point as\n> -\t * DeviceEnumerator::search() skips the busy ones for us.\n> -\t */\n> \tcio2MediaDev_ = enumerator->search(cio2_dm);\n> \tif (!cio2MediaDev_)\n> \t\treturn false;\n> \n> -\tcio2MediaDev_->acquire();\n> +\tif (!cio2MediaDev_->acquire())\n> +\t\treturn false;\n> \n> \timguMediaDev_ = enumerator->search(imgu_dm);\n> \tif (!imguMediaDev_)\n> \t\treturn false;\n> \n> -\timguMediaDev_->acquire();\n> +\tif (!imguMediaDev_->acquire())\n> +\t\treturn false;\n> \n> \t/*\n> \t * Disable all links that are enabled by default on CIO2, as camera\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 5d2f1c98fa36380c..e40b052f4d877d5d 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -183,7 +183,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> \tif (!media_)\n> \t\treturn false;\n> \n> -\tmedia_->acquire();\n> +\tif (!media_->acquire())\n> +\t\treturn false;\n> \n> \tstd::unique_ptr data = utils::make_unique(this);\n> \n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index cdf43770a9bfc40f..7b6ebd4cc0a27e25 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -193,7 +193,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> \tif (!media_)\n> \t\treturn false;\n> \n> -\tmedia_->acquire();\n> +\tif (!media_->acquire())\n> +\t\treturn false;\n> \n> \tstd::unique_ptr data = utils::make_unique(this);\n> \n> diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp\n> index 58a55cdfee634c0b..484d3691310f78a0 100644\n> --- a/test/media_device/media_device_link_test.cpp\n> +++ b/test/media_device/media_device_link_test.cpp\n> @@ -51,7 +51,11 @@ class MediaDeviceLinkTest : public Test\n> \t\t\treturn TestSkip;\n> \t\t}\n> \n> -\t\tdev_->acquire();\n> +\t\tif (!dev_->acquire()) {\n> +\t\t\tcerr << \"Unable to acquire media device \"\n> +\t\t\t << dev_->deviceNode() << endl;\n> +\t\t\treturn TestSkip;\n> +\t\t}\n> \n> \t\tif (dev_->open()) {\n> \t\t\tcerr << \"Failed to open media device at \"\n> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp\n> index 4225291bbb6e23f0..833038d56ea4631d 100644\n> --- a/test/v4l2_device/v4l2_device_test.cpp\n> +++ b/test/v4l2_device/v4l2_device_test.cpp\n> @@ -46,8 +46,6 @@ int V4L2DeviceTest::init()\n> \tif (!media_)\n> \t\treturn TestSkip;\n> \n> -\tmedia_->acquire();\n> -\n\nWhy is this call and the ones below unneeded ? Isn't it a good practice\nto always acquire the device before operating on it ? I suppose it's not\nstrictly mandatory here as we only inspect the media device to find\nviden and subdev nodes, but is there any drawback in keeping these calls\n?\n\n> \tMediaEntity *entity = media_->getEntityByName(\"vivid-000-vid-cap\");\n> \tif (!entity)\n> \t\treturn TestSkip;\n> @@ -61,8 +59,6 @@ int V4L2DeviceTest::init()\n> \n> void V4L2DeviceTest::cleanup()\n> {\n> -\tmedia_->release();\n> -\n> \tcapture_->streamOff();\n> \tcapture_->releaseBuffers();\n> \tcapture_->close();\n> diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> index 72d5f72543d29378..5810ef3c962fab8e 100644\n> --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> @@ -45,20 +45,16 @@ int V4L2SubdeviceTest::init()\n> \t\treturn TestSkip;\n> \t}\n> \n> -\tmedia_->acquire();\n> -\n> \tint ret = media_->open();\n> \tif (ret) {\n> \t\tcerr << \"Unable to open media device: \" << media_->deviceNode()\n> \t\t << \": \" << strerror(ret) << endl;\n> -\t\tmedia_->release();\n> \t\treturn TestSkip;\n> \t}\n> \n> \tMediaEntity *videoEntity = media_->getEntityByName(\"Scaler\");\n> \tif (!videoEntity) {\n> \t\tcerr << \"Unable to find media entity 'Scaler'\" << endl;\n> -\t\tmedia_->release();\n> \t\treturn TestFail;\n> \t}\n> \n> @@ -67,7 +63,6 @@ int V4L2SubdeviceTest::init()\n> \tif (ret) {\n> \t\tcerr << \"Unable to open video subdevice \"\n> \t\t << scaler_->entity()->deviceNode() << endl;\n> -\t\tmedia_->release();\n> \t\treturn TestSkip;\n> \t}\n> \n> @@ -76,7 +71,5 @@ int V4L2SubdeviceTest::init()\n> \n> void V4L2SubdeviceTest::cleanup()\n> {\n> -\tmedia_->release();\n> -\n> \tdelete scaler_;\n> }","headers":{"Return-Path":"","Received":["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 A2E9360E4F\n\tfor ;\n\tSat, 11 May 2019 14:53:04 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 17C15D5;\n\tSat, 11 May 2019 14:53:04 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557579184;\n\tbh=JmrshGNSRSMKnvpKeeOqm3hRVUAGS5s8TQCHWD5kziw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HsknJONCb65uPbtAwQqkahEf7fnzrbMBEqW6/kxeiuxxEuVNnfq/Wbpxrn/5fh4ku\n\td9UjTrlpNjz5Yk4WiTd/XTk6aQLZ9CNKQT91+Pm7mI1uDZYR2zFKrpJRPL3at1Y+sZ\n\t5pU0DVAJm4ehaaeN+wGfF5sXc+f3beXv3NMMV12o=","Date":"Sat, 11 May 2019 15:52:48 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190511125248.GB4946@pendragon.ideasonboard.com>","References":"<20190511091907.10050-1-niklas.soderlund@ragnatech.se>\n\t<20190511091907.10050-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190511091907.10050-2-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 01/11] libcamera: Always check\n\treturn value of MediaDevice::acquire()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 11 May 2019 12:53:04 -0000"}},{"id":1589,"web_url":"https://patchwork.libcamera.org/comment/1589/","msgid":"<20190511132228.GL28561@bigcity.dyn.berto.se>","date":"2019-05-11T13:22:28","subject":"Re: [libcamera-devel] [PATCH v3 01/11] libcamera: Always check\n\treturn value of MediaDevice::acquire()","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2019-05-11 15:52:48 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sat, May 11, 2019 at 11:18:57AM +0200, Niklas Söderlund wrote:\n> > In preparation for adding more responsibility to MediaDevice::acquire()\n> > remove unneeded calls to acquire() and release(), and make sure all\n> > needed calls to acquire() are checked and acted on.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 ++++------\n> > src/libcamera/pipeline/uvcvideo.cpp | 3 ++-\n> > src/libcamera/pipeline/vimc.cpp | 3 ++-\n> > test/media_device/media_device_link_test.cpp | 6 +++++-\n> > test/v4l2_device/v4l2_device_test.cpp | 4 ----\n> > test/v4l2_subdevice/v4l2_subdevice_test.cpp | 7 -------\n> > 6 files changed, 13 insertions(+), 20 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 0041662514e1d7ca..75e878afad4e67a9 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -614,21 +614,19 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > \timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n> > \timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n> > \n> > -\t/*\n> > -\t * It is safe to acquire both media devices at this point as\n> > -\t * DeviceEnumerator::search() skips the busy ones for us.\n> > -\t */\n> > \tcio2MediaDev_ = enumerator->search(cio2_dm);\n> > \tif (!cio2MediaDev_)\n> > \t\treturn false;\n> > \n> > -\tcio2MediaDev_->acquire();\n> > +\tif (!cio2MediaDev_->acquire())\n> > +\t\treturn false;\n> > \n> > \timguMediaDev_ = enumerator->search(imgu_dm);\n> > \tif (!imguMediaDev_)\n> > \t\treturn false;\n> > \n> > -\timguMediaDev_->acquire();\n> > +\tif (!imguMediaDev_->acquire())\n> > +\t\treturn false;\n> > \n> > \t/*\n> > \t * Disable all links that are enabled by default on CIO2, as camera\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 5d2f1c98fa36380c..e40b052f4d877d5d 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -183,7 +183,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> > \tif (!media_)\n> > \t\treturn false;\n> > \n> > -\tmedia_->acquire();\n> > +\tif (!media_->acquire())\n> > +\t\treturn false;\n> > \n> > \tstd::unique_ptr data = utils::make_unique(this);\n> > \n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index cdf43770a9bfc40f..7b6ebd4cc0a27e25 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -193,7 +193,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> > \tif (!media_)\n> > \t\treturn false;\n> > \n> > -\tmedia_->acquire();\n> > +\tif (!media_->acquire())\n> > +\t\treturn false;\n> > \n> > \tstd::unique_ptr data = utils::make_unique(this);\n> > \n> > diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp\n> > index 58a55cdfee634c0b..484d3691310f78a0 100644\n> > --- a/test/media_device/media_device_link_test.cpp\n> > +++ b/test/media_device/media_device_link_test.cpp\n> > @@ -51,7 +51,11 @@ class MediaDeviceLinkTest : public Test\n> > \t\t\treturn TestSkip;\n> > \t\t}\n> > \n> > -\t\tdev_->acquire();\n> > +\t\tif (!dev_->acquire()) {\n> > +\t\t\tcerr << \"Unable to acquire media device \"\n> > +\t\t\t << dev_->deviceNode() << endl;\n> > +\t\t\treturn TestSkip;\n> > +\t\t}\n> > \n> > \t\tif (dev_->open()) {\n> > \t\t\tcerr << \"Failed to open media device at \"\n> > diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp\n> > index 4225291bbb6e23f0..833038d56ea4631d 100644\n> > --- a/test/v4l2_device/v4l2_device_test.cpp\n> > +++ b/test/v4l2_device/v4l2_device_test.cpp\n> > @@ -46,8 +46,6 @@ int V4L2DeviceTest::init()\n> > \tif (!media_)\n> > \t\treturn TestSkip;\n> > \n> > -\tmedia_->acquire();\n> > -\n> \n> Why is this call and the ones below unneeded ? Isn't it a good practice\n> to always acquire the device before operating on it ? I suppose it's not\n> strictly mandatory here as we only inspect the media device to find\n> viden and subdev nodes, but is there any drawback in keeping these calls\n> ?\n\nIt's a good idea to acquire() the device if you intend to operate on it, \nat this point in time the init() do not need to operate on the media \ndevice so there is no need to acquire() it. Later in the series I do add \na call to acquire() and release() to this base init() function to be \nable to perform an operation on the device (reset the media links).\n\nBut I still think we should leave it unacquired and let each test-case \ncall acquire() if it needs to operate on the media device. Most tests \nwho inherit from this base would not need to modify the graph as it \nintends to test the v4l2 device and for those who do I think it's nice \nto show that clearly in the test case and not hide the acquire() in the \nbase.\n\n> \n> > \tMediaEntity *entity = media_->getEntityByName(\"vivid-000-vid-cap\");\n> > \tif (!entity)\n> > \t\treturn TestSkip;\n> > @@ -61,8 +59,6 @@ int V4L2DeviceTest::init()\n> > \n> > void V4L2DeviceTest::cleanup()\n> > {\n> > -\tmedia_->release();\n> > -\n> > \tcapture_->streamOff();\n> > \tcapture_->releaseBuffers();\n> > \tcapture_->close();\n> > diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> > index 72d5f72543d29378..5810ef3c962fab8e 100644\n> > --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> > +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> > @@ -45,20 +45,16 @@ int V4L2SubdeviceTest::init()\n> > \t\treturn TestSkip;\n> > \t}\n> > \n> > -\tmedia_->acquire();\n> > -\n> > \tint ret = media_->open();\n> > \tif (ret) {\n> > \t\tcerr << \"Unable to open media device: \" << media_->deviceNode()\n> > \t\t << \": \" << strerror(ret) << endl;\n> > -\t\tmedia_->release();\n> > \t\treturn TestSkip;\n> > \t}\n> > \n> > \tMediaEntity *videoEntity = media_->getEntityByName(\"Scaler\");\n> > \tif (!videoEntity) {\n> > \t\tcerr << \"Unable to find media entity 'Scaler'\" << endl;\n> > -\t\tmedia_->release();\n> > \t\treturn TestFail;\n> > \t}\n> > \n> > @@ -67,7 +63,6 @@ int V4L2SubdeviceTest::init()\n> > \tif (ret) {\n> > \t\tcerr << \"Unable to open video subdevice \"\n> > \t\t << scaler_->entity()->deviceNode() << endl;\n> > -\t\tmedia_->release();\n> > \t\treturn TestSkip;\n> > \t}\n> > \n> > @@ -76,7 +71,5 @@ int V4L2SubdeviceTest::init()\n> > \n> > void V4L2SubdeviceTest::cleanup()\n> > {\n> > -\tmedia_->release();\n> > -\n> > \tdelete scaler_;\n> > }\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 1591760E4F\n\tfor ;\n\tSat, 11 May 2019 15:22:31 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id j20so6027658lfh.2\n\tfor ;\n\tSat, 11 May 2019 06:22:31 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tt22sm2245589lje.58.2019.05.11.06.22.29\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSat, 11 May 2019 06:22:29 -0700 (PDT)"],"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\t:user-agent; bh=UDjflzO1b21+wu8niqc0uGoOalsX3cPKsIf080pbtSM=;\n\tb=gUFaI4JgGoXMhlz5msmzjEFl5k2BrjHDmumv0pNj07DSjqvtjkdPNTvD0xBK0jVQ7n\n\tLhKfa8vuQTV+xUFoPl9PXzeK+FrQW3dZl7hUY5LozXoGze9CcBANXdQp2i7eAfBYDGfR\n\t+eZ+fZ4UNYTdejKm/KjQ5ui3rxmn6bJZucVCdC0cA9iUlHajch3bfK7YskCile8VHbsv\n\tBQRYLXCvbAuawT5kBz2qZ4Telqx4GdJR2aqlyAQ6Rpzq64hfjofJRFcLNp83eu65tU8x\n\t2cqqi1W+W/ufL2WmCdAcjc22oGhTgxcHRr9zQuLA7oKaIIWvkqMUHZDKKVPzdx+tTxPz\n\tMVFw==","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:user-agent;\n\tbh=UDjflzO1b21+wu8niqc0uGoOalsX3cPKsIf080pbtSM=;\n\tb=BtJFHOKW2lYlZhqAMvsiZ70zCNNaYKRG8vlhgYkkYhMw4cK8z7d3w9tczqRTDXZHZb\n\tNLx0vZ94qnXnn7YCxvinwqY1I+2QaCbPBd+cYbE701fHIs+JQHyCKM8BS0GQG6vOoDpr\n\tNDD/BAFmsYuypWcJuR9KSe93s4ImkkTE6g+gA+uVRgN1EVd09zrcPb4aC+QiaMIkbx+P\n\tvRDO6SZSB2sKinsdLyTC6lP0MUWVnUnuCkqLgjJrIlfjBvMkpqjdEyfbQcm91I8wuQ0/\n\tW5nD/gUWBgC7uKiP9/G9b7lhlJ2ShAipyh2QxXYdKvciLdn/pS40v+zI4eSxvyk2V4gZ\n\tIr/g==","X-Gm-Message-State":"APjAAAVgGPv5za8Ay7EYiQSMNnLtSdwuc/aixpPv9UQNxao/cAYguCsa\n\tv+fyxiQT6ZxWnAV9gCyB9wvv7foUPDc=","X-Google-Smtp-Source":"APXvYqx1joWQZF0uZLOOx0+ZofeW0TPbpe9mwHfKH7btrgkKwzY+FIKP6qYicgkOktwtSPgqUQIQBw==","X-Received":"by 2002:ac2:434a:: with SMTP id\n\to10mr3317012lfl.122.1557580950076; \n\tSat, 11 May 2019 06:22:30 -0700 (PDT)","Date":"Sat, 11 May 2019 15:22:28 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190511132228.GL28561@bigcity.dyn.berto.se>","References":"<20190511091907.10050-1-niklas.soderlund@ragnatech.se>\n\t<20190511091907.10050-2-niklas.soderlund@ragnatech.se>\n\t<20190511125248.GB4946@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190511125248.GB4946@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v3 01/11] libcamera: Always check\n\treturn value of MediaDevice::acquire()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 11 May 2019 13:22:31 -0000"}},{"id":1591,"web_url":"https://patchwork.libcamera.org/comment/1591/","msgid":"<20190511133212.GI4946@pendragon.ideasonboard.com>","date":"2019-05-11T13:32:12","subject":"Re: [libcamera-devel] [PATCH v3 01/11] libcamera: Always check\n\treturn value of MediaDevice::acquire()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Sat, May 11, 2019 at 03:22:28PM +0200, Niklas Söderlund wrote:\n> On 2019-05-11 15:52:48 +0300, Laurent Pinchart wrote:\n> > On Sat, May 11, 2019 at 11:18:57AM +0200, Niklas Söderlund wrote:\n> > > In preparation for adding more responsibility to MediaDevice::acquire()\n> > > remove unneeded calls to acquire() and release(), and make sure all\n> > > needed calls to acquire() are checked and acted on.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 ++++------\n> > > src/libcamera/pipeline/uvcvideo.cpp | 3 ++-\n> > > src/libcamera/pipeline/vimc.cpp | 3 ++-\n> > > test/media_device/media_device_link_test.cpp | 6 +++++-\n> > > test/v4l2_device/v4l2_device_test.cpp | 4 ----\n> > > test/v4l2_subdevice/v4l2_subdevice_test.cpp | 7 -------\n> > > 6 files changed, 13 insertions(+), 20 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 0041662514e1d7ca..75e878afad4e67a9 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -614,21 +614,19 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > > \timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n> > > \timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n> > > \n> > > -\t/*\n> > > -\t * It is safe to acquire both media devices at this point as\n> > > -\t * DeviceEnumerator::search() skips the busy ones for us.\n> > > -\t */\n> > > \tcio2MediaDev_ = enumerator->search(cio2_dm);\n> > > \tif (!cio2MediaDev_)\n> > > \t\treturn false;\n> > > \n> > > -\tcio2MediaDev_->acquire();\n> > > +\tif (!cio2MediaDev_->acquire())\n> > > +\t\treturn false;\n> > > \n> > > \timguMediaDev_ = enumerator->search(imgu_dm);\n> > > \tif (!imguMediaDev_)\n> > > \t\treturn false;\n> > > \n> > > -\timguMediaDev_->acquire();\n> > > +\tif (!imguMediaDev_->acquire())\n> > > +\t\treturn false;\n> > > \n> > > \t/*\n> > > \t * Disable all links that are enabled by default on CIO2, as camera\n> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > > index 5d2f1c98fa36380c..e40b052f4d877d5d 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > > @@ -183,7 +183,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> > > \tif (!media_)\n> > > \t\treturn false;\n> > > \n> > > -\tmedia_->acquire();\n> > > +\tif (!media_->acquire())\n> > > +\t\treturn false;\n> > > \n> > > \tstd::unique_ptr data = utils::make_unique(this);\n> > > \n> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > > index cdf43770a9bfc40f..7b6ebd4cc0a27e25 100644\n> > > --- a/src/libcamera/pipeline/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc.cpp\n> > > @@ -193,7 +193,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> > > \tif (!media_)\n> > > \t\treturn false;\n> > > \n> > > -\tmedia_->acquire();\n> > > +\tif (!media_->acquire())\n> > > +\t\treturn false;\n> > > \n> > > \tstd::unique_ptr data = utils::make_unique(this);\n> > > \n> > > diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp\n> > > index 58a55cdfee634c0b..484d3691310f78a0 100644\n> > > --- a/test/media_device/media_device_link_test.cpp\n> > > +++ b/test/media_device/media_device_link_test.cpp\n> > > @@ -51,7 +51,11 @@ class MediaDeviceLinkTest : public Test\n> > > \t\t\treturn TestSkip;\n> > > \t\t}\n> > > \n> > > -\t\tdev_->acquire();\n> > > +\t\tif (!dev_->acquire()) {\n> > > +\t\t\tcerr << \"Unable to acquire media device \"\n> > > +\t\t\t << dev_->deviceNode() << endl;\n> > > +\t\t\treturn TestSkip;\n> > > +\t\t}\n> > > \n> > > \t\tif (dev_->open()) {\n> > > \t\t\tcerr << \"Failed to open media device at \"\n> > > diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp\n> > > index 4225291bbb6e23f0..833038d56ea4631d 100644\n> > > --- a/test/v4l2_device/v4l2_device_test.cpp\n> > > +++ b/test/v4l2_device/v4l2_device_test.cpp\n> > > @@ -46,8 +46,6 @@ int V4L2DeviceTest::init()\n> > > \tif (!media_)\n> > > \t\treturn TestSkip;\n> > > \n> > > -\tmedia_->acquire();\n> > > -\n> > \n> > Why is this call and the ones below unneeded ? Isn't it a good practice\n> > to always acquire the device before operating on it ? I suppose it's not\n> > strictly mandatory here as we only inspect the media device to find\n> > viden and subdev nodes, but is there any drawback in keeping these calls\n> > ?\n> \n> It's a good idea to acquire() the device if you intend to operate on it, \n> at this point in time the init() do not need to operate on the media \n> device so there is no need to acquire() it. Later in the series I do add \n> a call to acquire() and release() to this base init() function to be \n> able to perform an operation on the device (reset the media links).\n> \n> But I still think we should leave it unacquired and let each test-case \n> call acquire() if it needs to operate on the media device. Most tests \n> who inherit from this base would not need to modify the graph as it \n> intends to test the v4l2 device and for those who do I think it's nice \n> to show that clearly in the test case and not hide the acquire() in the \n> base.\n\nI'm thinking about the future and how we should try to run tests in\nparallel, with multiple instances of vivid and vimc. Some locking\nmechanism will be needed. But given that we'll have to wait for devices\nto be available, more work will be needed anyway, so for now\n\nReviewed-by: Laurent Pinchart \n\n> > > \tMediaEntity *entity = media_->getEntityByName(\"vivid-000-vid-cap\");\n> > > \tif (!entity)\n> > > \t\treturn TestSkip;\n> > > @@ -61,8 +59,6 @@ int V4L2DeviceTest::init()\n> > > \n> > > void V4L2DeviceTest::cleanup()\n> > > {\n> > > -\tmedia_->release();\n> > > -\n> > > \tcapture_->streamOff();\n> > > \tcapture_->releaseBuffers();\n> > > \tcapture_->close();\n> > > diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> > > index 72d5f72543d29378..5810ef3c962fab8e 100644\n> > > --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> > > +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> > > @@ -45,20 +45,16 @@ int V4L2SubdeviceTest::init()\n> > > \t\treturn TestSkip;\n> > > \t}\n> > > \n> > > -\tmedia_->acquire();\n> > > -\n> > > \tint ret = media_->open();\n> > > \tif (ret) {\n> > > \t\tcerr << \"Unable to open media device: \" << media_->deviceNode()\n> > > \t\t << \": \" << strerror(ret) << endl;\n> > > -\t\tmedia_->release();\n> > > \t\treturn TestSkip;\n> > > \t}\n> > > \n> > > \tMediaEntity *videoEntity = media_->getEntityByName(\"Scaler\");\n> > > \tif (!videoEntity) {\n> > > \t\tcerr << \"Unable to find media entity 'Scaler'\" << endl;\n> > > -\t\tmedia_->release();\n> > > \t\treturn TestFail;\n> > > \t}\n> > > \n> > > @@ -67,7 +63,6 @@ int V4L2SubdeviceTest::init()\n> > > \tif (ret) {\n> > > \t\tcerr << \"Unable to open video subdevice \"\n> > > \t\t << scaler_->entity()->deviceNode() << endl;\n> > > -\t\tmedia_->release();\n> > > \t\treturn TestSkip;\n> > > \t}\n> > > \n> > > @@ -76,7 +71,5 @@ int V4L2SubdeviceTest::init()\n> > > \n> > > void V4L2SubdeviceTest::cleanup()\n> > > {\n> > > -\tmedia_->release();\n> > > -\n> > > \tdelete scaler_;\n> > > }\n> > \n> > -- \n> > Regards,\n> > \n> > Laurent Pinchart\n> \n> -- \n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E287560E4F\n\tfor ;\n\tSat, 11 May 2019 15:32:28 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0C4F0D5;\n\tSat, 11 May 2019 15:32:27 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557581548;\n\tbh=AKkFFokN+AtQJi+6Slzj+8nfWW94VYnS6VVx35d6byw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k9yb2B8eiJF+ne7HaqA5x0QqauxSK8Dc1dU/6byKDuTS/jMFjohVDW3eRxJ2AS/nN\n\t2eD6rnBlJoB/rvkKJxJQ/ry1AYlaTSvdJh8Z4VxMcRmbLHvKO85tsSCTQu2b4pqxkJ\n\tpoXjebGtSgxkxM4Z4NgpY7v72oA4p7ajIoriy5gQ=","Date":"Sat, 11 May 2019 16:32:12 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190511133212.GI4946@pendragon.ideasonboard.com>","References":"<20190511091907.10050-1-niklas.soderlund@ragnatech.se>\n\t<20190511091907.10050-2-niklas.soderlund@ragnatech.se>\n\t<20190511125248.GB4946@pendragon.ideasonboard.com>\n\t<20190511132228.GL28561@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190511132228.GL28561@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 01/11] libcamera: Always check\n\treturn value of MediaDevice::acquire()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 11 May 2019 13:32:29 -0000"}}]