[PATCH v2 1/3] sur40: properly report a single frame rate of 60 FPS

[PATCH v2 1/3] sur40: properly report a single frame rate of 60 FPS

[Florian Echtler]

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,
+
 	.vidioc_enum_input	= sur40_vidioc_enum_input,
 	.vidioc_g_input		= sur40_vidioc_g_input,
 	.vidioc_s_input		= sur40_vidioc_s_input,
-- 
1.9.1

[PATCH v2 2/3] sur40: lower poll interval to fix occasional FPS drops to ~56 FPS

[Florian Echtler]

The framerate sometimes drops below 60 Hz if the poll interval is too high.
Lowering it to the minimum of 1 ms fixes this.

Signed-off-by: Martin Kaltenbrunner <modin@yuri.at>
Signed-off-by: Florian Echtler <floe@butterbrot.org>
---
 drivers/input/touchscreen/sur40.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 4b1f703..64e588c 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -126,7 +126,7 @@ struct sur40_image_header {
 #define VIDEO_PACKET_SIZE  16384
 
 /* polling interval (ms) */
-#define POLL_INTERVAL 4
+#define POLL_INTERVAL 1
 
 /* maximum number of contacts FIXME: this is a guess? */
 #define MAX_CONTACTS 64
-- 
1.9.1

[PATCH v2 3/3] sur40: fix occasional oopses on device close

[Florian Echtler]

Closing the V4L2 device sometimes triggers a kernel oops.
Present patch fixes this.

Signed-off-by: Martin Kaltenbrunner <modin@yuri.at>
Signed-off-by: Florian Echtler <floe@butterbrot.org>
---
 drivers/input/touchscreen/sur40.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 64e588c..85dedc1 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -448,7 +448,7 @@ static void sur40_process_video(struct sur40_state *sur40)
 
 	/* return error if streaming was stopped in the meantime */
 	if (sur40->sequence == -1)
-		goto err_poll;
+		return;
 
 	/* mark as finished */
 	new_buf->vb.vb2_buf.timestamp = ktime_get_ns();
@@ -736,6 +736,7 @@ static int sur40_start_streaming(struct vb2_queue *vq, unsigned int count)
 static void sur40_stop_streaming(struct vb2_queue *vq)
 {
 	struct sur40_state *sur40 = vb2_get_drv_priv(vq);
+	vb2_wait_for_all_buffers(vq);
 	sur40->sequence = -1;
 
 	/* Release all active buffers */
-- 
1.9.1

Re: [PATCH v2 1/3] sur40: properly report a single frame rate of 60 FPS

[Hans Verkuil]

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,
> 

Re: [PATCH v2 1/3] sur40: properly report a single frame rate of 60 FPS

[Florian Echtler]

[-- Attachment #1.1: Type: text/plain, Size: 1428 bytes --]

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
-- 
SENT FROM MY DEC VT50 TERMINAL


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 1/3] sur40: properly report a single frame rate of 60 FPS

[Hans Verkuil]

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
> 

Re: [PATCH v2 1/3] sur40: properly report a single frame rate of 60 FPS

[Hans Verkuil]

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

Re: [PATCH v2 1/3] sur40: properly report a single frame rate of 60 FPS

[Florian Echtler]

[-- Attachment #1.1: Type: text/plain, Size: 1920 bytes --]

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
-- 
SENT FROM MY DEC VT50 TERMINAL


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 1/3] sur40: properly report a single frame rate of 60 FPS

[Hans Verkuil]

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