| Message ID | 20190814202815.32491-3-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-ctrls: Add camera 'location' support | expand |
Hi Jacopo, Thank you for the patch. On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: > Add documentation for the V4L2_CID_LOCATION camera control. The newly > added read-only control reports the camera device mounting position. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > index 51c1d5c9eb00..fc0a02eee6d4 100644 > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > value down. A value of zero stops the motion if one is in progress > and has no effect otherwise. > > +``V4L2_CID_LOCATION (integer)`` Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below. > + This read-only control describes the camera location by reporting its Here too I would mention camera sensor instead of just camera (or possibly imaging sensor). > + mounting position on the device where the camera is installed. This > + control is particularly meaningful for devices which have a well defined > + orientation, such as phones, laptops and portable devices as the camera > + location is expressed as a position relative to the device intended > + usage position. In example, a camera installed on the user-facing side > + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > + position. The DT bindings could use such an example :-) I would extend this to tablets and laptops. > + > + > + Do we need three blank lines ? > +.. flat-table:: > + :header-rows: 0 > + :stub-columns: 0 > + > + * - ``V4L2_LOCATION_FRONT`` > + - The camera device is located on the front side of the device. > + * - ``V4L2_LOCATION_BACK`` > + - The camera device is located on the back side of the device. > + > + > + > .. [#f1] > This control may be changed to a menu control in the future, if more > options are required.
Hi Jacopo, On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: > Add documentation for the V4L2_CID_LOCATION camera control. The newly > added read-only control reports the camera device mounting position. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > index 51c1d5c9eb00..fc0a02eee6d4 100644 > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > value down. A value of zero stops the motion if one is in progress > and has no effect otherwise. > > +``V4L2_CID_LOCATION (integer)`` > + This read-only control describes the camera location by reporting its > + mounting position on the device where the camera is installed. This > + control is particularly meaningful for devices which have a well defined > + orientation, such as phones, laptops and portable devices as the camera > + location is expressed as a position relative to the device intended > + usage position. In example, a camera installed on the user-facing side s/In/For/ > + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > + position. > + > + > + > +.. flat-table:: > + :header-rows: 0 > + :stub-columns: 0 > + > + * - ``V4L2_LOCATION_FRONT`` > + - The camera device is located on the front side of the device. > + * - ``V4L2_LOCATION_BACK`` > + - The camera device is located on the back side of the device. > + > + > + > .. [#f1] > This control may be changed to a menu control in the future, if more > options are required. There's an effective limit of 64 for menus. ACPI has less than ten different locations for a device, I think 64 will be enough here. So I'd be actually in favour of switching to a menu.
Hi Laurent, On Thu, Aug 15, 2019 at 01:43:40AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: > > Add documentation for the V4L2_CID_LOCATION camera control. The newly > > added read-only control reports the camera device mounting position. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > index 51c1d5c9eb00..fc0a02eee6d4 100644 > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > > value down. A value of zero stops the motion if one is in progress > > and has no effect otherwise. > > > > +``V4L2_CID_LOCATION (integer)`` > > Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below. > > > + This read-only control describes the camera location by reporting its > > Here too I would mention camera sensor instead of just camera (or > possibly imaging sensor). > Let's sort this out in the discussion on the dt property. > > + mounting position on the device where the camera is installed. This > > + control is particularly meaningful for devices which have a well defined > > + orientation, such as phones, laptops and portable devices as the camera > > + location is expressed as a position relative to the device intended > > + usage position. In example, a camera installed on the user-facing side > > + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > > + position. > > The DT bindings could use such an example :-) I would extend this to > tablets and laptops. I could copy part of the text there and expand the example device list. > > > + > > + > > + > > Do we need three blank lines ? > I don't know :) I copied this style from the other tables in the file. There doesn't seem to a requirement about this, I just tried to keep the style consistent :) > > +.. flat-table:: > > + :header-rows: 0 > > + :stub-columns: 0 > > + > > + * - ``V4L2_LOCATION_FRONT`` > > + - The camera device is located on the front side of the device. > > + * - ``V4L2_LOCATION_BACK`` > > + - The camera device is located on the back side of the device. > > + > > + > > + > > .. [#f1] > > This control may be changed to a menu control in the future, if more > > options are required. > > -- > Regards, > > Laurent Pinchart
Hi Sakari, On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote: > On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: > > Add documentation for the V4L2_CID_LOCATION camera control. The newly > > added read-only control reports the camera device mounting position. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > index 51c1d5c9eb00..fc0a02eee6d4 100644 > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > > value down. A value of zero stops the motion if one is in progress > > and has no effect otherwise. > > > > +``V4L2_CID_LOCATION (integer)`` > > + This read-only control describes the camera location by reporting its > > + mounting position on the device where the camera is installed. This > > + control is particularly meaningful for devices which have a well defined > > + orientation, such as phones, laptops and portable devices as the camera > > + location is expressed as a position relative to the device intended > > + usage position. In example, a camera installed on the user-facing side > > s/In/For/ > > > + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > > + position. > > + > > + > > + > > +.. flat-table:: > > + :header-rows: 0 > > + :stub-columns: 0 > > + > > + * - ``V4L2_LOCATION_FRONT`` > > + - The camera device is located on the front side of the device. > > + * - ``V4L2_LOCATION_BACK`` > > + - The camera device is located on the back side of the device. > > + > > + > > + > > .. [#f1] > > This control may be changed to a menu control in the future, if more > > options are required. > > There's an effective limit of 64 for menus. ACPI has less than ten > different locations for a device, I think 64 will be enough here. > > So I'd be actually in favour of switching to a menu. Why ? As you explained yourself, it's a static read-only control, all it needs to report is a single value.
On Thu, Aug 15, 2019 at 03:59:38PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote: > > On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: > > > Add documentation for the V4L2_CID_LOCATION camera control. The newly > > > added read-only control reports the camera device mounting position. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > > index 51c1d5c9eb00..fc0a02eee6d4 100644 > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > > @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > > > value down. A value of zero stops the motion if one is in progress > > > and has no effect otherwise. > > > > > > +``V4L2_CID_LOCATION (integer)`` > > > + This read-only control describes the camera location by reporting its > > > + mounting position on the device where the camera is installed. This > > > + control is particularly meaningful for devices which have a well defined > > > + orientation, such as phones, laptops and portable devices as the camera > > > + location is expressed as a position relative to the device intended > > > + usage position. In example, a camera installed on the user-facing side > > > > s/In/For/ > > > > > + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > > > + position. > > > + > > > + > > > + > > > +.. flat-table:: > > > + :header-rows: 0 > > > + :stub-columns: 0 > > > + > > > + * - ``V4L2_LOCATION_FRONT`` > > > + - The camera device is located on the front side of the device. > > > + * - ``V4L2_LOCATION_BACK`` > > > + - The camera device is located on the back side of the device. > > > + > > > + > > > + > > > .. [#f1] > > > This control may be changed to a menu control in the future, if more > > > options are required. > > > > There's an effective limit of 64 for menus. ACPI has less than ten > > different locations for a device, I think 64 will be enough here. > > > > So I'd be actually in favour of switching to a menu. > > Why ? As you explained yourself, it's a static read-only control, all it > needs to report is a single value. Yes. That's true. It wasn't meant for this but it's nevertheless a convenient API to get that information, both as integer and string.
Hi Sakari, On Thu, Aug 15, 2019 at 04:08:49PM +0300, Sakari Ailus wrote: > On Thu, Aug 15, 2019 at 03:59:38PM +0300, Laurent Pinchart wrote: > > On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote: > > > On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: > > > > Add documentation for the V4L2_CID_LOCATION camera control. The newly > > > > added read-only control reports the camera device mounting position. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > > > > 1 file changed, 23 insertions(+) > > > > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > > > index 51c1d5c9eb00..fc0a02eee6d4 100644 > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > > > @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > > > > value down. A value of zero stops the motion if one is in progress > > > > and has no effect otherwise. > > > > > > > > +``V4L2_CID_LOCATION (integer)`` > > > > + This read-only control describes the camera location by reporting its > > > > + mounting position on the device where the camera is installed. This > > > > + control is particularly meaningful for devices which have a well defined > > > > + orientation, such as phones, laptops and portable devices as the camera > > > > + location is expressed as a position relative to the device intended > > > > + usage position. In example, a camera installed on the user-facing side > > > > > > s/In/For/ > > > > > > > + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > > > > + position. > > > > + > > > > + > > > > + > > > > +.. flat-table:: > > > > + :header-rows: 0 > > > > + :stub-columns: 0 > > > > + > > > > + * - ``V4L2_LOCATION_FRONT`` > > > > + - The camera device is located on the front side of the device. > > > > + * - ``V4L2_LOCATION_BACK`` > > > > + - The camera device is located on the back side of the device. > > > > + > > > > + > > > > + > > > > .. [#f1] > > > > This control may be changed to a menu control in the future, if more > > > > options are required. > > > > > > There's an effective limit of 64 for menus. ACPI has less than ten > > > different locations for a device, I think 64 will be enough here. > > > > > > So I'd be actually in favour of switching to a menu. > > > > Why ? As you explained yourself, it's a static read-only control, all it > > needs to report is a single value. > > Yes. That's true. It wasn't meant for this but it's nevertheless a > convenient API to get that information, both as integer and string. But why is that needed ? The integer seems enough to me.
On Thu, Aug 15, 2019 at 04:10:53PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Aug 15, 2019 at 04:08:49PM +0300, Sakari Ailus wrote: > > On Thu, Aug 15, 2019 at 03:59:38PM +0300, Laurent Pinchart wrote: > > > On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote: > > > > On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: > > > > > Add documentation for the V4L2_CID_LOCATION camera control. The newly > > > > > added read-only control reports the camera device mounting position. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > > > .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > > > > > 1 file changed, 23 insertions(+) > > > > > > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > > > > index 51c1d5c9eb00..fc0a02eee6d4 100644 > > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > > > > @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > > > > > value down. A value of zero stops the motion if one is in progress > > > > > and has no effect otherwise. > > > > > > > > > > +``V4L2_CID_LOCATION (integer)`` > > > > > + This read-only control describes the camera location by reporting its > > > > > + mounting position on the device where the camera is installed. This > > > > > + control is particularly meaningful for devices which have a well defined > > > > > + orientation, such as phones, laptops and portable devices as the camera > > > > > + location is expressed as a position relative to the device intended > > > > > + usage position. In example, a camera installed on the user-facing side > > > > > > > > s/In/For/ > > > > > > > > > + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > > > > > + position. > > > > > + > > > > > + > > > > > + > > > > > +.. flat-table:: > > > > > + :header-rows: 0 > > > > > + :stub-columns: 0 > > > > > + > > > > > + * - ``V4L2_LOCATION_FRONT`` > > > > > + - The camera device is located on the front side of the device. > > > > > + * - ``V4L2_LOCATION_BACK`` > > > > > + - The camera device is located on the back side of the device. > > > > > + > > > > > + > > > > > + > > > > > .. [#f1] > > > > > This control may be changed to a menu control in the future, if more > > > > > options are required. > > > > > > > > There's an effective limit of 64 for menus. ACPI has less than ten > > > > different locations for a device, I think 64 will be enough here. > > > > > > > > So I'd be actually in favour of switching to a menu. > > > > > > Why ? As you explained yourself, it's a static read-only control, all it > > > needs to report is a single value. > > > > Yes. That's true. It wasn't meant for this but it's nevertheless a > > convenient API to get that information, both as integer and string. > > But why is that needed ? The integer seems enough to me. Because it's a qualitative control, not a quantitative one.
On Thu, Aug 15, 2019 at 04:15:10PM +0300, Sakari Ailus wrote: > On Thu, Aug 15, 2019 at 04:10:53PM +0300, Laurent Pinchart wrote: > > On Thu, Aug 15, 2019 at 04:08:49PM +0300, Sakari Ailus wrote: > >> On Thu, Aug 15, 2019 at 03:59:38PM +0300, Laurent Pinchart wrote: > >>> On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote: > >>>> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: > >>>>> Add documentation for the V4L2_CID_LOCATION camera control. The newly > >>>>> added read-only control reports the camera device mounting position. > >>>>> > >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>> --- > >>>>> .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > >>>>> 1 file changed, 23 insertions(+) > >>>>> > >>>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > >>>>> index 51c1d5c9eb00..fc0a02eee6d4 100644 > >>>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > >>>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > >>>>> @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > >>>>> value down. A value of zero stops the motion if one is in progress > >>>>> and has no effect otherwise. > >>>>> > >>>>> +``V4L2_CID_LOCATION (integer)`` > >>>>> + This read-only control describes the camera location by reporting its > >>>>> + mounting position on the device where the camera is installed. This > >>>>> + control is particularly meaningful for devices which have a well defined > >>>>> + orientation, such as phones, laptops and portable devices as the camera > >>>>> + location is expressed as a position relative to the device intended > >>>>> + usage position. In example, a camera installed on the user-facing side > >>>> > >>>> s/In/For/ > >>>> > >>>>> + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > >>>>> + position. > >>>>> + > >>>>> + > >>>>> + > >>>>> +.. flat-table:: > >>>>> + :header-rows: 0 > >>>>> + :stub-columns: 0 > >>>>> + > >>>>> + * - ``V4L2_LOCATION_FRONT`` > >>>>> + - The camera device is located on the front side of the device. > >>>>> + * - ``V4L2_LOCATION_BACK`` > >>>>> + - The camera device is located on the back side of the device. > >>>>> + > >>>>> + > >>>>> + > >>>>> .. [#f1] > >>>>> This control may be changed to a menu control in the future, if more > >>>>> options are required. > >>>> > >>>> There's an effective limit of 64 for menus. ACPI has less than ten > >>>> different locations for a device, I think 64 will be enough here. > >>>> > >>>> So I'd be actually in favour of switching to a menu. > >>> > >>> Why ? As you explained yourself, it's a static read-only control, all it > >>> needs to report is a single value. > >> > >> Yes. That's true. It wasn't meant for this but it's nevertheless a > >> convenient API to get that information, both as integer and string. > > > > But why is that needed ? The integer seems enough to me. > > Because it's a qualitative control, not a quantitative one. And ? :-) The integer values are defined in the V4L2 spec, they map to a usage, and a name can easily be derived from that in userspace if desired.
On 8/14/19 10:28 PM, Jacopo Mondi wrote: > Add documentation for the V4L2_CID_LOCATION camera control. The newly > added read-only control reports the camera device mounting position. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > index 51c1d5c9eb00..fc0a02eee6d4 100644 > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > value down. A value of zero stops the motion if one is in progress > and has no effect otherwise. > > +``V4L2_CID_LOCATION (integer)`` > + This read-only control describes the camera location by reporting its > + mounting position on the device where the camera is installed. This > + control is particularly meaningful for devices which have a well defined > + orientation, such as phones, laptops and portable devices as the camera > + location is expressed as a position relative to the device intended > + usage position. In example, a camera installed on the user-facing side > + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > + position. When should this control be created? If there is only one location (e.g. all sensors are front-facing) would you still expose this? Or does it depend on the type of device? And is the sensor in a digital camera front or back facing? (Just curious about what you think about that situation!) Regards, Hans > + > + > + > +.. flat-table:: > + :header-rows: 0 > + :stub-columns: 0 > + > + * - ``V4L2_LOCATION_FRONT`` > + - The camera device is located on the front side of the device. > + * - ``V4L2_LOCATION_BACK`` > + - The camera device is located on the back side of the device. > + > + > + > .. [#f1] > This control may be changed to a menu control in the future, if more > options are required. > -- > 2.22.0 >
Hi Hans, On Thu, Aug 15, 2019 at 03:30:59PM +0200, Hans Verkuil wrote: > On 8/14/19 10:28 PM, Jacopo Mondi wrote: > > Add documentation for the V4L2_CID_LOCATION camera control. The newly > > added read-only control reports the camera device mounting position. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > index 51c1d5c9eb00..fc0a02eee6d4 100644 > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > > value down. A value of zero stops the motion if one is in progress > > and has no effect otherwise. > > > > +``V4L2_CID_LOCATION (integer)`` > > + This read-only control describes the camera location by reporting its > > + mounting position on the device where the camera is installed. This > > + control is particularly meaningful for devices which have a well defined > > + orientation, such as phones, laptops and portable devices as the camera > > + location is expressed as a position relative to the device intended > > + usage position. In example, a camera installed on the user-facing side > > + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > > + position. > > When should this control be created? If there is only one location (e.g. > all sensors are front-facing) would you still expose this? Or does it depend > on the type of device? Those are important questions that need to be answered :-) Going forward I think all camera sensors should expose this, and I'd like a helper function to create the control and set its value based on firmware properties that all (or most) camera sensor drivers should use. That helper function should also create the other mandatory or optional standard controls for camera sensors, such as the pixel rate or link frequency controls. > And is the sensor in a digital camera front or back facing? (Just curious > about what you think about that situation!) I think we should include here a list of supported device types, and for each device type, define what the front location is. All other locations are then derived from that. For a digital camera I would define the front side as facing the scene being photographed. > > + > > + > > + > > +.. flat-table:: > > + :header-rows: 0 > > + :stub-columns: 0 > > + > > + * - ``V4L2_LOCATION_FRONT`` > > + - The camera device is located on the front side of the device. > > + * - ``V4L2_LOCATION_BACK`` > > + - The camera device is located on the back side of the device. > > + > > + > > + > > .. [#f1] > > This control may be changed to a menu control in the future, if more > > options are required.
Hi Hans, On Thu, Aug 15, 2019 at 03:30:59PM +0200, Hans Verkuil wrote: > On 8/14/19 10:28 PM, Jacopo Mondi wrote: > > Add documentation for the V4L2_CID_LOCATION camera control. The newly > > added read-only control reports the camera device mounting position. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > index 51c1d5c9eb00..fc0a02eee6d4 100644 > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > > @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > > value down. A value of zero stops the motion if one is in progress > > and has no effect otherwise. > > > > +``V4L2_CID_LOCATION (integer)`` > > + This read-only control describes the camera location by reporting its > > + mounting position on the device where the camera is installed. This > > + control is particularly meaningful for devices which have a well defined > > + orientation, such as phones, laptops and portable devices as the camera > > + location is expressed as a position relative to the device intended > > + usage position. In example, a camera installed on the user-facing side > > + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > > + position. > > When should this control be created? If there is only one location (e.g. > all sensors are front-facing) would you still expose this? Or does it depend > on the type of device? If it's meaningful for the device, the location might be reported even if there's only a single camera in the system. > > And is the sensor in a digital camera front or back facing? (Just curious > about what you think about that situation!) I would say it really depends on the device type. For a digital camera like a webcam, defining what's front or back doesn't add much value. Wherever the camera sensor is oriented to, that's the front :) The same way, image sensor connected through long cables to the remotely located base board (I'm thinking about cameras installed in cars and connected by coax cables) will hardly have a position defined in the mainline board DTS file, but if someone would like to add "rearview-mirror" to the list of position and use them in their DTS for whatever reason, this control gives a way to retrieve the information easily. I tried to convey this mentioning the "intended usage orientation" of the device, to give the idea that the position is totally dependent on the nature of the device the sensor is installed on. As said, it's easy to define what "front" is for a smartphone, not so easy for a camera in a car. But I would not tie themselves to device specific detail, but instead focus on providing a meachanism to make easy to expose them. In mainline, we could start with very simple "back" and "front" position, and then grow them when the need arises. > > Regards, > > Hans > > > + > > + > > + > > +.. flat-table:: > > + :header-rows: 0 > > + :stub-columns: 0 > > + > > + * - ``V4L2_LOCATION_FRONT`` > > + - The camera device is located on the front side of the device. > > + * - ``V4L2_LOCATION_BACK`` > > + - The camera device is located on the back side of the device. > > + > > + > > + > > .. [#f1] > > This control may be changed to a menu control in the future, if more > > options are required. > > -- > > 2.22.0 > > >
On 8/15/19 12:43 AM, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: >> Add documentation for the V4L2_CID_LOCATION camera control. The newly >> added read-only control reports the camera device mounting position. >> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >> --- >> .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst >> index 51c1d5c9eb00..fc0a02eee6d4 100644 >> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst >> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst >> @@ -510,6 +510,29 @@ enum v4l2_scene_mode - >> value down. A value of zero stops the motion if one is in progress >> and has no effect otherwise. >> >> +``V4L2_CID_LOCATION (integer)`` > > Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below. Probably a better name, if a bit long. But we might need other location controls in the future (e.g. flash location), so CID_LOCATION is just too generic. Regards, Hans > >> + This read-only control describes the camera location by reporting its > > Here too I would mention camera sensor instead of just camera (or > possibly imaging sensor). > >> + mounting position on the device where the camera is installed. This >> + control is particularly meaningful for devices which have a well defined >> + orientation, such as phones, laptops and portable devices as the camera >> + location is expressed as a position relative to the device intended >> + usage position. In example, a camera installed on the user-facing side >> + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` >> + position. > > The DT bindings could use such an example :-) I would extend this to > tablets and laptops. > >> + >> + >> + > > Do we need three blank lines ? > >> +.. flat-table:: >> + :header-rows: 0 >> + :stub-columns: 0 >> + >> + * - ``V4L2_LOCATION_FRONT`` >> + - The camera device is located on the front side of the device. >> + * - ``V4L2_LOCATION_BACK`` >> + - The camera device is located on the back side of the device. >> + >> + >> + >> .. [#f1] >> This control may be changed to a menu control in the future, if more >> options are required. >
On 8/15/19 4:10 PM, Hans Verkuil wrote: > On 8/15/19 12:43 AM, Laurent Pinchart wrote: >> Hi Jacopo, >> >> Thank you for the patch. >> >> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: >>> Add documentation for the V4L2_CID_LOCATION camera control. The newly >>> added read-only control reports the camera device mounting position. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ >>> 1 file changed, 23 insertions(+) >>> >>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst >>> index 51c1d5c9eb00..fc0a02eee6d4 100644 >>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst >>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst >>> @@ -510,6 +510,29 @@ enum v4l2_scene_mode - >>> value down. A value of zero stops the motion if one is in progress >>> and has no effect otherwise. >>> >>> +``V4L2_CID_LOCATION (integer)`` >> >> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below. > > Probably a better name, if a bit long. But we might need other location > controls in the future (e.g. flash location), so CID_LOCATION is just too > generic. Note that the location defines themselves can most likely be used with any LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control. Regards, Hans > > Regards, > > Hans > >> >>> + This read-only control describes the camera location by reporting its >> >> Here too I would mention camera sensor instead of just camera (or >> possibly imaging sensor). >> >>> + mounting position on the device where the camera is installed. This >>> + control is particularly meaningful for devices which have a well defined >>> + orientation, such as phones, laptops and portable devices as the camera >>> + location is expressed as a position relative to the device intended >>> + usage position. In example, a camera installed on the user-facing side >>> + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` >>> + position. >> >> The DT bindings could use such an example :-) I would extend this to >> tablets and laptops. >> >>> + >>> + >>> + >> >> Do we need three blank lines ? >> >>> +.. flat-table:: >>> + :header-rows: 0 >>> + :stub-columns: 0 >>> + >>> + * - ``V4L2_LOCATION_FRONT`` >>> + - The camera device is located on the front side of the device. >>> + * - ``V4L2_LOCATION_BACK`` >>> + - The camera device is located on the back side of the device. >>> + >>> + >>> + >>> .. [#f1] >>> This control may be changed to a menu control in the future, if more >>> options are required. >> >
Hi Hans, On Thu, Aug 15, 2019 at 04:14:38PM +0200, Hans Verkuil wrote: > On 8/15/19 4:10 PM, Hans Verkuil wrote: > > On 8/15/19 12:43 AM, Laurent Pinchart wrote: > >> Hi Jacopo, > >> > >> Thank you for the patch. > >> > >> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: > >>> Add documentation for the V4L2_CID_LOCATION camera control. The newly > >>> added read-only control reports the camera device mounting position. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > >>> 1 file changed, 23 insertions(+) > >>> > >>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > >>> index 51c1d5c9eb00..fc0a02eee6d4 100644 > >>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > >>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > >>> @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > >>> value down. A value of zero stops the motion if one is in progress > >>> and has no effect otherwise. > >>> > >>> +``V4L2_CID_LOCATION (integer)`` > >> > >> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below. > > > > Probably a better name, if a bit long. But we might need other location > > controls in the future (e.g. flash location), so CID_LOCATION is just too > > generic. > Thanks for the feedback. > Note that the location defines themselves can most likely be used with any > LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control. > What do you think instead of the control type? Would a single integer control do or an integer menu one would be better? I see merit in both proposals actually... Once this is clarified, I can send a proper v1. Thanks j > Regards, > > Hans > > > > > Regards, > > > > Hans > > > >> > >>> + This read-only control describes the camera location by reporting its > >> > >> Here too I would mention camera sensor instead of just camera (or > >> possibly imaging sensor). > >> > >>> + mounting position on the device where the camera is installed. This > >>> + control is particularly meaningful for devices which have a well defined > >>> + orientation, such as phones, laptops and portable devices as the camera > >>> + location is expressed as a position relative to the device intended > >>> + usage position. In example, a camera installed on the user-facing side > >>> + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` > >>> + position. > >> > >> The DT bindings could use such an example :-) I would extend this to > >> tablets and laptops. > >> > >>> + > >>> + > >>> + > >> > >> Do we need three blank lines ? > >> > >>> +.. flat-table:: > >>> + :header-rows: 0 > >>> + :stub-columns: 0 > >>> + > >>> + * - ``V4L2_LOCATION_FRONT`` > >>> + - The camera device is located on the front side of the device. > >>> + * - ``V4L2_LOCATION_BACK`` > >>> + - The camera device is located on the back side of the device. > >>> + > >>> + > >>> + > >>> .. [#f1] > >>> This control may be changed to a menu control in the future, if more > >>> options are required. > >> > > >
On 8/15/19 4:34 PM, Jacopo Mondi wrote: > Hi Hans, > > On Thu, Aug 15, 2019 at 04:14:38PM +0200, Hans Verkuil wrote: >> On 8/15/19 4:10 PM, Hans Verkuil wrote: >>> On 8/15/19 12:43 AM, Laurent Pinchart wrote: >>>> Hi Jacopo, >>>> >>>> Thank you for the patch. >>>> >>>> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: >>>>> Add documentation for the V4L2_CID_LOCATION camera control. The newly >>>>> added read-only control reports the camera device mounting position. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>>> --- >>>>> .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ >>>>> 1 file changed, 23 insertions(+) >>>>> >>>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst >>>>> index 51c1d5c9eb00..fc0a02eee6d4 100644 >>>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst >>>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst >>>>> @@ -510,6 +510,29 @@ enum v4l2_scene_mode - >>>>> value down. A value of zero stops the motion if one is in progress >>>>> and has no effect otherwise. >>>>> >>>>> +``V4L2_CID_LOCATION (integer)`` >>>> >>>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below. >>> >>> Probably a better name, if a bit long. But we might need other location >>> controls in the future (e.g. flash location), so CID_LOCATION is just too >>> generic. >> > > Thanks for the feedback. > >> Note that the location defines themselves can most likely be used with any >> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control. >> > > What do you think instead of the control type? Would a single integer > control do or an integer menu one would be better? I see merit in both > proposals actually... Single integer. It's read-only, so it just reports the location. It would be different if this was a writable control: then you need to know which locations are possible to set, and that requires a menu type. But it doesn't make sense to set the location from software. However, the location might change as a result of other changes: e.g. if the camera has motor control of the tilt and the tilt changes from forward facing to downward facing, then the driver might change the location from FRONT to DOWN. A convoluted example perhaps, but this is just brainstorming. Regards, Hans > > Once this is clarified, I can send a proper v1. > > Thanks > j > >> Regards, >> >> Hans >> >>> >>> Regards, >>> >>> Hans >>> >>>> >>>>> + This read-only control describes the camera location by reporting its >>>> >>>> Here too I would mention camera sensor instead of just camera (or >>>> possibly imaging sensor). >>>> >>>>> + mounting position on the device where the camera is installed. This >>>>> + control is particularly meaningful for devices which have a well defined >>>>> + orientation, such as phones, laptops and portable devices as the camera >>>>> + location is expressed as a position relative to the device intended >>>>> + usage position. In example, a camera installed on the user-facing side >>>>> + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` >>>>> + position. >>>> >>>> The DT bindings could use such an example :-) I would extend this to >>>> tablets and laptops. >>>> >>>>> + >>>>> + >>>>> + >>>> >>>> Do we need three blank lines ? >>>> >>>>> +.. flat-table:: >>>>> + :header-rows: 0 >>>>> + :stub-columns: 0 >>>>> + >>>>> + * - ``V4L2_LOCATION_FRONT`` >>>>> + - The camera device is located on the front side of the device. >>>>> + * - ``V4L2_LOCATION_BACK`` >>>>> + - The camera device is located on the back side of the device. >>>>> + >>>>> + >>>>> + >>>>> .. [#f1] >>>>> This control may be changed to a menu control in the future, if more >>>>> options are required. >>>> >>> >>
Hi Hans, On Thu, Aug 15, 2019 at 04:40:03PM +0200, Hans Verkuil wrote: > On 8/15/19 4:34 PM, Jacopo Mondi wrote: > > Hi Hans, > > > > On Thu, Aug 15, 2019 at 04:14:38PM +0200, Hans Verkuil wrote: > >> On 8/15/19 4:10 PM, Hans Verkuil wrote: > >>> On 8/15/19 12:43 AM, Laurent Pinchart wrote: > >>>> Hi Jacopo, > >>>> > >>>> Thank you for the patch. > >>>> > >>>> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote: > >>>>> Add documentation for the V4L2_CID_LOCATION camera control. The newly > >>>>> added read-only control reports the camera device mounting position. > >>>>> > >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>> --- > >>>>> .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ > >>>>> 1 file changed, 23 insertions(+) > >>>>> > >>>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > >>>>> index 51c1d5c9eb00..fc0a02eee6d4 100644 > >>>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > >>>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst > >>>>> @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > >>>>> value down. A value of zero stops the motion if one is in progress > >>>>> and has no effect otherwise. > >>>>> > >>>>> +``V4L2_CID_LOCATION (integer)`` > >>>> > >>>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below. > >>> > >>> Probably a better name, if a bit long. But we might need other location > >>> controls in the future (e.g. flash location), so CID_LOCATION is just too > >>> generic. > >> > > > > Thanks for the feedback. > > > >> Note that the location defines themselves can most likely be used with any > >> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control. > >> > > > > What do you think instead of the control type? Would a single integer > > control do or an integer menu one would be better? I see merit in both > > proposals actually... > > Single integer. It's read-only, so it just reports the location. > > It would be different if this was a writable control: then you need to > know which locations are possible to set, and that requires a menu type. > > But it doesn't make sense to set the location from software. However, the > location might change as a result of other changes: e.g. if the camera > has motor control of the tilt and the tilt changes from forward facing to > downward facing, then the driver might change the location from FRONT > to DOWN. A convoluted example perhaps, but this is just brainstorming. When the camera points to another direction than directly away from the surface, then we need another property to describe that. Location tells where the camera is... well, located. :-)
Hi! > >>>>> @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > >>>>> value down. A value of zero stops the motion if one is in progress > >>>>> and has no effect otherwise. > >>>>> > >>>>> +``V4L2_CID_LOCATION (integer)`` > >>>> > >>>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below. > >>> > >>> Probably a better name, if a bit long. But we might need other location > >>> controls in the future (e.g. flash location), so CID_LOCATION is just too > >>> generic. > >> > > > > Thanks for the feedback. > > > >> Note that the location defines themselves can most likely be used with any > >> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control. > >> > > > > What do you think instead of the control type? Would a single integer > > control do or an integer menu one would be better? I see merit in both > > proposals actually... > > Single integer. It's read-only, so it just reports the location. > > It would be different if this was a writable control: then you need to > know which locations are possible to set, and that requires a menu type. > > But it doesn't make sense to set the location from software. However, the > location might change as a result of other changes: e.g. if the camera > has motor control of the tilt and the tilt changes from forward facing to > downward facing, then the driver might change the location from FRONT > to DOWN. A convoluted example perhaps, but this is just brainstorming. There are phones with exactly such camera setup. And yes, it makes sense to be writable in that case, as software can move the camera in such case. Pavel
Hi Pavel, On Sun, Sep 01, 2019 at 07:24:57PM +0200, Pavel Machek wrote: > >>>>>> @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > >>>>>> value down. A value of zero stops the motion if one is in progress > >>>>>> and has no effect otherwise. > >>>>>> > >>>>>> +``V4L2_CID_LOCATION (integer)`` > >>>>> > >>>>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below. > >>>> > >>>> Probably a better name, if a bit long. But we might need other location > >>>> controls in the future (e.g. flash location), so CID_LOCATION is just too > >>>> generic. > >>> > >> > >> Thanks for the feedback. > >> > >>> Note that the location defines themselves can most likely be used with any > >>> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control. > >>> > >> > >> What do you think instead of the control type? Would a single integer > >> control do or an integer menu one would be better? I see merit in both > >> proposals actually... > > > > Single integer. It's read-only, so it just reports the location. > > > > It would be different if this was a writable control: then you need to > > know which locations are possible to set, and that requires a menu type. > > > > But it doesn't make sense to set the location from software. However, the > > location might change as a result of other changes: e.g. if the camera > > has motor control of the tilt and the tilt changes from forward facing to > > downward facing, then the driver might change the location from FRONT > > to DOWN. A convoluted example perhaps, but this is just brainstorming. > > There are phones with exactly such camera setup. And yes, it makes > sense to be writable in that case, as software can move the camera in > such case. Out of curiosity, what phones are those ?
Hi! > > > Single integer. It's read-only, so it just reports the location. > > > > > > It would be different if this was a writable control: then you need to > > > know which locations are possible to set, and that requires a menu type. > > > > > > But it doesn't make sense to set the location from software. However, the > > > location might change as a result of other changes: e.g. if the camera > > > has motor control of the tilt and the tilt changes from forward facing to > > > downward facing, then the driver might change the location from FRONT > > > to DOWN. A convoluted example perhaps, but this is just brainstorming. > > > > There are phones with exactly such camera setup. And yes, it makes > > sense to be writable in that case, as software can move the camera in > > such case. > > Out of curiosity, what phones are those ? This one: https://www.samsung.com/global/galaxy/galaxy-a80/ Best regards, Pavel
On Mon, Sep 02, 2019 at 10:06:57AM +0200, Pavel Machek wrote: > >>> Single integer. It's read-only, so it just reports the location. > >>> > >>> It would be different if this was a writable control: then you need to > >>> know which locations are possible to set, and that requires a menu type. > >>> > >>> But it doesn't make sense to set the location from software. However, the > >>> location might change as a result of other changes: e.g. if the camera > >>> has motor control of the tilt and the tilt changes from forward facing to > >>> downward facing, then the driver might change the location from FRONT > >>> to DOWN. A convoluted example perhaps, but this is just brainstorming. > >> > >> There are phones with exactly such camera setup. And yes, it makes > >> sense to be writable in that case, as software can move the camera in > >> such case. > > > > Out of curiosity, what phones are those ? > > This one: > > https://www.samsung.com/global/galaxy/galaxy-a80/ Interesting device. I'm not sure we should control that through a location control though, as it seems there's more than the rotation of the camera involved. In any case I wouldn't care about it for now, and turn the location control from read-only to read-write later if needed. We need more information and more thought to support that use case.
On Mon 2019-09-02 11:19:42, Laurent Pinchart wrote: > On Mon, Sep 02, 2019 at 10:06:57AM +0200, Pavel Machek wrote: > > >>> Single integer. It's read-only, so it just reports the location. > > >>> > > >>> It would be different if this was a writable control: then you need to > > >>> know which locations are possible to set, and that requires a menu type. > > >>> > > >>> But it doesn't make sense to set the location from software. However, the > > >>> location might change as a result of other changes: e.g. if the camera > > >>> has motor control of the tilt and the tilt changes from forward facing to > > >>> downward facing, then the driver might change the location from FRONT > > >>> to DOWN. A convoluted example perhaps, but this is just brainstorming. > > >> > > >> There are phones with exactly such camera setup. And yes, it makes > > >> sense to be writable in that case, as software can move the camera in > > >> such case. > > > > > > Out of curiosity, what phones are those ? > > > > This one: > > > > https://www.samsung.com/global/galaxy/galaxy-a80/ > > Interesting device. I'm not sure we should control that through a > location control though, as it seems there's more than the rotation of > the camera involved. In any case I wouldn't care about it for now, and > turn the location control from read-only to read-write later if needed. > We need more information and more thought to support that use case. Well, the mechanism is there just to rotate the camera. Anyway, that phone is probably nowhere close to having mainline support, so... Pavel
Hi Pawel, On Mon, Sep 02, 2019 at 10:27:39AM +0200, Pavel Machek wrote: > On Mon 2019-09-02 11:19:42, Laurent Pinchart wrote: > > On Mon, Sep 02, 2019 at 10:06:57AM +0200, Pavel Machek wrote: > >>>>> Single integer. It's read-only, so it just reports the location. > >>>>> > >>>>> It would be different if this was a writable control: then you need to > >>>>> know which locations are possible to set, and that requires a menu type. > >>>>> > >>>>> But it doesn't make sense to set the location from software. However, the > >>>>> location might change as a result of other changes: e.g. if the camera > >>>>> has motor control of the tilt and the tilt changes from forward facing to > >>>>> downward facing, then the driver might change the location from FRONT > >>>>> to DOWN. A convoluted example perhaps, but this is just brainstorming. > >>>> > >>>> There are phones with exactly such camera setup. And yes, it makes > >>>> sense to be writable in that case, as software can move the camera in > >>>> such case. > >>> > >>> Out of curiosity, what phones are those ? > >> > >> This one: > >> > >> https://www.samsung.com/global/galaxy/galaxy-a80/ > > > > Interesting device. I'm not sure we should control that through a > > location control though, as it seems there's more than the rotation of > > the camera involved. In any case I wouldn't care about it for now, and > > turn the location control from read-only to read-write later if needed. > > We need more information and more thought to support that use case. > > Well, the mechanism is there just to rotate the camera. But we don't know how it's implemented, it could be heavily firmware-based for instance. > Anyway, that phone is probably nowhere close to having mainline > support, so... If we need to support such a device in the future (and I hope we will :-)) then I'm totally fine expanding the features of the location control. My only concern is that I don't want to over-design it right now without having enough information about the hardware that would make use of it.
Hi Pavel, On Sun, Sep 01, 2019 at 07:24:57PM +0200, Pavel Machek wrote: > Hi! > > > >>>>> @@ -510,6 +510,29 @@ enum v4l2_scene_mode - > > >>>>> value down. A value of zero stops the motion if one is in progress > > >>>>> and has no effect otherwise. > > >>>>> > > >>>>> +``V4L2_CID_LOCATION (integer)`` > > >>>> > > >>>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below. > > >>> > > >>> Probably a better name, if a bit long. But we might need other location > > >>> controls in the future (e.g. flash location), so CID_LOCATION is just too > > >>> generic. > > >> > > > > > > Thanks for the feedback. > > > > > >> Note that the location defines themselves can most likely be used with any > > >> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control. > > >> > > > > > > What do you think instead of the control type? Would a single integer > > > control do or an integer menu one would be better? I see merit in both > > > proposals actually... > > > > Single integer. It's read-only, so it just reports the location. > > > > It would be different if this was a writable control: then you need to > > know which locations are possible to set, and that requires a menu type. > > > > But it doesn't make sense to set the location from software. However, the > > location might change as a result of other changes: e.g. if the camera > > has motor control of the tilt and the tilt changes from forward facing to > > downward facing, then the driver might change the location from FRONT > > to DOWN. A convoluted example perhaps, but this is just brainstorming. > > There are phones with exactly such camera setup. And yes, it makes sense to be writable > in that case, as software can move the camera in such case. Nice, I had no idea! Support for those kind of devices seems a bit far-fetched at the moment, and as Laurent suggested, I would make the control writable once we have a use case for that. But let's keep in mind that could happen sooner or later! Thanks j > > Pavel
diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst index 51c1d5c9eb00..fc0a02eee6d4 100644 --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst @@ -510,6 +510,29 @@ enum v4l2_scene_mode - value down. A value of zero stops the motion if one is in progress and has no effect otherwise. +``V4L2_CID_LOCATION (integer)`` + This read-only control describes the camera location by reporting its + mounting position on the device where the camera is installed. This + control is particularly meaningful for devices which have a well defined + orientation, such as phones, laptops and portable devices as the camera + location is expressed as a position relative to the device intended + usage position. In example, a camera installed on the user-facing side + of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT`` + position. + + + +.. flat-table:: + :header-rows: 0 + :stub-columns: 0 + + * - ``V4L2_LOCATION_FRONT`` + - The camera device is located on the front side of the device. + * - ``V4L2_LOCATION_BACK`` + - The camera device is located on the back side of the device. + + + .. [#f1] This control may be changed to a menu control in the future, if more options are required.
Add documentation for the V4L2_CID_LOCATION camera control. The newly added read-only control reports the camera device mounting position. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- .../media/uapi/v4l/ext-ctrls-camera.rst | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) -- 2.22.0