| Message ID | 20200805105721.15445-3-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
On 05/08/2020 12:57, Jacopo Mondi wrote: > Provide a table to describe how the V4L2 selection targets can be used > to access an image sensor pixel array properties. > > Reference the table in the sub-device documentation. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ > .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ > 2 files changed, 53 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > index c47861dff9b9b..2f7da3832f458 100644 > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace > can set the analog crop rectangle to select which portion of the pixel array > to read out. > > +A description of each of the above mentioned targets when used to access the > +image sensor pixel array properties is provided by > +:ref:`v4l2-selection-targets-image-sensor-table` > + > > Types of selection targets > -------------------------- > diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > index 69f500093aa2a..632e6448b784e 100644 > --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > @@ -76,3 +76,52 @@ of the two interfaces they are used. > modified by hardware. > - Yes > - No > + > + > +.. _v4l2-selection-targets-image-sensor-table: > + > +******************************************** > +Selection Targets For Pixel Array Properties > +******************************************** > + > +The V4L2 selection API can be used to retrieve the size and disposition of the > +pixel units that compose and image sensor pixel matrix when applied to a video > +sub-device that represents an image sensor. > + > +A description of the properties associated with each of the sensor pixel array > +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. > + > +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| > + > +.. flat-table:: Selection target definitions > + :header-rows: 1 > + :stub-columns: 0 > + > + * - Target name > + - id > + - Definition > + - Read/Write > + * - ``V4L2_SEL_TGT_CROP`` > + - 0x0000 > + - The analog crop rectangle. Represents the portion of the active pixel > + array which is processed to produce images. > + - RW > + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` > + - 0x0001 > + - The active pixel array rectangle. It includes only active pixels and > + excludes other ones such as optical black pixels. Its width and height > + represent the maximum image resolution an image sensor can produce. > + - RO > + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` > + - 0x0002 > + - The readable portion of the physical pixel array matrix. It includes > + pixels that contains valid image data and calibration pixels such as the > + optical black ones. > + - RO > + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` > + - 0x0003 > + - The physical pixel array size, including readable and not readable > + pixels. As pixels that cannot be read from application processor are not > + relevant for calibration purposes, this rectangle is useful to calculate > + the physical properties of the image sensor. > + - RO > Hmm, this basically just duplicates the previous patch. I think you are documenting things at the wrong place. What you documented in the previous patch really belongs here since it is shared between the subdev API and the regular V4L2 API. And in dev-subdev.rst you then refer to here. Frankly, the selection API documentation is a mess. It's spread out over various sections: The VIDIOC_G/S_SELECTION and VIDIOC_SUBDEV_G/S_SELECTION descriptions in the Function Reference, section 8 ("Common definitions for V4L2 and V4L2 subdev interfaces"), 1.25 "Cropping, composing and scaling – the SELECTION API" and 4.13.3.2-4.13.3.3 "Selections: cropping, scaling and composition". In my view section 8 should be moved to section 1.25.2. Ideally 1.25 should be rewritten for both the V4L2 and V4L2 subdev APIs, but that's a lot of work. I would suggest that you add a first patch that moves section 8 to 1.25.2. Note that I don't like the tables for the selection targets and flags: the 'Valid for V4L2/V4L2 subdevs' columns should be removed and it should either be mentioned in the definition if a target/flag is invalid for an API, or it should be put in a separate table. And in 1.25.2 perhaps a new picture can be created for the specific sensor use-case that includes the NATIVE_SIZE target. Another pet peeve of mine is that section 8 splits the selection targets and flags into separate subsections. I'd just keep it in one section. Regards, Hans
Hi Hans, On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote: > On 05/08/2020 12:57, Jacopo Mondi wrote: > > Provide a table to describe how the V4L2 selection targets can be used > > to access an image sensor pixel array properties. > > > > Reference the table in the sub-device documentation. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ > > .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ > > 2 files changed, 53 insertions(+) > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > index c47861dff9b9b..2f7da3832f458 100644 > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace > > can set the analog crop rectangle to select which portion of the pixel array > > to read out. > > > > +A description of each of the above mentioned targets when used to access the > > +image sensor pixel array properties is provided by > > +:ref:`v4l2-selection-targets-image-sensor-table` > > + > > > > Types of selection targets > > -------------------------- > > diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > > index 69f500093aa2a..632e6448b784e 100644 > > --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > > +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > > @@ -76,3 +76,52 @@ of the two interfaces they are used. > > modified by hardware. > > - Yes > > - No > > + > > + > > +.. _v4l2-selection-targets-image-sensor-table: > > + > > +******************************************** > > +Selection Targets For Pixel Array Properties > > +******************************************** > > + > > +The V4L2 selection API can be used to retrieve the size and disposition of the > > +pixel units that compose and image sensor pixel matrix when applied to a video > > +sub-device that represents an image sensor. > > + > > +A description of the properties associated with each of the sensor pixel array > > +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. > > + > > +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| > > + > > +.. flat-table:: Selection target definitions > > + :header-rows: 1 > > + :stub-columns: 0 > > + > > + * - Target name > > + - id > > + - Definition > > + - Read/Write > > + * - ``V4L2_SEL_TGT_CROP`` > > + - 0x0000 > > + - The analog crop rectangle. Represents the portion of the active pixel > > + array which is processed to produce images. > > + - RW > > + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` > > + - 0x0001 > > + - The active pixel array rectangle. It includes only active pixels and > > + excludes other ones such as optical black pixels. Its width and height > > + represent the maximum image resolution an image sensor can produce. > > + - RO > > + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` > > + - 0x0002 > > + - The readable portion of the physical pixel array matrix. It includes > > + pixels that contains valid image data and calibration pixels such as the > > + optical black ones. > > + - RO > > + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` > > + - 0x0003 > > + - The physical pixel array size, including readable and not readable > > + pixels. As pixels that cannot be read from application processor are not > > + relevant for calibration purposes, this rectangle is useful to calculate > > + the physical properties of the image sensor. > > + - RO > > > > Hmm, this basically just duplicates the previous patch. > > I think you are documenting things at the wrong place. What you documented in the > previous patch really belongs here since it is shared between the subdev API and the > regular V4L2 API. And in dev-subdev.rst you then refer to here. I originally had it here, but then I moved to dev-subdev as an image sensor will always be represented as a video sub-device, doen't it ? > > Frankly, the selection API documentation is a mess. It's spread out over various sections: I know :( > The VIDIOC_G/S_SELECTION and VIDIOC_SUBDEV_G/S_SELECTION descriptions in the Function Reference, > section 8 ("Common definitions for V4L2 and V4L2 subdev interfaces"), 1.25 "Cropping, composing > and scaling – the SELECTION API" and 4.13.3.2-4.13.3.3 "Selections: cropping, scaling and composition". > > In my view section 8 should be moved to section 1.25.2. Ideally 1.25 should be rewritten for both > the V4L2 and V4L2 subdev APIs, but that's a lot of work. That's a lot of work indeed. But it could be split as 1) Augment the 1.25.1 section with a mention of subdev, maybe using pieces of 4.13.3.2 2) Move what's in section 8 to be 1.25.x Actually merging 1.25.2 and 8.1.1, splitting 1.25.2 in a device and sub-device section > > I would suggest that you add a first patch that moves section 8 to 1.25.2. Note that I don't like > the tables for the selection targets and flags: the 'Valid for V4L2/V4L2 subdevs' columns should > be removed and it should either be mentioned in the definition if a target/flag is invalid for > an API, or it should be put in a separate table. Two tables seems about right > > And in 1.25.2 perhaps a new picture can be created for the specific sensor use-case that includes > the NATIVE_SIZE target. I know 'image sensor' is not an API concept like device and subdevice, but if we split 1.25.2 in 'video device' and 'subdevice' parts, a section for 'image sensors' with what I've now put in section 4.13.3.3 (the description of the sensor array areas) could fit there ? Seems like a long re-work. Can the imx219 patch be broke out and fast-tracked ? Thanks j > > Another pet peeve of mine is that section 8 splits the selection targets and flags into separate > subsections. I'd just keep it in one section. > > Regards, > > Hans
On 06/08/2020 12:08, Jacopo Mondi wrote: > Hi Hans, > > On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote: >> On 05/08/2020 12:57, Jacopo Mondi wrote: >>> Provide a table to describe how the V4L2 selection targets can be used >>> to access an image sensor pixel array properties. >>> >>> Reference the table in the sub-device documentation. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ >>> .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ >>> 2 files changed, 53 insertions(+) >>> >>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst >>> index c47861dff9b9b..2f7da3832f458 100644 >>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst >>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst >>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace >>> can set the analog crop rectangle to select which portion of the pixel array >>> to read out. >>> >>> +A description of each of the above mentioned targets when used to access the >>> +image sensor pixel array properties is provided by >>> +:ref:`v4l2-selection-targets-image-sensor-table` >>> + >>> >>> Types of selection targets >>> -------------------------- >>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst >>> index 69f500093aa2a..632e6448b784e 100644 >>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst >>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst >>> @@ -76,3 +76,52 @@ of the two interfaces they are used. >>> modified by hardware. >>> - Yes >>> - No >>> + >>> + >>> +.. _v4l2-selection-targets-image-sensor-table: >>> + >>> +******************************************** >>> +Selection Targets For Pixel Array Properties >>> +******************************************** >>> + >>> +The V4L2 selection API can be used to retrieve the size and disposition of the >>> +pixel units that compose and image sensor pixel matrix when applied to a video >>> +sub-device that represents an image sensor. >>> + >>> +A description of the properties associated with each of the sensor pixel array >>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. >>> + >>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| >>> + >>> +.. flat-table:: Selection target definitions >>> + :header-rows: 1 >>> + :stub-columns: 0 >>> + >>> + * - Target name >>> + - id >>> + - Definition >>> + - Read/Write >>> + * - ``V4L2_SEL_TGT_CROP`` >>> + - 0x0000 >>> + - The analog crop rectangle. Represents the portion of the active pixel >>> + array which is processed to produce images. >>> + - RW >>> + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` >>> + - 0x0001 >>> + - The active pixel array rectangle. It includes only active pixels and >>> + excludes other ones such as optical black pixels. Its width and height >>> + represent the maximum image resolution an image sensor can produce. >>> + - RO >>> + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` >>> + - 0x0002 >>> + - The readable portion of the physical pixel array matrix. It includes >>> + pixels that contains valid image data and calibration pixels such as the >>> + optical black ones. >>> + - RO >>> + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` >>> + - 0x0003 >>> + - The physical pixel array size, including readable and not readable >>> + pixels. As pixels that cannot be read from application processor are not >>> + relevant for calibration purposes, this rectangle is useful to calculate >>> + the physical properties of the image sensor. >>> + - RO >>> >> >> Hmm, this basically just duplicates the previous patch. >> >> I think you are documenting things at the wrong place. What you documented in the >> previous patch really belongs here since it is shared between the subdev API and the >> regular V4L2 API. And in dev-subdev.rst you then refer to here. > > I originally had it here, but then I moved to dev-subdev as an image > sensor will always be represented as a video sub-device, doen't it ? No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple platform drivers that don't use the subdev API. > >> >> Frankly, the selection API documentation is a mess. It's spread out over various sections: > > I know :( > >> The VIDIOC_G/S_SELECTION and VIDIOC_SUBDEV_G/S_SELECTION descriptions in the Function Reference, >> section 8 ("Common definitions for V4L2 and V4L2 subdev interfaces"), 1.25 "Cropping, composing >> and scaling – the SELECTION API" and 4.13.3.2-4.13.3.3 "Selections: cropping, scaling and composition". >> >> In my view section 8 should be moved to section 1.25.2. Ideally 1.25 should be rewritten for both >> the V4L2 and V4L2 subdev APIs, but that's a lot of work. > > That's a lot of work indeed. But it could be split as > > 1) Augment the 1.25.1 section with a mention of subdev, maybe using > pieces of 4.13.3.2 > 2) Move what's in section 8 to be 1.25.x > Actually merging 1.25.2 and 8.1.1, splitting 1.25.2 in a device and > sub-device section >> >> I would suggest that you add a first patch that moves section 8 to 1.25.2. Note that I don't like >> the tables for the selection targets and flags: the 'Valid for V4L2/V4L2 subdevs' columns should >> be removed and it should either be mentioned in the definition if a target/flag is invalid for >> an API, or it should be put in a separate table. > > Two tables seems about right > >> >> And in 1.25.2 perhaps a new picture can be created for the specific sensor use-case that includes >> the NATIVE_SIZE target. > > I know 'image sensor' is not an API concept like device and subdevice, > but if we split 1.25.2 in 'video device' and 'subdevice' parts, a > section for 'image sensors' with what I've now put in section 4.13.3.3 > (the description of the sensor array areas) could fit there ? I think so. > > Seems like a long re-work. Can the imx219 patch be broke out and > fast-tracked ? Yes. Regards, Hans > > Thanks > j > >> >> Another pet peeve of mine is that section 8 splits the selection targets and flags into separate >> subsections. I'd just keep it in one section. >> >> Regards, >> >> Hans
Hi Hans, On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote: > On 06/08/2020 12:08, Jacopo Mondi wrote: > > Hi Hans, > > > > On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote: > >> On 05/08/2020 12:57, Jacopo Mondi wrote: > >>> Provide a table to describe how the V4L2 selection targets can be used > >>> to access an image sensor pixel array properties. > >>> > >>> Reference the table in the sub-device documentation. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ > >>> .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ > >>> 2 files changed, 53 insertions(+) > >>> > >>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>> index c47861dff9b9b..2f7da3832f458 100644 > >>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace > >>> can set the analog crop rectangle to select which portion of the pixel array > >>> to read out. > >>> > >>> +A description of each of the above mentioned targets when used to access the > >>> +image sensor pixel array properties is provided by > >>> +:ref:`v4l2-selection-targets-image-sensor-table` > >>> + > >>> > >>> Types of selection targets > >>> -------------------------- > >>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>> index 69f500093aa2a..632e6448b784e 100644 > >>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>> @@ -76,3 +76,52 @@ of the two interfaces they are used. > >>> modified by hardware. > >>> - Yes > >>> - No > >>> + > >>> + > >>> +.. _v4l2-selection-targets-image-sensor-table: > >>> + > >>> +******************************************** > >>> +Selection Targets For Pixel Array Properties > >>> +******************************************** > >>> + > >>> +The V4L2 selection API can be used to retrieve the size and disposition of the > >>> +pixel units that compose and image sensor pixel matrix when applied to a video > >>> +sub-device that represents an image sensor. > >>> + > >>> +A description of the properties associated with each of the sensor pixel array > >>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. > >>> + > >>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| > >>> + > >>> +.. flat-table:: Selection target definitions > >>> + :header-rows: 1 > >>> + :stub-columns: 0 > >>> + > >>> + * - Target name > >>> + - id > >>> + - Definition > >>> + - Read/Write > >>> + * - ``V4L2_SEL_TGT_CROP`` > >>> + - 0x0000 > >>> + - The analog crop rectangle. Represents the portion of the active pixel > >>> + array which is processed to produce images. > >>> + - RW > >>> + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` > >>> + - 0x0001 > >>> + - The active pixel array rectangle. It includes only active pixels and > >>> + excludes other ones such as optical black pixels. Its width and height > >>> + represent the maximum image resolution an image sensor can produce. > >>> + - RO > >>> + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` > >>> + - 0x0002 > >>> + - The readable portion of the physical pixel array matrix. It includes > >>> + pixels that contains valid image data and calibration pixels such as the > >>> + optical black ones. > >>> + - RO > >>> + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` > >>> + - 0x0003 > >>> + - The physical pixel array size, including readable and not readable > >>> + pixels. As pixels that cannot be read from application processor are not > >>> + relevant for calibration purposes, this rectangle is useful to calculate > >>> + the physical properties of the image sensor. > >>> + - RO > >>> > >> > >> Hmm, this basically just duplicates the previous patch. > >> > >> I think you are documenting things at the wrong place. What you documented in the > >> previous patch really belongs here since it is shared between the subdev API and the > >> regular V4L2 API. And in dev-subdev.rst you then refer to here. > > > > I originally had it here, but then I moved to dev-subdev as an image > > sensor will always be represented as a video sub-device, doen't it ? > > No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple > platform drivers that don't use the subdev API. Do we expect to be able to retrieve sensor array properties from video device nodes which represents, in my understanding a DMA engine that writes data to memory ? As I see it, not subdev for the image sensor, no pixel array properties. How can these be exposed by a video device which abstracts the full capture pipeline ?
On 06/08/2020 14:45, Jacopo Mondi wrote: > Hi Hans, > > On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote: >> On 06/08/2020 12:08, Jacopo Mondi wrote: >>> Hi Hans, >>> >>> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote: >>>> On 05/08/2020 12:57, Jacopo Mondi wrote: >>>>> Provide a table to describe how the V4L2 selection targets can be used >>>>> to access an image sensor pixel array properties. >>>>> >>>>> Reference the table in the sub-device documentation. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>>> --- >>>>> .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ >>>>> .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ >>>>> 2 files changed, 53 insertions(+) >>>>> >>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst >>>>> index c47861dff9b9b..2f7da3832f458 100644 >>>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst >>>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst >>>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace >>>>> can set the analog crop rectangle to select which portion of the pixel array >>>>> to read out. >>>>> >>>>> +A description of each of the above mentioned targets when used to access the >>>>> +image sensor pixel array properties is provided by >>>>> +:ref:`v4l2-selection-targets-image-sensor-table` >>>>> + >>>>> >>>>> Types of selection targets >>>>> -------------------------- >>>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst >>>>> index 69f500093aa2a..632e6448b784e 100644 >>>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst >>>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst >>>>> @@ -76,3 +76,52 @@ of the two interfaces they are used. >>>>> modified by hardware. >>>>> - Yes >>>>> - No >>>>> + >>>>> + >>>>> +.. _v4l2-selection-targets-image-sensor-table: >>>>> + >>>>> +******************************************** >>>>> +Selection Targets For Pixel Array Properties >>>>> +******************************************** >>>>> + >>>>> +The V4L2 selection API can be used to retrieve the size and disposition of the >>>>> +pixel units that compose and image sensor pixel matrix when applied to a video >>>>> +sub-device that represents an image sensor. >>>>> + >>>>> +A description of the properties associated with each of the sensor pixel array >>>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. >>>>> + >>>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| >>>>> + >>>>> +.. flat-table:: Selection target definitions >>>>> + :header-rows: 1 >>>>> + :stub-columns: 0 >>>>> + >>>>> + * - Target name >>>>> + - id >>>>> + - Definition >>>>> + - Read/Write >>>>> + * - ``V4L2_SEL_TGT_CROP`` >>>>> + - 0x0000 >>>>> + - The analog crop rectangle. Represents the portion of the active pixel >>>>> + array which is processed to produce images. >>>>> + - RW >>>>> + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` >>>>> + - 0x0001 >>>>> + - The active pixel array rectangle. It includes only active pixels and >>>>> + excludes other ones such as optical black pixels. Its width and height >>>>> + represent the maximum image resolution an image sensor can produce. >>>>> + - RO >>>>> + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` >>>>> + - 0x0002 >>>>> + - The readable portion of the physical pixel array matrix. It includes >>>>> + pixels that contains valid image data and calibration pixels such as the >>>>> + optical black ones. >>>>> + - RO >>>>> + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` >>>>> + - 0x0003 >>>>> + - The physical pixel array size, including readable and not readable >>>>> + pixels. As pixels that cannot be read from application processor are not >>>>> + relevant for calibration purposes, this rectangle is useful to calculate >>>>> + the physical properties of the image sensor. >>>>> + - RO >>>>> >>>> >>>> Hmm, this basically just duplicates the previous patch. >>>> >>>> I think you are documenting things at the wrong place. What you documented in the >>>> previous patch really belongs here since it is shared between the subdev API and the >>>> regular V4L2 API. And in dev-subdev.rst you then refer to here. >>> >>> I originally had it here, but then I moved to dev-subdev as an image >>> sensor will always be represented as a video sub-device, doen't it ? >> >> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple >> platform drivers that don't use the subdev API. > > Do we expect to be able to retrieve sensor array properties from video > device nodes which represents, in my understanding a DMA engine that > writes data to memory ? As I see it, not subdev for the image sensor, > no pixel array properties. How can these be exposed by a video device > which abstracts the full capture pipeline ? They will typically ask the subdev driver. The vidioc_g_selection op implementation will in turn call the get_selection op of the sensor subdev driver and pass that information back to userspace. There is nothing subdev specific to this API. Regards, Hans
Hi Hans, I'm sorry, I don't want to insist but.. On Thu, Aug 06, 2020 at 03:15:54PM +0200, Hans Verkuil wrote: > On 06/08/2020 14:45, Jacopo Mondi wrote: > > Hi Hans, > > > > On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote: > >> On 06/08/2020 12:08, Jacopo Mondi wrote: > >>> Hi Hans, > >>> > >>> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote: > >>>> On 05/08/2020 12:57, Jacopo Mondi wrote: > >>>>> Provide a table to describe how the V4L2 selection targets can be used > >>>>> to access an image sensor pixel array properties. > >>>>> > >>>>> Reference the table in the sub-device documentation. > >>>>> > >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>> --- > >>>>> .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ > >>>>> .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ > >>>>> 2 files changed, 53 insertions(+) > >>>>> > >>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>>>> index c47861dff9b9b..2f7da3832f458 100644 > >>>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace > >>>>> can set the analog crop rectangle to select which portion of the pixel array > >>>>> to read out. > >>>>> > >>>>> +A description of each of the above mentioned targets when used to access the > >>>>> +image sensor pixel array properties is provided by > >>>>> +:ref:`v4l2-selection-targets-image-sensor-table` > >>>>> + > >>>>> > >>>>> Types of selection targets > >>>>> -------------------------- > >>>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>>>> index 69f500093aa2a..632e6448b784e 100644 > >>>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>>>> @@ -76,3 +76,52 @@ of the two interfaces they are used. > >>>>> modified by hardware. > >>>>> - Yes > >>>>> - No > >>>>> + > >>>>> + > >>>>> +.. _v4l2-selection-targets-image-sensor-table: > >>>>> + > >>>>> +******************************************** > >>>>> +Selection Targets For Pixel Array Properties > >>>>> +******************************************** > >>>>> + > >>>>> +The V4L2 selection API can be used to retrieve the size and disposition of the > >>>>> +pixel units that compose and image sensor pixel matrix when applied to a video > >>>>> +sub-device that represents an image sensor. > >>>>> + > >>>>> +A description of the properties associated with each of the sensor pixel array > >>>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. > >>>>> + > >>>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| > >>>>> + > >>>>> +.. flat-table:: Selection target definitions > >>>>> + :header-rows: 1 > >>>>> + :stub-columns: 0 > >>>>> + > >>>>> + * - Target name > >>>>> + - id > >>>>> + - Definition > >>>>> + - Read/Write > >>>>> + * - ``V4L2_SEL_TGT_CROP`` > >>>>> + - 0x0000 > >>>>> + - The analog crop rectangle. Represents the portion of the active pixel > >>>>> + array which is processed to produce images. > >>>>> + - RW > >>>>> + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` > >>>>> + - 0x0001 > >>>>> + - The active pixel array rectangle. It includes only active pixels and > >>>>> + excludes other ones such as optical black pixels. Its width and height > >>>>> + represent the maximum image resolution an image sensor can produce. > >>>>> + - RO > >>>>> + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` > >>>>> + - 0x0002 > >>>>> + - The readable portion of the physical pixel array matrix. It includes > >>>>> + pixels that contains valid image data and calibration pixels such as the > >>>>> + optical black ones. > >>>>> + - RO > >>>>> + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` > >>>>> + - 0x0003 > >>>>> + - The physical pixel array size, including readable and not readable > >>>>> + pixels. As pixels that cannot be read from application processor are not > >>>>> + relevant for calibration purposes, this rectangle is useful to calculate > >>>>> + the physical properties of the image sensor. > >>>>> + - RO > >>>>> > >>>> > >>>> Hmm, this basically just duplicates the previous patch. > >>>> > >>>> I think you are documenting things at the wrong place. What you documented in the > >>>> previous patch really belongs here since it is shared between the subdev API and the > >>>> regular V4L2 API. And in dev-subdev.rst you then refer to here. > >>> > >>> I originally had it here, but then I moved to dev-subdev as an image > >>> sensor will always be represented as a video sub-device, doen't it ? > >> > >> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple > >> platform drivers that don't use the subdev API. > > > > Do we expect to be able to retrieve sensor array properties from video > > device nodes which represents, in my understanding a DMA engine that > > writes data to memory ? As I see it, not subdev for the image sensor, > > no pixel array properties. How can these be exposed by a video device > > which abstracts the full capture pipeline ? > > They will typically ask the subdev driver. The vidioc_g_selection op > implementation will in turn call the get_selection op of the sensor subdev > driver and pass that information back to userspace. How do we know that the any target reports the actual sensor properties and not some other limitation introduced by processing components down the pipeline, as everything sits behind a single video device node ? In example, the default crop rectangle might very well depend on the receiver's side decision to crop the frames received from the sensor to maximize the FOV or whatnot. How do we know which 'cropping point' is reported ? I hardly see a vidio_g_selection() being ideal to report properties of the camera sensor such as the pixel array ones. I still feel we're hijacking that API for that purpose and I would be glad to have dedicated targets for image sensors. This would promote 'image sensors' as first class citizens of the API like devices and sub-devices, and I'm not sure this is desirable. > > There is nothing subdev specific to this API. > > Regards, > > Hans
On 06/08/2020 15:36, Jacopo Mondi wrote: > Hi Hans, > I'm sorry, I don't want to insist but.. > > On Thu, Aug 06, 2020 at 03:15:54PM +0200, Hans Verkuil wrote: >> On 06/08/2020 14:45, Jacopo Mondi wrote: >>> Hi Hans, >>> >>> On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote: >>>> On 06/08/2020 12:08, Jacopo Mondi wrote: >>>>> Hi Hans, >>>>> >>>>> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote: >>>>>> On 05/08/2020 12:57, Jacopo Mondi wrote: >>>>>>> Provide a table to describe how the V4L2 selection targets can be used >>>>>>> to access an image sensor pixel array properties. >>>>>>> >>>>>>> Reference the table in the sub-device documentation. >>>>>>> >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>>>>> --- >>>>>>> .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ >>>>>>> .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ >>>>>>> 2 files changed, 53 insertions(+) >>>>>>> >>>>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst >>>>>>> index c47861dff9b9b..2f7da3832f458 100644 >>>>>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst >>>>>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst >>>>>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace >>>>>>> can set the analog crop rectangle to select which portion of the pixel array >>>>>>> to read out. >>>>>>> >>>>>>> +A description of each of the above mentioned targets when used to access the >>>>>>> +image sensor pixel array properties is provided by >>>>>>> +:ref:`v4l2-selection-targets-image-sensor-table` >>>>>>> + >>>>>>> >>>>>>> Types of selection targets >>>>>>> -------------------------- >>>>>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst >>>>>>> index 69f500093aa2a..632e6448b784e 100644 >>>>>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst >>>>>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst >>>>>>> @@ -76,3 +76,52 @@ of the two interfaces they are used. >>>>>>> modified by hardware. >>>>>>> - Yes >>>>>>> - No >>>>>>> + >>>>>>> + >>>>>>> +.. _v4l2-selection-targets-image-sensor-table: >>>>>>> + >>>>>>> +******************************************** >>>>>>> +Selection Targets For Pixel Array Properties >>>>>>> +******************************************** >>>>>>> + >>>>>>> +The V4L2 selection API can be used to retrieve the size and disposition of the >>>>>>> +pixel units that compose and image sensor pixel matrix when applied to a video >>>>>>> +sub-device that represents an image sensor. >>>>>>> + >>>>>>> +A description of the properties associated with each of the sensor pixel array >>>>>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. >>>>>>> + >>>>>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| >>>>>>> + >>>>>>> +.. flat-table:: Selection target definitions >>>>>>> + :header-rows: 1 >>>>>>> + :stub-columns: 0 >>>>>>> + >>>>>>> + * - Target name >>>>>>> + - id >>>>>>> + - Definition >>>>>>> + - Read/Write >>>>>>> + * - ``V4L2_SEL_TGT_CROP`` >>>>>>> + - 0x0000 >>>>>>> + - The analog crop rectangle. Represents the portion of the active pixel >>>>>>> + array which is processed to produce images. >>>>>>> + - RW >>>>>>> + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` >>>>>>> + - 0x0001 >>>>>>> + - The active pixel array rectangle. It includes only active pixels and >>>>>>> + excludes other ones such as optical black pixels. Its width and height >>>>>>> + represent the maximum image resolution an image sensor can produce. >>>>>>> + - RO >>>>>>> + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` >>>>>>> + - 0x0002 >>>>>>> + - The readable portion of the physical pixel array matrix. It includes >>>>>>> + pixels that contains valid image data and calibration pixels such as the >>>>>>> + optical black ones. >>>>>>> + - RO >>>>>>> + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` >>>>>>> + - 0x0003 >>>>>>> + - The physical pixel array size, including readable and not readable >>>>>>> + pixels. As pixels that cannot be read from application processor are not >>>>>>> + relevant for calibration purposes, this rectangle is useful to calculate >>>>>>> + the physical properties of the image sensor. >>>>>>> + - RO >>>>>>> >>>>>> >>>>>> Hmm, this basically just duplicates the previous patch. >>>>>> >>>>>> I think you are documenting things at the wrong place. What you documented in the >>>>>> previous patch really belongs here since it is shared between the subdev API and the >>>>>> regular V4L2 API. And in dev-subdev.rst you then refer to here. >>>>> >>>>> I originally had it here, but then I moved to dev-subdev as an image >>>>> sensor will always be represented as a video sub-device, doen't it ? >>>> >>>> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple >>>> platform drivers that don't use the subdev API. >>> >>> Do we expect to be able to retrieve sensor array properties from video >>> device nodes which represents, in my understanding a DMA engine that >>> writes data to memory ? As I see it, not subdev for the image sensor, >>> no pixel array properties. How can these be exposed by a video device >>> which abstracts the full capture pipeline ? >> >> They will typically ask the subdev driver. The vidioc_g_selection op >> implementation will in turn call the get_selection op of the sensor subdev >> driver and pass that information back to userspace. > > How do we know that the any target reports the actual sensor > properties and not some other limitation introduced by processing > components down the pipeline, as everything sits behind a single video > device node ? In example, the default crop rectangle might very well depend > on the receiver's side decision to crop the frames received from the > sensor to maximize the FOV or whatnot. How do we know which 'cropping > point' is reported ? Why would you care? Would you do anything different in userspace if a driver would modify these rectangles before passing it on to userspace? > I hardly see a vidio_g_selection() being ideal to report properties of > the camera sensor such as the pixel array ones. I still feel we're > hijacking that API for that purpose and I would be glad to have > dedicated targets for image sensors. This would promote 'image > sensors' as first class citizens of the API like devices and > sub-devices, and I'm not sure this is desirable. Sorry, but the selection API (and its CROP predecessor) has been in use for sensors and webcam drivers since pretty much the beginning of V4L1. I'm not sure what it is that you think is problematical with the current API. Regards, Hans > >> >> There is nothing subdev specific to this API. >> >> Regards, >> >> Hans
Hi Hans, On Thu, Aug 06, 2020 at 05:32:50PM +0200, Hans Verkuil wrote: > On 06/08/2020 15:36, Jacopo Mondi wrote: > > Hi Hans, > > I'm sorry, I don't want to insist but.. > > > > On Thu, Aug 06, 2020 at 03:15:54PM +0200, Hans Verkuil wrote: > >> On 06/08/2020 14:45, Jacopo Mondi wrote: > >>> Hi Hans, > >>> > >>> On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote: > >>>> On 06/08/2020 12:08, Jacopo Mondi wrote: > >>>>> Hi Hans, > >>>>> > >>>>> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote: > >>>>>> On 05/08/2020 12:57, Jacopo Mondi wrote: > >>>>>>> Provide a table to describe how the V4L2 selection targets can be used > >>>>>>> to access an image sensor pixel array properties. > >>>>>>> > >>>>>>> Reference the table in the sub-device documentation. > >>>>>>> > >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>>>> --- > >>>>>>> .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ > >>>>>>> .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ > >>>>>>> 2 files changed, 53 insertions(+) > >>>>>>> > >>>>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>>>>>> index c47861dff9b9b..2f7da3832f458 100644 > >>>>>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>>>>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>>>>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace > >>>>>>> can set the analog crop rectangle to select which portion of the pixel array > >>>>>>> to read out. > >>>>>>> > >>>>>>> +A description of each of the above mentioned targets when used to access the > >>>>>>> +image sensor pixel array properties is provided by > >>>>>>> +:ref:`v4l2-selection-targets-image-sensor-table` > >>>>>>> + > >>>>>>> > >>>>>>> Types of selection targets > >>>>>>> -------------------------- > >>>>>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>>>>>> index 69f500093aa2a..632e6448b784e 100644 > >>>>>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>>>>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>>>>>> @@ -76,3 +76,52 @@ of the two interfaces they are used. > >>>>>>> modified by hardware. > >>>>>>> - Yes > >>>>>>> - No > >>>>>>> + > >>>>>>> + > >>>>>>> +.. _v4l2-selection-targets-image-sensor-table: > >>>>>>> + > >>>>>>> +******************************************** > >>>>>>> +Selection Targets For Pixel Array Properties > >>>>>>> +******************************************** > >>>>>>> + > >>>>>>> +The V4L2 selection API can be used to retrieve the size and disposition of the > >>>>>>> +pixel units that compose and image sensor pixel matrix when applied to a video > >>>>>>> +sub-device that represents an image sensor. > >>>>>>> + > >>>>>>> +A description of the properties associated with each of the sensor pixel array > >>>>>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. > >>>>>>> + > >>>>>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| > >>>>>>> + > >>>>>>> +.. flat-table:: Selection target definitions > >>>>>>> + :header-rows: 1 > >>>>>>> + :stub-columns: 0 > >>>>>>> + > >>>>>>> + * - Target name > >>>>>>> + - id > >>>>>>> + - Definition > >>>>>>> + - Read/Write > >>>>>>> + * - ``V4L2_SEL_TGT_CROP`` > >>>>>>> + - 0x0000 > >>>>>>> + - The analog crop rectangle. Represents the portion of the active pixel > >>>>>>> + array which is processed to produce images. > >>>>>>> + - RW > >>>>>>> + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` > >>>>>>> + - 0x0001 > >>>>>>> + - The active pixel array rectangle. It includes only active pixels and > >>>>>>> + excludes other ones such as optical black pixels. Its width and height > >>>>>>> + represent the maximum image resolution an image sensor can produce. > >>>>>>> + - RO > >>>>>>> + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` > >>>>>>> + - 0x0002 > >>>>>>> + - The readable portion of the physical pixel array matrix. It includes > >>>>>>> + pixels that contains valid image data and calibration pixels such as the > >>>>>>> + optical black ones. > >>>>>>> + - RO > >>>>>>> + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` > >>>>>>> + - 0x0003 > >>>>>>> + - The physical pixel array size, including readable and not readable > >>>>>>> + pixels. As pixels that cannot be read from application processor are not > >>>>>>> + relevant for calibration purposes, this rectangle is useful to calculate > >>>>>>> + the physical properties of the image sensor. > >>>>>>> + - RO > >>>>>>> > >>>>>> > >>>>>> Hmm, this basically just duplicates the previous patch. > >>>>>> > >>>>>> I think you are documenting things at the wrong place. What you documented in the > >>>>>> previous patch really belongs here since it is shared between the subdev API and the > >>>>>> regular V4L2 API. And in dev-subdev.rst you then refer to here. > >>>>> > >>>>> I originally had it here, but then I moved to dev-subdev as an image > >>>>> sensor will always be represented as a video sub-device, doen't it ? > >>>> > >>>> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple > >>>> platform drivers that don't use the subdev API. > >>> > >>> Do we expect to be able to retrieve sensor array properties from video > >>> device nodes which represents, in my understanding a DMA engine that > >>> writes data to memory ? As I see it, not subdev for the image sensor, > >>> no pixel array properties. How can these be exposed by a video device > >>> which abstracts the full capture pipeline ? > >> > >> They will typically ask the subdev driver. The vidioc_g_selection op > >> implementation will in turn call the get_selection op of the sensor subdev > >> driver and pass that information back to userspace. > > > > How do we know that the any target reports the actual sensor > > properties and not some other limitation introduced by processing > > components down the pipeline, as everything sits behind a single video > > device node ? In example, the default crop rectangle might very well depend > > on the receiver's side decision to crop the frames received from the > > sensor to maximize the FOV or whatnot. How do we know which 'cropping > > point' is reported ? > > Why would you care? Would you do anything different in userspace if a driver > would modify these rectangles before passing it on to userspace? > Yes it indeed makes a difference, particularly for application dealing with RAW sensors and for IPS tuning algorithms that need to access the sensor pixel matrix sizes to calculate, in example, the FOV ratio, or might want to access black pixels for calibration purposes. Now, I'm not an expert on that part, but in example I see the RaspberryPi 3A algorthms in libcamera using the ratio between the active pixel array size and what I referred to as 'analogCrop' to calculate the lens shading correction maps https://git.linuxtv.org/libcamera.git/tree/src/ipa/raspberrypi/controller/rpi/alsc.cpp#n185 I can imagine there are other relevant image tuning algorithms that needs to access the sensor characteristics precisely. Knowing that what I'm getting describes the sensor properties is relevant. That's why I hardly see this happening going through a single device node. > > I hardly see a vidio_g_selection() being ideal to report properties of > > the camera sensor such as the pixel array ones. I still feel we're > > hijacking that API for that purpose and I would be glad to have > > dedicated targets for image sensors. This would promote 'image > > sensors' as first class citizens of the API like devices and > > sub-devices, and I'm not sure this is desirable. > > Sorry, but the selection API (and its CROP predecessor) has been in use > for sensors and webcam drivers since pretty much the beginning of V4L1. > > I'm not sure what it is that you think is problematical with the current > API. > I'm not proposing to kill that API, I just think the existing targets fall short or are under-specified when applied to RAW image sensor, that's it :) Thanks j > Regards, > > Hans > > > > >> > >> There is nothing subdev specific to this API. > >> > >> Regards, > >> > >> Hans >
Hi Jacopo and Hans, On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote: > On 05/08/2020 12:57, Jacopo Mondi wrote: > > Provide a table to describe how the V4L2 selection targets can be used > > to access an image sensor pixel array properties. > > > > Reference the table in the sub-device documentation. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ > > .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ > > 2 files changed, 53 insertions(+) > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > index c47861dff9b9b..2f7da3832f458 100644 > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace > > can set the analog crop rectangle to select which portion of the pixel array > > to read out. > > > > +A description of each of the above mentioned targets when used to access the > > +image sensor pixel array properties is provided by > > +:ref:`v4l2-selection-targets-image-sensor-table` > > + > > > > Types of selection targets > > -------------------------- > > diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > > index 69f500093aa2a..632e6448b784e 100644 > > --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > > +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > > @@ -76,3 +76,52 @@ of the two interfaces they are used. > > modified by hardware. > > - Yes > > - No > > + > > + > > +.. _v4l2-selection-targets-image-sensor-table: > > + > > +******************************************** > > +Selection Targets For Pixel Array Properties > > +******************************************** > > + > > +The V4L2 selection API can be used to retrieve the size and disposition of the > > +pixel units that compose and image sensor pixel matrix when applied to a video > > +sub-device that represents an image sensor. > > + > > +A description of the properties associated with each of the sensor pixel array > > +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. > > + > > +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| > > + > > +.. flat-table:: Selection target definitions > > + :header-rows: 1 > > + :stub-columns: 0 > > + > > + * - Target name > > + - id > > + - Definition > > + - Read/Write > > + * - ``V4L2_SEL_TGT_CROP`` > > + - 0x0000 > > + - The analog crop rectangle. Represents the portion of the active pixel > > + array which is processed to produce images. > > + - RW > > + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` > > + - 0x0001 > > + - The active pixel array rectangle. It includes only active pixels and > > + excludes other ones such as optical black pixels. Its width and height > > + represent the maximum image resolution an image sensor can produce. > > + - RO > > + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` > > + - 0x0002 > > + - The readable portion of the physical pixel array matrix. It includes > > + pixels that contains valid image data and calibration pixels such as the > > + optical black ones. > > + - RO > > + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` > > + - 0x0003 > > + - The physical pixel array size, including readable and not readable > > + pixels. As pixels that cannot be read from application processor are not > > + relevant for calibration purposes, this rectangle is useful to calculate > > + the physical properties of the image sensor. > > + - RO > > > > Hmm, this basically just duplicates the previous patch. > > I think you are documenting things at the wrong place. What you documented in the > previous patch really belongs here since it is shared between the subdev API and the > regular V4L2 API. And in dev-subdev.rst you then refer to here. > > Frankly, the selection API documentation is a mess. It's spread out over various sections: > The VIDIOC_G/S_SELECTION and VIDIOC_SUBDEV_G/S_SELECTION descriptions in the Function Reference, > section 8 ("Common definitions for V4L2 and V4L2 subdev interfaces"), 1.25 "Cropping, composing > and scaling – the SELECTION API" and 4.13.3.2-4.13.3.3 "Selections: cropping, scaling and composition". > > In my view section 8 should be moved to section 1.25.2. Agreed. > Ideally 1.25 should be rewritten for both > the V4L2 and V4L2 subdev APIs, but that's a lot of work. Agreed too. > I would suggest that you add a first patch that moves section 8 to 1.25.2. Note that I don't like > the tables for the selection targets and flags: the 'Valid for V4L2/V4L2 subdevs' columns should > be removed and it should either be mentioned in the definition if a target/flag is invalid for > an API, or it should be put in a separate table. Should "1.25.3.1. Configuration of video capture" be moved to "4.1. Video Capture Interface", and "1.25.3.2. Configuration of video output" to "4.3. Video Output Interface" ? I'm not sure where patch 1/4 should be moved to though. As I mentioned in the review, I think it should create a section specific to camera sensors. "4.13. Sub-device Interface" isn't a very good candidate as it describes the interface, it shouldn't document how particular classes of subdevs are to be handled. Maybe a new top-level section in "Part I - Video for Linux API" to describe different types of sub-devices ? > And in 1.25.2 perhaps a new picture can be created for the specific sensor use-case that includes > the NATIVE_SIZE target. > > Another pet peeve of mine is that section 8 splits the selection targets and flags into separate > subsections. I'd just keep it in one section.
Hi Jacopo and Hans, On Thu, Aug 06, 2020 at 06:11:56PM +0200, Jacopo Mondi wrote: > On Thu, Aug 06, 2020 at 05:32:50PM +0200, Hans Verkuil wrote: > > On 06/08/2020 15:36, Jacopo Mondi wrote: > >> On Thu, Aug 06, 2020 at 03:15:54PM +0200, Hans Verkuil wrote: > >>> On 06/08/2020 14:45, Jacopo Mondi wrote: > >>>> On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote: > >>>>> On 06/08/2020 12:08, Jacopo Mondi wrote: > >>>>>> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote: > >>>>>>> On 05/08/2020 12:57, Jacopo Mondi wrote: > >>>>>>>> Provide a table to describe how the V4L2 selection targets can be used > >>>>>>>> to access an image sensor pixel array properties. > >>>>>>>> > >>>>>>>> Reference the table in the sub-device documentation. > >>>>>>>> > >>>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>>>>> --- > >>>>>>>> .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ > >>>>>>>> .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ > >>>>>>>> 2 files changed, 53 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>>>>>>> index c47861dff9b9b..2f7da3832f458 100644 > >>>>>>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > >>>>>>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace > >>>>>>>> can set the analog crop rectangle to select which portion of the pixel array > >>>>>>>> to read out. > >>>>>>>> > >>>>>>>> +A description of each of the above mentioned targets when used to access the > >>>>>>>> +image sensor pixel array properties is provided by > >>>>>>>> +:ref:`v4l2-selection-targets-image-sensor-table` > >>>>>>>> + > >>>>>>>> > >>>>>>>> Types of selection targets > >>>>>>>> -------------------------- > >>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>>>>>>> index 69f500093aa2a..632e6448b784e 100644 > >>>>>>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > >>>>>>>> @@ -76,3 +76,52 @@ of the two interfaces they are used. > >>>>>>>> modified by hardware. > >>>>>>>> - Yes > >>>>>>>> - No > >>>>>>>> + > >>>>>>>> + > >>>>>>>> +.. _v4l2-selection-targets-image-sensor-table: > >>>>>>>> + > >>>>>>>> +******************************************** > >>>>>>>> +Selection Targets For Pixel Array Properties > >>>>>>>> +******************************************** > >>>>>>>> + > >>>>>>>> +The V4L2 selection API can be used to retrieve the size and disposition of the > >>>>>>>> +pixel units that compose and image sensor pixel matrix when applied to a video > >>>>>>>> +sub-device that represents an image sensor. > >>>>>>>> + > >>>>>>>> +A description of the properties associated with each of the sensor pixel array > >>>>>>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. > >>>>>>>> + > >>>>>>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| > >>>>>>>> + > >>>>>>>> +.. flat-table:: Selection target definitions > >>>>>>>> + :header-rows: 1 > >>>>>>>> + :stub-columns: 0 > >>>>>>>> + > >>>>>>>> + * - Target name > >>>>>>>> + - id > >>>>>>>> + - Definition > >>>>>>>> + - Read/Write > >>>>>>>> + * - ``V4L2_SEL_TGT_CROP`` > >>>>>>>> + - 0x0000 > >>>>>>>> + - The analog crop rectangle. Represents the portion of the active pixel > >>>>>>>> + array which is processed to produce images. > >>>>>>>> + - RW > >>>>>>>> + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` > >>>>>>>> + - 0x0001 > >>>>>>>> + - The active pixel array rectangle. It includes only active pixels and > >>>>>>>> + excludes other ones such as optical black pixels. Its width and height > >>>>>>>> + represent the maximum image resolution an image sensor can produce. > >>>>>>>> + - RO > >>>>>>>> + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` > >>>>>>>> + - 0x0002 > >>>>>>>> + - The readable portion of the physical pixel array matrix. It includes > >>>>>>>> + pixels that contains valid image data and calibration pixels such as the > >>>>>>>> + optical black ones. > >>>>>>>> + - RO > >>>>>>>> + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` > >>>>>>>> + - 0x0003 > >>>>>>>> + - The physical pixel array size, including readable and not readable > >>>>>>>> + pixels. As pixels that cannot be read from application processor are not > >>>>>>>> + relevant for calibration purposes, this rectangle is useful to calculate > >>>>>>>> + the physical properties of the image sensor. > >>>>>>>> + - RO > >>>>>>>> > >>>>>>> > >>>>>>> Hmm, this basically just duplicates the previous patch. > >>>>>>> > >>>>>>> I think you are documenting things at the wrong place. What you documented in the > >>>>>>> previous patch really belongs here since it is shared between the subdev API and the > >>>>>>> regular V4L2 API. And in dev-subdev.rst you then refer to here. > >>>>>> > >>>>>> I originally had it here, but then I moved to dev-subdev as an image > >>>>>> sensor will always be represented as a video sub-device, doen't it ? > >>>>> > >>>>> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple > >>>>> platform drivers that don't use the subdev API. > >>>> > >>>> Do we expect to be able to retrieve sensor array properties from video > >>>> device nodes which represents, in my understanding a DMA engine that > >>>> writes data to memory ? As I see it, not subdev for the image sensor, > >>>> no pixel array properties. How can these be exposed by a video device > >>>> which abstracts the full capture pipeline ? > >>> > >>> They will typically ask the subdev driver. The vidioc_g_selection op > >>> implementation will in turn call the get_selection op of the sensor subdev > >>> driver and pass that information back to userspace. > >> > >> How do we know that the any target reports the actual sensor > >> properties and not some other limitation introduced by processing > >> components down the pipeline, as everything sits behind a single video > >> device node ? In example, the default crop rectangle might very well depend > >> on the receiver's side decision to crop the frames received from the > >> sensor to maximize the FOV or whatnot. How do we know which 'cropping > >> point' is reported ? > > > > Why would you care? Would you do anything different in userspace if a driver > > would modify these rectangles before passing it on to userspace? > > Yes it indeed makes a difference, particularly for application dealing > with RAW sensors and for IPS tuning algorithms that need to access the > sensor pixel matrix sizes to calculate, in example, the FOV ratio, or > might want to access black pixels for calibration purposes. > > Now, I'm not an expert on that part, but in example I see the > RaspberryPi 3A algorthms in libcamera using the ratio between the > active pixel array size and what I referred to as 'analogCrop' to > calculate the lens shading correction maps > https://git.linuxtv.org/libcamera.git/tree/src/ipa/raspberrypi/controller/rpi/alsc.cpp#n185 > > I can imagine there are other relevant image tuning algorithms that > needs to access the sensor characteristics precisely. Knowing that > what I'm getting describes the sensor properties is relevant. That's > why I hardly see this happening going through a single device node. > > >> I hardly see a vidio_g_selection() being ideal to report properties of > >> the camera sensor such as the pixel array ones. I still feel we're > >> hijacking that API for that purpose and I would be glad to have > >> dedicated targets for image sensors. This would promote 'image > >> sensors' as first class citizens of the API like devices and > >> sub-devices, and I'm not sure this is desirable. > > > > Sorry, but the selection API (and its CROP predecessor) has been in use > > for sensors and webcam drivers since pretty much the beginning of V4L1. > > > > I'm not sure what it is that you think is problematical with the current > > API. > > I'm not proposing to kill that API, I just think the existing targets > fall short or are under-specified when applied to RAW image sensor, > that's it :) There's a bit difference between what is exposed by a V4L2 video capture device in video-node-centric designs, and by a camera sensor subdev in MC-centric designs. In the first case, the video node exposes a logical view of the device, with the ability to scale and crop if supported, but without any control on how and where those operations are performed. When userspace wants to crop the image, a USB webcam, an embedded system camera (for lack of a better term, I mean here a camera sensor and a receiver, both being controllable by Linux) or an analog capture device are exposed similarly, without applications having to care much about the device type (there are of course device-specific details exposed, such as the analog or digital TV standards, but that's separate). Note that in the USB case, it's common for UVC webcams to expose a set of resolutions, and use a combination of scaling and cropping to achieve them, without reporting to the host how much cropping is applied. The UVC standard allows webcams to crop internally, and doesn't expose any cropping control, so the UVC driver doesn't implement control of the crop rectangle. This is an extreme case that shows how userspace has little control of the internal configuration of the device. Reading optical black pixels is clearly out of the picture with this API, the video-node-centric device is really a simple, high-level view of a logical device. It's good enough for many devices. In the MC-centric case, applications need full control of all processing steps, and that's where getting detailed information about the crop rectangles, and the ability to control them all (including up to 3 different stages of cropping in SMIA++ devices for instance) is important. As Jacopo explained above, the lens shading correction map is calculated based on how much cropping has been applied. Here we're talking about specialized userspace, with the main user being libcamera. libcamera abstracts all this, and exposes a simpler, logical view of the device to applications. In effect, it turns an MC-centric kernel device into a simpler logical device (but with extra features compared to video-node-centric devices, such as a request-based API, and the ability to have some more control over the ISP). For this kind of userspace, the V4L2 video API was not enough, and we have introduced the MC API and the V4L2 subdev API. The lack of a userspace stack on top of that, a long term issue that we're fixing now, has prevented us to see the still numerous shortcomings of our kernel APIs. Vendors who have been using MC-centric drivers to implement proprietary camera stacks have thus resorted to either out-of-tree API extentions or abuses of the existing interfaces. For instance, I've seen out-of-tree drivers using the v4l2_streamparm.capture.capturemode field returned by VIDIOC_G_PARM to retrieve the resolution and crop configuration of the camera sensor by indexing in a table of modes in userspace. There are lots of examples of horrible code, which shows the need not only for API extensions, but for standardizing through documentation how the existing APIS are used. This is what Jacopo is trying to address with this series. I however don't think we need to define extra selection targets (but I'm open to considering that if there are good reasons I'm not aware of), but we very much need to describe in details how the selection targets apply to camera sensor subdevs. This is separate from the V4L2 video API, we may also need better descriptions there, but that should be handled separately. The base concepts and the targets are shared between the two cases, but that's where it stops. > >>> There is nothing subdev specific to this API.
diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst index c47861dff9b9b..2f7da3832f458 100644 --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace can set the analog crop rectangle to select which portion of the pixel array to read out. +A description of each of the above mentioned targets when used to access the +image sensor pixel array properties is provided by +:ref:`v4l2-selection-targets-image-sensor-table` + Types of selection targets -------------------------- diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst index 69f500093aa2a..632e6448b784e 100644 --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst @@ -76,3 +76,52 @@ of the two interfaces they are used. modified by hardware. - Yes - No + + +.. _v4l2-selection-targets-image-sensor-table: + +******************************************** +Selection Targets For Pixel Array Properties +******************************************** + +The V4L2 selection API can be used to retrieve the size and disposition of the +pixel units that compose and image sensor pixel matrix when applied to a video +sub-device that represents an image sensor. + +A description of the properties associated with each of the sensor pixel array +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section. + +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)| + +.. flat-table:: Selection target definitions + :header-rows: 1 + :stub-columns: 0 + + * - Target name + - id + - Definition + - Read/Write + * - ``V4L2_SEL_TGT_CROP`` + - 0x0000 + - The analog crop rectangle. Represents the portion of the active pixel + array which is processed to produce images. + - RW + * - ``V4L2_SEL_TGT_CROP_DEFAULT`` + - 0x0001 + - The active pixel array rectangle. It includes only active pixels and + excludes other ones such as optical black pixels. Its width and height + represent the maximum image resolution an image sensor can produce. + - RO + * - ``V4L2_SEL_TGT_CROP_BOUNDS`` + - 0x0002 + - The readable portion of the physical pixel array matrix. It includes + pixels that contains valid image data and calibration pixels such as the + optical black ones. + - RO + * - ``V4L2_SEL_TGT_NATIVE_SIZE`` + - 0x0003 + - The physical pixel array size, including readable and not readable + pixels. As pixels that cannot be read from application processor are not + relevant for calibration purposes, this rectangle is useful to calculate + the physical properties of the image sensor. + - RO
Provide a table to describe how the V4L2 selection targets can be used to access an image sensor pixel array properties. Reference the table in the sub-device documentation. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- .../userspace-api/media/v4l/dev-subdev.rst | 4 ++ .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++++++++++ 2 files changed, 53 insertions(+)