<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 7, 2021 at 9:46 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Mon, Jun 07, 2021 at 09:14:21AM +0900, Hirokazu Honda wrote:<br>
> On Mon, Jun 7, 2021 at 8:22 AM Laurent Pinchart wrote:<br>
> > On Fri, May 28, 2021 at 12:05:27PM +0900, Hirokazu Honda wrote:<br>
> > > This adds a support of v4l2 menu. The control info for v4l2 menu<br>
> > > contains indices without names and 64bit values of querymenu.<br>
> > ><br>
> > > Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> > > ---<br>
> > > include/libcamera/internal/v4l2_device.h | 2 +<br>
> > > src/libcamera/v4l2_device.cpp | 47 ++++++++++++++++++++++--<br>
> > > 2 files changed, 45 insertions(+), 4 deletions(-)<br>
> > ><br>
> > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h<br>
> > > index b82e2a14..687be9b2 100644<br>
> > > --- a/include/libcamera/internal/v4l2_device.h<br>
> > > +++ b/include/libcamera/internal/v4l2_device.h<br>
> > > @@ -56,6 +56,8 @@ protected:<br>
> > > int fd() const { return fd_; }<br>
> > ><br>
> > > private:<br>
> > > + ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);<br>
> > > +<br>
> > > void listControls();<br>
> > > void updateControls(ControlList *ctrls,<br>
> > > Span<const v4l2_ext_control> v4l2Ctrls);<br>
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp<br>
> > > index caafbc2d..032252bb 100644<br>
> > > --- a/src/libcamera/v4l2_device.cpp<br>
> > > +++ b/src/libcamera/v4l2_device.cpp<br>
> > > @@ -530,6 +530,33 @@ int V4L2Device::ioctl(unsigned long request, void *argp)<br>
> > > * \return The V4L2 device file descriptor, -1 if the device node is not open<br>
> > > */<br>
> > ><br>
> > > +/**<br>
> > > + * \brief Create ControlInfo for v4l2 menu ctrl<br>
> ><br>
> > s/for v4l2 menu ctrl/for a V4L2 menu control/<br>
> ><br>
> > > + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu<br>
> > > + * \param[out] ctrlInfo The created controlInfo<br>
> ><br>
> > It's not an out parameter, it's the return value.<br>
> ><br>
> > > + *<br>
> > > + * The created ControlInfo contains indices acquired by VIDIOC_QUERYMENU.<br>
> > > + *<br>
> > > + * \return ControlInfo for v4l2 menu ctrl.<br>
> > > + */<br>
> > > +ControlInfo V4L2Device::v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl)<br>
> > > +{<br>
> > > + std::vector<ControlValue> indices;<br>
> > > + v4l2_querymenu menu;<br>
> ><br>
> > We usually prefix the V4L2 structures with struct. While not strictly<br>
> > mandatory, it emphasizes that there are C structs from V4L2.<br>
> ><br>
> > > + memset(&menu, 0, sizeof(menu));<br>
> ><br>
> > You could also write this<br>
> ><br>
> > struct v4l2_querymenu menu = { };<br>
> ><br>
> > (I really wish designated initializers were available before C++20 :-().<br>
> ><br>
> > > + <a href="http://menu.id" rel="noreferrer" target="_blank">menu.id</a> = <a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>;<br>
> ><br>
> > As a sanity check, should we add<br>
> ><br>
> > if (ctrl.minimum <= 0)<br>
> > return ControlInfo();<br>
> ><br>
> > ?<br>
> ><br>
> > > +<br>
> > > + for (menu.index = ctrl.minimum;<br>
> > > + static_cast<int32_t>(menu.index) <= ctrl.maximum; menu.index++) {<br>
> ><br>
> > for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index)<br>
> > }<br>
> > menu.index = index;<br>
> ><br>
> > would allow dropping the cast. Up to you.<br>
> ><br>
> > > + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)<br>
> > > + continue;<br>
> > > +<br>
> > > + indices.emplace_back(static_cast<int32_t>(menu.index));<br>
> ><br>
> > This could be<br>
> ><br>
> > indices.emplace_back(index);<br>
> ><br>
> > or even<br>
> ><br>
> > indices.push_back(index);<br>
> ><br>
> > as it won't make much of a difference.<br>
> ><br>
> > > + }<br>
> > > +<br>
> > > + return ControlInfo(indices);<br>
> ><br>
> > Would it be useful to also pass the default value to the ControlInfo<br>
> > constructor ?<br>
><br>
> You mean, I should set ctrl.default_value or some value in indices like<br>
> indices.begin()?<br>
<br>
I mean (untested)<br>
<br>
return ControlInfo(indices, ControlValue<int32_t>(<a href="http://ctrl.dev" rel="noreferrer" target="_blank">ctrl.dev</a>));<br>
<br></blockquote><div><br></div><div>I guess you mean ctrl.default_value. Filled so if querymenu for the value is successful.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > > +}<br>
> > > +<br>
> > > /*<br>
> > > * \brief List and store information about all controls supported by the<br>
> > > * V4L2 device<br>
> > > @@ -539,7 +566,6 @@ void V4L2Device::listControls()<br>
> > > ControlInfoMap::Map ctrls;<br>
> > > struct v4l2_query_ext_ctrl ctrl = {};<br>
> > ><br>
> > > - /* \todo Add support for menu controls. */<br>
> > > while (1) {<br>
> > > <a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a> |= V4L2_CTRL_FLAG_NEXT_CTRL |<br>
> > > V4L2_CTRL_FLAG_NEXT_COMPOUND;<br>
> > > @@ -550,16 +576,22 @@ void V4L2Device::listControls()<br>
> > > ctrl.flags & V4L2_CTRL_FLAG_DISABLED)<br>
> > > continue;<br>
> > ><br>
> > > + ControlInfo ctrlInfo;<br>
> ><br>
> > I'd add a blank line here.<br>
> ><br>
> > > switch (ctrl.type) {<br>
> > > case V4L2_CTRL_TYPE_INTEGER:<br>
> > > case V4L2_CTRL_TYPE_BOOLEAN:<br>
> > > - case V4L2_CTRL_TYPE_MENU:<br>
> > > case V4L2_CTRL_TYPE_BUTTON:<br>
> > > case V4L2_CTRL_TYPE_INTEGER64:<br>
> > > case V4L2_CTRL_TYPE_BITMASK:<br>
> > > - case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
> > > case V4L2_CTRL_TYPE_U8:<br>
> > > + ctrlInfo = v4l2ControlInfo(ctrl);<br>
> > > + break;<br>
> > > +<br>
> > > + case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
> > > + case V4L2_CTRL_TYPE_MENU:<br>
> > > + ctrlInfo = v4l2MenuControlInfo(ctrl);<br>
> > > break;<br>
> > > +<br>
> > > /* \todo Support other control types. */<br>
> > > default:<br>
> > > LOG(V4L2, Debug)<br>
> > > @@ -568,10 +600,13 @@ void V4L2Device::listControls()<br>
> > > continue;<br>
> > > }<br>
> > ><br>
> > > + LOG(V4L2, Debug) << "Control: " << <a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a><br>
> > > + << " (" << utils::hex(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>) << ")";<br>
> > > +<br>
> > > controlIds_.emplace_back(v4l2ControlId(ctrl));<br>
> > > controlInfo_.emplace(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>, ctrl);<br>
> > ><br>
> > > - ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));<br>
> > > + ctrls.emplace(controlIds_.back().get(), std::move(ctrlInfo));<br>
> > > }<br>
> > ><br>
> > > controls_ = std::move(ctrls);<br>
> > > @@ -638,6 +673,10 @@ void V4L2Device::updateControls(ControlList *ctrls,<br>
> > > value.set<int64_t>(v4l2Ctrl->value64);<br>
> > > break;<br>
> > ><br>
> > > + case ControlTypeInteger32:<br>
> > > + value.set<int32_t>(v4l2Ctrl->value);<br>
> ><br>
> > This needs a rebase, it's now v4l2Ctrl.value.<br>
> <br>
> By which patch? On top of tree, v4l2Ctrl is still a const pointer.<br>
<br>
By commit abdb11d9ccad ("libcamera: V4L2Device: Remove the controls<br>
order assumption in updateControls()") :-)<br>
<br></blockquote><div><br></div><div>Okay, I saw it was merged just while ago. Thanks for merging.</div><div><br></div><div>-Hiro </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > With these minor issues addressed, the patch looks good to me. Thanks<br>
> > for your continuous effort.<br>
> ><br>
> > > + break;<br>
> > > +<br>
> > > case ControlTypeByte:<br>
> > > /*<br>
> > > * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>