Re: [PATCH v4] [media] vivi: Teach it to tune FPS

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 18-11-2012 07:24, Mauro Carvalho Chehab escreveu:
> Em 17-11-2012 08:45, Kirill Smelkov escreveu:
>> On Fri, Nov 16, 2012 at 01:46:58PM -0200, Mauro Carvalho Chehab wrote:
>>> Em 16-11-2012 11:38, Hans Verkuil escreveu:
>>>> On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
>> [...]
>>
>>>>> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
>>>>> index 37d0af8..5d1b374 100644
>>>>> --- a/drivers/media/platform/vivi.c
>>>>> +++ b/drivers/media/platform/vivi.c
>>>>> @@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>>>>>   /* Global font descriptor */
>>>>>   static const u8 *font8x16;
>>>>>
>>>>> -/* default to NTSC timeperframe */
>>>>> -static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
>>>>> +/* timeperframe: min/max and default */
>>>>> +static const struct v4l2_fract
>>>>> +    tpf_min     = {.numerator = 1,        .denominator = UINT_MAX},  /* 1/infty */
>>>>> +    tpf_max     = {.numerator = UINT_MAX,    .denominator = 1},         /* infty */
>>>>
>>>> I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
>>>> 1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
>>>> might hit application errors when they manipulate these values. The shortest time
>>>> between frames is 1 ms anyway.
>>>>
>>>> It's the only comment I have, it looks good otherwise.
>>>
>>> As those will be a arbitrary values, I suggest to declare a macro for it at the
>>> beginning of vivi.c file, with some comment explaining the rationale of the choose,
>>> and what else needs to be changed, if this changes (e. g. less than 1ms would require
>>> changing the image generation logic, to avoid producing frames with equal content).
>>
>> Maybe something like this? (please note, I'm not a good text writer. If
>> this needs adjustment please help me shape the text up)
>>
>>
>> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
>> index 5d1b374..45b8a81 100644
>> --- a/drivers/media/platform/vivi.c
>> +++ b/drivers/media/platform/vivi.c
>> @@ -36,6 +36,18 @@
>>
>>   #define VIVI_MODULE_NAME "vivi"
>>
>> +/* Maximum allowed frame rate
>> + *
>> + * Vivi will allow setting timeperframe in [1/FPS_MAX - FPS_MAX/1] range.
>> + *
>> + * Ideally FPS_MAX should be infinity, i.e. practically UINT_MAX, but that
>> + * might hit application errors when they manipulate these values.
>> + *
>> + * Besides, for tpf < 1ms image-generation logic should be changed, to avoid
>> + * producing frames with equal content.
>> + */
>> +#define FPS_MAX 1000
>> +
>>   #define MAX_WIDTH 1920
>>   #define MAX_HEIGHT 1200
>>
>> @@ -67,8 +79,8 @@ static const u8 *font8x16;
>>
>>   /* timeperframe: min/max and default */
>>   static const struct v4l2_fract
>> -    tpf_min     = {.numerator = 1,        .denominator = UINT_MAX},  /* 1/infty */
>> -    tpf_max     = {.numerator = UINT_MAX,    .denominator = 1},         /* infty */
>> +    tpf_min     = {.numerator = 1,        .denominator = FPS_MAX},   /* ~1/infty */
>> +    tpf_max     = {.numerator = FPS_MAX,    .denominator = 1},         /* ~infty */

Was too fast answering it... The comments there should also be dropped, as it doesn't
range anymore to infty.

>>       tpf_default = {.numerator = 1001,    .denominator = 30000};     /* NTSC */
>>
>>   #define dprintk(dev, level, fmt, arg...) \
>
> seems OK to me.
>
> Regards,
> Mauro
>
>>
>