Re: [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt

[Sylwester Nawrocki <snjw23@gmail.com>]

On 12/15/2011 11:01 PM, Sakari Ailus wrote:
>>>  	  <entry>__u32</entry>
>>> -	  <entry><structfield>reserved</structfield>[7]</entry>
>>> +	  <entry><structfield>pixel_clock</structfield></entry>
>>> +	  <entry>Pixel clock in kHz. This clock is the maximum rate at
>>> +	  which pixels are transferred on the bus. The pixel_clock
>>> +	  field is read-only.</entry>
>>
>> I searched a couple of datasheets to find out where I could use this pixel_clock
>> field but didn't find any so far. I haven't tried too hard though ;)
>> There seems to be more benefits from having the link frequency control.
> 
> There are a few reasons to have the pixel clock available to the user space.
> 
> The previously existing reason is that the user may get information on the
> pixel rates, including cases where the pixel rate of a subdev isn't enough
> for the streaming to be possible. Earlier on it just failed. Such cases are
> common on the OMAP 3 ISP, for example.
> 
> The second reason is to provide that for timing calculations in the user
> space.

Fair enough. Perhaps, if I have worked more with image signal processing
algorithms in user space I would not ask about that in the first place :-)

> 
>> It might be easy to confuse pixel_clock with the bus clock. The bus clock is
>> often referred in datasheets as Pixel Clock (PCLK, AFAIU it's described with
>> link frequency in your RFC). IMHO your original proposal was better, i.e.
>> using more explicit pixel_rate. Also why it is in kHz ? Doesn't it make more
>> sense to use bits or pixels  per second ?
> 
> Oh, yes, now that you mention it I did call it pixel rate. I'm fine
> withrenaming it back to e.g. "pixelrate".

I'm fine with that too, sounds good!

> 
> I picked kHz since the 32-bit field would allow rates up to 4 GiP/s. Not
> sure if that's overkill though. Could be. But in practice it should give
> good enough precision this way, too.

All right, however I was more concerned by the "Hz" part, rather than "k" ;)
It might be good to have the relevant unit defined in the spec, to avoid
misinterpretation and future interoperability issues .

>>> +	</row>
>>> +	<row>
>>> +	  <entry>__u32</entry>
>>> +	  <entry><structfield>reserved</structfield>[6]</entry>
>>>  	  <entry>Reserved for future extensions. Applications and drivers must
>>>  	  set the array to zero.</entry>
>>>  	</row>
>>> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
>>> index 5ea7f75..76a0df2 100644
>>> --- a/include/linux/v4l2-mediabus.h
>>> +++ b/include/linux/v4l2-mediabus.h
>>> @@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
>>>   * @code:	data format code (from enum v4l2_mbus_pixelcode)
>>>   * @field:	used interlacing type (from enum v4l2_field)
>>>   * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
>>> + * @pixel_clock: pixel clock, in kHz
>>>   */
>>>  struct v4l2_mbus_framefmt {
>>>  	__u32			width;
>>> @@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
>>>  	__u32			code;
>>>  	__u32			field;
>>>  	__u32			colorspace;
>>> -	__u32			reserved[7];
>>> +	__u32			pixel_clock;
>>
>> I'm wondering, whether it is worth to make it 'pixelclock' for consistency
>> with other fields? Perhaps it would make more sense to have color_space and
>> pixel_clock.
> 
> "pixelrate" is fine for me.

Ack.

-- 
Regards,
Sylwester