From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx1.redhat.com ([209.132.183.28]:9828 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751537Ab2KRJYh (ORCPT ); Sun, 18 Nov 2012 04:24:37 -0500 Message-ID: <50A8A939.7040505@redhat.com> Date: Sun, 18 Nov 2012 07:24:09 -0200 From: Mauro Carvalho Chehab MIME-Version: 1.0 To: Kirill Smelkov CC: Hans Verkuil , Mauro Carvalho Chehab , kirr@mns.spb.ru, linux-media@vger.kernel.org Subject: Re: [PATCH v4] [media] vivi: Teach it to tune FPS References: <1350914084-31618-1-git-send-email-kirr@mns.spb.ru> <201211021542.21944.hverkuil@xs4all.nl> <20121107113001.GA3097@tugrik.mns.mnsspb.ru> <201211161438.09046.hverkuil@xs4all.nl> <50A65FF2.8020801@redhat.com> <20121117104509.GA10789@mini.zxlink> In-Reply-To: <20121117104509.GA10789@mini.zxlink> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: 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 */ > tpf_default = {.numerator = 1001, .denominator = 30000}; /* NTSC */ > > #define dprintk(dev, level, fmt, arg...) \ seems OK to me. Regards, Mauro >