[{"id":21589,"web_url":"https://patchwork.libcamera.org/comment/21589/","msgid":"","date":"2021-12-04T00:12:29","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: media_device:\n\tHandle ancillary links in populateLinks()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Daniel,\n\nThank you for the patch.\n\nOn Fri, Dec 03, 2021 at 10:42:25PM +0000, Daniel Scally wrote:\n> The populateLinks() function can't currently handle ancillary links\n> which causes an error to be thrown in process() when the MediaObject\n> cannot be cast to a MediaPad.\n> \n> Add explicit handling for the different link types, creating either\n> pad-2-pad links or else storing the pointer to the ancillary device\n> MediaEntity in the ancillaryEntities_ member of the primary.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> Changes in v2:\n> \n> \t- Storing pointer to the sink entity in the new ancillaryEntities_\n> \tvector of the source entity where in case MEDIA_LNK_FL_ANCILLARY_LINK\n> \trather than creating a MediaLink (Laurent)\n> \n> include/linux/media.h | 1 +\n> src/libcamera/media_device.cpp | 46 +++++++++++++++++++++++-----------\n> 2 files changed, 32 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/linux/media.h b/include/linux/media.h\n> index 17432318..e3123d1a 100644\n> --- a/include/linux/media.h\n> +++ b/include/linux/media.h\n> @@ -222,6 +222,7 @@ struct media_pad_desc {\n> #define MEDIA_LNK_FL_LINK_TYPE\t\t\t(0xf << 28)\n> # define MEDIA_LNK_FL_DATA_LINK\t\t(0 << 28)\n> # define MEDIA_LNK_FL_INTERFACE_LINK\t\t(1 << 28)\n> +# define MEDIA_LNK_FL_ANCILLARY_LINK\t\t(2 << 28)\n\nThis should be split to a separate patch. You can run git log on\ninclude/linux/media.h to see how we usually handle kernel header\nupdates.\n\n> struct media_link_desc {\n> \tstruct media_pad_desc source;\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index aa93da75..6eddb5a0 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -699,45 +699,61 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> {\n> \tstruct media_v2_link *mediaLinks = reinterpret_cast\n> \t\t\t\t\t (topology.ptr_links);\n> +\tunsigned int link_type;\n\nlibcamera uses camelCase, not snake_case.\n\n> +\tMediaLink *link;\n> \n> \tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> \t\t/*\n> \t\t * Skip links between entities and interfaces: we only care\n> -\t\t * about pad-2-pad links here.\n> +\t\t * about pad-2-pad and entity-2-entity links here.\n> \t\t */\n> \t\tif ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) ==\n> \t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> \t\t\tcontinue;\n\nShould this be moved to the switch-case below to avoid a special case ?\n\n> \n> -\t\t/* Store references to source and sink pads in the link. */\n> +\t\t/* Store references to source and sink objects in the link. */\n\n\t\t/* Look up the source and sink objects. */\n\nas you don't necessarily store them in a link anymore.\n\n> \t\tunsigned int source_id = mediaLinks[i].source_id;\n> -\t\tMediaPad *source = dynamic_cast\n> -\t\t\t\t (object(source_id));\n> +\t\tMediaObject *source = object(source_id);\n> \t\tif (!source) {\n> \t\t\tLOG(MediaDevice, Error)\n> -\t\t\t\t<< \"Failed to find pad with id: \"\n> +\t\t\t\t<< \"Failed to find MediaObject with id: \"\n> \t\t\t\t<< source_id;\n> \t\t\treturn false;\n> \t\t}\n> \n> \t\tunsigned int sink_id = mediaLinks[i].sink_id;\n> -\t\tMediaPad *sink = dynamic_cast\n> -\t\t\t\t (object(sink_id));\n> +\t\tMediaObject *sink = object(sink_id);\n> \t\tif (!sink) {\n> \t\t\tLOG(MediaDevice, Error)\n> -\t\t\t\t<< \"Failed to find pad with id: \"\n> +\t\t\t\t<< \"Failed to find MediaObject with id: \"\n> \t\t\t\t<< sink_id;\n> \t\t\treturn false;\n> \t\t}\n> \n> -\t\tMediaLink *link = new MediaLink(&mediaLinks[i], source, sink);\n> -\t\tif (!addObject(link)) {\n> -\t\t\tdelete link;\n> -\t\t\treturn false;\n> -\t\t}\n> +\t\tlink_type = mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE;\n> +\n> +\t\tswitch (link_type) {\n\n\t\tswitch (mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) {\n\n> +\t\tcase MEDIA_LNK_FL_DATA_LINK:\n> +\t\t\tlink = new MediaLink(&mediaLinks[i],\n> +\t\t\t\t\t dynamic_cast(source),\n> +\t\t\t\t\t dynamic_cast(sink));\n> +\t\t\tif (!addObject(link)) {\n> +\t\t\t\tdelete link;\n> +\t\t\t\treturn false;\n> +\t\t\t}\n> +\n> +\t\t\tlink->source()->addLink(link);\n> +\t\t\tlink->sink()->addLink(link);\n> \n> -\t\tsource->addLink(link);\n> -\t\tsink->addLink(link);\n> +\t\t\tbreak;\n\nMissing blank line.\n\n> +\t\tcase MEDIA_LNK_FL_ANCILLARY_LINK:\n> +\t\t\tMediaEntity *primary = dynamic_cast(source);\n> +\t\t\tMediaEntity *ancillary = dynamic_cast(sink);\n> +\n> +\t\t\tprimary->addAncillaryEntity(ancillary);\n> +\n> +\t\t\tbreak;\n\nShould we add a default case and log a warning ?\n\n> +\t\t}\n> \t}\n> \n> \treturn true;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9C119BF415\n\tfor ;\n\tSat, 4 Dec 2021 00:12:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1125660832;\n\tSat, 4 Dec 2021 01:12:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2FF106011A\n\tfor ;\n\tSat, 4 Dec 2021 01:12:57 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9CC97BA9;\n\tSat, 4 Dec 2021 01:12:56 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mYj7BqOm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638576776;\n\tbh=zKVTPFYMRVOoz/+m77UDr7ZecwTahzvRYheEDvu+AT8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mYj7BqOmS3OmZmLw/PYo+bDb3jSyBXnZN4stdeJEQSr8ntfNioC+zmknBl+/BgeoG\n\tGP/e37ARRpsEOWG0JUzA0RqMqOHDCZSsNxueNJarm4z3FJzRD7q1+HSWB1Kllf3w6m\n\tSmgHf0DRNIwV2G9YoeOQihgSAS7IFufGCpWRmDQE=","Date":"Sat, 4 Dec 2021 02:12:29 +0200","From":"Laurent Pinchart ","To":"Daniel Scally ","Message-ID":"","References":"<20211203224230.38700-1-djrscally@gmail.com>\n\t<20211203224230.38700-3-djrscally@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211203224230.38700-3-djrscally@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: media_device:\n\tHandle ancillary links in populateLinks()","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21597,"web_url":"https://patchwork.libcamera.org/comment/21597/","msgid":"<454e081c-d188-f097-f2ec-a0d105b3782f@gmail.com>","date":"2021-12-04T21:40:21","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: media_device:\n\tHandle ancillary links in populateLinks()","submitter":{"id":90,"url":"https://patchwork.libcamera.org/api/people/90/","name":"Daniel Scally","email":"djrscally@gmail.com"},"content":"Hi Laurent\n\nOn 04/12/2021 00:12, Laurent Pinchart wrote:\n> Hi Daniel,\n> \n> Thank you for the patch.\n> \n> On Fri, Dec 03, 2021 at 10:42:25PM +0000, Daniel Scally wrote:\n>> The populateLinks() function can't currently handle ancillary links\n>> which causes an error to be thrown in process() when the MediaObject\n>> cannot be cast to a MediaPad.\n>>\n>> Add explicit handling for the different link types, creating either\n>> pad-2-pad links or else storing the pointer to the ancillary device\n>> MediaEntity in the ancillaryEntities_ member of the primary.\n>>\n>> Signed-off-by: Daniel Scally \n>> ---\n>> Changes in v2:\n>>\n>> \t- Storing pointer to the sink entity in the new ancillaryEntities_\n>> \tvector of the source entity where in case MEDIA_LNK_FL_ANCILLARY_LINK\n>> \trather than creating a MediaLink (Laurent)\n>>\n>> include/linux/media.h | 1 +\n>> src/libcamera/media_device.cpp | 46 +++++++++++++++++++++++-----------\n>> 2 files changed, 32 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/include/linux/media.h b/include/linux/media.h\n>> index 17432318..e3123d1a 100644\n>> --- a/include/linux/media.h\n>> +++ b/include/linux/media.h\n>> @@ -222,6 +222,7 @@ struct media_pad_desc {\n>> #define MEDIA_LNK_FL_LINK_TYPE\t\t\t(0xf << 28)\n>> # define MEDIA_LNK_FL_DATA_LINK\t\t(0 << 28)\n>> # define MEDIA_LNK_FL_INTERFACE_LINK\t\t(1 << 28)\n>> +# define MEDIA_LNK_FL_ANCILLARY_LINK\t\t(2 << 28)\n> \n> This should be split to a separate patch.You can run git log on\n> include/linux/media.h to see how we usually handle kernel header\n> updates.\n\nNo problem. I did wonder if this was something that would have to wait\nuntil it was merged kernelside, but it does look like a few manual\nadditions have been made (and kept) when the headers were upgraded to\nthe newest kernel version.\n\n> \n>> struct media_link_desc {\n>> \tstruct media_pad_desc source;\n>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n>> index aa93da75..6eddb5a0 100644\n>> --- a/src/libcamera/media_device.cpp\n>> +++ b/src/libcamera/media_device.cpp\n>> @@ -699,45 +699,61 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n>> {\n>> \tstruct media_v2_link *mediaLinks = reinterpret_cast\n>> \t\t\t\t\t (topology.ptr_links);\n>> +\tunsigned int link_type;\n> \n> libcamera uses camelCase, not snake_case.\n\nAck\n> \n>> +\tMediaLink *link;\n>> \n>> \tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n>> \t\t/*\n>> \t\t * Skip links between entities and interfaces: we only care\n>> -\t\t * about pad-2-pad links here.\n>> +\t\t * about pad-2-pad and entity-2-entity links here.\n>> \t\t */\n>> \t\tif ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) ==\n>> \t\t MEDIA_LNK_FL_INTERFACE_LINK)\n>> \t\t\tcontinue;\n> \n> Should this be moved to the switch-case below to avoid a special case ?\n\nYup, not sure why I didn't see that...\n\nAck, to the rest of the comments here.\n> \n>> \n>> -\t\t/* Store references to source and sink pads in the link. */\n>> +\t\t/* Store references to source and sink objects in the link. */\n> \n> \t\t/* Look up the source and sink objects. */\n> \n> as you don't necessarily store them in a link anymore.\n> \n>> \t\tunsigned int source_id = mediaLinks[i].source_id;\n>> -\t\tMediaPad *source = dynamic_cast\n>> -\t\t\t\t (object(source_id));\n>> +\t\tMediaObject *source = object(source_id);\n>> \t\tif (!source) {\n>> \t\t\tLOG(MediaDevice, Error)\n>> -\t\t\t\t<< \"Failed to find pad with id: \"\n>> +\t\t\t\t<< \"Failed to find MediaObject with id: \"\n>> \t\t\t\t<< source_id;\n>> \t\t\treturn false;\n>> \t\t}\n>> \n>> \t\tunsigned int sink_id = mediaLinks[i].sink_id;\n>> -\t\tMediaPad *sink = dynamic_cast\n>> -\t\t\t\t (object(sink_id));\n>> +\t\tMediaObject *sink = object(sink_id);\n>> \t\tif (!sink) {\n>> \t\t\tLOG(MediaDevice, Error)\n>> -\t\t\t\t<< \"Failed to find pad with id: \"\n>> +\t\t\t\t<< \"Failed to find MediaObject with id: \"\n>> \t\t\t\t<< sink_id;\n>> \t\t\treturn false;\n>> \t\t}\n>> \n>> -\t\tMediaLink *link = new MediaLink(&mediaLinks[i], source, sink);\n>> -\t\tif (!addObject(link)) {\n>> -\t\t\tdelete link;\n>> -\t\t\treturn false;\n>> -\t\t}\n>> +\t\tlink_type = mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE;\n>> +\n>> +\t\tswitch (link_type) {\n> \n> \t\tswitch (mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) {\n> \n>> +\t\tcase MEDIA_LNK_FL_DATA_LINK:\n>> +\t\t\tlink = new MediaLink(&mediaLinks[i],\n>> +\t\t\t\t\t dynamic_cast(source),\n>> +\t\t\t\t\t dynamic_cast(sink));\n>> +\t\t\tif (!addObject(link)) {\n>> +\t\t\t\tdelete link;\n>> +\t\t\t\treturn false;\n>> +\t\t\t}\n>> +\n>> +\t\t\tlink->source()->addLink(link);\n>> +\t\t\tlink->sink()->addLink(link);\n>> \n>> -\t\tsource->addLink(link);\n>> -\t\tsink->addLink(link);\n>> +\t\t\tbreak;\n> \n> Missing blank line.\n> \n>> +\t\tcase MEDIA_LNK_FL_ANCILLARY_LINK:\n>> +\t\t\tMediaEntity *primary = dynamic_cast(source);\n>> +\t\t\tMediaEntity *ancillary = dynamic_cast(sink);\n>> +\n>> +\t\t\tprimary->addAncillaryEntity(ancillary);\n>> +\n>> +\t\t\tbreak;\n> \n> Should we add a default case and log a warning ?\n> \n>> +\t\t}\n>> \t}\n>> \n>> \treturn true;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6678BBF415\n\tfor ;\n\tSat, 4 Dec 2021 21:40:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE81C60828;\n\tSat, 4 Dec 2021 22:40:25 +0100 (CET)","from mail-wr1-x432.google.com (mail-wr1-x432.google.com\n\t[IPv6:2a00:1450:4864:20::432])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D6BF2605C4\n\tfor ;\n\tSat, 4 Dec 2021 22:40:23 +0100 (CET)","by mail-wr1-x432.google.com with SMTP id a18so13660021wrn.6\n\tfor ;\n\tSat, 04 Dec 2021 13:40:23 -0800 (PST)","from [192.168.0.16]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net. [86.13.91.161])\n\tby smtp.gmail.com with ESMTPSA id\n\tj40sm6683957wms.16.2021.12.04.13.40.22\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSat, 04 Dec 2021 13:40:22 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"oul8r5Mc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=O0OvXRaLxEXKt6c6XhOlXXzOtdcGZipmne54mHKlz7c=;\n\tb=oul8r5MccDur3UVtU+kDk5LVyJBflr/FTHu3vkUQES0bZB/d5115yPonImUFgYN/9I\n\tShdTPE2Ez21CDKacxTp1MpmH6wepgPRwbGuEdQ2rngm6qTxSfrCkAoSnDmFqeRsBuBX+\n\tuA4wWkDqr8rz8VoZFYP+F7ktKmNToHfoiJ3r/xyaqLuP/xZiBzzLcOTu4zbfAZUPc7GH\n\taprVRLqobS8yA6fsNNB+mX5j1AGheSExwayIlxedCZ2dwljNtR9SJ3oDw53BOMKIzqA9\n\tjkAzp92o917Bx0qtyVjtUWkkLY1yFvL2Zej+jYyBwKArU+VoPXHoP+YowjwNPO2nVHlf\n\tfawA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=O0OvXRaLxEXKt6c6XhOlXXzOtdcGZipmne54mHKlz7c=;\n\tb=vSPk3FXXuJqCXehsQCtBNdXeSgecAJpuAhQ//ps68/hve0jY6Zshr7mD9Mobl0m2pE\n\tJDV0zmg0xjsd+LvKpxLmR5gFHjh0TUgPjkgh9jr5A57rrYFytvbrNR4sK+HP/Fo3LASo\n\tVrJY7BGPXl6/sdO/Kcufyp4jaWjMgwD7Ur0wAjCvDVQpPNFlSM6THSliovhSwYas0ovL\n\tKVqf40vjhX9EdRJR/jwT2YXsnx/EEdcT1puJ4CqJLm5jmuvbeErN30/ryBZys1bWqq+z\n\tOv2M3saZaieL7KKaC8YYve/F/L0+vHlW7qHxS0lqdDeQYAIBOtQJUWwJs7eI7bhnt7UB\n\tyrJQ==","X-Gm-Message-State":"AOAM530WzWSOcTMHte6Ww52RYycdd6MMPnYa04XoArysyJy2plOOGjpt\n\tl1JupQgR3cywMi5+Je5QtfHvtrWwnko=","X-Google-Smtp-Source":"ABdhPJxI167ojkR1YSgTP07EgXnsHDmlD7/jgIIrGGdrRGPYSmnDrXtllurfwsZIaFXQI3BcG9+QPQ==","X-Received":"by 2002:adf:9d88:: with SMTP id\n\tp8mr32628103wre.140.1638654023406; \n\tSat, 04 Dec 2021 13:40:23 -0800 (PST)","To":"Laurent Pinchart ","References":"<20211203224230.38700-1-djrscally@gmail.com>\n\t<20211203224230.38700-3-djrscally@gmail.com>\n\t","From":"Daniel Scally ","Message-ID":"<454e081c-d188-f097-f2ec-a0d105b3782f@gmail.com>","Date":"Sat, 4 Dec 2021 21:40:21 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.14.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: media_device:\n\tHandle ancillary links in populateLinks()","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]