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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
