[PATCH 0/3] Media controller changes to support DVB

[PATCH 0/3] Media controller changes to support DVB

[Mauro Carvalho Chehab]

This patch series change the media controller API to allow adding
support for DVB media controller support.

I removed the actual implementation from this series, in order to
better identify the API bits required to add media controller support
to DVB. They'll be sent o a separate patch series, after we agree
with the API needs.

If this gets accepted, the other patches will be basically the ones
already sent at:
	https://www.mail-archive.com/linux-media@vger.kernel.org/msg83895.html

With one small change at the patch that adds media controller support
at dvbdev, replacing "info.dvb" by "info.dev", due to a change at the
media controller's representation for all devnodes.

As a reference, a typical analog/digital TV hardware looks like:
	http://linuxtv.org/downloads/presentations/typical_hybrid_hardware.png

And the media controller representation for it is:
	http://linuxtv.org/downloads/presentations/cx231xx.dot
	http://linuxtv.org/downloads/presentations/cx231xx.ps

The full patch series with the DVB controller implementation is at:
	http://git.linuxtv.org/cgit.cgi/mchehab/experimental.git/log/?h=dvb-media-ctl

Mauro Carvalho Chehab (3):
  media: Fix ALSA and DVB representation at media controller API
  media: add new types for DVB devnodes
  media: add a subdev type for tuner

 drivers/media/v4l2-core/v4l2-dev.c    |  4 ++--
 drivers/media/v4l2-core/v4l2-device.c |  4 ++--
 include/media/media-entity.h          | 12 +-----------
 include/uapi/linux/media.h            | 26 +++++++++++++++++++++++++-
 4 files changed, 30 insertions(+), 16 deletions(-)

-- 
2.1.0

[PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Mauro Carvalho Chehab]

The previous provision for DVB media controller support were to
define an ID (likely meaning the adapter number) for the DVB
devnodes.

This is just plain wrong. Just like V4L, DVB devices (and ALSA,
or whatever) are identified via a (major, minor) tuple.

This is enough to uniquely identify a devnode, no matter what
API it implements.

So, before we go too far, let's mark the old v4l, dvb and alsa
"devnode" info as deprecated, and just call it as "dev".

As we don't want to break compilation on already existing apps,
let's just keep the old definitions as-is, adding a note that
those are deprecated at media-entity.h.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 86bb93fd7db8..d89d5cb465d9 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -943,8 +943,8 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
 	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
 		vdev->entity.type = MEDIA_ENT_T_DEVNODE_V4L;
 		vdev->entity.name = vdev->name;
-		vdev->entity.info.v4l.major = VIDEO_MAJOR;
-		vdev->entity.info.v4l.minor = vdev->minor;
+		vdev->entity.info.dev.major = VIDEO_MAJOR;
+		vdev->entity.info.dev.minor = vdev->minor;
 		ret = media_device_register_entity(vdev->v4l2_dev->mdev,
 			&vdev->entity);
 		if (ret < 0)
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 015f92aab44a..204cc67c84e8 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -248,8 +248,8 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 			goto clean_up;
 		}
 #if defined(CONFIG_MEDIA_CONTROLLER)
-		sd->entity.info.v4l.major = VIDEO_MAJOR;
-		sd->entity.info.v4l.minor = vdev->minor;
+		sd->entity.info.dev.major = VIDEO_MAJOR;
+		sd->entity.info.dev.minor = vdev->minor;
 #endif
 		sd->devnode = vdev;
 	}
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index e00459185d20..d6d74bcfe183 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -87,17 +87,7 @@ struct media_entity {
 		struct {
 			u32 major;
 			u32 minor;
-		} v4l;
-		struct {
-			u32 major;
-			u32 minor;
-		} fb;
-		struct {
-			u32 card;
-			u32 device;
-			u32 subdevice;
-		} alsa;
-		int dvb;
+		} dev;
 
 		/* Sub-device specifications */
 		/* Nothing needed yet */
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index d847c760e8f0..418f4fec391a 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -78,6 +78,20 @@ struct media_entity_desc {
 		struct {
 			__u32 major;
 			__u32 minor;
+		} dev;
+
+#if 1
+		/*
+		 * DEPRECATED: previous node specifications. Kept just to
+		 * avoid breaking compilation, but media_entity_desc.dev
+		 * should be used instead. In particular, alsa and dvb
+		 * fields below are wrong: for all devnodes, there should
+		 * be just major/minor inside the struct, as this is enough
+		 * to represent any devnode, no matter what type.
+		 */
+		struct {
+			__u32 major;
+			__u32 minor;
 		} v4l;
 		struct {
 			__u32 major;
@@ -89,6 +103,7 @@ struct media_entity_desc {
 			__u32 subdevice;
 		} alsa;
 		int dvb;
+#endif
 
 		/* Sub-device specifications */
 		/* Nothing needed yet */
-- 
2.1.0

[PATCH 2/3] media: add new types for DVB devnodes

[Mauro Carvalho Chehab]

Most of the DVB subdevs have already their own devnode.

Add support for them at the media controller API.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 418f4fec391a..4c8f26243252 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -50,7 +50,14 @@ struct media_device_info {
 #define MEDIA_ENT_T_DEVNODE_V4L		(MEDIA_ENT_T_DEVNODE + 1)
 #define MEDIA_ENT_T_DEVNODE_FB		(MEDIA_ENT_T_DEVNODE + 2)
 #define MEDIA_ENT_T_DEVNODE_ALSA	(MEDIA_ENT_T_DEVNODE + 3)
-#define MEDIA_ENT_T_DEVNODE_DVB		(MEDIA_ENT_T_DEVNODE + 4)
+#define MEDIA_ENT_T_DEVNODE_DVB_FE	(MEDIA_ENT_T_DEVNODE + 4)
+#define MEDIA_ENT_T_DEVNODE_DVB_DEMUX	(MEDIA_ENT_T_DEVNODE + 5)
+#define MEDIA_ENT_T_DEVNODE_DVB_DVR	(MEDIA_ENT_T_DEVNODE + 6)
+#define MEDIA_ENT_T_DEVNODE_DVB_CA	(MEDIA_ENT_T_DEVNODE + 7)
+#define MEDIA_ENT_T_DEVNODE_DVB_NET	(MEDIA_ENT_T_DEVNODE + 8)
+
+/* Legacy symbol. Use it to avoid userspace compilation breakages */
+#define MEDIA_ENT_T_DEVNODE_DVB		MEDIA_ENT_T_DEVNODE_DVB_FE
 
 #define MEDIA_ENT_T_V4L2_SUBDEV		(2 << MEDIA_ENT_TYPE_SHIFT)
 #define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR	(MEDIA_ENT_T_V4L2_SUBDEV + 1)
-- 
2.1.0

[PATCH 3/3] media: add a subdev type for tuner

[Mauro Carvalho Chehab]

Add MEDIA_ENT_T_V4L2_SUBDEV_TUNER to represent the V4L2
(and dvb) tuner subdevices.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 4c8f26243252..52cc2a6b19b7 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -66,6 +66,8 @@ struct media_device_info {
 /* A converter of analogue video to its digital representation. */
 #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	(MEDIA_ENT_T_V4L2_SUBDEV + 4)
 
+#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	(MEDIA_ENT_T_V4L2_SUBDEV + 5)
+
 #define MEDIA_ENT_FL_DEFAULT		(1 << 0)
 
 struct media_entity_desc {
-- 
2.1.0

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Hans Verkuil]

On 01/26/2015 01:47 PM, Mauro Carvalho Chehab wrote:
> The previous provision for DVB media controller support were to
> define an ID (likely meaning the adapter number) for the DVB
> devnodes.
> 
> This is just plain wrong. Just like V4L, DVB devices (and ALSA,
> or whatever) are identified via a (major, minor) tuple.
> 
> This is enough to uniquely identify a devnode, no matter what
> API it implements.
> 
> So, before we go too far, let's mark the old v4l, dvb and alsa
> "devnode" info as deprecated, and just call it as "dev".
> 
> As we don't want to break compilation on already existing apps,
> let's just keep the old definitions as-is, adding a note that
> those are deprecated at media-entity.h.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 86bb93fd7db8..d89d5cb465d9 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -943,8 +943,8 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
>  	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
>  		vdev->entity.type = MEDIA_ENT_T_DEVNODE_V4L;
>  		vdev->entity.name = vdev->name;
> -		vdev->entity.info.v4l.major = VIDEO_MAJOR;
> -		vdev->entity.info.v4l.minor = vdev->minor;
> +		vdev->entity.info.dev.major = VIDEO_MAJOR;
> +		vdev->entity.info.dev.minor = vdev->minor;
>  		ret = media_device_register_entity(vdev->v4l2_dev->mdev,
>  			&vdev->entity);
>  		if (ret < 0)
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index 015f92aab44a..204cc67c84e8 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -248,8 +248,8 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
>  			goto clean_up;
>  		}
>  #if defined(CONFIG_MEDIA_CONTROLLER)
> -		sd->entity.info.v4l.major = VIDEO_MAJOR;
> -		sd->entity.info.v4l.minor = vdev->minor;
> +		sd->entity.info.dev.major = VIDEO_MAJOR;
> +		sd->entity.info.dev.minor = vdev->minor;
>  #endif
>  		sd->devnode = vdev;
>  	}
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index e00459185d20..d6d74bcfe183 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -87,17 +87,7 @@ struct media_entity {
>  		struct {
>  			u32 major;
>  			u32 minor;
> -		} v4l;
> -		struct {
> -			u32 major;
> -			u32 minor;
> -		} fb;
> -		struct {
> -			u32 card;
> -			u32 device;
> -			u32 subdevice;
> -		} alsa;

I don't think the alsa entity information can be replaced by major/minor.
In particular you will loose the subdevice information which you need as
well. In addition, alsa devices are almost never referenced via major and
minor numbers, but always by card/device/subdevice numbers.

> -		int dvb;
> +		} dev;
>  
>  		/* Sub-device specifications */
>  		/* Nothing needed yet */
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index d847c760e8f0..418f4fec391a 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -78,6 +78,20 @@ struct media_entity_desc {
>  		struct {
>  			__u32 major;
>  			__u32 minor;
> +		} dev;
> +
> +#if 1
> +		/*
> +		 * DEPRECATED: previous node specifications. Kept just to
> +		 * avoid breaking compilation, but media_entity_desc.dev
> +		 * should be used instead. In particular, alsa and dvb
> +		 * fields below are wrong: for all devnodes, there should
> +		 * be just major/minor inside the struct, as this is enough
> +		 * to represent any devnode, no matter what type.
> +		 */
> +		struct {
> +			__u32 major;
> +			__u32 minor;
>  		} v4l;
>  		struct {
>  			__u32 major;
> @@ -89,6 +103,7 @@ struct media_entity_desc {
>  			__u32 subdevice;
>  		} alsa;
>  		int dvb;

I wouldn't merge all the v4l/fb/etc. structs into one struct. That will make it
difficult in the future if you need to add a field for e.g. v4l entities.

So I would keep the v4l, fb and alsa structs, and just add a new struct for
dvb. I wonder if the dvb field can't just be replaced since I doubt anyone is
using it. And even if someone does, then it can't be right since a single
int isn't enough and never worked anyway.

Regards,

	Hans

> +#endif
>  
>  		/* Sub-device specifications */
>  		/* Nothing needed yet */
> 

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Mauro Carvalho Chehab]

Em Mon, 26 Jan 2015 14:11:50 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 01/26/2015 01:47 PM, Mauro Carvalho Chehab wrote:
> > The previous provision for DVB media controller support were to
> > define an ID (likely meaning the adapter number) for the DVB
> > devnodes.
> > 
> > This is just plain wrong. Just like V4L, DVB devices (and ALSA,
> > or whatever) are identified via a (major, minor) tuple.
> > 
> > This is enough to uniquely identify a devnode, no matter what
> > API it implements.
> > 
> > So, before we go too far, let's mark the old v4l, dvb and alsa
> > "devnode" info as deprecated, and just call it as "dev".
> > 
> > As we don't want to break compilation on already existing apps,
> > let's just keep the old definitions as-is, adding a note that
> > those are deprecated at media-entity.h.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 86bb93fd7db8..d89d5cb465d9 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -943,8 +943,8 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
> >  	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
> >  		vdev->entity.type = MEDIA_ENT_T_DEVNODE_V4L;
> >  		vdev->entity.name = vdev->name;
> > -		vdev->entity.info.v4l.major = VIDEO_MAJOR;
> > -		vdev->entity.info.v4l.minor = vdev->minor;
> > +		vdev->entity.info.dev.major = VIDEO_MAJOR;
> > +		vdev->entity.info.dev.minor = vdev->minor;
> >  		ret = media_device_register_entity(vdev->v4l2_dev->mdev,
> >  			&vdev->entity);
> >  		if (ret < 0)
> > diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> > index 015f92aab44a..204cc67c84e8 100644
> > --- a/drivers/media/v4l2-core/v4l2-device.c
> > +++ b/drivers/media/v4l2-core/v4l2-device.c
> > @@ -248,8 +248,8 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
> >  			goto clean_up;
> >  		}
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> > -		sd->entity.info.v4l.major = VIDEO_MAJOR;
> > -		sd->entity.info.v4l.minor = vdev->minor;
> > +		sd->entity.info.dev.major = VIDEO_MAJOR;
> > +		sd->entity.info.dev.minor = vdev->minor;
> >  #endif
> >  		sd->devnode = vdev;
> >  	}
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index e00459185d20..d6d74bcfe183 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -87,17 +87,7 @@ struct media_entity {
> >  		struct {
> >  			u32 major;
> >  			u32 minor;
> > -		} v4l;
> > -		struct {
> > -			u32 major;
> > -			u32 minor;
> > -		} fb;
> > -		struct {
> > -			u32 card;
> > -			u32 device;
> > -			u32 subdevice;
> > -		} alsa;
> 
> I don't think the alsa entity information can be replaced by major/minor.
> In particular you will loose the subdevice information which you need as
> well. In addition, alsa devices are almost never referenced via major and
> minor numbers, but always by card/device/subdevice numbers.

For media-ctl, it is easier to handle major/minor, in order to identify
the associated devnode name. Btw, media-ctl currently assumes that all
devnode devices are specified by v4l.major/v4l.minor.

Ok, maybe for alsa we'll need also card/device/subdevice, but I think this
should be mapped elsewhere, if this can't be retrieved via its sysfs/udev
interface (with seems to be doubtful).

> 
> > -		int dvb;
> > +		} dev;
> >  
> >  		/* Sub-device specifications */
> >  		/* Nothing needed yet */
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index d847c760e8f0..418f4fec391a 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -78,6 +78,20 @@ struct media_entity_desc {
> >  		struct {
> >  			__u32 major;
> >  			__u32 minor;
> > +		} dev;
> > +
> > +#if 1
> > +		/*
> > +		 * DEPRECATED: previous node specifications. Kept just to
> > +		 * avoid breaking compilation, but media_entity_desc.dev
> > +		 * should be used instead. In particular, alsa and dvb
> > +		 * fields below are wrong: for all devnodes, there should
> > +		 * be just major/minor inside the struct, as this is enough
> > +		 * to represent any devnode, no matter what type.
> > +		 */
> > +		struct {
> > +			__u32 major;
> > +			__u32 minor;
> >  		} v4l;
> >  		struct {
> >  			__u32 major;
> > @@ -89,6 +103,7 @@ struct media_entity_desc {
> >  			__u32 subdevice;
> >  		} alsa;
> >  		int dvb;
> 
> I wouldn't merge all the v4l/fb/etc. structs into one struct. That will make it
> difficult in the future if you need to add a field for e.g. v4l entities.

No. You could just create another union for the API-specific bits, using the
reserved bytes.

> So I would keep the v4l, fb and alsa structs, and just add a new struct for
> dvb. I wonder if the dvb field can't just be replaced since I doubt anyone is
> using it. And even if someone does, then it can't be right since a single
> int isn't enough and never worked anyway.

All devnodes have major/minor. Making it standard for all devices makes
easy for userspace to properly get the data it requires to work.

Regards,
Mauro

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Hans Verkuil]

On 01/26/2015 02:34 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 26 Jan 2015 14:11:50 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 01/26/2015 01:47 PM, Mauro Carvalho Chehab wrote:
>>> The previous provision for DVB media controller support were to
>>> define an ID (likely meaning the adapter number) for the DVB
>>> devnodes.
>>>
>>> This is just plain wrong. Just like V4L, DVB devices (and ALSA,
>>> or whatever) are identified via a (major, minor) tuple.
>>>
>>> This is enough to uniquely identify a devnode, no matter what
>>> API it implements.
>>>
>>> So, before we go too far, let's mark the old v4l, dvb and alsa
>>> "devnode" info as deprecated, and just call it as "dev".
>>>
>>> As we don't want to break compilation on already existing apps,
>>> let's just keep the old definitions as-is, adding a note that
>>> those are deprecated at media-entity.h.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>> index 86bb93fd7db8..d89d5cb465d9 100644
>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>> @@ -943,8 +943,8 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
>>>  	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
>>>  		vdev->entity.type = MEDIA_ENT_T_DEVNODE_V4L;
>>>  		vdev->entity.name = vdev->name;
>>> -		vdev->entity.info.v4l.major = VIDEO_MAJOR;
>>> -		vdev->entity.info.v4l.minor = vdev->minor;
>>> +		vdev->entity.info.dev.major = VIDEO_MAJOR;
>>> +		vdev->entity.info.dev.minor = vdev->minor;
>>>  		ret = media_device_register_entity(vdev->v4l2_dev->mdev,
>>>  			&vdev->entity);
>>>  		if (ret < 0)
>>> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
>>> index 015f92aab44a..204cc67c84e8 100644
>>> --- a/drivers/media/v4l2-core/v4l2-device.c
>>> +++ b/drivers/media/v4l2-core/v4l2-device.c
>>> @@ -248,8 +248,8 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
>>>  			goto clean_up;
>>>  		}
>>>  #if defined(CONFIG_MEDIA_CONTROLLER)
>>> -		sd->entity.info.v4l.major = VIDEO_MAJOR;
>>> -		sd->entity.info.v4l.minor = vdev->minor;
>>> +		sd->entity.info.dev.major = VIDEO_MAJOR;
>>> +		sd->entity.info.dev.minor = vdev->minor;
>>>  #endif
>>>  		sd->devnode = vdev;
>>>  	}
>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>> index e00459185d20..d6d74bcfe183 100644
>>> --- a/include/media/media-entity.h
>>> +++ b/include/media/media-entity.h
>>> @@ -87,17 +87,7 @@ struct media_entity {
>>>  		struct {
>>>  			u32 major;
>>>  			u32 minor;
>>> -		} v4l;
>>> -		struct {
>>> -			u32 major;
>>> -			u32 minor;
>>> -		} fb;
>>> -		struct {
>>> -			u32 card;
>>> -			u32 device;
>>> -			u32 subdevice;
>>> -		} alsa;
>>
>> I don't think the alsa entity information can be replaced by major/minor.
>> In particular you will loose the subdevice information which you need as
>> well. In addition, alsa devices are almost never referenced via major and
>> minor numbers, but always by card/device/subdevice numbers.
> 
> For media-ctl, it is easier to handle major/minor, in order to identify
> the associated devnode name. Btw, media-ctl currently assumes that all
> devnode devices are specified by v4l.major/v4l.minor.
> 
> Ok, maybe for alsa we'll need also card/device/subdevice, but I think this
> should be mapped elsewhere, if this can't be retrieved via its sysfs/udev
> interface (with seems to be doubtful).

The card/device tuple can likely be mapped to major/minor, but not subdevice.
And since everything inside alsa is based on card/device I wouldn't change
that.

> 
>>
>>> -		int dvb;
>>> +		} dev;
>>>  
>>>  		/* Sub-device specifications */
>>>  		/* Nothing needed yet */
>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>> index d847c760e8f0..418f4fec391a 100644
>>> --- a/include/uapi/linux/media.h
>>> +++ b/include/uapi/linux/media.h
>>> @@ -78,6 +78,20 @@ struct media_entity_desc {
>>>  		struct {
>>>  			__u32 major;
>>>  			__u32 minor;
>>> +		} dev;
>>> +
>>> +#if 1
>>> +		/*
>>> +		 * DEPRECATED: previous node specifications. Kept just to
>>> +		 * avoid breaking compilation, but media_entity_desc.dev
>>> +		 * should be used instead. In particular, alsa and dvb
>>> +		 * fields below are wrong: for all devnodes, there should
>>> +		 * be just major/minor inside the struct, as this is enough
>>> +		 * to represent any devnode, no matter what type.
>>> +		 */
>>> +		struct {
>>> +			__u32 major;
>>> +			__u32 minor;
>>>  		} v4l;
>>>  		struct {
>>>  			__u32 major;
>>> @@ -89,6 +103,7 @@ struct media_entity_desc {
>>>  			__u32 subdevice;
>>>  		} alsa;
>>>  		int dvb;
>>
>> I wouldn't merge all the v4l/fb/etc. structs into one struct. That will make it
>> difficult in the future if you need to add a field for e.g. v4l entities.
> 
> No. You could just create another union for the API-specific bits, using the
> reserved bytes.
> 
>> So I would keep the v4l, fb and alsa structs, and just add a new struct for
>> dvb. I wonder if the dvb field can't just be replaced since I doubt anyone is
>> using it. And even if someone does, then it can't be right since a single
>> int isn't enough and never worked anyway.
> 
> All devnodes have major/minor. Making it standard for all devices makes
> easy for userspace to properly get the data it requires to work.

I think you are making assumptions here that may not be true. I don't see any
reason to make a 'dev' struct here. The real problem is the dvb int, so that's
what needs to be addressed. Changing anything else will cause API headaches
for no good reason.

Regards,

	Hans

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Hans Verkuil]

On 01/26/2015 02:34 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 26 Jan 2015 14:11:50 +0100
> Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> escreveu:
> 
>> On 01/26/2015 01:47 PM, Mauro Carvalho Chehab wrote:
>>> The previous provision for DVB media controller support were to
>>> define an ID (likely meaning the adapter number) for the DVB
>>> devnodes.
>>>
>>> This is just plain wrong. Just like V4L, DVB devices (and ALSA,
>>> or whatever) are identified via a (major, minor) tuple.
>>>
>>> This is enough to uniquely identify a devnode, no matter what
>>> API it implements.
>>>
>>> So, before we go too far, let's mark the old v4l, dvb and alsa
>>> "devnode" info as deprecated, and just call it as "dev".
>>>
>>> As we don't want to break compilation on already existing apps,
>>> let's just keep the old definitions as-is, adding a note that
>>> those are deprecated at media-entity.h.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>> index 86bb93fd7db8..d89d5cb465d9 100644
>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>> @@ -943,8 +943,8 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
>>>  	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
>>>  		vdev->entity.type = MEDIA_ENT_T_DEVNODE_V4L;
>>>  		vdev->entity.name = vdev->name;
>>> -		vdev->entity.info.v4l.major = VIDEO_MAJOR;
>>> -		vdev->entity.info.v4l.minor = vdev->minor;
>>> +		vdev->entity.info.dev.major = VIDEO_MAJOR;
>>> +		vdev->entity.info.dev.minor = vdev->minor;
>>>  		ret = media_device_register_entity(vdev->v4l2_dev->mdev,
>>>  			&vdev->entity);
>>>  		if (ret < 0)
>>> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
>>> index 015f92aab44a..204cc67c84e8 100644
>>> --- a/drivers/media/v4l2-core/v4l2-device.c
>>> +++ b/drivers/media/v4l2-core/v4l2-device.c
>>> @@ -248,8 +248,8 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
>>>  			goto clean_up;
>>>  		}
>>>  #if defined(CONFIG_MEDIA_CONTROLLER)
>>> -		sd->entity.info.v4l.major = VIDEO_MAJOR;
>>> -		sd->entity.info.v4l.minor = vdev->minor;
>>> +		sd->entity.info.dev.major = VIDEO_MAJOR;
>>> +		sd->entity.info.dev.minor = vdev->minor;
>>>  #endif
>>>  		sd->devnode = vdev;
>>>  	}
>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>> index e00459185d20..d6d74bcfe183 100644
>>> --- a/include/media/media-entity.h
>>> +++ b/include/media/media-entity.h
>>> @@ -87,17 +87,7 @@ struct media_entity {
>>>  		struct {
>>>  			u32 major;
>>>  			u32 minor;
>>> -		} v4l;
>>> -		struct {
>>> -			u32 major;
>>> -			u32 minor;
>>> -		} fb;
>>> -		struct {
>>> -			u32 card;
>>> -			u32 device;
>>> -			u32 subdevice;
>>> -		} alsa;
>>
>> I don't think the alsa entity information can be replaced by major/minor.
>> In particular you will loose the subdevice information which you need as
>> well. In addition, alsa devices are almost never referenced via major and
>> minor numbers, but always by card/device/subdevice numbers.
> 
> For media-ctl, it is easier to handle major/minor, in order to identify
> the associated devnode name. Btw, media-ctl currently assumes that all
> devnode devices are specified by v4l.major/v4l.minor.
> 
> Ok, maybe for alsa we'll need also card/device/subdevice, but I think this
> should be mapped elsewhere, if this can't be retrieved via its sysfs/udev
> interface (with seems to be doubtful).

The card/device tuple can likely be mapped to major/minor, but not subdevice.
And since everything inside alsa is based on card/device I wouldn't change
that.

> 
>>
>>> -		int dvb;
>>> +		} dev;
>>>  
>>>  		/* Sub-device specifications */
>>>  		/* Nothing needed yet */
>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>> index d847c760e8f0..418f4fec391a 100644
>>> --- a/include/uapi/linux/media.h
>>> +++ b/include/uapi/linux/media.h
>>> @@ -78,6 +78,20 @@ struct media_entity_desc {
>>>  		struct {
>>>  			__u32 major;
>>>  			__u32 minor;
>>> +		} dev;
>>> +
>>> +#if 1
>>> +		/*
>>> +		 * DEPRECATED: previous node specifications. Kept just to
>>> +		 * avoid breaking compilation, but media_entity_desc.dev
>>> +		 * should be used instead. In particular, alsa and dvb
>>> +		 * fields below are wrong: for all devnodes, there should
>>> +		 * be just major/minor inside the struct, as this is enough
>>> +		 * to represent any devnode, no matter what type.
>>> +		 */
>>> +		struct {
>>> +			__u32 major;
>>> +			__u32 minor;
>>>  		} v4l;
>>>  		struct {
>>>  			__u32 major;
>>> @@ -89,6 +103,7 @@ struct media_entity_desc {
>>>  			__u32 subdevice;
>>>  		} alsa;
>>>  		int dvb;
>>
>> I wouldn't merge all the v4l/fb/etc. structs into one struct. That will make it
>> difficult in the future if you need to add a field for e.g. v4l entities.
> 
> No. You could just create another union for the API-specific bits, using the
> reserved bytes.
> 
>> So I would keep the v4l, fb and alsa structs, and just add a new struct for
>> dvb. I wonder if the dvb field can't just be replaced since I doubt anyone is
>> using it. And even if someone does, then it can't be right since a single
>> int isn't enough and never worked anyway.
> 
> All devnodes have major/minor. Making it standard for all devices makes
> easy for userspace to properly get the data it requires to work.

I think you are making assumptions here that may not be true. I don't see any
reason to make a 'dev' struct here. The real problem is the dvb int, so that's
what needs to be addressed. Changing anything else will cause API headaches
for no good reason.

Regards,

	Hans

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Devin Heitmueller]

> For media-ctl, it is easier to handle major/minor, in order to identify
> the associated devnode name. Btw, media-ctl currently assumes that all
> devnode devices are specified by v4l.major/v4l.minor.

I suspect part of the motivation for the "id" that corresponds to the
adapter field was to make it easier to find the actual underlying
device node.  While it's trivial to convert a V4L device node from
major/minor to the device node (since for major number is constant and
the minor corresponds to the X in /dev/videoX), that's tougher with
DVB adapters because of the hierarchical nature of the DVB device
nodes.  Having the adapter number makes it trivial to open
/dev/dvb/adapterX.

Perhaps my POSIX is rusty -- is there a way to identify the device
node based on major minor without having to traverse the entire /dev
tree?

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Mauro Carvalho Chehab]

Em Mon, 26 Jan 2015 09:00:46 -0500
Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:

> > For media-ctl, it is easier to handle major/minor, in order to identify
> > the associated devnode name. Btw, media-ctl currently assumes that all
> > devnode devices are specified by v4l.major/v4l.minor.
> 
> I suspect part of the motivation for the "id" that corresponds to the
> adapter field was to make it easier to find the actual underlying
> device node. 

Yes, that was the reason why, back in 2007, we believed that just id
would be enough. Yet, we never tried to implement it, until the end
of the last year.

> While it's trivial to convert a V4L device node from
> major/minor to the device node (since for major number is constant and
> the minor corresponds to the X in /dev/videoX), that's tougher with
> DVB adapters because of the hierarchical nature of the DVB device
> nodes.  Having the adapter number makes it trivial to open
> /dev/dvb/adapterX.
> 
> Perhaps my POSIX is rusty -- is there a way to identify the device
> node based on major minor without having to traverse the entire /dev
> tree?

It is actually trivial to get the device nodes once you have the
major/minor. The media-ctl library does that for you. See:

$ media-ctl --print-dot
digraph board {
	rankdir=TB
	n00000001 [label="{{<port0> 0} | cx25840 19-0044 | {<port1> 1 | <port2> 2}}", shape=Mrecord, style=filled, fillcolor=green]
	n00000001:port1 -> n00000003
	n00000001:port2 -> n00000004
	n00000002 [label="{{} | NXP TDA18271HD | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
	n00000002:port0 -> n00000005 [style=dashed]
	n00000002:port0 -> n00000001:port0
	n00000003 [label="cx231xx #0 video\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
	n00000004 [label="cx231xx #0 vbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
	n00000005 [label="Fujitsu mb86A20s\n/dev/dvb/adapter0/frontend0", shape=box, style=filled, fillcolor=yellow]
	n00000005 -> n00000006
	n00000006 [label="demux\n/dev/dvb/adapter0/demux0", shape=box, style=filled, fillcolor=yellow]
	n00000006 -> n00000007
	n00000007 [label="dvr\n/dev/dvb/adapter0/dvr0", shape=box, style=filled, fillcolor=yellow]
	n00000008 [label="dvb net\n/dev/dvb/adapter0/net0", shape=box, style=filled, fillcolor=yellow]
}

There are two routines inside the media-ctl libraries to convert from
major/minor into a devnode name like: /dev/dvb/adapter0/demux0.

The first one uses sysfs:

 $ ls -la /sys/dev/char|grep dvb
lrwxrwxrwx. 1 root root 0 Jan 26 10:32 212:0 -> ../../devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/dvb/dvb0.frontend0
lrwxrwxrwx. 1 root root 0 Jan 26 10:32 212:1 -> ../../devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/dvb/dvb0.demux0
lrwxrwxrwx. 1 root root 0 Jan 26 10:32 212:2 -> ../../devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/dvb/dvb0.dvr0
lrwxrwxrwx. 1 root root 0 Jan 26 10:32 212:3 -> ../../devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/dvb/dvb0.net0

Unfortunately, the sysfs nodes are "dvb0" for adapter0, so a patch is needed 
to fix it:
	http://git.linuxtv.org/cgit.cgi/mchehab/experimental-v4l-utils.git/commit/?h=dvb-media-ctl&id=d854a9bb24706dbfc878484e4538d79b1ac52aae

The second (and better) approach is to require udev to return the name of the
devnode. The logic, implemented at utils/media-ctl/libmediactl.c, inside
v4l-utils, is:

	devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
	media_dbg(entity->media, "looking up device: %u:%u\n",
		  major(devnum), minor(devnum));
	device = udev_device_new_from_devnum(udev, 'c', devnum);

Right now, by default, media-ctl will use the sysfs approach, except
if an extra option is called at ./configure, in order to enable it to
use the udev library.

IMHO, we should make udev the default behavior, if libudev-dev(el) is 
there at compilation time.

Regards,
Mauro

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Devin Heitmueller]

> It is actually trivial to get the device nodes once you have the
> major/minor. The media-ctl library does that for you. See:

No objection then.

On a related note, you would be very well served to consider testing
your dvb changes with a device that has more than one DVB tuner (such
as the hvr-2200/2250).  That will help you shake out any edge cases
related to ensuring that the different DVB nodes appear in different
groups.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Devin Heitmueller]

> It is actually trivial to get the device nodes once you have the
> major/minor. The media-ctl library does that for you. See:

No objection then.

On a related note, you would be very well served to consider testing
your dvb changes with a device that has more than one DVB tuner (such
as the hvr-2200/2250).  That will help you shake out any edge cases
related to ensuring that the different DVB nodes appear in different
groups.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Mauro Carvalho Chehab]

Em Mon, 26 Jan 2015 09:41:41 -0500
Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:

> > It is actually trivial to get the device nodes once you have the
> > major/minor. The media-ctl library does that for you. See:
> 
> No objection then.
> 
> On a related note, you would be very well served to consider testing
> your dvb changes with a device that has more than one DVB tuner (such
> as the hvr-2200/2250).  That will help you shake out any edge cases
> related to ensuring that the different DVB nodes appear in different
> groups.

Hi Devin,

I did some tests (and fixes) for WinTV Nova-TD, with has two adapters.

I saw two alternatives for it:

1) to create a media controller device for each adapter;
2) to create just one media controller.

I actually implemented (1), as, in the case of this device, AFAIKT, the
two devices are indepentent, e. g. it is not possible to, for example,
share the same tuner with two demods:

$ ls -la /dev/media?
crw-rw----. 1 root video 249, 0 Fev 23 10:02 /dev/media0
crw-rw----. 1 root video 249, 1 Fev 23 10:02 /dev/media1

The adapter 0 corresponds to /dev/media0, and the adapter 1
to /dev/media1:

$ media-ctl --print-dot -d /dev/media0
digraph board {
	rankdir=TB
	n00000001 [label="dvb-demux\n/dev/dvb/adapter0/demux0", shape=box, style=filled, fillcolor=yellow]
	n00000001 -> n00000002
	n00000002 [label="dvb-dvr\n/dev/dvb/adapter0/dvr0", shape=box, style=filled, fillcolor=yellow]
	n00000003 [label="dvb-net\n/dev/dvb/adapter0/net0", shape=box, style=filled, fillcolor=yellow]
	n00000004 [label="DiBcom 7000PC\n/dev/dvb/adapter0/frontend0", shape=box, style=filled, fillcolor=yellow]
	n00000004 -> n00000001
}

$ media-ctl --print-dot -d /dev/media1
digraph board {
	rankdir=TB
	n00000001 [label="dvb-demux\n/dev/dvb/adapter1/demux0", shape=box, style=filled, fillcolor=yellow]
	n00000001 -> n00000002
	n00000002 [label="dvb-dvr\n/dev/dvb/adapter1/dvr0", shape=box, style=filled, fillcolor=yellow]
	n00000003 [label="dvb-net\n/dev/dvb/adapter1/net0", shape=box, style=filled, fillcolor=yellow]
	n00000004 [label="DiBcom 7000PC\n/dev/dvb/adapter1/frontend0", shape=box, style=filled, fillcolor=yellow]
	n00000004 -> n00000001
}

On a more complex hardware where some components may be rewired
on a different way, however, using just one media controller
would be a better approach.

Comments?

Mauro

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Mauro Carvalho Chehab]

Em Mon, 26 Jan 2015 09:41:41 -0500
Devin Heitmueller <dheitmueller-eb9eJ82Ua7k9XoPSrs7Ehg@public.gmane.org> escreveu:

> > It is actually trivial to get the device nodes once you have the
> > major/minor. The media-ctl library does that for you. See:
> 
> No objection then.
> 
> On a related note, you would be very well served to consider testing
> your dvb changes with a device that has more than one DVB tuner (such
> as the hvr-2200/2250).  That will help you shake out any edge cases
> related to ensuring that the different DVB nodes appear in different
> groups.

Hi Devin,

I did some tests (and fixes) for WinTV Nova-TD, with has two adapters.

I saw two alternatives for it:

1) to create a media controller device for each adapter;
2) to create just one media controller.

I actually implemented (1), as, in the case of this device, AFAIKT, the
two devices are indepentent, e. g. it is not possible to, for example,
share the same tuner with two demods:

$ ls -la /dev/media?
crw-rw----. 1 root video 249, 0 Fev 23 10:02 /dev/media0
crw-rw----. 1 root video 249, 1 Fev 23 10:02 /dev/media1

The adapter 0 corresponds to /dev/media0, and the adapter 1
to /dev/media1:

$ media-ctl --print-dot -d /dev/media0
digraph board {
	rankdir=TB
	n00000001 [label="dvb-demux\n/dev/dvb/adapter0/demux0", shape=box, style=filled, fillcolor=yellow]
	n00000001 -> n00000002
	n00000002 [label="dvb-dvr\n/dev/dvb/adapter0/dvr0", shape=box, style=filled, fillcolor=yellow]
	n00000003 [label="dvb-net\n/dev/dvb/adapter0/net0", shape=box, style=filled, fillcolor=yellow]
	n00000004 [label="DiBcom 7000PC\n/dev/dvb/adapter0/frontend0", shape=box, style=filled, fillcolor=yellow]
	n00000004 -> n00000001
}

$ media-ctl --print-dot -d /dev/media1
digraph board {
	rankdir=TB
	n00000001 [label="dvb-demux\n/dev/dvb/adapter1/demux0", shape=box, style=filled, fillcolor=yellow]
	n00000001 -> n00000002
	n00000002 [label="dvb-dvr\n/dev/dvb/adapter1/dvr0", shape=box, style=filled, fillcolor=yellow]
	n00000003 [label="dvb-net\n/dev/dvb/adapter1/net0", shape=box, style=filled, fillcolor=yellow]
	n00000004 [label="DiBcom 7000PC\n/dev/dvb/adapter1/frontend0", shape=box, style=filled, fillcolor=yellow]
	n00000004 -> n00000001
}

On a more complex hardware where some components may be rewired
on a different way, however, using just one media controller
would be a better approach.

Comments?

Mauro

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Laurent Pinchart]

Hi Mauro,

On Monday 23 February 2015 10:55:08 Mauro Carvalho Chehab wrote:
> Em Mon, 26 Jan 2015 09:41:41 -0500 Devin Heitmueller escreveu:
> >> It is actually trivial to get the device nodes once you have the
> >> major/minor. The media-ctl library does that for you. See:
> >
> > No objection then.
> > 
> > On a related note, you would be very well served to consider testing
> > your dvb changes with a device that has more than one DVB tuner (such
> > as the hvr-2200/2250).  That will help you shake out any edge cases
> > related to ensuring that the different DVB nodes appear in different
> > groups.
> 
> Hi Devin,
> 
> I did some tests (and fixes) for WinTV Nova-TD, with has two adapters.
> 
> I saw two alternatives for it:
> 
> 1) to create a media controller device for each adapter;
> 2) to create just one media controller.
> 
> I actually implemented (1), as, in the case of this device, AFAIKT, the
> two devices are indepentent, e. g. it is not possible to, for example,
> share the same tuner with two demods:
> 
> $ ls -la /dev/media?
> crw-rw----. 1 root video 249, 0 Fev 23 10:02 /dev/media0
> crw-rw----. 1 root video 249, 1 Fev 23 10:02 /dev/media1
> 
> The adapter 0 corresponds to /dev/media0, and the adapter 1
> to /dev/media1:
> 
> $ media-ctl --print-dot -d /dev/media0
> digraph board {
> 	rankdir=TB
> 	n00000001 [label="dvb-demux\n/dev/dvb/adapter0/demux0", shape=box,
> style=filled, fillcolor=yellow] n00000001 -> n00000002
> 	n00000002 [label="dvb-dvr\n/dev/dvb/adapter0/dvr0", shape=box,
> style=filled, fillcolor=yellow] n00000003
> [label="dvb-net\n/dev/dvb/adapter0/net0", shape=box, style=filled,
> fillcolor=yellow] n00000004 [label="DiBcom
> 7000PC\n/dev/dvb/adapter0/frontend0", shape=box, style=filled,
> fillcolor=yellow] n00000004 -> n00000001
> }
> 
> $ media-ctl --print-dot -d /dev/media1
> digraph board {
> 	rankdir=TB
> 	n00000001 [label="dvb-demux\n/dev/dvb/adapter1/demux0", shape=box,
> style=filled, fillcolor=yellow] n00000001 -> n00000002
> 	n00000002 [label="dvb-dvr\n/dev/dvb/adapter1/dvr0", shape=box,
> style=filled, fillcolor=yellow] n00000003
> [label="dvb-net\n/dev/dvb/adapter1/net0", shape=box, style=filled,
> fillcolor=yellow] n00000004 [label="DiBcom
> 7000PC\n/dev/dvb/adapter1/frontend0", shape=box, style=filled,
> fillcolor=yellow] n00000004 -> n00000001
> }
> 
> On a more complex hardware where some components may be rewired
> on a different way, however, using just one media controller
> would be a better approach.
> 
> Comments?

A few, yes :-)

There's surprisingly many "details" that are not fully specified in MC, and 
this is one of them. Historically the design idea was to create one media 
device per instance of master video device. For PCI devices that's roughly one 
media device per card, for platform devices one media device per master IP 
core (or group of IP cores) instance, and for USB devices one media device per 
USB control interface.

We can depart from that model in two ways.

As you mentioned above, it could make sense to create separate media device 
instances for a single master device (USB in this case) when the master device 
contains several completely independent pipelines. Completely independent 
means that no data link connects the two pipelines, and that no resource is 
shared between them in a way that affects the device's behaviour.

In the example above the only shared hardware resource seems to be the USB 
bridge chip, which implements two independent DMA channels through the same 
USB interface. If the DMA channels are really independent, in the sense that 
they have no influence on each other such as e.g. bandwidth sharing, then two 
media devices could be created. Note that there's no requirement to do so, 
creating a single media device in this case is still perfectly valid.

It should also be noted that split pipelines could be difficult to represent 
as independent media devices when an external chip is shared between the two 
pipelines, even if that chip is itself split in two independent parts. This is 
caused by how the kernel APIs used to manage composite devices (v4l2-async and 
the component framework) handle components. For instance, in the DT case, we 
can't use v4l2-async to bind to one of the subdevs create for a single I2C 
chip or IP core, as they would share the same hardware identifier (device 
name, DT node, ...) used to match subdevs. Arguably that's a framework issue 
that should be fixed, but it wouldn't be trivial.

The second way to depart from the existing model is unrelated to the DVB 
examples above, but is still worth mentioning for completeness. When several 
master devices share resources (either on-chip or off-chip), a single media 
device is required. I've discussed such a case with Jean-Michel Hautbois and 
Hans Verkuil at the FOSDEM for the i.MX6 SoC. The chip includes two 
independent IPUs (Image Processing Units), with two independent parallel 
receivers but one shared CSI-2 receiver. On the system Jean-Michel is working 
with, an external FPGA is connected to the two parallel receivers. The 
resulting media pipeline thus includes the FPGA, the CSI-2 receiver and the 
two IPUs, requiring a single media device to cover both IPUs. As the IPU 
driver is the master driver in this case, it would normally create one media 
device per hardware device instance.

A similar situation occurs when video capture devices, memory to memory 
devices and/or display devices are connected together through deep pipelining. 
Such an example can be found in the Renesas R-Car Gen2 SoCs, where a memory to 
memory video processing device (VSP1) has one output connected directly to the 
display engine (DU). The VSP1 and DU are otherwise completely separate IP 
cores, with the former supported by a V4L2 driver and the latter by a DRM/KMS 
driver. We also need a single media device to cover the whole graph, without 
intrusive changes in either the VSP1 or the DU driver as the two devices can 
operate completely independently and without both being present.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Laurent Pinchart]

Hi Mauro and Hans,

On Monday 26 January 2015 14:41:53 Hans Verkuil wrote:
> On 01/26/2015 02:34 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 26 Jan 2015 14:11:50 +0100 Hans Verkuil escreveu:
> >> On 01/26/2015 01:47 PM, Mauro Carvalho Chehab wrote:
> >>> The previous provision for DVB media controller support were to
> >>> define an ID (likely meaning the adapter number) for the DVB
> >>> devnodes.
> >>> 
> >>> This is just plain wrong. Just like V4L, DVB devices (and ALSA,
> >>> or whatever) are identified via a (major, minor) tuple.
> >>> 
> >>> This is enough to uniquely identify a devnode, no matter what
> >>> API it implements.
> >>> 
> >>> So, before we go too far, let's mark the old v4l, dvb and alsa
> >>> "devnode" info as deprecated, and just call it as "dev".
> >>> 
> >>> As we don't want to break compilation on already existing apps,
> >>> let's just keep the old definitions as-is, adding a note that
> >>> those are deprecated at media-entity.h.
> >>> 
> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

[snip]

> >>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> >>> index e00459185d20..d6d74bcfe183 100644
> >>> --- a/include/media/media-entity.h
> >>> +++ b/include/media/media-entity.h
> >>> @@ -87,17 +87,7 @@ struct media_entity {
> >>>  		struct {
> >>>  			u32 major;
> >>>  			u32 minor;
> >>> -		} v4l;
> >>> -		struct {
> >>> -			u32 major;
> >>> -			u32 minor;
> >>> -		} fb;
> >>> -		struct {
> >>> -			u32 card;
> >>> -			u32 device;
> >>> -			u32 subdevice;
> >>> -		} alsa;
> >> 
> >> I don't think the alsa entity information can be replaced by major/minor.
> >> In particular you will loose the subdevice information which you need as
> >> well. In addition, alsa devices are almost never referenced via major and
> >> minor numbers, but always by card/device/subdevice numbers.
> > 
> > For media-ctl, it is easier to handle major/minor, in order to identify
> > the associated devnode name. Btw, media-ctl currently assumes that all
> > devnode devices are specified by v4l.major/v4l.minor.
> > 
> > Ok, maybe for alsa we'll need also card/device/subdevice, but I think this
> > should be mapped elsewhere, if this can't be retrieved via its sysfs/udev
> > interface (with seems to be doubtful).
> 
> The card/device tuple can likely be mapped to major/minor, but not
> subdevice. And since everything inside alsa is based on card/device I
> wouldn't change that.

I think we'll likely need changes for ALSA, but I don't think we know which 
ones yet. For that reason I'd avoid creating any ALSA-specific API for now 
until we implement proper support for ALSA devices. I'm fine, however, with 
deprecating the ALSA API we have now, before it gets (ab)used.

> >>> -		int dvb;
> >>> +		} dev;
> >>> 
> >>>  		/* Sub-device specifications */
> >>>  		/* Nothing needed yet */
> >>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> >>> index d847c760e8f0..418f4fec391a 100644
> >>> --- a/include/uapi/linux/media.h
> >>> +++ b/include/uapi/linux/media.h
> >>> @@ -78,6 +78,20 @@ struct media_entity_desc {
> >>>  		struct {
> >>>  			__u32 major;
> >>>  			__u32 minor;
> >>> +		} dev;
> >>> +
> >>> +#if 1
> >>> +		/*
> >>> +		 * DEPRECATED: previous node specifications. Kept just to
> >>> +		 * avoid breaking compilation, but media_entity_desc.dev
> >>> +		 * should be used instead. In particular, alsa and dvb
> >>> +		 * fields below are wrong: for all devnodes, there should
> >>> +		 * be just major/minor inside the struct, as this is enough
> >>> +		 * to represent any devnode, no matter what type.
> >>> +		 */
> >>> +		struct {
> >>> +			__u32 major;
> >>> +			__u32 minor;
> >>>  		} v4l;
> >>>  		struct {
> >>>  			__u32 major;
> >>> @@ -89,6 +103,7 @@ struct media_entity_desc {
> >>>  			__u32 subdevice;
> >>>  		} alsa;
> >>>  		int dvb;
> >> 
> >> I wouldn't merge all the v4l/fb/etc. structs into one struct. That will
> >> make it difficult in the future if you need to add a field for e.g. v4l
> >> entities.

This is something Hans and I have discussed at the FOSDEM, and ended up 
disagreeing.

I like the single struct dev here, but I believe it should be moved before the 
union, or, as Hans proposed, that the reserved bytes should be moved after the 
union. However, the reason why I like your proposal has probably nothing to do 
with your intents.

To start directly with the big news, I believe the MC device node entity types 
are bogus. They should never have been created in the first place, and your 
proposal for DVB support in MC clearly shows it in my opinion.

The media controller model is supposed to describe the device hardware 
topology, or at least a logical view of that topology mapped to the operating 
system from a hardware point of view. Device nodes, on the other hand, are 
purely Linux concepts, and have nothing to do with the hardware topology. What 
we have tried to describe with the MEDIA_ENT_T_DEVNODE_V4L entity type is 
essentially a DMA engine. As DMA engines are mapped 1:1 to device nodes in 
V4L2, we have mixed the two concepts and have called those entities device 
nodes, while they should really have been called DMA engines.

One additional clue that made me realize this is that we now have device nodes 
for subdevs, which we don't report through the MC API. That's wrong, and 
should be fixed. We could simply consider that V4L2 subdevs, as they're V4L2 
entities, can use the media_entity_desc v4l union and report the device node 
major and minor there, but I think that would be a short-term solution.

Instead, I believe we should start describing our media entities based on the 
hardware point of view, and consider device nodes to be an entity property 
instead of an entity identity. In other words, a V4L2 DMA engine is not a 
device node by hardware nature, but has the property of exposing a device node 
to userspace. The same applies to other entities, such as V4L2 subdevices or 
DVB entities.

I've used the word property for a reason, as we've discussed numerous times in 
the past an MC API extension to report detailed entity properties. However, 
considering that exposing a device node is a very common property for 
entities, I believe it makes sense to report that information through 
media_entity_desc instead of requiring a separate, not implemented yet ioctl.

For those reasons, I believe that we should have a single struct dev in struct 
media_entity_desc used to report the "entity exposes a device node" property. 
This isn't linked to v4l, fb, dvb or alsa, and is independent from the entity 
type as such, so I wouldn't describe that property with subsystem-specific 
structures.

This doesn't preclude struct media_entity_desc from exposing subsystem-
specific information in v4l, fb, dvb or alsa structures, even though I 
currently believe that only core properties should be exposed through that 
structure, and subsystem-specific properties would likely not qualified, but 
that should be decided on a case-by-case basis. The struct dev, however, isn't 
a subsystem-specific property, and I would thus like it to be reported as 
such. A flag could also be used to report whether the entity has a device 
node, although that could be inferred from major:minor not being equal to 0:0.

With the above explanation hopefully clear, I believe that the following DVB 
entity types are wrong.

#define MEDIA_ENT_T_DEVNODE_DVB_FE      (MEDIA_ENT_T_DEVNODE + 4)
#define MEDIA_ENT_T_DEVNODE_DVB_DEMUX   (MEDIA_ENT_T_DEVNODE + 5)
#define MEDIA_ENT_T_DEVNODE_DVB_DVR     (MEDIA_ENT_T_DEVNODE + 6)
#define MEDIA_ENT_T_DEVNODE_DVB_CA      (MEDIA_ENT_T_DEVNODE + 7)
#define MEDIA_ENT_T_DEVNODE_DVB_NET     (MEDIA_ENT_T_DEVNODE + 8)

We should get rid of DEVNODE_ there.

Let's take http://linuxtv.org/downloads/presentations/cx231xx.ps as an 
example. If I understand it correctly, the frontend has a dedicated device 
node for control purposes (it has no DMA engine, no data can be transferred 
through that device node), while the demux and dvr have DMA engines. The demux 
can pass data to the dvr DMA engine, and also capture "raw" data directly. 
Please correct me if that's not correct.

The frontend should in my opinion be represented as a green box, not a yellow 
box, as it's a processing entity and not a DMA engine. It should, however, 
expose its device node property through struct dev.

The dvr, on the other side, is a DMA engine, for which a yellow box is thus 
correct. Its device node used for data transmission (and I suppose also some 
DMA-related configuration) should also be exposed through struct dev.

The demux is, if my understanding is correct, a hybrid beast, with both 
processing and DMA capabilities. I lack proper DVB knowledge here, but based 
on what I understand it might make sense to split it in two entities, one to 
handle the data processing capability, and the other one to handle the DMA 
engine. The processing entity could have one sink pad and two source pads, one 
connected to the dvr and the other one connected to the demux DMA engine 
entity.

This model departs from your proposal, but I think it would be a better base 
for future developments, for DVB but also other subsystems.

> > No. You could just create another union for the API-specific bits, using
> > the reserved bytes.
> > 
> >> So I would keep the v4l, fb and alsa structs, and just add a new struct
> >> for dvb. I wonder if the dvb field can't just be replaced since I doubt
> >> anyone is using it. And even if someone does, then it can't be right
> >> since a single int isn't enough and never worked anyway.
> > 
> > All devnodes have major/minor. Making it standard for all devices makes
> > easy for userspace to properly get the data it requires to work.
> 
> I think you are making assumptions here that may not be true. I don't see
> any reason to make a 'dev' struct here. The real problem is the dvb int, so
> that's what needs to be addressed. Changing anything else will cause API
> headaches for no good reason.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Mauro Carvalho Chehab]

Em Mon, 23 Feb 2015 23:20:15 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Monday 23 February 2015 10:55:08 Mauro Carvalho Chehab wrote:
> > Em Mon, 26 Jan 2015 09:41:41 -0500 Devin Heitmueller escreveu:
> > >> It is actually trivial to get the device nodes once you have the
> > >> major/minor. The media-ctl library does that for you. See:
> > >
> > > No objection then.
> > > 
> > > On a related note, you would be very well served to consider testing
> > > your dvb changes with a device that has more than one DVB tuner (such
> > > as the hvr-2200/2250).  That will help you shake out any edge cases
> > > related to ensuring that the different DVB nodes appear in different
> > > groups.
> > 
> > Hi Devin,
> > 
> > I did some tests (and fixes) for WinTV Nova-TD, with has two adapters.
> > 
> > I saw two alternatives for it:
> > 
> > 1) to create a media controller device for each adapter;
> > 2) to create just one media controller.
> > 
> > I actually implemented (1), as, in the case of this device, AFAIKT, the
> > two devices are indepentent, e. g. it is not possible to, for example,
> > share the same tuner with two demods:
> > 
> > $ ls -la /dev/media?
> > crw-rw----. 1 root video 249, 0 Fev 23 10:02 /dev/media0
> > crw-rw----. 1 root video 249, 1 Fev 23 10:02 /dev/media1
> > 
> > The adapter 0 corresponds to /dev/media0, and the adapter 1
> > to /dev/media1:
> > 
> > $ media-ctl --print-dot -d /dev/media0
> > digraph board {
> > 	rankdir=TB
> > 	n00000001 [label="dvb-demux\n/dev/dvb/adapter0/demux0", shape=box,
> > style=filled, fillcolor=yellow] n00000001 -> n00000002
> > 	n00000002 [label="dvb-dvr\n/dev/dvb/adapter0/dvr0", shape=box,
> > style=filled, fillcolor=yellow] n00000003
> > [label="dvb-net\n/dev/dvb/adapter0/net0", shape=box, style=filled,
> > fillcolor=yellow] n00000004 [label="DiBcom
> > 7000PC\n/dev/dvb/adapter0/frontend0", shape=box, style=filled,
> > fillcolor=yellow] n00000004 -> n00000001
> > }
> > 
> > $ media-ctl --print-dot -d /dev/media1
> > digraph board {
> > 	rankdir=TB
> > 	n00000001 [label="dvb-demux\n/dev/dvb/adapter1/demux0", shape=box,
> > style=filled, fillcolor=yellow] n00000001 -> n00000002
> > 	n00000002 [label="dvb-dvr\n/dev/dvb/adapter1/dvr0", shape=box,
> > style=filled, fillcolor=yellow] n00000003
> > [label="dvb-net\n/dev/dvb/adapter1/net0", shape=box, style=filled,
> > fillcolor=yellow] n00000004 [label="DiBcom
> > 7000PC\n/dev/dvb/adapter1/frontend0", shape=box, style=filled,
> > fillcolor=yellow] n00000004 -> n00000001
> > }
> > 
> > On a more complex hardware where some components may be rewired
> > on a different way, however, using just one media controller
> > would be a better approach.
> > 
> > Comments?
> 
> A few, yes :-)
> 
> There's surprisingly many "details" that are not fully specified in MC, and 
> this is one of them. Historically the design idea was to create one media 
> device per instance of master video device. For PCI devices that's roughly one 
> media device per card, for platform devices one media device per master IP 
> core (or group of IP cores) instance, and for USB devices one media device per 
> USB control interface.
> 
> We can depart from that model in two ways.
> 
> As you mentioned above, it could make sense to create separate media device 
> instances for a single master device (USB in this case) when the master device 
> contains several completely independent pipelines. Completely independent 
> means that no data link connects the two pipelines, and that no resource is 
> shared between them in a way that affects the device's behaviour.
> 
> In the example above the only shared hardware resource seems to be the USB 
> bridge chip, which implements two independent DMA channels through the same 
> USB interface. If the DMA channels are really independent, in the sense that 
> they have no influence on each other such as e.g. bandwidth sharing, then two 
> media devices could be created. Note that there's no requirement to do so, 
> creating a single media device in this case is still perfectly valid.

Ok, so both ways can be applied on this case. I'll think a little bit more
about it.

> It should also be noted that split pipelines could be difficult to represent 
> as independent media devices when an external chip is shared between the two 
> pipelines, even if that chip is itself split in two independent parts. This is 
> caused by how the kernel APIs used to manage composite devices (v4l2-async and 
> the component framework) handle components. For instance, in the DT case, we 
> can't use v4l2-async to bind to one of the subdevs create for a single I2C 
> chip or IP core, as they would share the same hardware identifier (device 
> name, DT node, ...) used to match subdevs. Arguably that's a framework issue 
> that should be fixed, but it wouldn't be trivial.
> 
> The second way to depart from the existing model is unrelated to the DVB 
> examples above, but is still worth mentioning for completeness. When several 
> master devices share resources (either on-chip or off-chip), a single media 
> device is required. I've discussed such a case with Jean-Michel Hautbois and 
> Hans Verkuil at the FOSDEM for the i.MX6 SoC. The chip includes two 
> independent IPUs (Image Processing Units), with two independent parallel 
> receivers but one shared CSI-2 receiver. On the system Jean-Michel is working 
> with, an external FPGA is connected to the two parallel receivers. The 
> resulting media pipeline thus includes the FPGA, the CSI-2 receiver and the 
> two IPUs, requiring a single media device to cover both IPUs. As the IPU 
> driver is the master driver in this case, it would normally create one media 
> device per hardware device instance.
> 
> A similar situation occurs when video capture devices, memory to memory 
> devices and/or display devices are connected together through deep pipelining. 
> Such an example can be found in the Renesas R-Car Gen2 SoCs, where a memory to 
> memory video processing device (VSP1) has one output connected directly to the 
> display engine (DU). The VSP1 and DU are otherwise completely separate IP 
> cores, with the former supported by a V4L2 driver and the latter by a DRM/KMS 
> driver. We also need a single media device to cover the whole graph, without 
> intrusive changes in either the VSP1 or the DU driver as the two devices can 
> operate completely independently and without both being present.

We have the same case on some DVB hardware, used on TV sets and Set Top Boxes,
but the ones currently mapped provide an independent graph for DVB than the one
for DRM/KMS.

This is something that we could discuss further during the Media Summit.

Regards,
Mauro

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Mauro Carvalho Chehab]

Em Mon, 23 Feb 2015 23:20:15 +0200
Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> escreveu:

> Hi Mauro,
> 
> On Monday 23 February 2015 10:55:08 Mauro Carvalho Chehab wrote:
> > Em Mon, 26 Jan 2015 09:41:41 -0500 Devin Heitmueller escreveu:
> > >> It is actually trivial to get the device nodes once you have the
> > >> major/minor. The media-ctl library does that for you. See:
> > >
> > > No objection then.
> > > 
> > > On a related note, you would be very well served to consider testing
> > > your dvb changes with a device that has more than one DVB tuner (such
> > > as the hvr-2200/2250).  That will help you shake out any edge cases
> > > related to ensuring that the different DVB nodes appear in different
> > > groups.
> > 
> > Hi Devin,
> > 
> > I did some tests (and fixes) for WinTV Nova-TD, with has two adapters.
> > 
> > I saw two alternatives for it:
> > 
> > 1) to create a media controller device for each adapter;
> > 2) to create just one media controller.
> > 
> > I actually implemented (1), as, in the case of this device, AFAIKT, the
> > two devices are indepentent, e. g. it is not possible to, for example,
> > share the same tuner with two demods:
> > 
> > $ ls -la /dev/media?
> > crw-rw----. 1 root video 249, 0 Fev 23 10:02 /dev/media0
> > crw-rw----. 1 root video 249, 1 Fev 23 10:02 /dev/media1
> > 
> > The adapter 0 corresponds to /dev/media0, and the adapter 1
> > to /dev/media1:
> > 
> > $ media-ctl --print-dot -d /dev/media0
> > digraph board {
> > 	rankdir=TB
> > 	n00000001 [label="dvb-demux\n/dev/dvb/adapter0/demux0", shape=box,
> > style=filled, fillcolor=yellow] n00000001 -> n00000002
> > 	n00000002 [label="dvb-dvr\n/dev/dvb/adapter0/dvr0", shape=box,
> > style=filled, fillcolor=yellow] n00000003
> > [label="dvb-net\n/dev/dvb/adapter0/net0", shape=box, style=filled,
> > fillcolor=yellow] n00000004 [label="DiBcom
> > 7000PC\n/dev/dvb/adapter0/frontend0", shape=box, style=filled,
> > fillcolor=yellow] n00000004 -> n00000001
> > }
> > 
> > $ media-ctl --print-dot -d /dev/media1
> > digraph board {
> > 	rankdir=TB
> > 	n00000001 [label="dvb-demux\n/dev/dvb/adapter1/demux0", shape=box,
> > style=filled, fillcolor=yellow] n00000001 -> n00000002
> > 	n00000002 [label="dvb-dvr\n/dev/dvb/adapter1/dvr0", shape=box,
> > style=filled, fillcolor=yellow] n00000003
> > [label="dvb-net\n/dev/dvb/adapter1/net0", shape=box, style=filled,
> > fillcolor=yellow] n00000004 [label="DiBcom
> > 7000PC\n/dev/dvb/adapter1/frontend0", shape=box, style=filled,
> > fillcolor=yellow] n00000004 -> n00000001
> > }
> > 
> > On a more complex hardware where some components may be rewired
> > on a different way, however, using just one media controller
> > would be a better approach.
> > 
> > Comments?
> 
> A few, yes :-)
> 
> There's surprisingly many "details" that are not fully specified in MC, and 
> this is one of them. Historically the design idea was to create one media 
> device per instance of master video device. For PCI devices that's roughly one 
> media device per card, for platform devices one media device per master IP 
> core (or group of IP cores) instance, and for USB devices one media device per 
> USB control interface.
> 
> We can depart from that model in two ways.
> 
> As you mentioned above, it could make sense to create separate media device 
> instances for a single master device (USB in this case) when the master device 
> contains several completely independent pipelines. Completely independent 
> means that no data link connects the two pipelines, and that no resource is 
> shared between them in a way that affects the device's behaviour.
> 
> In the example above the only shared hardware resource seems to be the USB 
> bridge chip, which implements two independent DMA channels through the same 
> USB interface. If the DMA channels are really independent, in the sense that 
> they have no influence on each other such as e.g. bandwidth sharing, then two 
> media devices could be created. Note that there's no requirement to do so, 
> creating a single media device in this case is still perfectly valid.

Ok, so both ways can be applied on this case. I'll think a little bit more
about it.

> It should also be noted that split pipelines could be difficult to represent 
> as independent media devices when an external chip is shared between the two 
> pipelines, even if that chip is itself split in two independent parts. This is 
> caused by how the kernel APIs used to manage composite devices (v4l2-async and 
> the component framework) handle components. For instance, in the DT case, we 
> can't use v4l2-async to bind to one of the subdevs create for a single I2C 
> chip or IP core, as they would share the same hardware identifier (device 
> name, DT node, ...) used to match subdevs. Arguably that's a framework issue 
> that should be fixed, but it wouldn't be trivial.
> 
> The second way to depart from the existing model is unrelated to the DVB 
> examples above, but is still worth mentioning for completeness. When several 
> master devices share resources (either on-chip or off-chip), a single media 
> device is required. I've discussed such a case with Jean-Michel Hautbois and 
> Hans Verkuil at the FOSDEM for the i.MX6 SoC. The chip includes two 
> independent IPUs (Image Processing Units), with two independent parallel 
> receivers but one shared CSI-2 receiver. On the system Jean-Michel is working 
> with, an external FPGA is connected to the two parallel receivers. The 
> resulting media pipeline thus includes the FPGA, the CSI-2 receiver and the 
> two IPUs, requiring a single media device to cover both IPUs. As the IPU 
> driver is the master driver in this case, it would normally create one media 
> device per hardware device instance.
> 
> A similar situation occurs when video capture devices, memory to memory 
> devices and/or display devices are connected together through deep pipelining. 
> Such an example can be found in the Renesas R-Car Gen2 SoCs, where a memory to 
> memory video processing device (VSP1) has one output connected directly to the 
> display engine (DU). The VSP1 and DU are otherwise completely separate IP 
> cores, with the former supported by a V4L2 driver and the latter by a DRM/KMS 
> driver. We also need a single media device to cover the whole graph, without 
> intrusive changes in either the VSP1 or the DU driver as the two devices can 
> operate completely independently and without both being present.

We have the same case on some DVB hardware, used on TV sets and Set Top Boxes,
but the ones currently mapped provide an independent graph for DVB than the one
for DRM/KMS.

This is something that we could discuss further during the Media Summit.

Regards,
Mauro

Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API

[Mauro Carvalho Chehab]

Em Tue, 24 Feb 2015 00:58:23 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro and Hans,
> 
> On Monday 26 January 2015 14:41:53 Hans Verkuil wrote:
> > On 01/26/2015 02:34 PM, Mauro Carvalho Chehab wrote:
> > > Em Mon, 26 Jan 2015 14:11:50 +0100 Hans Verkuil escreveu:
> > >> On 01/26/2015 01:47 PM, Mauro Carvalho Chehab wrote:
> > >>> The previous provision for DVB media controller support were to
> > >>> define an ID (likely meaning the adapter number) for the DVB
> > >>> devnodes.
> > >>> 
> > >>> This is just plain wrong. Just like V4L, DVB devices (and ALSA,
> > >>> or whatever) are identified via a (major, minor) tuple.
> > >>> 
> > >>> This is enough to uniquely identify a devnode, no matter what
> > >>> API it implements.
> > >>> 
> > >>> So, before we go too far, let's mark the old v4l, dvb and alsa
> > >>> "devnode" info as deprecated, and just call it as "dev".
> > >>> 
> > >>> As we don't want to break compilation on already existing apps,
> > >>> let's just keep the old definitions as-is, adding a note that
> > >>> those are deprecated at media-entity.h.
> > >>> 
> > >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> [snip]
> 
> > >>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > >>> index e00459185d20..d6d74bcfe183 100644
> > >>> --- a/include/media/media-entity.h
> > >>> +++ b/include/media/media-entity.h
> > >>> @@ -87,17 +87,7 @@ struct media_entity {
> > >>>  		struct {
> > >>>  			u32 major;
> > >>>  			u32 minor;
> > >>> -		} v4l;
> > >>> -		struct {
> > >>> -			u32 major;
> > >>> -			u32 minor;
> > >>> -		} fb;
> > >>> -		struct {
> > >>> -			u32 card;
> > >>> -			u32 device;
> > >>> -			u32 subdevice;
> > >>> -		} alsa;
> > >> 
> > >> I don't think the alsa entity information can be replaced by major/minor.
> > >> In particular you will loose the subdevice information which you need as
> > >> well. In addition, alsa devices are almost never referenced via major and
> > >> minor numbers, but always by card/device/subdevice numbers.
> > > 
> > > For media-ctl, it is easier to handle major/minor, in order to identify
> > > the associated devnode name. Btw, media-ctl currently assumes that all
> > > devnode devices are specified by v4l.major/v4l.minor.
> > > 
> > > Ok, maybe for alsa we'll need also card/device/subdevice, but I think this
> > > should be mapped elsewhere, if this can't be retrieved via its sysfs/udev
> > > interface (with seems to be doubtful).
> > 
> > The card/device tuple can likely be mapped to major/minor, but not
> > subdevice. And since everything inside alsa is based on card/device I
> > wouldn't change that.
> 
> I think we'll likely need changes for ALSA, but I don't think we know which 
> ones yet. For that reason I'd avoid creating any ALSA-specific API for now 
> until we implement proper support for ALSA devices. I'm fine, however, with 
> deprecating the ALSA API we have now, before it gets (ab)used.

That's my opinion too: deprecate it before it gets (ab)used.

> 
> > >>> -		int dvb;
> > >>> +		} dev;
> > >>> 
> > >>>  		/* Sub-device specifications */
> > >>>  		/* Nothing needed yet */
> > >>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > >>> index d847c760e8f0..418f4fec391a 100644
> > >>> --- a/include/uapi/linux/media.h
> > >>> +++ b/include/uapi/linux/media.h
> > >>> @@ -78,6 +78,20 @@ struct media_entity_desc {
> > >>>  		struct {
> > >>>  			__u32 major;
> > >>>  			__u32 minor;
> > >>> +		} dev;
> > >>> +
> > >>> +#if 1
> > >>> +		/*
> > >>> +		 * DEPRECATED: previous node specifications. Kept just to
> > >>> +		 * avoid breaking compilation, but media_entity_desc.dev
> > >>> +		 * should be used instead. In particular, alsa and dvb
> > >>> +		 * fields below are wrong: for all devnodes, there should
> > >>> +		 * be just major/minor inside the struct, as this is enough
> > >>> +		 * to represent any devnode, no matter what type.
> > >>> +		 */
> > >>> +		struct {
> > >>> +			__u32 major;
> > >>> +			__u32 minor;
> > >>>  		} v4l;
> > >>>  		struct {
> > >>>  			__u32 major;
> > >>> @@ -89,6 +103,7 @@ struct media_entity_desc {
> > >>>  			__u32 subdevice;
> > >>>  		} alsa;
> > >>>  		int dvb;
> > >> 
> > >> I wouldn't merge all the v4l/fb/etc. structs into one struct. That will
> > >> make it difficult in the future if you need to add a field for e.g. v4l
> > >> entities.
> 
> This is something Hans and I have discussed at the FOSDEM, and ended up 
> disagreeing.
> 
> I like the single struct dev here, but I believe it should be moved before the 
> union, or, as Hans proposed, that the reserved bytes should be moved after the 
> union. However, the reason why I like your proposal has probably nothing to do 
> with your intents.

We can't move it before the union, as it would otherwise break userspace.
The union should contain both the new name and the legacy ones.

Moving the reserved fields out of the union seems fine for me.

> To start directly with the big news, I believe the MC device node entity types 
> are bogus. They should never have been created in the first place, and your 
> proposal for DVB support in MC clearly shows it in my opinion.
> 
> The media controller model is supposed to describe the device hardware 
> topology, or at least a logical view of that topology mapped to the operating 
> system from a hardware point of view. Device nodes, on the other hand, are 
> purely Linux concepts, and have nothing to do with the hardware topology.

That's somewhat true, but all device nodes are the points where the hardware
can be controlled. Any userspace software that needs to control the hardware
need some way to detect the device nodes, in order to use them

> What 
> we have tried to describe with the MEDIA_ENT_T_DEVNODE_V4L entity type is 
> essentially a DMA engine. As DMA engines are mapped 1:1 to device nodes in 
> V4L2, we have mixed the two concepts and have called those entities device 
> nodes, while they should really have been called DMA engines.

As I said, it is, in practice, more than just the DMA, even for V4L2. It
is the control point of the device to where an specific API is used to
control the hardware.

Btw, calling them as DMA is a simplification. On simple devices, yes
there's a DMA directly associated with the devnode, but on devices like
USB, the DMA is actually hidden, and a piece of software at the Kernel
filters out the payload from the USB headers.

So, a devnode is actually several blocks:

	[hw control] or, eventually, [kernel control]
	[DMA] ---> [payload filter] ---> [userspace stream I/O]

> One additional clue that made me realize this is that we now have device nodes 
> for subdevs, which we don't report through the MC API. That's wrong, and 
> should be fixed.

A subdev node is actually:
	[hw control] or, eventually, [kernel control]

I agree that they should be reported via the MC API.

> We could simply consider that V4L2 subdevs, as they're V4L2 
> entities, can use the media_entity_desc v4l union and report the device node 
> major and minor there, but I think that would be a short-term solution.

> Instead, I believe we should start describing our media entities based on the 
> hardware point of view, and consider device nodes to be an entity property 
> instead of an entity identity. In other words, a V4L2 DMA engine is not a 
> device node by hardware nature, but has the property of exposing a device node 
> to userspace. The same applies to other entities, such as V4L2 subdevices or 
> DVB entities.

Well, for me a devnode has always an API associated to it, as all devnodes
allow some set of ioctls to use to control it.

So, for me, each media entity that is a devnode should have a property or
identity that will tell what API is supported there. This is so important
for userspace that I would keep using "type" for that.

Ok, some devnodes have more than just [hw control].

> I've used the word property for a reason, as we've discussed numerous times in 
> the past an MC API extension to report detailed entity properties. However, 
> considering that exposing a device node is a very common property for 
> entities, I believe it makes sense to report that information through 
> media_entity_desc instead of requiring a separate, not implemented yet ioctl.

Agreed.

> For those reasons, I believe that we should have a single struct dev in struct 
> media_entity_desc used to report the "entity exposes a device node" property. 
> This isn't linked to v4l, fb, dvb or alsa, and is independent from the entity 
> type as such, so I wouldn't describe that property with subsystem-specific 
> structures.

Agreed.

> This doesn't preclude struct media_entity_desc from exposing subsystem-
> specific information in v4l, fb, dvb or alsa structures, even though I 
> currently believe that only core properties should be exposed through that 
> structure, and subsystem-specific properties would likely not qualified, but 
> that should be decided on a case-by-case basis. The struct dev, however, isn't 
> a subsystem-specific property, and I would thus like it to be reported as 
> such. A flag could also be used to report whether the entity has a device 
> node, although that could be inferred from major:minor not being equal to 0:0.

Fully agreed.

> With the above explanation hopefully clear, I believe that the following DVB 
> entity types are wrong.
> 
> #define MEDIA_ENT_T_DEVNODE_DVB_FE      (MEDIA_ENT_T_DEVNODE + 4)
> #define MEDIA_ENT_T_DEVNODE_DVB_DEMUX   (MEDIA_ENT_T_DEVNODE + 5)
> #define MEDIA_ENT_T_DEVNODE_DVB_DVR     (MEDIA_ENT_T_DEVNODE + 6)
> #define MEDIA_ENT_T_DEVNODE_DVB_CA      (MEDIA_ENT_T_DEVNODE + 7)
> #define MEDIA_ENT_T_DEVNODE_DVB_NET     (MEDIA_ENT_T_DEVNODE + 8)
> 
> We should get rid of DEVNODE_ there.

I'm fine with such change.

> Let's take http://linuxtv.org/downloads/presentations/cx231xx.ps as an 
> example. If I understand it correctly, the frontend has a dedicated device 
> node for control purposes (it has no DMA engine, no data can be transferred 
> through that device node), while the demux and dvr have DMA engines. The demux 
> can pass data to the dvr DMA engine, and also capture "raw" data directly. 
> Please correct me if that's not correct.

Well, this is actually a simplification. It works fine for now, but
we'll need to go deeper in some future to properly describe the DVB
pipeline.

The frontend is actually composed of two parts:
	- A tuner;
	- A digital TV demodulator.

(On some devices, it is impossible to distinguish between them,
as the tuner is controlled by the firmware inside the demod)

The demodulator has a serial or parallel bus that either sends
the data to a hardware demux or sends it to the Kernel via DMA.

In the first case, the diagram is:

	[tuner] ---> [demod] --> [hw demux] --> [DMA] --> Kernel --> userspace (either via dmx or dvr devnode)

In the second case, it is:

	[tuner] ---> [demod] --> [DMA] --> Kernel --> [sw demux in Kernel] --> userspace (either via dmx or dvr devnode)

The DMA is thus hidden in the middle of the pipeline.

For userspace, it doesn't matter if the hardware has or not a demux.
The logic is that the DVB Demux API is used to set a filter at the
demux.

The demux filter will select the packet IDs (PID) that userspace
wants from the MPEG-TS stream, and either dynamically wire the hardware
to send the selected program via DMA (for hardware demux) or it will
program the Kernel to filter it in kernelspace.

Btw, the above diagrams are for PC typical hardware. For Set Top Boxes
and TV sets, in order to meet DRM (Digital Rights Management)
requirements at the DVB specs, they may not have any DMA engine. In
such case, the demuxed video should be passed to a CA module (CAM), that
handles the decrypt, and then the output should go to the video adapter
for display.

So, the diagram would be something like:

	[tuner] ---> [demod] --> [hw demux] --> [CAM] --> [/dev/fb0]

In such case, all device nodes are just for hardware control.

To make things a little worse, it is not [hw demux] but, actually
[hw demux filter #0] ... [[hw demux filter #n].

A typical PC hardware with hw demux implements something like 32 filters.

A TV set hardware supports a way more filters and some can even have
dynamically created filters that can be shared between different DVB adapters.

> The frontend should in my opinion be represented as a green box, not a yellow 
> box, as it's a processing entity and not a DMA engine. It should, however, 
> expose its device node property through struct dev.

I see your point.

> The dvr, on the other side, is a DMA engine, for which a yellow box is thus 
> correct. Its device node used for data transmission (and I suppose also some 
> DMA-related configuration) should also be exposed through struct dev.

A dvr interface is always associated with the stream output to userspace.

> 
> The demux is, if my understanding is correct, a hybrid beast, with both 
> processing and DMA capabilities.

Yes. it is hw/sw control and can be stream output too.

> I lack proper DVB knowledge here, but based 
> on what I understand it might make sense to split it in two entities, one to 
> handle the data processing capability, and the other one to handle the DMA 
> engine. The processing entity could have one sink pad and two source pads, one 
> connected to the dvr and the other one connected to the demux DMA engine 
> entity.

Actually, the processing unity can have one sink pad with the TS input, and
a dynamically created number of PADs, one for each filter.

Please notice that a filter is associated with a file descriptor, and not
with a devnode.

So, from devnode perspective, we have:

	/dev/adapter0/frontend0 - to control the tuner/demod
	/dev/adapter0/demux0 - to create the filters
	/dev/adapter0/dvr0 - to receive the data
	/dev/adapter0/ca0 - to control data decrypt

but, from the data flux, we have:

	frontend0 -> demux0 input pad

	demux0 output pad filter 0 --> DMA --> dvr0 fd 5 
	demux0 output pad filter 1 --> DMA --> dvr0 fd 6
	demux0 output pad filter 2 --> DMA --> dvr0 fd 7
	demux0 output pad filter 3 --> ca0 --> DMA --> dvr0 fd 8
	demux0 output pad filter 4 --> /dev/fb0
	...

So, what we're really mapping right now is the hardware/software
control points associated with the device nodes. So, the way I see
is that we'll still need to represent at the media controller
the device nodes as such, as this is the point where the hardware
(and the Kernel) can be controlled.

The data flux can be simplified as we're doing right now, but we'll
need to add more features at the media controller (and to better
discuss) how to implement the DVB data flux.

In other words, for DVB, we'll need to represent:

- all devnodes used to control the DVB API (that's the goal of the
  current patchset);

- the demux processing unit with their filter outputs and the userspace
  file descriptors. I intend to discuss this further during the
  Media summit, and should be object of another set of patches.

> This model departs from your proposal, but I think it would be a better base 
> for future developments, for DVB but also other subsystems.
> 
> > > No. You could just create another union for the API-specific bits, using
> > > the reserved bytes.
> > > 
> > >> So I would keep the v4l, fb and alsa structs, and just add a new struct
> > >> for dvb. I wonder if the dvb field can't just be replaced since I doubt
> > >> anyone is using it. And even if someone does, then it can't be right
> > >> since a single int isn't enough and never worked anyway.
> > > 
> > > All devnodes have major/minor. Making it standard for all devices makes
> > > easy for userspace to properly get the data it requires to work.
> > 
> > I think you are making assumptions here that may not be true. I don't see
> > any reason to make a 'dev' struct here. The real problem is the dvb int, so
> > that's what needs to be addressed. Changing anything else will cause API
> > headaches for no good reason.
>