[PATCH] [media] v4l2-core: create MC interfaces for devnodes

[PATCH] [media] v4l2-core: create MC interfaces for devnodes

[Mauro Carvalho Chehab]

All V4L2 device nodes should create an interface, if the
Media Controller support is enabled.

Please notice that radio devices should not create an
entity, as radio input/output is either via wires or
via ALSA.

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

---

With this patch, V4L is now creating interfaces for all device nodes,
using the proper types:

$ ./mc_nextgen_test |grep -e dev -e entity#[1-5]\\b -e entity#262\\b
entity entity#1: au8522 19-0047, num pads = 3
entity entity#2: Xceive XC5000, num pads = 1
entity entity#3: au0828a video, num pads = 1
entity entity#4: au0828a vbi, num pads = 1
entity entity#5: Auvitek AU8522 QAM/8VSB Frontend, num pads = 2
entity entity#262: dvb-demux, num pads = 257
interface intf_devnode#1: video (81,0)
interface intf_devnode#2: vbi (81,1)
interface intf_devnode#3: v4l2_subdev (81,2)
interface intf_devnode#4: frontend (212,0)
interface intf_devnode#5: demux (212,1)
interface intf_devnode#6: DVR (212,2)
interface intf_devnode#7: dvbnet (212,3)
link link#1: intf_devnode#1 and entity#3
link link#2: intf_devnode#2 and entity#4
link link#3: intf_devnode#3 and entity#2
link link#4: intf_devnode#4 and entity#5
link link#5: intf_devnode#5 and entity#262
link link#1034: intf_devnode#4 and entity#2
link link#1035: intf_devnode#6 and entity#262

However, it should be noticed that there are missing links there, as the
interfaces intf_devnode#1: and intf_devnode#2: should also be linked to
the analog entities (e. g. entity#1 to entity#5).

It is hard to implememt that, however, as some platform drivers don't
have such connections. There are two issues to be solved here:

1) how the V4L2 core will identify that it could automatically create
   links to the other analog interfaces;

2) Where to place such code.

Let's do it on separate patches.


diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 44b330589787..472117f32c05 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -713,6 +713,78 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			BASE_VIDIOC_PRIVATE);
 }
 
+
+static int video_register_media_controller(struct video_device *vdev, int type)
+{
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	u32 entity_type, intf_type;
+	int ret;
+	bool create_entity = true;
+
+	if (!vdev->v4l2_dev->mdev)
+		return 0;
+
+	switch (type) {
+	case VFL_TYPE_GRABBER:
+		intf_type = MEDIA_INTF_T_V4L_VIDEO;
+		entity_type = MEDIA_ENT_T_V4L2_VIDEO;
+		break;
+	case VFL_TYPE_VBI:
+		intf_type = MEDIA_INTF_T_V4L_VBI;
+		entity_type = MEDIA_ENT_T_V4L2_VBI;
+		break;
+	case VFL_TYPE_SDR:
+		intf_type = MEDIA_INTF_T_V4L_SWRADIO;
+		entity_type = MEDIA_ENT_T_V4L2_SWRADIO;
+		break;
+	case VFL_TYPE_RADIO:
+		intf_type = MEDIA_INTF_T_V4L_RADIO;
+		/*
+		 * Radio doesn't have an entity at the V4L2 side to represent
+		 * radio input or output. Instead, the audio input/output goes
+		 * via either physical wires or ALSA.
+		 */
+		create_entity = false;
+		break;
+	default: /* Only type left is subdevs */
+		/* Subdevs are created via v4l2_device_register_subdev_nodes */
+		return 0;
+	}
+
+	if (create_entity) {
+		vdev->entity.type = entity_type;
+		vdev->entity.name = vdev->name;
+		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) {
+			printk(KERN_WARNING
+				"%s: media_device_register_entity failed\n",
+				__func__);
+			return ret;
+		}
+	}
+
+	vdev->intf_devnode = media_devnode_create(vdev->v4l2_dev->mdev,
+						  intf_type,
+						  0, VIDEO_MAJOR,
+						  vdev->minor,
+						  GFP_KERNEL);
+	if (!vdev->intf_devnode)
+		return -ENOMEM;
+
+	if (create_entity)
+		media_create_intf_link(&vdev->entity,
+				       &vdev->intf_devnode->intf, 0);
+
+	/* FIXME: how to create the other interface links? */
+
+#endif
+	return 0;
+}
+
 /**
  *	__video_register_device - register video4linux devices
  *	@vdev: video device structure we want to register
@@ -908,22 +980,9 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
 	/* Increase v4l2_device refcount */
 	v4l2_device_get(vdev->v4l2_dev);
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
 	/* Part 5: Register the entity. */
-	if (vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
-		vdev->entity.type = MEDIA_ENT_T_V4L2_VIDEO;
-		vdev->entity.name = vdev->name;
-		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)
-			printk(KERN_WARNING
-			       "%s: media_device_register_entity failed\n",
-			       __func__);
-	}
-#endif
+	ret = video_register_media_controller(vdev, type);
+
 	/* Part 6: Activate this minor. The char device can now be used. */
 	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
 
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 1e5176c558bf..2124a0e793f3 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -251,18 +251,18 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 		sd->entity.info.dev.major = VIDEO_MAJOR;
 		sd->entity.info.dev.minor = vdev->minor;
 
-		sd->intf_devnode = media_devnode_create(sd->entity.graph_obj.mdev,
+		vdev->intf_devnode = media_devnode_create(sd->entity.graph_obj.mdev,
 							MEDIA_INTF_T_V4L_SUBDEV,
 							0, VIDEO_MAJOR,
 							vdev->minor,
 							GFP_KERNEL);
-		if (!sd->intf_devnode) {
+		if (!vdev->intf_devnode) {
 			err = -ENOMEM;
 			kfree(vdev);
 			goto clean_up;
 		}
 
-		media_create_intf_link(&sd->entity, &sd->intf_devnode->intf, 0);
+		media_create_intf_link(&sd->entity, &vdev->intf_devnode->intf, 0);
 #endif
 		sd->devnode = vdev;
 	}
@@ -282,6 +282,7 @@ EXPORT_SYMBOL_GPL(v4l2_device_register_subdev_nodes);
 void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 {
 	struct v4l2_device *v4l2_dev;
+	struct video_device *vdev = sd->devnode;
 
 	/* return if it isn't registered */
 	if (sd == NULL || sd->v4l2_dev == NULL)
@@ -300,7 +301,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	if (v4l2_dev->mdev) {
 		media_entity_remove_links(&sd->entity);
-		media_devnode_remove(sd->intf_devnode);
+		media_devnode_remove(vdev->intf_devnode);
 		media_device_unregister_entity(&sd->entity);
 	}
 #endif
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index acbcd2f5fe7f..eeabf20e87a6 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -86,6 +86,7 @@ struct video_device
 {
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	struct media_entity entity;
+	struct media_intf_devnode *intf_devnode;
 #endif
 	/* device ops */
 	const struct v4l2_file_operations *fops;
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 1aa44f11eeb5..370fc38c34f1 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -584,7 +584,6 @@ struct v4l2_subdev_platform_data {
 struct v4l2_subdev {
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	struct media_entity entity;
-	struct media_intf_devnode *intf_devnode;
 #endif
 	struct list_head list;
 	struct module *owner;
-- 
2.4.3

Re: [PATCH] [media] v4l2-core: create MC interfaces for devnodes

[Hans Verkuil]

On 08/24/2015 03:19 PM, Mauro Carvalho Chehab wrote:
> All V4L2 device nodes should create an interface, if the
> Media Controller support is enabled.
> 
> Please notice that radio devices should not create an
> entity, as radio input/output is either via wires or
> via ALSA.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> ---
> 
> With this patch, V4L is now creating interfaces for all device nodes,
> using the proper types:
> 
> $ ./mc_nextgen_test |grep -e dev -e entity#[1-5]\\b -e entity#262\\b
> entity entity#1: au8522 19-0047, num pads = 3
> entity entity#2: Xceive XC5000, num pads = 1
> entity entity#3: au0828a video, num pads = 1
> entity entity#4: au0828a vbi, num pads = 1
> entity entity#5: Auvitek AU8522 QAM/8VSB Frontend, num pads = 2
> entity entity#262: dvb-demux, num pads = 257
> interface intf_devnode#1: video (81,0)
> interface intf_devnode#2: vbi (81,1)
> interface intf_devnode#3: v4l2_subdev (81,2)
> interface intf_devnode#4: frontend (212,0)
> interface intf_devnode#5: demux (212,1)
> interface intf_devnode#6: DVR (212,2)
> interface intf_devnode#7: dvbnet (212,3)
> link link#1: intf_devnode#1 and entity#3
> link link#2: intf_devnode#2 and entity#4
> link link#3: intf_devnode#3 and entity#2
> link link#4: intf_devnode#4 and entity#5
> link link#5: intf_devnode#5 and entity#262
> link link#1034: intf_devnode#4 and entity#2
> link link#1035: intf_devnode#6 and entity#262
> 
> However, it should be noticed that there are missing links there, as the
> interfaces intf_devnode#1: and intf_devnode#2: should also be linked to
> the analog entities (e. g. entity#1 to entity#5).
> 
> It is hard to implememt that, however, as some platform drivers don't
> have such connections. There are two issues to be solved here:
> 
> 1) how the V4L2 core will identify that it could automatically create
>    links to the other analog interfaces;
> 
> 2) Where to place such code.
> 
> Let's do it on separate patches.

Agreed.

I would add that we shouldn't attempt to be 100% perfect in automatically
creating links. If we can do 80-90% of the drivers that way, then that would
be great. The remaining drivers will then need to be modified manually.

> 
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 44b330589787..472117f32c05 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -713,6 +713,78 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  			BASE_VIDIOC_PRIVATE);
>  }
>  
> +
> +static int video_register_media_controller(struct video_device *vdev, int type)
> +{
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	u32 entity_type, intf_type;
> +	int ret;
> +	bool create_entity = true;
> +
> +	if (!vdev->v4l2_dev->mdev)
> +		return 0;
> +
> +	switch (type) {
> +	case VFL_TYPE_GRABBER:
> +		intf_type = MEDIA_INTF_T_V4L_VIDEO;
> +		entity_type = MEDIA_ENT_T_V4L2_VIDEO;
> +		break;
> +	case VFL_TYPE_VBI:
> +		intf_type = MEDIA_INTF_T_V4L_VBI;
> +		entity_type = MEDIA_ENT_T_V4L2_VBI;
> +		break;
> +	case VFL_TYPE_SDR:
> +		intf_type = MEDIA_INTF_T_V4L_SWRADIO;
> +		entity_type = MEDIA_ENT_T_V4L2_SWRADIO;
> +		break;
> +	case VFL_TYPE_RADIO:
> +		intf_type = MEDIA_INTF_T_V4L_RADIO;
> +		/*
> +		 * Radio doesn't have an entity at the V4L2 side to represent
> +		 * radio input or output. Instead, the audio input/output goes
> +		 * via either physical wires or ALSA.
> +		 */
> +		create_entity = false;
> +		break;
> +	default: /* Only type left is subdevs */
> +		/* Subdevs are created via v4l2_device_register_subdev_nodes */

As mentioned in the other patch: why can't you create the interface for subdevs
as well?

> +		return 0;
> +	}
> +
> +	if (create_entity) {
> +		vdev->entity.type = entity_type;
> +		vdev->entity.name = vdev->name;
> +		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) {
> +			printk(KERN_WARNING
> +				"%s: media_device_register_entity failed\n",
> +				__func__);
> +			return ret;
> +		}
> +	}
> +
> +	vdev->intf_devnode = media_devnode_create(vdev->v4l2_dev->mdev,
> +						  intf_type,
> +						  0, VIDEO_MAJOR,
> +						  vdev->minor,
> +						  GFP_KERNEL);
> +	if (!vdev->intf_devnode)
> +		return -ENOMEM;
> +
> +	if (create_entity)
> +		media_create_intf_link(&vdev->entity,
> +				       &vdev->intf_devnode->intf, 0);
> +
> +	/* FIXME: how to create the other interface links? */
> +
> +#endif
> +	return 0;
> +}
> +
>  /**
>   *	__video_register_device - register video4linux devices
>   *	@vdev: video device structure we want to register
> @@ -908,22 +980,9 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
>  	/* Increase v4l2_device refcount */
>  	v4l2_device_get(vdev->v4l2_dev);
>  
> -#if defined(CONFIG_MEDIA_CONTROLLER)
>  	/* Part 5: Register the entity. */
> -	if (vdev->v4l2_dev->mdev &&
> -	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
> -		vdev->entity.type = MEDIA_ENT_T_V4L2_VIDEO;
> -		vdev->entity.name = vdev->name;
> -		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)
> -			printk(KERN_WARNING
> -			       "%s: media_device_register_entity failed\n",
> -			       __func__);
> -	}
> -#endif
> +	ret = video_register_media_controller(vdev, type);
> +
>  	/* Part 6: Activate this minor. The char device can now be used. */
>  	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
>  

I'm missing changes to v4l2_device_release() to clean up the interface and
entity.

> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index 1e5176c558bf..2124a0e793f3 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -251,18 +251,18 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
>  		sd->entity.info.dev.major = VIDEO_MAJOR;
>  		sd->entity.info.dev.minor = vdev->minor;
>  
> -		sd->intf_devnode = media_devnode_create(sd->entity.graph_obj.mdev,
> +		vdev->intf_devnode = media_devnode_create(sd->entity.graph_obj.mdev,
>  							MEDIA_INTF_T_V4L_SUBDEV,
>  							0, VIDEO_MAJOR,
>  							vdev->minor,
>  							GFP_KERNEL);
> -		if (!sd->intf_devnode) {
> +		if (!vdev->intf_devnode) {
>  			err = -ENOMEM;
>  			kfree(vdev);
>  			goto clean_up;
>  		}
>  
> -		media_create_intf_link(&sd->entity, &sd->intf_devnode->intf, 0);
> +		media_create_intf_link(&sd->entity, &vdev->intf_devnode->intf, 0);
>  #endif
>  		sd->devnode = vdev;
>  	}
> @@ -282,6 +282,7 @@ EXPORT_SYMBOL_GPL(v4l2_device_register_subdev_nodes);
>  void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
>  {
>  	struct v4l2_device *v4l2_dev;
> +	struct video_device *vdev = sd->devnode;
>  
>  	/* return if it isn't registered */
>  	if (sd == NULL || sd->v4l2_dev == NULL)
> @@ -300,7 +301,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	if (v4l2_dev->mdev) {
>  		media_entity_remove_links(&sd->entity);
> -		media_devnode_remove(sd->intf_devnode);
> +		media_devnode_remove(vdev->intf_devnode);
>  		media_device_unregister_entity(&sd->entity);
>  	}
>  #endif
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index acbcd2f5fe7f..eeabf20e87a6 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -86,6 +86,7 @@ struct video_device
>  {
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	struct media_entity entity;
> +	struct media_intf_devnode *intf_devnode;
>  #endif
>  	/* device ops */
>  	const struct v4l2_file_operations *fops;
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1aa44f11eeb5..370fc38c34f1 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -584,7 +584,6 @@ struct v4l2_subdev_platform_data {
>  struct v4l2_subdev {
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	struct media_entity entity;
> -	struct media_intf_devnode *intf_devnode;
>  #endif
>  	struct list_head list;
>  	struct module *owner;
> 

Regards,

	Hans

Re: [PATCH] [media] v4l2-core: create MC interfaces for devnodes

[Mauro Carvalho Chehab]

Em Fri, 28 Aug 2015 16:24:56 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 08/24/2015 03:19 PM, Mauro Carvalho Chehab wrote:
> > All V4L2 device nodes should create an interface, if the
> > Media Controller support is enabled.
> > 
> > Please notice that radio devices should not create an
> > entity, as radio input/output is either via wires or
> > via ALSA.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > 
> > ---
> > 
> > With this patch, V4L is now creating interfaces for all device nodes,
> > using the proper types:
> > 
> > $ ./mc_nextgen_test |grep -e dev -e entity#[1-5]\\b -e entity#262\\b
> > entity entity#1: au8522 19-0047, num pads = 3
> > entity entity#2: Xceive XC5000, num pads = 1
> > entity entity#3: au0828a video, num pads = 1
> > entity entity#4: au0828a vbi, num pads = 1
> > entity entity#5: Auvitek AU8522 QAM/8VSB Frontend, num pads = 2
> > entity entity#262: dvb-demux, num pads = 257
> > interface intf_devnode#1: video (81,0)
> > interface intf_devnode#2: vbi (81,1)
> > interface intf_devnode#3: v4l2_subdev (81,2)
> > interface intf_devnode#4: frontend (212,0)
> > interface intf_devnode#5: demux (212,1)
> > interface intf_devnode#6: DVR (212,2)
> > interface intf_devnode#7: dvbnet (212,3)
> > link link#1: intf_devnode#1 and entity#3
> > link link#2: intf_devnode#2 and entity#4
> > link link#3: intf_devnode#3 and entity#2
> > link link#4: intf_devnode#4 and entity#5
> > link link#5: intf_devnode#5 and entity#262
> > link link#1034: intf_devnode#4 and entity#2
> > link link#1035: intf_devnode#6 and entity#262
> > 
> > However, it should be noticed that there are missing links there, as the
> > interfaces intf_devnode#1: and intf_devnode#2: should also be linked to
> > the analog entities (e. g. entity#1 to entity#5).
> > 
> > It is hard to implememt that, however, as some platform drivers don't
> > have such connections. There are two issues to be solved here:
> > 
> > 1) how the V4L2 core will identify that it could automatically create
> >    links to the other analog interfaces;
> > 
> > 2) Where to place such code.
> > 
> > Let's do it on separate patches.
> 
> Agreed.
> 
> I would add that we shouldn't attempt to be 100% perfect in automatically
> creating links. If we can do 80-90% of the drivers that way, then that would
> be great. The remaining drivers will then need to be modified manually.

Yes, agreed, but there are just two cases, IMHO:

1) direct links: they should always be created. 

2) indirect links:

Well, 80%-90% of the drivers are PC-consumer ones, and they all look
the same: the radio/video/vbi interfaces should be indirectly linked to the
tuner and to analog demod.

Ok, there are some exceptions here: the ones with TV out and or with
MPEG encoders/decoders (like saa7134, cx88, ivtv, cx18, go7007).

For those, the driver will need to take care of additional link
creation and may require to override the default for indirect links
creation.

Btw, I guess we'll need a flag to indicate if the link is direct or
indirect.

At least for some usecases I have in mind, such flag could be helpful.


> 
> > 
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 44b330589787..472117f32c05 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -713,6 +713,78 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >  			BASE_VIDIOC_PRIVATE);
> >  }
> >  
> > +
> > +static int video_register_media_controller(struct video_device *vdev, int type)
> > +{
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +	u32 entity_type, intf_type;
> > +	int ret;
> > +	bool create_entity = true;
> > +
> > +	if (!vdev->v4l2_dev->mdev)
> > +		return 0;
> > +
> > +	switch (type) {
> > +	case VFL_TYPE_GRABBER:
> > +		intf_type = MEDIA_INTF_T_V4L_VIDEO;
> > +		entity_type = MEDIA_ENT_T_V4L2_VIDEO;
> > +		break;
> > +	case VFL_TYPE_VBI:
> > +		intf_type = MEDIA_INTF_T_V4L_VBI;
> > +		entity_type = MEDIA_ENT_T_V4L2_VBI;
> > +		break;
> > +	case VFL_TYPE_SDR:
> > +		intf_type = MEDIA_INTF_T_V4L_SWRADIO;
> > +		entity_type = MEDIA_ENT_T_V4L2_SWRADIO;
> > +		break;
> > +	case VFL_TYPE_RADIO:
> > +		intf_type = MEDIA_INTF_T_V4L_RADIO;
> > +		/*
> > +		 * Radio doesn't have an entity at the V4L2 side to represent
> > +		 * radio input or output. Instead, the audio input/output goes
> > +		 * via either physical wires or ALSA.
> > +		 */
> > +		create_entity = false;
> > +		break;
> > +	default: /* Only type left is subdevs */
> > +		/* Subdevs are created via v4l2_device_register_subdev_nodes */
> 
> As mentioned in the other patch: why can't you create the interface for subdevs
> as well?

I guess it is possible.

I'll check if aren't there any issues, and will reply on the other
patch if not possible/not recommended for some reason.

> 
> > +		return 0;
> > +	}
> > +
> > +	if (create_entity) {
> > +		vdev->entity.type = entity_type;
> > +		vdev->entity.name = vdev->name;
> > +		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) {
> > +			printk(KERN_WARNING
> > +				"%s: media_device_register_entity failed\n",
> > +				__func__);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	vdev->intf_devnode = media_devnode_create(vdev->v4l2_dev->mdev,
> > +						  intf_type,
> > +						  0, VIDEO_MAJOR,
> > +						  vdev->minor,
> > +						  GFP_KERNEL);
> > +	if (!vdev->intf_devnode)
> > +		return -ENOMEM;
> > +
> > +	if (create_entity)
> > +		media_create_intf_link(&vdev->entity,
> > +				       &vdev->intf_devnode->intf, 0);
> > +
> > +	/* FIXME: how to create the other interface links? */
> > +
> > +#endif
> > +	return 0;
> > +}
> > +
> >  /**
> >   *	__video_register_device - register video4linux devices
> >   *	@vdev: video device structure we want to register
> > @@ -908,22 +980,9 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
> >  	/* Increase v4l2_device refcount */
> >  	v4l2_device_get(vdev->v4l2_dev);
> >  
> > -#if defined(CONFIG_MEDIA_CONTROLLER)
> >  	/* Part 5: Register the entity. */
> > -	if (vdev->v4l2_dev->mdev &&
> > -	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
> > -		vdev->entity.type = MEDIA_ENT_T_V4L2_VIDEO;
> > -		vdev->entity.name = vdev->name;
> > -		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)
> > -			printk(KERN_WARNING
> > -			       "%s: media_device_register_entity failed\n",
> > -			       __func__);
> > -	}
> > -#endif
> > +	ret = video_register_media_controller(vdev, type);
> > +
> >  	/* Part 6: Activate this minor. The char device can now be used. */
> >  	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
> >  
> 
> I'm missing changes to v4l2_device_release() to clean up the interface and
> entity.

Ok. I'll add it.

> 
> > diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> > index 1e5176c558bf..2124a0e793f3 100644
> > --- a/drivers/media/v4l2-core/v4l2-device.c
> > +++ b/drivers/media/v4l2-core/v4l2-device.c
> > @@ -251,18 +251,18 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
> >  		sd->entity.info.dev.major = VIDEO_MAJOR;
> >  		sd->entity.info.dev.minor = vdev->minor;
> >  
> > -		sd->intf_devnode = media_devnode_create(sd->entity.graph_obj.mdev,
> > +		vdev->intf_devnode = media_devnode_create(sd->entity.graph_obj.mdev,
> >  							MEDIA_INTF_T_V4L_SUBDEV,
> >  							0, VIDEO_MAJOR,
> >  							vdev->minor,
> >  							GFP_KERNEL);
> > -		if (!sd->intf_devnode) {
> > +		if (!vdev->intf_devnode) {
> >  			err = -ENOMEM;
> >  			kfree(vdev);
> >  			goto clean_up;
> >  		}
> >  
> > -		media_create_intf_link(&sd->entity, &sd->intf_devnode->intf, 0);
> > +		media_create_intf_link(&sd->entity, &vdev->intf_devnode->intf, 0);
> >  #endif
> >  		sd->devnode = vdev;
> >  	}
> > @@ -282,6 +282,7 @@ EXPORT_SYMBOL_GPL(v4l2_device_register_subdev_nodes);
> >  void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
> >  {
> >  	struct v4l2_device *v4l2_dev;
> > +	struct video_device *vdev = sd->devnode;
> >  
> >  	/* return if it isn't registered */
> >  	if (sd == NULL || sd->v4l2_dev == NULL)
> > @@ -300,7 +301,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> >  	if (v4l2_dev->mdev) {
> >  		media_entity_remove_links(&sd->entity);
> > -		media_devnode_remove(sd->intf_devnode);
> > +		media_devnode_remove(vdev->intf_devnode);
> >  		media_device_unregister_entity(&sd->entity);
> >  	}
> >  #endif
> > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > index acbcd2f5fe7f..eeabf20e87a6 100644
> > --- a/include/media/v4l2-dev.h
> > +++ b/include/media/v4l2-dev.h
> > @@ -86,6 +86,7 @@ struct video_device
> >  {
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> >  	struct media_entity entity;
> > +	struct media_intf_devnode *intf_devnode;
> >  #endif
> >  	/* device ops */
> >  	const struct v4l2_file_operations *fops;
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 1aa44f11eeb5..370fc38c34f1 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -584,7 +584,6 @@ struct v4l2_subdev_platform_data {
> >  struct v4l2_subdev {
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> >  	struct media_entity entity;
> > -	struct media_intf_devnode *intf_devnode;
> >  #endif
> >  	struct list_head list;
> >  	struct module *owner;
> > 
> 
> Regards,
> 
> 	Hans

[PATCH] [media] v4l2-core: create MC interfaces for devnodes

[Mauro Carvalho Chehab]

V4L2 device (and subdevice) nodes should create an interface, if the
Media Controller support is enabled.

Please notice that radio devices should not create an entity, as radio
input/output is either via wires or via ALSA.

PS.: Before this patch, au0828 should remove the media_device earlier.
However, after the core changes, this should be postponed, otherwise
an OOPS occurs. So, fold the au0828 changes on this patch.

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---

PS.: I'm adding this patch just after "media-device: remove interfaces and interface links".

It prevents some troubles with DEBUG_KMEMLEAK and KASAN, with makes
them to hang the Kernel during physical device removal (or device driver
removal). Without this, we get this error:

	kasan: GPF could be caused by NULL-ptr deref or user memory accessgeneral protection fault: 0000 [#1] SMP KASAN

 drivers/media/usb/au0828/au0828-core.c |   6 +-
 drivers/media/v4l2-core/v4l2-dev.c     | 111 +++++++++++++++++++++++++++------
 drivers/media/v4l2-core/v4l2-device.c  |  16 ++++-
 include/media/v4l2-dev.h               |   1 +
 4 files changed, 113 insertions(+), 21 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index 7f645bcb7463..5f0b210cc797 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -175,8 +175,6 @@ static void au0828_usb_disconnect(struct usb_interface *interface)
 	*/
 	dev->dev_state = DEV_DISCONNECTED;
 
-	au0828_unregister_media_device(dev);
-
 	au0828_rc_unregister(dev);
 	/* Digital TV */
 	au0828_dvb_unregister(dev);
@@ -190,6 +188,10 @@ static void au0828_usb_disconnect(struct usb_interface *interface)
 		au0828_analog_unregister(dev);
 		v4l2_device_disconnect(&dev->v4l2_dev);
 		v4l2_device_put(&dev->v4l2_dev);
+		/*
+		 * No need to call au0828_usb_release() if V4L2 is enabled,
+		 * as this is already called via au0828_usb_v4l2_release()
+		 */
 		return;
 	}
 #endif
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 2297571a0568..077cdc92778c 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -194,9 +194,12 @@ static void v4l2_device_release(struct device *cd)
 	mutex_unlock(&videodev_lock);
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	if (v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV)
-		media_device_unregister_entity(&vdev->entity);
+	if (v4l2_dev->mdev) {
+		/* Remove interfaces and interface links */
+		media_devnode_remove(vdev->intf_devnode);
+		if (vdev->entity.type != MEDIA_ENT_T_UNKNOWN)
+			media_device_unregister_entity(&vdev->entity);
+	}
 #endif
 
 	/* Do not call v4l2_device_put if there is no release callback set.
@@ -723,6 +726,91 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			BASE_VIDIOC_PRIVATE);
 }
 
+static int video_register_media_controller(struct video_device *vdev, int type)
+{
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	u32 intf_type;
+	int ret;
+
+	if (!vdev->v4l2_dev->mdev)
+		return 0;
+
+	vdev->entity.type = MEDIA_ENT_T_UNKNOWN;
+
+	switch (type) {
+	case VFL_TYPE_GRABBER:
+		intf_type = MEDIA_INTF_T_V4L_VIDEO;
+		vdev->entity.type = MEDIA_ENT_T_V4L2_VIDEO;
+		break;
+	case VFL_TYPE_VBI:
+		intf_type = MEDIA_INTF_T_V4L_VBI;
+		vdev->entity.type = MEDIA_ENT_T_V4L2_VBI;
+		break;
+	case VFL_TYPE_SDR:
+		intf_type = MEDIA_INTF_T_V4L_SWRADIO;
+		vdev->entity.type = MEDIA_ENT_T_V4L2_SWRADIO;
+		break;
+	case VFL_TYPE_RADIO:
+		intf_type = MEDIA_INTF_T_V4L_RADIO;
+		/*
+		 * Radio doesn't have an entity at the V4L2 side to represent
+		 * radio input or output. Instead, the audio input/output goes
+		 * via either physical wires or ALSA.
+		 */
+		break;
+	case VFL_TYPE_SUBDEV:
+		intf_type = MEDIA_INTF_T_V4L_SUBDEV;
+		/* Entity will be created via v4l2_device_register_subdev() */
+		break;
+	default:
+		return 0;
+	}
+
+	if (vdev->entity.type != MEDIA_ENT_T_UNKNOWN) {
+		vdev->entity.name = vdev->name;
+
+		/* Needed just for backward compatibility with legacy MC API */
+		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) {
+			printk(KERN_WARNING
+				"%s: media_device_register_entity failed\n",
+				__func__);
+			return ret;
+		}
+	}
+
+	vdev->intf_devnode = media_devnode_create(vdev->v4l2_dev->mdev,
+						  intf_type,
+						  0, VIDEO_MAJOR,
+						  vdev->minor,
+						  GFP_KERNEL);
+	if (!vdev->intf_devnode) {
+		media_device_unregister_entity(&vdev->entity);
+		return -ENOMEM;
+	}
+
+	if (vdev->entity.type != MEDIA_ENT_T_UNKNOWN) {
+		struct media_link *link;
+
+		link = media_create_intf_link(&vdev->entity,
+					      &vdev->intf_devnode->intf, 0);
+		if (!link) {
+			media_devnode_remove(vdev->intf_devnode);
+			media_device_unregister_entity(&vdev->entity);
+			return -ENOMEM;
+		}
+	}
+
+	/* FIXME: how to create the other interface links? */
+
+#endif
+	return 0;
+}
+
 /**
  *	__video_register_device - register video4linux devices
  *	@vdev: video device structure we want to register
@@ -918,22 +1006,9 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
 	/* Increase v4l2_device refcount */
 	v4l2_device_get(vdev->v4l2_dev);
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
 	/* Part 5: Register the entity. */
-	if (vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
-		vdev->entity.type = MEDIA_ENT_T_V4L2_VIDEO;
-		vdev->entity.name = vdev->name;
-		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)
-			printk(KERN_WARNING
-			       "%s: media_device_register_entity failed\n",
-			       __func__);
-	}
-#endif
+	ret = video_register_media_controller(vdev, type);
+
 	/* Part 6: Activate this minor. The char device can now be used. */
 	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
 
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 7129e438f29e..3c87307e56f0 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -258,6 +258,17 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 #if defined(CONFIG_MEDIA_CONTROLLER)
 		sd->entity.info.dev.major = VIDEO_MAJOR;
 		sd->entity.info.dev.minor = vdev->minor;
+
+		/* Interface is created by __video_register_device() */
+		if (vdev->v4l2_dev->mdev) {
+			struct media_link *link;
+
+			link = media_create_intf_link(&sd->entity,
+						      &vdev->intf_devnode->intf,
+						      0);
+			if (!link)
+				goto clean_up;
+		}
 #endif
 		sd->devnode = vdev;
 	}
@@ -294,7 +305,10 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	if (v4l2_dev->mdev) {
-		media_entity_remove_links(&sd->entity);
+		/*
+		 * No need to explicitly remove links, as both pads and
+		 * links are removed by the function below, in the right order
+		 */
 		media_device_unregister_entity(&sd->entity);
 	}
 #endif
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index acbcd2f5fe7f..eeabf20e87a6 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -86,6 +86,7 @@ struct video_device
 {
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	struct media_entity entity;
+	struct media_intf_devnode *intf_devnode;
 #endif
 	/* device ops */
 	const struct v4l2_file_operations *fops;
-- 
2.5.0

Re: [PATCH] [media] v4l2-core: create MC interfaces for devnodes

[Mauro Carvalho Chehab]

Em Thu, 10 Dec 2015 08:32:21 -0200
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> V4L2 device (and subdevice) nodes should create an interface, if the
> Media Controller support is enabled.
> 
> Please notice that radio devices should not create an entity, as radio
> input/output is either via wires or via ALSA.
> 
> PS.: Before this patch, au0828 should remove the media_device earlier.
> However, after the core changes, this should be postponed, otherwise
> an OOPS occurs. So, fold the au0828 changes on this patch.
> 
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
> 
> PS.: I'm adding this patch just after "media-device: remove interfaces and interface links".
> 
> It prevents some troubles with DEBUG_KMEMLEAK and KASAN, with makes
> them to hang the Kernel during physical device removal (or device driver
> removal). Without this, we get this error:
> 
> 	kasan: GPF could be caused by NULL-ptr deref or user memory accessgeneral protection fault: 0000 [#1] SMP KASAN


Sorry, I ended by sending the wrong patch. Please ignore this e-mail.

Regards,
Mauro