[libcamera-devel,2/3] libcamera: v4l2_device: Add support for integer array controls
diff mbox series

Message ID 20211223080110.9766-3-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Make use of V4L2_CID_NOTIFY_GAINS
Related show

Commit Message

David Plowman Dec. 23, 2021, 8:01 a.m. UTC 
Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/v4l2_device.cpp | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Laurent Pinchart Dec. 24, 2021, 3:16 a.m. UTC | #1 
Hi David,

Thank you for the patch.

On Thu, Dec 23, 2021 at 08:01:09AM +0000, David Plowman wrote:

A commit message would be nice.

> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/libcamera/v4l2_device.cpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 62c91779..6f9de8ad 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -316,6 +316,18 @@ int V4L2Device::setControls(ControlList *ctrls)
>  			break;
>  		}
>  
> +		case ControlTypeInteger32: {

Could you move this above ControlTypeInteger32 ?

> +			if (value.isArray()) {
> +				Span<uint8_t> data = value.data();
> +				v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data());
> +				v4l2Ctrl.size = data.size();
> +			} else {
> +				v4l2Ctrl.value = value.get<int32_t>();
> +			}

This looks fine, but I think you also need to update
V4L2Device::updateControls().

> +
> +			break;
> +		}
> +
>  		default:
>  			/* \todo To be changed to support strings. */
>  			v4l2Ctrl.value = value.get<int32_t>();
David Plowman Dec. 30, 2021, 9:28 a.m. UTC | #2 
Hi Laurent

Thanks for the review!

On Fri, 24 Dec 2021 at 03:16, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Thu, Dec 23, 2021 at 08:01:09AM +0000, David Plowman wrote:
>
> A commit message would be nice.
>
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/libcamera/v4l2_device.cpp | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 62c91779..6f9de8ad 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -316,6 +316,18 @@ int V4L2Device::setControls(ControlList *ctrls)
> >                       break;
> >               }
> >
> > +             case ControlTypeInteger32: {
>
> Could you move this above ControlTypeInteger32 ?

I'm guessing you mean this should go between ControlTypeInteger64 and
ControlTypeIntegerByte, like I see in updateControls()? Will do!

>
> > +                     if (value.isArray()) {
> > +                             Span<uint8_t> data = value.data();
> > +                             v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data());
> > +                             v4l2Ctrl.size = data.size();
> > +                     } else {
> > +                             v4l2Ctrl.value = value.get<int32_t>();
> > +                     }
>
> This looks fine, but I think you also need to update
> V4L2Device::updateControls().

Just to check (my understanding of how ControlLists work has always
been a bit hazy...), the same logic will apply there as in the "Byte"
case, i.e. I just need to check for the array type and then do
nothing?

Thanks!
David

>
> > +
> > +                     break;
> > +             }
> > +
> >               default:
> >                       /* \todo To be changed to support strings. */
> >                       v4l2Ctrl.value = value.get<int32_t>();
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Dec. 30, 2021, 4:22 p.m. UTC | #3 
Hi David,

On Thu, Dec 30, 2021 at 09:28:40AM +0000, David Plowman wrote:
> On Fri, 24 Dec 2021 at 03:16, Laurent Pinchart wrote:
> > On Thu, Dec 23, 2021 at 08:01:09AM +0000, David Plowman wrote:
> >
> > A commit message would be nice.
> >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  src/libcamera/v4l2_device.cpp | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > index 62c91779..6f9de8ad 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -316,6 +316,18 @@ int V4L2Device::setControls(ControlList *ctrls)
> > >                       break;
> > >               }
> > >
> > > +             case ControlTypeInteger32: {
> >
> > Could you move this above ControlTypeInteger32 ?
> 
> I'm guessing you mean this should go between ControlTypeInteger64 and
> ControlTypeIntegerByte, like I see in updateControls()? Will do!

I would actually put 32 before 64 (so just before ControlTypeInteger64).
That doesn't match V4L2Device::updateControls(), but as you'll have to
update that function anyway, you can also fix the order there :-)

> >
> > > +                     if (value.isArray()) {
> > > +                             Span<uint8_t> data = value.data();
> > > +                             v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data());
> > > +                             v4l2Ctrl.size = data.size();
> > > +                     } else {
> > > +                             v4l2Ctrl.value = value.get<int32_t>();
> > > +                     }
> >
> > This looks fine, but I think you also need to update
> > V4L2Device::updateControls().
> 
> Just to check (my understanding of how ControlLists work has always
> been a bit hazy...), the same logic will apply there as in the "Byte"
> case, i.e. I just need to check for the array type and then do
> nothing?

That's correct. Actually, maybe the following would be more generic ?

diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 1e26305742db..be66b8eb523a 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -671,6 +671,14 @@ void V4L2Device::updateControls(ControlList *ctrls,
 		const unsigned int id = v4l2Ctrl.id;

 		ControlValue value = ctrls->get(id);
+		if (value.isArray()) {
+			/*
+			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
+			 * accessed the ControlValue storage directly for array
+			 * controls.
+			 */
+			continue;
+		}

 		const auto iter = controls_.find(id);
 		ASSERT(iter != controls_.end());
@@ -684,13 +692,6 @@ void V4L2Device::updateControls(ControlList *ctrls,
 			value.set<int32_t>(v4l2Ctrl.value);
 			break;

-		case ControlTypeByte:
-			/*
-			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
-			 * accessed the ControlValue storage directly.
-			 */
-			break;
-
 		default:
 			/*
 			 * \todo To be changed when support for string controls


I haven't tested it, maybe I'm missing something.

> > > +
> > > +                     break;
> > > +             }
> > > +
> > >               default:
> > >                       /* \todo To be changed to support strings. */
> > >                       v4l2Ctrl.value = value.get<int32_t>();
Laurent Pinchart Dec. 30, 2021, 4:30 p.m. UTC | #4 
Hi David,

On Thu, Dec 30, 2021 at 06:22:33PM +0200, Laurent Pinchart wrote:
> On Thu, Dec 30, 2021 at 09:28:40AM +0000, David Plowman wrote:
> > On Fri, 24 Dec 2021 at 03:16, Laurent Pinchart wrote:
> > > On Thu, Dec 23, 2021 at 08:01:09AM +0000, David Plowman wrote:
> > >
> > > A commit message would be nice.
> > >
> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > ---
> > > >  src/libcamera/v4l2_device.cpp | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > > index 62c91779..6f9de8ad 100644
> > > > --- a/src/libcamera/v4l2_device.cpp
> > > > +++ b/src/libcamera/v4l2_device.cpp
> > > > @@ -316,6 +316,18 @@ int V4L2Device::setControls(ControlList *ctrls)
> > > >                       break;
> > > >               }
> > > >
> > > > +             case ControlTypeInteger32: {
> > >
> > > Could you move this above ControlTypeInteger32 ?
> > 
> > I'm guessing you mean this should go between ControlTypeInteger64 and
> > ControlTypeIntegerByte, like I see in updateControls()? Will do!
> 
> I would actually put 32 before 64 (so just before ControlTypeInteger64).
> That doesn't match V4L2Device::updateControls(), but as you'll have to
> update that function anyway, you can also fix the order there :-)
> 
> > >
> > > > +                     if (value.isArray()) {
> > > > +                             Span<uint8_t> data = value.data();
> > > > +                             v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data());
> > > > +                             v4l2Ctrl.size = data.size();
> > > > +                     } else {
> > > > +                             v4l2Ctrl.value = value.get<int32_t>();
> > > > +                     }
> > >
> > > This looks fine, but I think you also need to update
> > > V4L2Device::updateControls().
> > 
> > Just to check (my understanding of how ControlLists work has always
> > been a bit hazy...), the same logic will apply there as in the "Byte"
> > case, i.e. I just need to check for the array type and then do
> > nothing?
> 
> That's correct. Actually, maybe the following would be more generic ?
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 1e26305742db..be66b8eb523a 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -671,6 +671,14 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  		const unsigned int id = v4l2Ctrl.id;
> 
>  		ControlValue value = ctrls->get(id);
> +		if (value.isArray()) {
> +			/*
> +			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
> +			 * accessed the ControlValue storage directly for array
> +			 * controls.
> +			 */
> +			continue;
> +		}
> 
>  		const auto iter = controls_.find(id);
>  		ASSERT(iter != controls_.end());
> @@ -684,13 +692,6 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  			value.set<int32_t>(v4l2Ctrl.value);
>  			break;
> 
> -		case ControlTypeByte:
> -			/*
> -			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
> -			 * accessed the ControlValue storage directly.
> -			 */
> -			break;
> -
>  		default:
>  			/*
>  			 * \todo To be changed when support for string controls

Or even

@@ -681,16 +689,6 @@ void V4L2Device::updateControls(ControlList *ctrls,
 			break;

 		case ControlTypeInteger32:
-			value.set<int32_t>(v4l2Ctrl.value);
-			break;
-
-		case ControlTypeByte:
-			/*
-			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
-			 * accessed the ControlValue storage directly.
-			 */
-			break;
-
 		default:
 			/*
 			 * \todo To be changed when support for string controls


> I haven't tested it, maybe I'm missing something.
> 
> > > > +
> > > > +                     break;
> > > > +             }
> > > > +
> > > >               default:
> > > >                       /* \todo To be changed to support strings. */
> > > >                       v4l2Ctrl.value = value.get<int32_t>();

Patch
diff mbox series 

diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 62c91779..6f9de8ad 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -316,6 +316,18 @@  int V4L2Device::setControls(ControlList *ctrls)
 			break;
 		}
 
+		case ControlTypeInteger32: {
+			if (value.isArray()) {
+				Span<uint8_t> data = value.data();
+				v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data());
+				v4l2Ctrl.size = data.size();
+			} else {
+				v4l2Ctrl.value = value.get<int32_t>();
+			}
+
+			break;
+		}
+
 		default:
 			/* \todo To be changed to support strings. */
 			v4l2Ctrl.value = value.get<int32_t>();