[media-ctl PATCH 1/1] libv4l2subdev: Add v4l2_subdev_enum_mbus_code()

[media-ctl PATCH 1/1] libv4l2subdev: Add v4l2_subdev_enum_mbus_code()

[Sakari Ailus]

v4l2_subdev_enum_mbus_code() enumerates over supported media bus formats on
a pad.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 src/v4l2subdev.c |   23 +++++++++++++++++++++++
 src/v4l2subdev.h |   14 ++++++++++++++
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
index d60bd7e..6b6df0a 100644
--- a/src/v4l2subdev.c
+++ b/src/v4l2subdev.c
@@ -58,6 +58,29 @@ void v4l2_subdev_close(struct media_entity *entity)
 	entity->fd = -1;
 }
 
+int v4l2_subdev_enum_mbus_code(struct media_entity *entity,
+			       uint32_t *code, uint32_t pad, uint32_t index)
+{
+	struct v4l2_subdev_mbus_code_enum c;
+	int ret;
+
+	ret = v4l2_subdev_open(entity);
+	if (ret < 0)
+		return ret;
+
+	memset(&c, 0, sizeof(c));
+	c.pad = pad;
+	c.index = index;
+
+	ret = ioctl(entity->fd, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &c);
+	if (ret < 0)
+		return -errno;
+
+	*code = c.code;
+
+	return 0;
+}
+
 int v4l2_subdev_get_format(struct media_entity *entity,
 	struct v4l2_mbus_framefmt *format, unsigned int pad,
 	enum v4l2_subdev_format_whence which)
diff --git a/src/v4l2subdev.h b/src/v4l2subdev.h
index 5d55482..1cca7b9 100644
--- a/src/v4l2subdev.h
+++ b/src/v4l2subdev.h
@@ -22,6 +22,7 @@
 #ifndef __SUBDEV_H__
 #define __SUBDEV_H__
 
+#include <stdint.h>
 #include <linux/v4l2-subdev.h>
 
 struct media_entity;
@@ -47,6 +48,19 @@ int v4l2_subdev_open(struct media_entity *entity);
 void v4l2_subdev_close(struct media_entity *entity);
 
 /**
+ * @brief Enumerate mbus pixel codes.
+ * @param entity - subdev-device media entity.
+ * @param code - mbus pixel code
+ *
+ * Enumerate media bus pixel codes. This is just a wrapper for
+ * VIDIOC_SUBDEV_ENUM_MBUS_CODE IOCTL.
+ *
+ * @return 0 on success, or a negative error code on failure.
+ */
+int v4l2_subdev_enum_mbus_code(struct media_entity *entity,
+			       uint32_t *code, uint32_t pad, uint32_t index);
+
+/**
  * @brief Retrieve the format on a pad.
  * @param entity - subdev-device media entity.
  * @param format - format to be filled.
-- 
1.7.2.5

Re: [media-ctl PATCH 1/1] libv4l2subdev: Add v4l2_subdev_enum_mbus_code()

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Tuesday 31 July 2012 01:16:00 Sakari Ailus wrote:
> v4l2_subdev_enum_mbus_code() enumerates over supported media bus formats on
> a pad.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  src/v4l2subdev.c |   23 +++++++++++++++++++++++
>  src/v4l2subdev.h |   14 ++++++++++++++
>  2 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
> index d60bd7e..6b6df0a 100644
> --- a/src/v4l2subdev.c
> +++ b/src/v4l2subdev.c
> @@ -58,6 +58,29 @@ void v4l2_subdev_close(struct media_entity *entity)
>  	entity->fd = -1;
>  }
> 
> +int v4l2_subdev_enum_mbus_code(struct media_entity *entity,
> +			       uint32_t *code, uint32_t pad, uint32_t index)

I would use unsigned int for the pad and index arguments to match the other 
functions. We could then fix all of them in one go to use stdint types to 
match the kernel API types.

> +{
> +	struct v4l2_subdev_mbus_code_enum c;
> +	int ret;
> +
> +	ret = v4l2_subdev_open(entity);
> +	if (ret < 0)
> +		return ret;
> +
> +	memset(&c, 0, sizeof(c));
> +	c.pad = pad;
> +	c.index = index;
> +
> +	ret = ioctl(entity->fd, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &c);
> +	if (ret < 0)
> +		return -errno;
> +
> +	*code = c.code;
> +
> +	return 0;
> +}

What about a higher-level API that would enumerate all formats and return a 
list/array ?

> +
>  int v4l2_subdev_get_format(struct media_entity *entity,
>  	struct v4l2_mbus_framefmt *format, unsigned int pad,
>  	enum v4l2_subdev_format_whence which)
> diff --git a/src/v4l2subdev.h b/src/v4l2subdev.h
> index 5d55482..1cca7b9 100644
> --- a/src/v4l2subdev.h
> +++ b/src/v4l2subdev.h
> @@ -22,6 +22,7 @@
>  #ifndef __SUBDEV_H__
>  #define __SUBDEV_H__
> 
> +#include <stdint.h>
>  #include <linux/v4l2-subdev.h>
> 
>  struct media_entity;
> @@ -47,6 +48,19 @@ int v4l2_subdev_open(struct media_entity *entity);
>  void v4l2_subdev_close(struct media_entity *entity);
> 
>  /**
> + * @brief Enumerate mbus pixel codes.
> + * @param entity - subdev-device media entity.
> + * @param code - mbus pixel code
> + *
> + * Enumerate media bus pixel codes. This is just a wrapper for
> + * VIDIOC_SUBDEV_ENUM_MBUS_CODE IOCTL.
> + *
> + * @return 0 on success, or a negative error code on failure.
> + */
> +int v4l2_subdev_enum_mbus_code(struct media_entity *entity,
> +			       uint32_t *code, uint32_t pad, uint32_t index);
> +
> +/**
>   * @brief Retrieve the format on a pad.
>   * @param entity - subdev-device media entity.
>   * @param format - format to be filled.

-- 
Regards,

Laurent Pinchart

Re: [media-ctl PATCH 1/1] libv4l2subdev: Add v4l2_subdev_enum_mbus_code()

[Sakari Ailus]

Hi Laurent,

On Tue, Jul 31, 2012 at 01:38:41PM +0200, Laurent Pinchart wrote:
> Thanks for the patch.

Thanks for the comments!

> On Tuesday 31 July 2012 01:16:00 Sakari Ailus wrote:
> > v4l2_subdev_enum_mbus_code() enumerates over supported media bus formats on
> > a pad.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> >  src/v4l2subdev.c |   23 +++++++++++++++++++++++
> >  src/v4l2subdev.h |   14 ++++++++++++++
> >  2 files changed, 37 insertions(+), 0 deletions(-)
> > 
> > diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
> > index d60bd7e..6b6df0a 100644
> > --- a/src/v4l2subdev.c
> > +++ b/src/v4l2subdev.c
> > @@ -58,6 +58,29 @@ void v4l2_subdev_close(struct media_entity *entity)
> >  	entity->fd = -1;
> >  }
> > 
> > +int v4l2_subdev_enum_mbus_code(struct media_entity *entity,
> > +			       uint32_t *code, uint32_t pad, uint32_t index)
> 
> I would use unsigned int for the pad and index arguments to match the other 
> functions. We could then fix all of them in one go to use stdint types to 
> match the kernel API types.

I'm fine with that.

> > +{
> > +	struct v4l2_subdev_mbus_code_enum c;
> > +	int ret;
> > +
> > +	ret = v4l2_subdev_open(entity);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	memset(&c, 0, sizeof(c));
> > +	c.pad = pad;
> > +	c.index = index;
> > +
> > +	ret = ioctl(entity->fd, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &c);
> > +	if (ret < 0)
> > +		return -errno;
> > +
> > +	*code = c.code;
> > +
> > +	return 0;
> > +}
> 
> What about a higher-level API that would enumerate all formats and return a 
> list/array ?

The information could be stored to media entities. We could add a V4L2
subdev pointer to media entities, and have the information stored there the
first time this function is called. How about that?

On source pads the pixel code is obviously possibly dependent on the pixel
code on the sink pad so I need to store mappings from sink pad pixel code to
a list of source pad pixel code, but can it have other dependencies? None
come to mind right now, though.

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [media-ctl PATCH 1/1] libv4l2subdev: Add v4l2_subdev_enum_mbus_code()

[Laurent Pinchart]

Hi Sakari,

On Tuesday 31 July 2012 15:17:04 Sakari Ailus wrote:
> On Tue, Jul 31, 2012 at 01:38:41PM +0200, Laurent Pinchart wrote:
> > On Tuesday 31 July 2012 01:16:00 Sakari Ailus wrote:
> > > v4l2_subdev_enum_mbus_code() enumerates over supported media bus formats
> > > on a pad.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > ---
> > > 
> > >  src/v4l2subdev.c |   23 +++++++++++++++++++++++
> > >  src/v4l2subdev.h |   14 ++++++++++++++
> > >  2 files changed, 37 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
> > > index d60bd7e..6b6df0a 100644
> > > --- a/src/v4l2subdev.c
> > > +++ b/src/v4l2subdev.c
> > > @@ -58,6 +58,29 @@ void v4l2_subdev_close(struct media_entity *entity)
> > > 
> > >  	entity->fd = -1;
> > >  
> > >  }
> > > 
> > > +int v4l2_subdev_enum_mbus_code(struct media_entity *entity,
> > > +			       uint32_t *code, uint32_t pad, uint32_t index)
> > 
> > I would use unsigned int for the pad and index arguments to match the
> > other functions. We could then fix all of them in one go to use stdint
> > types to match the kernel API types.
> 
> I'm fine with that.
> 
> > > +{
> > > +	struct v4l2_subdev_mbus_code_enum c;
> > > +	int ret;
> > > +
> > > +	ret = v4l2_subdev_open(entity);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	memset(&c, 0, sizeof(c));
> > > +	c.pad = pad;
> > > +	c.index = index;
> > > +
> > > +	ret = ioctl(entity->fd, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &c);
> > > +	if (ret < 0)
> > > +		return -errno;
> > > +
> > > +	*code = c.code;
> > > +
> > > +	return 0;
> > > +}
> > 
> > What about a higher-level API that would enumerate all formats and return
> > a list/array ?
> 
> The information could be stored to media entities. We could add a V4L2
> subdev pointer to media entities, and have the information stored there the
> first time this function is called. How about that?
> 
> On source pads the pixel code is obviously possibly dependent on the pixel
> code on the sink pad so I need to store mappings from sink pad pixel code to
> a list of source pad pixel code, but can it have other dependencies? None
> come to mind right now, though.

I would make this function store the enumerated mbus codes in the media 
entity, as the codes on a source pad could depend on the selected code on a 
sink pad. I would instead make the function allocate an array, fill it with 
media bus codes and return it. The caller would be responsible for freeing it 
(possibly later, after storing it).

-- 
Regards,

Laurent Pinchart

Re: [media-ctl PATCH 1/1] libv4l2subdev: Add v4l2_subdev_enum_mbus_code()

[Sakari Ailus]

Hi Laurent,

On Tue, Jul 31, 2012 at 11:50:53PM +0200, Laurent Pinchart wrote:
> On Tuesday 31 July 2012 15:17:04 Sakari Ailus wrote:
> > On Tue, Jul 31, 2012 at 01:38:41PM +0200, Laurent Pinchart wrote:
> > > On Tuesday 31 July 2012 01:16:00 Sakari Ailus wrote:
> > > > v4l2_subdev_enum_mbus_code() enumerates over supported media bus formats
> > > > on a pad.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > ---
> > > > 
> > > >  src/v4l2subdev.c |   23 +++++++++++++++++++++++
> > > >  src/v4l2subdev.h |   14 ++++++++++++++
> > > >  2 files changed, 37 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
> > > > index d60bd7e..6b6df0a 100644
> > > > --- a/src/v4l2subdev.c
> > > > +++ b/src/v4l2subdev.c
> > > > @@ -58,6 +58,29 @@ void v4l2_subdev_close(struct media_entity *entity)
> > > > 
> > > >  	entity->fd = -1;
> > > >  
> > > >  }
> > > > 
> > > > +int v4l2_subdev_enum_mbus_code(struct media_entity *entity,
> > > > +			       uint32_t *code, uint32_t pad, uint32_t index)
> > > 
> > > I would use unsigned int for the pad and index arguments to match the
> > > other functions. We could then fix all of them in one go to use stdint
> > > types to match the kernel API types.
> > 
> > I'm fine with that.
> > 
> > > > +{
> > > > +	struct v4l2_subdev_mbus_code_enum c;
> > > > +	int ret;
> > > > +
> > > > +	ret = v4l2_subdev_open(entity);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	memset(&c, 0, sizeof(c));
> > > > +	c.pad = pad;
> > > > +	c.index = index;
> > > > +
> > > > +	ret = ioctl(entity->fd, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &c);
> > > > +	if (ret < 0)
> > > > +		return -errno;
> > > > +
> > > > +	*code = c.code;
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > What about a higher-level API that would enumerate all formats and return
> > > a list/array ?
> > 
> > The information could be stored to media entities. We could add a V4L2
> > subdev pointer to media entities, and have the information stored there the
> > first time this function is called. How about that?
> > 
> > On source pads the pixel code is obviously possibly dependent on the pixel
> > code on the sink pad so I need to store mappings from sink pad pixel code to
> > a list of source pad pixel code, but can it have other dependencies? None
> > come to mind right now, though.
> 
> I would make this function store the enumerated mbus codes in the media 
> entity, as the codes on a source pad could depend on the selected code on a 
> sink pad. I would instead make the function allocate an array, fill it with 

(After a short discussion it has been confirmed that " not" should be added
somewhere to the above sentence.)

> media bus codes and return it. The caller would be responsible for freeing it 
> (possibly later, after storing it).

I like this option better as it forces the user to make a decision how long
to keep the enumeration around. Changing the sink pad's try format is one
thing that may invalidate the enumeration on a source pad, but that doesn't
rule out other reasons since they may be hardware dependent.

So I'll change it so.

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk