Re: [PATCH v8 53/55] [media] v4l2-core: create MC interfaces for devnodes

[Mauro Carvalho Chehab]

Em Mon, 23 Nov 2015 23:10:04 +0200
Laurent Pinchart  escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> On Sunday 06 September 2015 09:03:13 Mauro Carvalho Chehab wrote:
> > 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.
> 
> I'd go one step further, video nodes should not create an entity, ever. This 
> was one of the design principles behind the MC rework. We'll need to patch 
> drivers to create DMA engine entity explicitly, possibly through a helper 
> function (maybe a function to create and initialize a DMA engine entity based 
> on a struct video_device). This can of course be done on a separate patch, 
> but 
> removing the entity field from struct video_device should be part of the 
> series.

Not sure what exactly you mean here.

There are two separate things happening with regards to device
nodes that do stream I/O:

1) They may have a DMA engine somewhere at the hardware pipeline.
   On some drivers, like USB ones, the DMA engine is actually not
   responsible to deliver the data to userspace. This is also
   true on ALSA and DVB (right now). Also, eventually, some
   drivers may not have a DMA engine (they may eventually use PIO
   like some old sound cards);

2) The I/O interface to where the data stream is delivered. This is
   actually not a hardware interface, but a software implementation
   on Linux. Ok, on V4L2, this is typically associated with a DMA
   engine, but the actual interface is a buffer that could be
   obtained by one of the delivery methods: read()/write() sysctl,
   mmapped buffer and/or DMABUF.

IMHO, (2) would ideally be mapped as a media_interface, as this is
not hardware, but linux software. However, such change won't be
backward compatible. So, as agreed at the MC workshop, we're using
a media entity for such I/O interface, calling its function as
MEDIA_ENT_F_IO (as you may see at the review comments from the
others, Shuah requested to actually split this function into one
I/O function per type of interface, in order to be able to easily
distinguish ALSA I/O from V4L2 I/O). 

I agree that (1) should be driver-specific, as only the driver knows
where the DMA engine is at the pipeline.

However, (2) should be created by the media core. So, I can't see
any rationale to remove the entity field from struct video_device.

> 
> > Signed-off-by: Mauro Carvalho Chehab 
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> > b/drivers/media/v4l2-core/v4l2-dev.c index 44b330589787..07123dd569c4
> > 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(_lock);
> > 
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> > -   if (v4l2_dev->mdev &&
> > -   vdev->vfl_type != VFL_TYPE_SUBDEV)
> > -   media_device_unregister_entity(>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(>entity);
> > +   }
> >  #endif
> > 
> > /* Do not call v4l2_device_put if there is no release callback set.
> > @@ -713,6 +716,92 @@ static void determine_valid_ioctls(struct video_device
> > *vdev) BASE_VIDIOC_PRIVATE);
> >  }
> > 
> > +
> 
> Extra blank line.
> 
> > +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() */
> > +

Re: [PATCH v8 53/55] [media] v4l2-core: create MC interfaces for devnodes

[Laurent Pinchart]

Hi Mauro,

Thank you for the patch.

On Sunday 06 September 2015 09:03:13 Mauro Carvalho Chehab wrote:
> 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.

I'd go one step further, video nodes should not create an entity, ever. This 
was one of the design principles behind the MC rework. We'll need to patch 
drivers to create DMA engine entity explicitly, possibly through a helper 
function (maybe a function to create and initialize a DMA engine entity based 
on a struct video_device). This can of course be done on a separate patch, but 
removing the entity field from struct video_device should be part of the 
series.

> Signed-off-by: Mauro Carvalho Chehab 
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> b/drivers/media/v4l2-core/v4l2-dev.c index 44b330589787..07123dd569c4
> 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(_lock);
> 
>  #if defined(CONFIG_MEDIA_CONTROLLER)
> - if (v4l2_dev->mdev &&
> - vdev->vfl_type != VFL_TYPE_SUBDEV)
> - media_device_unregister_entity(>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(>entity);
> + }
>  #endif
> 
>   /* Do not call v4l2_device_put if there is no release callback set.
> @@ -713,6 +716,92 @@ static void determine_valid_ioctls(struct video_device
> *vdev) BASE_VIDIOC_PRIVATE);
>  }
> 
> +

Extra blank line.

> +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,
> +>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(>entity);
> + return -ENOMEM;
> + }
> +
> + if (vdev->entity.type != MEDIA_ENT_T_UNKNOWN) {
> + struct media_link *link;
> +
> + link = media_create_intf_link(>entity,
> +   >intf_devnode->intf, 0);
> + if (!link) {
> + media_devnode_remove(vdev->intf_devnode);
> + media_device_unregister_entity(>entity);
> + return -ENOMEM;
> + }
> + }
> +
> + /* FIXME: how to create the other interface links? */

If they're 

Re: [PATCH v8 53/55] [media] v4l2-core: create MC interfaces for devnodes

[Hans Verkuil]

On 09/06/2015 02:03 PM, Mauro Carvalho Chehab wrote:
> 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.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
> b/drivers/media/v4l2-core/v4l2-dev.c
> index 44b330589787..07123dd569c4 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(_lock);
>  
>  #if defined(CONFIG_MEDIA_CONTROLLER)
> - if (v4l2_dev->mdev &&
> - vdev->vfl_type != VFL_TYPE_SUBDEV)
> - media_device_unregister_entity(>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(>entity);
> + }
>  #endif
>  
>   /* Do not call v4l2_device_put if there is no release callback set.
> @@ -713,6 +716,92 @@ 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,
> +>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(>entity);
> + return -ENOMEM;
> + }
> +
> + if (vdev->entity.type != MEDIA_ENT_T_UNKNOWN) {
> + struct media_link *link;
> +
> + link = media_create_intf_link(>entity,
> +   >intf_devnode->intf, 0);
> + if (!link) {
> + media_devnode_remove(vdev->intf_devnode);
> + media_device_unregister_entity(>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
> @@ -908,22 +997,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) {

Re: [PATCH v8 53/55] [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.

Signed-off-by: Mauro Carvalho Chehab 

diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
b/drivers/media/v4l2-core/v4l2-dev.c
index 44b330589787..07123dd569c4 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(_lock);
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
-   if (v4l2_dev->mdev &&
-   vdev->vfl_type != VFL_TYPE_SUBDEV)
-   media_device_unregister_entity(>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(>entity);
+   }
 #endif
 
/* Do not call v4l2_device_put if there is no release callback set.
@@ -713,6 +716,92 @@ 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,
+  >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(>entity);
+   return -ENOMEM;
+   }
+
+   if (vdev->entity.type != MEDIA_ENT_T_UNKNOWN) {
+   struct media_link *link;
+
+   link = media_create_intf_link(>entity,
+ >intf_devnode->intf, 0);
+   if (!link) {
+   media_devnode_remove(vdev->intf_devnode);
+   media_device_unregister_entity(>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
@@ -908,22 +997,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;
-   

Re: [PATCH v8 53/55] [media] v4l2-core: create MC interfaces for devnodes

[Mauro Carvalho Chehab]

Em Mon, 31 Aug 2015 15:23:59 +0200
Hans Verkuil  escreveu:

> On 08/30/2015 05:07 AM, Mauro Carvalho Chehab wrote:
> > 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.
> 
> A general note: I think this patch (and any prerequisite patches) should come 
> before
> the patches adding G_TOPOLOGY support.
> 
> What the G_TOPOLOGY ioctl returns only makes sense IMHO if this code is 
> present as well,
> so it is a more logical order for this patch series.
> 
> In addition, since G_TOPOLOGY is a userspace API it is likely that that will 
> create
> more discussions, whereas this is internal to the kernel and could be merged 
> before
> G_TOPOLOGY.
> 
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
> > b/drivers/media/v4l2-core/v4l2-dev.c
> > index 44b330589787..427a5a32b3de 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(_lock);
> >  
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> > -   if (v4l2_dev->mdev &&
> > -   vdev->vfl_type != VFL_TYPE_SUBDEV)
> > -   media_device_unregister_entity(>entity);
> > +   if (v4l2_dev->mdev) {
> > +   /* Remove interfaces and interface links */
> > +   media_devnode_remove(vdev->intf_devnode);
> > +   if (vdev->vfl_type != VFL_TYPE_SUBDEV)
> > +   media_device_unregister_entity(>entity);
> 
> RADIO doesn't have an entity either, so this should probably check for both
> SUBDEV and RADIO.
> 
> I think it is cleaner if video_register_media_controller() sets a new 
> video_device
> flag: V4L2_FL_CREATED_ENTITY, and if this release function would just check 
> the
> flag.

I don't see the need for a new flag here. I guess this should do the job:

diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
b/drivers/media/v4l2-core/v4l2-dev.c
index 427a5a32b3de..298aaf6f4296 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -197,7 +197,7 @@ static void v4l2_device_release(struct device *cd)
if (v4l2_dev->mdev) {
/* Remove interfaces and interface links */
media_devnode_remove(vdev->intf_devnode);
-   if (vdev->vfl_type != VFL_TYPE_SUBDEV)
+   if (vdev->entity.type)
media_device_unregister_entity(>entity);
}
 #endif
@@ -775,6 +775,8 @@ static int video_register_media_controller(struct 
video_device *vdev, int type)
__func__);
return ret;
}
+   } else {
+   vdev->entity.type = 0;
}
 
vdev->intf_devnode = media_devnode_create(vdev->v4l2_dev->mdev,

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 53/55] [media] v4l2-core: create MC interfaces for devnodes

[Hans Verkuil]

On 08/30/2015 05:07 AM, Mauro Carvalho Chehab wrote:
> 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.

A general note: I think this patch (and any prerequisite patches) should come 
before
the patches adding G_TOPOLOGY support.

What the G_TOPOLOGY ioctl returns only makes sense IMHO if this code is present 
as well,
so it is a more logical order for this patch series.

In addition, since G_TOPOLOGY is a userspace API it is likely that that will 
create
more discussions, whereas this is internal to the kernel and could be merged 
before
G_TOPOLOGY.

> 
> Signed-off-by: Mauro Carvalho Chehab 
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
> b/drivers/media/v4l2-core/v4l2-dev.c
> index 44b330589787..427a5a32b3de 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(_lock);
>  
>  #if defined(CONFIG_MEDIA_CONTROLLER)
> - if (v4l2_dev->mdev &&
> - vdev->vfl_type != VFL_TYPE_SUBDEV)
> - media_device_unregister_entity(>entity);
> + if (v4l2_dev->mdev) {
> + /* Remove interfaces and interface links */
> + media_devnode_remove(vdev->intf_devnode);
> + if (vdev->vfl_type != VFL_TYPE_SUBDEV)
> + media_device_unregister_entity(>entity);

RADIO doesn't have an entity either, so this should probably check for both
SUBDEV and RADIO.

I think it is cleaner if video_register_media_controller() sets a new 
video_device
flag: V4L2_FL_CREATED_ENTITY, and if this release function would just check the
flag.

> + }
>  #endif
>  
>   /* Do not call v4l2_device_put if there is no release callback set.
> @@ -713,6 +716,85 @@ 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 = MEDIA_ENT_T_UNKNOWN;
> + u32 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;
> + case VFL_TYPE_SUBDEV:
> + intf_type = MEDIA_INTF_T_V4L_SUBDEV;
> + /* Entity will be created via v4l2_device_register_subdev() */
> + create_entity = false;
> + break;
> + default:
> + return 0;
> + }
> +
> + if (create_entity) {
> + vdev->entity.type = entity_type;
> + 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,
> +>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(>entity,
> +>intf_devnode->intf, 0);

You need a NULL pointer check here as well. It might be a good idea to add
__must_check to the media_create_intf_link() prototype. I think there are

[PATCH v8 53/55] [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.

Signed-off-by: Mauro Carvalho Chehab mche...@osg.samsung.com

diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
b/drivers/media/v4l2-core/v4l2-dev.c
index 44b330589787..427a5a32b3de 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-vfl_type != VFL_TYPE_SUBDEV)
+   media_device_unregister_entity(vdev-entity);
+   }
 #endif
 
/* Do not call v4l2_device_put if there is no release callback set.
@@ -713,6 +716,85 @@ 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 = MEDIA_ENT_T_UNKNOWN;
+   u32 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;
+   case VFL_TYPE_SUBDEV:
+   intf_type = MEDIA_INTF_T_V4L_SUBDEV;
+   /* Entity will be created via v4l2_device_register_subdev() */
+   create_entity = false;
+   break;
+   default:
+   return 0;
+   }
+
+   if (create_entity) {
+   vdev-entity.type = entity_type;
+   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)
+   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 +990,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
-