[{"id":2290,"web_url":"https://patchwork.libcamera.org/comment/2290/","msgid":"<20190716075957.GE22355@pendragon.ideasonboard.com>","date":"2019-07-16T07:59:57","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipu3: Disable links\n\tat configure() time","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Jul 16, 2019 at 09:27:38AM +0200, Jacopo Mondi wrote:\n> With the current IPU3 kernel driver implementation, a linked pipe shall be\n> used (buffers should be queued on it) in order not to block all other pipes.\n> \n> Currently all links on the ImgU device are only disabled at match() time,\n> implying that once an ImgU pipe gets linked, it should be used until the\n> whole pipeline is not re-matched and links disabled again. This is a severe\n> limitation for applications that wants to switch between cameras using\n> different pipes going through a full library tear-down and reload.\n> \n> Perform link disabling at configure() time as well, so that a camera\n> configuration operation always unlock the usage of the assigned pipe,\n> regardless of the previously linked ones.\n> \n> Unfortunately this requires a camera start/stop sequence to always go\n> through a configure step, a requirement that is not enforced by the\n> Camera state machine.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 54 ++++++++++++++++------------\n> 1 file changed, 31 insertions(+), 23 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 159a9312f95e..bae3072b177f 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -499,6 +499,37 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \tImgUDevice *imgu = data->imgu_;\n> \tint ret;\n> \n> +\t/*\n> +\t * FIXME: enabled links in one ImgU pipe interfere with capture\n> +\t * operations on the other one. This can be easily triggered by\n> +\t * capturing from one camera and then trying to capture from the other\n> +\t * one right after, without disabling media links on the first used\n> +\t * pipe.\n> +\t *\n> +\t * The tricky part here is where to disable links on the ImgU instance\n> +\t * which is currently not in use:\n> +\t * 1) Link enable/disable cannot be done at start()/stop() time as video\n> +\t * devices needs to be linked first before format can be configured on\n> +\t * them.\n> +\t * 2) As link enable has to be done at the least in configure(),\n> +\t * before configuring formats, the only place where to disable links\n> +\t * would be 'stop()', but the Camera class state machine allows\n> +\t * start()<->stop() sequences without any configure() in between.\n> +\t *\n> +\t * As of now, disable all links in the ImgU media graph before\n> +\t * configuring the device, to allow alternate the usage of the two\n> +\t * ImgU pipes.\n> +\t *\n> +\t * As a consequence, a Camera using an ImgU shall be configured before\n> +\t * any start()/stop() sequence. An application that wants to\n> +\t * pre-configure all the camera and then start/stop them alternatively\n> +\t * without going through any re-configuration (a sequence that is\n> +\t * allowed by the Camera state machine) would now fail on the IPU3.\n> +\t */\n> +\tret = imguMediaDev_->disableLinks();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \t/*\n> \t * \\todo: Enable links selectively based on the requested streams.\n> \t * As of now, enable all links unconditionally.\n> @@ -784,29 +815,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> \tif (cio2MediaDev_->disableLinks())\n> \t\treturn false;\n> \n> -\t/*\n> -\t * FIXME: enabled links in one ImgU instance interfere with capture\n> -\t * operations on the other one. This can be easily triggered by\n> -\t * capturing from one camera and then trying to capture from the other\n> -\t * one right after, without disabling media links in the media graph\n> -\t * first.\n> -\t *\n> -\t * The tricky part here is where to disable links on the ImgU instance\n> -\t * which is currently not in use:\n> -\t * 1) Link enable/disable cannot be done at start/stop time as video\n> -\t * devices needs to be linked first before format can be configured on\n> -\t * them.\n> -\t * 2) As link enable has to be done at the least in configure(),\n> -\t * before configuring formats, the only place where to disable links\n> -\t * would be 'stop()', but the Camera class state machine allows\n> -\t * start()<->stop() sequences without any configure() in between.\n> -\t *\n> -\t * As of now, disable all links in the media graph at 'match()' time,\n> -\t * to allow testing different cameras in different test applications\n> -\t * runs. A test application that would use two distinct cameras without\n> -\t * going through a library teardown->match() sequence would fail\n> -\t * at the moment.\n> -\t */\n> \tret = imguMediaDev_->disableLinks();\n> \tif (ret)\n> \t\treturn ret;","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 2EADD61AD3\n\tfor ;\n\tTue, 16 Jul 2019 10:00:27 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a00:79e1:abc:3602:59ec:6c:1869:337])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E2831564;\n\tTue, 16 Jul 2019 10:00:25 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1563264026;\n\tbh=eH8LOBTlw/hssdsVUG7z+JEw4tk9t3YLW34W6i92EAg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aFpyQglpINaf/quoBttUJFAIM2cPoVlMJfyqt//tv6dpn0n6ppiyyIVl3X/vUPoJd\n\tksJm28pDIUHEMeYInDop4+WebyOJhWyVuUtLH2xo+2s4gH9cdAFpGfCO8l+a2MOR0A\n\tk/WIxoY6mgBHq/kmLHHDsGXMyatDfhFtIqU3u52A=","Date":"Tue, 16 Jul 2019 10:59:57 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190716075957.GE22355@pendragon.ideasonboard.com>","References":"<20190716072739.2071-1-jacopo@jmondi.org>\n\t<20190716072739.2071-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190716072739.2071-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipu3: Disable links\n\tat configure() time","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":"Tue, 16 Jul 2019 08:00:27 -0000"}}]