diff mbox

[v3,1/3] v4l: Add test pattern colour component controls

Message ID 1401374448-30411-2-git-send-email-sakari.ailus@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sakari Ailus May 29, 2014, 2:40 p.m. UTC
In many cases the test pattern has selectable values for each colour
component. Implement controls for raw bayer components. Additional controls
should be defined for colour components that are not covered by these
controls.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/DocBook/media/v4l/controls.xml | 34 ++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c         |  4 ++++
 include/uapi/linux/v4l2-controls.h           |  4 ++++
 3 files changed, 42 insertions(+)

Comments

Laurent Pinchart May 29, 2014, 2:47 p.m. UTC | #1
Hi Sakari,

Thank you for the patch.

On Thursday 29 May 2014 17:40:46 Sakari Ailus wrote:
> In many cases the test pattern has selectable values for each colour
> component. Implement controls for raw bayer components. Additional controls
> should be defined for colour components that are not covered by these
> controls.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/DocBook/media/v4l/controls.xml | 34 +++++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c         |  4 ++++
>  include/uapi/linux/v4l2-controls.h           |  4 ++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> b/Documentation/DocBook/media/v4l/controls.xml index 47198ee..bf23994
> 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -4677,6 +4677,40 @@ interface and may change in the future.</para>
>  	    conversion.
>  	    </entry>
>  	  </row>
> +	  <row>
> +	    <entry
> spanname="id"><constant>V4L2_CID_TEST_PATTERN_RED</constant></entry> +	   
> <entry>integer</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Test pattern red colour component.
> +	    </entry>
> +	  </row>
> +	  <row>
> +	    <entry
> spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENR</constant></entry>
> +	    <entry>integer</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Test pattern green (next to red)
> +	    colour component.

What about non-Bayer RGB sensors ? Should they use the GREENR or the GREENB 
control for the green component ? Or a different control ?

I'm wondering whether we shouldn't have a single test pattern color control 
and create a color type using Hans' complex controls API.

> +	    </entry>
> +	  </row>
> +	  <row>
> +	    <entry
> spanname="id"><constant>V4L2_CID_TEST_PATTERN_BLUE</constant></entry> +	   
> <entry>integer</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Test pattern blue colour component.
> +	    </entry>
> +	  </row>
> +	  <row>
> +	    <entry
> spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENB</constant></entry> +	 
>   <entry>integer</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Test pattern green (next to blue)
> +	    colour component.
> +	    </entry>
> +	  </row>
>  	  <row><entry></entry></row>
>  	</tbody>
>        </tgroup>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c index 55c6832..a4104a7 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -839,6 +839,10 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_VBLANK:			return "Vertical Blanking";
>  	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
>  	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
> +	case V4L2_CID_TEST_PATTERN_RED:		return "Red Pixel Value";
> +	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";
> 
>  	/* Image processing controls */
>  	case V4L2_CID_IMAGE_PROC_CLASS:		return "Image Processing Controls";
> diff --git a/include/uapi/linux/v4l2-controls.h
> b/include/uapi/linux/v4l2-controls.h index 2ac5597..5c55a19 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -855,6 +855,10 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define V4L2_CID_VBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 1)
>  #define V4L2_CID_HBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
>  #define V4L2_CID_ANALOGUE_GAIN			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE 
+ 3)
> +#define V4L2_CID_TEST_PATTERN_RED		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 
4)
> +#define V4L2_CID_TEST_PATTERN_GREENR		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE 
+
> 5) +#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)
> 
> 
>  /* Image processing controls */
Sakari Ailus May 29, 2014, 2:58 p.m. UTC | #2
Hi Laurent,

Thanks for your comments.

Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Thursday 29 May 2014 17:40:46 Sakari Ailus wrote:
>> In many cases the test pattern has selectable values for each colour
>> component. Implement controls for raw bayer components. Additional controls
>> should be defined for colour components that are not covered by these
>> controls.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>   Documentation/DocBook/media/v4l/controls.xml | 34 +++++++++++++++++++++++++
>>   drivers/media/v4l2-core/v4l2-ctrls.c         |  4 ++++
>>   include/uapi/linux/v4l2-controls.h           |  4 ++++
>>   3 files changed, 42 insertions(+)
>>
>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
>> b/Documentation/DocBook/media/v4l/controls.xml index 47198ee..bf23994
>> 100644
>> --- a/Documentation/DocBook/media/v4l/controls.xml
>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>> @@ -4677,6 +4677,40 @@ interface and may change in the future.</para>
>>   	    conversion.
>>   	    </entry>
>>   	  </row>
>> +	  <row>
>> +	    <entry
>> spanname="id"><constant>V4L2_CID_TEST_PATTERN_RED</constant></entry> +	
>> <entry>integer</entry>
>> +	  </row>
>> +	  <row>
>> +	    <entry spanname="descr">Test pattern red colour component.
>> +	    </entry>
>> +	  </row>
>> +	  <row>
>> +	    <entry
>> spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENR</constant></entry>
>> +	    <entry>integer</entry>
>> +	  </row>
>> +	  <row>
>> +	    <entry spanname="descr">Test pattern green (next to red)
>> +	    colour component.
>
> What about non-Bayer RGB sensors ? Should they use the GREENR or the GREENB
> control for the green component ? Or a different control ?

A different one. It should be simply green. I could add it to the same 
patch if you wish.

> I'm wondering whether we shouldn't have a single test pattern color control
> and create a color type using Hans' complex controls API.

A raw bayer four-pixel value, you mean?
Laurent Pinchart May 29, 2014, 3:01 p.m. UTC | #3
On Thursday 29 May 2014 17:58:59 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Thursday 29 May 2014 17:40:46 Sakari Ailus wrote:
> >> In many cases the test pattern has selectable values for each colour
> >> component. Implement controls for raw bayer components. Additional
> >> controls
> >> should be defined for colour components that are not covered by these
> >> controls.
> >> 
> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >> 
> >>   Documentation/DocBook/media/v4l/controls.xml | 34 +++++++++++++++++++++
> >>   drivers/media/v4l2-core/v4l2-ctrls.c         |  4 ++++
> >>   include/uapi/linux/v4l2-controls.h           |  4 ++++
> >>   3 files changed, 42 insertions(+)
> >> 
> >> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> >> b/Documentation/DocBook/media/v4l/controls.xml index 47198ee..bf23994
> >> 100644
> >> --- a/Documentation/DocBook/media/v4l/controls.xml
> >> +++ b/Documentation/DocBook/media/v4l/controls.xml
> >> @@ -4677,6 +4677,40 @@ interface and may change in the future.</para>
> >>   	    conversion.
> >>   	    </entry>
> >>   	  </row>
> >> +	  <row>
> >> +	    <entry
> >> spanname="id"><constant>V4L2_CID_TEST_PATTERN_RED</constant></entry>
> >> +       <entry>integer</entry>
> >> +	  </row>
> >> +	  <row>
> >> +	    <entry spanname="descr">Test pattern red colour component.
> >> +	    </entry>
> >> +	  </row>
> >> +	  <row>
> >> +	    <entry
> >> spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENR</constant></entry>
> >> +	    <entry>integer</entry>
> >> +	  </row>
> >> +	  <row>
> >> +	    <entry spanname="descr">Test pattern green (next to red)
> >> +	    colour component.
> > 
> > What about non-Bayer RGB sensors ? Should they use the GREENR or the
> > GREENB control for the green component ? Or a different control ?
> 
> A different one. It should be simply green. I could add it to the same
> patch if you wish.
> 
> > I'm wondering whether we shouldn't have a single test pattern color
> > control and create a color type using Hans' complex controls API.
> 
> A raw bayer four-pixel value, you mean?

Yes. I'll let Hans comment on that.
Hans Verkuil May 30, 2014, 12:28 p.m. UTC | #4
On 05/29/2014 05:01 PM, Laurent Pinchart wrote:
> On Thursday 29 May 2014 17:58:59 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>>> On Thursday 29 May 2014 17:40:46 Sakari Ailus wrote:
>>>> In many cases the test pattern has selectable values for each colour
>>>> component. Implement controls for raw bayer components. Additional
>>>> controls
>>>> should be defined for colour components that are not covered by these
>>>> controls.
>>>>
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> ---
>>>>
>>>>   Documentation/DocBook/media/v4l/controls.xml | 34 +++++++++++++++++++++
>>>>   drivers/media/v4l2-core/v4l2-ctrls.c         |  4 ++++
>>>>   include/uapi/linux/v4l2-controls.h           |  4 ++++
>>>>   3 files changed, 42 insertions(+)
>>>>
>>>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
>>>> b/Documentation/DocBook/media/v4l/controls.xml index 47198ee..bf23994
>>>> 100644
>>>> --- a/Documentation/DocBook/media/v4l/controls.xml
>>>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>>>> @@ -4677,6 +4677,40 @@ interface and may change in the future.</para>
>>>>   	    conversion.
>>>>   	    </entry>
>>>>   	  </row>
>>>> +	  <row>
>>>> +	    <entry
>>>> spanname="id"><constant>V4L2_CID_TEST_PATTERN_RED</constant></entry>
>>>> +       <entry>integer</entry>
>>>> +	  </row>
>>>> +	  <row>
>>>> +	    <entry spanname="descr">Test pattern red colour component.
>>>> +	    </entry>
>>>> +	  </row>
>>>> +	  <row>
>>>> +	    <entry
>>>> spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENR</constant></entry>
>>>> +	    <entry>integer</entry>
>>>> +	  </row>
>>>> +	  <row>
>>>> +	    <entry spanname="descr">Test pattern green (next to red)
>>>> +	    colour component.
>>>
>>> What about non-Bayer RGB sensors ? Should they use the GREENR or the
>>> GREENB control for the green component ? Or a different control ?
>>
>> A different one. It should be simply green. I could add it to the same
>> patch if you wish.
>>
>>> I'm wondering whether we shouldn't have a single test pattern color
>>> control and create a color type using Hans' complex controls API.
>>
>> A raw bayer four-pixel value, you mean?
> 
> Yes. I'll let Hans comment on that.
> 

Why would you need the complex control API for that? It would fit in a s32,
and certainly in a s64.

We have done something similar to this in the past (V4L2_CID_BG_COLOR).

The main problem is that the interpretation of the s32 value has to be
clearly defined. And if different sensors might have different min/max values
for each component, then it becomes messy to use a single control. My feeling
is that it is better to go with separate controls, one for each component.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart June 3, 2014, 12:21 p.m. UTC | #5
Hi Hans,

On Friday 30 May 2014 14:28:16 Hans Verkuil wrote:
> On 05/29/2014 05:01 PM, Laurent Pinchart wrote:
> > On Thursday 29 May 2014 17:58:59 Sakari Ailus wrote:
> >> Laurent Pinchart wrote:
> >>> On Thursday 29 May 2014 17:40:46 Sakari Ailus wrote:
> >>>> In many cases the test pattern has selectable values for each colour
> >>>> component. Implement controls for raw bayer components. Additional
> >>>> controls should be defined for colour components that are not covered
> >>>> by these controls.
> >>>> 
> >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>> ---
> >>>> 
> >>>>  Documentation/DocBook/media/v4l/controls.xml | 34 ++++++++++++++++++++
> >>>>  drivers/media/v4l2-core/v4l2-ctrls.c         |  4 ++++
> >>>>  include/uapi/linux/v4l2-controls.h           |  4 ++++
> >>>>  3 files changed, 42 insertions(+)
> >>>> 
> >>>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> >>>> b/Documentation/DocBook/media/v4l/controls.xml index 47198ee..bf23994
> >>>> 100644
> >>>> --- a/Documentation/DocBook/media/v4l/controls.xml
> >>>> +++ b/Documentation/DocBook/media/v4l/controls.xml
> >>>> @@ -4677,6 +4677,40 @@ interface and may change in the future.</para>
> >>>>   	    conversion.
> >>>>   	    </entry>
> >>>>   	  </row>
> >>>> +	  <row>
> >>>> +	    <entry
> >>>> spanname="id"><constant>V4L2_CID_TEST_PATTERN_RED</constant></entry>
> >>>> +       <entry>integer</entry>
> >>>> +	  </row>
> >>>> +	  <row>
> >>>> +	    <entry spanname="descr">Test pattern red colour component.
> >>>> +	    </entry>
> >>>> +	  </row>
> >>>> +	  <row>
> >>>> +	    <entry
> >>>> spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENR</constant></entry>
> >>>> +	    <entry>integer</entry>
> >>>> +	  </row>
> >>>> +	  <row>
> >>>> +	    <entry spanname="descr">Test pattern green (next to red)
> >>>> +	    colour component.
> >>> 
> >>> What about non-Bayer RGB sensors ? Should they use the GREENR or the
> >>> GREENB control for the green component ? Or a different control ?
> >> 
> >> A different one. It should be simply green. I could add it to the same
> >> patch if you wish.
> >> 
> >>> I'm wondering whether we shouldn't have a single test pattern color
> >>> control and create a color type using Hans' complex controls API.
> >> 
> >> A raw bayer four-pixel value, you mean?
> > 
> > Yes. I'll let Hans comment on that.
> 
> Why would you need the complex control API for that? It would fit in a s32,
> and certainly in a s64.

It wouldn't fit in a s32 when using more than 8 bits per component. s64 would 
be an option, until we reach 16 bits per component (or more than 4 
components).

> We have done something similar to this in the past (V4L2_CID_BG_COLOR).
> 
> The main problem is that the interpretation of the s32 value has to be
> clearly defined. And if different sensors might have different min/max
> values for each component, then it becomes messy to use a single control.

The interpretation would depend on both the sensor and the color format.

> My feeling is that it is better to go with separate controls, one for each
> component.

What bothers me is that we'll need to add lots of controls, for each 
component. There's 4 controls for Bayer, one additional green control for RGB, 
3 controls for YUV, ... That's already 8 controls to support the common 
Bayer/RGB/YUV formats. As colors can be used for different purposes (test 
pattern with possibly more than one color, background color, ...) that would 
increase the complexity beyond my comfort zone.
Sakari Ailus June 3, 2014, 12:27 p.m. UTC | #6
Hi Laurent,

Laurent Pinchart wrote:
> Hi Hans,
>
> On Friday 30 May 2014 14:28:16 Hans Verkuil wrote:
>> On 05/29/2014 05:01 PM, Laurent Pinchart wrote:
>>> On Thursday 29 May 2014 17:58:59 Sakari Ailus wrote:
>>>> Laurent Pinchart wrote:
>>>>> On Thursday 29 May 2014 17:40:46 Sakari Ailus wrote:
>>>>>> In many cases the test pattern has selectable values for each colour
>>>>>> component. Implement controls for raw bayer components. Additional
>>>>>> controls should be defined for colour components that are not covered
>>>>>> by these controls.
>>>>>>
>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>> ---
>>>>>>
>>>>>>   Documentation/DocBook/media/v4l/controls.xml | 34 ++++++++++++++++++++
>>>>>>   drivers/media/v4l2-core/v4l2-ctrls.c         |  4 ++++
>>>>>>   include/uapi/linux/v4l2-controls.h           |  4 ++++
>>>>>>   3 files changed, 42 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
>>>>>> b/Documentation/DocBook/media/v4l/controls.xml index 47198ee..bf23994
>>>>>> 100644
>>>>>> --- a/Documentation/DocBook/media/v4l/controls.xml
>>>>>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>>>>>> @@ -4677,6 +4677,40 @@ interface and may change in the future.</para>
>>>>>>    	    conversion.
>>>>>>    	    </entry>
>>>>>>    	  </row>
>>>>>> +	  <row>
>>>>>> +	    <entry
>>>>>> spanname="id"><constant>V4L2_CID_TEST_PATTERN_RED</constant></entry>
>>>>>> +       <entry>integer</entry>
>>>>>> +	  </row>
>>>>>> +	  <row>
>>>>>> +	    <entry spanname="descr">Test pattern red colour component.
>>>>>> +	    </entry>
>>>>>> +	  </row>
>>>>>> +	  <row>
>>>>>> +	    <entry
>>>>>> spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENR</constant></entry>
>>>>>> +	    <entry>integer</entry>
>>>>>> +	  </row>
>>>>>> +	  <row>
>>>>>> +	    <entry spanname="descr">Test pattern green (next to red)
>>>>>> +	    colour component.
>>>>>
>>>>> What about non-Bayer RGB sensors ? Should they use the GREENR or the
>>>>> GREENB control for the green component ? Or a different control ?
>>>>
>>>> A different one. It should be simply green. I could add it to the same
>>>> patch if you wish.
>>>>
>>>>> I'm wondering whether we shouldn't have a single test pattern color
>>>>> control and create a color type using Hans' complex controls API.
>>>>
>>>> A raw bayer four-pixel value, you mean?
>>>
>>> Yes. I'll let Hans comment on that.
>>
>> Why would you need the complex control API for that? It would fit in a s32,
>> and certainly in a s64.
>
> It wouldn't fit in a s32 when using more than 8 bits per component. s64 would
> be an option, until we reach 16 bits per component (or more than 4
> components).
>
>> We have done something similar to this in the past (V4L2_CID_BG_COLOR).
>>
>> The main problem is that the interpretation of the s32 value has to be
>> clearly defined. And if different sensors might have different min/max
>> values for each component, then it becomes messy to use a single control.
>
> The interpretation would depend on both the sensor and the color format.
>
>> My feeling is that it is better to go with separate controls, one for each
>> component.
>
> What bothers me is that we'll need to add lots of controls, for each
> component. There's 4 controls for Bayer, one additional green control for RGB,
> 3 controls for YUV, ... That's already 8 controls to support the common
> Bayer/RGB/YUV formats. As colors can be used for different purposes (test

Three if you make colour controls, and three more control types.

Something to consider as well is that these controls are commonly used 
by test programs and you'd need to implement support for each new colour 
specific control type in the test programs such as yavta.

I'm not sure if implementing three new control types for that purpose 
only makes sense. Do you expect to see much use for such control types 
outside the test patterns?

> pattern with possibly more than one color, background color, ...) that would
> increase the complexity beyond my comfort zone.

I think that where they exist, such, likely device specific, controls 
wouldn't make it to the list of standard controls anyway. The test 
patterns typically are quite simple after all.
Hans Verkuil June 3, 2014, 12:42 p.m. UTC | #7
On 06/03/14 14:21, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 30 May 2014 14:28:16 Hans Verkuil wrote:
>> On 05/29/2014 05:01 PM, Laurent Pinchart wrote:
>>> On Thursday 29 May 2014 17:58:59 Sakari Ailus wrote:
>>>> Laurent Pinchart wrote:
>>>>> On Thursday 29 May 2014 17:40:46 Sakari Ailus wrote:
>>>>>> In many cases the test pattern has selectable values for each colour
>>>>>> component. Implement controls for raw bayer components. Additional
>>>>>> controls should be defined for colour components that are not covered
>>>>>> by these controls.
>>>>>>
>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>> ---
>>>>>>
>>>>>>  Documentation/DocBook/media/v4l/controls.xml | 34 ++++++++++++++++++++
>>>>>>  drivers/media/v4l2-core/v4l2-ctrls.c         |  4 ++++
>>>>>>  include/uapi/linux/v4l2-controls.h           |  4 ++++
>>>>>>  3 files changed, 42 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
>>>>>> b/Documentation/DocBook/media/v4l/controls.xml index 47198ee..bf23994
>>>>>> 100644
>>>>>> --- a/Documentation/DocBook/media/v4l/controls.xml
>>>>>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>>>>>> @@ -4677,6 +4677,40 @@ interface and may change in the future.</para>
>>>>>>   	    conversion.
>>>>>>   	    </entry>
>>>>>>   	  </row>
>>>>>> +	  <row>
>>>>>> +	    <entry
>>>>>> spanname="id"><constant>V4L2_CID_TEST_PATTERN_RED</constant></entry>
>>>>>> +       <entry>integer</entry>
>>>>>> +	  </row>
>>>>>> +	  <row>
>>>>>> +	    <entry spanname="descr">Test pattern red colour component.
>>>>>> +	    </entry>
>>>>>> +	  </row>
>>>>>> +	  <row>
>>>>>> +	    <entry
>>>>>> spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENR</constant></entry>
>>>>>> +	    <entry>integer</entry>
>>>>>> +	  </row>
>>>>>> +	  <row>
>>>>>> +	    <entry spanname="descr">Test pattern green (next to red)
>>>>>> +	    colour component.
>>>>>
>>>>> What about non-Bayer RGB sensors ? Should they use the GREENR or the
>>>>> GREENB control for the green component ? Or a different control ?
>>>>
>>>> A different one. It should be simply green. I could add it to the same
>>>> patch if you wish.
>>>>
>>>>> I'm wondering whether we shouldn't have a single test pattern color
>>>>> control and create a color type using Hans' complex controls API.
>>>>
>>>> A raw bayer four-pixel value, you mean?
>>>
>>> Yes. I'll let Hans comment on that.
>>
>> Why would you need the complex control API for that? It would fit in a s32,
>> and certainly in a s64.
> 
> It wouldn't fit in a s32 when using more than 8 bits per component. s64 would 
> be an option, until we reach 16 bits per component (or more than 4 
> components).
> 
>> We have done something similar to this in the past (V4L2_CID_BG_COLOR).
>>
>> The main problem is that the interpretation of the s32 value has to be
>> clearly defined. And if different sensors might have different min/max
>> values for each component, then it becomes messy to use a single control.
> 
> The interpretation would depend on both the sensor and the color format.
> 
>> My feeling is that it is better to go with separate controls, one for each
>> component.
> 
> What bothers me is that we'll need to add lots of controls, for each 
> component. There's 4 controls for Bayer, one additional green control for RGB, 
> 3 controls for YUV, ... That's already 8 controls to support the common 
> Bayer/RGB/YUV formats. As colors can be used for different purposes (test 
> pattern with possibly more than one color, background color, ...) that would 
> increase the complexity beyond my comfort zone.

Why would this be any better with compound type controls? In fact, I think
it is worse: simple controls will show up in qv4l2 and v4l2-ctl and are easy
to set from the GUI/command line. To set compound controls you will need to
write custom code for them. You also only see the controls that the driver
actually needs. So while there may be X component controls, a driver for a
Bayer sensor will only use four of them.

If you want to support a driver that has highly specialized test pattern
support, then I'm leaning towards a custom ioctl for that driver.

Regards,

	Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil June 10, 2014, 12:15 p.m. UTC | #8
On 05/29/14 16:40, Sakari Ailus wrote:
> In many cases the test pattern has selectable values for each colour
> component. Implement controls for raw bayer components. Additional controls
> should be defined for colour components that are not covered by these
> controls.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

	Hans

> ---
>  Documentation/DocBook/media/v4l/controls.xml | 34 ++++++++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c         |  4 ++++
>  include/uapi/linux/v4l2-controls.h           |  4 ++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> index 47198ee..bf23994 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -4677,6 +4677,40 @@ interface and may change in the future.</para>
>  	    conversion.
>  	    </entry>
>  	  </row>
> +	  <row>
> +	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN_RED</constant></entry>
> +	    <entry>integer</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Test pattern red colour component.
> +	    </entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENR</constant></entry>
> +	    <entry>integer</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Test pattern green (next to red)
> +	    colour component.
> +	    </entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN_BLUE</constant></entry>
> +	    <entry>integer</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Test pattern blue colour component.
> +	    </entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENB</constant></entry>
> +	    <entry>integer</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Test pattern green (next to blue)
> +	    colour component.
> +	    </entry>
> +	  </row>
>  	  <row><entry></entry></row>
>  	</tbody>
>        </tgroup>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 55c6832..a4104a7 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -839,6 +839,10 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_VBLANK:			return "Vertical Blanking";
>  	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
>  	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
> +	case V4L2_CID_TEST_PATTERN_RED:		return "Red Pixel Value";
> +	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";
>  
>  	/* Image processing controls */
>  	case V4L2_CID_IMAGE_PROC_CLASS:		return "Image Processing Controls";
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 2ac5597..5c55a19 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -855,6 +855,10 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define V4L2_CID_VBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 1)
>  #define V4L2_CID_HBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
>  #define V4L2_CID_ANALOGUE_GAIN			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3)
> +#define V4L2_CID_TEST_PATTERN_RED		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4)
> +#define V4L2_CID_TEST_PATTERN_GREENR		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
> +#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)
>  
>  
>  /* Image processing controls */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index 47198ee..bf23994 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -4677,6 +4677,40 @@  interface and may change in the future.</para>
 	    conversion.
 	    </entry>
 	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN_RED</constant></entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row>
+	    <entry spanname="descr">Test pattern red colour component.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENR</constant></entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row>
+	    <entry spanname="descr">Test pattern green (next to red)
+	    colour component.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN_BLUE</constant></entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row>
+	    <entry spanname="descr">Test pattern blue colour component.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN_GREENB</constant></entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row>
+	    <entry spanname="descr">Test pattern green (next to blue)
+	    colour component.
+	    </entry>
+	  </row>
 	  <row><entry></entry></row>
 	</tbody>
       </tgroup>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 55c6832..a4104a7 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -839,6 +839,10 @@  const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_VBLANK:			return "Vertical Blanking";
 	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
 	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
+	case V4L2_CID_TEST_PATTERN_RED:		return "Red Pixel Value";
+	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";
 
 	/* Image processing controls */
 	case V4L2_CID_IMAGE_PROC_CLASS:		return "Image Processing Controls";
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 2ac5597..5c55a19 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -855,6 +855,10 @@  enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_VBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 1)
 #define V4L2_CID_HBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
 #define V4L2_CID_ANALOGUE_GAIN			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3)
+#define V4L2_CID_TEST_PATTERN_RED		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4)
+#define V4L2_CID_TEST_PATTERN_GREENR		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
+#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)
 
 
 /* Image processing controls */