[PATCH] V4L: add convenience macros to the subdevice / Media Controller API

[PATCH] V4L: add convenience macros to the subdevice / Media Controller API

[Guennadi Liakhovetski]

Drivers, that can be built and work with and without
CONFIG_VIDEO_V4L2_SUBDEV_API, need the v4l2_subdev_get_try_format() and
v4l2_subdev_get_try_crop() functions, even though their return value
should never be dereferenced. Also add convenience macros to init and
clean up subdevice internal media entities.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 include/media/v4l2-subdev.h |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index f0f3358..4670506 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -569,6 +569,9 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev_fh *fh, unsigned int pad)
 {
 	return &fh->try_crop[pad];
 }
+#else
+#define v4l2_subdev_get_try_format(arg...)	NULL
+#define v4l2_subdev_get_try_crop(arg...)	NULL
 #endif
 
 extern const struct v4l2_file_operations v4l2_subdev_fops;
@@ -610,4 +613,12 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
 	((!(sd) || !(sd)->v4l2_dev || !(sd)->v4l2_dev->notify) ? -ENODEV : \
 	 (sd)->v4l2_dev->notify((sd), (notification), (arg)))
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+#define subdev_media_entity_init(sd, n, p, e)	media_entity_init(&(sd)->entity, n, p, e)
+#define subdev_media_entity_cleanup(sd)		media_entity_cleanup(&(sd)->entity)
+#else
+#define subdev_media_entity_init(sd, n, p, e)	0
+#define subdev_media_entity_cleanup(sd)		do {} while (0)
+#endif
+
 #endif
-- 
1.7.2.5

Re: [PATCH] V4L: add convenience macros to the subdevice / Media Controller API

[Laurent Pinchart]

Hi Guennadi,

Thanks for the patch.

On Thursday 29 September 2011 10:18:31 Guennadi Liakhovetski wrote:
> Drivers, that can be built and work with and without
> CONFIG_VIDEO_V4L2_SUBDEV_API, need the v4l2_subdev_get_try_format() and
> v4l2_subdev_get_try_crop() functions, even though their return value
> should never be dereferenced. Also add convenience macros to init and
> clean up subdevice internal media entities.

Why don't you just make the drivers depend on CONFIG_VIDEO_V4L2_SUBDEV_API ? 
They don't need to actually export a device node to userspace, but they 
require the in-kernel API.

> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  include/media/v4l2-subdev.h |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index f0f3358..4670506 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -569,6 +569,9 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev_fh *fh,
> unsigned int pad) {
>  	return &fh->try_crop[pad];
>  }
> +#else
> +#define v4l2_subdev_get_try_format(arg...)	NULL
> +#define v4l2_subdev_get_try_crop(arg...)	NULL
>  #endif
> 
>  extern const struct v4l2_file_operations v4l2_subdev_fops;
> @@ -610,4 +613,12 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
>  	((!(sd) || !(sd)->v4l2_dev || !(sd)->v4l2_dev->notify) ? -ENODEV : \
>  	 (sd)->v4l2_dev->notify((sd), (notification), (arg)))
> 
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +#define subdev_media_entity_init(sd, n, p,
> e)	media_entity_init(&(sd)->entity, n, p, e) +#define
> subdev_media_entity_cleanup(sd)		media_entity_cleanup(&(sd)->entity)
> +#else
> +#define subdev_media_entity_init(sd, n, p, e)	0
> +#define subdev_media_entity_cleanup(sd)		do {} while (0)
> +#endif
> +
>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH] V4L: add convenience macros to the subdevice / Media Controller API

[Guennadi Liakhovetski]

On Thu, 29 Sep 2011, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thanks for the patch.
> 
> On Thursday 29 September 2011 10:18:31 Guennadi Liakhovetski wrote:
> > Drivers, that can be built and work with and without
> > CONFIG_VIDEO_V4L2_SUBDEV_API, need the v4l2_subdev_get_try_format() and
> > v4l2_subdev_get_try_crop() functions, even though their return value
> > should never be dereferenced. Also add convenience macros to init and
> > clean up subdevice internal media entities.
> 
> Why don't you just make the drivers depend on CONFIG_VIDEO_V4L2_SUBDEV_API ? 
> They don't need to actually export a device node to userspace, but they 
> require the in-kernel API.

Why? Why should the user build and load all the media controller stuff, 
buy all the in-kernel objects and code to never actually use it? Where 
OTOH all is needed to avoid that is a couple of NOP macros?

Thanks
Guennadi

> 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  include/media/v4l2-subdev.h |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index f0f3358..4670506 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -569,6 +569,9 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev_fh *fh,
> > unsigned int pad) {
> >  	return &fh->try_crop[pad];
> >  }
> > +#else
> > +#define v4l2_subdev_get_try_format(arg...)	NULL
> > +#define v4l2_subdev_get_try_crop(arg...)	NULL
> >  #endif
> > 
> >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> > @@ -610,4 +613,12 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
> >  	((!(sd) || !(sd)->v4l2_dev || !(sd)->v4l2_dev->notify) ? -ENODEV : \
> >  	 (sd)->v4l2_dev->notify((sd), (notification), (arg)))
> > 
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +#define subdev_media_entity_init(sd, n, p,
> > e)	media_entity_init(&(sd)->entity, n, p, e) +#define
> > subdev_media_entity_cleanup(sd)		media_entity_cleanup(&(sd)->entity)
> > +#else
> > +#define subdev_media_entity_init(sd, n, p, e)	0
> > +#define subdev_media_entity_cleanup(sd)		do {} while (0)
> > +#endif
> > +
> >  #endif
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] V4L: add convenience macros to the subdevice / Media Controller API

[Laurent Pinchart]

Hi Guennadi,

On Thursday 29 September 2011 10:44:14 Guennadi Liakhovetski wrote:
> On Thu, 29 Sep 2011, Laurent Pinchart wrote:
> > On Thursday 29 September 2011 10:18:31 Guennadi Liakhovetski wrote:
> > > Drivers, that can be built and work with and without
> > > CONFIG_VIDEO_V4L2_SUBDEV_API, need the v4l2_subdev_get_try_format() and
> > > v4l2_subdev_get_try_crop() functions, even though their return value
> > > should never be dereferenced. Also add convenience macros to init and
> > > clean up subdevice internal media entities.
> > 
> > Why don't you just make the drivers depend on
> > CONFIG_VIDEO_V4L2_SUBDEV_API ? They don't need to actually export a
> > device node to userspace, but they require the in-kernel API.
> 
> Why? Why should the user build and load all the media controller stuff,
> buy all the in-kernel objects and code to never actually use it? Where
> OTOH all is needed to avoid that is a couple of NOP macros?

Because the automatic compatibility layer that will translate video operations 
to pad operations will need to access pads, so subdevs that implement a pad-
level API need to export it to the bridge, even if the bridge is not MC-aware.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] V4L: add convenience macros to the subdevice / Media Controller API

[Guennadi Liakhovetski]

Hi Laurent

On Thu, 29 Sep 2011, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Thursday 29 September 2011 10:44:14 Guennadi Liakhovetski wrote:
> > On Thu, 29 Sep 2011, Laurent Pinchart wrote:
> > > On Thursday 29 September 2011 10:18:31 Guennadi Liakhovetski wrote:
> > > > Drivers, that can be built and work with and without
> > > > CONFIG_VIDEO_V4L2_SUBDEV_API, need the v4l2_subdev_get_try_format() and
> > > > v4l2_subdev_get_try_crop() functions, even though their return value
> > > > should never be dereferenced. Also add convenience macros to init and
> > > > clean up subdevice internal media entities.
> > > 
> > > Why don't you just make the drivers depend on
> > > CONFIG_VIDEO_V4L2_SUBDEV_API ? They don't need to actually export a
> > > device node to userspace, but they require the in-kernel API.
> > 
> > Why? Why should the user build and load all the media controller stuff,
> > buy all the in-kernel objects and code to never actually use it? Where
> > OTOH all is needed to avoid that is a couple of NOP macros?
> 
> Because the automatic compatibility layer that will translate video operations 
> to pad operations will need to access pads, so subdevs that implement a pad-
> level API need to export it to the bridge, even if the bridge is not MC-aware.

I might be missing something, but it seems to me, that if 
CONFIG_VIDEO_V4L2_SUBDEV_API is not defined, no pads are exported to the 
user space (and you mean a compatibility layer in the user space, don't 
you?), so, noone will be able to accesss them.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] V4L: add convenience macros to the subdevice / Media Controller API

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 06 December 2011 09:40:41 Guennadi Liakhovetski wrote:
> On Thu, 29 Sep 2011, Laurent Pinchart wrote:
> > On Thursday 29 September 2011 10:44:14 Guennadi Liakhovetski wrote:
> > > On Thu, 29 Sep 2011, Laurent Pinchart wrote:
> > > > On Thursday 29 September 2011 10:18:31 Guennadi Liakhovetski wrote:
> > > > > Drivers, that can be built and work with and without
> > > > > CONFIG_VIDEO_V4L2_SUBDEV_API, need the v4l2_subdev_get_try_format()
> > > > > and v4l2_subdev_get_try_crop() functions, even though their return
> > > > > value should never be dereferenced. Also add convenience macros to
> > > > > init and clean up subdevice internal media entities.
> > > > 
> > > > Why don't you just make the drivers depend on
> > > > CONFIG_VIDEO_V4L2_SUBDEV_API ? They don't need to actually export a
> > > > device node to userspace, but they require the in-kernel API.
> > > 
> > > Why? Why should the user build and load all the media controller stuff,
> > > buy all the in-kernel objects and code to never actually use it? Where
> > > OTOH all is needed to avoid that is a couple of NOP macros?
> > 
> > Because the automatic compatibility layer that will translate video
> > operations to pad operations will need to access pads, so subdevs that
> > implement a pad- level API need to export it to the bridge, even if the
> > bridge is not MC-aware.
> 
> I might be missing something, but it seems to me, that if
> CONFIG_VIDEO_V4L2_SUBDEV_API is not defined, no pads are exported to the
> user space (and you mean a compatibility layer in the user space, don't
> you?), so, noone will be able to accesss them.

No, I meant a compatibility layer in kernel space. Basically something like 
(totally untested)

int v4l2_subdev_get_mbus_format(struct v4l2_subdev *sd, struct 
v4l2_mbus_framefmt *format)
{
	struct v4l2_subdev_format fmt;
	int ret;

	ret = v4l2_subdev_call(sd, video, g_mbus_fmt, format);
	if (ret != ENOIOCTLCMD)
		return ret;

	fmt.pad = 0;
	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
	fmt.format = *format;

	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
	if (ret < 0 && ret != ENOIOCTLCMD)
		*format = fmt.format;

	return ret;
}

Or the other way around, trying pad::get_fmt before video::g_mbus_fmt.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] V4L: add convenience macros to the subdevice / Media Controller API

[Guennadi Liakhovetski]

On Tue, 6 Dec 2011, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday 06 December 2011 09:40:41 Guennadi Liakhovetski wrote:
> > On Thu, 29 Sep 2011, Laurent Pinchart wrote:
> > > On Thursday 29 September 2011 10:44:14 Guennadi Liakhovetski wrote:
> > > > On Thu, 29 Sep 2011, Laurent Pinchart wrote:
> > > > > On Thursday 29 September 2011 10:18:31 Guennadi Liakhovetski wrote:
> > > > > > Drivers, that can be built and work with and without
> > > > > > CONFIG_VIDEO_V4L2_SUBDEV_API, need the v4l2_subdev_get_try_format()
> > > > > > and v4l2_subdev_get_try_crop() functions, even though their return
> > > > > > value should never be dereferenced. Also add convenience macros to
> > > > > > init and clean up subdevice internal media entities.
> > > > > 
> > > > > Why don't you just make the drivers depend on
> > > > > CONFIG_VIDEO_V4L2_SUBDEV_API ? They don't need to actually export a
> > > > > device node to userspace, but they require the in-kernel API.
> > > > 
> > > > Why? Why should the user build and load all the media controller stuff,
> > > > buy all the in-kernel objects and code to never actually use it? Where
> > > > OTOH all is needed to avoid that is a couple of NOP macros?
> > > 
> > > Because the automatic compatibility layer that will translate video
> > > operations to pad operations will need to access pads, so subdevs that
> > > implement a pad- level API need to export it to the bridge, even if the
> > > bridge is not MC-aware.
> > 
> > I might be missing something, but it seems to me, that if
> > CONFIG_VIDEO_V4L2_SUBDEV_API is not defined, no pads are exported to the
> > user space (and you mean a compatibility layer in the user space, don't
> > you?), so, noone will be able to accesss them.
> 
> No, I meant a compatibility layer in kernel space. Basically something like 
> (totally untested)

Aha, so, a "future" layer.

> int v4l2_subdev_get_mbus_format(struct v4l2_subdev *sd, struct 
> v4l2_mbus_framefmt *format)
> {
> 	struct v4l2_subdev_format fmt;
> 	int ret;
> 
> 	ret = v4l2_subdev_call(sd, video, g_mbus_fmt, format);
> 	if (ret != ENOIOCTLCMD)

you mean "-E..."

> 		return ret;
> 
> 	fmt.pad = 0;
> 	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> 	fmt.format = *format;
> 
> 	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
> 	if (ret < 0 && ret != ENOIOCTLCMD)

You, probably, actually mean "if (!ret)"

> 		*format = fmt.format;
> 
> 	return ret;
> }
> 
> Or the other way around, trying pad::get_fmt before video::g_mbus_fmt.

Ok, I understand what you mean now. Any idea when this layer is going to 
be implemented?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] V4L: add convenience macros to the subdevice / Media Controller API

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 06 December 2011 13:04:00 Guennadi Liakhovetski wrote:
> On Tue, 6 Dec 2011, Laurent Pinchart wrote:
> > On Tuesday 06 December 2011 09:40:41 Guennadi Liakhovetski wrote:
> > > On Thu, 29 Sep 2011, Laurent Pinchart wrote:
> > > > On Thursday 29 September 2011 10:44:14 Guennadi Liakhovetski wrote:
> > > > > On Thu, 29 Sep 2011, Laurent Pinchart wrote:
> > > > > > On Thursday 29 September 2011 10:18:31 Guennadi Liakhovetski 
wrote:
> > > > > > > Drivers, that can be built and work with and without
> > > > > > > CONFIG_VIDEO_V4L2_SUBDEV_API, need the
> > > > > > > v4l2_subdev_get_try_format() and v4l2_subdev_get_try_crop()
> > > > > > > functions, even though their return value should never be
> > > > > > > dereferenced. Also add convenience macros to init and clean up
> > > > > > > subdevice internal media entities.
> > > > > > 
> > > > > > Why don't you just make the drivers depend on
> > > > > > CONFIG_VIDEO_V4L2_SUBDEV_API ? They don't need to actually export
> > > > > > a device node to userspace, but they require the in-kernel API.
> > > > > 
> > > > > Why? Why should the user build and load all the media controller
> > > > > stuff, buy all the in-kernel objects and code to never actually
> > > > > use it? Where OTOH all is needed to avoid that is a couple of NOP
> > > > > macros?
> > > > 
> > > > Because the automatic compatibility layer that will translate video
> > > > operations to pad operations will need to access pads, so subdevs
> > > > that implement a pad- level API need to export it to the bridge,
> > > > even if the bridge is not MC-aware.
> > > 
> > > I might be missing something, but it seems to me, that if
> > > CONFIG_VIDEO_V4L2_SUBDEV_API is not defined, no pads are exported to
> > > the user space (and you mean a compatibility layer in the user space,
> > > don't you?), so, noone will be able to accesss them.
> > 
> > No, I meant a compatibility layer in kernel space. Basically something
> > like (totally untested)
> 
> Aha, so, a "future" layer.
> 
> > int v4l2_subdev_get_mbus_format(struct v4l2_subdev *sd, struct
> > v4l2_mbus_framefmt *format)
> > {
> > 
> > 	struct v4l2_subdev_format fmt;
> > 	int ret;
> > 	
> > 	ret = v4l2_subdev_call(sd, video, g_mbus_fmt, format);
> > 	if (ret != ENOIOCTLCMD)
> 
> you mean "-E..."
> 
> > 		return ret;
> > 	
> > 	fmt.pad = 0;
> > 	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > 	fmt.format = *format;
> > 	
> > 	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
> > 	if (ret < 0 && ret != ENOIOCTLCMD)
> 
> You, probably, actually mean "if (!ret)"
> 
> > 		*format = fmt.format;
> > 	
> > 	return ret;
> > 
> > }
> > 
> > Or the other way around, trying pad::get_fmt before video::g_mbus_fmt.
> 
> Ok, I understand what you mean now. Any idea when this layer is going to
> be implemented?

As soon as someone needs it and works on it I guess :-) I can give it a try, 
but I'm pretty busy at the moment. Do you have driver(s) that would be good 
test candidates for that ?

-- 
Regards,

Laurent Pinchart