* [PATCH 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX
@ 2021-05-17 10:02 David Plowman
2021-05-17 10:02 ` [PATCH 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls David Plowman
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: David Plowman @ 2021-05-17 10:02 UTC (permalink / raw)
To: linux-media; +Cc: David Plowman
Hi
I'd like to propose some new V4L2 controls as defined in the attached
patches. The controls are:
V4L2_CID_NOTIFY_GAIN_RED
V4L2_CID_NOTIFY_GAIN_GREENR
V4L2_CID_NOTIFY_GAIN_BLUE
V4L2_CID_NOTIFY_GAIN_GREENB
The purpose of these controls is to be able to notify a raw sensor
what colour gains will be applied by subsequent processing (such as by
an ISP). Equivalently we can think of them as telling the sensor what
the white point is. Note that the sensor is told these gains but does
not apply them - which the choice of name is trying to convey.
Some sensors need to know these numbers for the processing that they
perform. Here I'm thinking in particular of so-called "quad Bayer"
(also sometimes "tetracell") sensors that have a special read-out mode
that converts the non-standard Bayer pattern into a standard one, also
at full resolution.
Sensors of this type are becoming quite common on cell phones where,
for example, a 48MP sensor may be able to deliver multiple exposures
at 12MP (for HDR processing perhaps) but they may also have a mode as
described above where they can generate a standard Bayer output at
48MP. This processing works better - we might expect less colour
aliasing? - when the sensor knows what values correspond to "white".
One question in my mind is whether it's worth having a control for
each green channel. The sensor I'm currently looking at only wants a
single green gain, but perhaps it's one of those instances where it
would be annoying to put in a single gain and discover, sometime
later, a sensor that wants both. Opinions on the matter always
appreciated!
Thanks very much and best regards
David
David Plowman (2):
media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls
media: v4l2-ctrls: Document V4L2_CID_NOTIFY_GAIN_XXX controls
.../media/v4l/ext-ctrls-image-source.rst | 28 +++++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls.c | 4 +++
include/uapi/linux/v4l2-controls.h | 4 +++
3 files changed, 36 insertions(+)
--
2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-05-17 10:02 [PATCH 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX David Plowman @ 2021-05-17 10:02 ` David Plowman 2021-05-17 10:02 ` [PATCH 2/2] media: v4l2-ctrls: Document " David Plowman 2021-05-27 7:07 ` [PATCH 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX Hans Verkuil 2 siblings, 0 replies; 9+ messages in thread From: David Plowman @ 2021-05-17 10:02 UTC (permalink / raw) To: linux-media; +Cc: David Plowman We add new controls, one for each of the four usual Bayer channels: V4L2_CID_NOTIFY_GAIN_RED V4L2_CID_NOTIFY_GAIN_GREENR V4L2_CID_NOTIFY_GAIN_BLUE V4L2_CID_NOTIFY_GAIN_GREENB These are provided for sensors that need to know what colour gains will be applied to the Bayer channel by subsequent processing (such as by an ISP), even though the sensor will not apply this gain itself. The units, as with analogue gain, are determined by the driver. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- drivers/media/v4l2-core/v4l2-ctrls.c | 4 ++++ include/uapi/linux/v4l2-controls.h | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 0d7fe1bd975a..2f4436e04cf9 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -1150,6 +1150,10 @@ const char *v4l2_ctrl_get_name(u32 id) case V4L2_CID_TEST_PATTERN_GREENR: return "Green (Red) Pixel Value"; case V4L2_CID_TEST_PATTERN_BLUE: return "Blue Pixel Value"; case V4L2_CID_TEST_PATTERN_GREENB: return "Green (Blue) Pixel Value"; + case V4L2_CID_NOTIFY_GAIN_RED: return "Notify Red Gain"; + case V4L2_CID_NOTIFY_GAIN_GREENR: return "Notify Green (Red) Gain"; + case V4L2_CID_NOTIFY_GAIN_BLUE: return "Notify Blue Gain"; + case V4L2_CID_NOTIFY_GAIN_GREENB: return "Notify Green (Blue) Gain"; /* Image processing controls */ /* Keep the order of the 'case's the same as in v4l2-controls.h! */ diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h index d43bec5f1afd..dff5f0d26d4a 100644 --- a/include/uapi/linux/v4l2-controls.h +++ b/include/uapi/linux/v4l2-controls.h @@ -1116,6 +1116,10 @@ enum v4l2_jpeg_chroma_subsampling { #define V4L2_CID_TEST_PATTERN_BLUE (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 6) #define V4L2_CID_TEST_PATTERN_GREENB (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7) #define V4L2_CID_UNIT_CELL_SIZE (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8) +#define V4L2_CID_NOTIFY_GAIN_RED (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9) +#define V4L2_CID_NOTIFY_GAIN_GREENR (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 10) +#define V4L2_CID_NOTIFY_GAIN_BLUE (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 11) +#define V4L2_CID_NOTIFY_GAIN_GREENB (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 12) /* Image processing controls */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] media: v4l2-ctrls: Document V4L2_CID_NOTIFY_GAIN_XXX controls 2021-05-17 10:02 [PATCH 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX David Plowman 2021-05-17 10:02 ` [PATCH 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls David Plowman @ 2021-05-17 10:02 ` David Plowman 2021-05-19 19:01 ` Sakari Ailus 2021-05-27 7:07 ` [PATCH 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX Hans Verkuil 2 siblings, 1 reply; 9+ messages in thread From: David Plowman @ 2021-05-17 10:02 UTC (permalink / raw) To: linux-media; +Cc: David Plowman Add documentation for each of the controls V4L2_CID_NOTIFY_GAIN_RED V4L2_CID_NOTIFY_GAIN_GREENR V4L2_CID_NOTIFY_GAIN_BLUE V4L2_CID_NOTIFY_GAIN_GREENB These controls are required by sensors that need to know what colour gains will be applied to pixels by downstream processing (such as by an ISP), though the sensor does not apply these gains itself. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- .../media/v4l/ext-ctrls-image-source.rst | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst index de43f5c8486d..f824d6c36ae8 100644 --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst @@ -72,3 +72,31 @@ Image Source Control IDs * - __u32 - ``height`` - Height of the area. + +``V4L2_CID_NOTIFY_GAIN_RED (integer)`` + Notify the sensor what gain will be applied to red pixels by the + subsequent processing (such as by an ISP). The sensor is merely + informed of this value in case it performs processing that requires + it, but it is not applied to the output pixels themselves. The + units are determined by the sensor driver. + +``V4L2_CID_NOTIFY_GAIN_GREENR (integer)`` + Notify the sensor what gain will be applied to green pixels (on + red rows) by subsequent processing (such as by an ISP). The sensor + is merely informed of this value in case it performs processing + that requires it, but it is not applied to the output pixels + themselves. The units are determined by the sensor driver. + +``V4L2_CID_NOTIFY_GAIN_BLUE (integer)`` + Notify the sensor what gain will be applied to blue pixels by the + subsequent processing (such as by an ISP). The sensor is merely + informed of this value in case it performs processing that requires + it, but it is not applied to the output pixels themselves. The + units are determined by the sensor driver. + +``V4L2_CID_NOTIFY_GAIN_GREENB (integer)`` + Notify the sensor what gain will be applied to green pixels (on + blue rows) by subsequent processing (such as by an ISP). The sensor + is merely informed of this value in case it performs processing + that requires it, but it is not applied to the output pixels + themselves. The units are determined by the sensor driver. -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] media: v4l2-ctrls: Document V4L2_CID_NOTIFY_GAIN_XXX controls 2021-05-17 10:02 ` [PATCH 2/2] media: v4l2-ctrls: Document " David Plowman @ 2021-05-19 19:01 ` Sakari Ailus 2021-05-19 19:01 ` Sakari Ailus 0 siblings, 1 reply; 9+ messages in thread From: Sakari Ailus @ 2021-05-19 19:01 UTC (permalink / raw) To: David Plowman; +Cc: linux-media Hi David, Thanks for the patch. Cc'ing Laurent, too. On Mon, May 17, 2021 at 11:02:40AM +0100, David Plowman wrote: > Add documentation for each of the controls > > V4L2_CID_NOTIFY_GAIN_RED > V4L2_CID_NOTIFY_GAIN_GREENR > V4L2_CID_NOTIFY_GAIN_BLUE > V4L2_CID_NOTIFY_GAIN_GREENB > > These controls are required by sensors that need to know what colour > gains will be applied to pixels by downstream processing (such as by > an ISP), though the sensor does not apply these gains itself. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../media/v4l/ext-ctrls-image-source.rst | 28 +++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > index de43f5c8486d..f824d6c36ae8 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > @@ -72,3 +72,31 @@ Image Source Control IDs > * - __u32 > - ``height`` > - Height of the area. > + > +``V4L2_CID_NOTIFY_GAIN_RED (integer)`` > + Notify the sensor what gain will be applied to red pixels by the > + subsequent processing (such as by an ISP). The sensor is merely > + informed of this value in case it performs processing that requires > + it, but it is not applied to the output pixels themselves. The > + units are determined by the sensor driver. I wonder if this should say the default value should reflect gain of 1. It probably wouldn't hurt at least. > + > +``V4L2_CID_NOTIFY_GAIN_GREENR (integer)`` > + Notify the sensor what gain will be applied to green pixels (on > + red rows) by subsequent processing (such as by an ISP). The sensor > + is merely informed of this value in case it performs processing > + that requires it, but it is not applied to the output pixels > + themselves. The units are determined by the sensor driver. > + > +``V4L2_CID_NOTIFY_GAIN_BLUE (integer)`` > + Notify the sensor what gain will be applied to blue pixels by the > + subsequent processing (such as by an ISP). The sensor is merely > + informed of this value in case it performs processing that requires > + it, but it is not applied to the output pixels themselves. The > + units are determined by the sensor driver. > + > +``V4L2_CID_NOTIFY_GAIN_GREENB (integer)`` > + Notify the sensor what gain will be applied to green pixels (on > + blue rows) by subsequent processing (such as by an ISP). The sensor > + is merely informed of this value in case it performs processing > + that requires it, but it is not applied to the output pixels > + themselves. The units are determined by the sensor driver. -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] media: v4l2-ctrls: Document V4L2_CID_NOTIFY_GAIN_XXX controls 2021-05-19 19:01 ` Sakari Ailus @ 2021-05-19 19:01 ` Sakari Ailus 2021-05-24 1:07 ` Laurent Pinchart 0 siblings, 1 reply; 9+ messages in thread From: Sakari Ailus @ 2021-05-19 19:01 UTC (permalink / raw) To: David Plowman; +Cc: linux-media, laurent.pinchart Added Laurent to cc. On Wed, May 19, 2021 at 10:01:21PM +0300, Sakari Ailus wrote: > Hi David, > > Thanks for the patch. > > Cc'ing Laurent, too. > > On Mon, May 17, 2021 at 11:02:40AM +0100, David Plowman wrote: > > Add documentation for each of the controls > > > > V4L2_CID_NOTIFY_GAIN_RED > > V4L2_CID_NOTIFY_GAIN_GREENR > > V4L2_CID_NOTIFY_GAIN_BLUE > > V4L2_CID_NOTIFY_GAIN_GREENB > > > > These controls are required by sensors that need to know what colour > > gains will be applied to pixels by downstream processing (such as by > > an ISP), though the sensor does not apply these gains itself. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../media/v4l/ext-ctrls-image-source.rst | 28 +++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > index de43f5c8486d..f824d6c36ae8 100644 > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > @@ -72,3 +72,31 @@ Image Source Control IDs > > * - __u32 > > - ``height`` > > - Height of the area. > > + > > +``V4L2_CID_NOTIFY_GAIN_RED (integer)`` > > + Notify the sensor what gain will be applied to red pixels by the > > + subsequent processing (such as by an ISP). The sensor is merely > > + informed of this value in case it performs processing that requires > > + it, but it is not applied to the output pixels themselves. The > > + units are determined by the sensor driver. > > I wonder if this should say the default value should reflect gain of 1. It > probably wouldn't hurt at least. > > > + > > +``V4L2_CID_NOTIFY_GAIN_GREENR (integer)`` > > + Notify the sensor what gain will be applied to green pixels (on > > + red rows) by subsequent processing (such as by an ISP). The sensor > > + is merely informed of this value in case it performs processing > > + that requires it, but it is not applied to the output pixels > > + themselves. The units are determined by the sensor driver. > > + > > +``V4L2_CID_NOTIFY_GAIN_BLUE (integer)`` > > + Notify the sensor what gain will be applied to blue pixels by the > > + subsequent processing (such as by an ISP). The sensor is merely > > + informed of this value in case it performs processing that requires > > + it, but it is not applied to the output pixels themselves. The > > + units are determined by the sensor driver. > > + > > +``V4L2_CID_NOTIFY_GAIN_GREENB (integer)`` > > + Notify the sensor what gain will be applied to green pixels (on > > + blue rows) by subsequent processing (such as by an ISP). The sensor > > + is merely informed of this value in case it performs processing > > + that requires it, but it is not applied to the output pixels > > + themselves. The units are determined by the sensor driver. > > -- > Kind regards, > > Sakari Ailus -- Sakari Ailus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] media: v4l2-ctrls: Document V4L2_CID_NOTIFY_GAIN_XXX controls 2021-05-19 19:01 ` Sakari Ailus @ 2021-05-24 1:07 ` Laurent Pinchart 2021-05-24 10:31 ` David Plowman 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2021-05-24 1:07 UTC (permalink / raw) To: Sakari Ailus; +Cc: David Plowman, linux-media Hello, On Wed, May 19, 2021 at 10:01:49PM +0300, Sakari Ailus wrote: > Added Laurent to cc. > > On Wed, May 19, 2021 at 10:01:21PM +0300, Sakari Ailus wrote: > > Hi David, > > > > Thanks for the patch. > > > > Cc'ing Laurent, too. > > > > On Mon, May 17, 2021 at 11:02:40AM +0100, David Plowman wrote: > > > Add documentation for each of the controls > > > > > > V4L2_CID_NOTIFY_GAIN_RED > > > V4L2_CID_NOTIFY_GAIN_GREENR > > > V4L2_CID_NOTIFY_GAIN_BLUE > > > V4L2_CID_NOTIFY_GAIN_GREENB > > > > > > These controls are required by sensors that need to know what colour > > > gains will be applied to pixels by downstream processing (such as by > > > an ISP), though the sensor does not apply these gains itself. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > .../media/v4l/ext-ctrls-image-source.rst | 28 +++++++++++++++++++ > > > 1 file changed, 28 insertions(+) > > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > > index de43f5c8486d..f824d6c36ae8 100644 > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > > @@ -72,3 +72,31 @@ Image Source Control IDs > > > * - __u32 > > > - ``height`` > > > - Height of the area. > > > + > > > +``V4L2_CID_NOTIFY_GAIN_RED (integer)`` > > > + Notify the sensor what gain will be applied to red pixels by the > > > + subsequent processing (such as by an ISP). The sensor is merely > > > + informed of this value in case it performs processing that requires > > > + it, but it is not applied to the output pixels themselves. The > > > + units are determined by the sensor driver. > > > > I wonder if this should say the default value should reflect gain of 1. It > > probably wouldn't hurt at least. Seems reasonable to me. David, do you think we could also document that the values of these controls are linear, or would that be too restrictive ? > > > + > > > +``V4L2_CID_NOTIFY_GAIN_GREENR (integer)`` > > > + Notify the sensor what gain will be applied to green pixels (on > > > + red rows) by subsequent processing (such as by an ISP). The sensor > > > + is merely informed of this value in case it performs processing > > > + that requires it, but it is not applied to the output pixels > > > + themselves. The units are determined by the sensor driver. > > > + > > > +``V4L2_CID_NOTIFY_GAIN_BLUE (integer)`` > > > + Notify the sensor what gain will be applied to blue pixels by the > > > + subsequent processing (such as by an ISP). The sensor is merely > > > + informed of this value in case it performs processing that requires > > > + it, but it is not applied to the output pixels themselves. The > > > + units are determined by the sensor driver. > > > + > > > +``V4L2_CID_NOTIFY_GAIN_GREENB (integer)`` > > > + Notify the sensor what gain will be applied to green pixels (on > > > + blue rows) by subsequent processing (such as by an ISP). The sensor > > > + is merely informed of this value in case it performs processing > > > + that requires it, but it is not applied to the output pixels > > > + themselves. The units are determined by the sensor driver. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] media: v4l2-ctrls: Document V4L2_CID_NOTIFY_GAIN_XXX controls 2021-05-24 1:07 ` Laurent Pinchart @ 2021-05-24 10:31 ` David Plowman 2021-05-24 11:27 ` Sakari Ailus 0 siblings, 1 reply; 9+ messages in thread From: David Plowman @ 2021-05-24 10:31 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media Hi Sakari, Laurent, everyone Thanks for the feedback. On Mon, 24 May 2021 at 02:07, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Wed, May 19, 2021 at 10:01:49PM +0300, Sakari Ailus wrote: > > Added Laurent to cc. > > > > On Wed, May 19, 2021 at 10:01:21PM +0300, Sakari Ailus wrote: > > > Hi David, > > > > > > Thanks for the patch. > > > > > > Cc'ing Laurent, too. > > > > > > On Mon, May 17, 2021 at 11:02:40AM +0100, David Plowman wrote: > > > > Add documentation for each of the controls > > > > > > > > V4L2_CID_NOTIFY_GAIN_RED > > > > V4L2_CID_NOTIFY_GAIN_GREENR > > > > V4L2_CID_NOTIFY_GAIN_BLUE > > > > V4L2_CID_NOTIFY_GAIN_GREENB > > > > > > > > These controls are required by sensors that need to know what colour > > > > gains will be applied to pixels by downstream processing (such as by > > > > an ISP), though the sensor does not apply these gains itself. > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > .../media/v4l/ext-ctrls-image-source.rst | 28 +++++++++++++++++++ > > > > 1 file changed, 28 insertions(+) > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > > > index de43f5c8486d..f824d6c36ae8 100644 > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > > > @@ -72,3 +72,31 @@ Image Source Control IDs > > > > * - __u32 > > > > - ``height`` > > > > - Height of the area. > > > > + > > > > +``V4L2_CID_NOTIFY_GAIN_RED (integer)`` > > > > + Notify the sensor what gain will be applied to red pixels by the > > > > + subsequent processing (such as by an ISP). The sensor is merely > > > > + informed of this value in case it performs processing that requires > > > > + it, but it is not applied to the output pixels themselves. The > > > > + units are determined by the sensor driver. > > > > > > I wonder if this should say the default value should reflect gain of 1. It > > > probably wouldn't hurt at least. > > Seems reasonable to me. Yes, I think this is a good idea. > > David, do you think we could also document that the values of these > controls are linear, or would that be too restrictive ? That's an interesting point. I guess I was drawing on the precedent set by analogue gain, but in many respects mandating a linear response, and not letting device specific units escape the driver, might be more convenient for application code. The typical use would, I expect, involve firmware reading colour gains from an ISP and passing them in here. As such, linear is quite likely to be desirable. In the event that there is a conversion to be done, the driver can always take care of it. Further, if we want to go with linear values, I wonder whether we should go all the way and specify it fully. For example we could mandate u8.8 values, so that 256 means 1.0x - this already feels like more than enough range and precision. There's a slight question in my mind as to whether we should specify the range of the control too, in particular whether gains can be less than unity or not. Again, this would be kind to applications, but drivers might have to re-interpret it internally for "fussy" sensors. Any other opinions on that? One side effect of specifying the meaning precisely is that all four necessarily become identical. I hadn't mentioned this previously, but having them different would clearly be a right nuisance! If that sounds good to everyone I'll make up a second version with updated documentation. Thanks and best regards David > > > > > + > > > > +``V4L2_CID_NOTIFY_GAIN_GREENR (integer)`` > > > > + Notify the sensor what gain will be applied to green pixels (on > > > > + red rows) by subsequent processing (such as by an ISP). The sensor > > > > + is merely informed of this value in case it performs processing > > > > + that requires it, but it is not applied to the output pixels > > > > + themselves. The units are determined by the sensor driver. > > > > + > > > > +``V4L2_CID_NOTIFY_GAIN_BLUE (integer)`` > > > > + Notify the sensor what gain will be applied to blue pixels by the > > > > + subsequent processing (such as by an ISP). The sensor is merely > > > > + informed of this value in case it performs processing that requires > > > > + it, but it is not applied to the output pixels themselves. The > > > > + units are determined by the sensor driver. > > > > + > > > > +``V4L2_CID_NOTIFY_GAIN_GREENB (integer)`` > > > > + Notify the sensor what gain will be applied to green pixels (on > > > > + blue rows) by subsequent processing (such as by an ISP). The sensor > > > > + is merely informed of this value in case it performs processing > > > > + that requires it, but it is not applied to the output pixels > > > > + themselves. The units are determined by the sensor driver. > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] media: v4l2-ctrls: Document V4L2_CID_NOTIFY_GAIN_XXX controls 2021-05-24 10:31 ` David Plowman @ 2021-05-24 11:27 ` Sakari Ailus 0 siblings, 0 replies; 9+ messages in thread From: Sakari Ailus @ 2021-05-24 11:27 UTC (permalink / raw) To: David Plowman; +Cc: Laurent Pinchart, linux-media Hi David, On Mon, May 24, 2021 at 11:31:22AM +0100, David Plowman wrote: > Hi Sakari, Laurent, everyone > > Thanks for the feedback. > > On Mon, 24 May 2021 at 02:07, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hello, > > > > On Wed, May 19, 2021 at 10:01:49PM +0300, Sakari Ailus wrote: > > > Added Laurent to cc. > > > > > > On Wed, May 19, 2021 at 10:01:21PM +0300, Sakari Ailus wrote: > > > > Hi David, > > > > > > > > Thanks for the patch. > > > > > > > > Cc'ing Laurent, too. > > > > > > > > On Mon, May 17, 2021 at 11:02:40AM +0100, David Plowman wrote: > > > > > Add documentation for each of the controls > > > > > > > > > > V4L2_CID_NOTIFY_GAIN_RED > > > > > V4L2_CID_NOTIFY_GAIN_GREENR > > > > > V4L2_CID_NOTIFY_GAIN_BLUE > > > > > V4L2_CID_NOTIFY_GAIN_GREENB > > > > > > > > > > These controls are required by sensors that need to know what colour > > > > > gains will be applied to pixels by downstream processing (such as by > > > > > an ISP), though the sensor does not apply these gains itself. > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > .../media/v4l/ext-ctrls-image-source.rst | 28 +++++++++++++++++++ > > > > > 1 file changed, 28 insertions(+) > > > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > > > > index de43f5c8486d..f824d6c36ae8 100644 > > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst > > > > > @@ -72,3 +72,31 @@ Image Source Control IDs > > > > > * - __u32 > > > > > - ``height`` > > > > > - Height of the area. > > > > > + > > > > > +``V4L2_CID_NOTIFY_GAIN_RED (integer)`` > > > > > + Notify the sensor what gain will be applied to red pixels by the > > > > > + subsequent processing (such as by an ISP). The sensor is merely > > > > > + informed of this value in case it performs processing that requires > > > > > + it, but it is not applied to the output pixels themselves. The > > > > > + units are determined by the sensor driver. > > > > > > > > I wonder if this should say the default value should reflect gain of 1. It > > > > probably wouldn't hurt at least. > > > > Seems reasonable to me. > > Yes, I think this is a good idea. > > > > > David, do you think we could also document that the values of these > > controls are linear, or would that be too restrictive ? > > That's an interesting point. I guess I was drawing on the precedent > set by analogue gain, but in many respects mandating a linear > response, and not letting device specific units escape the driver, > might be more convenient for application code. > > The typical use would, I expect, involve firmware reading colour gains > from an ISP and passing them in here. As such, linear is quite likely > to be desirable. In the event that there is a conversion to be done, > the driver can always take care of it. I don't see issues with the approach. > > Further, if we want to go with linear values, I wonder whether we > should go all the way and specify it fully. For example we could > mandate u8.8 values, so that 256 means 1.0x - this already feels like > more than enough range and precision. > > There's a slight question in my mind as to whether we should specify > the range of the control too, in particular whether gains can be less > than unity or not. Again, this would be kind to applications, but > drivers might have to re-interpret it internally for "fussy" sensors. > Any other opinions on that? The controls API already can provide this information, I wouldn't add it to the documentation of a specific control. > > One side effect of specifying the meaning precisely is that all four > necessarily become identical. I hadn't mentioned this previously, but > having them different would clearly be a right nuisance! Agreed. > > If that sounds good to everyone I'll make up a second version with > updated documentation. I wonder how it'd look like if all these controls had a single explanation instead of one for each. I don't think we've done this earlier, but they're all pretty much alike and used together in any case. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX 2021-05-17 10:02 [PATCH 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX David Plowman 2021-05-17 10:02 ` [PATCH 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls David Plowman 2021-05-17 10:02 ` [PATCH 2/2] media: v4l2-ctrls: Document " David Plowman @ 2021-05-27 7:07 ` Hans Verkuil 2 siblings, 0 replies; 9+ messages in thread From: Hans Verkuil @ 2021-05-27 7:07 UTC (permalink / raw) To: David Plowman, linux-media Hi David, On 17/05/2021 12:02, David Plowman wrote: > Hi > > I'd like to propose some new V4L2 controls as defined in the attached > patches. The controls are: > > V4L2_CID_NOTIFY_GAIN_RED > V4L2_CID_NOTIFY_GAIN_GREENR > V4L2_CID_NOTIFY_GAIN_BLUE > V4L2_CID_NOTIFY_GAIN_GREENB > > The purpose of these controls is to be able to notify a raw sensor > what colour gains will be applied by subsequent processing (such as by > an ISP). Equivalently we can think of them as telling the sensor what > the white point is. Note that the sensor is told these gains but does > not apply them - which the choice of name is trying to convey. > > Some sensors need to know these numbers for the processing that they > perform. Here I'm thinking in particular of so-called "quad Bayer" > (also sometimes "tetracell") sensors that have a special read-out mode > that converts the non-standard Bayer pattern into a standard one, also > at full resolution. > > Sensors of this type are becoming quite common on cell phones where, > for example, a 48MP sensor may be able to deliver multiple exposures > at 12MP (for HDR processing perhaps) but they may also have a mode as > described above where they can generate a standard Bayer output at > 48MP. This processing works better - we might expect less colour > aliasing? - when the sensor knows what values correspond to "white". > > One question in my mind is whether it's worth having a control for > each green channel. The sensor I'm currently looking at only wants a > single green gain, but perhaps it's one of those instances where it > would be annoying to put in a single gain and discover, sometime > later, a sensor that wants both. Opinions on the matter always > appreciated! I think I would like to see this series in combination with an actual sensor driver + ISP that uses them. I am a bit concerned about the fact that the units are sensor-specific, so how would an ISP driver be able to set them in a sensor-independent manner? It seems that this would require knowledge about the sensor internals, which is something you want to avoid in a ISP driver. Perhaps this should be a multiplication factor instead? I.e. if the value is X, then the ISP calculated red as the original red value times (X / 100). So if X = 212, then the red gain factor is 2.12. It's probably best to have two controls for green IMHO. Regards, Hans > > Thanks very much and best regards > > David > > David Plowman (2): > media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls > media: v4l2-ctrls: Document V4L2_CID_NOTIFY_GAIN_XXX controls > > .../media/v4l/ext-ctrls-image-source.rst | 28 +++++++++++++++++++ > drivers/media/v4l2-core/v4l2-ctrls.c | 4 +++ > include/uapi/linux/v4l2-controls.h | 4 +++ > 3 files changed, 36 insertions(+) > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-05-27 7:07 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-17 10:02 [PATCH 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX David Plowman 2021-05-17 10:02 ` [PATCH 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls David Plowman 2021-05-17 10:02 ` [PATCH 2/2] media: v4l2-ctrls: Document " David Plowman 2021-05-19 19:01 ` Sakari Ailus 2021-05-19 19:01 ` Sakari Ailus 2021-05-24 1:07 ` Laurent Pinchart 2021-05-24 10:31 ` David Plowman 2021-05-24 11:27 ` Sakari Ailus 2021-05-27 7:07 ` [PATCH 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX Hans Verkuil
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.