Hi everyone
On Mon, 9 Aug 2021 at 13:24, Hans Verkuil <hverkuil-cisco@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >>> ---
> >>> 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 */
> >
>
