From: Hans Verkuil <hidden> Date: 2009-03-29 09:06:25
On Sunday 29 March 2009 10:50:02 Patch from Trent Piepho wrote:
The patch number 11260 was added via Trent Piepho [off-list ref]
to http://linuxtv.org/hg/v4l-dvb master development tree.
Kernel patches in this development tree may be modified to be backward
compatible with older kernels. Compatibility modifications will be
removed before inclusion into the mainstream Kernel
If anyone has any objections, please let us know by sending a message to:
Linux Media Mailing List [off-list ref]
------
From: Trent Piepho <redacted>
v4l2-ioctl: Check format for S_PARM and G_PARM
Return EINVAL if VIDIOC_S/G_PARM is called for a buffer type that the
driver doesn't define a ->vidioc_try_fmt_XXX() method for. Several other
ioctls, like QUERYBUF, QBUF, and DQBUF, etc. do this too. It saves each
driver from having to check if the buffer type is one that it supports.
Hi Trent,
I wonder whether this change is correct. Looking at the spec I see that
g/s_parm only supports VIDEO_CAPTURE, VIDEO_OUTPUT and PRIVATE or up.
So what should happen if the type is VIDEO_OVERLAY? I think the g/s_parm
implementation in v4l2-ioctl.c should first exclude the unsupported types
before calling check_fmt.
I also wonder whether check_fmt shouldn't check for the presence of the
s_fmt callbacks instead of try_fmt since try_fmt is an optional ioctl.
Regards,
Hans
quoted hunk
Priority: normal
Signed-off-by: Trent Piepho <redacted>
---
linux/drivers/media/video/v4l2-ioctl.c | 7 +++++++
1 file changed, 7 insertions(+)
diff -r 346bab8698ea -r 44632c5fe5b2
linux/drivers/media/video/v4l2-ioctl.c ---
a/linux/drivers/media/video/v4l2-ioctl.c Sat Mar 28 18:25:35 2009 -0700
+++ b/linux/drivers/media/video/v4l2-ioctl.c Sat Mar 28 18:25:35 2009
-0700 @@ -1552,6 +1552,9 @@ static long __video_do_ioctl(struct file
struct v4l2_streamparm *p = arg;
if (ops->vidioc_g_parm) {
+ ret = check_fmt(ops, p->type);
+ if (ret)
+ break;
ret = ops->vidioc_g_parm(file, fh, p);
} else {
if (p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
@@ -1571,6 +1574,10 @@ static long __video_do_ioctl(struct fileif(!ops->vidioc_s_parm)break;+ret=check_fmt(ops,p->type);+if(ret)+break;+dbgarg(cmd,"type=%d\n",p->type);ret=ops->vidioc_s_parm(file,fh,p);break;---
On Sun, 29 Mar 2009 11:06:19 +0200
Hans Verkuil [off-list ref] wrote:
On Sunday 29 March 2009 10:50:02 Patch from Trent Piepho wrote:
quoted
The patch number 11260 was added via Trent Piepho [off-list ref]
to http://linuxtv.org/hg/v4l-dvb master development tree.
Kernel patches in this development tree may be modified to be backward
compatible with older kernels. Compatibility modifications will be
removed before inclusion into the mainstream Kernel
If anyone has any objections, please let us know by sending a message to:
Linux Media Mailing List [off-list ref]
------
From: Trent Piepho <redacted>
v4l2-ioctl: Check format for S_PARM and G_PARM
Return EINVAL if VIDIOC_S/G_PARM is called for a buffer type that the
driver doesn't define a ->vidioc_try_fmt_XXX() method for. Several other
ioctls, like QUERYBUF, QBUF, and DQBUF, etc. do this too. It saves each
driver from having to check if the buffer type is one that it supports.
Hi Trent,
I wonder whether this change is correct. Looking at the spec I see that
g/s_parm only supports VIDEO_CAPTURE, VIDEO_OUTPUT and PRIVATE or up.
So what should happen if the type is VIDEO_OVERLAY? I think the g/s_parm
implementation in v4l2-ioctl.c should first exclude the unsupported types
before calling check_fmt.
Makes sense to me.
I also wonder whether check_fmt shouldn't check for the presence of the
s_fmt callbacks instead of try_fmt since try_fmt is an optional ioctl.
One developer suggested to merge try_fmt and s_fmt into one callback.
IMO, this makes sense, since I have the feeling that this will simplify the
code a little bit on the drivers. If we go this way, then we can check for
the new try_s_fmt callback.
Cheers,
Mauro
From: Hans Verkuil <hidden> Date: 2009-03-29 09:40:59
On Sunday 29 March 2009 11:35:45 Mauro Carvalho Chehab wrote:
On Sun, 29 Mar 2009 11:06:19 +0200
Hans Verkuil [off-list ref] wrote:
quoted
On Sunday 29 March 2009 10:50:02 Patch from Trent Piepho wrote:
quoted
The patch number 11260 was added via Trent Piepho
[off-list ref] to http://linuxtv.org/hg/v4l-dvb master
development tree.
Kernel patches in this development tree may be modified to be
backward compatible with older kernels. Compatibility modifications
will be removed before inclusion into the mainstream Kernel
If anyone has any objections, please let us know by sending a message
to: Linux Media Mailing List <redacted>
------
From: Trent Piepho <redacted>
v4l2-ioctl: Check format for S_PARM and G_PARM
Return EINVAL if VIDIOC_S/G_PARM is called for a buffer type that the
driver doesn't define a ->vidioc_try_fmt_XXX() method for. Several
other ioctls, like QUERYBUF, QBUF, and DQBUF, etc. do this too. It
saves each driver from having to check if the buffer type is one that
it supports.
Hi Trent,
I wonder whether this change is correct. Looking at the spec I see that
g/s_parm only supports VIDEO_CAPTURE, VIDEO_OUTPUT and PRIVATE or up.
So what should happen if the type is VIDEO_OVERLAY? I think the
g/s_parm implementation in v4l2-ioctl.c should first exclude the
unsupported types before calling check_fmt.
Makes sense to me.
quoted
I also wonder whether check_fmt shouldn't check for the presence of the
s_fmt callbacks instead of try_fmt since try_fmt is an optional ioctl.
One developer suggested to merge try_fmt and s_fmt into one callback.
IMO, this makes sense, since I have the feeling that this will simplify
the code a little bit on the drivers. If we go this way, then we can
check for the new try_s_fmt callback.
I agree with this. It's also easy to gradually migrate to such a new
callback since it is probably quite difficult to do this in one big bang
patch.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
On Sunday 29 March 2009 10:50:02 Patch from Trent Piepho wrote:
quoted
From: Trent Piepho <redacted>
v4l2-ioctl: Check format for S_PARM and G_PARM
Return EINVAL if VIDIOC_S/G_PARM is called for a buffer type that the
driver doesn't define a ->vidioc_try_fmt_XXX() method for. Several other
ioctls, like QUERYBUF, QBUF, and DQBUF, etc. do this too. It saves each
driver from having to check if the buffer type is one that it supports.
Hi Trent,
I wonder whether this change is correct. Looking at the spec I see that
g/s_parm only supports VIDEO_CAPTURE, VIDEO_OUTPUT and PRIVATE or up.
So what should happen if the type is VIDEO_OVERLAY? I think the g/s_parm
implementation in v4l2-ioctl.c should first exclude the unsupported types
before calling check_fmt.
This change doesn't actually enable g/s_parm for VIDEO_OVERLAY (or
VBI_CAPTURE). It's the later bttv and saa7146 changes that do that. I
considered this when I made those changes, as mentioned in those patch
descriptions, but decided it was better to allow it.
In those drivers g_parm only returns the frame rate, which seems just as
valid for VIDEO_OVERLAY as it does for VIDEO_CAPTURE. Why should the
driver not be allowed to return the frame rate for video overlay? Why
should setting the overlay frame rate not be allowed? Those seems like
perfectly reasonable operations to me.
The spec doesn't explicitly say that only VIDEO_CAPTURE, VIDEO_OUTPUT and
PRIVATE are supported. It says the "capture" field of the parm union is
used when type is VIDEO_CAPTURE, the "output" field is used for
VIDEO_OUTPUT, and "raw_data" is used for PRIVATE or higher. You're right
in that it doesn't say what you're supposed to use for VIDEO_OVERLAY,
VBI_CAPTURE or any other buffer types. But it doesn't say they're not
allowed either. IMHO, it's likely the spec authors' intent wasn't to not
allow g_parm with VIDEO_OVERLAY, but rather that they just didn't think of
that case.
Thinking about it now, I think what makes the most sense is to use
"capture" for VIDEO_OVERLAY, VBI_CAPTURE, and SLICED_VBI_CAPTURE. And use
"output" for VBI_OUTPUT, SLICED_VBI_OUTPUT and VIDEO_OUTPUT_OVERLAY.
I also wonder whether check_fmt shouldn't check for the presence of the
s_fmt callbacks instead of try_fmt since try_fmt is an optional ioctl.
I noticed that too. saa7146 doesn't have a try_fmt call for vbi_capture
but is apparently supposed to support it. I sent a message about that
earlier.
From: Hans Verkuil <hidden> Date: 2009-03-29 10:42:46
On Sunday 29 March 2009 12:21:58 Trent Piepho wrote:
On Sun, 29 Mar 2009, Hans Verkuil wrote:
quoted
On Sunday 29 March 2009 10:50:02 Patch from Trent Piepho wrote:
quoted
From: Trent Piepho <redacted>
v4l2-ioctl: Check format for S_PARM and G_PARM
Return EINVAL if VIDIOC_S/G_PARM is called for a buffer type that the
driver doesn't define a ->vidioc_try_fmt_XXX() method for. Several
other ioctls, like QUERYBUF, QBUF, and DQBUF, etc. do this too. It
saves each driver from having to check if the buffer type is one that
it supports.
Hi Trent,
I wonder whether this change is correct. Looking at the spec I see that
g/s_parm only supports VIDEO_CAPTURE, VIDEO_OUTPUT and PRIVATE or up.
So what should happen if the type is VIDEO_OVERLAY? I think the
g/s_parm implementation in v4l2-ioctl.c should first exclude the
unsupported types before calling check_fmt.
This change doesn't actually enable g/s_parm for VIDEO_OVERLAY (or
VBI_CAPTURE). It's the later bttv and saa7146 changes that do that. I
considered this when I made those changes, as mentioned in those patch
descriptions, but decided it was better to allow it.
In those drivers g_parm only returns the frame rate, which seems just as
valid for VIDEO_OVERLAY as it does for VIDEO_CAPTURE. Why should the
driver not be allowed to return the frame rate for video overlay? Why
should setting the overlay frame rate not be allowed? Those seems like
perfectly reasonable operations to me.
Not to me. VIDEO_OVERLAY just defines where the overlay is. But the actual
framerate is entirely dependent on the VIDEO_CAPTURE framerate. Just keep
to the spec for now. If a new driver appears that needs it then we can
always change it.
The spec doesn't explicitly say that only VIDEO_CAPTURE, VIDEO_OUTPUT and
PRIVATE are supported. It says the "capture" field of the parm union is
used when type is VIDEO_CAPTURE, the "output" field is used for
VIDEO_OUTPUT, and "raw_data" is used for PRIVATE or higher. You're right
in that it doesn't say what you're supposed to use for VIDEO_OVERLAY,
VBI_CAPTURE or any other buffer types. But it doesn't say they're not
allowed either. IMHO, it's likely the spec authors' intent wasn't to not
allow g_parm with VIDEO_OVERLAY, but rather that they just didn't think
of that case.
No, I agree with the spec in that I see no use case for it. Should there be
one, then I'd like to see that in an actual driver implementation and in
that case the spec should be adjusted as well.
In addition, g/s_parm is only used in combination with webcams/sensors for
which overlays and vbi are irrelevant.
Thinking about it now, I think what makes the most sense is to use
"capture" for VIDEO_OVERLAY, VBI_CAPTURE, and SLICED_VBI_CAPTURE. And
use "output" for VBI_OUTPUT, SLICED_VBI_OUTPUT and VIDEO_OUTPUT_OVERLAY.
quoted
I also wonder whether check_fmt shouldn't check for the presence of the
s_fmt callbacks instead of try_fmt since try_fmt is an optional ioctl.
I noticed that too. saa7146 doesn't have a try_fmt call for vbi_capture
but is apparently supposed to support it. I sent a message about that
earlier.
I saw that. So why not check for s_fmt instead of try_fmt? That would solve
this potential problem.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
On Sunday 29 March 2009 12:21:58 Trent Piepho wrote:
quoted
On Sun, 29 Mar 2009, Hans Verkuil wrote:
quoted
On Sunday 29 March 2009 10:50:02 Patch from Trent Piepho wrote:
quoted
From: Trent Piepho <redacted>
v4l2-ioctl: Check format for S_PARM and G_PARM
Return EINVAL if VIDIOC_S/G_PARM is called for a buffer type that the
driver doesn't define a ->vidioc_try_fmt_XXX() method for. Several
other ioctls, like QUERYBUF, QBUF, and DQBUF, etc. do this too. It
saves each driver from having to check if the buffer type is one that
it supports.
Hi Trent,
I wonder whether this change is correct. Looking at the spec I see that
g/s_parm only supports VIDEO_CAPTURE, VIDEO_OUTPUT and PRIVATE or up.
So what should happen if the type is VIDEO_OVERLAY? I think the
g/s_parm implementation in v4l2-ioctl.c should first exclude the
unsupported types before calling check_fmt.
This change doesn't actually enable g/s_parm for VIDEO_OVERLAY (or
VBI_CAPTURE). It's the later bttv and saa7146 changes that do that. I
considered this when I made those changes, as mentioned in those patch
descriptions, but decided it was better to allow it.
In those drivers g_parm only returns the frame rate, which seems just as
valid for VIDEO_OVERLAY as it does for VIDEO_CAPTURE. Why should the
driver not be allowed to return the frame rate for video overlay? Why
should setting the overlay frame rate not be allowed? Those seems like
perfectly reasonable operations to me.
Not to me. VIDEO_OVERLAY just defines where the overlay is. But the actual
framerate is entirely dependent on the VIDEO_CAPTURE framerate. Just keep
to the spec for now. If a new driver appears that needs it then we can
always change it.
How does overlay depend on video capture in any way? It's perfectly
reasonable for a driver to support _only_ overlay and not video capture.
The zr36067 chip is only designed to support uncompressed data for video
overlay for example. Allowing uncompressed video capture is a hack that
the driver didn't have at one point.
quoted
The spec doesn't explicitly say that only VIDEO_CAPTURE, VIDEO_OUTPUT and
PRIVATE are supported. It says the "capture" field of the parm union is
used when type is VIDEO_CAPTURE, the "output" field is used for
VIDEO_OUTPUT, and "raw_data" is used for PRIVATE or higher. You're right
in that it doesn't say what you're supposed to use for VIDEO_OVERLAY,
VBI_CAPTURE or any other buffer types. But it doesn't say they're not
allowed either. IMHO, it's likely the spec authors' intent wasn't to not
allow g_parm with VIDEO_OVERLAY, but rather that they just didn't think
of that case.
No, I agree with the spec in that I see no use case for it. Should there be
one, then I'd like to see that in an actual driver implementation and in
that case the spec should be adjusted as well.
How about getting the frame rate of video overlay? Works with bttv.
In addition, g/s_parm is only used in combination with webcams/sensors for
which overlays and vbi are irrelevant.
There are several drivers for non-webcams, like bttv, saa7134, and saa7146,
that support g_parm. Why is returning the frame rate for video capture not
valid? Why does the number of buffers used for read() mode only make
sense for webcams?
quoted
Thinking about it now, I think what makes the most sense is to use
"capture" for VIDEO_OVERLAY, VBI_CAPTURE, and SLICED_VBI_CAPTURE. And
use "output" for VBI_OUTPUT, SLICED_VBI_OUTPUT and VIDEO_OUTPUT_OVERLAY.
quoted
I also wonder whether check_fmt shouldn't check for the presence of the
s_fmt callbacks instead of try_fmt since try_fmt is an optional ioctl.
I noticed that too. saa7146 doesn't have a try_fmt call for vbi_capture
but is apparently supposed to support it. I sent a message about that
earlier.
I saw that. So why not check for s_fmt instead of try_fmt? That would solve
this potential problem.
Because that's clearly a change that belongs in another patch.
From: Hans Verkuil <hidden> Date: 2009-03-29 11:31:39
On Sunday 29 March 2009 13:03:13 Trent Piepho wrote:
On Sun, 29 Mar 2009, Hans Verkuil wrote:
quoted
On Sunday 29 March 2009 12:21:58 Trent Piepho wrote:
quoted
On Sun, 29 Mar 2009, Hans Verkuil wrote:
quoted
On Sunday 29 March 2009 10:50:02 Patch from Trent Piepho wrote:
quoted
From: Trent Piepho <redacted>
v4l2-ioctl: Check format for S_PARM and G_PARM
Return EINVAL if VIDIOC_S/G_PARM is called for a buffer type that
the driver doesn't define a ->vidioc_try_fmt_XXX() method for.
Several other ioctls, like QUERYBUF, QBUF, and DQBUF, etc. do
this too. It saves each driver from having to check if the
buffer type is one that it supports.
Hi Trent,
I wonder whether this change is correct. Looking at the spec I see
that g/s_parm only supports VIDEO_CAPTURE, VIDEO_OUTPUT and PRIVATE
or up.
So what should happen if the type is VIDEO_OVERLAY? I think the
g/s_parm implementation in v4l2-ioctl.c should first exclude the
unsupported types before calling check_fmt.
This change doesn't actually enable g/s_parm for VIDEO_OVERLAY (or
VBI_CAPTURE). It's the later bttv and saa7146 changes that do that.
I considered this when I made those changes, as mentioned in those
patch descriptions, but decided it was better to allow it.
In those drivers g_parm only returns the frame rate, which seems just
as valid for VIDEO_OVERLAY as it does for VIDEO_CAPTURE. Why should
the driver not be allowed to return the frame rate for video overlay?
Why should setting the overlay frame rate not be allowed? Those
seems like perfectly reasonable operations to me.
Not to me. VIDEO_OVERLAY just defines where the overlay is. But the
actual framerate is entirely dependent on the VIDEO_CAPTURE framerate.
Just keep to the spec for now. If a new driver appears that needs it
then we can always change it.
How does overlay depend on video capture in any way? It's perfectly
reasonable for a driver to support _only_ overlay and not video capture.
The zr36067 chip is only designed to support uncompressed data for video
overlay for example. Allowing uncompressed video capture is a hack that
the driver didn't have at one point.
Ah. Live and learn. In that case the spec is out of date and needs to be
updated.
quoted
quoted
The spec doesn't explicitly say that only VIDEO_CAPTURE, VIDEO_OUTPUT
and PRIVATE are supported. It says the "capture" field of the parm
union is used when type is VIDEO_CAPTURE, the "output" field is used
for VIDEO_OUTPUT, and "raw_data" is used for PRIVATE or higher.
You're right in that it doesn't say what you're supposed to use for
VIDEO_OVERLAY, VBI_CAPTURE or any other buffer types. But it doesn't
say they're not allowed either. IMHO, it's likely the spec authors'
intent wasn't to not allow g_parm with VIDEO_OVERLAY, but rather that
they just didn't think of that case.
No, I agree with the spec in that I see no use case for it. Should
there be one, then I'd like to see that in an actual driver
implementation and in that case the spec should be adjusted as well.
How about getting the frame rate of video overlay? Works with bttv.
Hmm, I grepped only on s_parm, not on g_parm.
quoted
In addition, g/s_parm is only used in combination with webcams/sensors
for which overlays and vbi are irrelevant.
There are several drivers for non-webcams, like bttv, saa7134, and
saa7146, that support g_parm. Why is returning the frame rate for video
capture not valid? Why does the number of buffers used for read() mode
only make sense for webcams?
OK, I'd forgot to check for the usage of g_parm. My bad.
quoted
quoted
Thinking about it now, I think what makes the most sense is to use
"capture" for VIDEO_OVERLAY, VBI_CAPTURE, and SLICED_VBI_CAPTURE.
And use "output" for VBI_OUTPUT, SLICED_VBI_OUTPUT and
VIDEO_OUTPUT_OVERLAY.
You're absolutely correct. I was too hasty.
Can you update the spec to reflect this?
quoted
quoted
quoted
I also wonder whether check_fmt shouldn't check for the presence of
the s_fmt callbacks instead of try_fmt since try_fmt is an optional
ioctl.
I noticed that too. saa7146 doesn't have a try_fmt call for
vbi_capture but is apparently supposed to support it. I sent a
message about that earlier.
I saw that. So why not check for s_fmt instead of try_fmt? That would
solve this potential problem.
Because that's clearly a change that belongs in another patch.
OK, great.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
On Sunday 29 March 2009 13:03:13 Trent Piepho wrote:
quoted
How does overlay depend on video capture in any way? It's perfectly
reasonable for a driver to support _only_ overlay and not video capture.
The zr36067 chip is only designed to support uncompressed data for video
overlay for example. Allowing uncompressed video capture is a hack that
the driver didn't have at one point.
Ah. Live and learn. In that case the spec is out of date and needs to be
updated.
Do you mean there is something in the spec that makes overlay depend on
video capture? Or that s/g_parm don't mention buffer types other than
video capture and video output?
quoted
quoted
No, I agree with the spec in that I see no use case for it. Should
there be one, then I'd like to see that in an actual driver
implementation and in that case the spec should be adjusted as well.
How about getting the frame rate of video overlay? Works with bttv.
Hmm, I grepped only on s_parm, not on g_parm.
It would be nice to use s_parm with drivers like bttv to reduce the frame
rate. With multi-chip capture cards running out of bus bandwidth is a big
problem. Reducing the frame rate is one way to deal with it. The bt8x8
and cx2388x chips do have a temporal decimation feature and I've tried to
add support for it but I never got it to work correctly.
quoted
quoted
In addition, g/s_parm is only used in combination with webcams/sensors
for which overlays and vbi are irrelevant.
There are several drivers for non-webcams, like bttv, saa7134, and
saa7146, that support g_parm. Why is returning the frame rate for video
capture not valid? Why does the number of buffers used for read() mode
only make sense for webcams?
OK, I'd forgot to check for the usage of g_parm. My bad.
There is also a default g_parm handler in video_ioctl2 that might be used
if the driver doesn't provide one. I don't know what drivers actually use
it.
quoted
quoted
quoted
Thinking about it now, I think what makes the most sense is to use
"capture" for VIDEO_OVERLAY, VBI_CAPTURE, and SLICED_VBI_CAPTURE.
And use "output" for VBI_OUTPUT, SLICED_VBI_OUTPUT and
VIDEO_OUTPUT_OVERLAY.
You're absolutely correct. I was too hasty.
Can you update the spec to reflect this?
I'll try, but I hear the doc build system is quite a challenge.
quoted
quoted
quoted
quoted
I also wonder whether check_fmt shouldn't check for the presence of
the s_fmt callbacks instead of try_fmt since try_fmt is an optional
ioctl.
I noticed that too. saa7146 doesn't have a try_fmt call for
vbi_capture but is apparently supposed to support it. I sent a
message about that earlier.
I saw that. So why not check for s_fmt instead of try_fmt? That would
solve this potential problem.
Because that's clearly a change that belongs in another patch.
OK, great.
My patch just called check_fmt() for s/g_parm. I haven't touched
check_fmt(). But you're right that try_fmt is optional so it makes a bad
test.
Maybe it should use g_fmt? saa7146 doesn't provide a s_fmt call either for
vbi capture, only g_fmt. Though maybe this is a bug in saa7146? It seems
like any driver that provides g_fmt can always just use that same method
for s_fmt as well and be correct.