[{"id":202,"web_url":"https://patchwork.libcamera.org/comment/202/","msgid":"<20190104164509.GN22790@bigcity.dyn.berto.se>","date":"2019-01-04T16:45:09","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: Add MediaLink link\n\tsetup function","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your patch.\n\nOn 2019-01-03 18:38:57 +0100, Jacopo Mondi wrote:\n> Add a function to the MediaLink class to set the state of a link to\n> enabled or disabled. The function makes use of an internal MediaDevice\n> method, which is defined private and only accessible by the MediaLink\n> setup() function itself.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/media_device.h | 8 ++++++\n> src/libcamera/include/media_object.h | 1 +\n> src/libcamera/media_device.cpp | 30 +++++++++++++++++++++\n> src/libcamera/media_object.cpp | 40 ++++++++++++++++++++++++++++\n> 4 files changed, 79 insertions(+)\n> \n> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h\n> index 3228ad5..d019639 100644\n> --- a/src/libcamera/include/media_device.h\n> +++ b/src/libcamera/include/media_device.h\n> @@ -65,6 +65,14 @@ private:\n> \tbool populateEntities(const struct media_v2_topology &topology);\n> \tbool populatePads(const struct media_v2_topology &topology);\n> \tbool populateLinks(const struct media_v2_topology &topology);\n> +\n> +\t/*\n> +\t * The MediaLink enable method needs to access the private\n> +\t * setLink() function.\n> +\t */\n> +\tfriend int MediaLink::enable(bool enable);\n\nI'm not saying we won't need this. But reading this makes me a bit \nuncomfortable that the over all design is not sound. Would it not be OK \nto make setLink() public instead? This interface will not be exposed to \napplications so I don't feel there is a great need to protect us from us \nself :-)\n\n> +\tint setLink(const MediaPad *source, const MediaPad *sink,\n> +\t\t unsigned int flags);\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h\n> index 0f0bb29..e00c639 100644\n> --- a/src/libcamera/include/media_object.h\n> +++ b/src/libcamera/include/media_object.h\n> @@ -40,6 +40,7 @@ public:\n> \tMediaPad *source() const { return source_; }\n> \tMediaPad *sink() const { return sink_; }\n> \tunsigned int flags() const { return flags_; }\n> +\tint enable(bool enable);\n> \n> private:\n> \tfriend class MediaDevice;\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 6892300..b86d0c4 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -641,4 +641,34 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> \treturn true;\n> }\n> \n> +int MediaDevice::setLink(const MediaPad *source, const MediaPad *sink,\n> +\t\t\t unsigned int flags)\n> +{\n> +\tstruct media_link_desc linkDesc = { };\n> +\n> +\tlinkDesc.source.entity = source->entity()->id();\n> +\tlinkDesc.source.index = source->index();\n> +\tlinkDesc.source.flags = MEDIA_PAD_FL_SOURCE;\n> +\n> +\tlinkDesc.sink.entity = sink->entity()->id();\n> +\tlinkDesc.sink.index = sink->index();\n> +\tlinkDesc.sink.flags = MEDIA_PAD_FL_SINK;\n> +\n> +\tlinkDesc.flags = flags;\n> +\n> +\tint ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Failed to setup link: \" << strerror(-ret) << \"\\n\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tLOG(Debug) << source->entity()->name() << \"[\"\n> +\t\t << source->index() << \"] -> \"\n> +\t\t << sink->entity()->name() << \"[\"\n> +\t\t << sink->index() << \"]: \" << flags;\n\nShould you not log linkDesc.flags here in case the IOCTL changed the \nflags? Or maybe even compare flags to linkDesc.flags to make sure the \nflags you want set is set. This is a open question and I'm fine with \nthis approach if MEDIA_IOC_SETUP_LINK can not modify the flags.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp\n> index f1535e6..1ee8823 100644\n> --- a/src/libcamera/media_object.cpp\n> +++ b/src/libcamera/media_object.cpp\n> @@ -16,6 +16,7 @@\n> \n> #include \"log.h\"\n> #include \"media_object.h\"\n> +#include \"media_device.h\"\n> \n> /**\n> * \\file media_object.h\n> @@ -87,6 +88,45 @@ namespace libcamera {\n> * Each link is referenced in the link array of both of the pads it connect.\n> */\n> \n> +/**\n> + * \\brief Set a link state to enabled or disabled\n\nEnable or disable a link in the media graph\n\n> + * \\param enable The enable flag, true enables the link, false disables it\n> + *\n> + * Set the status of a link, according to the value of \\a enable.\n> + * Links between two pads can be set to the enabled or disabled state freely,\n> + * unless they're immutable links, whose status cannot be changed.\n> + * Enabling an immutable link is not considered an error, while trying to\n> + * disable it is. Caller should inspect the link flags before trying to\n> + * disable an immutable link.\n> + *\n> + * Enabling a link establishes a data connection between two pads, while\n> + * disabling it interrupts such a connections.\n> + *\n> + * \\return 0 for success, negative error code otherwise\n> + */\n> +int MediaLink::enable(bool enable)\n> +{\n> +\tunsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;\n> +\n> +\tif ((flags_ & MEDIA_LNK_FL_IMMUTABLE) && !enable) {\n> +\t\tLOG(Error) << \"Immutable links cannot be disabled\";\n> +\t\treturn -EINVAL;\n> +\t}\n\nIs it sound to relay on cached values here? What if the kernel have \nchanged the state of the link to once more be mutable (not sure any \ndrivers to this today)? I would drop this check and let the kernel deal \nwith this and relay on the return code from the MEDIA_IOC_SETUP_LINK \nioctl.\n\n> +\n> +\t/* Do not try to enable an already enabled link. */\n> +\tif ((flags_ & MEDIA_LNK_FL_ENABLED) && enable)\n> +\t\treturn 0;\n\nSame here, I would not trust the cached value. What if another process \nhave modified the media graph unknown to the library? Better let the \nrequest happen and let the kernel deal with it.\n\n> +\n> +\tint ret = media_->setLink(source_, sink_, flags);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* Only update flags if 'setLink()' didn't fail. */\n> +\tflags_ = flags;\n> +\n> +\treturn 0;\n\nI think you can drop the comment and change this to:\n\n int ret = media_->setLink(source_, sink_, flags);\n\n if (!ret)\n flags_ = flags;\n\n return ret;\n\n> +}\n> +\n> /**\n> * \\brief Construct a MediaLink\n> * \\param media The media device this entity belongs to\n> -- \n> 2.20.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-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D306860B0B\n\tfor ;\n\tFri, 4 Jan 2019 17:45:11 +0100 (CET)","by mail-lj1-x243.google.com with SMTP id c19-v6so32936937lja.5\n\tfor ;\n\tFri, 04 Jan 2019 08:45:11 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tl63sm11426174lfl.76.2019.01.04.08.45.10\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 04 Jan 2019 08:45:10 -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\t:user-agent; bh=BGkEWuCOsiyqn9pQ4Lq6lO5k/pWL0F6vvX+TMeXdeIs=;\n\tb=ylxRoDBB1DzgiUlFZkInXvycnqUZcBoqEC+R6/fN6DMOtThXeH9r6sbDMDs3ZTchyu\n\tMkeY5Lej90xOr6vjEXEbmFdWrIzaJs/3UlxroTPXoGNbUjiK/b6PEuet3jbc9VH5huOf\n\tqEk0qfLU9MnZJpxbmeDgXbh9q0Az9l0rsQqoxv8RLDUMv8VMVIjOCYiUiPnM3lw30oFo\n\tSCKrKKazg6rQ55aUiDijticVa3wQI3M5Ks3bieZY88MFi7bI3GOTqjBdzVo2zL55GcRp\n\tcNGacp13T2nP08vD7KyzziRucJzkq399/LBU/5z9/0SFlZZc+Pc3kYhtQGsoe69sOEXn\n\t9r5g==","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=BGkEWuCOsiyqn9pQ4Lq6lO5k/pWL0F6vvX+TMeXdeIs=;\n\tb=VCrh9Q50HwuP3n8BJOjfyC0eaF92vnGYkgsIElmRn5XQShv3cmKQH7x1PYla+v+amj\n\tkMDR1IOmSP+GPLCZS4eT8FRDR5gr5zhWFbzwV0igeWdnFPK14EPPwfL8uvQoGm1ws7zb\n\tg+lffqYNmfCQ2nCIHP5hqu61OnvOIaJaI5Y7WchFs2HI4aTUC+jk7hqks2sJCWJGyRO0\n\tdGRdtpQOHP1GFq48cf+F0koVKFm9OhN6zP/81aILZ7fnBX/r/aoRYIAZYLwLs2xrNlUR\n\t9DRy/R2wj1lVyrOG1nv16pS5BbFT1EiLrrdCJTpMxXq1bQfGjkiOxKSwoGgmPhfgkUdb\n\taJow==","X-Gm-Message-State":"AJcUukfdBch/fz/F9OQCI4jDJftDVqK8CAwFsAIga7VmwYKT3E7zoaYi\n\tYjzUhgbrp6skFNjHhHWbUIeaE5Q1tNw=","X-Google-Smtp-Source":"ALg8bN4FR3y6/Izj9akJcy8JM4WThTZ9gH2//h2Pty/XEuy7uwOr8ajwVgXx8+FkWwNE0DcZNem/Jg==","X-Received":"by 2002:a2e:5109:: with SMTP id\n\tf9-v6mr31894501ljb.52.1546620310988; \n\tFri, 04 Jan 2019 08:45:10 -0800 (PST)","Date":"Fri, 4 Jan 2019 17:45:09 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190104164509.GN22790@bigcity.dyn.berto.se>","References":"<20190103173859.22624-1-jacopo@jmondi.org>\n\t<20190103173859.22624-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190103173859.22624-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: Add MediaLink link\n\tsetup function","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":"Fri, 04 Jan 2019 16:45:12 -0000"}},{"id":215,"web_url":"https://patchwork.libcamera.org/comment/215/","msgid":"<20190107084117.rzuu3dh2o45z2tyq@uno.localdomain>","date":"2019-01-07T08:41:17","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: Add MediaLink link\n\tsetup function","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n let me start from this comment\n\nOn Fri, Jan 04, 2019 at 05:45:09PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your patch.\n>\n> On 2019-01-03 18:38:57 +0100, Jacopo Mondi wrote:\n> > Add a function to the MediaLink class to set the state of a link to\n> > enabled or disabled. The function makes use of an internal MediaDevice\n> > method, which is defined private and only accessible by the MediaLink\n> > setup() function itself.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/media_device.h | 8 ++++++\n> > src/libcamera/include/media_object.h | 1 +\n> > src/libcamera/media_device.cpp | 30 +++++++++++++++++++++\n> > src/libcamera/media_object.cpp | 40 ++++++++++++++++++++++++++++\n> > 4 files changed, 79 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h\n> > index 3228ad5..d019639 100644\n> > --- a/src/libcamera/include/media_device.h\n> > +++ b/src/libcamera/include/media_device.h\n> > @@ -65,6 +65,14 @@ private:\n> > \tbool populateEntities(const struct media_v2_topology &topology);\n> > \tbool populatePads(const struct media_v2_topology &topology);\n> > \tbool populateLinks(const struct media_v2_topology &topology);\n> > +\n> > +\t/*\n> > +\t * The MediaLink enable method needs to access the private\n> > +\t * setLink() function.\n> > +\t */\n> > +\tfriend int MediaLink::enable(bool enable);\n>\n> I'm not saying we won't need this. But reading this makes me a bit\n> uncomfortable that the over all design is not sound. Would it not be OK\n> to make setLink() public instead? This interface will not be exposed to\n> applications so I don't feel there is a great need to protect us from us\n> self :-)\n>\n\nI discussed it privately as well, as my initial idea was to have all\nof this link handling operations going through the media device object,\nin which case a public setLink() function would be the one the pipeline\nhandlers would have to use. To me that would have made possible to\nserialize operations and guarantee that a configuration on the media\ngraph is applied entirely, and through a single entity-oriented API,\ninstead of providing an interface to retrieve a link and then operate\non it asynchronously.\n\nI've been convinced that for now is better to let pipeline handlers\nto retrieve links and then enable/disable them singularly, as the\n'MediaLink::enable()' operation does, and in that design I had to do a\nfew things I'm not super happy with:\n- Add a media device reference to MediaObjects\n- Add the here below friend method so that i can call setLink (which\n in this way, is at least not public)\n\nWhat are your concerns on this design for thinking it is not sound?\n\nAh, a note: we don't have to protect us from ourselves (which is not\ntotally true anyway...) but remember pipeline handlers will be\nimplemented by anyone who has a platform to run libcamera on, and it\nis important we define an unambigous interface to provide them.\n\nThanks\n j\n\n> > +\tint setLink(const MediaPad *source, const MediaPad *sink,\n> > +\t\t unsigned int flags);\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h\n> > index 0f0bb29..e00c639 100644\n> > --- a/src/libcamera/include/media_object.h\n> > +++ b/src/libcamera/include/media_object.h\n> > @@ -40,6 +40,7 @@ public:\n> > \tMediaPad *source() const { return source_; }\n> > \tMediaPad *sink() const { return sink_; }\n> > \tunsigned int flags() const { return flags_; }\n> > +\tint enable(bool enable);\n> >\n> > private:\n> > \tfriend class MediaDevice;\n> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> > index 6892300..b86d0c4 100644\n> > --- a/src/libcamera/media_device.cpp\n> > +++ b/src/libcamera/media_device.cpp\n> > @@ -641,4 +641,34 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> > \treturn true;\n> > }\n> >\n> > +int MediaDevice::setLink(const MediaPad *source, const MediaPad *sink,\n> > +\t\t\t unsigned int flags)\n> > +{\n> > +\tstruct media_link_desc linkDesc = { };\n> > +\n> > +\tlinkDesc.source.entity = source->entity()->id();\n> > +\tlinkDesc.source.index = source->index();\n> > +\tlinkDesc.source.flags = MEDIA_PAD_FL_SOURCE;\n> > +\n> > +\tlinkDesc.sink.entity = sink->entity()->id();\n> > +\tlinkDesc.sink.index = sink->index();\n> > +\tlinkDesc.sink.flags = MEDIA_PAD_FL_SINK;\n> > +\n> > +\tlinkDesc.flags = flags;\n> > +\n> > +\tint ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Failed to setup link: \" << strerror(-ret) << \"\\n\";\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tLOG(Debug) << source->entity()->name() << \"[\"\n> > +\t\t << source->index() << \"] -> \"\n> > +\t\t << sink->entity()->name() << \"[\"\n> > +\t\t << sink->index() << \"]: \" << flags;\n>\n> Should you not log linkDesc.flags here in case the IOCTL changed the\n> flags? Or maybe even compare flags to linkDesc.flags to make sure the\n> flags you want set is set. This is a open question and I'm fine with\n> this approach if MEDIA_IOC_SETUP_LINK can not modify the flags.\n>\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp\n> > index f1535e6..1ee8823 100644\n> > --- a/src/libcamera/media_object.cpp\n> > +++ b/src/libcamera/media_object.cpp\n> > @@ -16,6 +16,7 @@\n> >\n> > #include \"log.h\"\n> > #include \"media_object.h\"\n> > +#include \"media_device.h\"\n> >\n> > /**\n> > * \\file media_object.h\n> > @@ -87,6 +88,45 @@ namespace libcamera {\n> > * Each link is referenced in the link array of both of the pads it connect.\n> > */\n> >\n> > +/**\n> > + * \\brief Set a link state to enabled or disabled\n>\n> Enable or disable a link in the media graph\n>\n> > + * \\param enable The enable flag, true enables the link, false disables it\n> > + *\n> > + * Set the status of a link, according to the value of \\a enable.\n> > + * Links between two pads can be set to the enabled or disabled state freely,\n> > + * unless they're immutable links, whose status cannot be changed.\n> > + * Enabling an immutable link is not considered an error, while trying to\n> > + * disable it is. Caller should inspect the link flags before trying to\n> > + * disable an immutable link.\n> > + *\n> > + * Enabling a link establishes a data connection between two pads, while\n> > + * disabling it interrupts such a connections.\n> > + *\n> > + * \\return 0 for success, negative error code otherwise\n> > + */\n> > +int MediaLink::enable(bool enable)\n> > +{\n> > +\tunsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;\n> > +\n> > +\tif ((flags_ & MEDIA_LNK_FL_IMMUTABLE) && !enable) {\n> > +\t\tLOG(Error) << \"Immutable links cannot be disabled\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n>\n> Is it sound to relay on cached values here? What if the kernel have\n> changed the state of the link to once more be mutable (not sure any\n> drivers to this today)? I would drop this check and let the kernel deal\n> with this and relay on the return code from the MEDIA_IOC_SETUP_LINK\n> ioctl.\n>\n> > +\n> > +\t/* Do not try to enable an already enabled link. */\n> > +\tif ((flags_ & MEDIA_LNK_FL_ENABLED) && enable)\n> > +\t\treturn 0;\n>\n> Same here, I would not trust the cached value. What if another process\n> have modified the media graph unknown to the library? Better let the\n> request happen and let the kernel deal with it.\n>\n> > +\n> > +\tint ret = media_->setLink(source_, sink_, flags);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Only update flags if 'setLink()' didn't fail. */\n> > +\tflags_ = flags;\n> > +\n> > +\treturn 0;\n>\n> I think you can drop the comment and change this to:\n>\n> int ret = media_->setLink(source_, sink_, flags);\n>\n> if (!ret)\n> flags_ = flags;\n>\n> return ret;\n>\n> > +}\n> > +\n> > /**\n> > * \\brief Construct a MediaLink\n> > * \\param media The media device this entity belongs to\n> > --\n> > 2.20.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","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8308C60B0B\n\tfor ;\n\tMon, 7 Jan 2019 09:41:12 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 136B740010;\n\tMon, 7 Jan 2019 08:41:11 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 7 Jan 2019 09:41:17 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190107084117.rzuu3dh2o45z2tyq@uno.localdomain>","References":"<20190103173859.22624-1-jacopo@jmondi.org>\n\t<20190103173859.22624-4-jacopo@jmondi.org>\n\t<20190104164509.GN22790@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"yplysas5rte7bdou\"","Content-Disposition":"inline","In-Reply-To":"<20190104164509.GN22790@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: Add MediaLink link\n\tsetup function","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, 07 Jan 2019 08:41:12 -0000"}},{"id":237,"web_url":"https://patchwork.libcamera.org/comment/237/","msgid":"<1591398.Nf03XtP9Ca@avalon>","date":"2019-01-07T21:50:49","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: Add MediaLink link\n\tsetup function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Monday, 7 January 2019 10:41:17 EET Jacopo Mondi wrote:\n> On Fri, Jan 04, 2019 at 05:45:09PM +0100, Niklas Söderlund wrote:\n> > On 2019-01-03 18:38:57 +0100, Jacopo Mondi wrote:\n> > > Add a function to the MediaLink class to set the state of a link to\n> > > enabled or disabled. The function makes use of an internal MediaDevice\n> > > method, which is defined private and only accessible by the MediaLink\n> > > setup() function itself.\n> > > \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > \n> > > src/libcamera/include/media_device.h | 8 ++++++\n> > > src/libcamera/include/media_object.h | 1 +\n> > > src/libcamera/media_device.cpp | 30 +++++++++++++++++++++\n> > > src/libcamera/media_object.cpp | 40 ++++++++++++++++++++++++++++\n> > > 4 files changed, 79 insertions(+)\n> > > \n> > > diff --git a/src/libcamera/include/media_device.h\n> > > b/src/libcamera/include/media_device.h index 3228ad5..d019639 100644\n> > > --- a/src/libcamera/include/media_device.h\n> > > +++ b/src/libcamera/include/media_device.h\n> > > @@ -65,6 +65,14 @@ private:\n> > > \tbool populateEntities(const struct media_v2_topology &topology);\n> > > \tbool populatePads(const struct media_v2_topology &topology);\n> > > \tbool populateLinks(const struct media_v2_topology &topology);\n> > > +\n> > > +\t/*\n> > > +\t * The MediaLink enable method needs to access the private\n> > > +\t * setLink() function.\n> > > +\t */\n\nDo we need this comment ?\n\n> > > +\tfriend int MediaLink::enable(bool enable);\n> > \n> > I'm not saying we won't need this. But reading this makes me a bit\n> > uncomfortable that the over all design is not sound. Would it not be OK\n> > to make setLink() public instead? This interface will not be exposed to\n> > applications so I don't feel there is a great need to protect us from us\n> > self :-)\n> \n> I discussed it privately as well, as my initial idea was to have all\n> of this link handling operations going through the media device object,\n> in which case a public setLink() function would be the one the pipeline\n> handlers would have to use. To me that would have made possible to\n> serialize operations and guarantee that a configuration on the media\n> graph is applied entirely, and through a single entity-oriented API,\n> instead of providing an interface to retrieve a link and then operate\n> on it asynchronously.\n> \n> I've been convinced that for now is better to let pipeline handlers\n> to retrieve links and then enable/disable them singularly, as the\n> 'MediaLink::enable()' operation does, and in that design I had to do a\n> few things I'm not super happy with:\n> - Add a media device reference to MediaObjects\n> - Add the here below friend method so that i can call setLink (which\n> in this way, is at least not public)\n> \n> What are your concerns on this design for thinking it is not sound?\n> \n> Ah, a note: we don't have to protect us from ourselves (which is not\n> totally true anyway...) but remember pipeline handlers will be\n> implemented by anyone who has a platform to run libcamera on, and it\n> is important we define an unambigous interface to provide them.\n> \n> > > +\tint setLink(const MediaPad *source, const MediaPad *sink,\n> > > +\t\t unsigned int flags);\n\nWhy do you pass the source and sink pointers instead of the link pointer ?\n\n> > > };\n> > > \n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/include/media_object.h\n> > > b/src/libcamera/include/media_object.h index 0f0bb29..e00c639 100644\n> > > --- a/src/libcamera/include/media_object.h\n> > > +++ b/src/libcamera/include/media_object.h\n> > > @@ -40,6 +40,7 @@ public:\n> > > \tMediaPad *source() const { return source_; }\n> > > \tMediaPad *sink() const { return sink_; }\n> > > \tunsigned int flags() const { return flags_; }\n> > > \n> > > +\tint enable(bool enable);\n\nShould this be called setEnabled() to follow the \"getters are named as \nproperties, setters with a set prefix\" rule ? Furthermore disabling a link by \ncalling the enable() method is a bit confusing :-)\n\n> > > private:\n> > > \tfriend class MediaDevice;\n> > > \n> > > diff --git a/src/libcamera/media_device.cpp\n> > > b/src/libcamera/media_device.cpp index 6892300..b86d0c4 100644\n> > > --- a/src/libcamera/media_device.cpp\n> > > +++ b/src/libcamera/media_device.cpp\n> > > @@ -641,4 +641,34 @@ bool MediaDevice::populateLinks(const struct\n> > > media_v2_topology &topology)\n> > > \treturn true;\n> > > }\n> > > \n> > > +int MediaDevice::setLink(const MediaPad *source, const MediaPad *sink,\n> > > +\t\t\t unsigned int flags)\n> > > +{\n> > > +\tstruct media_link_desc linkDesc = { };\n> > > +\n> > > +\tlinkDesc.source.entity = source->entity()->id();\n> > > +\tlinkDesc.source.index = source->index();\n> > > +\tlinkDesc.source.flags = MEDIA_PAD_FL_SOURCE;\n> > > +\n> > > +\tlinkDesc.sink.entity = sink->entity()->id();\n> > > +\tlinkDesc.sink.index = sink->index();\n> > > +\tlinkDesc.sink.flags = MEDIA_PAD_FL_SINK;\n> > > +\n> > > +\tlinkDesc.flags = flags;\n> > > +\n> > > +\tint ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc);\n> > > +\tif (ret) {\n> > > +\t\tret = -errno;\n> > > +\t\tLOG(Error) << \"Failed to setup link: \" << strerror(-ret) << \"\\n\";\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tLOG(Debug) << source->entity()->name() << \"[\"\n> > > +\t\t << source->index() << \"] -> \"\n> > > +\t\t << sink->entity()->name() << \"[\"\n> > > +\t\t << sink->index() << \"]: \" << flags;\n> > \n> > Should you not log linkDesc.flags here in case the IOCTL changed the\n> > flags? Or maybe even compare flags to linkDesc.flags to make sure the\n> > flags you want set is set. This is a open question and I'm fine with\n> > this approach if MEDIA_IOC_SETUP_LINK can not modify the flags.\n> > \n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/media_object.cpp\n> > > b/src/libcamera/media_object.cpp index f1535e6..1ee8823 100644\n> > > --- a/src/libcamera/media_object.cpp\n> > > +++ b/src/libcamera/media_object.cpp\n> > > @@ -16,6 +16,7 @@\n> > > \n> > > #include \"log.h\"\n> > > #include \"media_object.h\"\n> > > +#include \"media_device.h\"\n\nAlphabetical order please :-)\n\n> > > \n> > > /**\n> > > * \\file media_object.h\n> > > @@ -87,6 +88,45 @@ namespace libcamera {\n> > > * Each link is referenced in the link array of both of the pads it\n> > > connect.\n> > > */\n> > > \n> > > +/**\n> > > + * \\brief Set a link state to enabled or disabled\n> > \n> > Enable or disable a link in the media graph\n> > \n> > > + * \\param enable The enable flag, true enables the link, false disables\n> > > it\n\nFrom the point of view of this method, it's not a link flag, but an enable/\ndisable boolean, so I wouldn't mention flag here.\n\n * \\param enable True to enable the link, false to disable it\n\n> > > + *\n> > > + * Set the status of a link, according to the value of \\a enable.\n> > > + * Links between two pads can be set to the enabled or disabled state\n> > > freely, + * unless they're immutable links, whose status cannot be\n> > > changed. + * Enabling an immutable link is not considered an error,\n> > > while trying to + * disable it is. Caller should inspect the link flags\n> > > before trying to + * disable an immutable link.\n\nShouldn't we reject attempts to enable immutable links ? At best they're no-op \nthat should be removed, and at worst they're bugs and should be fixed.\n\n> > > + *\n> > > + * Enabling a link establishes a data connection between two pads,\n> > > while\n> > > + * disabling it interrupts such a connections.\n> > > + *\n> > > + * \\return 0 for success, negative error code otherwise\n\nI think we usually write this as \"0 on success, or a negative error code \notherwise\".\n\n> > > + */\n> > > +int MediaLink::enable(bool enable)\n> > > +{\n> > > +\tunsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;\n> > > +\n> > > +\tif ((flags_ & MEDIA_LNK_FL_IMMUTABLE) && !enable) {\n> > > +\t\tLOG(Error) << \"Immutable links cannot be disabled\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > \n> > Is it sound to relay on cached values here? What if the kernel have\n> > changed the state of the link to once more be mutable (not sure any\n> > drivers to this today)?\n\nNo driver does this, and the spec doesn't really allow it.\n\n> > I would drop this check and let the kernel deal with this and relay on the\n> > return code from the MEDIA_IOC_SETUP_LINK ioctl.\n\nWe indeed don't really need to optimize the error code path.\n\n> > > +\n> > > +\t/* Do not try to enable an already enabled link. */\n> > > +\tif ((flags_ & MEDIA_LNK_FL_ENABLED) && enable)\n> > > +\t\treturn 0;\n> > \n> > Same here, I would not trust the cached value. What if another process\n> > have modified the media graph unknown to the library? Better let the\n> > request happen and let the kernel deal with it.\n\nWe're likely screwed in that case anyway, but I agree there's no real need to \noptimize this.\n\n> > > +\n> > > +\tint ret = media_->setLink(source_, sink_, flags);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\t/* Only update flags if 'setLink()' didn't fail. */\n\nI think you can drop this comment.\n\n> > > +\tflags_ = flags;\n> > > +\n> > > +\treturn 0;\n> > \n> > I think you can drop the comment and change this to:\n\nAh indeed :-)\n\n> > int ret = media_->setLink(source_, sink_, flags);\n> > \n> > if (!ret)\n> > flags_ = flags;\n\nI would have kept the existing test though, I find the code more readable when \nerror paths returns as early as possible. It also makes it easier to later add \ncode between the setLink() call and the flags_ assignment.\n\n> > return ret;\n> > > \n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Construct a MediaLink\n> > > * \\param media The media device this entity belongs to","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 1045C600CC\n\tfor ;\n\tMon, 7 Jan 2019 22:49:43 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69F6DE22;\n\tMon, 7 Jan 2019 22:49:42 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546897782;\n\tbh=V0XsQXRmerMwpKxzasYFP8lbnCWCoLw8HmAa2JP5XcU=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=V2KS0FvTzQdJ0fc6fDHOpp7NBalTcogp6AXegKqK5Xl1t++0G0dWjGJ3i65czgeEW\n\tTj0lqXXkG5JTvGABPdIj5G8gOTjn9HFOpkINxnA8keyznV2U8LvDGOMCznd1X0xIAP\n\teIolTo2dqnfQkltC+B1R8+tADou//QbpW4JK76P4=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Mon, 07 Jan 2019 23:50:49 +0200","Message-ID":"<1591398.Nf03XtP9Ca@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190107084117.rzuu3dh2o45z2tyq@uno.localdomain>","References":"<20190103173859.22624-1-jacopo@jmondi.org>\n\t<20190104164509.GN22790@bigcity.dyn.berto.se>\n\t<20190107084117.rzuu3dh2o45z2tyq@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: Add MediaLink link\n\tsetup function","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, 07 Jan 2019 21:49:43 -0000"}}]