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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> > > > > > --- > > > > > .../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
