Re: [PATCH] v4l: soc-camera: Add support for enum_frameintervals ioctl
On Fri, 4 May 2012, Aguirre, Sergio wrote:
Hi Guennadi,
No problem.
On Fri, May 4, 2012 at 10:05 AM, Guennadi Liakhovetski
g.liakhovet...@gmx.de wrote:
Hi Sergio
Sorry about the delay.
On Wed, 18 Apr 2012, Aguirre, Sergio wrote:
From: Sergio Aguirre saagui...@ti.com
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/soc_camera.c | 37
+
include/media/soc_camera.h | 1 +
2 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/soc_camera.c
b/drivers/media/video/soc_camera.c
index eb25756..62c8956 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -266,6 +266,15 @@ static int soc_camera_enum_fsizes(struct file
*file, void *fh,
return ici-ops-enum_fsizes(icd, fsize);
}
+static int soc_camera_enum_fivals(struct file *file, void *fh,
fivals is a bit short for my taste. Yes, I know about the
*_enum_fsizes() precedent in soc_camera.c, we should have used a more
descriptive name for that too. So, maybe I'll push a patch to change that
to enum_frmsizes() or enum_framesizes().
Agreed.
But that brings in a larger question, which is also the reason, why I
added a couple more people to the CC: the following 3 operations in struct
v4l2_subdev_video_ops don't make me particularly happy:
int (*enum_framesizes)(struct v4l2_subdev *sd, struct
v4l2_frmsizeenum *fsize);
int (*enum_frameintervals)(struct v4l2_subdev *sd, struct
v4l2_frmivalenum *fival);
int (*enum_mbus_fsizes)(struct v4l2_subdev *sd,
struct v4l2_frmsizeenum *fsize);
The problems are:
1. enum_framesizes and enum_mbus_fsizes seem to be identical (yes, I see
my Sob under the latter:-()
Yeah, IMHO, the mbus one should go, since there's no mbus specific structure
being handed as a parameter.
Right, we can do that.
2. both struct v4l2_frmsizeenum and struct v4l2_frmivalenum are the
structs, used in the respective V4L2 ioctl()s, and they both contain a
field for a fourcc value, which doesn't make sense to subdevice drivers.
So far the only driver combination in the mainline, that I see, that uses
these operations is marvell-ccic ov7670. These drivers just ignore the
pixel format. Relatively recently enum_mbus_fsizes() has been added to
soc-camera, and this patch is adding enum_frameintervals(). Both these
implementations abuse the .pixel_format field to pass a media-bus code
value in it to subdevice drivers. This sends meaningful information to
subdevice drivers, but is really a hack, rather than a proper
implementation.
True.
Any idea how to improve this? Shall we create mediabus clones of those
structs with an mbus code instead of fourcc, and drop one of the above
enum_framesizes() operations?
Well, to add more confusion to this.. :)
We have this v4l2-subdev IOCTLs exported to userspace:
#define VIDIOC_SUBDEV_ENUM_FRAME_SIZE \
_IOWR('V', 74, struct v4l2_subdev_frame_size_enum)
#define VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL \
_IOWR('V', 75, struct v4l2_subdev_frame_interval_enum)
Which in drivers/media/video/v4l2-subdev.c, are translated to pad ops:
- v4l2_subdev_call(... enum_frame_size ...);
- v4l2_subdev_call(... enum_frame_interval ...);
respectively.
So, this is also another thing that's causing some confusion.
Wow, didn't know about those. I was about to propose to use those in video
subdev ops, but then I noticed: pad operations don't support stepwise
enumerations. struct v4l2_subdev_frame_size_enum seems to implement
continuous enumeration implicitly, with discrete being a particular
degenerate case of it. struct v4l2_subdev_frame_interval_enum only
implements discrete enumeration only. I personally don't have a problem
with that, I'm not a big fan of these enumeration operations anyway, and
AFAICS, the only subdevice driver, implementing those video operations is
ov7670, and it implements only DISCRETE.
So, would it be good enough for everyone to drop enum_mbus_fsizes() and to
convert enum_frameintervals() and enum_framesizes() video operations to
use structs from pad operations and just ignore the pad field? Any
objections?
Does soc_camera use pad ops?
No, not yet.
Regards,
Sergio
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] v4l: soc-camera: Add support for enum_frameintervals ioctl
Hi Sergio
Sorry about the delay.
On Wed, 18 Apr 2012, Aguirre, Sergio wrote:
From: Sergio Aguirre saagui...@ti.com
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/soc_camera.c | 37 +
include/media/soc_camera.h |1 +
2 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/soc_camera.c
b/drivers/media/video/soc_camera.c
index eb25756..62c8956 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -266,6 +266,15 @@ static int soc_camera_enum_fsizes(struct file
*file, void *fh,
return ici-ops-enum_fsizes(icd, fsize);
}
+static int soc_camera_enum_fivals(struct file *file, void *fh,
fivals is a bit short for my taste. Yes, I know about the
*_enum_fsizes() precedent in soc_camera.c, we should have used a more
descriptive name for that too. So, maybe I'll push a patch to change that
to enum_frmsizes() or enum_framesizes().
But that brings in a larger question, which is also the reason, why I
added a couple more people to the CC: the following 3 operations in struct
v4l2_subdev_video_ops don't make me particularly happy:
int (*enum_framesizes)(struct v4l2_subdev *sd, struct v4l2_frmsizeenum
*fsize);
int (*enum_frameintervals)(struct v4l2_subdev *sd, struct
v4l2_frmivalenum *fival);
int (*enum_mbus_fsizes)(struct v4l2_subdev *sd,
struct v4l2_frmsizeenum *fsize);
The problems are:
1. enum_framesizes and enum_mbus_fsizes seem to be identical (yes, I see
my Sob under the latter:-()
2. both struct v4l2_frmsizeenum and struct v4l2_frmivalenum are the
structs, used in the respective V4L2 ioctl()s, and they both contain a
field for a fourcc value, which doesn't make sense to subdevice drivers.
So far the only driver combination in the mainline, that I see, that uses
these operations is marvell-ccic ov7670. These drivers just ignore the
pixel format. Relatively recently enum_mbus_fsizes() has been added to
soc-camera, and this patch is adding enum_frameintervals(). Both these
implementations abuse the .pixel_format field to pass a media-bus code
value in it to subdevice drivers. This sends meaningful information to
subdevice drivers, but is really a hack, rather than a proper
implementation.
Any idea how to improve this? Shall we create mediabus clones of those
structs with an mbus code instead of fourcc, and drop one of the above
enum_framesizes() operations?
Thanks
Guennadi
+struct v4l2_frmivalenum *fival)
+{
+ struct soc_camera_device *icd = file-private_data;
+ struct soc_camera_host *ici = to_soc_camera_host(icd-parent);
+
+ return ici-ops-enum_fivals(icd, fival);
+}
+
static int soc_camera_reqbufs(struct file *file, void *priv,
struct v4l2_requestbuffers *p)
{
@@ -1266,6 +1275,31 @@ static int default_enum_fsizes(struct
soc_camera_device *icd,
return 0;
}
+static int default_enum_fivals(struct soc_camera_device *icd,
+ struct v4l2_frmivalenum *fival)
+{
+ int ret;
+ struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+ const struct soc_camera_format_xlate *xlate;
+ __u32 pixfmt = fival-pixel_format;
+ struct v4l2_frmivalenum fival_sd = *fival;
+
+ xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
+ if (!xlate)
+ return -EINVAL;
+ /* map xlate-code to pixel_format, sensor only handle xlate-code*/
+ fival_sd.pixel_format = xlate-code;
+
+ ret = v4l2_subdev_call(sd, video, enum_frameintervals, fival_sd);
+ if (ret 0)
+ return ret;
+
+ *fival = fival_sd;
+ fival-pixel_format = pixfmt;
+
+ return 0;
+}
+
int soc_camera_host_register(struct soc_camera_host *ici)
{
struct soc_camera_host *ix;
@@ -1297,6 +1331,8 @@ int soc_camera_host_register(struct soc_camera_host
*ici)
ici-ops-get_parm = default_g_parm;
if (!ici-ops-enum_fsizes)
ici-ops-enum_fsizes = default_enum_fsizes;
+ if (!ici-ops-enum_fivals)
+ ici-ops-enum_fivals = default_enum_fivals;
mutex_lock(list_lock);
list_for_each_entry(ix, hosts, list) {
@@ -1387,6 +1423,7 @@ static const struct v4l2_ioctl_ops
soc_camera_ioctl_ops = {
.vidioc_s_std= soc_camera_s_std,
.vidioc_g_std= soc_camera_g_std,
.vidioc_enum_framesizes = soc_camera_enum_fsizes,
+ .vidioc_enum_frameintervals = soc_camera_enum_fivals,
.vidioc_reqbufs = soc_camera_reqbufs,
.vidioc_querybuf = soc_camera_querybuf,
.vidioc_qbuf = soc_camera_qbuf,
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index b5c2b6c..0a3ac07 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -98,6 +98,7 @@ struct soc_camera_host_ops
Re: [PATCH] v4l: soc-camera: Add support for enum_frameintervals ioctl
Hi Guennadi,
No problem.
On Fri, May 4, 2012 at 10:05 AM, Guennadi Liakhovetski
g.liakhovet...@gmx.de wrote:
Hi Sergio
Sorry about the delay.
On Wed, 18 Apr 2012, Aguirre, Sergio wrote:
From: Sergio Aguirre saagui...@ti.com
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/soc_camera.c | 37
+
include/media/soc_camera.h | 1 +
2 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/soc_camera.c
b/drivers/media/video/soc_camera.c
index eb25756..62c8956 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -266,6 +266,15 @@ static int soc_camera_enum_fsizes(struct file
*file, void *fh,
return ici-ops-enum_fsizes(icd, fsize);
}
+static int soc_camera_enum_fivals(struct file *file, void *fh,
fivals is a bit short for my taste. Yes, I know about the
*_enum_fsizes() precedent in soc_camera.c, we should have used a more
descriptive name for that too. So, maybe I'll push a patch to change that
to enum_frmsizes() or enum_framesizes().
Agreed.
But that brings in a larger question, which is also the reason, why I
added a couple more people to the CC: the following 3 operations in struct
v4l2_subdev_video_ops don't make me particularly happy:
int (*enum_framesizes)(struct v4l2_subdev *sd, struct v4l2_frmsizeenum
*fsize);
int (*enum_frameintervals)(struct v4l2_subdev *sd, struct
v4l2_frmivalenum *fival);
int (*enum_mbus_fsizes)(struct v4l2_subdev *sd,
struct v4l2_frmsizeenum *fsize);
The problems are:
1. enum_framesizes and enum_mbus_fsizes seem to be identical (yes, I see
my Sob under the latter:-()
Yeah, IMHO, the mbus one should go, since there's no mbus specific structure
being handed as a parameter.
2. both struct v4l2_frmsizeenum and struct v4l2_frmivalenum are the
structs, used in the respective V4L2 ioctl()s, and they both contain a
field for a fourcc value, which doesn't make sense to subdevice drivers.
So far the only driver combination in the mainline, that I see, that uses
these operations is marvell-ccic ov7670. These drivers just ignore the
pixel format. Relatively recently enum_mbus_fsizes() has been added to
soc-camera, and this patch is adding enum_frameintervals(). Both these
implementations abuse the .pixel_format field to pass a media-bus code
value in it to subdevice drivers. This sends meaningful information to
subdevice drivers, but is really a hack, rather than a proper
implementation.
True.
Any idea how to improve this? Shall we create mediabus clones of those
structs with an mbus code instead of fourcc, and drop one of the above
enum_framesizes() operations?
Well, to add more confusion to this.. :)
We have this v4l2-subdev IOCTLs exported to userspace:
#define VIDIOC_SUBDEV_ENUM_FRAME_SIZE \
_IOWR('V', 74, struct v4l2_subdev_frame_size_enum)
#define VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL \
_IOWR('V', 75, struct v4l2_subdev_frame_interval_enum)
Which in drivers/media/video/v4l2-subdev.c, are translated to pad ops:
- v4l2_subdev_call(... enum_frame_size ...);
- v4l2_subdev_call(... enum_frame_interval ...);
respectively.
So, this is also another thing that's causing some confusion.
Does soc_camera use pad ops?
Regards,
Sergio
Thanks
Guennadi
+ struct v4l2_frmivalenum *fival)
+{
+ struct soc_camera_device *icd = file-private_data;
+ struct soc_camera_host *ici = to_soc_camera_host(icd-parent);
+
+ return ici-ops-enum_fivals(icd, fival);
+}
+
static int soc_camera_reqbufs(struct file *file, void *priv,
struct v4l2_requestbuffers *p)
{
@@ -1266,6 +1275,31 @@ static int default_enum_fsizes(struct
soc_camera_device *icd,
return 0;
}
+static int default_enum_fivals(struct soc_camera_device *icd,
+ struct v4l2_frmivalenum *fival)
+{
+ int ret;
+ struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+ const struct soc_camera_format_xlate *xlate;
+ __u32 pixfmt = fival-pixel_format;
+ struct v4l2_frmivalenum fival_sd = *fival;
+
+ xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
+ if (!xlate)
+ return -EINVAL;
+ /* map xlate-code to pixel_format, sensor only handle xlate-code*/
+ fival_sd.pixel_format = xlate-code;
+
+ ret = v4l2_subdev_call(sd, video, enum_frameintervals, fival_sd);
+ if (ret 0)
+ return ret;
+
+ *fival = fival_sd;
+ fival-pixel_format = pixfmt;
+
+ return 0;
+}
+
int soc_camera_host_register(struct soc_camera_host *ici)
{
struct soc_camera_host *ix;
@@ -1297,6 +1331,8 @@ int soc_camera_host_register(struct soc_camera_host
*ici)
ici-ops-get_parm = default_g_parm;
if (!ici-ops-enum_fsizes)
[PATCH] v4l: soc-camera: Add support for enum_frameintervals ioctl
From: Sergio Aguirre saagui...@ti.com
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/soc_camera.c | 37 +
include/media/soc_camera.h |1 +
2 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index eb25756..62c8956 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -266,6 +266,15 @@ static int soc_camera_enum_fsizes(struct file
*file, void *fh,
return ici-ops-enum_fsizes(icd, fsize);
}
+static int soc_camera_enum_fivals(struct file *file, void *fh,
+ struct v4l2_frmivalenum *fival)
+{
+ struct soc_camera_device *icd = file-private_data;
+ struct soc_camera_host *ici = to_soc_camera_host(icd-parent);
+
+ return ici-ops-enum_fivals(icd, fival);
+}
+
static int soc_camera_reqbufs(struct file *file, void *priv,
struct v4l2_requestbuffers *p)
{
@@ -1266,6 +1275,31 @@ static int default_enum_fsizes(struct
soc_camera_device *icd,
return 0;
}
+static int default_enum_fivals(struct soc_camera_device *icd,
+ struct v4l2_frmivalenum *fival)
+{
+ int ret;
+ struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+ const struct soc_camera_format_xlate *xlate;
+ __u32 pixfmt = fival-pixel_format;
+ struct v4l2_frmivalenum fival_sd = *fival;
+
+ xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
+ if (!xlate)
+ return -EINVAL;
+ /* map xlate-code to pixel_format, sensor only handle xlate-code*/
+ fival_sd.pixel_format = xlate-code;
+
+ ret = v4l2_subdev_call(sd, video, enum_frameintervals, fival_sd);
+ if (ret 0)
+ return ret;
+
+ *fival = fival_sd;
+ fival-pixel_format = pixfmt;
+
+ return 0;
+}
+
int soc_camera_host_register(struct soc_camera_host *ici)
{
struct soc_camera_host *ix;
@@ -1297,6 +1331,8 @@ int soc_camera_host_register(struct soc_camera_host *ici)
ici-ops-get_parm = default_g_parm;
if (!ici-ops-enum_fsizes)
ici-ops-enum_fsizes = default_enum_fsizes;
+ if (!ici-ops-enum_fivals)
+ ici-ops-enum_fivals = default_enum_fivals;
mutex_lock(list_lock);
list_for_each_entry(ix, hosts, list) {
@@ -1387,6 +1423,7 @@ static const struct v4l2_ioctl_ops
soc_camera_ioctl_ops = {
.vidioc_s_std= soc_camera_s_std,
.vidioc_g_std= soc_camera_g_std,
.vidioc_enum_framesizes = soc_camera_enum_fsizes,
+ .vidioc_enum_frameintervals = soc_camera_enum_fivals,
.vidioc_reqbufs = soc_camera_reqbufs,
.vidioc_querybuf = soc_camera_querybuf,
.vidioc_qbuf = soc_camera_qbuf,
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index b5c2b6c..0a3ac07 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -98,6 +98,7 @@ struct soc_camera_host_ops {
int (*get_parm)(struct soc_camera_device *, struct v4l2_streamparm *);
int (*set_parm)(struct soc_camera_device *, struct v4l2_streamparm *);
int (*enum_fsizes)(struct soc_camera_device *, struct v4l2_frmsizeenum
*);
+ int (*enum_fivals)(struct soc_camera_device *, struct v4l2_frmivalenum
*);
unsigned int (*poll)(struct file *, poll_table *);
};
--
1.7.5.4
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
