v4l: soc-camera: Add support for enum_frameintervals ioctl

  From: Sergio Aguirre <saaguirre@ti.com>

Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
---
 drivers/media/video/soc_camera.c |   37 +++++++++++++++++++++++++++++++++++++
 include/media/soc_camera.h       |    1 +
 2 files changed, 38 insertions(+), 0 deletions(-)
  

Hi Sergio

Sorry about the delay.

On Wed, 18 Apr 2012, Aguirre, Sergio wrote:

> From: Sergio Aguirre <saaguirre@ti.com>
> 
> Signed-off-by: Sergio Aguirre <saaguirre@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 {
>  	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
> 

---
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Guennadi,

No problem.

On Fri, May 4, 2012 at 10:05 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Hi Sergio
>
> Sorry about the delay.
>
> On Wed, 18 Apr 2012, Aguirre, Sergio wrote:
>
>> From: Sergio Aguirre <saaguirre@ti.com>
>>
>> Signed-off-by: Sergio Aguirre <saaguirre@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)
>>               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
>>
>
> ---
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Fri, 4 May 2012, Aguirre, Sergio wrote:

> Hi Guennadi,
> 
> No problem.
> 
> On Fri, May 4, 2012 at 10:05 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Hi Sergio
> >
> > Sorry about the delay.
> >
> > On Wed, 18 Apr 2012, Aguirre, Sergio wrote:
> >
> >> From: Sergio Aguirre <saaguirre@ti.com>
> >>
> >> Signed-off-by: Sergio Aguirre <saaguirre@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html