[RFC] omap3-isp G_FMT & ENUM_FMT

[RFC] omap3-isp G_FMT & ENUM_FMT

[Michael Jones]

Hello,

I would like to (re)submit a couple of patches to support V4L2 behavior at the
V4L2 device nodes of the omap3-isp driver, but I'm guessing they require some
discussion first.

I've previously submitted one of them here [1] to support ENUM_FMT for the
omap3-isp. This sparked some discussion, the result of which was that my patch
probably made sense. Later [2], Laurent mentioned that there was some
discussion/decision about adding "profiles" to the V4L2 specification, and the
OMAP3 ISP would probably implement the "streaming" profile.  That was 7 months
ago and I haven't seen any discussion of such profiles.  Can somebody bring me
up to speed on this?

The purpose of these two patches is for the V4L2 device nodes to support
mandatory V4L2 ioctls G_FMT and ENUM_FMT, such that a pure V4L2 application,
ignorant of the media controller, can still stream the images from the video
nodes.  This presumes that the media controller would have been pre-configured.
This approach is often seen on the mailing list using 'media-ctl' to configure
the ISP, then 'yavta' to retrieve images.  Currently this works because yavta
doesn't require ENUM_FMT (unlike Gstreamer), and only as long as one sets the
same format with yavta as had already been set with media-ctl. I think yavta
should be able to just do G_FMT to see what is configured.

To be clear (as discussed in [1]), ENUM_FMT does not behave according to its
original intent, because it cannot enumerate possible formats the ISP can
deliver.  It will enumerate only one format: the one configured with the media
controller.  In a sense this complies with the specification, because S_FMT
wouldn't be able to change the format to anything else.

I have tested these patches on v3.4, but I have rebased them to v3.5 here.
I would remove the pr_debug() calls before submitting upstream, but they're
useful for testing.

[1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg29640.html
[2] http://www.mail-archive.com/linux-media@vger.kernel.org/msg40618.html

Michael Jones (2):
  [media] omap3isp: implement ENUM_FMT
  [media] omap3isp: support G_FMT

 drivers/media/video/omap3isp/ispvideo.c |   50 +++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier

[PATCH 1/2] [media] omap3isp: implement ENUM_FMT

[Michael Jones]

ENUM_FMT will not enumerate all formats that the ISP is capable of,
it will only return the format which has been previously configured
using the media controller, because this is the only format available
to a V4L2 application which is unaware of the media controller.

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 drivers/media/video/omap3isp/ispvideo.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispvideo.c b/drivers/media/video/omap3isp/ispvideo.c
index b37379d..d1d2c14 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -678,6 +678,28 @@ isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
 }
 
 static int
+isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *fmtdesc)
+{
+	struct isp_video_fh *vfh = to_isp_video_fh(fh);
+	struct isp_video *video = video_drvdata(file);
+
+	if (fmtdesc->index)
+		return -EINVAL;
+
+	if (fmtdesc->type != video->type)
+		return -EINVAL;
+
+	fmtdesc->flags = 0;
+	fmtdesc->description[0] = 0;
+
+	mutex_lock(&video->mutex);
+	fmtdesc->pixelformat = vfh->format.fmt.pix.pixelformat;
+	mutex_unlock(&video->mutex);
+
+	return 0;
+}
+
+static int
 isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
 {
 	struct isp_video_fh *vfh = to_isp_video_fh(fh);
@@ -1191,6 +1213,7 @@ isp_video_s_input(struct file *file, void *fh, unsigned int input)
 
 static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
 	.vidioc_querycap		= isp_video_querycap,
+	.vidioc_enum_fmt_vid_cap	= isp_video_enum_format,
 	.vidioc_g_fmt_vid_cap		= isp_video_get_format,
 	.vidioc_s_fmt_vid_cap		= isp_video_set_format,
 	.vidioc_try_fmt_vid_cap		= isp_video_try_format,
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier

[PATCH 2/2] [media] omap3isp: support G_FMT

[Michael Jones]

This allows a V4L2 application which has no knowledge of the media
controller to open a video device node of the already-configured ISP
and query what it will deliver. Previously, G_FMT only worked after a
S_FMT had already been done.

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 drivers/media/video/omap3isp/ispvideo.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispvideo.c b/drivers/media/video/omap3isp/ispvideo.c
index d1d2c14..955211b 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -1244,6 +1244,7 @@ static int isp_video_open(struct file *file)
 {
 	struct isp_video *video = video_drvdata(file);
 	struct isp_video_fh *handle;
+	struct media_pad *src_pad;
 	int ret = 0;
 
 	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
@@ -1273,6 +1274,32 @@ static int isp_video_open(struct file *file)
 	handle->format.type = video->type;
 	handle->timeperframe.denominator = 1;
 
+	src_pad = media_entity_remote_source(&video->pad);
+
+	if (src_pad) { /* it's on an active link */
+		struct v4l2_subdev_format srcfmt = {
+			.pad = src_pad->index,
+			.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		};
+		struct v4l2_subdev *src_subdev =
+			isp_video_remote_subdev(video, NULL);
+		pr_debug("%s src_subdev=\"%s\"\n", __func__, src_subdev->name);
+
+		ret = v4l2_subdev_call(src_subdev, pad, get_fmt, NULL, &srcfmt);
+		if (ret)
+			goto done;
+		pr_debug("%s MBUS format %dx%d code:%x\n", __func__,
+				srcfmt.format.width, srcfmt.format.height,
+				srcfmt.format.code);
+
+		isp_video_mbus_to_pix(video, &srcfmt.format,
+			&handle->format.fmt.pix);
+		pr_debug("%s V4L format %dx%d 4CC:%x\n", __func__,
+				handle->format.fmt.pix.width,
+				handle->format.fmt.pix.height,
+				handle->format.fmt.pix.pixelformat);
+	}
+
 	handle->video = video;
 	file->private_data = &handle->vfh;
 
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier

Re: [RFC] omap3-isp G_FMT & ENUM_FMT

[Hans Verkuil]

On Thu 26 July 2012 13:59:54 Michael Jones wrote:
> Hello,
> 
> I would like to (re)submit a couple of patches to support V4L2 behavior at the
> V4L2 device nodes of the omap3-isp driver, but I'm guessing they require some
> discussion first.
> 
> I've previously submitted one of them here [1] to support ENUM_FMT for the
> omap3-isp. This sparked some discussion, the result of which was that my patch
> probably made sense. Later [2], Laurent mentioned that there was some
> discussion/decision about adding "profiles" to the V4L2 specification, and the
> OMAP3 ISP would probably implement the "streaming" profile.  That was 7 months
> ago and I haven't seen any discussion of such profiles.  Can somebody bring me
> up to speed on this?

Not much has happened. Actually, I've prepared a 'radio' profile which I hope
to post before the V4L2 workshop next month. But no other profiles were made yet.

In practice the v4l2-compliance tool does lots of checks that very closely
resemble a 'profile' checker.

But it will need some more work to correctly support low-level media
controller-based drivers.

Regards,

	Hans

> 
> The purpose of these two patches is for the V4L2 device nodes to support
> mandatory V4L2 ioctls G_FMT and ENUM_FMT, such that a pure V4L2 application,
> ignorant of the media controller, can still stream the images from the video
> nodes.  This presumes that the media controller would have been pre-configured.
> This approach is often seen on the mailing list using 'media-ctl' to configure
> the ISP, then 'yavta' to retrieve images.  Currently this works because yavta
> doesn't require ENUM_FMT (unlike Gstreamer), and only as long as one sets the
> same format with yavta as had already been set with media-ctl. I think yavta
> should be able to just do G_FMT to see what is configured.
> 
> To be clear (as discussed in [1]), ENUM_FMT does not behave according to its
> original intent, because it cannot enumerate possible formats the ISP can
> deliver.  It will enumerate only one format: the one configured with the media
> controller.  In a sense this complies with the specification, because S_FMT
> wouldn't be able to change the format to anything else.
> 
> I have tested these patches on v3.4, but I have rebased them to v3.5 here.
> I would remove the pr_debug() calls before submitting upstream, but they're
> useful for testing.
> 
> [1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg29640.html
> [2] http://www.mail-archive.com/linux-media@vger.kernel.org/msg40618.html
> 
> Michael Jones (2):
>   [media] omap3isp: implement ENUM_FMT
>   [media] omap3isp: support G_FMT
> 
>  drivers/media/video/omap3isp/ispvideo.c |   50 +++++++++++++++++++++++++++++++
>  1 files changed, 50 insertions(+), 0 deletions(-)
> 
> 

Re: [RFC] omap3-isp G_FMT & ENUM_FMT

[Laurent Pinchart]

Hi Michael,

On Thursday 26 July 2012 13:59:54 Michael Jones wrote:
> Hello,
> 
> I would like to (re)submit a couple of patches to support V4L2 behavior at
> the V4L2 device nodes of the omap3-isp driver, but I'm guessing they require
> some discussion first.

Indeed.

The main reason why the OMAP3 ISP driver implements G_FMT/S_FMT as it does 
today is to hack around a restriction in the V4L2 API. We needed a way to 
preallocate and possibly prequeue buffers for snapshot, which wasn't possible 
in a standard-compliant way back then.

The situation has since changed, and we now have the VIDIOC_CREATE_BUFS and 
VIDIOC_PREPARE_BUF ioctls. My plan is to

- port the OMAP3 ISP driver to videobuf2
- implement support for CREATE_BUFS and PREPARE_BUF
- fix the G_FMT/S_FMT/ENUM_FMT behaviour

The real problem will be to find time to implement this.

> I've previously submitted one of them here [1] to support ENUM_FMT for the
> omap3-isp. This sparked some discussion, the result of which was that my
> patch probably made sense. Later [2], Laurent mentioned that there was some
> discussion/decision about adding "profiles" to the V4L2 specification, and
> the OMAP3 ISP would probably implement the "streaming" profile. That was 7
> months ago and I haven't seen any discussion of such profiles. Can somebody
> bring me up to speed on this?
> 
> The purpose of these two patches is for the V4L2 device nodes to support
> mandatory V4L2 ioctls G_FMT and ENUM_FMT, such that a pure V4L2 application,
> ignorant of the media controller, can still stream the images from the video
> nodes. This presumes that the media controller would have been
> pre-configured. This approach is often seen on the mailing list using
> 'media-ctl' to configure the ISP, then 'yavta' to retrieve images. Currently
> this works because yavta doesn't require ENUM_FMT (unlike Gstreamer), and
> only as long as one sets the same format with yavta as had already been set
> with media-ctl. I think yavta should be able to just do G_FMT to see what is
> configured.
> 
> To be clear (as discussed in [1]), ENUM_FMT does not behave according to its
> original intent, because it cannot enumerate possible formats the ISP can
> deliver. It will enumerate only one format: the one configured with the
> media controller. In a sense this complies with the specification, because
> S_FMT wouldn't be able to change the format to anything else.
> 
> I have tested these patches on v3.4, but I have rebased them to v3.5 here.
> I would remove the pr_debug() calls before submitting upstream, but they're
> useful for testing.
> 
> [1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg29640.html
> [2] http://www.mail-archive.com/linux-media@vger.kernel.org/msg40618.html
> 
> Michael Jones (2):
>   [media] omap3isp: implement ENUM_FMT
>   [media] omap3isp: support G_FMT
> 
>  drivers/media/video/omap3isp/ispvideo.c |   50 ++++++++++++++++++++++++++++
>  1 files changed, 50 insertions(+), 0 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [RFC] omap3-isp G_FMT & ENUM_FMT

[Michael Jones]

Hi Laurent,

Thanks for the reply.

On 07/26/2012 04:05 PM, Laurent Pinchart wrote:
> Hi Michael,
>
> On Thursday 26 July 2012 13:59:54 Michael Jones wrote:
>> Hello,
>>
>> I would like to (re)submit a couple of patches to support V4L2 behavior at
>> the V4L2 device nodes of the omap3-isp driver, but I'm guessing they require
>> some discussion first.
>
> Indeed.
>
> The main reason why the OMAP3 ISP driver implements G_FMT/S_FMT as it does
> today is to hack around a restriction in the V4L2 API. We needed a way to
> preallocate and possibly prequeue buffers for snapshot, which wasn't possible
> in a standard-compliant way back then.
>
> The situation has since changed, and we now have the VIDIOC_CREATE_BUFS and
> VIDIOC_PREPARE_BUF ioctls. My plan is to
>
> - port the OMAP3 ISP driver to videobuf2
> - implement support for CREATE_BUFS and PREPARE_BUF
> - fix the G_FMT/S_FMT/ENUM_FMT behaviour

What will the G_FMT/S_FMT/ENUM_FMT behavior be then?  Can you contrast 
it with the behavior of my patches?  If the behavior will be the same 
for user space, and your proposed changes won't be in very soon, can we 
use my patches until you make your changes?

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier

Re: [RFC] omap3-isp G_FMT & ENUM_FMT

[Laurent Pinchart]

Hi Michael,

On Thursday 26 July 2012 16:22:17 Michael Jones wrote:
> On 07/26/2012 04:05 PM, Laurent Pinchart wrote:
> > On Thursday 26 July 2012 13:59:54 Michael Jones wrote:
> >> Hello,
> >> 
> >> I would like to (re)submit a couple of patches to support V4L2 behavior
> >> at the V4L2 device nodes of the omap3-isp driver, but I'm guessing they
> >> require some discussion first.
> > 
> > Indeed.
> > 
> > The main reason why the OMAP3 ISP driver implements G_FMT/S_FMT as it does
> > today is to hack around a restriction in the V4L2 API. We needed a way to
> > preallocate and possibly prequeue buffers for snapshot, which wasn't
> > possible in a standard-compliant way back then.
> > 
> > The situation has since changed, and we now have the VIDIOC_CREATE_BUFS
> > and VIDIOC_PREPARE_BUF ioctls. My plan is to
> > 
> > - port the OMAP3 ISP driver to videobuf2
> > - implement support for CREATE_BUFS and PREPARE_BUF
> > - fix the G_FMT/S_FMT/ENUM_FMT behaviour
> 
> What will the G_FMT/S_FMT/ENUM_FMT behavior be then?  Can you contrast
> it with the behavior of my patches?  If the behavior will be the same
> for user space, and your proposed changes won't be in very soon, can we
> use my patches until you make your changes?

At the moment the driver accepts any format you give it in a S_FMT call, 
regardless of the format of the connected pad. The reason for that is to allow 
VIDIOC_REQBUFS to allocate buffers for an arbitrary size.

With CREATE_BUFS and PREPARE_BUFS support, G_FMT, S_FMT and ENUM_FMT should 
return the format of the connected pad. 

-- 
Regards,

Laurent Pinchart

Re: [RFC] omap3-isp G_FMT & ENUM_FMT

[Michael Jones]

Hi Laurent,

On 07/27/2012 01:32 AM, Laurent Pinchart wrote:
> Hi Michael,
>
> On Thursday 26 July 2012 16:22:17 Michael Jones wrote:
>> On 07/26/2012 04:05 PM, Laurent Pinchart wrote:
>>> On Thursday 26 July 2012 13:59:54 Michael Jones wrote:
>>>> Hello,
>>>>
>>>> I would like to (re)submit a couple of patches to support V4L2 behavior
>>>> at the V4L2 device nodes of the omap3-isp driver, but I'm guessing they
>>>> require some discussion first.
>>>
>>> Indeed.
>>>
>>> The main reason why the OMAP3 ISP driver implements G_FMT/S_FMT as it does
>>> today is to hack around a restriction in the V4L2 API. We needed a way to
>>> preallocate and possibly prequeue buffers for snapshot, which wasn't
>>> possible in a standard-compliant way back then.
>>>
>>> The situation has since changed, and we now have the VIDIOC_CREATE_BUFS
>>> and VIDIOC_PREPARE_BUF ioctls. My plan is to
>>>
>>> - port the OMAP3 ISP driver to videobuf2
>>> - implement support for CREATE_BUFS and PREPARE_BUF
>>> - fix the G_FMT/S_FMT/ENUM_FMT behaviour
>>
>> What will the G_FMT/S_FMT/ENUM_FMT behavior be then?  Can you contrast
>> it with the behavior of my patches?  If the behavior will be the same
>> for user space, and your proposed changes won't be in very soon, can we
>> use my patches until you make your changes?
>
> At the moment the driver accepts any format you give it in a S_FMT call,
> regardless of the format of the connected pad. The reason for that is to allow
> VIDIOC_REQBUFS to allocate buffers for an arbitrary size.
>
> With CREATE_BUFS and PREPARE_BUFS support, G_FMT, S_FMT and ENUM_FMT should
> return the format of the connected pad.
>

OK, so this sounds like the same behavior I'd like to add before 
CREATE_BUFS and PREPARE_BUFS support is in.  My other question was if 
this is the case, can we use my approach until your planned changes are in?

-Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier

Re: [RFC] omap3-isp G_FMT & ENUM_FMT

[Laurent Pinchart]

On Friday 27 July 2012 11:07:50 Michael Jones wrote:
> Hi Laurent,
> 
> On 07/27/2012 01:32 AM, Laurent Pinchart wrote:
> > Hi Michael,
> > 
> > On Thursday 26 July 2012 16:22:17 Michael Jones wrote:
> >> On 07/26/2012 04:05 PM, Laurent Pinchart wrote:
> >>> On Thursday 26 July 2012 13:59:54 Michael Jones wrote:
> >>>> Hello,
> >>>> 
> >>>> I would like to (re)submit a couple of patches to support V4L2 behavior
> >>>> at the V4L2 device nodes of the omap3-isp driver, but I'm guessing they
> >>>> require some discussion first.
> >>> 
> >>> Indeed.
> >>> 
> >>> The main reason why the OMAP3 ISP driver implements G_FMT/S_FMT as it
> >>> does today is to hack around a restriction in the V4L2 API. We needed a
> >>> way to preallocate and possibly prequeue buffers for snapshot, which
> >>> wasn't possible in a standard-compliant way back then.
> >>> 
> >>> The situation has since changed, and we now have the VIDIOC_CREATE_BUFS
> >>> and VIDIOC_PREPARE_BUF ioctls. My plan is to
> >>> 
> >>> - port the OMAP3 ISP driver to videobuf2
> >>> - implement support for CREATE_BUFS and PREPARE_BUF
> >>> - fix the G_FMT/S_FMT/ENUM_FMT behaviour
> >> 
> >> What will the G_FMT/S_FMT/ENUM_FMT behavior be then?  Can you contrast
> >> it with the behavior of my patches?  If the behavior will be the same
> >> for user space, and your proposed changes won't be in very soon, can we
> >> use my patches until you make your changes?
> > 
> > At the moment the driver accepts any format you give it in a S_FMT call,
> > regardless of the format of the connected pad. The reason for that is to
> > allow VIDIOC_REQBUFS to allocate buffers for an arbitrary size.
> > 
> > With CREATE_BUFS and PREPARE_BUFS support, G_FMT, S_FMT and ENUM_FMT
> > should return the format of the connected pad.
> 
> OK, so this sounds like the same behavior I'd like to add before
> CREATE_BUFS and PREPARE_BUFS support is in.  My other question was if
> this is the case, can we use my approach until your planned changes are in?

We can't, as it would break the use case of preallocating buffers without 
providing any alternative solution. That's why I haven't fixed the 
G_FMT/S_FMT/ENUM_FMT issue yet.

-- 
Regards,

Laurent Pinchart

Re: [RFC] omap3-isp G_FMT & ENUM_FMT

[Michael Jones]

Hi Laurent,

On 07/27/2012 11:35 AM, Laurent Pinchart wrote:
> On Friday 27 July 2012 11:07:50 Michael Jones wrote:
>> Hi Laurent,
>>
>> On 07/27/2012 01:32 AM, Laurent Pinchart wrote:
>>> Hi Michael,
[snip]
>> OK, so this sounds like the same behavior I'd like to add before
>> CREATE_BUFS and PREPARE_BUFS support is in.  My other question was if
>> this is the case, can we use my approach until your planned changes are in?
>
> We can't, as it would break the use case of preallocating buffers without
> providing any alternative solution. That's why I haven't fixed the
> G_FMT/S_FMT/ENUM_FMT issue yet.
>

OK, now I understand.  Thanks for clarifying.

-Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier