* [PATCH v3 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX @ 2021-07-22 12:12 David Plowman 2021-07-22 12:12 ` [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls David Plowman 2021-07-22 12:12 ` [PATCH v3 2/2] media: v4l2-ctrls: Document " David Plowman 0 siblings, 2 replies; 13+ messages in thread From: David Plowman @ 2021-07-22 12:12 UTC (permalink / raw) To: linux-media, sakari.ailus, Laurent Pinchart, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil Cc: David Plowman Hi Sorry for the delay with these patches, I think my previous attempt didn't cast the email net wide enough and they got rather lost - my apologies! Anyway, this version addresses the feedback I had received originally, so thank you for that. Therefore: * The values are now specified to be linear gains. * The default value is defined to be the value that is equivalent to a gain of 1x. * I think there was a minor comment over the repetitiveness of the documentation, so I've tried to address that too. 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 | 25 +++++++++++++++++++ drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 +++ include/uapi/linux/v4l2-controls.h | 4 +++ 3 files changed, 33 insertions(+) -- 2.30.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-07-22 12:12 [PATCH v3 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX David Plowman @ 2021-07-22 12:12 ` David Plowman 2021-08-05 15:04 ` Hans Verkuil 2021-08-05 15:22 ` Laurent Pinchart 2021-07-22 12:12 ` [PATCH v3 2/2] media: v4l2-ctrls: Document " David Plowman 1 sibling, 2 replies; 13+ messages in thread From: David Plowman @ 2021-07-22 12:12 UTC (permalink / raw) To: linux-media, sakari.ailus, Laurent Pinchart, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil 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 are 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 | 4 ++++ include/uapi/linux/v4l2-controls.h | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c index b6344bbf1e00..12c810cd4ae6 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c @@ -1106,6 +1106,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 fdf97a6d7d18..711930bb54f0 100644 --- a/include/uapi/linux/v4l2-controls.h +++ b/include/uapi/linux/v4l2-controls.h @@ -1117,6 +1117,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.30.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-07-22 12:12 ` [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls David Plowman @ 2021-08-05 15:04 ` Hans Verkuil 2021-08-05 15:22 ` Laurent Pinchart 1 sibling, 0 replies; 13+ messages in thread From: Hans Verkuil @ 2021-08-05 15:04 UTC (permalink / raw) To: David Plowman, linux-media, sakari.ailus, Laurent Pinchart, Kieran Bingham, Mauro Carvalho Chehab On 22/07/2021 14:12, David Plowman wrote: > 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 are linear with the default value indicating a gain of > exactly 1. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans > --- > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 ++++ > include/uapi/linux/v4l2-controls.h | 4 ++++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > index b6344bbf1e00..12c810cd4ae6 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > @@ -1106,6 +1106,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 fdf97a6d7d18..711930bb54f0 100644 > --- a/include/uapi/linux/v4l2-controls.h > +++ b/include/uapi/linux/v4l2-controls.h > @@ -1117,6 +1117,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 */ > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-07-22 12:12 ` [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls David Plowman 2021-08-05 15:04 ` Hans Verkuil @ 2021-08-05 15:22 ` Laurent Pinchart 2021-08-05 15:40 ` Sakari Ailus 1 sibling, 1 reply; 13+ messages in thread From: Laurent Pinchart @ 2021-08-05 15:22 UTC (permalink / raw) To: David Plowman Cc: linux-media, sakari.ailus, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil Hi David, Thank you for the patch. On Thu, Jul 22, 2021 at 01:12:48PM +0100, David Plowman wrote: > 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 This will effectively limit the API to Bayer patterns. I wonder if we should instead implement it as a single array control, with one element per CFA component. > 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 are 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 | 4 ++++ > include/uapi/linux/v4l2-controls.h | 4 ++++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > index b6344bbf1e00..12c810cd4ae6 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > @@ -1106,6 +1106,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 fdf97a6d7d18..711930bb54f0 100644 > --- a/include/uapi/linux/v4l2-controls.h > +++ b/include/uapi/linux/v4l2-controls.h > @@ -1117,6 +1117,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 */ -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-08-05 15:22 ` Laurent Pinchart @ 2021-08-05 15:40 ` Sakari Ailus 2021-08-05 15:49 ` Laurent Pinchart 0 siblings, 1 reply; 13+ messages in thread From: Sakari Ailus @ 2021-08-05 15:40 UTC (permalink / raw) To: Laurent Pinchart Cc: David Plowman, linux-media, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil Hi David, Laurent, On Thu, Aug 05, 2021 at 06:22:32PM +0300, Laurent Pinchart wrote: > Hi David, > > Thank you for the patch. > > On Thu, Jul 22, 2021 at 01:12:48PM +0100, David Plowman wrote: > > 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 > > This will effectively limit the API to Bayer patterns. I wonder if we > should instead implement it as a single array control, with one element > per CFA component. There are other raw patterns, too. Supporting them would likely require one or a few more controls. That said, as the values change often it's more efficient to use a single control. But each colour combination (not each pattern) would require its own control in this case, eventually requiring more controls. Given that the number of sensors using non-Bayer patterns but still produce Bayer output is likely to grow, these could be used by a number of devices. I'd hence prefer an array control as well. -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-08-05 15:40 ` Sakari Ailus @ 2021-08-05 15:49 ` Laurent Pinchart 2021-08-05 18:05 ` Sakari Ailus 0 siblings, 1 reply; 13+ messages in thread From: Laurent Pinchart @ 2021-08-05 15:49 UTC (permalink / raw) To: Sakari Ailus Cc: David Plowman, linux-media, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil Hi Sakari, On Thu, Aug 05, 2021 at 06:40:42PM +0300, Sakari Ailus wrote: > On Thu, Aug 05, 2021 at 06:22:32PM +0300, Laurent Pinchart wrote: > > On Thu, Jul 22, 2021 at 01:12:48PM +0100, David Plowman wrote: > > > 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 > > > > This will effectively limit the API to Bayer patterns. I wonder if we > > should instead implement it as a single array control, with one element > > per CFA component. > > There are other raw patterns, too. Supporting them would likely require one > or a few more controls. > > That said, as the values change often it's more efficient to use a single > control. But each colour combination (not each pattern) would require its > own control in this case, eventually requiring more controls. I'm not sure to follow you. My idea is to define a single control, with a number of elements that depends on the pattern being used, and the order specified in the native sensor pattern. I don't think each colour combination would then require its own control. > Given that the number of sensors using non-Bayer patterns but still produce > Bayer output is likely to grow, these could be used by a number of devices. > I'd hence prefer an array control as well. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-08-05 15:49 ` Laurent Pinchart @ 2021-08-05 18:05 ` Sakari Ailus 2021-08-06 8:15 ` David Plowman 0 siblings, 1 reply; 13+ messages in thread From: Sakari Ailus @ 2021-08-05 18:05 UTC (permalink / raw) To: Laurent Pinchart Cc: David Plowman, linux-media, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil On Thu, Aug 05, 2021 at 06:49:33PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Aug 05, 2021 at 06:40:42PM +0300, Sakari Ailus wrote: > > On Thu, Aug 05, 2021 at 06:22:32PM +0300, Laurent Pinchart wrote: > > > On Thu, Jul 22, 2021 at 01:12:48PM +0100, David Plowman wrote: > > > > 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 > > > > > > This will effectively limit the API to Bayer patterns. I wonder if we > > > should instead implement it as a single array control, with one element > > > per CFA component. > > > > There are other raw patterns, too. Supporting them would likely require one > > or a few more controls. > > > > That said, as the values change often it's more efficient to use a single > > control. But each colour combination (not each pattern) would require its > > own control in this case, eventually requiring more controls. > > I'm not sure to follow you. My idea is to define a single control, with > a number of elements that depends on the pattern being used, and the > order specified in the native sensor pattern. I don't think each colour > combination would then require its own control. Ah, I guess that would work, too. Then we'll need to define what kind of pixel orders are supported for that single control (and for which formats). -- Sakari Ailus ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-08-05 18:05 ` Sakari Ailus @ 2021-08-06 8:15 ` David Plowman 2021-08-06 8:32 ` Sakari Ailus 2021-08-06 8:38 ` Laurent Pinchart 0 siblings, 2 replies; 13+ messages in thread From: David Plowman @ 2021-08-06 8:15 UTC (permalink / raw) To: Sakari Ailus Cc: Laurent Pinchart, linux-media, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil Hi Sakari, Laurent Thanks for the various comments and discussion. It did prompt me to have some second thoughts about some of the details here, as follows. These controls are aimed specifically at sensors that do some kind of on-board "demosaic / remosaic" process, for instance to produce standard Bayer patterns from non-standard ones. As such the principal requirement is for the sensor to know what "grey" looks like, which we tell it by sending it the red and blue gains from the white balance algorithm. (This obviously enables it to reduce colour aliasing during the processing that it does.) So perhaps the comparison here should be with the existing V4L2_CID_RED/BLUE_BALANCE controls. I'm not sure that it really matters exactly what the colours of the pixels on the sensor really are, it's knowing what "grey" looks like that is important. Any new controls could be quite cumbersome to use if you have to figure out what the underlying pixel arrangement looks like, it certainly feels much easier to refer simply to red/blue pixels, leaving the driver to deal with its own internal idiosyncrasies. Having said that, the particular sensor I have exposes a gain for each of the four Bayer channels, even though I find myself ignoring the green ones!! Anyway, I certainly feel I could go either way on this one - do you have any more thoughts on the matter? Thanks and best regards David On Thu, 5 Aug 2021 at 19:05, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > On Thu, Aug 05, 2021 at 06:49:33PM +0300, Laurent Pinchart wrote: > > Hi Sakari, > > > > On Thu, Aug 05, 2021 at 06:40:42PM +0300, Sakari Ailus wrote: > > > On Thu, Aug 05, 2021 at 06:22:32PM +0300, Laurent Pinchart wrote: > > > > On Thu, Jul 22, 2021 at 01:12:48PM +0100, David Plowman wrote: > > > > > 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 > > > > > > > > This will effectively limit the API to Bayer patterns. I wonder if we > > > > should instead implement it as a single array control, with one element > > > > per CFA component. > > > > > > There are other raw patterns, too. Supporting them would likely require one > > > or a few more controls. > > > > > > That said, as the values change often it's more efficient to use a single > > > control. But each colour combination (not each pattern) would require its > > > own control in this case, eventually requiring more controls. > > > > I'm not sure to follow you. My idea is to define a single control, with > > a number of elements that depends on the pattern being used, and the > > order specified in the native sensor pattern. I don't think each colour > > combination would then require its own control. > > Ah, I guess that would work, too. Then we'll need to define what kind of > pixel orders are supported for that single control (and for which formats). > > -- > Sakari Ailus ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-08-06 8:15 ` David Plowman @ 2021-08-06 8:32 ` Sakari Ailus 2021-08-06 8:38 ` Laurent Pinchart 1 sibling, 0 replies; 13+ messages in thread From: Sakari Ailus @ 2021-08-06 8:32 UTC (permalink / raw) To: David Plowman Cc: Laurent Pinchart, linux-media, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil Hi David, On Fri, Aug 06, 2021 at 09:15:09AM +0100, David Plowman wrote: > Hi Sakari, Laurent > > Thanks for the various comments and discussion. It did prompt me to > have some second thoughts about some of the details here, as follows. > > These controls are aimed specifically at sensors that do some kind of > on-board "demosaic / remosaic" process, for instance to produce > standard Bayer patterns from non-standard ones. As such the principal > requirement is for the sensor to know what "grey" looks like, which we > tell it by sending it the red and blue gains from the white balance > algorithm. (This obviously enables it to reduce colour aliasing during > the processing that it does.) > > So perhaps the comparison here should be with the existing > V4L2_CID_RED/BLUE_BALANCE controls. I'm not sure that it really > matters exactly what the colours of the pixels on the sensor really > are, it's knowing what "grey" looks like that is important. Any new > controls could be quite cumbersome to use if you have to figure out > what the underlying pixel arrangement looks like, it certainly feels > much easier to refer simply to red/blue pixels, leaving the driver to > deal with its own internal idiosyncrasies. I think I missed these are the gains of the Bayer pattern components, and by definition you'll be only setting the gains of those components that are sent out by the sensor. So there wouldn't be other components around, just these four. > > Having said that, the particular sensor I have exposes a gain for each > of the four Bayer channels, even though I find myself ignoring the > green ones!! > > Anyway, I certainly feel I could go either way on this one - do you > have any more thoughts on the matter? I'm leaning towards a single array control with specified, mbus code independent order of components. You should add this control is applicable to Bayer pattern output only. That leaves it easily extendable later on. -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-08-06 8:15 ` David Plowman 2021-08-06 8:32 ` Sakari Ailus @ 2021-08-06 8:38 ` Laurent Pinchart 2021-08-06 10:34 ` David Plowman 1 sibling, 1 reply; 13+ messages in thread From: Laurent Pinchart @ 2021-08-06 8:38 UTC (permalink / raw) To: David Plowman Cc: Sakari Ailus, linux-media, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil Hi David, On Fri, Aug 06, 2021 at 09:15:09AM +0100, David Plowman wrote: > Hi Sakari, Laurent > > Thanks for the various comments and discussion. It did prompt me to > have some second thoughts about some of the details here, as follows. > > These controls are aimed specifically at sensors that do some kind of > on-board "demosaic / remosaic" process, for instance to produce > standard Bayer patterns from non-standard ones. As such the principal > requirement is for the sensor to know what "grey" looks like, which we > tell it by sending it the red and blue gains from the white balance > algorithm. (This obviously enables it to reduce colour aliasing during > the processing that it does.) > > So perhaps the comparison here should be with the existing > V4L2_CID_RED/BLUE_BALANCE controls. I'm not sure that it really > matters exactly what the colours of the pixels on the sensor really > are, it's knowing what "grey" looks like that is important. Any new > controls could be quite cumbersome to use if you have to figure out > what the underlying pixel arrangement looks like, it certainly feels > much easier to refer simply to red/blue pixels, leaving the driver to > deal with its own internal idiosyncrasies. > > Having said that, the particular sensor I have exposes a gain for each > of the four Bayer channels, even though I find myself ignoring the > green ones!! > > Anyway, I certainly feel I could go either way on this one - do you > have any more thoughts on the matter? With an array control, we would have to decide (and document) which components are stored in the array, and in which order. For Bayer sensors, that would be B, Gb, Gr and R. As for the order, there are three options: - fixed order (e.g. always B, Gb, Gr, R) - matching the sensor's CFA native order (always the same for a given sensor, but varying between different sensors) - matching the currently configured format (the bayer pattern can change when moving the crop rectangle by one pixels or when mirroring the sensor readout) The last two options seem quite impractical to me. The first option, if we only consider Bayer sensors, is equivalent to the four controls your propose in the sense that the semantics is defined in the control specification and identical for all sensors, but with the advantage of bundling all four values together. It will also allow expanding this to other patterns if the need arise, which I think would be useful. If we were to redesign the red/blue gains, I'd go for a single array control today, with one value per CFA component. > On Thu, 5 Aug 2021 at 19:05, Sakari Ailus wrote: > > On Thu, Aug 05, 2021 at 06:49:33PM +0300, Laurent Pinchart wrote: > > > On Thu, Aug 05, 2021 at 06:40:42PM +0300, Sakari Ailus wrote: > > > > On Thu, Aug 05, 2021 at 06:22:32PM +0300, Laurent Pinchart wrote: > > > > > On Thu, Jul 22, 2021 at 01:12:48PM +0100, David Plowman wrote: > > > > > > 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 > > > > > > > > > > This will effectively limit the API to Bayer patterns. I wonder if we > > > > > should instead implement it as a single array control, with one element > > > > > per CFA component. > > > > > > > > There are other raw patterns, too. Supporting them would likely require one > > > > or a few more controls. > > > > > > > > That said, as the values change often it's more efficient to use a single > > > > control. But each colour combination (not each pattern) would require its > > > > own control in this case, eventually requiring more controls. > > > > > > I'm not sure to follow you. My idea is to define a single control, with > > > a number of elements that depends on the pattern being used, and the > > > order specified in the native sensor pattern. I don't think each colour > > > combination would then require its own control. > > > > Ah, I guess that would work, too. Then we'll need to define what kind of > > pixel orders are supported for that single control (and for which formats). -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls 2021-08-06 8:38 ` Laurent Pinchart @ 2021-08-06 10:34 ` David Plowman 0 siblings, 0 replies; 13+ messages in thread From: David Plowman @ 2021-08-06 10:34 UTC (permalink / raw) To: Laurent Pinchart Cc: Sakari Ailus, linux-media, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil Hi Laurent Thanks for the reply. Yes, I agree that a fixed order (B, Gb, Gr, R) is much easier to use, and I think that works well for the cases I'm dealing with. I'll update the patch set accordingly and re-post it. Best regards David On Fri, 6 Aug 2021 at 09:38, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > On Fri, Aug 06, 2021 at 09:15:09AM +0100, David Plowman wrote: > > Hi Sakari, Laurent > > > > Thanks for the various comments and discussion. It did prompt me to > > have some second thoughts about some of the details here, as follows. > > > > These controls are aimed specifically at sensors that do some kind of > > on-board "demosaic / remosaic" process, for instance to produce > > standard Bayer patterns from non-standard ones. As such the principal > > requirement is for the sensor to know what "grey" looks like, which we > > tell it by sending it the red and blue gains from the white balance > > algorithm. (This obviously enables it to reduce colour aliasing during > > the processing that it does.) > > > > So perhaps the comparison here should be with the existing > > V4L2_CID_RED/BLUE_BALANCE controls. I'm not sure that it really > > matters exactly what the colours of the pixels on the sensor really > > are, it's knowing what "grey" looks like that is important. Any new > > controls could be quite cumbersome to use if you have to figure out > > what the underlying pixel arrangement looks like, it certainly feels > > much easier to refer simply to red/blue pixels, leaving the driver to > > deal with its own internal idiosyncrasies. > > > > Having said that, the particular sensor I have exposes a gain for each > > of the four Bayer channels, even though I find myself ignoring the > > green ones!! > > > > Anyway, I certainly feel I could go either way on this one - do you > > have any more thoughts on the matter? > > With an array control, we would have to decide (and document) which > components are stored in the array, and in which order. For Bayer > sensors, that would be B, Gb, Gr and R. As for the order, there are > three options: > > - fixed order (e.g. always B, Gb, Gr, R) > - matching the sensor's CFA native order (always the same for a given > sensor, but varying between different sensors) > - matching the currently configured format (the bayer pattern can change > when moving the crop rectangle by one pixels or when mirroring the > sensor readout) > > The last two options seem quite impractical to me. The first option, if > we only consider Bayer sensors, is equivalent to the four controls your > propose in the sense that the semantics is defined in the control > specification and identical for all sensors, but with the advantage of > bundling all four values together. It will also allow expanding this to > other patterns if the need arise, which I think would be useful. > > If we were to redesign the red/blue gains, I'd go for a single array > control today, with one value per CFA component. > > > On Thu, 5 Aug 2021 at 19:05, Sakari Ailus wrote: > > > On Thu, Aug 05, 2021 at 06:49:33PM +0300, Laurent Pinchart wrote: > > > > On Thu, Aug 05, 2021 at 06:40:42PM +0300, Sakari Ailus wrote: > > > > > On Thu, Aug 05, 2021 at 06:22:32PM +0300, Laurent Pinchart wrote: > > > > > > On Thu, Jul 22, 2021 at 01:12:48PM +0100, David Plowman wrote: > > > > > > > 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 > > > > > > > > > > > > This will effectively limit the API to Bayer patterns. I wonder if we > > > > > > should instead implement it as a single array control, with one element > > > > > > per CFA component. > > > > > > > > > > There are other raw patterns, too. Supporting them would likely require one > > > > > or a few more controls. > > > > > > > > > > That said, as the values change often it's more efficient to use a single > > > > > control. But each colour combination (not each pattern) would require its > > > > > own control in this case, eventually requiring more controls. > > > > > > > > I'm not sure to follow you. My idea is to define a single control, with > > > > a number of elements that depends on the pattern being used, and the > > > > order specified in the native sensor pattern. I don't think each colour > > > > combination would then require its own control. > > > > > > Ah, I guess that would work, too. Then we'll need to define what kind of > > > pixel orders are supported for that single control (and for which formats). > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 2/2] media: v4l2-ctrls: Document V4L2_CID_NOTIFY_GAIN_XXX controls 2021-07-22 12:12 [PATCH v3 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX David Plowman 2021-07-22 12:12 ` [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls David Plowman @ 2021-07-22 12:12 ` David Plowman 2021-08-05 15:04 ` Hans Verkuil 1 sibling, 1 reply; 13+ messages in thread From: David Plowman @ 2021-07-22 12:12 UTC (permalink / raw) To: linux-media, sakari.ailus, Laurent Pinchart, Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil 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 | 25 +++++++++++++++++++ 1 file changed, 25 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..d50b56156a8e 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,28 @@ Image Source Control IDs * - __u32 - ``height`` - Height of the area. + +``V4L2_CID_NOTIFY_GAIN_RED (integer)`` + Please refer to ``V4L2_CID_NOTIFY_GAIN_GREENB`` below. + +``V4L2_CID_NOTIFY_GAIN_GREENR (integer)`` + Please refer to ``V4L2_CID_NOTIFY_GAIN_GREENB`` below. + +``V4L2_CID_NOTIFY_GAIN_BLUE (integer)`` + Please refer to ``V4L2_CID_NOTIFY_GAIN_GREENB`` below. + +``V4L2_CID_NOTIFY_GAIN_GREENB (integer)`` + Note: this description covers all four ``V4L2_CID_NOTIFY_GAIN`` + controls, according to the Bayer channel indicated at the end of + the control name. + + The sensor is notified what gain will be applied to this Bayer + channel 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 does not apply it to the output pixels + themselves. + + The units for the gain value are linear, with the default value + being shared between all four controls and representing a gain of + exactly 1. Other control properties, such as ranges, should also + be common. -- 2.30.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] media: v4l2-ctrls: Document V4L2_CID_NOTIFY_GAIN_XXX controls 2021-07-22 12:12 ` [PATCH v3 2/2] media: v4l2-ctrls: Document " David Plowman @ 2021-08-05 15:04 ` Hans Verkuil 0 siblings, 0 replies; 13+ messages in thread From: Hans Verkuil @ 2021-08-05 15:04 UTC (permalink / raw) To: David Plowman, linux-media, sakari.ailus, Laurent Pinchart, Kieran Bingham, Mauro Carvalho Chehab On 22/07/2021 14:12, 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> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans > --- > .../media/v4l/ext-ctrls-image-source.rst | 25 +++++++++++++++++++ > 1 file changed, 25 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..d50b56156a8e 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,28 @@ Image Source Control IDs > * - __u32 > - ``height`` > - Height of the area. > + > +``V4L2_CID_NOTIFY_GAIN_RED (integer)`` > + Please refer to ``V4L2_CID_NOTIFY_GAIN_GREENB`` below. > + > +``V4L2_CID_NOTIFY_GAIN_GREENR (integer)`` > + Please refer to ``V4L2_CID_NOTIFY_GAIN_GREENB`` below. > + > +``V4L2_CID_NOTIFY_GAIN_BLUE (integer)`` > + Please refer to ``V4L2_CID_NOTIFY_GAIN_GREENB`` below. > + > +``V4L2_CID_NOTIFY_GAIN_GREENB (integer)`` > + Note: this description covers all four ``V4L2_CID_NOTIFY_GAIN`` > + controls, according to the Bayer channel indicated at the end of > + the control name. > + > + The sensor is notified what gain will be applied to this Bayer > + channel 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 does not apply it to the output pixels > + themselves. > + > + The units for the gain value are linear, with the default value > + being shared between all four controls and representing a gain of > + exactly 1. Other control properties, such as ranges, should also > + be common. > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-08-06 10:35 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-22 12:12 [PATCH v3 0/2] New V4L2 controls V4L2_CID_NOTIFY_GAIN_XXX David Plowman 2021-07-22 12:12 ` [PATCH v3 1/2] media: v4l2-ctrls: Add V4L2_CID_NOTIFY_GAIN_XXX controls David Plowman 2021-08-05 15:04 ` Hans Verkuil 2021-08-05 15:22 ` Laurent Pinchart 2021-08-05 15:40 ` Sakari Ailus 2021-08-05 15:49 ` Laurent Pinchart 2021-08-05 18:05 ` Sakari Ailus 2021-08-06 8:15 ` David Plowman 2021-08-06 8:32 ` Sakari Ailus 2021-08-06 8:38 ` Laurent Pinchart 2021-08-06 10:34 ` David Plowman 2021-07-22 12:12 ` [PATCH v3 2/2] media: v4l2-ctrls: Document " David Plowman 2021-08-05 15:04 ` 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.