| Message ID | 1464725733-22119-1-git-send-email-floe@butterbrot.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/31/2016 10:15 PM, Florian Echtler wrote: > The device hardware is always running at 60 FPS, so report this both via > PARM_IOCTL and ENUM_FRAMEINTERVALS. > > Signed-off-by: Martin Kaltenbrunner <modin@yuri.at> > Signed-off-by: Florian Echtler <floe@butterbrot.org> > --- > drivers/input/touchscreen/sur40.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c > index 880c40b..4b1f703 100644 > --- a/drivers/input/touchscreen/sur40.c > +++ b/drivers/input/touchscreen/sur40.c > @@ -788,6 +788,19 @@ static int sur40_vidioc_fmt(struct file *file, void *priv, > return 0; > } > > +static int sur40_ioctl_parm(struct file *file, void *priv, > + struct v4l2_streamparm *p) > +{ > + if (p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > + return -EINVAL; > + > + p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME; > + p->parm.capture.timeperframe.numerator = 1; > + p->parm.capture.timeperframe.denominator = 60; > + p->parm.capture.readbuffers = 3; > + return 0; > +} > + > static int sur40_vidioc_enum_fmt(struct file *file, void *priv, > struct v4l2_fmtdesc *f) > { > @@ -814,13 +827,13 @@ static int sur40_vidioc_enum_framesizes(struct file *file, void *priv, > static int sur40_vidioc_enum_frameintervals(struct file *file, void *priv, > struct v4l2_frmivalenum *f) > { > - if ((f->index > 1) || (f->pixel_format != V4L2_PIX_FMT_GREY) > + if ((f->index > 0) || (f->pixel_format != V4L2_PIX_FMT_GREY) > || (f->width != sur40_video_format.width) > || (f->height != sur40_video_format.height)) > return -EINVAL; > > f->type = V4L2_FRMIVAL_TYPE_DISCRETE; > - f->discrete.denominator = 60/(f->index+1); > + f->discrete.denominator = 60; > f->discrete.numerator = 1; > return 0; > } > @@ -880,6 +893,9 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = { > .vidioc_enum_framesizes = sur40_vidioc_enum_framesizes, > .vidioc_enum_frameintervals = sur40_vidioc_enum_frameintervals, > > + .vidioc_g_parm = sur40_ioctl_parm, > + .vidioc_s_parm = sur40_ioctl_parm, Why is s_parm added when you can't change the framerate? Same questions for the enum_frameintervals function: it doesn't hurt to have it, but if there is only one unchangeable framerate, then it doesn't make much sense. Sorry, missed this when I reviewed this the first time around. Regards, Hans > + > .vidioc_enum_input = sur40_vidioc_enum_input, > .vidioc_g_input = sur40_vidioc_g_input, > .vidioc_s_input = sur40_vidioc_s_input, > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Hans, On 05.07.2016 08:41, Hans Verkuil wrote: > On 05/31/2016 10:15 PM, Florian Echtler wrote: >> The device hardware is always running at 60 FPS, so report this both via >> PARM_IOCTL and ENUM_FRAMEINTERVALS. >> >> Signed-off-by: Martin Kaltenbrunner <modin@yuri.at> >> Signed-off-by: Florian Echtler <floe@butterbrot.org> >> --- >> drivers/input/touchscreen/sur40.c | 20 ++++++++++++++++++-- >> 1 file changed, 18 insertions(+), 2 deletions(-) >> >> @@ -880,6 +893,9 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = { >> .vidioc_enum_framesizes = sur40_vidioc_enum_framesizes, >> .vidioc_enum_frameintervals = sur40_vidioc_enum_frameintervals, >> >> + .vidioc_g_parm = sur40_ioctl_parm, >> + .vidioc_s_parm = sur40_ioctl_parm, > > Why is s_parm added when you can't change the framerate? Oh, I thought it's mandatory to always have s_parm if you have g_parm (even if it always returns the same values). > Same questions for the > enum_frameintervals function: it doesn't hurt to have it, but if there is only > one unchangeable framerate, then it doesn't make much sense. If you don't have enum_frameintervals, how would you find out about the framerate otherwise? Is g_parm itself enough already for all userspace tools? > Sorry, missed this when I reviewed this the first time around. No problem. Best, Florian
On 07/05/2016 08:56 AM, Florian Echtler wrote: > Hello Hans, > > On 05.07.2016 08:41, Hans Verkuil wrote: >> On 05/31/2016 10:15 PM, Florian Echtler wrote: >>> The device hardware is always running at 60 FPS, so report this both via >>> PARM_IOCTL and ENUM_FRAMEINTERVALS. >>> >>> Signed-off-by: Martin Kaltenbrunner <modin@yuri.at> >>> Signed-off-by: Florian Echtler <floe@butterbrot.org> >>> --- >>> drivers/input/touchscreen/sur40.c | 20 ++++++++++++++++++-- >>> 1 file changed, 18 insertions(+), 2 deletions(-) >>> >>> @@ -880,6 +893,9 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = { >>> .vidioc_enum_framesizes = sur40_vidioc_enum_framesizes, >>> .vidioc_enum_frameintervals = sur40_vidioc_enum_frameintervals, >>> >>> + .vidioc_g_parm = sur40_ioctl_parm, >>> + .vidioc_s_parm = sur40_ioctl_parm, >> >> Why is s_parm added when you can't change the framerate? > > Oh, I thought it's mandatory to always have s_parm if you have g_parm > (even if it always returns the same values). > >> Same questions for the >> enum_frameintervals function: it doesn't hurt to have it, but if there is only >> one unchangeable framerate, then it doesn't make much sense. > > If you don't have enum_frameintervals, how would you find out about the > framerate otherwise? Is g_parm itself enough already for all userspace > tools? It should be. The enum_frameintervals function is much newer than g_parm. Frankly, I have the same problem with enum_framesizes: it reports only one size. These two enum ioctls are normally only implemented if there are at least two choices. If there is no choice, then G_FMT will return the size and G_PARM the framerate and there is no need to enumerate anything. The problem is that the spec is ambiguous as to the requirements if there is only one choice for size and interval. Are the enum ioctls allowed in that case? Personally I think there is nothing against that. But should S_PARM also be allowed even though it can't actually change the frameperiod? Don't bother making changes yet, let me think about this for a bit. Regards, Hans > >> Sorry, missed this when I reviewed this the first time around. > > No problem. > > Best, Florian > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/05/16 09:06, Hans Verkuil wrote: > On 07/05/2016 08:56 AM, Florian Echtler wrote: >> Hello Hans, >> >> On 05.07.2016 08:41, Hans Verkuil wrote: >>> On 05/31/2016 10:15 PM, Florian Echtler wrote: >>>> The device hardware is always running at 60 FPS, so report this both via >>>> PARM_IOCTL and ENUM_FRAMEINTERVALS. >>>> >>>> Signed-off-by: Martin Kaltenbrunner <modin@yuri.at> >>>> Signed-off-by: Florian Echtler <floe@butterbrot.org> >>>> --- >>>> drivers/input/touchscreen/sur40.c | 20 ++++++++++++++++++-- >>>> 1 file changed, 18 insertions(+), 2 deletions(-) >>>> >>>> @@ -880,6 +893,9 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = { >>>> .vidioc_enum_framesizes = sur40_vidioc_enum_framesizes, >>>> .vidioc_enum_frameintervals = sur40_vidioc_enum_frameintervals, >>>> >>>> + .vidioc_g_parm = sur40_ioctl_parm, >>>> + .vidioc_s_parm = sur40_ioctl_parm, >>> >>> Why is s_parm added when you can't change the framerate? >> >> Oh, I thought it's mandatory to always have s_parm if you have g_parm >> (even if it always returns the same values). >> >>> Same questions for the >>> enum_frameintervals function: it doesn't hurt to have it, but if there is only >>> one unchangeable framerate, then it doesn't make much sense. >> >> If you don't have enum_frameintervals, how would you find out about the >> framerate otherwise? Is g_parm itself enough already for all userspace >> tools? > > It should be. The enum_frameintervals function is much newer than g_parm. > > Frankly, I have the same problem with enum_framesizes: it reports only one > size. These two enum ioctls are normally only implemented if there are at > least two choices. If there is no choice, then G_FMT will return the size > and G_PARM the framerate and there is no need to enumerate anything. > > The problem is that the spec is ambiguous as to the requirements if there > is only one choice for size and interval. Are the enum ioctls allowed in > that case? Personally I think there is nothing against that. But should > S_PARM also be allowed even though it can't actually change the frameperiod? > > Don't bother making changes yet, let me think about this for a bit. OK, I came to the conclusion that if enum_frameintervals returns one or more intervals, then s_parm should be present, even if there is only one possible interval. I have updated the compliance utility to check for this. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06.07.2016 10:40, Hans Verkuil wrote: > On 07/05/16 09:06, Hans Verkuil wrote: >> On 07/05/2016 08:56 AM, Florian Echtler wrote: >>> On 05.07.2016 08:41, Hans Verkuil wrote: >>>> >>>> Why is s_parm added when you can't change the framerate? >>> >>> Oh, I thought it's mandatory to always have s_parm if you have g_parm >>> (even if it always returns the same values). >>> >>>> Same questions for the >>>> enum_frameintervals function: it doesn't hurt to have it, but if there is only >>>> one unchangeable framerate, then it doesn't make much sense. >>> >>> If you don't have enum_frameintervals, how would you find out about the >>> framerate otherwise? Is g_parm itself enough already for all userspace >>> tools? >> >> It should be. The enum_frameintervals function is much newer than g_parm. >> >> Frankly, I have the same problem with enum_framesizes: it reports only one >> size. These two enum ioctls are normally only implemented if there are at >> least two choices. If there is no choice, then G_FMT will return the size >> and G_PARM the framerate and there is no need to enumerate anything. >> >> The problem is that the spec is ambiguous as to the requirements if there >> is only one choice for size and interval. Are the enum ioctls allowed in >> that case? Personally I think there is nothing against that. But should >> S_PARM also be allowed even though it can't actually change the frameperiod? >> >> Don't bother making changes yet, let me think about this for a bit. > > OK, I came to the conclusion that if enum_frameintervals returns one or > more intervals, then s_parm should be present, even if there is only one > possible interval. > > I have updated the compliance utility to check for this. AFAICT, the original patch does meet the requirements, then? Or do you have any change requests? Best, Florian
On 07/06/2016 10:51 PM, Florian Echtler wrote: > On 06.07.2016 10:40, Hans Verkuil wrote: >> On 07/05/16 09:06, Hans Verkuil wrote: >>> On 07/05/2016 08:56 AM, Florian Echtler wrote: >>>> On 05.07.2016 08:41, Hans Verkuil wrote: >>>>> >>>>> Why is s_parm added when you can't change the framerate? >>>> >>>> Oh, I thought it's mandatory to always have s_parm if you have g_parm >>>> (even if it always returns the same values). >>>> >>>>> Same questions for the >>>>> enum_frameintervals function: it doesn't hurt to have it, but if there is only >>>>> one unchangeable framerate, then it doesn't make much sense. >>>> >>>> If you don't have enum_frameintervals, how would you find out about the >>>> framerate otherwise? Is g_parm itself enough already for all userspace >>>> tools? >>> >>> It should be. The enum_frameintervals function is much newer than g_parm. >>> >>> Frankly, I have the same problem with enum_framesizes: it reports only one >>> size. These two enum ioctls are normally only implemented if there are at >>> least two choices. If there is no choice, then G_FMT will return the size >>> and G_PARM the framerate and there is no need to enumerate anything. >>> >>> The problem is that the spec is ambiguous as to the requirements if there >>> is only one choice for size and interval. Are the enum ioctls allowed in >>> that case? Personally I think there is nothing against that. But should >>> S_PARM also be allowed even though it can't actually change the frameperiod? >>> >>> Don't bother making changes yet, let me think about this for a bit. >> >> OK, I came to the conclusion that if enum_frameintervals returns one or >> more intervals, then s_parm should be present, even if there is only one >> possible interval. >> >> I have updated the compliance utility to check for this. > > AFAICT, the original patch does meet the requirements, then? Or do you > have any change requests? Can you run the latest v4l2-compliance test? If that passes, then I'll take this patch as-is. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c index 880c40b..4b1f703 100644 --- a/drivers/input/touchscreen/sur40.c +++ b/drivers/input/touchscreen/sur40.c @@ -788,6 +788,19 @@ static int sur40_vidioc_fmt(struct file *file, void *priv, return 0; } +static int sur40_ioctl_parm(struct file *file, void *priv, + struct v4l2_streamparm *p) +{ + if (p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) + return -EINVAL; + + p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME; + p->parm.capture.timeperframe.numerator = 1; + p->parm.capture.timeperframe.denominator = 60; + p->parm.capture.readbuffers = 3; + return 0; +} + static int sur40_vidioc_enum_fmt(struct file *file, void *priv, struct v4l2_fmtdesc *f) { @@ -814,13 +827,13 @@ static int sur40_vidioc_enum_framesizes(struct file *file, void *priv, static int sur40_vidioc_enum_frameintervals(struct file *file, void *priv, struct v4l2_frmivalenum *f) { - if ((f->index > 1) || (f->pixel_format != V4L2_PIX_FMT_GREY) + if ((f->index > 0) || (f->pixel_format != V4L2_PIX_FMT_GREY) || (f->width != sur40_video_format.width) || (f->height != sur40_video_format.height)) return -EINVAL; f->type = V4L2_FRMIVAL_TYPE_DISCRETE; - f->discrete.denominator = 60/(f->index+1); + f->discrete.denominator = 60; f->discrete.numerator = 1; return 0; } @@ -880,6 +893,9 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = { .vidioc_enum_framesizes = sur40_vidioc_enum_framesizes, .vidioc_enum_frameintervals = sur40_vidioc_enum_frameintervals, + .vidioc_g_parm = sur40_ioctl_parm, + .vidioc_s_parm = sur40_ioctl_parm, + .vidioc_enum_input = sur40_vidioc_enum_input, .vidioc_g_input = sur40_vidioc_g_input, .vidioc_s_input = sur40_vidioc_s_input,