[libcamera-devel,v3,4/5] libcamera: properties: Provide a Devices camera property
diff mbox series

Message ID 20230515124550.3601128-5-kieran.bingham@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Add new Camera devices property
Related show

Commit Message

Kieran Bingham May 15, 2023, 12:45 p.m. UTC 
Provide a new Camera property that allows pipeline handlers to list any
devices used to operate the device. This allows other frameworks and
daemons such as Pipewire to better understand the resources consumed by
a Camera and consider ignoring those resources when enumerating camera
devices on a system.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/libcamera/property_ids.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Ashok Sidipotu June 6, 2023, 11:56 a.m. UTC | #1 
Tested-by: Ashok Sidipotu <ashok.sidipotu@collabora.com>

On Monday, May 15, 2023 18:15 IST, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote:
 Provide a new Camera property that allows pipeline handlers to list any
devices used to operate the device. This allows other frameworks and
daemons such as Pipewire to better understand the resources consumed by
a Camera and consider ignoring those resources when enumerating camera
devices on a system.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
src/libcamera/property_ids.yaml | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
index cb55e0ed2283..6141942969f9 100644
--- a/src/libcamera/property_ids.yaml
+++ b/src/libcamera/property_ids.yaml
@@ -690,6 +690,13 @@ controls:
that is twice that of the full resolution mode. This value will be valid
after the configure method has returned successfully.

+ - Devices:
+ type: int64_t
+ size: [n]
+ description: |
+ A list of integer values of type dev_t denoting major and minor device
+ number of the underlying devices used in the operation of this camera.
+
# ----------------------------------------------------------------------------
# Draft properties section

--
2.34.1
Laurent Pinchart June 6, 2023, 4:21 p.m. UTC | #2 
Hi Kieran,

Thank you for the patch.

On Mon, May 15, 2023 at 01:45:49PM +0100, Kieran Bingham via libcamera-devel wrote:
> Provide a new Camera property that allows pipeline handlers to list any
> devices used to operate the device. This allows other frameworks and

Maybe "any kernel device used to operate the camera" to be more precise
?

> daemons such as Pipewire to better understand the resources consumed by

s/Pipewire/PipeWire/

> a Camera and consider ignoring those resources when enumerating camera
> devices on a system.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/libcamera/property_ids.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index cb55e0ed2283..6141942969f9 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -690,6 +690,13 @@ controls:
>          that is twice that of the full resolution mode. This value will be valid
>          after the configure method has returned successfully.
>  
> +  - Devices:
> +      type: int64_t
> +      size: [n]
> +      description: |
> +        A list of integer values of type dev_t denoting major and minor device
> +        number of the underlying devices used in the operation of this camera.

s/number/numbers/

I would extend it with

        Different cameras may report identical devices.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>    # ----------------------------------------------------------------------------
>    # Draft properties section
>
Barnabás Pőcze June 11, 2023, 9:46 p.m. UTC | #3 
Hi


2023. május 15., hétfő 14:45 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> írta:

> Provide a new Camera property that allows pipeline handlers to list any
> devices used to operate the device. This allows other frameworks and
> daemons such as Pipewire to better understand the resources consumed by
> a Camera and consider ignoring those resources when enumerating camera
> devices on a system.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/libcamera/property_ids.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index cb55e0ed2283..6141942969f9 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -690,6 +690,13 @@ controls:
>          that is twice that of the full resolution mode. This value will be valid
>          after the configure method has returned successfully.
> 
> +  - Devices:
> +      type: int64_t

I am wondering why the type is `int64_t`. POSIX only says `dev_t` is an integer type[0].
Both glibc[1] and musl[2] define it as an unsigned (64-bit) integer (at least as far as I checked).


> +      size: [n]
> +      description: |
> +        A list of integer values of type dev_t denoting major and minor device
> +        number of the underlying devices used in the operation of this camera.
> +
>    # ----------------------------------------------------------------------------
>    # Draft properties section
> 
> --
> 2.34.1


[0]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html#tag_13_67
[1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/typesizes.h;h=5dd1700649463583c573b95cc6df8ac677316ea9;hb=HEAD#l29
[2]: https://git.musl-libc.org/cgit/musl/tree/include/alltypes.h.in?id=718f363bc2067b6487900eddc9180c84e7739f80#n31
Kieran Bingham June 12, 2023, 4:28 p.m. UTC | #4 
Quoting Barnabás Pőcze (2023-06-11 22:46:06)
> Hi
> 
> 
> 2023. május 15., hétfő 14:45 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> írta:
> 
> > Provide a new Camera property that allows pipeline handlers to list any
> > devices used to operate the device. This allows other frameworks and
> > daemons such as Pipewire to better understand the resources consumed by
> > a Camera and consider ignoring those resources when enumerating camera
> > devices on a system.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/libcamera/property_ids.yaml | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > index cb55e0ed2283..6141942969f9 100644
> > --- a/src/libcamera/property_ids.yaml
> > +++ b/src/libcamera/property_ids.yaml
> > @@ -690,6 +690,13 @@ controls:
> >          that is twice that of the full resolution mode. This value will be valid
> >          after the configure method has returned successfully.
> > 
> > +  - Devices:
> > +      type: int64_t
> 
> I am wondering why the type is `int64_t`. POSIX only says `dev_t` is an integer type[0].
> Both glibc[1] and musl[2] define it as an unsigned (64-bit) integer (at least as far as I checked).

Indeed every implementation I can see define it as a 64-bit, but the
kernel uses only 32 bits.

This is a libcamera control limitation ultimately, as we don't have
unsigned integer controls. I'm not sure yet if adding would be helpful
or difficult. It might be worth trying out.

--
Kieran



> > +      size: [n]
> > +      description: |
> > +        A list of integer values of type dev_t denoting major and minor device
> > +        number of the underlying devices used in the operation of this camera.
> > +
> >    # ----------------------------------------------------------------------------
> >    # Draft properties section
> > 
> > --
> > 2.34.1
> 
> 
> [0]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html#tag_13_67
> [1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/typesizes.h;h=5dd1700649463583c573b95cc6df8ac677316ea9;hb=HEAD#l29
> [2]: https://git.musl-libc.org/cgit/musl/tree/include/alltypes.h.in?id=718f363bc2067b6487900eddc9180c84e7739f80#n31
Laurent Pinchart June 12, 2023, 5:44 p.m. UTC | #5 
On Mon, Jun 12, 2023 at 05:28:19PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Barnabás Pőcze (2023-06-11 22:46:06)
> > 2023. május 15., hétfő 14:45 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> írta:
> > 
> > > Provide a new Camera property that allows pipeline handlers to list any
> > > devices used to operate the device. This allows other frameworks and
> > > daemons such as Pipewire to better understand the resources consumed by
> > > a Camera and consider ignoring those resources when enumerating camera
> > > devices on a system.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  src/libcamera/property_ids.yaml | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > > index cb55e0ed2283..6141942969f9 100644
> > > --- a/src/libcamera/property_ids.yaml
> > > +++ b/src/libcamera/property_ids.yaml
> > > @@ -690,6 +690,13 @@ controls:
> > >          that is twice that of the full resolution mode. This value will be valid
> > >          after the configure method has returned successfully.
> > > 
> > > +  - Devices:
> > > +      type: int64_t
> > 
> > I am wondering why the type is `int64_t`. POSIX only says `dev_t` is an integer type[0].
> > Both glibc[1] and musl[2] define it as an unsigned (64-bit) integer (at least as far as I checked).
> 
> Indeed every implementation I can see define it as a 64-bit, but the
> kernel uses only 32 bits.
> 
> This is a libcamera control limitation ultimately, as we don't have
> unsigned integer controls. I'm not sure yet if adding would be helpful
> or difficult. It might be worth trying out.

An int64_t makes sure it will always fit the dev_t value regardless of
the platform, so I don't really see much added value in adding a
platform-dependent integer type for controls.

> > > +      size: [n]
> > > +      description: |
> > > +        A list of integer values of type dev_t denoting major and minor device
> > > +        number of the underlying devices used in the operation of this camera.
> > > +
> > >    # ----------------------------------------------------------------------------
> > >    # Draft properties section
> > > 
> > 
> > [0]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html#tag_13_67
> > [1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/typesizes.h;h=5dd1700649463583c573b95cc6df8ac677316ea9;hb=HEAD#l29
> > [2]: https://git.musl-libc.org/cgit/musl/tree/include/alltypes.h.in?id=718f363bc2067b6487900eddc9180c84e7739f80#n31
Barnabás Pőcze June 13, 2023, 3:48 p.m. UTC | #6 
Hi


2023. június 12., hétfő 18:28 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> [...]
> > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > > index cb55e0ed2283..6141942969f9 100644
> > > --- a/src/libcamera/property_ids.yaml
> > > +++ b/src/libcamera/property_ids.yaml
> > > @@ -690,6 +690,13 @@ controls:
> > >          that is twice that of the full resolution mode. This value will be valid
> > >          after the configure method has returned successfully.
> > >
> > > +  - Devices:
> > > +      type: int64_t
> >
> > I am wondering why the type is `int64_t`. POSIX only says `dev_t` is an integer type[0].
> > Both glibc[1] and musl[2] define it as an unsigned (64-bit) integer (at least as far as I checked).
> 
> Indeed every implementation I can see define it as a 64-bit, but the
> kernel uses only 32 bits.
> 
> This is a libcamera control limitation ultimately, as we don't have
> unsigned integer controls. I'm not sure yet if adding would be helpful
> or difficult. It might be worth trying out.
> [...]

Ahh, I wasn't aware that libcamera does not have uint controls. Thanks for the clarification.


Regards,
Barnabás Pőcze
Barnabás Pőcze June 13, 2023, 3:55 p.m. UTC | #7 
Hi


2023. június 12., hétfő 19:44 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> [...]
> > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > > > index cb55e0ed2283..6141942969f9 100644
> > > > --- a/src/libcamera/property_ids.yaml
> > > > +++ b/src/libcamera/property_ids.yaml
> > > > @@ -690,6 +690,13 @@ controls:
> > > >          that is twice that of the full resolution mode. This value will be valid
> > > >          after the configure method has returned successfully.
> > > >
> > > > +  - Devices:
> > > > +      type: int64_t
> > >
> > > I am wondering why the type is `int64_t`. POSIX only says `dev_t` is an integer type[0].
> > > Both glibc[1] and musl[2] define it as an unsigned (64-bit) integer (at least as far as I checked).
> >
> > Indeed every implementation I can see define it as a 64-bit, but the
> > kernel uses only 32 bits.
> >
> > This is a libcamera control limitation ultimately, as we don't have
> > unsigned integer controls. I'm not sure yet if adding would be helpful
> > or difficult. It might be worth trying out.
> 
> An int64_t makes sure it will always fit the dev_t value regardless of
> the platform, so I don't really see much added value in adding a
> platform-dependent integer type for controls.
> [...]

I did not mean to imply that the size should be platform dependent.
Just that I would have used uint64_t probably, but at that time
I was not aware that libcamera had no uint controls.


Regards,
Barnabás Pőcze
Kieran Bingham June 13, 2023, 5:10 p.m. UTC | #8 
Quoting Barnabás Pőcze (2023-06-13 16:48:39)
> Hi
> 
> 
> 2023. június 12., hétfő 18:28 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> 
> > [...]
> > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > > > index cb55e0ed2283..6141942969f9 100644
> > > > --- a/src/libcamera/property_ids.yaml
> > > > +++ b/src/libcamera/property_ids.yaml
> > > > @@ -690,6 +690,13 @@ controls:
> > > >          that is twice that of the full resolution mode. This value will be valid
> > > >          after the configure method has returned successfully.
> > > >
> > > > +  - Devices:
> > > > +      type: int64_t
> > >
> > > I am wondering why the type is `int64_t`. POSIX only says `dev_t` is an integer type[0].
> > > Both glibc[1] and musl[2] define it as an unsigned (64-bit) integer (at least as far as I checked).
> > 
> > Indeed every implementation I can see define it as a 64-bit, but the
> > kernel uses only 32 bits.
> > 
> > This is a libcamera control limitation ultimately, as we don't have
> > unsigned integer controls. I'm not sure yet if adding would be helpful
> > or difficult. It might be worth trying out.
> > [...]
> 
> Ahh, I wasn't aware that libcamera does not have uint controls. Thanks for the clarification.

Have you reviewed the patches enough to consider providing a set of
Reviewed-by tags perhaps?

Do you have any thoughts or preferences on the control name? Laurent has
discussed the option between "Devices" and "SystemDevices" - I'm really
hoping someone will weigh in on which they prefer to be able to move
forwards and merge this series.

(I prefer Devices, so that makes it 1 vs 1)

--
Regards

Kieran


> 
> 
> Regards,
> Barnabás Pőcze
Barnabás Pőcze June 14, 2023, 9:50 p.m. UTC | #9 
Hi


2023. június 13., kedd 19:10 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> Quoting Barnabás Pőcze (2023-06-13 16:48:39)
> > Hi
> >
> >
> > 2023. június 12., hétfő 18:28 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> >
> > > [...]
> > > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > > > > index cb55e0ed2283..6141942969f9 100644
> > > > > --- a/src/libcamera/property_ids.yaml
> > > > > +++ b/src/libcamera/property_ids.yaml
> > > > > @@ -690,6 +690,13 @@ controls:
> > > > >          that is twice that of the full resolution mode. This value will be valid
> > > > >          after the configure method has returned successfully.
> > > > >
> > > > > +  - Devices:
> > > > > +      type: int64_t
> > > >
> > > > I am wondering why the type is `int64_t`. POSIX only says `dev_t` is an integer type[0].
> > > > Both glibc[1] and musl[2] define it as an unsigned (64-bit) integer (at least as far as I checked).
> > >
> > > Indeed every implementation I can see define it as a 64-bit, but the
> > > kernel uses only 32 bits.
> > >
> > > This is a libcamera control limitation ultimately, as we don't have
> > > unsigned integer controls. I'm not sure yet if adding would be helpful
> > > or difficult. It might be worth trying out.
> > > [...]
> >
> > Ahh, I wasn't aware that libcamera does not have uint controls. Thanks for the clarification.
> 
> Have you reviewed the patches enough to consider providing a set of
> Reviewed-by tags perhaps?
> 
> Do you have any thoughts or preferences on the control name? Laurent has
> discussed the option between "Devices" and "SystemDevices" - I'm really
> hoping someone will weigh in on which they prefer to be able to move
> forwards and merge this series.
> 
> (I prefer Devices, so that makes it 1 vs 1)
> [...]

Well, I can certainly weigh in, but I don't think you will be pleased, because
my preference leans towards "SystemDevices". But I would even go as far as to
suggest "{,Underlying}{Kernel,System}DeviceIDs" as potential candidates.
Admittedly some of those are long, but consider that

 - most programs won't need this, and
 - those that need will very likely only query it in a single place.

So I think even something like "UnderlyingKernelDeviceIDs" could work,
and I think that name is quite descriptive.

But, of course, since this is likely a very rarely used control, I think both
"Devices" and "SystemDevices" would work just fine.


Regards,
Barnabás Pőcze
Kieran Bingham June 15, 2023, 8:21 a.m. UTC | #10 
Hi Barnabás

Thank you for weighing in!

Quoting Barnabás Pőcze (2023-06-14 22:50:57)
> Hi
> 
> 
> 2023. június 13., kedd 19:10 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> 
> > Quoting Barnabás Pőcze (2023-06-13 16:48:39)
> > > Hi
> > >
> > >
> > > 2023. június 12., hétfő 18:28 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> > >
> > > > [...]
> > > > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > > > > > index cb55e0ed2283..6141942969f9 100644
> > > > > > --- a/src/libcamera/property_ids.yaml
> > > > > > +++ b/src/libcamera/property_ids.yaml
> > > > > > @@ -690,6 +690,13 @@ controls:
> > > > > >          that is twice that of the full resolution mode. This value will be valid
> > > > > >          after the configure method has returned successfully.
> > > > > >
> > > > > > +  - Devices:
> > > > > > +      type: int64_t
> > > > >
> > > > > I am wondering why the type is `int64_t`. POSIX only says `dev_t` is an integer type[0].
> > > > > Both glibc[1] and musl[2] define it as an unsigned (64-bit) integer (at least as far as I checked).
> > > >
> > > > Indeed every implementation I can see define it as a 64-bit, but the
> > > > kernel uses only 32 bits.
> > > >
> > > > This is a libcamera control limitation ultimately, as we don't have
> > > > unsigned integer controls. I'm not sure yet if adding would be helpful
> > > > or difficult. It might be worth trying out.
> > > > [...]
> > >
> > > Ahh, I wasn't aware that libcamera does not have uint controls. Thanks for the clarification.
> > 
> > Have you reviewed the patches enough to consider providing a set of
> > Reviewed-by tags perhaps?

How about this point. Do you conisder that you've looked through this
code enough to say you have reviewed it?

> > 
> > Do you have any thoughts or preferences on the control name? Laurent has
> > discussed the option between "Devices" and "SystemDevices" - I'm really
> > hoping someone will weigh in on which they prefer to be able to move
> > forwards and merge this series.
> > 
> > (I prefer Devices, so that makes it 1 vs 1)
> > [...]
> 
> Well, I can certainly weigh in, but I don't think you will be pleased, because
> my preference leans towards "SystemDevices". But I would even go as far as to

I'm afraid to say I am pleased! I don't mind favouring the least popular
option. That's just my personal preference, but if more people voice
their opinions it breaks an otherwise tie point ;-)

> suggest "{,Underlying}{Kernel,System}DeviceIDs" as potential candidates.
> Admittedly some of those are long, but consider that
> 
>  - most programs won't need this, and
>  - those that need will very likely only query it in a single place.
> 
> So I think even something like "UnderlyingKernelDeviceIDs" could work,
> and I think that name is quite descriptive.
> 
> But, of course, since this is likely a very rarely used control, I think both
> "Devices" and "SystemDevices" would work just fine.

I don't think I want to make the name overly long, but you did throw in
one option that I like more than SystemDevices

 KernelDevices

But now I have a third option to pick from - so I need more voters and
people to weigh in on their opinion ...

Maybe I won't be pleased after all :-) (Joking of course, I'm very happy
for the input - thanks!)

Anyway, as it stands, if no one else comments I'll update this with
SystemDevices to prepare for merge (Unless we raise the votes on
KernelDevices).

Regards
--
Kieran

Patch
diff mbox series 

diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
index cb55e0ed2283..6141942969f9 100644
--- a/src/libcamera/property_ids.yaml
+++ b/src/libcamera/property_ids.yaml
@@ -690,6 +690,13 @@  controls:
         that is twice that of the full resolution mode. This value will be valid
         after the configure method has returned successfully.
 
+  - Devices:
+      type: int64_t
+      size: [n]
+      description: |
+        A list of integer values of type dev_t denoting major and minor device
+        number of the underlying devices used in the operation of this camera.
+
   # ----------------------------------------------------------------------------
   # Draft properties section