[{"id":2271,"web_url":"https://patchwork.libcamera.org/comment/2271/","msgid":"<20190715064135.GA19114@pendragon.ideasonboard.com>","date":"2019-07-15T06:41:35","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: ipu3: Move link\n\tdisabling at 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 Mon, Jul 15, 2019 at 07:59:33AM +0200, Jacopo Mondi wrote:\n> With the current IPU3 kernel driver implementation, if links are enabled on\n> an ImgU pipe buffers should be queued on its output devices in order not to\n> block the other pipe.\n> \n> Currently all links on the ImgU device are disabled at match() time,\n> implying that once an ImgU pipe gets linked, it should be used until the\n> whole pipeline handler is not re-matched and links disabled again. This is a\n> severe limitation for applications that wants to switch between cameras\n> using different pipes without going through a full library tear-down and\n> re-initialization.\n> \n> Move the link disabling at configure() time, so that a camera\n> configuration operation always unlock the usage of the assigned pipe,\n> regardless of the status of the one previously in use.\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\nHow about disabling links in both match() and stop() then ?\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 55 +++++++++++++++-------------\n> 1 file changed, 29 insertions(+), 26 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 22987dbf1460..5204487684c2 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -496,6 +496,35 @@ 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 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 usage of two 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> @@ -778,32 +807,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;\n> \n> \tret = registerCameras();\n>","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 4BAEA61A74\n\tfor ;\n\tMon, 15 Jul 2019 08:42:04 +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 020FC578;\n\tMon, 15 Jul 2019 08:42:02 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1563172923;\n\tbh=cKudykW/RWKnLltvw9Rob2AaiEQkfWcz3zNNBd01lrQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KYgVy3GsHuitcwVLESa7IgV7+yGRdn9ZJjCc7psIlm6buOHlgFFQevVlcmkVSFF7T\n\t9WJSyhgA17M4ktq2wCxmHYSaLbquJtqtyIC/hqFDujFAcJmyVQhPuZ08qsIEK3Ty+0\n\trqAHawf8IpfS3c88GgtmeYzci2rvApxMNpEbiyOM=","Date":"Mon, 15 Jul 2019 09:41:35 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190715064135.GA19114@pendragon.ideasonboard.com>","References":"<20190715055935.21233-1-jacopo@jmondi.org>\n\t<20190715055935.21233-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190715055935.21233-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: ipu3: Move link\n\tdisabling at 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":"Mon, 15 Jul 2019 06:42:04 -0000"}},{"id":2280,"web_url":"https://patchwork.libcamera.org/comment/2280/","msgid":"<20190716062330.opuhv3gz5clrmzba@uno.localdomain>","date":"2019-07-16T06:23:30","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: ipu3: Move link\n\tdisabling at configure time","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jul 15, 2019 at 09:41:35AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Jul 15, 2019 at 07:59:33AM +0200, Jacopo Mondi wrote:\n> > With the current IPU3 kernel driver implementation, if links are enabled on\n> > an ImgU pipe buffers should be queued on its output devices in order not to\n> > block the other pipe.\n> >\n> > Currently all links on the ImgU device are disabled at match() time,\n> > implying that once an ImgU pipe gets linked, it should be used until the\n> > whole pipeline handler is not re-matched and links disabled again. This is a\n> > severe limitation for applications that wants to switch between cameras\n> > using different pipes without going through a full library tear-down and\n> > re-initialization.\n> >\n> > Move the link disabling at configure() time, so that a camera\n> > configuration operation always unlock the usage of the assigned pipe,\n> > regardless of the status of the one previously in use.\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> How about disabling links in both match() and stop() then ?\n\nFine with match() as starting with a clean media device is desirable\n(I'll move the comment here anyway).\n\nIf we move this at stop() time though, a start->stop->start sequence\nwould always fail, so I would avoid that.\n\nThanks\n j\n\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 55 +++++++++++++++-------------\n> > 1 file changed, 29 insertions(+), 26 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 22987dbf1460..5204487684c2 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -496,6 +496,35 @@ 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 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 usage of two 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> > @@ -778,32 +807,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;\n> >\n> > \tret = registerCameras();\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0437D60E3E\n\tfor ;\n\tTue, 16 Jul 2019 08:22:15 +0200 (CEST)","from uno.localdomain (unknown [104.132.146.101])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id B8F7A20003;\n\tTue, 16 Jul 2019 06:22:13 +0000 (UTC)"],"X-Originating-IP":"104.132.146.101","Date":"Tue, 16 Jul 2019 08:23:30 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190716062330.opuhv3gz5clrmzba@uno.localdomain>","References":"<20190715055935.21233-1-jacopo@jmondi.org>\n\t<20190715055935.21233-2-jacopo@jmondi.org>\n\t<20190715064135.GA19114@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"5tx53pyxx4hwq6es\"","Content-Disposition":"inline","In-Reply-To":"<20190715064135.GA19114@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: ipu3: Move link\n\tdisabling at 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 06:22:15 -0000"}}]