| Message ID | 20210809093448.4461-2-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | New V4L2 control V4L2_CID_NOTIFY_GAINS | expand |
Hi Hans, On Mon, Aug 09, 2021 at 01:05:00PM +0200, Hans Verkuil wrote: > On 09/08/2021 11:34, David Plowman wrote: > > We add a new control V4L2_CID_NOTIFY_GAINS which allows the sensor to > > be notified what gains will be applied to the different colour > > channels by subsequent processing (such as by an ISP), even though the > > sensor will not apply any of these gains itself. > > > > For Bayer sensors this will be an array control taking 4 values which > > are the 4 gains arranged in the fixed order B, Gb, Gr and R, > > irrespective of the exact Bayer order of the sensor itself. > > > > The units are in all cases linear with the default value indicating a > > gain of exactly 1. > > So a value of 2 means a gain of 2? Or are these fixed point values? How do > I represent a gain of 1.5? No, the default value corresponds to a x1.0 gain, but it's not 1. If the default is, let's say, 128, then x2.0 will be 256. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 + > > include/uapi/linux/v4l2-controls.h | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > > index 421300e13a41..f87053c83249 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > > @@ -1107,6 +1107,7 @@ 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_GAINS: return "Notify Gains"; > > > > /* Image processing controls */ > > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > > Since this is a standard control, it should also be configured correctly in > v4l2_ctrl_fill(). > > Instead of an array, would a compound control (aka a struct) be better? Then you can > explicitly have field names g, gb, gr and r. > > Is there a specific reason we want an array instead of that? I'm not opposed, but > I'd like to see a rationale for that. Bayer ia only one of the possible CFA patterns for sensors. With a structure containing named b, gb, gr and r fields, we wouldn't be able to support, for instance, RGBW filters. It's not clear at this point what other CFA patterns will be seen in sensors that require this feature, but an array control will be able to more easily support these use cases. > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > > index 5532b5f68493..133e20444939 100644 > > --- a/include/uapi/linux/v4l2-controls.h > > +++ b/include/uapi/linux/v4l2-controls.h > > @@ -1118,6 +1118,7 @@ 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_GAINS (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9) > > > > > > /* Image processing controls */
On 09/08/2021 14:19, Laurent Pinchart wrote: > Hi Hans, > > On Mon, Aug 09, 2021 at 01:05:00PM +0200, Hans Verkuil wrote: >> On 09/08/2021 11:34, David Plowman wrote: >>> We add a new control V4L2_CID_NOTIFY_GAINS which allows the sensor to >>> be notified what gains will be applied to the different colour >>> channels by subsequent processing (such as by an ISP), even though the >>> sensor will not apply any of these gains itself. >>> >>> For Bayer sensors this will be an array control taking 4 values which >>> are the 4 gains arranged in the fixed order B, Gb, Gr and R, >>> irrespective of the exact Bayer order of the sensor itself. >>> >>> The units are in all cases linear with the default value indicating a >>> gain of exactly 1. >> >> So a value of 2 means a gain of 2? Or are these fixed point values? How do >> I represent a gain of 1.5? > > No, the default value corresponds to a x1.0 gain, but it's not 1. If the > default is, let's say, 128, then x2.0 will be 256. Ah, now I get it. Perhaps a small example of this in the documentation patch will help clarify this. > >>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> >>> --- >>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 + >>> include/uapi/linux/v4l2-controls.h | 1 + >>> 2 files changed, 2 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c >>> index 421300e13a41..f87053c83249 100644 >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c >>> @@ -1107,6 +1107,7 @@ 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_GAINS: return "Notify Gains"; >>> >>> /* Image processing controls */ >>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */ >> >> Since this is a standard control, it should also be configured correctly in >> v4l2_ctrl_fill(). >> >> Instead of an array, would a compound control (aka a struct) be better? Then you can >> explicitly have field names g, gb, gr and r. >> >> Is there a specific reason we want an array instead of that? I'm not opposed, but >> I'd like to see a rationale for that. > > Bayer ia only one of the possible CFA patterns for sensors. With a > structure containing named b, gb, gr and r fields, we wouldn't be able > to support, for instance, RGBW filters. It's not clear at this point > what other CFA patterns will be seen in sensors that require this > feature, but an array control will be able to more easily support these > use cases. OK. It is probably a good idea to mention this in the commit log at least. Regards, Hans > >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h >>> index 5532b5f68493..133e20444939 100644 >>> --- a/include/uapi/linux/v4l2-controls.h >>> +++ b/include/uapi/linux/v4l2-controls.h >>> @@ -1118,6 +1118,7 @@ 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_GAINS (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9) >>> >>> >>> /* Image processing controls */ >
Hi everyone On Mon, 9 Aug 2021 at 13:24, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 09/08/2021 14:19, Laurent Pinchart wrote: > > Hi Hans, > > > > On Mon, Aug 09, 2021 at 01:05:00PM +0200, Hans Verkuil wrote: > >> On 09/08/2021 11:34, David Plowman wrote: > >>> We add a new control V4L2_CID_NOTIFY_GAINS which allows the sensor to > >>> be notified what gains will be applied to the different colour > >>> channels by subsequent processing (such as by an ISP), even though the > >>> sensor will not apply any of these gains itself. > >>> > >>> For Bayer sensors this will be an array control taking 4 values which > >>> are the 4 gains arranged in the fixed order B, Gb, Gr and R, > >>> irrespective of the exact Bayer order of the sensor itself. > >>> > >>> The units are in all cases linear with the default value indicating a > >>> gain of exactly 1. > >> > >> So a value of 2 means a gain of 2? Or are these fixed point values? How do > >> I represent a gain of 1.5? > > > > No, the default value corresponds to a x1.0 gain, but it's not 1. If the > > default is, let's say, 128, then x2.0 will be 256. > > Ah, now I get it. Perhaps a small example of this in the documentation patch will > help clarify this. Yes I agree that would be helpful. I'll put that in the next version shortly (just waiting to see if there are any other changes suggested). > > > > >>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > >>> --- > >>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 + > >>> include/uapi/linux/v4l2-controls.h | 1 + > >>> 2 files changed, 2 insertions(+) > >>> > >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > >>> index 421300e13a41..f87053c83249 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c > >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > >>> @@ -1107,6 +1107,7 @@ 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_GAINS: return "Notify Gains"; > >>> > >>> /* Image processing controls */ > >>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > >> > >> Since this is a standard control, it should also be configured correctly in > >> v4l2_ctrl_fill(). Just a small clarification on this. Given that the min/max/default values are really up to the sensor what would I fill in here, maybe just the control type? > >> > >> Instead of an array, would a compound control (aka a struct) be better? Then you can > >> explicitly have field names g, gb, gr and r. > >> > >> Is there a specific reason we want an array instead of that? I'm not opposed, but > >> I'd like to see a rationale for that. > > > > Bayer ia only one of the possible CFA patterns for sensors. With a > > structure containing named b, gb, gr and r fields, we wouldn't be able > > to support, for instance, RGBW filters. It's not clear at this point > > what other CFA patterns will be seen in sensors that require this > > feature, but an array control will be able to more easily support these > > use cases. > > OK. It is probably a good idea to mention this in the commit log at least. Will do! Thanks and best regards David > > Regards, > > Hans > > > > >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > >>> index 5532b5f68493..133e20444939 100644 > >>> --- a/include/uapi/linux/v4l2-controls.h > >>> +++ b/include/uapi/linux/v4l2-controls.h > >>> @@ -1118,6 +1118,7 @@ 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_GAINS (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9) > >>> > >>> > >>> /* Image processing controls */ > > >
Hi David, On Mon, Aug 09, 2021 at 01:31:44PM +0100, David Plowman wrote: > Hi everyone > > On Mon, 9 Aug 2021 at 13:24, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > > > On 09/08/2021 14:19, Laurent Pinchart wrote: > > > Hi Hans, > > > > > > On Mon, Aug 09, 2021 at 01:05:00PM +0200, Hans Verkuil wrote: > > >> On 09/08/2021 11:34, David Plowman wrote: > > >>> We add a new control V4L2_CID_NOTIFY_GAINS which allows the sensor to > > >>> be notified what gains will be applied to the different colour > > >>> channels by subsequent processing (such as by an ISP), even though the > > >>> sensor will not apply any of these gains itself. > > >>> > > >>> For Bayer sensors this will be an array control taking 4 values which > > >>> are the 4 gains arranged in the fixed order B, Gb, Gr and R, > > >>> irrespective of the exact Bayer order of the sensor itself. > > >>> > > >>> The units are in all cases linear with the default value indicating a > > >>> gain of exactly 1. > > >> > > >> So a value of 2 means a gain of 2? Or are these fixed point values? How do > > >> I represent a gain of 1.5? > > > > > > No, the default value corresponds to a x1.0 gain, but it's not 1. If the > > > default is, let's say, 128, then x2.0 will be 256. > > > > Ah, now I get it. Perhaps a small example of this in the documentation patch will > > help clarify this. > > Yes I agree that would be helpful. I'll put that in the next version > shortly (just waiting to see if there are any other changes > suggested). The digital gain control has the same semantics, see Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst .
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c index 421300e13a41..f87053c83249 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c @@ -1107,6 +1107,7 @@ 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_GAINS: return "Notify Gains"; /* 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 5532b5f68493..133e20444939 100644 --- a/include/uapi/linux/v4l2-controls.h +++ b/include/uapi/linux/v4l2-controls.h @@ -1118,6 +1118,7 @@ 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_GAINS (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9) /* Image processing controls */
We add a new control V4L2_CID_NOTIFY_GAINS which allows the sensor to be notified what gains will be applied to the different colour channels by subsequent processing (such as by an ISP), even though the sensor will not apply any of these gains itself. For Bayer sensors this will be an array control taking 4 values which are the 4 gains arranged in the fixed order B, Gb, Gr and R, irrespective of the exact Bayer order of the sensor itself. The units are in all cases linear with the default value indicating a gain of exactly 1. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 + include/uapi/linux/v4l2-controls.h | 1 + 2 files changed, 2 insertions(+)