[PATCH] media: Add type field to struct media_entity

[PATCH] media: Add type field to struct media_entity

[Laurent Pinchart]

Code that processes media entities can require knowledge of the
structure type that embeds a particular media entity instance in order
to use the API provided by that structure. This needs is shown by the
presence of the is_media_entity_v4l2_io and is_media_entity_v4l2_subdev
functions.

The implementation of those two functions relies on the entity function
field, which is both a wrong and an inefficient design, without even
mentioning the maintenance issue involved in updating the functions
every time a new entity function is added. Fix this by adding add a type
field to the media entity structure to carry the information.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-dev.c    |  1 +
 drivers/media/v4l2-core/v4l2-subdev.c |  1 +
 include/media/media-entity.h          | 65 +++++++++++++++--------------------
 3 files changed, 30 insertions(+), 37 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index d8e5994cccf1..7e766a92e3d9 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -735,6 +735,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
 	if (!vdev->v4l2_dev->mdev)
 		return 0;
 
+	vdev->entity.type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
 	vdev->entity.function = MEDIA_ENT_F_UNKNOWN;
 
 	switch (type) {
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index d63083803144..bb6e79f14bb8 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -584,6 +584,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
 	sd->host_priv = NULL;
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	sd->entity.name = sd->name;
+	sd->entity.type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
 	sd->entity.function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
 #endif
 }
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index fe485d367985..2be38483f3a4 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -187,10 +187,27 @@ struct media_entity_operations {
 };
 
 /**
+ * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
+ *
+ * @MEDIA_ENTITY_TYPE_NONE:
+ *	The entity isn't embedded in a standard structure.
+ * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
+ *	The media entity is embedded in a struct video_device.
+ * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
+ *	The media entity is embedded in a struct v4l2_subdev.
+ */
+enum media_entity_type {
+	MEDIA_ENTITY_TYPE_NONE,
+	MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
+	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
+};
+
+/**
  * struct media_entity - A media entity graph object.
  *
  * @graph_obj:	Embedded structure containing the media object common data.
  * @name:	Entity name.
+ * @type:	Type of the object that embeds the media_entity instance.
  * @function:	Entity main function, as defined in uapi/media.h
  *		(MEDIA_ENT_F_*)
  * @flags:	Entity flags, as defined in uapi/media.h (MEDIA_ENT_FL_*)
@@ -219,6 +236,7 @@ struct media_entity_operations {
 struct media_entity {
 	struct media_gobj graph_obj;	/* must be first field in struct */
 	const char *name;
+	enum media_entity_type type;
 	u32 function;
 	unsigned long flags;
 
@@ -328,56 +346,29 @@ static inline u32 media_gobj_gen_id(enum media_gobj_type type, u64 local_id)
 }
 
 /**
- * is_media_entity_v4l2_io() - identify if the entity main function
- *			       is a V4L2 I/O
- *
+ * is_media_entity_v4l2_io() - Check if the entity implements the video_device
+ *			       API
  * @entity:	pointer to entity
  *
- * Return: true if the entity main function is one of the V4L2 I/O types
- *	(video, VBI or SDR radio); false otherwise.
+ * Return: true if the entity implement the video_device API (is directly
+ * embedded in a struct video_device instance) or false otherwise.
  */
 static inline bool is_media_entity_v4l2_io(struct media_entity *entity)
 {
-	if (!entity)
-		return false;
-
-	switch (entity->function) {
-	case MEDIA_ENT_F_IO_V4L:
-	case MEDIA_ENT_F_IO_VBI:
-	case MEDIA_ENT_F_IO_SWRADIO:
-		return true;
-	default:
-		return false;
-	}
+	return entity && entity->type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
 }
 
 /**
- * is_media_entity_v4l2_subdev - return true if the entity main function is
- *				 associated with the V4L2 API subdev usage
- *
+ * is_media_entity_v4l2_subdev() - Check if the entity implements the
+ *				   v4l2_subdev API
  * @entity:	pointer to entity
  *
- * This is an ancillary function used by subdev-based V4L2 drivers.
- * It checks if the entity function is one of functions used by a V4L2 subdev,
- * e. g. camera-relatef functions, analog TV decoder, TV tuner, V4L2 DSPs.
+ * Return: true if the entity implement the v4l2_subdev API (is directly
+ * embedded in a struct v4l2_subdev instance) or false otherwise.
  */
 static inline bool is_media_entity_v4l2_subdev(struct media_entity *entity)
 {
-	if (!entity)
-		return false;
-
-	switch (entity->function) {
-	case MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN:
-	case MEDIA_ENT_F_CAM_SENSOR:
-	case MEDIA_ENT_F_FLASH:
-	case MEDIA_ENT_F_LENS:
-	case MEDIA_ENT_F_ATV_DECODER:
-	case MEDIA_ENT_F_TUNER:
-		return true;
-
-	default:
-		return false;
-	}
+	return entity && entity->type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
 }
 
 /**
-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: Add type field to struct media_entity

[Mauro Carvalho Chehab]

Em Mon, 22 Feb 2016 03:53:16 +0200
Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> escreveu:

> Code that processes media entities can require knowledge of the
> structure type that embeds a particular media entity instance in order
> to use the API provided by that structure. This needs is shown by the
> presence of the is_media_entity_v4l2_io and is_media_entity_v4l2_subdev
> functions.
> 
> The implementation of those two functions relies on the entity function
> field, which is both a wrong and an inefficient design, without even
> mentioning the maintenance issue involved in updating the functions
> every time a new entity function is added. Fix this by adding add a type
> field to the media entity structure to carry the information.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
>  include/media/media-entity.h          | 65 +++++++++++++++--------------------
>  3 files changed, 30 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index d8e5994cccf1..7e766a92e3d9 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -735,6 +735,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
>  	if (!vdev->v4l2_dev->mdev)
>  		return 0;
>  
> +	vdev->entity.type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
>  	vdev->entity.function = MEDIA_ENT_F_UNKNOWN;
>  
>  	switch (type) {
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index d63083803144..bb6e79f14bb8 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -584,6 +584,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>  	sd->host_priv = NULL;
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	sd->entity.name = sd->name;
> +	sd->entity.type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
>  	sd->entity.function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>  #endif
>  }
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index fe485d367985..2be38483f3a4 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -187,10 +187,27 @@ struct media_entity_operations {
>  };
>  
>  /**
> + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
> + *

s/MEDIA_ENTITY_TYPE_NONE/media_entity_type/

(it seems you didn't test producing the docbook, otherwise you would
have seen this error - Please always generate the docbook when the
patch contains kernel-doc markups)

I don't like the idea of calling it as "type", as this is confusing,
specially since we used to call entity.type for what we now call function.

What we're actually wanting to represent is the Linux kABI group where
the entity belongs. So, maybe we could call it as
media_entity_kabi_type, instead.

> + * @MEDIA_ENTITY_TYPE_NONE:
> + *	The entity isn't embedded in a standard structure.

I also don't like having a NONE here. All objects belong to some
kABI type, but not all subsystems need to use this field
(so far, DVB doesn't need nor ALSA).

So, I would either call it as DEFAULT or UNDEFINED.

> + * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
> + *	The media entity is embedded in a struct video_device.
> + * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
> + *	The media entity is embedded in a struct v4l2_subdev.
> + */
> +enum media_entity_type {
> +	MEDIA_ENTITY_TYPE_NONE,
> +	MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
> +	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
> +};

> +
> +/**
>   * struct media_entity - A media entity graph object.
>   *
>   * @graph_obj:	Embedded structure containing the media object common data.
>   * @name:	Entity name.
> + * @type:	Type of the object that embeds the media_entity instance.
>   * @function:	Entity main function, as defined in uapi/media.h
>   *		(MEDIA_ENT_F_*)
>   * @flags:	Entity flags, as defined in uapi/media.h (MEDIA_ENT_FL_*)
> @@ -219,6 +236,7 @@ struct media_entity_operations {
>  struct media_entity {
>  	struct media_gobj graph_obj;	/* must be first field in struct */
>  	const char *name;
> +	enum media_entity_type type;

Same as before: I would call it as kabi_type.

>  	u32 function;
>  	unsigned long flags;
>  
> @@ -328,56 +346,29 @@ static inline u32 media_gobj_gen_id(enum media_gobj_type type, u64 local_id)
>  }
>  
>  /**
> - * is_media_entity_v4l2_io() - identify if the entity main function
> - *			       is a V4L2 I/O
> - *
> + * is_media_entity_v4l2_io() - Check if the entity implements the video_device
> + *			       API
>   * @entity:	pointer to entity
>   *
> - * Return: true if the entity main function is one of the V4L2 I/O types
> - *	(video, VBI or SDR radio); false otherwise.
> + * Return: true if the entity implement the video_device API (is directly
> + * embedded in a struct video_device instance) or false otherwise.

s/implement/implements/

Yet, I don't think the above comment is ok. First of all, video_device is
a kABI. We're nowadays calling the kernel APIs as kABI, and the userspace
ones as uAPI.

Also, it doesn't make clear that it would be used also for radio, and 
it is repeating the same thing twice.

So, I would either keep the original comment or change it to:

"Return: true if the entity implements the video_device kABI for video,
 VBI or SDR radio (e. g. if the entity is embeddded at a struct
 video_device instance) or false otherwise."


>   */
>  static inline bool is_media_entity_v4l2_io(struct media_entity *entity)
>  {
> -	if (!entity)
> -		return false;
> -
> -	switch (entity->function) {
> -	case MEDIA_ENT_F_IO_V4L:
> -	case MEDIA_ENT_F_IO_VBI:
> -	case MEDIA_ENT_F_IO_SWRADIO:
> -		return true;
> -	default:
> -		return false;
> -	}
> +	return entity && entity->type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
>  }
>  
>  /**
> - * is_media_entity_v4l2_subdev - return true if the entity main function is
> - *				 associated with the V4L2 API subdev usage
> - *
> + * is_media_entity_v4l2_subdev() - Check if the entity implements the
> + *				   v4l2_subdev API
>   * @entity:	pointer to entity
>   *
> - * This is an ancillary function used by subdev-based V4L2 drivers.
> - * It checks if the entity function is one of functions used by a V4L2 subdev,
> - * e. g. camera-relatef functions, analog TV decoder, TV tuner, V4L2 DSPs.
> + * Return: true if the entity implement the v4l2_subdev API (is directly
> + * embedded in a struct v4l2_subdev instance) or false otherwise.
>   */

s/API/kABI/

>  static inline bool is_media_entity_v4l2_subdev(struct media_entity *entity)
>  {
> -	if (!entity)
> -		return false;
> -
> -	switch (entity->function) {
> -	case MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN:
> -	case MEDIA_ENT_F_CAM_SENSOR:
> -	case MEDIA_ENT_F_FLASH:
> -	case MEDIA_ENT_F_LENS:
> -	case MEDIA_ENT_F_ATV_DECODER:
> -	case MEDIA_ENT_F_TUNER:
> -		return true;
> -
> -	default:
> -		return false;
> -	}
> +	return entity && entity->type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
>  }
>  
>  /**


-- 
Thanks,
Mauro

Re: [PATCH] media: Add type field to struct media_entity

[Sakari Ailus]

Hi Mauro and Laurent,

On Mon, Feb 22, 2016 at 06:46:01AM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 22 Feb 2016 03:53:16 +0200
> Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> escreveu:
> 
> > Code that processes media entities can require knowledge of the
> > structure type that embeds a particular media entity instance in order
> > to use the API provided by that structure. This needs is shown by the
> > presence of the is_media_entity_v4l2_io and is_media_entity_v4l2_subdev
> > functions.
> > 
> > The implementation of those two functions relies on the entity function
> > field, which is both a wrong and an inefficient design, without even

I wouldn't necessarily say "wrong", but it is risky. A device's function not
only defines the interface it offers but also which struct is considered to
contain the media entity. Having a wrong value in the function field may
thus lead memory corruption and / or system crash.

> > mentioning the maintenance issue involved in updating the functions
> > every time a new entity function is added. Fix this by adding add a type
> > field to the media entity structure to carry the information.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >  include/media/media-entity.h          | 65 +++++++++++++++--------------------
> >  3 files changed, 30 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index d8e5994cccf1..7e766a92e3d9 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -735,6 +735,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
> >  	if (!vdev->v4l2_dev->mdev)
> >  		return 0;
> >  
> > +	vdev->entity.type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
> >  	vdev->entity.function = MEDIA_ENT_F_UNKNOWN;
> >  
> >  	switch (type) {
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index d63083803144..bb6e79f14bb8 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -584,6 +584,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
> >  	sd->host_priv = NULL;
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> >  	sd->entity.name = sd->name;
> > +	sd->entity.type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
> >  	sd->entity.function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> >  #endif
> >  }
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index fe485d367985..2be38483f3a4 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -187,10 +187,27 @@ struct media_entity_operations {
> >  };
> >  
> >  /**
> > + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
> > + *
> 
> s/MEDIA_ENTITY_TYPE_NONE/media_entity_type/
> 
> (it seems you didn't test producing the docbook, otherwise you would
> have seen this error - Please always generate the docbook when the
> patch contains kernel-doc markups)
> 
> I don't like the idea of calling it as "type", as this is confusing,
> specially since we used to call entity.type for what we now call function.

What that field essentially defines is which struct embeds the media entity.
(Well, there's some cleanups to be done there, as we have extra entity for
V4L2 subdevices, but that's another story.)

The old type field had that information, plus the "function" of the entity.

I think "type" isn't a bad name for this field, as what we would really need
is inheritance. It refers to the object type. What would you think of
"class"?

> 
> What we're actually wanting to represent is the Linux kABI group where
> the entity belongs. So, maybe we could call it as
> media_entity_kabi_type, instead.
> 
> > + * @MEDIA_ENTITY_TYPE_NONE:
> > + *	The entity isn't embedded in a standard structure.
> 
> I also don't like having a NONE here. All objects belong to some
> kABI type, but not all subsystems need to use this field
> (so far, DVB doesn't need nor ALSA).
> 
> So, I would either call it as DEFAULT or UNDEFINED.

I prefer UNDEFINED from the two. There really is no interface in that case,
and we don't have a "default" interface either.

> 
> > + * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
> > + *	The media entity is embedded in a struct video_device.
> > + * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
> > + *	The media entity is embedded in a struct v4l2_subdev.
> > + */
> > +enum media_entity_type {
> > +	MEDIA_ENTITY_TYPE_NONE,
> > +	MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
> > +	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
> > +};
> 
> > +
> > +/**
> >   * struct media_entity - A media entity graph object.
> >   *
> >   * @graph_obj:	Embedded structure containing the media object common data.
> >   * @name:	Entity name.
> > + * @type:	Type of the object that embeds the media_entity instance.
> >   * @function:	Entity main function, as defined in uapi/media.h
> >   *		(MEDIA_ENT_F_*)
> >   * @flags:	Entity flags, as defined in uapi/media.h (MEDIA_ENT_FL_*)
> > @@ -219,6 +236,7 @@ struct media_entity_operations {
> >  struct media_entity {
> >  	struct media_gobj graph_obj;	/* must be first field in struct */
> >  	const char *name;
> > +	enum media_entity_type type;
> 
> Same as before: I would call it as kabi_type.
> 
> >  	u32 function;
> >  	unsigned long flags;
> >  
> > @@ -328,56 +346,29 @@ static inline u32 media_gobj_gen_id(enum media_gobj_type type, u64 local_id)
> >  }
> >  
> >  /**
> > - * is_media_entity_v4l2_io() - identify if the entity main function
> > - *			       is a V4L2 I/O
> > - *
> > + * is_media_entity_v4l2_io() - Check if the entity implements the video_device
> > + *			       API
> >   * @entity:	pointer to entity
> >   *
> > - * Return: true if the entity main function is one of the V4L2 I/O types
> > - *	(video, VBI or SDR radio); false otherwise.
> > + * Return: true if the entity implement the video_device API (is directly
> > + * embedded in a struct video_device instance) or false otherwise.
> 
> s/implement/implements/
> 
> Yet, I don't think the above comment is ok. First of all, video_device is
> a kABI. We're nowadays calling the kernel APIs as kABI, and the userspace
> ones as uAPI.

Are the exact definitions of the two available somewhere? ABI doesn't matter
much in the kernel itself but towards user space both ABI and API are
important...

> 
> Also, it doesn't make clear that it would be used also for radio, and 
> it is repeating the same thing twice.
> 
> So, I would either keep the original comment or change it to:
> 
> "Return: true if the entity implements the video_device kABI for video,
>  VBI or SDR radio (e. g. if the entity is embeddded at a struct
>  video_device instance) or false otherwise."
> 
> 
> >   */
> >  static inline bool is_media_entity_v4l2_io(struct media_entity *entity)
> >  {
> > -	if (!entity)
> > -		return false;
> > -
> > -	switch (entity->function) {
> > -	case MEDIA_ENT_F_IO_V4L:
> > -	case MEDIA_ENT_F_IO_VBI:
> > -	case MEDIA_ENT_F_IO_SWRADIO:
> > -		return true;
> > -	default:
> > -		return false;
> > -	}
> > +	return entity && entity->type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
> >  }
> >  
> >  /**
> > - * is_media_entity_v4l2_subdev - return true if the entity main function is
> > - *				 associated with the V4L2 API subdev usage
> > - *
> > + * is_media_entity_v4l2_subdev() - Check if the entity implements the
> > + *				   v4l2_subdev API
> >   * @entity:	pointer to entity
> >   *
> > - * This is an ancillary function used by subdev-based V4L2 drivers.
> > - * It checks if the entity function is one of functions used by a V4L2 subdev,
> > - * e. g. camera-relatef functions, analog TV decoder, TV tuner, V4L2 DSPs.
> > + * Return: true if the entity implement the v4l2_subdev API (is directly
> > + * embedded in a struct v4l2_subdev instance) or false otherwise.
> >   */
> 
> s/API/kABI/
> 
> >  static inline bool is_media_entity_v4l2_subdev(struct media_entity *entity)
> >  {
> > -	if (!entity)
> > -		return false;
> > -
> > -	switch (entity->function) {
> > -	case MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN:
> > -	case MEDIA_ENT_F_CAM_SENSOR:
> > -	case MEDIA_ENT_F_FLASH:
> > -	case MEDIA_ENT_F_LENS:
> > -	case MEDIA_ENT_F_ATV_DECODER:
> > -	case MEDIA_ENT_F_TUNER:
> > -		return true;
> > -
> > -	default:
> > -		return false;
> > -	}
> > +	return entity && entity->type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
> >  }
> >  
> >  /**
> 
> 

-- 
Kind regards,

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

Re: [PATCH] media: Add type field to struct media_entity

[Laurent Pinchart]

Hello,

On Monday 22 February 2016 23:20:58 Sakari Ailus wrote:
> On Mon, Feb 22, 2016 at 06:46:01AM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 22 Feb 2016 03:53:16 +0200 Laurent Pinchart escreveu:
> >> Code that processes media entities can require knowledge of the
> >> structure type that embeds a particular media entity instance in order
> >> to use the API provided by that structure. This needs is shown by the
> >> presence of the is_media_entity_v4l2_io and is_media_entity_v4l2_subdev
> >> functions.
> >> 
> >> The implementation of those two functions relies on the entity function
> >> field, which is both a wrong and an inefficient design, without even
> 
> I wouldn't necessarily say "wrong", but it is risky. A device's function not
> only defines the interface it offers but also which struct is considered to
> contain the media entity. Having a wrong value in the function field may
> thus lead memory corruption and / or system crash.
> 
> >> mentioning the maintenance issue involved in updating the functions
> >> every time a new entity function is added. Fix this by adding add a type
> >> field to the media entity structure to carry the information.
> >> 
> >> Signed-off-by: Laurent Pinchart
> >> <laurent.pinchart+renesas@ideasonboard.com>
> >> ---
> >> 
> >>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>  include/media/media-entity.h          | 65 +++++++++++------------------
> >>  3 files changed, 30 insertions(+), 37 deletions(-)

[snip]

> >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> >> index fe485d367985..2be38483f3a4 100644
> >> --- a/include/media/media-entity.h
> >> +++ b/include/media/media-entity.h
> >> @@ -187,10 +187,27 @@ struct media_entity_operations {
> >>  };
> >>  
> >>  /**
> >> + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
> >> + *
> > 
> > s/MEDIA_ENTITY_TYPE_NONE/media_entity_type/
> > 
> > (it seems you didn't test producing the docbook, otherwise you would
> > have seen this error - Please always generate the docbook when the
> > patch contains kernel-doc markups)

Oops, sorry. I'll fix that.

> > I don't like the idea of calling it as "type", as this is confusing,
> > specially since we used to call entity.type for what we now call function.
> 
> What that field essentially defines is which struct embeds the media entity.
> (Well, there's some cleanups to be done there, as we have extra entity for
> V4L2 subdevices, but that's another story.)
> 
> The old type field had that information, plus the "function" of the entity.
> 
> I think "type" isn't a bad name for this field, as what we would really need
> is inheritance. It refers to the object type. What would you think of
> "class"?

I'd prefer type as class has other meanings in the kernel, but I can live with 
it. Mauro, I agree with Sakari here, what the field contains is really the 
object type in an object-oriented programming context.

> > What we're actually wanting to represent is the Linux kABI group where
> > the entity belongs. So, maybe we could call it as
> > media_entity_kabi_type, instead.
> > 
> > > + * @MEDIA_ENTITY_TYPE_NONE:
> > > + *	The entity isn't embedded in a standard structure.
> > 
> > I also don't like having a NONE here. All objects belong to some
> > kABI type, but not all subsystems need to use this field
> > (so far, DVB doesn't need nor ALSA).
> > 
> > So, I would either call it as DEFAULT or UNDEFINED.
> 
> I prefer UNDEFINED from the two. There really is no interface in that case,
> and we don't have a "default" interface either.

I prefer UNDEFINED too, I'll fix that.

> >> + * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
> >> + *	The media entity is embedded in a struct video_device.
> >> + * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
> >> + *	The media entity is embedded in a struct v4l2_subdev.
> >> + */
> >> +enum media_entity_type {
> >> +	MEDIA_ENTITY_TYPE_NONE,
> >> +	MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
> >> +	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
> >> +};

[snip]

> >> @@ -328,56 +346,29 @@ static inline u32 media_gobj_gen_id(enum
> >> media_gobj_type type, u64 local_id)
> >>  }
> >>  
> >>  /**
> >> - * is_media_entity_v4l2_io() - identify if the entity main function
> >> - *			       is a V4L2 I/O
> >> - *
> >> + * is_media_entity_v4l2_io() - Check if the entity implements the
> >> video_device
> >> + *			       API
> >>   * @entity:	pointer to entity
> >>   *
> >> - * Return: true if the entity main function is one of the V4L2 I/O
> >> types
> >> - *	(video, VBI or SDR radio); false otherwise.
> >> + * Return: true if the entity implement the video_device API (is
> >> directly
> >> + * embedded in a struct video_device instance) or false otherwise.
> > 
> > s/implement/implements/

Will fix.

> > Yet, I don't think the above comment is ok. First of all, video_device is
> > a kABI. We're nowadays calling the kernel APIs as kABI, and the userspace
> > ones as uAPI.
> 
> Are the exact definitions of the two available somewhere? ABI doesn't matter
> much in the kernel itself but towards user space both ABI and API are
> important...

the Kernel ABI is defined as the binary interface exported by the kernel to 
modules. I don't think that's relevant here.

> > Also, it doesn't make clear that it would be used also for radio, and
> > it is repeating the same thing twice.
> > 
> > So, I would either keep the original comment or change it to:
> >
> > "Return: true if the entity implements the video_device kABI for video,
> >  VBI or SDR radio (e. g. if the entity is embeddded at a struct
> >  video_device instance) or false otherwise."

The original comment isn't correct, as the is_media_entity_v4l2_io() doesn't 
care about the entity main function. I don't think we need to mention video, 
VBI or radio, this is really about whether the entity is embedded in a 
video_device structure. How about

"Return: true if the entity is embedded in a struct video_device instance or 
false otherwise."

> >>   */
> >>  static inline bool is_media_entity_v4l2_io(struct media_entity *entity)
> >>  {
> >> -	if (!entity)
> >> -		return false;
> >> -
> >> -	switch (entity->function) {
> >> -	case MEDIA_ENT_F_IO_V4L:
> >> -	case MEDIA_ENT_F_IO_VBI:
> >> -	case MEDIA_ENT_F_IO_SWRADIO:
> >> -		return true;
> >> -	default:
> >> -		return false;
> >> -	}
> >> +	return entity && entity->type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
> >>  }

[snip]

I'm happy to see that the comments are mostly about details and that the 
overall idea doesn't raise any strong opposition :-) Let's try to sort the 
last issues out and get this merged.

By the way, this patch is a prerequisite to fix the warnings printed by the MC 
core for the vsp1 driver since v4.5-rc1. Should it thus be merged as a fix for 
v4.5 ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: Add type field to struct media_entity

[Mauro Carvalho Chehab]

Em Fri, 26 Feb 2016 13:18:30 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hello,
> 
> On Monday 22 February 2016 23:20:58 Sakari Ailus wrote:
> > On Mon, Feb 22, 2016 at 06:46:01AM -0300, Mauro Carvalho Chehab wrote:  
> > > Em Mon, 22 Feb 2016 03:53:16 +0200 Laurent Pinchart escreveu:  
> > >> Code that processes media entities can require knowledge of the
> > >> structure type that embeds a particular media entity instance in order
> > >> to use the API provided by that structure. This needs is shown by the
> > >> presence of the is_media_entity_v4l2_io and is_media_entity_v4l2_subdev
> > >> functions.
> > >> 
> > >> The implementation of those two functions relies on the entity function
> > >> field, which is both a wrong and an inefficient design, without even  
> > 
> > I wouldn't necessarily say "wrong", but it is risky. A device's function not
> > only defines the interface it offers but also which struct is considered to
> > contain the media entity. Having a wrong value in the function field may
> > thus lead memory corruption and / or system crash.
> >   
> > >> mentioning the maintenance issue involved in updating the functions
> > >> every time a new entity function is added. Fix this by adding add a type
> > >> field to the media entity structure to carry the information.
> > >> 
> > >> Signed-off-by: Laurent Pinchart
> > >> <laurent.pinchart+renesas@ideasonboard.com>
> > >> ---
> > >> 
> > >>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> > >>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> > >>  include/media/media-entity.h          | 65 +++++++++++------------------
> > >>  3 files changed, 30 insertions(+), 37 deletions(-)  
> 
> [snip]
> 
> > >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > >> index fe485d367985..2be38483f3a4 100644
> > >> --- a/include/media/media-entity.h
> > >> +++ b/include/media/media-entity.h
> > >> @@ -187,10 +187,27 @@ struct media_entity_operations {
> > >>  };
> > >>  
> > >>  /**
> > >> + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
> > >> + *  
> > > 
> > > s/MEDIA_ENTITY_TYPE_NONE/media_entity_type/
> > > 
> > > (it seems you didn't test producing the docbook, otherwise you would
> > > have seen this error - Please always generate the docbook when the
> > > patch contains kernel-doc markups)  
> 
> Oops, sorry. I'll fix that.
> 
> > > I don't like the idea of calling it as "type", as this is confusing,
> > > specially since we used to call entity.type for what we now call function.  
> > 
> > What that field essentially defines is which struct embeds the media entity.
> > (Well, there's some cleanups to be done there, as we have extra entity for
> > V4L2 subdevices, but that's another story.)
> > 
> > The old type field had that information, plus the "function" of the entity.
> > 
> > I think "type" isn't a bad name for this field, as what we would really need
> > is inheritance. It refers to the object type. What would you think of
> > "class"?  
> 
> I'd prefer type as class has other meanings in the kernel, but I can live with 
> it. Mauro, I agree with Sakari here, what the field contains is really the 
> object type in an object-oriented programming context.

Well, as we could have entities not embedded on some other object, this
is actually not an object type, even on OO programming. What we're actually
representing here is a graph object class.

The problem is that "type" is a very generic term, and, as we used it before
with some other meaning, so I prefer to call it as something else.

I'm ok with any other name, although I agree that Kernel uses "class" for
other things. Maybe gr_class or obj_class?

> 
> > > What we're actually wanting to represent is the Linux kABI group where
> > > the entity belongs. So, maybe we could call it as
> > > media_entity_kabi_type, instead.
> > >   
> > > > + * @MEDIA_ENTITY_TYPE_NONE:
> > > > + *	The entity isn't embedded in a standard structure.  
> > > 
> > > I also don't like having a NONE here. All objects belong to some
> > > kABI type, but not all subsystems need to use this field
> > > (so far, DVB doesn't need nor ALSA).
> > > 
> > > So, I would either call it as DEFAULT or UNDEFINED.  
> > 
> > I prefer UNDEFINED from the two. There really is no interface in that case,
> > and we don't have a "default" interface either.  
> 
> I prefer UNDEFINED too, I'll fix that.

OK.

> 
> > >> + * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
> > >> + *	The media entity is embedded in a struct video_device.
> > >> + * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
> > >> + *	The media entity is embedded in a struct v4l2_subdev.
> > >> + */
> > >> +enum media_entity_type {
> > >> +	MEDIA_ENTITY_TYPE_NONE,
> > >> +	MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
> > >> +	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
> > >> +};  
> 
> [snip]
> 
> > >> @@ -328,56 +346,29 @@ static inline u32 media_gobj_gen_id(enum
> > >> media_gobj_type type, u64 local_id)
> > >>  }
> > >>  
> > >>  /**
> > >> - * is_media_entity_v4l2_io() - identify if the entity main function
> > >> - *			       is a V4L2 I/O
> > >> - *
> > >> + * is_media_entity_v4l2_io() - Check if the entity implements the
> > >> video_device
> > >> + *			       API
> > >>   * @entity:	pointer to entity
> > >>   *
> > >> - * Return: true if the entity main function is one of the V4L2 I/O
> > >> types
> > >> - *	(video, VBI or SDR radio); false otherwise.
> > >> + * Return: true if the entity implement the video_device API (is
> > >> directly
> > >> + * embedded in a struct video_device instance) or false otherwise.  
> > > 
> > > s/implement/implements/  
> 
> Will fix.
> 
> > > Yet, I don't think the above comment is ok. First of all, video_device is
> > > a kABI. We're nowadays calling the kernel APIs as kABI, and the userspace
> > > ones as uAPI.  
> > 
> > Are the exact definitions of the two available somewhere? ABI doesn't matter
> > much in the kernel itself but towards user space both ABI and API are
> > important...  
> 
> the Kernel ABI is defined as the binary interface exported by the kernel to 
> modules. I don't think that's relevant here.

Maybe we could then call it as kAPI, do distinguish from uAPI.

> 
> > > Also, it doesn't make clear that it would be used also for radio, and
> > > it is repeating the same thing twice.
> > > 
> > > So, I would either keep the original comment or change it to:
> > >
> > > "Return: true if the entity implements the video_device kABI for video,
> > >  VBI or SDR radio (e. g. if the entity is embeddded at a struct
> > >  video_device instance) or false otherwise."  
> 
> The original comment isn't correct, as the is_media_entity_v4l2_io() doesn't 
> care about the entity main function. I don't think we need to mention video, 
> VBI or radio, this is really about whether the entity is embedded in a 
> video_device structure. How about
> 
> "Return: true if the entity is embedded in a struct video_device instance or 
> false otherwise."

Works for me.

> 
> > >>   */
> > >>  static inline bool is_media_entity_v4l2_io(struct media_entity *entity)
> > >>  {
> > >> -	if (!entity)
> > >> -		return false;
> > >> -
> > >> -	switch (entity->function) {
> > >> -	case MEDIA_ENT_F_IO_V4L:
> > >> -	case MEDIA_ENT_F_IO_VBI:
> > >> -	case MEDIA_ENT_F_IO_SWRADIO:
> > >> -		return true;
> > >> -	default:
> > >> -		return false;
> > >> -	}
> > >> +	return entity && entity->type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
> > >>  }  
> 
> [snip]
> 
> I'm happy to see that the comments are mostly about details and that the 
> overall idea doesn't raise any strong opposition :-) Let's try to sort the 
> last issues out and get this merged.
> 
> By the way, this patch is a prerequisite to fix the warnings printed by the MC 
> core for the vsp1 driver since v4.5-rc1. Should it thus be merged as a fix for 
> v4.5 ?

This patch itself won't remove the warnings, but the followup ones that
you're likely writing adding the proper entity functions to each
subdev used by vsp1. The very same thing is also needed for omap3 and
some other drivers.

I guess there's no hurry to merge such warnings removal on v4.5. The entity 
types on vsp1 driver are not properly reported even before 4.5. The only
thing is that, before 4.5, those undefined entities were silently ignored by 
the MC core. So, I don't see any urgency.

Regards,
Mauro

Re: [PATCH] media: Add type field to struct media_entity

[Hans Verkuil]

On 02/26/2016 02:21 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 26 Feb 2016 13:18:30 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> 
>> Hello,
>>
>> On Monday 22 February 2016 23:20:58 Sakari Ailus wrote:
>>> On Mon, Feb 22, 2016 at 06:46:01AM -0300, Mauro Carvalho Chehab wrote:  
>>>> Em Mon, 22 Feb 2016 03:53:16 +0200 Laurent Pinchart escreveu:  
>>>>> Code that processes media entities can require knowledge of the
>>>>> structure type that embeds a particular media entity instance in order
>>>>> to use the API provided by that structure. This needs is shown by the
>>>>> presence of the is_media_entity_v4l2_io and is_media_entity_v4l2_subdev
>>>>> functions.
>>>>>
>>>>> The implementation of those two functions relies on the entity function
>>>>> field, which is both a wrong and an inefficient design, without even  
>>>
>>> I wouldn't necessarily say "wrong", but it is risky. A device's function not
>>> only defines the interface it offers but also which struct is considered to
>>> contain the media entity. Having a wrong value in the function field may
>>> thus lead memory corruption and / or system crash.
>>>   
>>>>> mentioning the maintenance issue involved in updating the functions
>>>>> every time a new entity function is added. Fix this by adding add a type
>>>>> field to the media entity structure to carry the information.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart
>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>>> ---
>>>>>
>>>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
>>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
>>>>>  include/media/media-entity.h          | 65 +++++++++++------------------
>>>>>  3 files changed, 30 insertions(+), 37 deletions(-)  
>>
>> [snip]
>>
>>>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>>>> index fe485d367985..2be38483f3a4 100644
>>>>> --- a/include/media/media-entity.h
>>>>> +++ b/include/media/media-entity.h
>>>>> @@ -187,10 +187,27 @@ struct media_entity_operations {
>>>>>  };
>>>>>  
>>>>>  /**
>>>>> + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
>>>>> + *  
>>>>
>>>> s/MEDIA_ENTITY_TYPE_NONE/media_entity_type/
>>>>
>>>> (it seems you didn't test producing the docbook, otherwise you would
>>>> have seen this error - Please always generate the docbook when the
>>>> patch contains kernel-doc markups)  
>>
>> Oops, sorry. I'll fix that.
>>
>>>> I don't like the idea of calling it as "type", as this is confusing,
>>>> specially since we used to call entity.type for what we now call function.  
>>>
>>> What that field essentially defines is which struct embeds the media entity.
>>> (Well, there's some cleanups to be done there, as we have extra entity for
>>> V4L2 subdevices, but that's another story.)
>>>
>>> The old type field had that information, plus the "function" of the entity.
>>>
>>> I think "type" isn't a bad name for this field, as what we would really need
>>> is inheritance. It refers to the object type. What would you think of
>>> "class"?  
>>
>> I'd prefer type as class has other meanings in the kernel, but I can live with 
>> it. Mauro, I agree with Sakari here, what the field contains is really the 
>> object type in an object-oriented programming context.
> 
> Well, as we could have entities not embedded on some other object, this
> is actually not an object type, even on OO programming. What we're actually
> representing here is a graph object class.
> 
> The problem is that "type" is a very generic term, and, as we used it before
> with some other meaning, so I prefer to call it as something else.
> 
> I'm ok with any other name, although I agree that Kernel uses "class" for
> other things. Maybe gr_class or obj_class?

I had to think about this a bit, but IMHO it is an entity classification that
a subsystem sets when creating the entity.

So v4l2 has the classifications V4L2_SUBDEV and V4L2_IO. And while all entities of the
V4L2_SUBDEV classification are indeed embedded in a struct v4l2_subdev, that is not
true for V4L2_IO (radio interface entities are embedded in struct video_device, but
are not of the V4L2_IO class).

Other subsystems may need other classifications.

So what about this:

enum media_entity_class {
	MEDIA_ENTITY_CLASS_UNDEFINED, // Actually, CLASS_NONE would work here too
	MEDIA_ENTITY_CLASS_V4L2_IO,
	MEDIA_ENTITY_CLASS_V4L2_SUBDEV,
};

and the field enum media_entity_class class; in struct media_entity with documentation:

@class:	Classification of the media_entity, subsystems can set this to quickly classify
	what sort of media_entity this is.

Regards,

	Hans

Re: [PATCH] media: Add type field to struct media_entity

[Mauro Carvalho Chehab]

Em Fri, 26 Feb 2016 15:00:06 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 02/26/2016 02:21 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 26 Feb 2016 13:18:30 +0200
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> >   
> >> Hello,
> >>
> >> On Monday 22 February 2016 23:20:58 Sakari Ailus wrote:  
> >>> On Mon, Feb 22, 2016 at 06:46:01AM -0300, Mauro Carvalho Chehab wrote:    
> >>>> Em Mon, 22 Feb 2016 03:53:16 +0200 Laurent Pinchart escreveu:    
> >>>>> Code that processes media entities can require knowledge of the
> >>>>> structure type that embeds a particular media entity instance in order
> >>>>> to use the API provided by that structure. This needs is shown by the
> >>>>> presence of the is_media_entity_v4l2_io and is_media_entity_v4l2_subdev
> >>>>> functions.
> >>>>>
> >>>>> The implementation of those two functions relies on the entity function
> >>>>> field, which is both a wrong and an inefficient design, without even    
> >>>
> >>> I wouldn't necessarily say "wrong", but it is risky. A device's function not
> >>> only defines the interface it offers but also which struct is considered to
> >>> contain the media entity. Having a wrong value in the function field may
> >>> thus lead memory corruption and / or system crash.
> >>>     
> >>>>> mentioning the maintenance issue involved in updating the functions
> >>>>> every time a new entity function is added. Fix this by adding add a type
> >>>>> field to the media entity structure to carry the information.
> >>>>>
> >>>>> Signed-off-by: Laurent Pinchart
> >>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>> ---
> >>>>>
> >>>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>>>>  include/media/media-entity.h          | 65 +++++++++++------------------
> >>>>>  3 files changed, 30 insertions(+), 37 deletions(-)    
> >>
> >> [snip]
> >>  
> >>>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> >>>>> index fe485d367985..2be38483f3a4 100644
> >>>>> --- a/include/media/media-entity.h
> >>>>> +++ b/include/media/media-entity.h
> >>>>> @@ -187,10 +187,27 @@ struct media_entity_operations {
> >>>>>  };
> >>>>>  
> >>>>>  /**
> >>>>> + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
> >>>>> + *    
> >>>>
> >>>> s/MEDIA_ENTITY_TYPE_NONE/media_entity_type/
> >>>>
> >>>> (it seems you didn't test producing the docbook, otherwise you would
> >>>> have seen this error - Please always generate the docbook when the
> >>>> patch contains kernel-doc markups)    
> >>
> >> Oops, sorry. I'll fix that.
> >>  
> >>>> I don't like the idea of calling it as "type", as this is confusing,
> >>>> specially since we used to call entity.type for what we now call function.    
> >>>
> >>> What that field essentially defines is which struct embeds the media entity.
> >>> (Well, there's some cleanups to be done there, as we have extra entity for
> >>> V4L2 subdevices, but that's another story.)
> >>>
> >>> The old type field had that information, plus the "function" of the entity.
> >>>
> >>> I think "type" isn't a bad name for this field, as what we would really need
> >>> is inheritance. It refers to the object type. What would you think of
> >>> "class"?    
> >>
> >> I'd prefer type as class has other meanings in the kernel, but I can live with 
> >> it. Mauro, I agree with Sakari here, what the field contains is really the 
> >> object type in an object-oriented programming context.  
> > 
> > Well, as we could have entities not embedded on some other object, this
> > is actually not an object type, even on OO programming. What we're actually
> > representing here is a graph object class.
> > 
> > The problem is that "type" is a very generic term, and, as we used it before
> > with some other meaning, so I prefer to call it as something else.
> > 
> > I'm ok with any other name, although I agree that Kernel uses "class" for
> > other things. Maybe gr_class or obj_class?  
> 
> I had to think about this a bit, but IMHO it is an entity classification that
> a subsystem sets when creating the entity.
> 
> So v4l2 has the classifications V4L2_SUBDEV and V4L2_IO. And while all entities of the
> V4L2_SUBDEV classification are indeed embedded in a struct v4l2_subdev, that is not
> true for V4L2_IO (radio interface entities are embedded in struct video_device, but
> are not of the V4L2_IO class).
> 
> Other subsystems may need other classifications.
> 
> So what about this:
> 
> enum media_entity_class {
> 	MEDIA_ENTITY_CLASS_UNDEFINED, // Actually, CLASS_NONE would work here too
> 	MEDIA_ENTITY_CLASS_V4L2_IO,
> 	MEDIA_ENTITY_CLASS_V4L2_SUBDEV,
> };
> 
> and the field enum media_entity_class class; in struct media_entity with documentation:
> 
> @class:	Classification of the media_entity, subsystems can set this to quickly classify
> 	what sort of media_entity this is.

Works for me.

> 
> Regards,
> 
> 	Hans


-- 
Thanks,
Mauro

Re: [PATCH] media: Add type field to struct media_entity

[Laurent Pinchart]

Hi Mauro,

On Friday 26 February 2016 10:21:41 Mauro Carvalho Chehab wrote:
> Em Fri, 26 Feb 2016 13:18:30 +0200 Laurent Pinchart escreveu:
> > On Monday 22 February 2016 23:20:58 Sakari Ailus wrote:
> >> On Mon, Feb 22, 2016 at 06:46:01AM -0300, Mauro Carvalho Chehab wrote:
> >>> Em Mon, 22 Feb 2016 03:53:16 +0200 Laurent Pinchart escreveu:
> >>>> Code that processes media entities can require knowledge of the
> >>>> structure type that embeds a particular media entity instance in
> >>>> order to use the API provided by that structure. This needs is shown
> >>>> by the presence of the is_media_entity_v4l2_io and
> >>>> is_media_entity_v4l2_subdev functions.
> >>>> 
> >>>> The implementation of those two functions relies on the entity
> >>>> function field, which is both a wrong and an inefficient design,
> >>>> without even
> >> 
> >> I wouldn't necessarily say "wrong", but it is risky. A device's function
> >> not only defines the interface it offers but also which struct is
> >> considered to contain the media entity. Having a wrong value in the
> >> function field may thus lead memory corruption and / or system crash.
> >> 
> >>>> mentioning the maintenance issue involved in updating the functions
> >>>> every time a new entity function is added. Fix this by adding add a
> >>>> type field to the media entity structure to carry the information.
> >>>> 
> >>>> Signed-off-by: Laurent Pinchart
> >>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>> ---
> >>> 
> >>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>>>  include/media/media-entity.h          | 65 ++++++++++-----------------
> >>>>  3 files changed, 30 insertions(+), 37 deletions(-)
> > 
> > [snip]
> > 
> >>>> diff --git a/include/media/media-entity.h
> >>>> b/include/media/media-entity.h
> >>>> index fe485d367985..2be38483f3a4 100644
> >>>> --- a/include/media/media-entity.h
> >>>> +++ b/include/media/media-entity.h
> >>>> @@ -187,10 +187,27 @@ struct media_entity_operations {
> >>>>  };
> >>>>  
> >>>>  /**
> >>>> + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
> >>>> + *

[snip]

> >>> I don't like the idea of calling it as "type", as this is confusing,
> >>> specially since we used to call entity.type for what we now call
> >>> function.
> >> 
> >> What that field essentially defines is which struct embeds the media
> >> entity. (Well, there's some cleanups to be done there, as we have extra
> >> entity for V4L2 subdevices, but that's another story.)
> >> 
> >> The old type field had that information, plus the "function" of the
> >> entity.
> >> 
> >> I think "type" isn't a bad name for this field, as what we would really
> >> need is inheritance. It refers to the object type. What would you think
> >> of "class"?
> > 
> > I'd prefer type as class has other meanings in the kernel, but I can live
> > with it. Mauro, I agree with Sakari here, what the field contains is
> > really the object type in an object-oriented programming context.
> 
> Well, as we could have entities not embedded on some other object, this
> is actually not an object type, even on OO programming.

I don't agree with that. From an OOP point of view, object instances have a 
type. A media_entity instance that is not embedded in another structure has 
the media_entity type.

> What we're actually representing here is a graph object class.

No we're not. It's not about graph objects. It's about knowing the type of a 
particular media_entity instance in order to safely cast it to the proper 
object for its type. It's the same concept as run-time type identification 
(see https://en.wikipedia.org/wiki/Run-time_type_information) in C++ for 
instance.

> The problem is that "type" is a very generic term, and, as we used it before
> with some other meaning, so I prefer to call it as something else.
> 
> I'm ok with any other name, although I agree that Kernel uses "class" for
> other things. Maybe gr_class or obj_class?

I could go for entity_type, but given that the field is located in a struct 
media_entity, the entity_ prefix is really redundant.

> >>> What we're actually wanting to represent is the Linux kABI group where
> >>> the entity belongs. So, maybe we could call it as
> >>> media_entity_kabi_type, instead.
> >>> 
> >>>> + * @MEDIA_ENTITY_TYPE_NONE:
> >>>> + *	The entity isn't embedded in a standard structure.
> >>> 
> >>> I also don't like having a NONE here. All objects belong to some
> >>> kABI type, but not all subsystems need to use this field
> >>> (so far, DVB doesn't need nor ALSA).
> >>> 
> >>> So, I would either call it as DEFAULT or UNDEFINED.
> >> 
> >> I prefer UNDEFINED from the two. There really is no interface in that
> >> case, and we don't have a "default" interface either.
> > 
> > I prefer UNDEFINED too, I'll fix that.
> 
> OK.
> 
> >>>> + * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
> >>>> + *	The media entity is embedded in a struct video_device.
> >>>> + * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
> >>>> + *	The media entity is embedded in a struct v4l2_subdev.
> >>>> + */
> >>>> +enum media_entity_type {
> >>>> +	MEDIA_ENTITY_TYPE_NONE,
> >>>> +	MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
> >>>> +	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
> >>>> +};
> > 
> > [snip]
> > 
> >>>> @@ -328,56 +346,29 @@ static inline u32 media_gobj_gen_id(enum
> >>>> media_gobj_type type, u64 local_id)
> >>>>  }
> >>>>  
> >>>>  /**
> >>>> - * is_media_entity_v4l2_io() - identify if the entity main function
> >>>> - *			       is a V4L2 I/O
> >>>> - *
> >>>> + * is_media_entity_v4l2_io() - Check if the entity implements the
> >>>> video_device
> >>>> + *			       API
> >>>>   * @entity:	pointer to entity
> >>>>   *
> >>>> - * Return: true if the entity main function is one of the V4L2 I/O
> >>>> types
> >>>> - *	(video, VBI or SDR radio); false otherwise.
> >>>> + * Return: true if the entity implement the video_device API (is
> >>>> directly
> >>>> + * embedded in a struct video_device instance) or false otherwise.
> >>> 
> >>> s/implement/implements/
> > 
> > Will fix.
> > 
> >>> Yet, I don't think the above comment is ok. First of all, video_device
> >>> is a kABI. We're nowadays calling the kernel APIs as kABI, and the
> >>> userspace ones as uAPI.
> >> 
> >> Are the exact definitions of the two available somewhere? ABI doesn't
> >> matter much in the kernel itself but towards user space both ABI and
> >> API are important...
> > 
> > the Kernel ABI is defined as the binary interface exported by the kernel
> > to modules. I don't think that's relevant here.
> 
> Maybe we could then call it as kAPI, do distinguish from uAPI.

Or just stop talking about ABI/API completely and just mention that the entity 
instance is a video_device or v4l2_subdev instance.

> >>> Also, it doesn't make clear that it would be used also for radio, and
> >>> it is repeating the same thing twice.
> >>> 
> >>> So, I would either keep the original comment or change it to:
> >>> 
> >>> "Return: true if the entity implements the video_device kABI for
> >>> video,
> >>> 
> >>>  VBI or SDR radio (e. g. if the entity is embeddded at a struct
> >>>  video_device instance) or false otherwise."
> > 
> > The original comment isn't correct, as the is_media_entity_v4l2_io()
> > doesn't care about the entity main function. I don't think we need to
> > mention video, VBI or radio, this is really about whether the entity is
> > embedded in a video_device structure. How about
> > 
> > "Return: true if the entity is embedded in a struct video_device instance
> > or false otherwise."
> 
> Works for me.

OK, I'll change the patch accordingly.

> >>>>   */

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: Add type field to struct media_entity

[Laurent Pinchart]

Hi Hans,

On Friday 26 February 2016 15:00:06 Hans Verkuil wrote:
> On 02/26/2016 02:21 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 26 Feb 2016 13:18:30 +0200 Laurent Pinchart escreveu:
> >> On Monday 22 February 2016 23:20:58 Sakari Ailus wrote:
> >>> On Mon, Feb 22, 2016 at 06:46:01AM -0300, Mauro Carvalho Chehab wrote:
> >>>> Em Mon, 22 Feb 2016 03:53:16 +0200 Laurent Pinchart escreveu:
> >>>>> Code that processes media entities can require knowledge of the
> >>>>> structure type that embeds a particular media entity instance in order
> >>>>> to use the API provided by that structure. This needs is shown by the
> >>>>> presence of the is_media_entity_v4l2_io and
> >>>>> is_media_entity_v4l2_subdev
> >>>>> functions.
> >>>>> 
> >>>>> The implementation of those two functions relies on the entity
> >>>>> function field, which is both a wrong and an inefficient design,
> >>>>> without even
> >>> 
> >>> I wouldn't necessarily say "wrong", but it is risky. A device's function
> >>> not only defines the interface it offers but also which struct is
> >>> considered to contain the media entity. Having a wrong value in the
> >>> function field may thus lead memory corruption and / or system crash.
> >>> 
> >>>>> mentioning the maintenance issue involved in updating the functions
> >>>>> every time a new entity function is added. Fix this by adding add a
> >>>>> type field to the media entity structure to carry the information.
> >>>>> 
> >>>>> Signed-off-by: Laurent Pinchart
> >>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>>>>  include/media/media-entity.h          | 65  +++++++++----------------
> >>>>>  3 files changed, 30 insertions(+), 37 deletions(-)
> >> 
> >> [snip]
> >> 
> >>>>> diff --git a/include/media/media-entity.h
> >>>>> b/include/media/media-entity.h
> >>>>> index fe485d367985..2be38483f3a4 100644
> >>>>> --- a/include/media/media-entity.h
> >>>>> +++ b/include/media/media-entity.h
> >>>>> @@ -187,10 +187,27 @@ struct media_entity_operations {
> >>>>>  };
> >>>>>  
> >>>>>  /**
> >>>>> + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
> >>>>> + *
> >>>> 
> >>>> s/MEDIA_ENTITY_TYPE_NONE/media_entity_type/
> >>>> 
> >>>> (it seems you didn't test producing the docbook, otherwise you would
> >>>> have seen this error - Please always generate the docbook when the
> >>>> patch contains kernel-doc markups)
> >> 
> >> Oops, sorry. I'll fix that.
> >> 
> >>>> I don't like the idea of calling it as "type", as this is confusing,
> >>>> specially since we used to call entity.type for what we now call
> >>>> function.
> >>> 
> >>> What that field essentially defines is which struct embeds the media
> >>> entity. (Well, there's some cleanups to be done there, as we have extra
> >>> entity for V4L2 subdevices, but that's another story.)
> >>> 
> >>> The old type field had that information, plus the "function" of the
> >>> entity.
> >>> 
> >>> I think "type" isn't a bad name for this field, as what we would really
> >>> need is inheritance. It refers to the object type. What would you think
> >>> of "class"?
> >> 
> >> I'd prefer type as class has other meanings in the kernel, but I can live
> >> with it. Mauro, I agree with Sakari here, what the field contains is
> >> really the object type in an object-oriented programming context.
> > 
> > Well, as we could have entities not embedded on some other object, this
> > is actually not an object type, even on OO programming. What we're
> > actually representing here is a graph object class.
> > 
> > The problem is that "type" is a very generic term, and, as we used it
> > before with some other meaning, so I prefer to call it as something else.
> > 
> > I'm ok with any other name, although I agree that Kernel uses "class" for
> > other things. Maybe gr_class or obj_class?
> 
> I had to think about this a bit, but IMHO it is an entity classification
> that a subsystem sets when creating the entity.
>
> So v4l2 has the classifications V4L2_SUBDEV and V4L2_IO. And while all
> entities of the V4L2_SUBDEV classification are indeed embedded in a struct
> v4l2_subdev, that is not true for V4L2_IO (radio interface entities are
> embedded in struct video_device, but are not of the V4L2_IO class).
> 
> Other subsystems may need other classifications.
> 
> So what about this:
> 
> enum media_entity_class {
> 	MEDIA_ENTITY_CLASS_UNDEFINED, // Actually, CLASS_NONE would work here too
> 	MEDIA_ENTITY_CLASS_V4L2_IO,
> 	MEDIA_ENTITY_CLASS_V4L2_SUBDEV,
> };

The purpose of the type is solely to identify the type of the media_entity 
instance to safely cast it to the proper object type (in an OOP sense). That's 
what I want the name of the field to describe. It's not about a 
classification, it's about object instance type identification.

>From that point of view, the V4L2_IO class/type is wrong. We want to tell that 
the entity instance is a video_device instance (and given that we use C, this 
OOP construct is implemented by embedding the struct media_entity in a struct 
video_device). We really want VIDEO_DEVICE here, there is no struct v4l2_io.

> and the field enum media_entity_class class; in struct media_entity with
> documentation:
> 
> @class:	Classification of the media_entity, subsystems can set this to
> quickly classify what sort of media_entity this is.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: Add type field to struct media_entity

[Hans Verkuil]

On 02/28/2016 08:09 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 26 February 2016 15:00:06 Hans Verkuil wrote:
>> On 02/26/2016 02:21 PM, Mauro Carvalho Chehab wrote:
>>> Em Fri, 26 Feb 2016 13:18:30 +0200 Laurent Pinchart escreveu:
>>>> On Monday 22 February 2016 23:20:58 Sakari Ailus wrote:
>>>>> On Mon, Feb 22, 2016 at 06:46:01AM -0300, Mauro Carvalho Chehab wrote:
>>>>>> Em Mon, 22 Feb 2016 03:53:16 +0200 Laurent Pinchart escreveu:
>>>>>>> Code that processes media entities can require knowledge of the
>>>>>>> structure type that embeds a particular media entity instance in order
>>>>>>> to use the API provided by that structure. This needs is shown by the
>>>>>>> presence of the is_media_entity_v4l2_io and
>>>>>>> is_media_entity_v4l2_subdev
>>>>>>> functions.
>>>>>>>
>>>>>>> The implementation of those two functions relies on the entity
>>>>>>> function field, which is both a wrong and an inefficient design,
>>>>>>> without even
>>>>>
>>>>> I wouldn't necessarily say "wrong", but it is risky. A device's function
>>>>> not only defines the interface it offers but also which struct is
>>>>> considered to contain the media entity. Having a wrong value in the
>>>>> function field may thus lead memory corruption and / or system crash.
>>>>>
>>>>>>> mentioning the maintenance issue involved in updating the functions
>>>>>>> every time a new entity function is added. Fix this by adding add a
>>>>>>> type field to the media entity structure to carry the information.
>>>>>>>
>>>>>>> Signed-off-by: Laurent Pinchart
>>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
>>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
>>>>>>>  include/media/media-entity.h          | 65  +++++++++----------------
>>>>>>>  3 files changed, 30 insertions(+), 37 deletions(-)
>>>>
>>>> [snip]
>>>>
>>>>>>> diff --git a/include/media/media-entity.h
>>>>>>> b/include/media/media-entity.h
>>>>>>> index fe485d367985..2be38483f3a4 100644
>>>>>>> --- a/include/media/media-entity.h
>>>>>>> +++ b/include/media/media-entity.h
>>>>>>> @@ -187,10 +187,27 @@ struct media_entity_operations {
>>>>>>>  };
>>>>>>>  
>>>>>>>  /**
>>>>>>> + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
>>>>>>> + *
>>>>>>
>>>>>> s/MEDIA_ENTITY_TYPE_NONE/media_entity_type/
>>>>>>
>>>>>> (it seems you didn't test producing the docbook, otherwise you would
>>>>>> have seen this error - Please always generate the docbook when the
>>>>>> patch contains kernel-doc markups)
>>>>
>>>> Oops, sorry. I'll fix that.
>>>>
>>>>>> I don't like the idea of calling it as "type", as this is confusing,
>>>>>> specially since we used to call entity.type for what we now call
>>>>>> function.
>>>>>
>>>>> What that field essentially defines is which struct embeds the media
>>>>> entity. (Well, there's some cleanups to be done there, as we have extra
>>>>> entity for V4L2 subdevices, but that's another story.)
>>>>>
>>>>> The old type field had that information, plus the "function" of the
>>>>> entity.
>>>>>
>>>>> I think "type" isn't a bad name for this field, as what we would really
>>>>> need is inheritance. It refers to the object type. What would you think
>>>>> of "class"?
>>>>
>>>> I'd prefer type as class has other meanings in the kernel, but I can live
>>>> with it. Mauro, I agree with Sakari here, what the field contains is
>>>> really the object type in an object-oriented programming context.
>>>
>>> Well, as we could have entities not embedded on some other object, this
>>> is actually not an object type, even on OO programming. What we're
>>> actually representing here is a graph object class.
>>>
>>> The problem is that "type" is a very generic term, and, as we used it
>>> before with some other meaning, so I prefer to call it as something else.
>>>
>>> I'm ok with any other name, although I agree that Kernel uses "class" for
>>> other things. Maybe gr_class or obj_class?
>>
>> I had to think about this a bit, but IMHO it is an entity classification
>> that a subsystem sets when creating the entity.
>>
>> So v4l2 has the classifications V4L2_SUBDEV and V4L2_IO. And while all
>> entities of the V4L2_SUBDEV classification are indeed embedded in a struct
>> v4l2_subdev, that is not true for V4L2_IO (radio interface entities are
>> embedded in struct video_device, but are not of the V4L2_IO class).
>>
>> Other subsystems may need other classifications.
>>
>> So what about this:
>>
>> enum media_entity_class {
>> 	MEDIA_ENTITY_CLASS_UNDEFINED, // Actually, CLASS_NONE would work here too
>> 	MEDIA_ENTITY_CLASS_V4L2_IO,
>> 	MEDIA_ENTITY_CLASS_V4L2_SUBDEV,
>> };
> 
> The purpose of the type is solely to identify the type of the media_entity 
> instance to safely cast it to the proper object type (in an OOP sense). That's 
> what I want the name of the field to describe. It's not about a 
> classification, it's about object instance type identification.

No, it's not. The way it is used is to find entities that are embedded in a
video_device AND that do I/O. While all I/O V4L2 entities are a video_device,
the reverse is not true. Most obviously radio devices, but video devices can
also be used for control only and not for I/O. For example (a real-life example)
a sensor -> FPGA -> HDMI TX pipeline where video devices are used to control
the sensor and HDMI transmitter, but no I/O to/from memory happens since that's
all inside the FPGA.

So this really is CLASS_V4L2_IO.

And the name is_media_entity_v4l2_io is OK too.

This could be done differently, though, but it requires more work.

I do think it would be useful to know that the entity is embedded in the video_device,
so I agree with you on that.

But to make is_media_entity_v4l2_io work you would need to know if the device
could do I/O. One way this can be done is to make the device_caps of v4l2_capabilities
part of struct video_device.

This would have other beneficial results as well since the core can now know the
caps of the device and it can take decisions accordingly. I've thought about
this before but never had enough of a reason to implement it.

So the is_media_entity_v4l2_io() function would check if the entity is of type
VIDEO_DEVICE, then use container_of to get the caps from video_device and check for
(CAP_STREAMING | CAP_READWRITE).

In this case I would call it:

enum media_entity_is_of_type {
	MEDIA_ENTITY_TYPE_UNDEFINED,
	MEDIA_ENTITY_TYPE_V4L2_VIDEO_DEVICE,
	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
};

And the field name would be:

	enum media_entity_is_of_type type;

But this requires more work since all drivers need to be modified, starting with
those used by those drivers are also use the is_media_entity_* functions. I do
think it would be beneficial to make the device_caps part of video_device
regardless.

Regards,

	Hans

> From that point of view, the V4L2_IO class/type is wrong. We want to tell that 
> the entity instance is a video_device instance (and given that we use C, this 
> OOP construct is implemented by embedding the struct media_entity in a struct 
> video_device). We really want VIDEO_DEVICE here, there is no struct v4l2_io.
> 
>> and the field enum media_entity_class class; in struct media_entity with
>> documentation:
>>
>> @class:	Classification of the media_entity, subsystems can set this to
>> quickly classify what sort of media_entity this is.
> 

Re: [PATCH] media: Add type field to struct media_entity

[Laurent Pinchart]

Hi Hans,

On Monday 29 February 2016 09:28:18 Hans Verkuil wrote:
> On 02/28/2016 08:09 PM, Laurent Pinchart wrote:
> > On Friday 26 February 2016 15:00:06 Hans Verkuil wrote:
> >> On 02/26/2016 02:21 PM, Mauro Carvalho Chehab wrote:
> >>> Em Fri, 26 Feb 2016 13:18:30 +0200 Laurent Pinchart escreveu:
> >>>> On Monday 22 February 2016 23:20:58 Sakari Ailus wrote:
> >>>>> On Mon, Feb 22, 2016 at 06:46:01AM -0300, Mauro Carvalho Chehab wrote:
> >>>>>> Em Mon, 22 Feb 2016 03:53:16 +0200 Laurent Pinchart escreveu:
> >>>>>>> Code that processes media entities can require knowledge of the
> >>>>>>> structure type that embeds a particular media entity instance in
> >>>>>>> order to use the API provided by that structure. This needs is shown
> >>>>>>> by the presence of the is_media_entity_v4l2_io and
> >>>>>>> is_media_entity_v4l2_subdev
> >>>>>>> functions.
> >>>>>>> 
> >>>>>>> The implementation of those two functions relies on the entity
> >>>>>>> function field, which is both a wrong and an inefficient design,
> >>>>>>> without even
> >>>>> 
> >>>>> I wouldn't necessarily say "wrong", but it is risky. A device's
> >>>>> function not only defines the interface it offers but also which
> >>>>> struct is considered to contain the media entity. Having a wrong value
> >>>>> in the function field may thus lead memory corruption and / or system
> >>>>> crash.
> >>>>> 
> >>>>>>> mentioning the maintenance issue involved in updating the functions
> >>>>>>> every time a new entity function is added. Fix this by adding add a
> >>>>>>> type field to the media entity structure to carry the information.
> >>>>>>> 
> >>>>>>> Signed-off-by: Laurent Pinchart
> >>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>>>>>>  include/media/media-entity.h          | 65 +++++++++--------------
> >>>>>>>  3 files changed, 30 insertions(+), 37 deletions(-)
> >>>> 
> >>>> [snip]
> >>>> 
> >>>>>>> diff --git a/include/media/media-entity.h
> >>>>>>> b/include/media/media-entity.h
> >>>>>>> index fe485d367985..2be38483f3a4 100644
> >>>>>>> --- a/include/media/media-entity.h
> >>>>>>> +++ b/include/media/media-entity.h
> >>>>>>> @@ -187,10 +187,27 @@ struct media_entity_operations {
> >>>>>>>  };
> >>>>>>>  
> >>>>>>>  /**
> >>>>>>> + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
> >>>>>>> + *
> >>>>>> 
> >>>>>> s/MEDIA_ENTITY_TYPE_NONE/media_entity_type/
> >>>>>> 
> >>>>>> (it seems you didn't test producing the docbook, otherwise you would
> >>>>>> have seen this error - Please always generate the docbook when the
> >>>>>> patch contains kernel-doc markups)
> >>>> 
> >>>> Oops, sorry. I'll fix that.
> >>>> 
> >>>>>> I don't like the idea of calling it as "type", as this is confusing,
> >>>>>> specially since we used to call entity.type for what we now call
> >>>>>> function.
> >>>>> 
> >>>>> What that field essentially defines is which struct embeds the media
> >>>>> entity. (Well, there's some cleanups to be done there, as we have
> >>>>> extra entity for V4L2 subdevices, but that's another story.)
> >>>>> 
> >>>>> The old type field had that information, plus the "function" of the
> >>>>> entity.
> >>>>> 
> >>>>> I think "type" isn't a bad name for this field, as what we would
> >>>>> really need is inheritance. It refers to the object type. What would
> >>>>> you think of "class"?
> >>>> 
> >>>> I'd prefer type as class has other meanings in the kernel, but I can
> >>>> live with it. Mauro, I agree with Sakari here, what the field contains
> >>>> is really the object type in an object-oriented programming context.
> >>> 
> >>> Well, as we could have entities not embedded on some other object, this
> >>> is actually not an object type, even on OO programming. What we're
> >>> actually representing here is a graph object class.
> >>> 
> >>> The problem is that "type" is a very generic term, and, as we used it
> >>> before with some other meaning, so I prefer to call it as something
> >>> else.
> >>> 
> >>> I'm ok with any other name, although I agree that Kernel uses "class"
> >>> for other things. Maybe gr_class or obj_class?
> >> 
> >> I had to think about this a bit, but IMHO it is an entity classification
> >> that a subsystem sets when creating the entity.
> >> 
> >> So v4l2 has the classifications V4L2_SUBDEV and V4L2_IO. And while all
> >> entities of the V4L2_SUBDEV classification are indeed embedded in a
> >> struct v4l2_subdev, that is not true for V4L2_IO (radio interface
> >> entities are embedded in struct video_device, but are not of the V4L2_IO
> >> class).
> >> 
> >> Other subsystems may need other classifications.
> >> 
> >> So what about this:
> >> 
> >> enum media_entity_class {
> >> 
> >> 	MEDIA_ENTITY_CLASS_UNDEFINED, // Actually, CLASS_NONE would work here
> >> 	too
> >> 	MEDIA_ENTITY_CLASS_V4L2_IO,
> >> 	MEDIA_ENTITY_CLASS_V4L2_SUBDEV,
> >> 
> >> };
> > 
> > The purpose of the type is solely to identify the type of the media_entity
> > instance to safely cast it to the proper object type (in an OOP sense).
> > That's what I want the name of the field to describe. It's not about a
> > classification, it's about object instance type identification.
> 
> No, it's not. The way it is used is to find entities that are embedded in a
> video_device AND that do I/O.

Used by who ? There are currently 5 drivers using the 
is_media_entity_v4l2_io() function (exynos4-is, omap3isp, vsp1, omap4iss, 
davinci_vpfe). They all use two entity types only (video_device and 
v4l2_subdev), and use is_media_entity_v4l2_io() to check whether the entity is 
implemented by a video_device (and in the case of the vsp1 driver to check 
whether the entity is not a subdev, I should probably use 
!is_media_entity_v4l2_subdev() instead). Those are the current use cases, and 
they make sense to me. They might not align with the function name though, 
which is why I propose renaming it in the v2 I've sent yesterday.

Furthermore, the type of the entity embedded in struct video_device is 
unconditionally set to MEDIA_ENTITY_TYPE_V4L2_VIDEO_DEVICE in the video device 
registration function. In practice is_media_entity_v4l2_io() will return true 
for all video_device instances, regardless of whether they can do I/O.

I agree that knowing whether a particular video_device instance can perform 
I/O could be useful to drivers, but I believe that information shouldn't be 
carried by the media_entity type field but discovered through a method 
specific to struct video_device instances. We have no driver requiring that 
information today, so there's no urgency.

> While all I/O V4L2 entities are a video_device, the reverse is not true.
> Most obviously radio devices, but video devices can also be used for control
> only and not for I/O. For example (a real-life example) a sensor -> FPGA ->
> HDMI TX pipeline where video devices are used to control the sensor and HDMI
> transmitter, but no I/O to/from memory happens since that's all inside the
> FPGA.
> 
> So this really is CLASS_V4L2_IO.
> 
> And the name is_media_entity_v4l2_io is OK too.
> 
> This could be done differently, though, but it requires more work.
> 
> I do think it would be useful to know that the entity is embedded in the
> video_device, so I agree with you on that.
> 
> But to make is_media_entity_v4l2_io work you would need to know if the
> device could do I/O. One way this can be done is to make the device_caps of
> v4l2_capabilities part of struct video_device.
> 
> This would have other beneficial results as well since the core can now know
> the caps of the device and it can take decisions accordingly. I've thought
> about this before but never had enough of a reason to implement it.

Given that caps are static (at least in the use cases I can think of, and we 
could allow drivers to override the capabilities operation if really needed) 
it would indeed make sense and would allow getting rid of a bunch of trivial 
functions. At the expense of 8 more bytes per video_device instance I believe 
we could still decrease memory usage.

> So the is_media_entity_v4l2_io() function would check if the entity is of
> type VIDEO_DEVICE, then use container_of to get the caps from video_device
> and check for (CAP_STREAMING | CAP_READWRITE).
> 
> In this case I would call it:
> 
> enum media_entity_is_of_type {
> 	MEDIA_ENTITY_TYPE_UNDEFINED,
> 	MEDIA_ENTITY_TYPE_V4L2_VIDEO_DEVICE,
> 	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
> };
> 
> And the field name would be:
> 
> 	enum media_entity_is_of_type type;
> 
> But this requires more work since all drivers need to be modified, starting
> with those used by those drivers are also use the is_media_entity_*
> functions. I do think it would be beneficial to make the device_caps part
> of video_device regardless.
> 
> Regards,
> 
> 	Hans
> 
> > From that point of view, the V4L2_IO class/type is wrong. We want to tell
> > that the entity instance is a video_device instance (and given that we
> > use C, this OOP construct is implemented by embedding the struct
> > media_entity in a struct video_device). We really want VIDEO_DEVICE here,
> > there is no struct v4l2_io.> 
> >> and the field enum media_entity_class class; in struct media_entity with
> >> documentation:
> >> 
> >> @class:	Classification of the media_entity, subsystems can set this to
> >> quickly classify what sort of media_entity this is.

-- 
Regards,

Laurent Pinchart