[{"id":30289,"web_url":"https://patchwork.libcamera.org/comment/30289/","msgid":"<172008177189.3353312.1466426302104264369@ping.linuxembedded.co.uk>","date":"2024-07-04T08:29:31","subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hans,\n\nSo the IPU6 enablement in the simple pipeline handler is straight\nforward ... lets see what we can do here.\n\nQuoting Hans de Goede (2024-07-03 16:09:22)\n> The /dev/video# nodes of the IPU6 driver set both V4L2_CAP_VIDEO_CAPTURE\n> and V4L2_CAP_META_CAPTURE.\n\nIs this a driver that is already upstream?\n\nIs it really valid to set both of those in the V4L2 spec?\n\nIf so - then we have to do something here... if not - what control do we\nhave to fix the current driver?\n\n> This was resulting in V4L2VideoDevice::getFormat() / tryFormat() /\n> setFormat() calling the metadata related ioctls instead of the videocap\n> ioctls causing things to not work.\n> \n> Fix this by making V4L2Capability::isMeta() return false when the device\n> also has one of the video capabilities.\n> \n> Signed-off-by: Hans de Goede \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 9057be08..fdd877c8 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -92,6 +92,10 @@ struct V4L2Capability final : v4l2_capability {\n> }\n> bool isMeta() const\n> {\n> + /* Treat devs which combine video and meta as video not meta */\n\nIf this is a workaround, and really not expected I would want more\ndetail here. Especially if this is a short term workaround for any\npossibly out of tree or staging drivers (I haven't checked where IPU6\nstate is currently), including a \\todo: to remove it in the future\nwhen the driver is fixed/upstreamed.\n\nHowevfer if it's simply entirely within the rights of a v4l2 driver to\nset both - then I think the comment would stand and be valid on it's\nown, but some how we'd have to stay aware or document somewhere else in\nthe doxygen for this class that we directly treat devices as video\ndevices if they report both video and metadata capture.\n\n--\nKieran\n\n> + if (isVideo())\n> + return false;\n> +\n> return device_caps() & (V4L2_CAP_META_CAPTURE |\n> V4L2_CAP_META_OUTPUT);\n> }\n> -- \n> 2.45.1\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 BBFEABEFBE\n\tfor ;\n\tThu, 4 Jul 2024 08:29:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DABD362C99;\n\tThu, 4 Jul 2024 10:29:36 +0200 (CEST)","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 83D2B619C4\n\tfor ;\n\tThu, 4 Jul 2024 10:29:34 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC1E6502;\n\tThu, 4 Jul 2024 10:29:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EbmeWAGD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720081745;\n\tbh=gmOpNN2q8Qz8GLlUn106Ag+qEQWyJi3AJrzZBaaYGRI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=EbmeWAGD3UPQ7XAmwBT/snufh4ILrc5r7IT+Zq/8HjVEC90vo2WZ2YmEv+J6pMaDr\n\tWQrwZuC8/83Xruudm/LesY6XN5nTbM/dLXB1sAcs1vWjtxChMj9LI1dKNwa1w20qJE\n\thg3oZepbQOZ+AzbYsTVy1AkCSxESI6XaXreXU5AY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240703150923.25994-1-hdegoede@redhat.com>","References":"<20240703150923.25994-1-hdegoede@redhat.com>","Subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","From":"Kieran Bingham ","Cc":"Milan Zamazal , Maxime Ripard , \n\tHans de Goede ","To":"Hans de Goede , libcamera-devel@lists.libcamera.org","Date":"Thu, 04 Jul 2024 09:29:31 +0100","Message-ID":"<172008177189.3353312.1466426302104264369@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30317,"web_url":"https://patchwork.libcamera.org/comment/30317/","msgid":"<20240704103709.GC10099@pendragon.ideasonboard.com>","date":"2024-07-04T10:37:09","subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jul 04, 2024 at 09:29:31AM +0100, Kieran Bingham wrote:\n> Hi Hans,\n> \n> So the IPU6 enablement in the simple pipeline handler is straight\n> forward ... lets see what we can do here.\n> \n> Quoting Hans de Goede (2024-07-03 16:09:22)\n> > The /dev/video# nodes of the IPU6 driver set both V4L2_CAP_VIDEO_CAPTURE\n> > and V4L2_CAP_META_CAPTURE.\n> \n> Is this a driver that is already upstream?\n\nIt's the ISYS (input subsystem, CSI-2 RX) driver, it will be in v6.10.\nThe PSYS (processing subsystem, ISP) driver isn't anywhere close to\nmainline yet.\n\n> Is it really valid to set both of those in the V4L2 spec?\n> \n> If so - then we have to do something here... if not - what control do we\n> have to fix the current driver?\n\nIt's not forbidden at least. The ISYS has multiple DMA engines connected\nto each CSI-2 RX, and they're not dedicated to image data or embedded\ndata. Streams are filtered based on VC/DT, and routed to those generic\nDMA engines.\n\nIt's ugly though. The driver reports both capabilities, it maintains\nseparate image and metadata formats that can be accessed through ioctl\nhandlers, and selects the effective vb2 queue type when VIDIOC_REQBUFS\nis called.\n\n> > This was resulting in V4L2VideoDevice::getFormat() / tryFormat() /\n> > setFormat() calling the metadata related ioctls instead of the videocap\n> > ioctls causing things to not work.\n> > \n> > Fix this by making V4L2Capability::isMeta() return false when the device\n> > also has one of the video capabilities.\n> > \n> > Signed-off-by: Hans de Goede \n> > ---\n> > include/libcamera/internal/v4l2_videodevice.h | 4 ++++\n> > 1 file changed, 4 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index 9057be08..fdd877c8 100644\n> > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > @@ -92,6 +92,10 @@ struct V4L2Capability final : v4l2_capability {\n> > }\n> > bool isMeta() const\n> > {\n> > + /* Treat devs which combine video and meta as video not meta */\n> \n> If this is a workaround, and really not expected I would want more\n> detail here. Especially if this is a short term workaround for any\n> possibly out of tree or staging drivers (I haven't checked where IPU6\n> state is currently), including a \\todo: to remove it in the future\n> when the driver is fixed/upstreamed.\n> \n> Howevfer if it's simply entirely within the rights of a v4l2 driver to\n> set both - then I think the comment would stand and be valid on it's\n> own, but some how we'd have to stay aware or document somewhere else in\n> the doxygen for this class that we directly treat devices as video\n> devices if they report both video and metadata capture.\n\nThat won't be enough as soon as we need to capture embedded data from\nthe ISYS. We need to extend the V4L2 helper classes to allow selecting\nthe type, among the supported types. I fear this mode of operation,\nwhile supported by the kernel, has seen little testing beyond\napplications that are highly specific to particular devices, so I expect\nrough edges.\n\n> > + if (isVideo())\n> > + return false;\n> > +\n> > return device_caps() & (V4L2_CAP_META_CAPTURE |\n> > V4L2_CAP_META_OUTPUT);\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 D1EE6BEFBE\n\tfor ;\n\tThu, 4 Jul 2024 10:37:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9897A62E24;\n\tThu, 4 Jul 2024 12:37:33 +0200 (CEST)","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 2C57A619C8\n\tfor ;\n\tThu, 4 Jul 2024 12:37:31 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F7E363D;\n\tThu, 4 Jul 2024 12:37:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nOxs5d4G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720089422;\n\tbh=JYGuZ6OUMBLj+02gfyeDx9iUMmEBxP3YvR24GdXEorU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nOxs5d4GGOlCXENJ8Aa3V3cvZph/R73TFeHA4QQ6MRt8hLt3rjWzv0R3rqmCXqx6v\n\tC1UeCa8t36fpsAJZEDiQBQBvkTcVfbQ2YHkEa9DuStv6lO49AbKd4ap5m26/RbFJb9\n\tKOc26mfLnI9le6xojSEJVbam8B5oyChaCCb/mNKo=","Date":"Thu, 4 Jul 2024 13:37:09 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , Maxime Ripard ","Subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","Message-ID":"<20240704103709.GC10099@pendragon.ideasonboard.com>","References":"<20240703150923.25994-1-hdegoede@redhat.com>\n\t<172008177189.3353312.1466426302104264369@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172008177189.3353312.1466426302104264369@ping.linuxembedded.co.uk>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30319,"web_url":"https://patchwork.libcamera.org/comment/30319/","msgid":"","date":"2024-07-04T10:48:49","subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 4 Jul 2024 at 11:37, Laurent Pinchart\n wrote:\n>\n> On Thu, Jul 04, 2024 at 09:29:31AM +0100, Kieran Bingham wrote:\n> > Hi Hans,\n> >\n> > So the IPU6 enablement in the simple pipeline handler is straight\n> > forward ... lets see what we can do here.\n> >\n> > Quoting Hans de Goede (2024-07-03 16:09:22)\n> > > The /dev/video# nodes of the IPU6 driver set both V4L2_CAP_VIDEO_CAPTURE\n> > > and V4L2_CAP_META_CAPTURE.\n> >\n> > Is this a driver that is already upstream?\n>\n> It's the ISYS (input subsystem, CSI-2 RX) driver, it will be in v6.10.\n> The PSYS (processing subsystem, ISP) driver isn't anywhere close to\n> mainline yet.\n\nThe original proposal for the Pi5 CFE driver also did this, but we\nreverted back to a single queue type per-node because of the ambiguity\nintroduced in the V4L2VideoDevice class.\n\nNaush\n\n>\n> > Is it really valid to set both of those in the V4L2 spec?\n> >\n> > If so - then we have to do something here... if not - what control do we\n> > have to fix the current driver?\n>\n> It's not forbidden at least. The ISYS has multiple DMA engines connected\n> to each CSI-2 RX, and they're not dedicated to image data or embedded\n> data. Streams are filtered based on VC/DT, and routed to those generic\n> DMA engines.\n>\n> It's ugly though. The driver reports both capabilities, it maintains\n> separate image and metadata formats that can be accessed through ioctl\n> handlers, and selects the effective vb2 queue type when VIDIOC_REQBUFS\n> is called.\n>\n> > > This was resulting in V4L2VideoDevice::getFormat() / tryFormat() /\n> > > setFormat() calling the metadata related ioctls instead of the videocap\n> > > ioctls causing things to not work.\n> > >\n> > > Fix this by making V4L2Capability::isMeta() return false when the device\n> > > also has one of the video capabilities.\n> > >\n> > > Signed-off-by: Hans de Goede \n> > > ---\n> > > include/libcamera/internal/v4l2_videodevice.h | 4 ++++\n> > > 1 file changed, 4 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > index 9057be08..fdd877c8 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -92,6 +92,10 @@ struct V4L2Capability final : v4l2_capability {\n> > > }\n> > > bool isMeta() const\n> > > {\n> > > + /* Treat devs which combine video and meta as video not meta */\n> >\n> > If this is a workaround, and really not expected I would want more\n> > detail here. Especially if this is a short term workaround for any\n> > possibly out of tree or staging drivers (I haven't checked where IPU6\n> > state is currently), including a \\todo: to remove it in the future\n> > when the driver is fixed/upstreamed.\n> >\n> > Howevfer if it's simply entirely within the rights of a v4l2 driver to\n> > set both - then I think the comment would stand and be valid on it's\n> > own, but some how we'd have to stay aware or document somewhere else in\n> > the doxygen for this class that we directly treat devices as video\n> > devices if they report both video and metadata capture.\n>\n> That won't be enough as soon as we need to capture embedded data from\n> the ISYS. We need to extend the V4L2 helper classes to allow selecting\n> the type, among the supported types. I fear this mode of operation,\n> while supported by the kernel, has seen little testing beyond\n> applications that are highly specific to particular devices, so I expect\n> rough edges.\n>\n> > > + if (isVideo())\n> > > + return false;\n> > > +\n> > > return device_caps() & (V4L2_CAP_META_CAPTURE |\n> > > V4L2_CAP_META_OUTPUT);\n> > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 BEB76BD87C\n\tfor ;\n\tThu, 4 Jul 2024 10:49:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7048162E24;\n\tThu, 4 Jul 2024 12:49:29 +0200 (CEST)","from mail-yw1-x112a.google.com (mail-yw1-x112a.google.com\n\t[IPv6:2607:f8b0:4864:20::112a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5569D619C8\n\tfor ;\n\tThu, 4 Jul 2024 12:49:27 +0200 (CEST)","by mail-yw1-x112a.google.com with SMTP id\n\t00721157ae682-65240d22f7cso4476937b3.0\n\tfor ;\n\tThu, 04 Jul 2024 03:49:27 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"rD2lUmoD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1720090166; x=1720694966;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=jtL0/NzSMRVN0+UBqo3HxeDycOYAjzR5wOifJRyEtQc=;\n\tb=rD2lUmoDZW/d49sYYZsO3US1d/DLWO1dW1lzkFIMYZK1QReBNlbijkWKqQRthcs9t4\n\tLJWAY09gy4YXxn7L6660izKm1v0YB+vxj1AuzymZSHLk70VJbHbkIpicXYZrL6dls6NF\n\tU3KaUM9nfeOmD6f4glR/hIJaMlDw2rzTkjOdg9SShsQPzP/rmDcvmnthe4S5pODkIyqT\n\tAjTU5J78Bw3I5TVv21i5KPGWeB+d7wqkcXfhID/4iQnJyaOVEHS2ChBzENgJNXaEbGh1\n\tiQGNwJYYCXd5bTCmykeOC1VXz/lher+BV8iNgTrkqYjohd0JwTxg8pwHNf9XVBQEjbTO\n\t9ssQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1720090166; x=1720694966;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=jtL0/NzSMRVN0+UBqo3HxeDycOYAjzR5wOifJRyEtQc=;\n\tb=T+8xYkzD/JBekAN5zrjLy3Zb/z5IkKwORRTPJA1M41xfhGxovcfOrTIJgXLYKbgngD\n\txiDoYl67UphcGtcUZJyFyVdMJg7XGSywA0o03eYbHC2svgHX8d3Gc/yHYkaxOYTOOQhF\n\tOGWPwoygnLAEMqY2hU0x06/etNnRXjMNMFQu5ELx+ZPupibkAWcWsQ93Ohy4du7XMXWy\n\tgrT+dflEPdC6bIEqFRpHRFOSNtBZt8pP/iX6n+EvgIZ2hdpShFac8gwHCftp/L0SokvW\n\thTIrGOCqicSLrWGvft1nqhCBQ11EoADHE1UH7TDz/4lFS65Q9FobEKI6In7nSpjWKqQY\n\tW37g==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWFv+7EwjOC+8BUdjefBuDdem82pVrFxcNDCxm6gvvoiZxwB7PfU2deTqHsttlJ4Jfk6Sm2nJBH31GReIg4p1yPQ6HBJF66FckLGK3jL7gWXthAVg==","X-Gm-Message-State":"AOJu0YxYk73IBD2exxN0hdAqUeb6JL4u6fURs0H+q6FQtjmweRwHzfmU\n\tCTLnxJ1mTLI420/FWxeoeZO71vQhxy26ViVtgR5Pc9VqNqA5r1+WFqGXVWm+U6iSyEHp7ddvUtD\n\tHwveaAJSrddfUVIURyYPNsZpvCzzXxeuYC/g9ow==","X-Google-Smtp-Source":"AGHT+IEYQMZuaALctoDjTdQFixelZHzXO7jidZqysT7YcsIQMsVyPwVLBYCCWU83AX7fpNIMqZ9bBSxpRh3GMXErdW8=","X-Received":"by 2002:a81:9215:0:b0:64b:8b8f:7770 with SMTP id\n\t00721157ae682-652d60ecec4mr12208337b3.23.1720090166111;\n\tThu, 04 Jul 2024 03:49:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20240703150923.25994-1-hdegoede@redhat.com>\n\t<172008177189.3353312.1466426302104264369@ping.linuxembedded.co.uk>\n\t<20240704103709.GC10099@pendragon.ideasonboard.com>","In-Reply-To":"<20240704103709.GC10099@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Thu, 4 Jul 2024 11:48:49 +0100","Message-ID":"","Subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tHans de Goede , \n\tlibcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , Maxime Ripard ","Content-Type":"text/plain; charset=\"UTF-8\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30325,"web_url":"https://patchwork.libcamera.org/comment/30325/","msgid":"<4f770ad8-e02c-487d-9fa9-18154802de9c@redhat.com>","date":"2024-07-04T11:03:58","subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 7/4/24 12:37 PM, Laurent Pinchart wrote:\n> On Thu, Jul 04, 2024 at 09:29:31AM +0100, Kieran Bingham wrote:\n>> Hi Hans,\n>>\n>> So the IPU6 enablement in the simple pipeline handler is straight\n>> forward ... lets see what we can do here.\n>>\n>> Quoting Hans de Goede (2024-07-03 16:09:22)\n>>> The /dev/video# nodes of the IPU6 driver set both V4L2_CAP_VIDEO_CAPTURE\n>>> and V4L2_CAP_META_CAPTURE.\n>>\n>> Is this a driver that is already upstream?\n> \n> It's the ISYS (input subsystem, CSI-2 RX) driver, it will be in v6.10.\n> The PSYS (processing subsystem, ISP) driver isn't anywhere close to\n> mainline yet.\n> \n>> Is it really valid to set both of those in the V4L2 spec?\n>>\n>> If so - then we have to do something here... if not - what control do we\n>> have to fix the current driver?\n> \n> It's not forbidden at least. The ISYS has multiple DMA engines connected\n> to each CSI-2 RX, and they're not dedicated to image data or embedded\n> data. Streams are filtered based on VC/DT, and routed to those generic\n> DMA engines.\n> \n> It's ugly though. The driver reports both capabilities, it maintains\n> separate image and metadata formats that can be accessed through ioctl\n> handlers, and selects the effective vb2 queue type when VIDIOC_REQBUFS\n> is called.\n> \n>>> This was resulting in V4L2VideoDevice::getFormat() / tryFormat() /\n>>> setFormat() calling the metadata related ioctls instead of the videocap\n>>> ioctls causing things to not work.\n>>>\n>>> Fix this by making V4L2Capability::isMeta() return false when the device\n>>> also has one of the video capabilities.\n>>>\n>>> Signed-off-by: Hans de Goede \n>>> ---\n>>> include/libcamera/internal/v4l2_videodevice.h | 4 ++++\n>>> 1 file changed, 4 insertions(+)\n>>>\n>>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n>>> index 9057be08..fdd877c8 100644\n>>> --- a/include/libcamera/internal/v4l2_videodevice.h\n>>> +++ b/include/libcamera/internal/v4l2_videodevice.h\n>>> @@ -92,6 +92,10 @@ struct V4L2Capability final : v4l2_capability {\n>>> }\n>>> bool isMeta() const\n>>> {\n>>> + /* Treat devs which combine video and meta as video not meta */\n>>\n>> If this is a workaround, and really not expected I would want more\n>> detail here. Especially if this is a short term workaround for any\n>> possibly out of tree or staging drivers (I haven't checked where IPU6\n>> state is currently), including a \\todo: to remove it in the future\n>> when the driver is fixed/upstreamed.\n>>\n>> Howevfer if it's simply entirely within the rights of a v4l2 driver to\n>> set both - then I think the comment would stand and be valid on it's\n>> own, but some how we'd have to stay aware or document somewhere else in\n>> the doxygen for this class that we directly treat devices as video\n>> devices if they report both video and metadata capture.\n> \n> That won't be enough as soon as we need to capture embedded data from\n> the ISYS. We need to extend the V4L2 helper classes to allow selecting\n> the type, among the supported types. I fear this mode of operation,\n> while supported by the kernel, has seen little testing beyond\n> applications that are highly specific to particular devices, so I expect\n> rough edges.\n\nHmm, I see that we already default to video-capture for these /dev/video\nnodes in V4L2VideoDevice::open() :\n\n if (caps_.isVideoCapture()) {\n notifierType = EventNotifier::Read;\n bufferType_ = caps_.isMultiplanar()\n ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n } else if (caps_.isVideoOutput()) {\n notifierType = EventNotifier::Write;\n bufferType_ = caps_.isMultiplanar()\n ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n } else if (caps_.isMetaCapture()) {\n notifierType = EventNotifier::Read;\n bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n } else if (caps_.isMetaOutput()) {\n notifierType = EventNotifier::Write;\n bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n } else {\n\t\t...\n\nSo if we change other functions to check for the bufferType_\ninstead of re-calling isMeta() there then later on open()\ncould get a type argument to override the default order\nfrom above (when set) and to select meta capture on nodes\nwhich support that instead.\n\nSo what I'm proposing is to e.g. change getFormat() from:\n\n if (caps_.isMeta())\n return getFormatMeta(format);\n else if (caps_.isMultiplanar())\n return getFormatMultiplane(format);\n else\n return getFormatSingleplane(format);\n\nto:\n\n if (bufferType_ == V4L2_BUF_TYPE_META_CAPTURE || bufferType_ == V4L2_BUF_TYPE_META_OUTPUT)\n return getFormatMeta(format);\n else if (caps_.isMultiplanar())\n return getFormatMultiplane(format);\n else\n return getFormatSingleplane(format);\n\nAnd probable add macros to mirror V4L2_TYPE_IS_OUTPUT so this becomes:\n\n if (V4L2_TYPE_IS_META(bufferType_))\n return getFormatMeta(format);\n else if (V4L2_TYPE_IS_MULTI_PLANAR(bufferType_))\n return getFormatMultiplane(format);\n else\n return getFormatSingleplane(format);\n\nAnd the same for e.g. setFormat() and tryFormat()\n\nThis would make the bufferType selection in V4L2VideoDevice::open()\nthe sole source of truth for which type of, well, buffers this\nV4L2VideoDevice instance is dealing with.\n\nNote that e.g. getFormatXxxplane() already rely on\nbufferType_:\n\nint V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n{\n struct v4l2_format v4l2Format = {};\n struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n int ret;\n\n v4l2Format.type = bufferType_;\n\t...\n\nSo the suggest change to the getFormat() wrapper around\nthe getFormatXxx() methods simply makes using bufferType_ as\n*the* way to determine things consistent.\n\nLaurent, I think the change suggested above will be more to your\nliking, right ?\n\nRegards,\n\nHans","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 09822BEFBE\n\tfor ;\n\tThu, 4 Jul 2024 11:04:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CBAC62E24;\n\tThu, 4 Jul 2024 13:04:05 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 32DCF62C99\n\tfor ;\n\tThu, 4 Jul 2024 13:04:03 +0200 (CEST)","from mail-ed1-f69.google.com (mail-ed1-f69.google.com\n\t[209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-631-XYz0BPBwNNWJces2wHGnwg-1; Thu, 04 Jul 2024 07:04:00 -0400","by mail-ed1-f69.google.com with SMTP id\n\t4fb4d7f45d1cf-58bb70e381bso404023a12.2\n\tfor ;\n\tThu, 04 Jul 2024 04:04:00 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-58cf5bdeda0sm2012004a12.73.2024.07.04.04.03.58\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tThu, 04 Jul 2024 04:03:58 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"PJcfCewP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1720091042;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=wRnPp48uVA0zeirgQhZSeGrzwvBsok/pCHtD4rTAzrs=;\n\tb=PJcfCewPH96EjNxMlqty9xZqtZpBlUroANG88cayJ5pwXJ7xXj9yksfdaj2md8VJJdniW3\n\t2w9j7yTtEK+ltUmQ6PrnTADBV/9P8Uif0bWEvvayp+xtJTEp6V622uYVhltxMuYdfFDeil\n\thEY7sJ6y1pguo+T8gV7jU3Yv37HTA00=","X-MC-Unique":"XYz0BPBwNNWJces2wHGnwg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1720091039; x=1720695839;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=wRnPp48uVA0zeirgQhZSeGrzwvBsok/pCHtD4rTAzrs=;\n\tb=RBD00YidaZj5HZh9+4OViq3AtZUFtPzVGC/uQGyLeX1VGizjWxQF1pVMx7Ua7zhVh+\n\ttmoZz8loIuF9WlFTKpmpOfwgcU+nP1imJWuGZFr0RFAKnqHvU9PN4J8eQtIDOcKNhDVQ\n\tyhC8nmFeM94MONQPGnani18ztLnaaBfrHfoAnRxTXwFPSyksobxeJqCStCNfxWXI3C6I\n\tZvFfjzCmTDthIfkw4Q78WLF6WiXnGVjz+5hmTfO+B5+G5ZYq3oAV/F0EWFqtkSp71hh1\n\tQa8NEC/ayEgpHIARLiHtly8rhadL3553uoU6m9WYUPbSGZrOA6tEr+hDnhy5r9LHhlFy\n\ttk2w==","X-Gm-Message-State":"AOJu0Yx+6hcuPSzcUzwPL9tR5MivWGGqWgdpwT7n2jhR+91TLhjbp956\n\t6ToBJEB0qA0mFbs6hUF5K6ljXNj8VjFaTUSDlbXktB64+mfWGPAS715FriOasBa5H59VA1gJoAR\n\tE5vuigopYFgHpH8nq2ekjIm++qsViBM1xJEvUF7aSrGP1BO78zvl3ZiUjvZDEnEk9qAjaO6s=","X-Received":["by 2002:aa7:d5d6:0:b0:58d:703b:c1d5 with SMTP id\n\t4fb4d7f45d1cf-58e5cd151b2mr831475a12.38.1720091039687; \n\tThu, 04 Jul 2024 04:03:59 -0700 (PDT)","by 2002:aa7:d5d6:0:b0:58d:703b:c1d5 with SMTP id\n\t4fb4d7f45d1cf-58e5cd151b2mr831459a12.38.1720091039281; \n\tThu, 04 Jul 2024 04:03:59 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHow8zRnKZcHBs0Yyuall0T4oBKsDt+hAnQYLpfgtGP6WHtAm4CTQ8PjsxptNf91Ho3lo3bpA==","Message-ID":"<4f770ad8-e02c-487d-9fa9-18154802de9c@redhat.com>","Date":"Thu, 4 Jul 2024 13:03:58 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","To":"Laurent Pinchart ,\n\tKieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","References":"<20240703150923.25994-1-hdegoede@redhat.com>\n\t<172008177189.3353312.1466426302104264369@ping.linuxembedded.co.uk>\n\t<20240704103709.GC10099@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240704103709.GC10099@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30372,"web_url":"https://patchwork.libcamera.org/comment/30372/","msgid":"<172039439823.3296918.5249684835054159670@ping.linuxembedded.co.uk>","date":"2024-07-07T23:19:58","subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2024-07-04 12:03:58)\n> Hi,\n> \n> On 7/4/24 12:37 PM, Laurent Pinchart wrote:\n> > On Thu, Jul 04, 2024 at 09:29:31AM +0100, Kieran Bingham wrote:\n> >> Hi Hans,\n> >>\n> >> So the IPU6 enablement in the simple pipeline handler is straight\n> >> forward ... lets see what we can do here.\n> >>\n> >> Quoting Hans de Goede (2024-07-03 16:09:22)\n> >>> The /dev/video# nodes of the IPU6 driver set both V4L2_CAP_VIDEO_CAPTURE\n> >>> and V4L2_CAP_META_CAPTURE.\n> >>\n> >> Is this a driver that is already upstream?\n> > \n> > It's the ISYS (input subsystem, CSI-2 RX) driver, it will be in v6.10.\n> > The PSYS (processing subsystem, ISP) driver isn't anywhere close to\n> > mainline yet.\n> > \n> >> Is it really valid to set both of those in the V4L2 spec?\n> >>\n> >> If so - then we have to do something here... if not - what control do we\n> >> have to fix the current driver?\n> > \n> > It's not forbidden at least. The ISYS has multiple DMA engines connected\n> > to each CSI-2 RX, and they're not dedicated to image data or embedded\n> > data. Streams are filtered based on VC/DT, and routed to those generic\n> > DMA engines.\n> > \n> > It's ugly though. The driver reports both capabilities, it maintains\n> > separate image and metadata formats that can be accessed through ioctl\n> > handlers, and selects the effective vb2 queue type when VIDIOC_REQBUFS\n> > is called.\n> > \n> >>> This was resulting in V4L2VideoDevice::getFormat() / tryFormat() /\n> >>> setFormat() calling the metadata related ioctls instead of the videocap\n> >>> ioctls causing things to not work.\n> >>>\n> >>> Fix this by making V4L2Capability::isMeta() return false when the device\n> >>> also has one of the video capabilities.\n> >>>\n> >>> Signed-off-by: Hans de Goede \n> >>> ---\n> >>> include/libcamera/internal/v4l2_videodevice.h | 4 ++++\n> >>> 1 file changed, 4 insertions(+)\n> >>>\n> >>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> >>> index 9057be08..fdd877c8 100644\n> >>> --- a/include/libcamera/internal/v4l2_videodevice.h\n> >>> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> >>> @@ -92,6 +92,10 @@ struct V4L2Capability final : v4l2_capability {\n> >>> }\n> >>> bool isMeta() const\n> >>> {\n> >>> + /* Treat devs which combine video and meta as video not meta */\n> >>\n> >> If this is a workaround, and really not expected I would want more\n> >> detail here. Especially if this is a short term workaround for any\n> >> possibly out of tree or staging drivers (I haven't checked where IPU6\n> >> state is currently), including a \\todo: to remove it in the future\n> >> when the driver is fixed/upstreamed.\n> >>\n> >> Howevfer if it's simply entirely within the rights of a v4l2 driver to\n> >> set both - then I think the comment would stand and be valid on it's\n> >> own, but some how we'd have to stay aware or document somewhere else in\n> >> the doxygen for this class that we directly treat devices as video\n> >> devices if they report both video and metadata capture.\n> > \n> > That won't be enough as soon as we need to capture embedded data from\n> > the ISYS. We need to extend the V4L2 helper classes to allow selecting\n> > the type, among the supported types. I fear this mode of operation,\n> > while supported by the kernel, has seen little testing beyond\n> > applications that are highly specific to particular devices, so I expect\n> > rough edges.\n> \n> Hmm, I see that we already default to video-capture for these /dev/video\n> nodes in V4L2VideoDevice::open() :\n> \n> if (caps_.isVideoCapture()) {\n> notifierType = EventNotifier::Read;\n> bufferType_ = caps_.isMultiplanar()\n> ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> } else if (caps_.isVideoOutput()) {\n> notifierType = EventNotifier::Write;\n> bufferType_ = caps_.isMultiplanar()\n> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> } else if (caps_.isMetaCapture()) {\n> notifierType = EventNotifier::Read;\n> bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n> } else if (caps_.isMetaOutput()) {\n> notifierType = EventNotifier::Write;\n> bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n> } else {\n> ...\n> \n> So if we change other functions to check for the bufferType_\n> instead of re-calling isMeta() there then later on open()\n> could get a type argument to override the default order\n> from above (when set) and to select meta capture on nodes\n> which support that instead.\n> \n> So what I'm proposing is to e.g. change getFormat() from:\n> \n> if (caps_.isMeta())\n> return getFormatMeta(format);\n> else if (caps_.isMultiplanar())\n> return getFormatMultiplane(format);\n> else\n> return getFormatSingleplane(format);\n> \n> to:\n> \n> if (bufferType_ == V4L2_BUF_TYPE_META_CAPTURE || bufferType_ == V4L2_BUF_TYPE_META_OUTPUT)\n> return getFormatMeta(format);\n> else if (caps_.isMultiplanar())\n> return getFormatMultiplane(format);\n> else\n> return getFormatSingleplane(format);\n> \n> And probable add macros to mirror V4L2_TYPE_IS_OUTPUT so this becomes:\n> \n> if (V4L2_TYPE_IS_META(bufferType_))\n> return getFormatMeta(format);\n> else if (V4L2_TYPE_IS_MULTI_PLANAR(bufferType_))\n> return getFormatMultiplane(format);\n> else\n> return getFormatSingleplane(format);\n> \n> And the same for e.g. setFormat() and tryFormat()\n> \n> This would make the bufferType selection in V4L2VideoDevice::open()\n> the sole source of truth for which type of, well, buffers this\n> V4L2VideoDevice instance is dealing with.\n> \n> Note that e.g. getFormatXxxplane() already rely on\n> bufferType_:\n> \n> int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> {\n> struct v4l2_format v4l2Format = {};\n> struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n> int ret;\n> \n> v4l2Format.type = bufferType_;\n> ...\n> \n> So the suggest change to the getFormat() wrapper around\n> the getFormatXxx() methods simply makes using bufferType_ as\n> *the* way to determine things consistent.\n> \n> Laurent, I think the change suggested above will be more to your\n> liking, right ?\n\n\nThat all sounds pretty justifiable to me ....\n\nIs it something you can / plan to tackle?\n--\nKieran\n\n> \n> Regards,\n> \n> Hans\n> \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 79526BD87C\n\tfor ;\n\tSun, 7 Jul 2024 23:20:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC9DE62E22;\n\tMon, 8 Jul 2024 01:20:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C9AD619C4\n\tfor ;\n\tMon, 8 Jul 2024 01:20:02 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 97DA04C9;\n\tMon, 8 Jul 2024 01:19:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"W1m46+z7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720394370;\n\tbh=xq6b5tyotLjAGEx5fKeKtNFWC7GnPbl4DAcTb6ckfyo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=W1m46+z7Sd40pgGtXCmY2DSVjs53TQWEPLgvXgZsPqTdn4nRXtt+sWe2nZuGv12iF\n\tgKrDcMZAFV/d6ardYe4Z2sW2td/9h9IJsuKPlcTcwvhk7m7pVhVRmTyUYYPTgN2QzS\n\t+xLbRy90w9j3dJwKYqHRkbXb5lDf6ucstS0GP7U0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<4f770ad8-e02c-487d-9fa9-18154802de9c@redhat.com>","References":"<20240703150923.25994-1-hdegoede@redhat.com>\n\t<172008177189.3353312.1466426302104264369@ping.linuxembedded.co.uk>\n\t<20240704103709.GC10099@pendragon.ideasonboard.com>\n\t<4f770ad8-e02c-487d-9fa9-18154802de9c@redhat.com>","Subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","To":"Hans de Goede ,\n\tLaurent Pinchart ","Date":"Mon, 08 Jul 2024 00:19:58 +0100","Message-ID":"<172039439823.3296918.5249684835054159670@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30373,"web_url":"https://patchwork.libcamera.org/comment/30373/","msgid":"","date":"2024-07-08T08:53:28","subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 7/8/24 1:19 AM, Kieran Bingham wrote:\n> Quoting Hans de Goede (2024-07-04 12:03:58)\n>> Hi,\n>>\n>> On 7/4/24 12:37 PM, Laurent Pinchart wrote:\n>>> On Thu, Jul 04, 2024 at 09:29:31AM +0100, Kieran Bingham wrote:\n>>>> Hi Hans,\n>>>>\n>>>> So the IPU6 enablement in the simple pipeline handler is straight\n>>>> forward ... lets see what we can do here.\n>>>>\n>>>> Quoting Hans de Goede (2024-07-03 16:09:22)\n>>>>> The /dev/video# nodes of the IPU6 driver set both V4L2_CAP_VIDEO_CAPTURE\n>>>>> and V4L2_CAP_META_CAPTURE.\n>>>>\n>>>> Is this a driver that is already upstream?\n>>>\n>>> It's the ISYS (input subsystem, CSI-2 RX) driver, it will be in v6.10.\n>>> The PSYS (processing subsystem, ISP) driver isn't anywhere close to\n>>> mainline yet.\n>>>\n>>>> Is it really valid to set both of those in the V4L2 spec?\n>>>>\n>>>> If so - then we have to do something here... if not - what control do we\n>>>> have to fix the current driver?\n>>>\n>>> It's not forbidden at least. The ISYS has multiple DMA engines connected\n>>> to each CSI-2 RX, and they're not dedicated to image data or embedded\n>>> data. Streams are filtered based on VC/DT, and routed to those generic\n>>> DMA engines.\n>>>\n>>> It's ugly though. The driver reports both capabilities, it maintains\n>>> separate image and metadata formats that can be accessed through ioctl\n>>> handlers, and selects the effective vb2 queue type when VIDIOC_REQBUFS\n>>> is called.\n>>>\n>>>>> This was resulting in V4L2VideoDevice::getFormat() / tryFormat() /\n>>>>> setFormat() calling the metadata related ioctls instead of the videocap\n>>>>> ioctls causing things to not work.\n>>>>>\n>>>>> Fix this by making V4L2Capability::isMeta() return false when the device\n>>>>> also has one of the video capabilities.\n>>>>>\n>>>>> Signed-off-by: Hans de Goede \n>>>>> ---\n>>>>> include/libcamera/internal/v4l2_videodevice.h | 4 ++++\n>>>>> 1 file changed, 4 insertions(+)\n>>>>>\n>>>>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n>>>>> index 9057be08..fdd877c8 100644\n>>>>> --- a/include/libcamera/internal/v4l2_videodevice.h\n>>>>> +++ b/include/libcamera/internal/v4l2_videodevice.h\n>>>>> @@ -92,6 +92,10 @@ struct V4L2Capability final : v4l2_capability {\n>>>>> }\n>>>>> bool isMeta() const\n>>>>> {\n>>>>> + /* Treat devs which combine video and meta as video not meta */\n>>>>\n>>>> If this is a workaround, and really not expected I would want more\n>>>> detail here. Especially if this is a short term workaround for any\n>>>> possibly out of tree or staging drivers (I haven't checked where IPU6\n>>>> state is currently), including a \\todo: to remove it in the future\n>>>> when the driver is fixed/upstreamed.\n>>>>\n>>>> Howevfer if it's simply entirely within the rights of a v4l2 driver to\n>>>> set both - then I think the comment would stand and be valid on it's\n>>>> own, but some how we'd have to stay aware or document somewhere else in\n>>>> the doxygen for this class that we directly treat devices as video\n>>>> devices if they report both video and metadata capture.\n>>>\n>>> That won't be enough as soon as we need to capture embedded data from\n>>> the ISYS. We need to extend the V4L2 helper classes to allow selecting\n>>> the type, among the supported types. I fear this mode of operation,\n>>> while supported by the kernel, has seen little testing beyond\n>>> applications that are highly specific to particular devices, so I expect\n>>> rough edges.\n>>\n>> Hmm, I see that we already default to video-capture for these /dev/video\n>> nodes in V4L2VideoDevice::open() :\n>>\n>> if (caps_.isVideoCapture()) {\n>> notifierType = EventNotifier::Read;\n>> bufferType_ = caps_.isMultiplanar()\n>> ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n>> : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n>> } else if (caps_.isVideoOutput()) {\n>> notifierType = EventNotifier::Write;\n>> bufferType_ = caps_.isMultiplanar()\n>> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n>> : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>> } else if (caps_.isMetaCapture()) {\n>> notifierType = EventNotifier::Read;\n>> bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n>> } else if (caps_.isMetaOutput()) {\n>> notifierType = EventNotifier::Write;\n>> bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n>> } else {\n>> ...\n>>\n>> So if we change other functions to check for the bufferType_\n>> instead of re-calling isMeta() there then later on open()\n>> could get a type argument to override the default order\n>> from above (when set) and to select meta capture on nodes\n>> which support that instead.\n>>\n>> So what I'm proposing is to e.g. change getFormat() from:\n>>\n>> if (caps_.isMeta())\n>> return getFormatMeta(format);\n>> else if (caps_.isMultiplanar())\n>> return getFormatMultiplane(format);\n>> else\n>> return getFormatSingleplane(format);\n>>\n>> to:\n>>\n>> if (bufferType_ == V4L2_BUF_TYPE_META_CAPTURE || bufferType_ == V4L2_BUF_TYPE_META_OUTPUT)\n>> return getFormatMeta(format);\n>> else if (caps_.isMultiplanar())\n>> return getFormatMultiplane(format);\n>> else\n>> return getFormatSingleplane(format);\n>>\n>> And probable add macros to mirror V4L2_TYPE_IS_OUTPUT so this becomes:\n>>\n>> if (V4L2_TYPE_IS_META(bufferType_))\n>> return getFormatMeta(format);\n>> else if (V4L2_TYPE_IS_MULTI_PLANAR(bufferType_))\n>> return getFormatMultiplane(format);\n>> else\n>> return getFormatSingleplane(format);\n>>\n>> And the same for e.g. setFormat() and tryFormat()\n>>\n>> This would make the bufferType selection in V4L2VideoDevice::open()\n>> the sole source of truth for which type of, well, buffers this\n>> V4L2VideoDevice instance is dealing with.\n>>\n>> Note that e.g. getFormatXxxplane() already rely on\n>> bufferType_:\n>>\n>> int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n>> {\n>> struct v4l2_format v4l2Format = {};\n>> struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n>> int ret;\n>>\n>> v4l2Format.type = bufferType_;\n>> ...\n>>\n>> So the suggest change to the getFormat() wrapper around\n>> the getFormatXxx() methods simply makes using bufferType_ as\n>> *the* way to determine things consistent.\n>>\n>> Laurent, I think the change suggested above will be more to your\n>> liking, right ?\n> \n> \n> That all sounds pretty justifiable to me ....\n> \n> Is it something you can / plan to tackle?\n\nYes I plan to work on this this week.\n\nRegards,\n\nHans","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 BDAA6BDB1C\n\tfor ;\n\tMon, 8 Jul 2024 08:53:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3F89562E22;\n\tMon, 8 Jul 2024 10:53:36 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A0DA619C3\n\tfor ;\n\tMon, 8 Jul 2024 10:53:33 +0200 (CEST)","from mail-ej1-f69.google.com (mail-ej1-f69.google.com\n\t[209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-64-Zi5g6MRtOR6Y3w4FpLndGA-1; Mon, 08 Jul 2024 04:53:31 -0400","by mail-ej1-f69.google.com with SMTP id\n\ta640c23a62f3a-a77e044ff17so138260466b.3\n\tfor ;\n\tMon, 08 Jul 2024 01:53:31 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a77c720a09dsm304836666b.218.2024.07.08.01.53.28\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 08 Jul 2024 01:53:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"fN7Fo/bM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1720428813;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=C1LLHvVKzkz5Abz9SiFx9nDdKi4Wxf0hgldVDeGOXqw=;\n\tb=fN7Fo/bMn6ACZIX238y8y+klqoSgYQq7yKSZmywV7X64Hss+Ru9KXfRK6AN3YOD4lkQ8kO\n\tGgAOWo/JqaljRhTeSFQY16kWSXs/E5k999JwTD41DxstlbHureQQn576PMQfPKCWMCh91i\n\tDuo/d1qgMsyG92Nhz7sm5FG+yu5Jbf4=","X-MC-Unique":"Zi5g6MRtOR6Y3w4FpLndGA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1720428810; x=1721033610;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=C1LLHvVKzkz5Abz9SiFx9nDdKi4Wxf0hgldVDeGOXqw=;\n\tb=ufE66TEqBZts0ok6tcNpqNw7Hx5EX6/XTrPFrNR9v/GP9KvVxfF9XSG8cy0eYaWKeb\n\tAQiEY7MRKBMUa0KS3Stw1M7mmU7g8qxA/uYaUd7b9bsDz+o69VmMBxylMzj6wi9Bau61\n\tMnAO651bJbvzZVdq9yTzH9Y9WiaZ98sk7hFGiDK7wm3mbT57S4lf0U6onrK1j3052gAR\n\t65wdlnb4vTfTjkXU5l+RVlZh2B+3CO4wEjyWun3vTTxJO51Z8dK4uhXm3PjdOCF9T8z3\n\tpfB3gX19cs2ozm8v8OLkun/SGQpRyH/3MdOdVnoJoL/6ANWN0t54TgKAtXkcRf9ldYQv\n\tAu7A==","X-Gm-Message-State":"AOJu0YzYufI1SEHFlGOs5M9F6sqeJEp0HKIRG9W1BGQC1cG5+hLV7mle\n\tb/NdWMygiDBdf1d42UGEhrjN01McNLvZQoGoJCzH8oBNGwtmNo698gB9MGUc1hqUkF4f716pkPj\n\tdJr1KEa9UL9BcndMAlNmPNRZl9dTLRtAlRLI48kBJVSBP1zL+xbZ4oid9EXdpzLfPtOH0eUI=","X-Received":["by 2002:a17:907:7212:b0:a77:cf9d:f498 with SMTP id\n\ta640c23a62f3a-a77cf9df683mr630395466b.40.1720428810174; \n\tMon, 08 Jul 2024 01:53:30 -0700 (PDT)","by 2002:a17:907:7212:b0:a77:cf9d:f498 with SMTP id\n\ta640c23a62f3a-a77cf9df683mr630393766b.40.1720428809619; \n\tMon, 08 Jul 2024 01:53:29 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGlrg0wEBtT53fTkAXzRprmQV/UB+BDNAB8EctVrknbEY58knEHoahqm8OIx9cMSJrkxD68UQ==","Message-ID":"","Date":"Mon, 8 Jul 2024 10:53:28 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices\n\twhich set both V4L2_CAP_VIDEO and V4L2_CAP_META","To":"Kieran Bingham ,\n\tLaurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","References":"<20240703150923.25994-1-hdegoede@redhat.com>\n\t<172008177189.3353312.1466426302104264369@ping.linuxembedded.co.uk>\n\t<20240704103709.GC10099@pendragon.ideasonboard.com>\n\t<4f770ad8-e02c-487d-9fa9-18154802de9c@redhat.com>\n\t<172039439823.3296918.5249684835054159670@ping.linuxembedded.co.uk>","From":"Hans de Goede ","In-Reply-To":"<172039439823.3296918.5249684835054159670@ping.linuxembedded.co.uk>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]