[v5,1/2] media: Add obj_type field to struct media_entity

Commit Message

Laurent Pinchart March 23, 2016, 8:45 a.m. UTC

Code that processes media entities can require knowledge of the
structure type that embeds a particular media entity instance in order
to cast the entity to the proper object type. 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 an
obj_type field to the media entity structure to carry the information.

Signed-off-by: Laurent Pinchart <[e-mail]>
Acked-by: Hans Verkuil <[e-mail]>
Acked-by: Sakari Ailus <[e-mail]>
---
 drivers/media/media-device.c          |  2 +
 drivers/media/v4l2-core/v4l2-dev.c    |  1 +
 drivers/media/v4l2-core/v4l2-subdev.c |  1 +
 include/media/media-entity.h          | 79 +++++++++++++++++++----------------
 4 files changed, 46 insertions(+), 37 deletions(-)

Comments

Mauro Carvalho Chehab March 23, 2016, 10:35 a.m. UTC | #1

Em Wed, 23 Mar 2016 10:45:55 +0200
Laurent Pinchart <[e-mail]> escreveu:

> Code that processes media entities can require knowledge of the
> structure type that embeds a particular media entity instance in order
> to cast the entity to the proper object type. 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 an
> obj_type field to the media entity structure to carry the information.
> 
> Signed-off-by: Laurent Pinchart <[e-mail]>
> Acked-by: Hans Verkuil <[e-mail]>
> Acked-by: Sakari Ailus <[e-mail]>
> ---
>  drivers/media/media-device.c          |  2 +
>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
>  include/media/media-entity.h          | 79 +++++++++++++++++++----------------
>  4 files changed, 46 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 4a97d92a7e7d..88d8de3b7a4f 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -580,6 +580,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
>  			 "Entity type for entity %s was not initialized!\n",
>  			 entity->name);
>  
> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> +

This is not ok. There are valid cases where the entity is not embedded
on some other struct. That's the case of connectors/connections, for
example: a connector/connection entity doesn't need anything else but
struct media device.

Also, this is V4L2 specific. Neither ALSA nor DVB need to use container_of().
Actually, this won't even work there, as the entity is stored as a pointer,
and not as an embedded data.

So, if we're willing to do this, then it should, instead, create
something like:

struct embedded_media_entity {
	struct media_entity entity;
	enum media_entity_type obj_type;
};

And then replace the occurrences of embedded media_entity by
embedded_media_entity at the V4L2 subsystem only, in the places where
the struct is embeded on another one.

>  	/* Warn if we apparently re-register an entity */
>  	WARN_ON(entity->graph_obj.mdev != NULL);
>  	entity->graph_obj.mdev = mdev;
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index d8e5994cccf1..70b559d7ca80 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.obj_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..0fa60801a428 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.obj_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 6dc9e4e8cbd4..5cea57955a3a 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -188,10 +188,41 @@ struct media_entity_operations {
>  };
>  
>  /**
> + * enum media_entity_type - Media entity type
> + *
> + * @MEDIA_ENTITY_TYPE_INVALID:
> + *	Invalid type, used to catch uninitialized fields.
> + * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
> + *	The entity is embedded in a struct video_device instance.
> + * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
> + *	The entity is embedded in a struct v4l2_subdev instance.
> + *
> + * Media entity objects are not instantiated directly, 

As I said before, this is not true (nor even at V4L2 subsystem, due to
the connectors/connections).

As before, you should call this as:
	enum embedded_media_entity_type

And then change the test to:

	"Media entity objects declared via struct embedded_media_device are not
	 instantiated directly,"

and fix the text below accordingly.

> but the media entity
> + * structure is inherited by (through embedding) other subsystem-specific
> + * structures. The media entity type identifies the type of the subclass
> + * structure that implements a media entity instance.
> + *
> + * This allows runtime type identification of media entities and safe casting to
> + * the correct object type. For instance, a media entity structure instance
> + * embedded in a v4l2_subdev structure instance will have the type
> + * MEDIA_ENTITY_TYPE_V4L2_SUBDEV and can safely be cast to a v4l2_subdev
> + * structure using the container_of() macro.
> + *
> + * The MEDIA_ENTITY_TYPE_INVALID type should never be set as an entity type, it
> + * only serves to catch uninitialized fields when registering entities.
> + */
> +enum media_entity_type {
> +	MEDIA_ENTITY_TYPE_INVALID,
> +	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.
> + * @obj_type:	Type of the object that implements the media_entity.
>   * @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_*)
> @@ -220,6 +251,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 obj_type;

See above. This doesn't below to the generic media entity struct,
but to an special type that is meant to be embedded on some places.

>  	u32 function;
>  	unsigned long flags;
>  
> @@ -329,56 +361,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 is a video_device
>   * @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 is an instance of a video_device object and can
> + * safely be cast to a struct video_device using the container_of() macro, 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->obj_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 is a v4l2_subdev
>   * @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 is an instance of a v4l2_subdev object and can
> + * safely be cast to a struct v4l2_subdev using the container_of() macro, 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->obj_type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
>  }
>  
>  /**

Hans Verkuil March 23, 2016, 2:05 p.m. UTC | #2

On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
> Em Wed, 23 Mar 2016 10:45:55 +0200
> Laurent Pinchart <[e-mail]> escreveu:
> 
>> Code that processes media entities can require knowledge of the
>> structure type that embeds a particular media entity instance in order
>> to cast the entity to the proper object type. 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 an
>> obj_type field to the media entity structure to carry the information.
>>
>> Signed-off-by: Laurent Pinchart <[e-mail]>
>> Acked-by: Hans Verkuil <[e-mail]>
>> Acked-by: Sakari Ailus <[e-mail]>
>> ---
>>  drivers/media/media-device.c          |  2 +
>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
>>  include/media/media-entity.h          | 79 +++++++++++++++++++----------------
>>  4 files changed, 46 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 4a97d92a7e7d..88d8de3b7a4f 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -580,6 +580,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
>>  			 "Entity type for entity %s was not initialized!\n",
>>  			 entity->name);
>>  
>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
>> +
> 
> This is not ok. There are valid cases where the entity is not embedded
> on some other struct. That's the case of connectors/connections, for
> example: a connector/connection entity doesn't need anything else but
> struct media device.

Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR or
MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.

> Also, this is V4L2 specific. Neither ALSA nor DVB need to use container_of().
> Actually, this won't even work there, as the entity is stored as a pointer,
> and not as an embedded data.

Any other subsystem that *does* embed this can use obj_type. If it doesn't embed
it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be used (or whatever
name we give it). I agree that we need a type define for the case where it is
not embedded.

> 
> So, if we're willing to do this, then it should, instead, create
> something like:
> 
> struct embedded_media_entity {
> 	struct media_entity entity;
> 	enum media_entity_type obj_type;
> };

It's not v4l2 specific. It is just that v4l2 is the only subsystem that requires
this information at the moment. I see no reason at all to create such an ugly
struct.

I very strongly suspect that other subsystems will also embed this in their own
internal structs. I actually wonder why it isn't embedded in struct dvb_device,
but I have to admit that I didn't take a close look at that. The pads are embedded
there, so it is somewhat odd that the entity isn't.

Regards,

	Hans

> 
> And then replace the occurrences of embedded media_entity by
> embedded_media_entity at the V4L2 subsystem only, in the places where
> the struct is embeded on another one.
> 
>>  	/* Warn if we apparently re-register an entity */
>>  	WARN_ON(entity->graph_obj.mdev != NULL);
>>  	entity->graph_obj.mdev = mdev;
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index d8e5994cccf1..70b559d7ca80 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.obj_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..0fa60801a428 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.obj_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 6dc9e4e8cbd4..5cea57955a3a 100644
>> --- a/include/media/media-entity.h
>> +++ b/include/media/media-entity.h
>> @@ -188,10 +188,41 @@ struct media_entity_operations {
>>  };
>>  
>>  /**
>> + * enum media_entity_type - Media entity type
>> + *
>> + * @MEDIA_ENTITY_TYPE_INVALID:
>> + *	Invalid type, used to catch uninitialized fields.
>> + * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
>> + *	The entity is embedded in a struct video_device instance.
>> + * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
>> + *	The entity is embedded in a struct v4l2_subdev instance.
>> + *
>> + * Media entity objects are not instantiated directly, 
> 
> As I said before, this is not true (nor even at V4L2 subsystem, due to
> the connectors/connections).
> 
> As before, you should call this as:
> 	enum embedded_media_entity_type
> 
> And then change the test to:
> 
> 	"Media entity objects declared via struct embedded_media_device are not
> 	 instantiated directly,"
> 
> and fix the text below accordingly.
> 
>> but the media entity
>> + * structure is inherited by (through embedding) other subsystem-specific
>> + * structures. The media entity type identifies the type of the subclass
>> + * structure that implements a media entity instance.
>> + *
>> + * This allows runtime type identification of media entities and safe casting to
>> + * the correct object type. For instance, a media entity structure instance
>> + * embedded in a v4l2_subdev structure instance will have the type
>> + * MEDIA_ENTITY_TYPE_V4L2_SUBDEV and can safely be cast to a v4l2_subdev
>> + * structure using the container_of() macro.
>> + *
>> + * The MEDIA_ENTITY_TYPE_INVALID type should never be set as an entity type, it
>> + * only serves to catch uninitialized fields when registering entities.
>> + */
>> +enum media_entity_type {
>> +	MEDIA_ENTITY_TYPE_INVALID,
>> +	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.
>> + * @obj_type:	Type of the object that implements the media_entity.
>>   * @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_*)
>> @@ -220,6 +251,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 obj_type;
> 
> See above. This doesn't below to the generic media entity struct,
> but to an special type that is meant to be embedded on some places.
> 
>>  	u32 function;
>>  	unsigned long flags;
>>  
>> @@ -329,56 +361,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 is a video_device
>>   * @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 is an instance of a video_device object and can
>> + * safely be cast to a struct video_device using the container_of() macro, 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->obj_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 is a v4l2_subdev
>>   * @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 is an instance of a v4l2_subdev object and can
>> + * safely be cast to a struct v4l2_subdev using the container_of() macro, 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->obj_type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
>>  }
>>  
>>  /**
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart March 23, 2016, 2:45 p.m. UTC | #3

Hi Hans,

On Wednesday 23 Mar 2016 15:05:41 Hans Verkuil wrote:
> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
> > Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an
> >> obj_type field to the media entity structure to carry the information.
> >> 
> >> Signed-off-by: Laurent Pinchart
> >> <[e-mail]>
> >> Acked-by: Hans Verkuil <[e-mail]>
> >> Acked-by: Sakari Ailus <[e-mail]>
> >> ---
> >> 
> >>  drivers/media/media-device.c          |  2 +
> >>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>  include/media/media-entity.h          | 79 ++++++++++++++++-------------
> >>  4 files changed, 46 insertions(+), 37 deletions(-)
> >> 
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 4a97d92a7e7d..88d8de3b7a4f 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -580,6 +580,8 @@ int __must_check media_device_register_entity(struct
> >> media_device *mdev,
> >>  			 "Entity type for entity %s was not initialized!\n",
> >>  			 entity->name);
> >> 
> >> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> >> +
> > 
> > This is not ok. There are valid cases where the entity is not embedded
> > on some other struct. That's the case of connectors/connections, for
> > example: a connector/connection entity doesn't need anything else but
> > struct media device.
> 
> Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR
> or MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.

MEDIA_ENTITY_TYPE_CONNECTOR would make sense, but only if we add a struct 
media_connector. I believe that can be a good idea, at least to simplify 
management of the entity and the connector pad(s).

> > Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> > container_of(). Actually, this won't even work there, as the entity is
> > stored as a pointer, and not as an embedded data.

That's sounds like a strange design decision at the very least. There can be 
valid cases that require creation of bare entities, but I don't think they 
should be that common.

> Any other subsystem that *does* embed this can use obj_type. If it doesn't
> embed it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be used (or
> whatever name we give it). I agree that we need a type define for the case
> where it is not embedded.

I'd like to point out that I had defined MEDIA_ENTITY_TYPE_MEDIA_ENTITY for 
that purpose in v4, and was requested to drop it.

I can submit a v6 with MEDIA_ENTITY_TYPE_MEDIA_ENTITY added back. I'd like a 
confirmation that it won't be rejected straight away though.

The WARN_ON() is in my opinion useful, but I'm ready to leave it out for now 
until we fix the connectors mess if it can help getting this patch merged 
faster.

> > So, if we're willing to do this, then it should, instead, create
> > something like:
> > 
> > struct embedded_media_entity {
> > 	struct media_entity entity;
> > 	enum media_entity_type obj_type;
> > };
> 
> It's not v4l2 specific. It is just that v4l2 is the only subsystem that
> requires this information at the moment. I see no reason at all to create
> such an ugly struct.

I totally agree.

> I very strongly suspect that other subsystems will also embed this in their
> own internal structs. I actually wonder why it isn't embedded in struct
> dvb_device, but I have to admit that I didn't take a close look at that.
> The pads are embedded there, so it is somewhat odd that the entity isn't.
> 
> > And then replace the occurrences of embedded media_entity by
> > embedded_media_entity at the V4L2 subsystem only, in the places where
> > the struct is embeded on another one.
> > 
> >>  	/* Warn if we apparently re-register an entity */
> >>  	WARN_ON(entity->graph_obj.mdev != NULL);
> >>  	entity->graph_obj.mdev = mdev;
> >> 
> >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> >> b/drivers/media/v4l2-core/v4l2-dev.c index d8e5994cccf1..70b559d7ca80
> >> 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.obj_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..0fa60801a428
> >> 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.obj_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 6dc9e4e8cbd4..5cea57955a3a 100644
> >> --- a/include/media/media-entity.h
> >> +++ b/include/media/media-entity.h
> >> @@ -188,10 +188,41 @@ struct media_entity_operations {
> >> 
> >>  };
> >>  
> >>  /**
> >> 
> >> + * enum media_entity_type - Media entity type
> >> + *
> >> + * @MEDIA_ENTITY_TYPE_INVALID:
> >> + *	Invalid type, used to catch uninitialized fields.
> >> + * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
> >> + *	The entity is embedded in a struct video_device instance.
> >> + * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
> >> + *	The entity is embedded in a struct v4l2_subdev instance.
> >> + *
> >> + * Media entity objects are not instantiated directly,
> > 
> > As I said before, this is not true (nor even at V4L2 subsystem, due to
> > the connectors/connections).
> > 
> > As before, you should call this as:
> > 	enum embedded_media_entity_type
> > 
> > And then change the test to:
> > 	"Media entity objects declared via struct embedded_media_device are not
> > 	
> > 	 instantiated directly,"
> > 
> > and fix the text below accordingly.
> > 
> >> but the media entity
> >> + * structure is inherited by (through embedding) other
> >> subsystem-specific
> >> + * structures. The media entity type identifies the type of the subclass
> >> + * structure that implements a media entity instance.
> >> + *
> >> + * This allows runtime type identification of media entities and safe
> >> casting to + * the correct object type. For instance, a media entity
> >> structure instance + * embedded in a v4l2_subdev structure instance will
> >> have the type + * MEDIA_ENTITY_TYPE_V4L2_SUBDEV and can safely be cast
> >> to a v4l2_subdev + * structure using the container_of() macro.
> >> + *
> >> + * The MEDIA_ENTITY_TYPE_INVALID type should never be set as an entity
> >> type, it + * only serves to catch uninitialized fields when registering
> >> entities. + */
> >> +enum media_entity_type {
> >> +	MEDIA_ENTITY_TYPE_INVALID,
> >> +	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.
> >> 
> >> + * @obj_type:	Type of the object that implements the media_entity.
> >> 
> >>   * @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_*)
> >> 
> >> @@ -220,6 +251,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 obj_type;
> > 
> > See above. This doesn't below to the generic media entity struct,
> > but to an special type that is meant to be embedded on some places.
> > 
> >>  	u32 function;
> >>  	unsigned long flags;
> >> 
> >> @@ -329,56 +361,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 is a video_device
> >> 
> >>   * @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 is an instance of a video_device object
> >> and can + * safely be cast to a struct video_device using the
> >> container_of() macro, 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->obj_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 is a v4l2_subdev
> >> 
> >>   * @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 is an instance of a
> >> v4l2_subdev object and can + * safely be cast to a struct v4l2_subdev
> >> using the container_of() macro, 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->obj_type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
> >> 
> >>  }
> >>  
> >>  /**

Hans Verkuil March 23, 2016, 2:57 p.m. UTC | #4

On 03/23/2016 03:45 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wednesday 23 Mar 2016 15:05:41 Hans Verkuil wrote:
>> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
>>> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an
>>>> obj_type field to the media entity structure to carry the information.
>>>>
>>>> Signed-off-by: Laurent Pinchart
>>>> <[e-mail]>
>>>> Acked-by: Hans Verkuil <[e-mail]>
>>>> Acked-by: Sakari Ailus <[e-mail]>
>>>> ---
>>>>
>>>>  drivers/media/media-device.c          |  2 +
>>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
>>>>  include/media/media-entity.h          | 79 ++++++++++++++++-------------
>>>>  4 files changed, 46 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>> index 4a97d92a7e7d..88d8de3b7a4f 100644
>>>> --- a/drivers/media/media-device.c
>>>> +++ b/drivers/media/media-device.c
>>>> @@ -580,6 +580,8 @@ int __must_check media_device_register_entity(struct
>>>> media_device *mdev,
>>>>  			 "Entity type for entity %s was not initialized!\n",
>>>>  			 entity->name);
>>>>
>>>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
>>>> +
>>>
>>> This is not ok. There are valid cases where the entity is not embedded
>>> on some other struct. That's the case of connectors/connections, for
>>> example: a connector/connection entity doesn't need anything else but
>>> struct media device.
>>
>> Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR
>> or MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.
> 
> MEDIA_ENTITY_TYPE_CONNECTOR would make sense, but only if we add a struct 
> media_connector. I believe that can be a good idea, at least to simplify 
> management of the entity and the connector pad(s).
> 
>>> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
>>> container_of(). Actually, this won't even work there, as the entity is
>>> stored as a pointer, and not as an embedded data.
> 
> That's sounds like a strange design decision at the very least. There can be 
> valid cases that require creation of bare entities, but I don't think they 
> should be that common.
> 
>> Any other subsystem that *does* embed this can use obj_type. If it doesn't
>> embed it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be used (or
>> whatever name we give it). I agree that we need a type define for the case
>> where it is not embedded.
> 
> I'd like to point out that I had defined MEDIA_ENTITY_TYPE_MEDIA_ENTITY for 
> that purpose in v4, and was requested to drop it.

I think MEDIA_ENTITY_TYPE_MEDIA_ENTITY is very ugly. Hm, some alternatives:

MEDIA_ENTITY_TYPE_NONE
MEDIA_ENTITY_TYPE_ME
MEDIA_ENTITY_TYPE_SINGLETON
MEDIA_ENTITY_TYPE_BASE

I personally prefer the last since it is just the media_entity base class by
itself.

Regards,

	Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab March 23, 2016, 3 p.m. UTC | #5

Em Wed, 23 Mar 2016 15:05:41 +0100
Hans Verkuil <[e-mail]> escreveu:

> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
> > Em Wed, 23 Mar 2016 10:45:55 +0200
> > Laurent Pinchart <[e-mail]> escreveu:
> >   
> >> Code that processes media entities can require knowledge of the
> >> structure type that embeds a particular media entity instance in order
> >> to cast the entity to the proper object type. 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 an
> >> obj_type field to the media entity structure to carry the information.
> >>
> >> Signed-off-by: Laurent Pinchart <[e-mail]>
> >> Acked-by: Hans Verkuil <[e-mail]>
> >> Acked-by: Sakari Ailus <[e-mail]>
> >> ---
> >>  drivers/media/media-device.c          |  2 +
> >>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>  include/media/media-entity.h          | 79 +++++++++++++++++++----------------
> >>  4 files changed, 46 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 4a97d92a7e7d..88d8de3b7a4f 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -580,6 +580,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> >>  			 "Entity type for entity %s was not initialized!\n",
> >>  			 entity->name);
> >>  
> >> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> >> +  
> > 
> > This is not ok. There are valid cases where the entity is not embedded
> > on some other struct. That's the case of connectors/connections, for
> > example: a connector/connection entity doesn't need anything else but
> > struct media device.  
> 
> Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR or
> MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.
> 
> > Also, this is V4L2 specific. Neither ALSA nor DVB need to use container_of().
> > Actually, this won't even work there, as the entity is stored as a pointer,
> > and not as an embedded data.  
> 
> Any other subsystem that *does* embed this can use obj_type. If it doesn't embed
> it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be used (or whatever
> name we give it). I agree that we need a type define for the case where it is
> not embedded.
> 
> > 
> > So, if we're willing to do this, then it should, instead, create
> > something like:
> > 
> > struct embedded_media_entity {
> > 	struct media_entity entity;
> > 	enum media_entity_type obj_type;
> > };  
> 
> It's not v4l2 specific. It is just that v4l2 is the only subsystem that requires
> this information at the moment. I see no reason at all to create such an ugly
> struct.

At the minute we added a BUG_ON() there, it became mandatory that all
struct media_entity to be embedded. This is not always true, but
as the intention is to avoid the risk of embedding it without a type,
it makes sense to have the above struct. This way, the obj_type
usage will be enforced *only* in the places where it is needed.

We could, instead, remove BUG_ON() and make MEDIA_ENTITY_TYPE_STANDALONE
the default type, but that won't enforce its usage where it is needed.

> I very strongly suspect that other subsystems will also embed this in their own
> internal structs. 

They will if they need.

> I actually wonder why it isn't embedded in struct dvb_device,
> but I have to admit that I didn't take a close look at that. The pads are embedded
> there, so it is somewhat odd that the entity isn't.

The only advantage of embedding instead of using a pointer is that
it would allow to use container_of() to get the struct. On the
other hand, there's one drawback: both container and embedded
structs will be destroyed at the same time. This can be a problem
if the embedded object needs to live longer than the container.

Also, the usage of container_of() doesn't work fine if the
container have embedded two objects of the same type.

In the specific case of DVB, let's imagine we would use the above
solution and add a MEDIA_ENTITY_TYPE_DVB_DEVICE.

If you look into struct dvb_device, you'll see that there are
actually two media_entities on it:

struct dvb_device {
...
        struct media_entity *entity, *tsout_entity;
...
};

If we had embedded them, just knowing that the container is
struct dvb_device won't help, as the offsets for "entity"
and for "tsout_entity" to get its container would be different.

OK, we could have added two types there, but all of these
would be just adding uneeded complexity and wound't be error
prone. Also, there's no need to use container_of(), as a pointer
to the dvb_device struct is always there at the DVB code.

The same happens at ALSA code: so far, there's no need to go from a
media_entity to its container.

So, as I said before, the usage of container_of() and the need for an
object type is currently V4L2 specific, and it is due to the way
the v4l2 core and subdev framework was modeled. Don't expect or
force that all subsystems would do the same.

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

Laurent Pinchart March 23, 2016, 3:04 p.m. UTC | #6

Hi Hans,

On Wednesday 23 Mar 2016 15:57:10 Hans Verkuil wrote:
> On 03/23/2016 03:45 PM, Laurent Pinchart wrote:
> > On Wednesday 23 Mar 2016 15:05:41 Hans Verkuil wrote:
> >> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
> >>> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an
> >>>> obj_type field to the media entity structure to carry the information.
> >>>> 
> >>>> Signed-off-by: Laurent Pinchart
> >>>> <[e-mail]>
> >>>> Acked-by: Hans Verkuil <[e-mail]>
> >>>> Acked-by: Sakari Ailus <[e-mail]>
> >>>> ---
> >>>> 
> >>>>  drivers/media/media-device.c          |  2 +
> >>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>>>  include/media/media-entity.h          | 79 +++++++++++++++------------
> >>>>  4 files changed, 46 insertions(+), 37 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/media/media-device.c
> >>>> b/drivers/media/media-device.c
> >>>> index 4a97d92a7e7d..88d8de3b7a4f 100644
> >>>> --- a/drivers/media/media-device.c
> >>>> +++ b/drivers/media/media-device.c
> >>>> @@ -580,6 +580,8 @@ int __must_check
> >>>> media_device_register_entity(struct
> >>>> media_device *mdev,
> >>>>  			 "Entity type for entity %s was not initialized!\n",
> >>>>  			 entity->name);
> >>>> 
> >>>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> >>>> +
> >>> 
> >>> This is not ok. There are valid cases where the entity is not embedded
> >>> on some other struct. That's the case of connectors/connections, for
> >>> example: a connector/connection entity doesn't need anything else but
> >>> struct media device.
> >> 
> >> Once connectors are enabled, then we do need a
> >> MEDIA_ENTITY_TYPE_CONNECTOR or MEDIA_ENTITY_TYPE_STANDALONE or something
> >> along those lines.
> > 
> > MEDIA_ENTITY_TYPE_CONNECTOR would make sense, but only if we add a struct
> > media_connector. I believe that can be a good idea, at least to simplify
> > management of the entity and the connector pad(s).
> > 
> >>> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> >>> container_of(). Actually, this won't even work there, as the entity is
> >>> stored as a pointer, and not as an embedded data.
> > 
> > That's sounds like a strange design decision at the very least. There can
> > be valid cases that require creation of bare entities, but I don't think
> > they should be that common.
> > 
> >> Any other subsystem that *does* embed this can use obj_type. If it
> >> doesn't embed it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be
> >> used (or whatever name we give it). I agree that we need a type define
> >> for the case where it is not embedded.
> > 
> > I'd like to point out that I had defined MEDIA_ENTITY_TYPE_MEDIA_ENTITY
> > for that purpose in v4, and was requested to drop it.
> 
> I think MEDIA_ENTITY_TYPE_MEDIA_ENTITY is very ugly. Hm, some alternatives:
> 
> MEDIA_ENTITY_TYPE_NONE
> MEDIA_ENTITY_TYPE_ME
> MEDIA_ENTITY_TYPE_SINGLETON
> MEDIA_ENTITY_TYPE_BASE
> 
> I personally prefer the last since it is just the media_entity base class by
> itself.

That's good because I don't like any of the first three :-) I'm OK with BASE.

Laurent Pinchart March 23, 2016, 3:11 p.m. UTC | #7

Hi Mauro,

On Wednesday 23 Mar 2016 12:00:59 Mauro Carvalho Chehab wrote:
> Em Wed, 23 Mar 2016 15:05:41 +0100 Hans Verkuil escreveu:
> > On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
> >> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an
> >>> obj_type field to the media entity structure to carry the information.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <[e-mail]>
> >>> Acked-by: Hans Verkuil <[e-mail]>
> >>> Acked-by: Sakari Ailus <[e-mail]>
> >>> ---
> >>> 
> >>>  drivers/media/media-device.c          |  2 +
> >>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>>  include/media/media-entity.h          | 79 ++++++++++++++-------------
> >>>  4 files changed, 46 insertions(+), 37 deletions(-)
> >>> 
> >>> diff --git a/drivers/media/media-device.c
> >>> b/drivers/media/media-device.c
> >>> index 4a97d92a7e7d..88d8de3b7a4f 100644
> >>> --- a/drivers/media/media-device.c
> >>> +++ b/drivers/media/media-device.c
> >>> @@ -580,6 +580,8 @@ int __must_check
> >>> media_device_register_entity(struct media_device *mdev,> >> 
> >>>  			 "Entity type for entity %s was not initialized!\n",
> >>>  			 entity->name);
> >>> 
> >>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> >>> +
> >> 
> >> This is not ok. There are valid cases where the entity is not embedded
> >> on some other struct. That's the case of connectors/connections, for
> >> example: a connector/connection entity doesn't need anything else but
> >> struct media device.
> > 
> > Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR
> > or MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.
> > 
> >> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> >> container_of(). Actually, this won't even work there, as the entity is
> >> stored as a pointer, and not as an embedded data.
> > 
> > Any other subsystem that *does* embed this can use obj_type. If it doesn't
> > embed it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be used
> > (or whatever name we give it). I agree that we need a type define for the
> > case where it is not embedded.
> > 
> >> So, if we're willing to do this, then it should, instead, create
> >> something like:
> >> 
> >> struct embedded_media_entity {
> >> 
> >> 	struct media_entity entity;
> >> 	enum media_entity_type obj_type;
> >> 
> >> };
> > 
> > It's not v4l2 specific. It is just that v4l2 is the only subsystem that
> > requires this information at the moment. I see no reason at all to create
> > such an ugly struct.
> 
> At the minute we added a BUG_ON() there,

Note that it's a WARN_ON(), not a BUG_ON().

> it became mandatory that all struct media_entity to be embedded.

No, it becomes mandatory to initialize the field.

> This is not always true, but as the intention is to avoid the risk of
> embedding it without a type, it makes sense to have the above struct. This
> way, the obj_type usage will be enforced *only* in the places where it is
> needed.
> 
> We could, instead, remove BUG_ON() and make MEDIA_ENTITY_TYPE_STANDALONE
> the default type, but that won't enforce its usage where it is needed.
> 
> > I very strongly suspect that other subsystems will also embed this in
> > their own internal structs.
> 
> They will if they need.
> 
> > I actually wonder why it isn't embedded in struct dvb_device,
> > but I have to admit that I didn't take a close look at that. The pads are
> > embedded there, so it is somewhat odd that the entity isn't.
> 
> The only advantage of embedding instead of using a pointer is that
> it would allow to use container_of() to get the struct. On the
> other hand, there's one drawback: both container and embedded
> structs will be destroyed at the same time. This can be a problem
> if the embedded object needs to live longer than the container.
> 
> Also, the usage of container_of() doesn't work fine if the
> container have embedded two objects of the same type.
> 
> In the specific case of DVB, let's imagine we would use the above
> solution and add a MEDIA_ENTITY_TYPE_DVB_DEVICE.
> 
> If you look into struct dvb_device, you'll see that there are
> actually two media_entities on it:
> 
> struct dvb_device {
> ...
>         struct media_entity *entity, *tsout_entity;
> ...
> };
> 
> If we had embedded them, just knowing that the container is
> struct dvb_device won't help, as the offsets for "entity"
> and for "tsout_entity" to get its container would be different.

That's not the issue. The two entities above do not represent the DVB device, 
struct dvb_device should certainly not inherit from media_entity. Those two 
entities should be embedded in the DVB structures that model the objects they 
represented.

> OK, we could have added two types there, but all of these
> would be just adding uneeded complexity and wound't be error
> prone. Also, there's no need to use container_of(), as a pointer
> to the dvb_device struct is always there at the DVB code.
> 
> The same happens at ALSA code: so far, there's no need to go from a
> media_entity to its container.
> 
> So, as I said before, the usage of container_of() and the need for an
> object type is currently V4L2 specific, and it is due to the way
> the v4l2 core and subdev framework was modeled. Don't expect or
> force that all subsystems would do the same.

It's not a V4L2-specific concept, it's an OOP concept. The fact that the very 
few users of media entities outside of V4L2 don't currently embed struct 
media_entity doesn't change anything here.

Mauro Carvalho Chehab March 23, 2016, 3:17 p.m. UTC | #8

Em Wed, 23 Mar 2016 15:57:10 +0100
Hans Verkuil <[e-mail]> escreveu:

> On 03/23/2016 03:45 PM, Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > On Wednesday 23 Mar 2016 15:05:41 Hans Verkuil wrote:  
> >> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:  
> >>> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an
> >>>> obj_type field to the media entity structure to carry the information.
> >>>>
> >>>> Signed-off-by: Laurent Pinchart
> >>>> <[e-mail]>
> >>>> Acked-by: Hans Verkuil <[e-mail]>
> >>>> Acked-by: Sakari Ailus <[e-mail]>
> >>>> ---
> >>>>
> >>>>  drivers/media/media-device.c          |  2 +
> >>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>>>  include/media/media-entity.h          | 79 ++++++++++++++++-------------
> >>>>  4 files changed, 46 insertions(+), 37 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >>>> index 4a97d92a7e7d..88d8de3b7a4f 100644
> >>>> --- a/drivers/media/media-device.c
> >>>> +++ b/drivers/media/media-device.c
> >>>> @@ -580,6 +580,8 @@ int __must_check media_device_register_entity(struct
> >>>> media_device *mdev,
> >>>>  			 "Entity type for entity %s was not initialized!\n",
> >>>>  			 entity->name);
> >>>>
> >>>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> >>>> +  
> >>>
> >>> This is not ok. There are valid cases where the entity is not embedded
> >>> on some other struct. That's the case of connectors/connections, for
> >>> example: a connector/connection entity doesn't need anything else but
> >>> struct media device.  
> >>
> >> Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR
> >> or MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.  
> > 
> > MEDIA_ENTITY_TYPE_CONNECTOR would make sense, but only if we add a struct 
> > media_connector. I believe that can be a good idea, at least to simplify 
> > management of the entity and the connector pad(s).
> >   
> >>> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> >>> container_of(). Actually, this won't even work there, as the entity is
> >>> stored as a pointer, and not as an embedded data.  
> > 
> > That's sounds like a strange design decision at the very least. There can be 
> > valid cases that require creation of bare entities, but I don't think they 
> > should be that common.

This is where we disagree.

Basically the problem we have is that we have something like:

struct container {
	struct object obj;
};

or

struct container {
	struct object *obj;
};


The normal usage is the way both DVB and ALSA currently does: they
always go from the container to the obj:

	obj = container.obj;
or
	obj = container->obj;

Anyway, either embeeding or usin a pointer, for such usage, there's no
need for an "obj_type".

At some V4L2 drivers, however, it is needed to do something like:

if (obj_type == MEDIA_TYPE_FOO)
	container_foo = container_of(obj, struct container_foo, obj);

if (obj_type == MEDIA_TYPE_BAR)
	container_bar = container_of(obj, struct container_bar, obj);

Ok, certainly there are cases where this could be unavoidable, but it is
*ugly*.

The way DVB uses it is a way cleaner, as never needs to use
container_of(), as the container struct is always known. Also, there's
no need to embed the struct.

As not all DVB drivers support the media controller, using pointers
make the data footprint smaller.

Also, as I answered on my previous e-mail, struct dvb_device needs
two media_entity structs on it.

So, there's no good reason why not using pointers there.

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

Hans Verkuil March 23, 2016, 3:20 p.m. UTC | #9

On 03/23/2016 04:11 PM, Laurent Pinchart wrote:
> Hi Mauro,
> 
> On Wednesday 23 Mar 2016 12:00:59 Mauro Carvalho Chehab wrote:
>> Em Wed, 23 Mar 2016 15:05:41 +0100 Hans Verkuil escreveu:
>>> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
>>>> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an
>>>>> obj_type field to the media entity structure to carry the information.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart
>>>>> <[e-mail]>
>>>>> Acked-by: Hans Verkuil <[e-mail]>
>>>>> Acked-by: Sakari Ailus <[e-mail]>
>>>>> ---
>>>>>
>>>>>  drivers/media/media-device.c          |  2 +
>>>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
>>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
>>>>>  include/media/media-entity.h          | 79 ++++++++++++++-------------
>>>>>  4 files changed, 46 insertions(+), 37 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/media-device.c
>>>>> b/drivers/media/media-device.c
>>>>> index 4a97d92a7e7d..88d8de3b7a4f 100644
>>>>> --- a/drivers/media/media-device.c
>>>>> +++ b/drivers/media/media-device.c
>>>>> @@ -580,6 +580,8 @@ int __must_check
>>>>> media_device_register_entity(struct media_device *mdev,> >> 
>>>>>  			 "Entity type for entity %s was not initialized!\n",
>>>>>  			 entity->name);
>>>>>
>>>>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
>>>>> +
>>>>
>>>> This is not ok. There are valid cases where the entity is not embedded
>>>> on some other struct. That's the case of connectors/connections, for
>>>> example: a connector/connection entity doesn't need anything else but
>>>> struct media device.
>>>
>>> Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR
>>> or MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.
>>>
>>>> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
>>>> container_of(). Actually, this won't even work there, as the entity is
>>>> stored as a pointer, and not as an embedded data.
>>>
>>> Any other subsystem that *does* embed this can use obj_type. If it doesn't
>>> embed it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be used
>>> (or whatever name we give it). I agree that we need a type define for the
>>> case where it is not embedded.
>>>
>>>> So, if we're willing to do this, then it should, instead, create
>>>> something like:
>>>>
>>>> struct embedded_media_entity {
>>>>
>>>> 	struct media_entity entity;
>>>> 	enum media_entity_type obj_type;
>>>>
>>>> };
>>>
>>> It's not v4l2 specific. It is just that v4l2 is the only subsystem that
>>> requires this information at the moment. I see no reason at all to create
>>> such an ugly struct.
>>
>> At the minute we added a BUG_ON() there,
> 
> Note that it's a WARN_ON(), not a BUG_ON().
> 
>> it became mandatory that all struct media_entity to be embedded.
> 
> No, it becomes mandatory to initialize the field.

I think the _INVALID type should just be dropped. _BASE would be the default.

If the entity is embedded in a larger struct, then whoever creates that struct
will set the type correctly. That's not done in drivers anyway but in subsystem
core code, so I don't think you need an _INVALID type at all.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab March 23, 2016, 3:24 p.m. UTC | #10

Em Wed, 23 Mar 2016 17:11:30 +0200
Laurent Pinchart <[e-mail]> escreveu:

> Hi Mauro,
> 
> On Wednesday 23 Mar 2016 12:00:59 Mauro Carvalho Chehab wrote:
> > Em Wed, 23 Mar 2016 15:05:41 +0100 Hans Verkuil escreveu:  
> > > On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:  
> > >> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an
> > >>> obj_type field to the media entity structure to carry the information.
> > >>> 
> > >>> Signed-off-by: Laurent Pinchart
> > >>> <[e-mail]>
> > >>> Acked-by: Hans Verkuil <[e-mail]>
> > >>> Acked-by: Sakari Ailus <[e-mail]>
> > >>> ---
> > >>> 
> > >>>  drivers/media/media-device.c          |  2 +
> > >>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> > >>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> > >>>  include/media/media-entity.h          | 79 ++++++++++++++-------------
> > >>>  4 files changed, 46 insertions(+), 37 deletions(-)
> > >>> 
> > >>> diff --git a/drivers/media/media-device.c
> > >>> b/drivers/media/media-device.c
> > >>> index 4a97d92a7e7d..88d8de3b7a4f 100644
> > >>> --- a/drivers/media/media-device.c
> > >>> +++ b/drivers/media/media-device.c
> > >>> @@ -580,6 +580,8 @@ int __must_check
> > >>> media_device_register_entity(struct media_device *mdev,> >> 
> > >>>  			 "Entity type for entity %s was not initialized!\n",
> > >>>  			 entity->name);
> > >>> 
> > >>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> > >>> +  
> > >> 
> > >> This is not ok. There are valid cases where the entity is not embedded
> > >> on some other struct. That's the case of connectors/connections, for
> > >> example: a connector/connection entity doesn't need anything else but
> > >> struct media device.  
> > > 
> > > Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR
> > > or MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.
> > >   
> > >> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> > >> container_of(). Actually, this won't even work there, as the entity is
> > >> stored as a pointer, and not as an embedded data.  
> > > 
> > > Any other subsystem that *does* embed this can use obj_type. If it doesn't
> > > embed it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be used
> > > (or whatever name we give it). I agree that we need a type define for the
> > > case where it is not embedded.
> > >   
> > >> So, if we're willing to do this, then it should, instead, create
> > >> something like:
> > >> 
> > >> struct embedded_media_entity {
> > >> 
> > >> 	struct media_entity entity;
> > >> 	enum media_entity_type obj_type;
> > >> 
> > >> };  
> > > 
> > > It's not v4l2 specific. It is just that v4l2 is the only subsystem that
> > > requires this information at the moment. I see no reason at all to create
> > > such an ugly struct.  
> > 
> > At the minute we added a BUG_ON() there,  
> 
> Note that it's a WARN_ON(), not a BUG_ON().

WARN_ON() should warn about a trouble. This is not the case here.
It is only a problem for a few drivers that need to use container_of()
to get the container struct..

> 
> > it became mandatory that all struct media_entity to be embedded.  
> 
> No, it becomes mandatory to initialize the field.

The current patch makes it mandatory, causing lots of bogus WARN_ON().

> 
> > This is not always true, but as the intention is to avoid the risk of
> > embedding it without a type, it makes sense to have the above struct. This
> > way, the obj_type usage will be enforced *only* in the places where it is
> > needed.
> > 
> > We could, instead, remove BUG_ON() and make MEDIA_ENTITY_TYPE_STANDALONE
> > the default type, but that won't enforce its usage where it is needed.
> >   
> > > I very strongly suspect that other subsystems will also embed this in
> > > their own internal structs.  
> > 
> > They will if they need.
> >   
> > > I actually wonder why it isn't embedded in struct dvb_device,
> > > but I have to admit that I didn't take a close look at that. The pads are
> > > embedded there, so it is somewhat odd that the entity isn't.  
> > 
> > The only advantage of embedding instead of using a pointer is that
> > it would allow to use container_of() to get the struct. On the
> > other hand, there's one drawback: both container and embedded
> > structs will be destroyed at the same time. This can be a problem
> > if the embedded object needs to live longer than the container.
> > 
> > Also, the usage of container_of() doesn't work fine if the
> > container have embedded two objects of the same type.
> > 
> > In the specific case of DVB, let's imagine we would use the above
> > solution and add a MEDIA_ENTITY_TYPE_DVB_DEVICE.
> > 
> > If you look into struct dvb_device, you'll see that there are
> > actually two media_entities on it:
> > 
> > struct dvb_device {
> > ...
> >         struct media_entity *entity, *tsout_entity;
> > ...
> > };
> > 
> > If we had embedded them, just knowing that the container is
> > struct dvb_device won't help, as the offsets for "entity"
> > and for "tsout_entity" to get its container would be different.  
> 
> That's not the issue. The two entities above do not represent the DVB device, 
> struct dvb_device should certainly not inherit from media_entity. Those two 
> entities should be embedded in the DVB structures that model the objects they 
> represented.

On what structs they should be embedded? There's no subdev concept
at the DVB subsystem, and it doesn't make any sense to add it just
to make you happy.

> 
> > OK, we could have added two types there, but all of these
> > would be just adding uneeded complexity and wound't be error
> > prone. Also, there's no need to use container_of(), as a pointer
> > to the dvb_device struct is always there at the DVB code.
> > 
> > The same happens at ALSA code: so far, there's no need to go from a
> > media_entity to its container.
> > 
> > So, as I said before, the usage of container_of() and the need for an
> > object type is currently V4L2 specific, and it is due to the way
> > the v4l2 core and subdev framework was modeled. Don't expect or
> > force that all subsystems would do the same.  
> 
> It's not a V4L2-specific concept, it's an OOP concept. The fact that the very 
> few users of media entities outside of V4L2 don't currently embed struct 
> media_entity doesn't change anything here.

If there was a consensus at the Kernel that this is something that
should be used everywhere, it should have something at the core to
handle it.

Also, the kernel was not written in c++. OOP usage should not be
enforced everywhere. It should be used only when there are good
reasons for doing it.

Laurent Pinchart March 23, 2016, 3:41 p.m. UTC | #11

On Wednesday 23 Mar 2016 12:17:30 Mauro Carvalho Chehab wrote:
> Em Wed, 23 Mar 2016 15:57:10 +0100 Hans Verkuil escreveu:
> > On 03/23/2016 03:45 PM, Laurent Pinchart wrote:
> >> On Wednesday 23 Mar 2016 15:05:41 Hans Verkuil wrote:
> >>> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
> >>>> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an
> >>>>> obj_type field to the media entity structure to carry the
> >>>>> information.
> >>>>> 
> >>>>> Signed-off-by: Laurent Pinchart
> >>>>> <[e-mail]>
> >>>>> Acked-by: Hans Verkuil <[e-mail]>
> >>>>> Acked-by: Sakari Ailus <[e-mail]>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/media/media-device.c          |  2 +
> >>>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>>>>  include/media/media-entity.h          | 79 ++++++++++++-------------
> >>>>>  4 files changed, 46 insertions(+), 37 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/media/media-device.c
> >>>>> b/drivers/media/media-device.c
> >>>>> index 4a97d92a7e7d..88d8de3b7a4f 100644
> >>>>> --- a/drivers/media/media-device.c
> >>>>> +++ b/drivers/media/media-device.c
> >>>>> @@ -580,6 +580,8 @@ int __must_check
> >>>>> media_device_register_entity(struct
> >>>>> media_device *mdev,
> >>>>> 
> >>>>>  			 "Entity type for entity %s was not initialized!\n",
> >>>>>  			 entity->name);
> >>>>> 
> >>>>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> >>>>> +
> >>>> 
> >>>> This is not ok. There are valid cases where the entity is not embedded
> >>>> on some other struct. That's the case of connectors/connections, for
> >>>> example: a connector/connection entity doesn't need anything else but
> >>>> struct media device.
> >>> 
> >>> Once connectors are enabled, then we do need a
> >>> MEDIA_ENTITY_TYPE_CONNECTOR
> >>> or MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.
> >> 
> >> MEDIA_ENTITY_TYPE_CONNECTOR would make sense, but only if we add a
> >> struct media_connector. I believe that can be a good idea, at least to
> >> simplify management of the entity and the connector pad(s).
> >> 
> >>>> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> >>>> container_of(). Actually, this won't even work there, as the entity is
> >>>> stored as a pointer, and not as an embedded data.
> >> 
> >> That's sounds like a strange design decision at the very least. There
> >> can be valid cases that require creation of bare entities, but I don't
> >> think they should be that common.
> 
> This is where we disagree.
> 
> Basically the problem we have is that we have something like:
> 
> struct container {
> 	struct object obj;
> };
> 
> or
> 
> struct container {
> 	struct object *obj;
> };
> 
> 
> The normal usage is the way both DVB and ALSA currently does: they
> always go from the container to the obj:
> 
> 	obj = container.obj;
> or
> 	obj = container->obj;
> 
> Anyway, either embeeding or usin a pointer, for such usage, there's no
> need for an "obj_type".
> 
> At some V4L2 drivers, however, it is needed to do something like:
> 
> if (obj_type == MEDIA_TYPE_FOO)
> 	container_foo = container_of(obj, struct container_foo, obj);
> 
> if (obj_type == MEDIA_TYPE_BAR)
> 	container_bar = container_of(obj, struct container_bar, obj);
> 
> Ok, certainly there are cases where this could be unavoidable, but it is
> *ugly*.
> 
> The way DVB uses it is a way cleaner, as never needs to use
> container_of(), as the container struct is always known. Also, there's
> no need to embed the struct.

No, no, no and no. Looks like it's time for a bit of Object Oriented 
Programming 101.

Casting from a superclass (a.k.a. base class) type to a subclass type is a 
basic programming concept found in most languages that deal with objects. It 
allows creating collections of objects of different subclasses than all 
inherit from the same base class, handle them with generic code and still 
offer the ability for custom processing when needed.

C++ implements this concept with the dynamic_cast<> operator. As the kernel is 
written in plain C we use container_of() instead for the same purpose, and 
need explicit object types to perform RTTI.

> As not all DVB drivers support the media controller, using pointers
> make the data footprint smaller.
> 
> Also, as I answered on my previous e-mail, struct dvb_device needs
> two media_entity structs on it.
> 
> So, there's no good reason why not using pointers there.

Sakari Ailus March 23, 2016, 4:17 p.m. UTC | #12

Hi Mauro,

On Wed, Mar 23, 2016 at 12:00:59PM -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 23 Mar 2016 15:05:41 +0100
> Hans Verkuil <[e-mail]> escreveu:
> 
> > On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
> > > Em Wed, 23 Mar 2016 10:45:55 +0200
> > > Laurent Pinchart <[e-mail]> escreveu:
> > >   
> > >> Code that processes media entities can require knowledge of the
> > >> structure type that embeds a particular media entity instance in order
> > >> to cast the entity to the proper object type. 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 an
> > >> obj_type field to the media entity structure to carry the information.
> > >>
> > >> Signed-off-by: Laurent Pinchart <[e-mail]>
> > >> Acked-by: Hans Verkuil <[e-mail]>
> > >> Acked-by: Sakari Ailus <[e-mail]>
> > >> ---
> > >>  drivers/media/media-device.c          |  2 +
> > >>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> > >>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> > >>  include/media/media-entity.h          | 79 +++++++++++++++++++----------------
> > >>  4 files changed, 46 insertions(+), 37 deletions(-)
> > >>
> > >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > >> index 4a97d92a7e7d..88d8de3b7a4f 100644
> > >> --- a/drivers/media/media-device.c
> > >> +++ b/drivers/media/media-device.c
> > >> @@ -580,6 +580,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> > >>  			 "Entity type for entity %s was not initialized!\n",
> > >>  			 entity->name);
> > >>  
> > >> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> > >> +  
> > > 
> > > This is not ok. There are valid cases where the entity is not embedded
> > > on some other struct. That's the case of connectors/connections, for
> > > example: a connector/connection entity doesn't need anything else but
> > > struct media device.  
> > 
> > Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR or
> > MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.
> > 
> > > Also, this is V4L2 specific. Neither ALSA nor DVB need to use container_of().
> > > Actually, this won't even work there, as the entity is stored as a pointer,
> > > and not as an embedded data.  
> > 
> > Any other subsystem that *does* embed this can use obj_type. If it doesn't embed
> > it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be used (or whatever
> > name we give it). I agree that we need a type define for the case where it is
> > not embedded.
> > 
> > > 
> > > So, if we're willing to do this, then it should, instead, create
> > > something like:
> > > 
> > > struct embedded_media_entity {
> > > 	struct media_entity entity;
> > > 	enum media_entity_type obj_type;
> > > };  
> > 
> > It's not v4l2 specific. It is just that v4l2 is the only subsystem that requires
> > this information at the moment. I see no reason at all to create such an ugly
> > struct.
> 
> At the minute we added a BUG_ON() there, it became mandatory that all
> struct media_entity to be embedded. This is not always true, but
> as the intention is to avoid the risk of embedding it without a type,
> it makes sense to have the above struct. This way, the obj_type
> usage will be enforced *only* in the places where it is needed.
> 
> We could, instead, remove BUG_ON() and make MEDIA_ENTITY_TYPE_STANDALONE
> the default type, but that won't enforce its usage where it is needed.

My understanding, based on the previous discussion, was that the
media_entity would in practice be always embedded in another struct. If
that's not the case, I think Hans proposed a few good options for type enums
telling the media_entity is not embedded in another struct.

For what it's worth, my personal favourite is "NONE".

> 
> > I very strongly suspect that other subsystems will also embed this in their own
> > internal structs. 
> 
> They will if they need.
> 
> > I actually wonder why it isn't embedded in struct dvb_device,
> > but I have to admit that I didn't take a close look at that. The pads are embedded
> > there, so it is somewhat odd that the entity isn't.
> 
> The only advantage of embedding instead of using a pointer is that
> it would allow to use container_of() to get the struct. On the
> other hand, there's one drawback: both container and embedded
> structs will be destroyed at the same time. This can be a problem
> if the embedded object needs to live longer than the container.
> 
> Also, the usage of container_of() doesn't work fine if the
> container have embedded two objects of the same type.
> 
> In the specific case of DVB, let's imagine we would use the above
> solution and add a MEDIA_ENTITY_TYPE_DVB_DEVICE.
> 
> If you look into struct dvb_device, you'll see that there are
> actually two media_entities on it:
> 
> struct dvb_device {
> ...
>         struct media_entity *entity, *tsout_entity;
> ...
> };
> 
> If we had embedded them, just knowing that the container is
> struct dvb_device won't help, as the offsets for "entity"
> and for "tsout_entity" to get its container would be different.
> 
> OK, we could have added two types there, but all of these
> would be just adding uneeded complexity and wound't be error
> prone. Also, there's no need to use container_of(), as a pointer
> to the dvb_device struct is always there at the DVB code.

If you wanted to obtain the dvb_device here, you'd instead add another
struct that embeds the struct media_entity. This struct would contain a
pointer to struct dvb_device. Simple and efficient.

> 
> The same happens at ALSA code: so far, there's no need to go from a
> media_entity to its container.
> 
> So, as I said before, the usage of container_of() and the need for an
> object type is currently V4L2 specific, and it is due to the way
> the v4l2 core and subdev framework was modeled. Don't expect or
> force that all subsystems would do the same.

What you do miss with using a pointer above is to find sub-system specific
information related to the entity. It's a very common operation, if there is
sub-system specific information related to an entity. I would say that if
you don't, the driver creating the media device must be aware of a lot of
the properties of the devices the media graph consists of, or use 
alternative means to associate subsystem specific information to entities.
Looping over a linked list, for instance, is bound to be less efficient.

I don't see anything V4L2 specific here as such, even though no other
sub-system would (yet) need the feature.

Laurent Pinchart March 23, 2016, 4:30 p.m. UTC | #13

On Wednesday 23 Mar 2016 12:24:38 Mauro Carvalho Chehab wrote:
> Em Wed, 23 Mar 2016 17:11:30 +0200 Laurent Pinchart escreveu:
> > On Wednesday 23 Mar 2016 12:00:59 Mauro Carvalho Chehab wrote:
> > > Em Wed, 23 Mar 2016 15:05:41 +0100 Hans Verkuil escreveu:
> > >> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
> > >>> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an obj_type field to the media entity structure to carry
> > >>>> the information.
> > >>>> 
> > >>>> Signed-off-by: Laurent Pinchart
> > >>>> <[e-mail]>
> > >>>> Acked-by: Hans Verkuil <[e-mail]>
> > >>>> Acked-by: Sakari Ailus <[e-mail]>
> > >>>> ---
> > >>>> 
> > >>>> drivers/media/media-device.c          |  2 +
> > >>>> drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> > >>>> drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> > >>>> include/media/media-entity.h          | 79  ++++++++++++-----------
> > >>>> 4 files changed, 46 insertions(+), 37 deletions(-)
> > >>>> 
> > >>>> diff --git a/drivers/media/media-device.c
> > >>>> b/drivers/media/media-device.c
> > >>>> index 4a97d92a7e7d..88d8de3b7a4f 100644
> > >>>> --- a/drivers/media/media-device.c
> > >>>> +++ b/drivers/media/media-device.c
> > >>>> @@ -580,6 +580,8 @@ int __must_check
> > >>>> media_device_register_entity(struct media_device *mdev,> >>
> > >>>> 
> > >>>>  			 "Entity type for entity %s was not initialized!\n",
> > >>>>  			 entity->name);
> > >>>> 
> > >>>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> > >>>> +
> > >>> 
> > >>> This is not ok. There are valid cases where the entity is not
> > >>> embedded on some other struct. That's the case of
> > >>> connectors/connections, for example: a connector/connection entity
> > >>> doesn't need anything else but struct media device.
> > >> 
> > >> Once connectors are enabled, then we do need a
> > >> MEDIA_ENTITY_TYPE_CONNECTOR or MEDIA_ENTITY_TYPE_STANDALONE or
> > >> something along those lines.
> > >> 
> > >>> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> > >>> container_of(). Actually, this won't even work there, as the entity
> > >>> is stored as a pointer, and not as an embedded data.
> > >> 
> > >> Any other subsystem that *does* embed this can use obj_type. If it
> > >> doesn't embed it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should
> > >> be used (or whatever name we give it). I agree that we need a type
> > >> define for the case where it is not embedded.
> > >> 
> > >>> So, if we're willing to do this, then it should, instead, create
> > >>> something like:
> > >>> 
> > >>> struct embedded_media_entity {
> > >>> 
> > >>> 	struct media_entity entity;
> > >>> 	enum media_entity_type obj_type;
> > >>> 
> > >>> };
> > >> 
> > >> It's not v4l2 specific. It is just that v4l2 is the only subsystem
> > >> that requires this information at the moment. I see no reason at all to
> > >> create such an ugly struct.
> > > 
> > > At the minute we added a BUG_ON() there,
> > 
> > Note that it's a WARN_ON(), not a BUG_ON().
> 
> WARN_ON() should warn about a trouble. This is not the case here.
> It is only a problem for a few drivers that need to use container_of()
> to get the container struct..

No, the purpose of this WARN_ON() is to catch unitialized obj_type fields. The 
field must be initialized, otherwise code dealing with entities will fail. 
Now, if we consider that the MEDIA_ENTITY_TYPE_BASE (to use Hans' proposed 
name) case is the most common case, and if we would like to avoid forcing all 
drivers that create entities directly to initialize their type explicitly, we 
could make that type be equal to 0. I would however prevent us from catching 
missing initializations, but I could live with that.

Again, the purpose of the WARN_ON() isn't to enforce a programming model, but 
to catch missing initialization of an important field.

> >> it became mandatory that all struct media_entity to be embedded.
> > 
> > No, it becomes mandatory to initialize the field.
> 
> The current patch makes it mandatory, causing lots of bogus WARN_ON().
> 
> >> This is not always true, but as the intention is to avoid the risk of
> >> embedding it without a type, it makes sense to have the above struct.
> >> This way, the obj_type usage will be enforced *only* in the places where
> >> it is needed.
> >> 
> >> We could, instead, remove BUG_ON() and make MEDIA_ENTITY_TYPE_STANDALONE
> >> the default type, but that won't enforce its usage where it is needed.
> >> 
> >>> I very strongly suspect that other subsystems will also embed this in
> >>> their own internal structs.
> >> 
> >> They will if they need.
> >> 
> >>> I actually wonder why it isn't embedded in struct dvb_device,
> >>> but I have to admit that I didn't take a close look at that. The pads
> >>> are embedded there, so it is somewhat odd that the entity isn't.
> >> 
> >> The only advantage of embedding instead of using a pointer is that
> >> it would allow to use container_of() to get the struct. On the
> >> other hand, there's one drawback: both container and embedded
> >> structs will be destroyed at the same time. This can be a problem
> >> if the embedded object needs to live longer than the container.
> >> 
> >> Also, the usage of container_of() doesn't work fine if the
> >> container have embedded two objects of the same type.
> >> 
> >> In the specific case of DVB, let's imagine we would use the above
> >> solution and add a MEDIA_ENTITY_TYPE_DVB_DEVICE.
> >> 
> >> If you look into struct dvb_device, you'll see that there are
> >> actually two media_entities on it:
> >> 
> >> struct dvb_device {
> >> ...
> >>         struct media_entity *entity, *tsout_entity;
> >> ...
> >> };
> >> 
> >> If we had embedded them, just knowing that the container is
> >> struct dvb_device won't help, as the offsets for "entity"
> >> and for "tsout_entity" to get its container would be different.
> > 
> > That's not the issue. The two entities above do not represent the DVB
> > device, struct dvb_device should certainly not inherit from media_entity.
> > Those two entities should be embedded in the DVB structures that model
> > the objects they represented.
> 
> On what structs they should be embedded? There's no subdev concept
> at the DVB subsystem, and it doesn't make any sense to add it just
> to make you happy.

I'm surprised that there's no DVB kernel objects that model what the entities 
represent, but if that's indeed the case, there's no need to embed them in 
anything. They certainly shouldn't be embedded in struct dvb_device, those two 
entities do *not* represent the DVB decide.

> >> OK, we could have added two types there, but all of these
> >> would be just adding uneeded complexity and wound't be error
> >> prone. Also, there's no need to use container_of(), as a pointer
> >> to the dvb_device struct is always there at the DVB code.
> >> 
> >> The same happens at ALSA code: so far, there's no need to go from a
> >> media_entity to its container.
> >> 
> >> So, as I said before, the usage of container_of() and the need for an
> >> object type is currently V4L2 specific, and it is due to the way
> >> the v4l2 core and subdev framework was modeled. Don't expect or
> >> force that all subsystems would do the same.
> > 
> > It's not a V4L2-specific concept, it's an OOP concept. The fact that the
> > very few users of media entities outside of V4L2 don't currently embed
> > struct media_entity doesn't change anything here.
> 
> If there was a consensus at the Kernel that this is something that
> should be used everywhere, it should have something at the core to
> handle it.

I don't think you can do that in the core in plain C, although it would be 
useful.

> Also, the kernel was not written in c++. OOP usage should not be
> enforced everywhere. It should be used only when there are good
> reasons for doing it.

Those are two orthogonal issues. We're using OOP throughout the whole kernel 
in C. It requires slightly more verbose coding, but hasn't been an issue so 
far. OOP doesn't mean and isn't limited to C++.

Mauro Carvalho Chehab March 23, 2016, 4:47 p.m. UTC | #14

Em Wed, 23 Mar 2016 18:17:38 +0200
Sakari Ailus <[e-mail]> escreveu:

> Hi Mauro,
> 
> On Wed, Mar 23, 2016 at 12:00:59PM -0300, Mauro Carvalho Chehab wrote:
> > Em Wed, 23 Mar 2016 15:05:41 +0100
> > Hans Verkuil <[e-mail]> escreveu:
> >   
> > > On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:  
> > > > Em Wed, 23 Mar 2016 10:45:55 +0200
> > > > Laurent Pinchart <[e-mail]> escreveu:
> > > >     
> > > >> Code that processes media entities can require knowledge of the
> > > >> structure type that embeds a particular media entity instance in order
> > > >> to cast the entity to the proper object type. 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 an
> > > >> obj_type field to the media entity structure to carry the information.
> > > >>
> > > >> Signed-off-by: Laurent Pinchart <[e-mail]>
> > > >> Acked-by: Hans Verkuil <[e-mail]>
> > > >> Acked-by: Sakari Ailus <[e-mail]>
> > > >> ---
> > > >>  drivers/media/media-device.c          |  2 +
> > > >>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> > > >>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> > > >>  include/media/media-entity.h          | 79 +++++++++++++++++++----------------
> > > >>  4 files changed, 46 insertions(+), 37 deletions(-)
> > > >>
> > > >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > > >> index 4a97d92a7e7d..88d8de3b7a4f 100644
> > > >> --- a/drivers/media/media-device.c
> > > >> +++ b/drivers/media/media-device.c
> > > >> @@ -580,6 +580,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> > > >>  			 "Entity type for entity %s was not initialized!\n",
> > > >>  			 entity->name);
> > > >>  
> > > >> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> > > >> +    
> > > > 
> > > > This is not ok. There are valid cases where the entity is not embedded
> > > > on some other struct. That's the case of connectors/connections, for
> > > > example: a connector/connection entity doesn't need anything else but
> > > > struct media device.    
> > > 
> > > Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR or
> > > MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.
> > >   
> > > > Also, this is V4L2 specific. Neither ALSA nor DVB need to use container_of().
> > > > Actually, this won't even work there, as the entity is stored as a pointer,
> > > > and not as an embedded data.    
> > > 
> > > Any other subsystem that *does* embed this can use obj_type. If it doesn't embed
> > > it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be used (or whatever
> > > name we give it). I agree that we need a type define for the case where it is
> > > not embedded.
> > >   
> > > > 
> > > > So, if we're willing to do this, then it should, instead, create
> > > > something like:
> > > > 
> > > > struct embedded_media_entity {
> > > > 	struct media_entity entity;
> > > > 	enum media_entity_type obj_type;
> > > > };    
> > > 
> > > It's not v4l2 specific. It is just that v4l2 is the only subsystem that requires
> > > this information at the moment. I see no reason at all to create such an ugly
> > > struct.  
> > 
> > At the minute we added a BUG_ON() there, it became mandatory that all
> > struct media_entity to be embedded. This is not always true, but
> > as the intention is to avoid the risk of embedding it without a type,
> > it makes sense to have the above struct. This way, the obj_type
> > usage will be enforced *only* in the places where it is needed.
> > 
> > We could, instead, remove BUG_ON() and make MEDIA_ENTITY_TYPE_STANDALONE
> > the default type, but that won't enforce its usage where it is needed.  
> 
> My understanding, based on the previous discussion, was that the
> media_entity would in practice be always embedded in another struct. If
> that's not the case, I think Hans proposed a few good options for type enums
> telling the media_entity is not embedded in another struct.
> 
> For what it's worth, my personal favourite is "NONE".

A "standalone" type would work. "none" doesn't seem a good name
though, but I don't intend to spend much time with naming issues.

> 
> >   
> > > I very strongly suspect that other subsystems will also embed this in their own
> > > internal structs.   
> > 
> > They will if they need.
> >   
> > > I actually wonder why it isn't embedded in struct dvb_device,
> > > but I have to admit that I didn't take a close look at that. The pads are embedded
> > > there, so it is somewhat odd that the entity isn't.  
> > 
> > The only advantage of embedding instead of using a pointer is that
> > it would allow to use container_of() to get the struct. On the
> > other hand, there's one drawback: both container and embedded
> > structs will be destroyed at the same time. This can be a problem
> > if the embedded object needs to live longer than the container.
> > 
> > Also, the usage of container_of() doesn't work fine if the
> > container have embedded two objects of the same type.
> > 
> > In the specific case of DVB, let's imagine we would use the above
> > solution and add a MEDIA_ENTITY_TYPE_DVB_DEVICE.
> > 
> > If you look into struct dvb_device, you'll see that there are
> > actually two media_entities on it:
> > 
> > struct dvb_device {
> > ...
> >         struct media_entity *entity, *tsout_entity;
> > ...
> > };
> > 
> > If we had embedded them, just knowing that the container is
> > struct dvb_device won't help, as the offsets for "entity"
> > and for "tsout_entity" to get its container would be different.
> > 
> > OK, we could have added two types there, but all of these
> > would be just adding uneeded complexity and wound't be error
> > prone. Also, there's no need to use container_of(), as a pointer
> > to the dvb_device struct is always there at the DVB code.  
> 
> If you wanted to obtain the dvb_device here, you'd instead add another
> struct that embeds the struct media_entity. This struct would contain a
> pointer to struct dvb_device. Simple and efficient.

Why would someone need that? There's no need to embedding it at 
the DVB core. Adding an extra struct just to make the media core
happy would just add extra complexity for no good reason.

> 
> > 
> > The same happens at ALSA code: so far, there's no need to go from a
> > media_entity to its container.
> > 
> > So, as I said before, the usage of container_of() and the need for an
> > object type is currently V4L2 specific, and it is due to the way
> > the v4l2 core and subdev framework was modeled. Don't expect or
> > force that all subsystems would do the same.  
> 
> What you do miss with using a pointer above is to find sub-system specific
> information related to the entity. It's a very common operation, if there is
> sub-system specific information related to an entity. I would say that if
> you don't, the driver creating the media device must be aware of a lot of
> the properties of the devices the media graph consists of, or use 
> alternative means to associate subsystem specific information to entities.
> Looping over a linked list, for instance, is bound to be less efficient.

The media device creation is handled by the core at the DVB side.
he knows already about any subsystem-specific data.

> I don't see anything V4L2 specific here as such, even though no other
> sub-system would (yet) need the feature.

Right now, this need happens in the case here the media entity is created
inside I2C devices, where the I2C driver doesn't know much about the
callers.

Such need is specific to a few subsystems: like v4l2 and hwmon.

Ok, other systems could need to do that in the future, but such
need assumes that the media_entities will be created by drivers
that are connected to the main driver via some bus, with is not
the generic case.

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

Mauro Carvalho Chehab March 23, 2016, 5:06 p.m. UTC | #15

Em Wed, 23 Mar 2016 18:30:47 +0200
Laurent Pinchart <[e-mail]> escreveu:

> On Wednesday 23 Mar 2016 12:24:38 Mauro Carvalho Chehab wrote:
> > Em Wed, 23 Mar 2016 17:11:30 +0200 Laurent Pinchart escreveu:  
> > > On Wednesday 23 Mar 2016 12:00:59 Mauro Carvalho Chehab wrote:  
> > > > Em Wed, 23 Mar 2016 15:05:41 +0100 Hans Verkuil escreveu:  
> > > >> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:  
> > > >>> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an obj_type field to the media entity structure to carry
> > > >>>> the information.
> > > >>>> 
> > > >>>> Signed-off-by: Laurent Pinchart
> > > >>>> <[e-mail]>
> > > >>>> Acked-by: Hans Verkuil <[e-mail]>
> > > >>>> Acked-by: Sakari Ailus <[e-mail]>
> > > >>>> ---
> > > >>>> 
> > > >>>> drivers/media/media-device.c          |  2 +
> > > >>>> drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> > > >>>> drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> > > >>>> include/media/media-entity.h          | 79  ++++++++++++-----------
> > > >>>> 4 files changed, 46 insertions(+), 37 deletions(-)
> > > >>>> 
> > > >>>> diff --git a/drivers/media/media-device.c
> > > >>>> b/drivers/media/media-device.c
> > > >>>> index 4a97d92a7e7d..88d8de3b7a4f 100644
> > > >>>> --- a/drivers/media/media-device.c
> > > >>>> +++ b/drivers/media/media-device.c
> > > >>>> @@ -580,6 +580,8 @@ int __must_check
> > > >>>> media_device_register_entity(struct media_device *mdev,> >>
> > > >>>> 
> > > >>>>  			 "Entity type for entity %s was not initialized!\n",
> > > >>>>  			 entity->name);
> > > >>>> 
> > > >>>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> > > >>>> +  
> > > >>> 
> > > >>> This is not ok. There are valid cases where the entity is not
> > > >>> embedded on some other struct. That's the case of
> > > >>> connectors/connections, for example: a connector/connection entity
> > > >>> doesn't need anything else but struct media device.  
> > > >> 
> > > >> Once connectors are enabled, then we do need a
> > > >> MEDIA_ENTITY_TYPE_CONNECTOR or MEDIA_ENTITY_TYPE_STANDALONE or
> > > >> something along those lines.
> > > >>   
> > > >>> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> > > >>> container_of(). Actually, this won't even work there, as the entity
> > > >>> is stored as a pointer, and not as an embedded data.  
> > > >> 
> > > >> Any other subsystem that *does* embed this can use obj_type. If it
> > > >> doesn't embed it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should
> > > >> be used (or whatever name we give it). I agree that we need a type
> > > >> define for the case where it is not embedded.
> > > >>   
> > > >>> So, if we're willing to do this, then it should, instead, create
> > > >>> something like:
> > > >>> 
> > > >>> struct embedded_media_entity {
> > > >>> 
> > > >>> 	struct media_entity entity;
> > > >>> 	enum media_entity_type obj_type;
> > > >>> 
> > > >>> };  
> > > >> 
> > > >> It's not v4l2 specific. It is just that v4l2 is the only subsystem
> > > >> that requires this information at the moment. I see no reason at all to
> > > >> create such an ugly struct.  
> > > > 
> > > > At the minute we added a BUG_ON() there,  
> > > 
> > > Note that it's a WARN_ON(), not a BUG_ON().  
> > 
> > WARN_ON() should warn about a trouble. This is not the case here.
> > It is only a problem for a few drivers that need to use container_of()
> > to get the container struct..  
> 
> No, the purpose of this WARN_ON() is to catch unitialized obj_type fields. The 
> field must be initialized, otherwise code dealing with entities will fail. 
> Now, if we consider that the MEDIA_ENTITY_TYPE_BASE (to use Hans' proposed 
> name) case is the most common case, and if we would like to avoid forcing all 
> drivers that create entities directly to initialize their type explicitly, we 
> could make that type be equal to 0. I would however prevent us from catching 
> missing initializations, but I could live with that.

Works for me, but, as I said, if you want to enforce, the best is to create
a separate struct for the cases where the entity is embed on some other
object.

> Again, the purpose of the WARN_ON() isn't to enforce a programming model, but 
> to catch missing initialization of an important field.

If this is the case, I'm OK.

> > >> it became mandatory that all struct media_entity to be embedded.  
> > > 
> > > No, it becomes mandatory to initialize the field.  
> > 
> > The current patch makes it mandatory, causing lots of bogus WARN_ON().
> >   
> > >> This is not always true, but as the intention is to avoid the risk of
> > >> embedding it without a type, it makes sense to have the above struct.
> > >> This way, the obj_type usage will be enforced *only* in the places where
> > >> it is needed.
> > >> 
> > >> We could, instead, remove BUG_ON() and make MEDIA_ENTITY_TYPE_STANDALONE
> > >> the default type, but that won't enforce its usage where it is needed.
> > >>   
> > >>> I very strongly suspect that other subsystems will also embed this in
> > >>> their own internal structs.  
> > >> 
> > >> They will if they need.
> > >>   
> > >>> I actually wonder why it isn't embedded in struct dvb_device,
> > >>> but I have to admit that I didn't take a close look at that. The pads
> > >>> are embedded there, so it is somewhat odd that the entity isn't.  
> > >> 
> > >> The only advantage of embedding instead of using a pointer is that
> > >> it would allow to use container_of() to get the struct. On the
> > >> other hand, there's one drawback: both container and embedded
> > >> structs will be destroyed at the same time. This can be a problem
> > >> if the embedded object needs to live longer than the container.
> > >> 
> > >> Also, the usage of container_of() doesn't work fine if the
> > >> container have embedded two objects of the same type.
> > >> 
> > >> In the specific case of DVB, let's imagine we would use the above
> > >> solution and add a MEDIA_ENTITY_TYPE_DVB_DEVICE.
> > >> 
> > >> If you look into struct dvb_device, you'll see that there are
> > >> actually two media_entities on it:
> > >> 
> > >> struct dvb_device {
> > >> ...
> > >>         struct media_entity *entity, *tsout_entity;
> > >> ...
> > >> };
> > >> 
> > >> If we had embedded them, just knowing that the container is
> > >> struct dvb_device won't help, as the offsets for "entity"
> > >> and for "tsout_entity" to get its container would be different.  
> > > 
> > > That's not the issue. The two entities above do not represent the DVB
> > > device, struct dvb_device should certainly not inherit from media_entity.
> > > Those two entities should be embedded in the DVB structures that model
> > > the objects they represented.  
> > 
> > On what structs they should be embedded? There's no subdev concept
> > at the DVB subsystem, and it doesn't make any sense to add it just
> > to make you happy.  
> 
> I'm surprised that there's no DVB kernel objects that model what the entities 
> represent, but if that's indeed the case, there's no need to embed them in 
> anything. They certainly shouldn't be embedded in struct dvb_device, those two 
> entities do *not* represent the DVB decide.

Well, the DVB subsystem doesn't use the I2C bus approach (except for
the tuner, when it is shared with V4L2). It uses the low-level I2C API, 
with leads to a very different way for a DVB driver to work. Basically,
all DVB subdevs directly access the DVB structs, as those are passed
via the binding functions.

Also, some entities are really a piece of a hardware with no distinct
I2C address, or even implemented in software for really cheap devices
(like the DVB soft demux).

Also, in *all* cases, the core is called to create the model of
the objects, using their own standard structs. So, it was easier
to just let the core create the entities at the register functions.

> > >> OK, we could have added two types there, but all of these
> > >> would be just adding uneeded complexity and wound't be error
> > >> prone. Also, there's no need to use container_of(), as a pointer
> > >> to the dvb_device struct is always there at the DVB code.
> > >> 
> > >> The same happens at ALSA code: so far, there's no need to go from a
> > >> media_entity to its container.
> > >> 
> > >> So, as I said before, the usage of container_of() and the need for an
> > >> object type is currently V4L2 specific, and it is due to the way
> > >> the v4l2 core and subdev framework was modeled. Don't expect or
> > >> force that all subsystems would do the same.  
> > > 
> > > It's not a V4L2-specific concept, it's an OOP concept. The fact that the
> > > very few users of media entities outside of V4L2 don't currently embed
> > > struct media_entity doesn't change anything here.  
> > 
> > If there was a consensus at the Kernel that this is something that
> > should be used everywhere, it should have something at the core to
> > handle it.  
> 
> I don't think you can do that in the core in plain C, although it would be 
> useful.

I guess it is possible, but not easy. I remember that some of the early c++
"compilers" that would actually convert the C++ code into a pure C code,
and then compile it. The end result were not nice, IMHO, as cleaning
up memory were done via some sort of garbage collector.

> > Also, the kernel was not written in c++. OOP usage should not be
> > enforced everywhere. It should be used only when there are good
> > reasons for doing it.  
> 
> Those are two orthogonal issues. We're using OOP throughout the whole kernel 
> in C. It requires slightly more verbose coding, but hasn't been an issue so 
> far. OOP doesn't mean and isn't limited to C++.

We use some OOP concepts in the Kernel, where it is helpful.

What I'm against on this patch is that, the way it is, it is forcing
all places to have a struct to embeed media_entity, even where this is
not needed/wanted.

If you remove that WARN_ON() and initialize all media_entities
as _BASE objects, that would work for me.

It would also work if you create a separate struct to enforce that the
obj type will always be filled for the cases it gets embedded.

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

Mauro Carvalho Chehab March 23, 2016, 5:29 p.m. UTC | #16

Em Wed, 23 Mar 2016 17:41:44 +0200
Laurent Pinchart <[e-mail]> escreveu:

> On Wednesday 23 Mar 2016 12:17:30 Mauro Carvalho Chehab wrote:
> > Em Wed, 23 Mar 2016 15:57:10 +0100 Hans Verkuil escreveu:  
> > > On 03/23/2016 03:45 PM, Laurent Pinchart wrote:  
> > >> On Wednesday 23 Mar 2016 15:05:41 Hans Verkuil wrote:  
> > >>> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:  
> > >>>> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an
> > >>>>> obj_type field to the media entity structure to carry the
> > >>>>> information.
> > >>>>> 
> > >>>>> Signed-off-by: Laurent Pinchart
> > >>>>> <[e-mail]>
> > >>>>> Acked-by: Hans Verkuil <[e-mail]>
> > >>>>> Acked-by: Sakari Ailus <[e-mail]>
> > >>>>> ---
> > >>>>> 
> > >>>>>  drivers/media/media-device.c          |  2 +
> > >>>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> > >>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> > >>>>>  include/media/media-entity.h          | 79 ++++++++++++-------------
> > >>>>>  4 files changed, 46 insertions(+), 37 deletions(-)
> > >>>>> 
> > >>>>> diff --git a/drivers/media/media-device.c
> > >>>>> b/drivers/media/media-device.c
> > >>>>> index 4a97d92a7e7d..88d8de3b7a4f 100644
> > >>>>> --- a/drivers/media/media-device.c
> > >>>>> +++ b/drivers/media/media-device.c
> > >>>>> @@ -580,6 +580,8 @@ int __must_check
> > >>>>> media_device_register_entity(struct
> > >>>>> media_device *mdev,
> > >>>>> 
> > >>>>>  			 "Entity type for entity %s was not initialized!\n",
> > >>>>>  			 entity->name);
> > >>>>> 
> > >>>>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> > >>>>> +  
> > >>>> 
> > >>>> This is not ok. There are valid cases where the entity is not embedded
> > >>>> on some other struct. That's the case of connectors/connections, for
> > >>>> example: a connector/connection entity doesn't need anything else but
> > >>>> struct media device.  
> > >>> 
> > >>> Once connectors are enabled, then we do need a
> > >>> MEDIA_ENTITY_TYPE_CONNECTOR
> > >>> or MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.  
> > >> 
> > >> MEDIA_ENTITY_TYPE_CONNECTOR would make sense, but only if we add a
> > >> struct media_connector. I believe that can be a good idea, at least to
> > >> simplify management of the entity and the connector pad(s).
> > >>   
> > >>>> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> > >>>> container_of(). Actually, this won't even work there, as the entity is
> > >>>> stored as a pointer, and not as an embedded data.  
> > >> 
> > >> That's sounds like a strange design decision at the very least. There
> > >> can be valid cases that require creation of bare entities, but I don't
> > >> think they should be that common.  
> > 
> > This is where we disagree.
> > 
> > Basically the problem we have is that we have something like:
> > 
> > struct container {
> > 	struct object obj;
> > };
> > 
> > or
> > 
> > struct container {
> > 	struct object *obj;
> > };
> > 
> > 
> > The normal usage is the way both DVB and ALSA currently does: they
> > always go from the container to the obj:
> > 
> > 	obj = container.obj;
> > or
> > 	obj = container->obj;
> > 
> > Anyway, either embeeding or usin a pointer, for such usage, there's no
> > need for an "obj_type".
> > 
> > At some V4L2 drivers, however, it is needed to do something like:
> > 
> > if (obj_type == MEDIA_TYPE_FOO)
> > 	container_foo = container_of(obj, struct container_foo, obj);
> > 
> > if (obj_type == MEDIA_TYPE_BAR)
> > 	container_bar = container_of(obj, struct container_bar, obj);
> > 
> > Ok, certainly there are cases where this could be unavoidable, but it is
> > *ugly*.
> > 
> > The way DVB uses it is a way cleaner, as never needs to use
> > container_of(), as the container struct is always known. Also, there's
> > no need to embed the struct.  
> 
> No, no, no and no. Looks like it's time for a bit of Object Oriented 
> Programming 101.

I know what you're doing. I had my usual workload of programs 
in c++ programming.

> Casting from a superclass (a.k.a. base class) type to a subclass type is a 
> basic programming concept found in most languages that deal with objects. It 
> allows creating collections of objects of different subclasses than all 
> inherit from the same base class, handle them with generic code and still 
> offer the ability for custom processing when needed.
> 
> C++ implements this concept with the dynamic_cast<> operator. As the kernel is 
> written in plain C we use container_of() instead for the same purpose, and 
> need explicit object types to perform RTTI.

I'm not arguing against the c++ theory, but, instead, about its
implementation.

On (almost?) all cases where we use container_of() in the Kernel, the
container type is already known. So, drivers just use container_of()
without any if/switch().

That's OK.

What's different in this usecase is that the driver that needs to
use container_of() doesn't know the type of the object that it
is needing to reference. So, it has things like[1]:

        while (pad) {
                if (!is_media_entity_v4l2_subdev(pad->entity))
                       	break;

		subdev = container_of(pad->entity, struct v4l2_subdev, entity)
                entity = container_of(subdev, struct vsp1_entity, subdev);
...
	}

[1] I changed the code snippet a little bit to show the container_of()
that would be otherwise hidden by a function call.

This is needed only because the loop doesn't know if the pad->entity
may not be contained inside struct v4l2_subdev.

While the above *works*, it is, IMHO, ugly, as, if the type is not
properly set, it will cause an horrible crash.

I bet you won't find too many places with similar tests at the Kernel.

On most places, what you'll find is just:

	function xxx(struct foo)
	{
		struct bar = container_of(obj, foo, obj);

without any ifs.

Yet, while I don't like the if's before container_of() and I would
avoid doing that on my own code, I see why you need it, and I'm ok
with that.

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

Sakari Ailus March 23, 2016, 11:42 p.m. UTC | #17

Hi Mauro,

On Wed, Mar 23, 2016 at 01:47:42PM -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 23 Mar 2016 18:17:38 +0200
> Sakari Ailus <[e-mail]> escreveu:
> 
> > Hi Mauro,
> > 
> > On Wed, Mar 23, 2016 at 12:00:59PM -0300, Mauro Carvalho Chehab wrote:
> > > Em Wed, 23 Mar 2016 15:05:41 +0100
> > > Hans Verkuil <[e-mail]> escreveu:
> > >   
> > > > On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:  
> > > > > Em Wed, 23 Mar 2016 10:45:55 +0200
> > > > > Laurent Pinchart <[e-mail]> escreveu:
> > > > >     
> > > > >> Code that processes media entities can require knowledge of the
> > > > >> structure type that embeds a particular media entity instance in order
> > > > >> to cast the entity to the proper object type. 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 an
> > > > >> obj_type field to the media entity structure to carry the information.
> > > > >>
> > > > >> Signed-off-by: Laurent Pinchart <[e-mail]>
> > > > >> Acked-by: Hans Verkuil <[e-mail]>
> > > > >> Acked-by: Sakari Ailus <[e-mail]>
> > > > >> ---
> > > > >>  drivers/media/media-device.c          |  2 +
> > > > >>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> > > > >>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> > > > >>  include/media/media-entity.h          | 79 +++++++++++++++++++----------------
> > > > >>  4 files changed, 46 insertions(+), 37 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > > > >> index 4a97d92a7e7d..88d8de3b7a4f 100644
> > > > >> --- a/drivers/media/media-device.c
> > > > >> +++ b/drivers/media/media-device.c
> > > > >> @@ -580,6 +580,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> > > > >>  			 "Entity type for entity %s was not initialized!\n",
> > > > >>  			 entity->name);
> > > > >>  
> > > > >> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> > > > >> +    
> > > > > 
> > > > > This is not ok. There are valid cases where the entity is not embedded
> > > > > on some other struct. That's the case of connectors/connections, for
> > > > > example: a connector/connection entity doesn't need anything else but
> > > > > struct media device.    
> > > > 
> > > > Once connectors are enabled, then we do need a MEDIA_ENTITY_TYPE_CONNECTOR or
> > > > MEDIA_ENTITY_TYPE_STANDALONE or something along those lines.
> > > >   
> > > > > Also, this is V4L2 specific. Neither ALSA nor DVB need to use container_of().
> > > > > Actually, this won't even work there, as the entity is stored as a pointer,
> > > > > and not as an embedded data.    
> > > > 
> > > > Any other subsystem that *does* embed this can use obj_type. If it doesn't embed
> > > > it in anything, then MEDIA_ENTITY_TYPE_STANDALONE should be used (or whatever
> > > > name we give it). I agree that we need a type define for the case where it is
> > > > not embedded.
> > > >   
> > > > > 
> > > > > So, if we're willing to do this, then it should, instead, create
> > > > > something like:
> > > > > 
> > > > > struct embedded_media_entity {
> > > > > 	struct media_entity entity;
> > > > > 	enum media_entity_type obj_type;
> > > > > };    
> > > > 
> > > > It's not v4l2 specific. It is just that v4l2 is the only subsystem that requires
> > > > this information at the moment. I see no reason at all to create such an ugly
> > > > struct.  
> > > 
> > > At the minute we added a BUG_ON() there, it became mandatory that all
> > > struct media_entity to be embedded. This is not always true, but
> > > as the intention is to avoid the risk of embedding it without a type,
> > > it makes sense to have the above struct. This way, the obj_type
> > > usage will be enforced *only* in the places where it is needed.
> > > 
> > > We could, instead, remove BUG_ON() and make MEDIA_ENTITY_TYPE_STANDALONE
> > > the default type, but that won't enforce its usage where it is needed.  
> > 
> > My understanding, based on the previous discussion, was that the
> > media_entity would in practice be always embedded in another struct. If
> > that's not the case, I think Hans proposed a few good options for type enums
> > telling the media_entity is not embedded in another struct.
> > 
> > For what it's worth, my personal favourite is "NONE".
> 
> A "standalone" type would work. "none" doesn't seem a good name
> though, but I don't intend to spend much time with naming issues.

Fine with me.

> 
> > 
> > >   
> > > > I very strongly suspect that other subsystems will also embed this in their own
> > > > internal structs.   
> > > 
> > > They will if they need.
> > >   
> > > > I actually wonder why it isn't embedded in struct dvb_device,
> > > > but I have to admit that I didn't take a close look at that. The pads are embedded
> > > > there, so it is somewhat odd that the entity isn't.  
> > > 
> > > The only advantage of embedding instead of using a pointer is that
> > > it would allow to use container_of() to get the struct. On the
> > > other hand, there's one drawback: both container and embedded
> > > structs will be destroyed at the same time. This can be a problem
> > > if the embedded object needs to live longer than the container.
> > > 
> > > Also, the usage of container_of() doesn't work fine if the
> > > container have embedded two objects of the same type.
> > > 
> > > In the specific case of DVB, let's imagine we would use the above
> > > solution and add a MEDIA_ENTITY_TYPE_DVB_DEVICE.
> > > 
> > > If you look into struct dvb_device, you'll see that there are
> > > actually two media_entities on it:
> > > 
> > > struct dvb_device {
> > > ...
> > >         struct media_entity *entity, *tsout_entity;
> > > ...
> > > };
> > > 
> > > If we had embedded them, just knowing that the container is
> > > struct dvb_device won't help, as the offsets for "entity"
> > > and for "tsout_entity" to get its container would be different.
> > > 
> > > OK, we could have added two types there, but all of these
> > > would be just adding uneeded complexity and wound't be error
> > > prone. Also, there's no need to use container_of(), as a pointer
> > > to the dvb_device struct is always there at the DVB code.  
> > 
> > If you wanted to obtain the dvb_device here, you'd instead add another
> > struct that embeds the struct media_entity. This struct would contain a
> > pointer to struct dvb_device. Simple and efficient.
> 
> Why would someone need that? There's no need to embedding it at 
> the DVB core. Adding an extra struct just to make the media core
> happy would just add extra complexity for no good reason.

If your entities are just "hanging around" and you do not need to find
device specific data structures based on them, there is indeed no reason to
do so.

> 
> > 
> > > 
> > > The same happens at ALSA code: so far, there's no need to go from a
> > > media_entity to its container.
> > > 
> > > So, as I said before, the usage of container_of() and the need for an
> > > object type is currently V4L2 specific, and it is due to the way
> > > the v4l2 core and subdev framework was modeled. Don't expect or
> > > force that all subsystems would do the same.  
> > 
> > What you do miss with using a pointer above is to find sub-system specific
> > information related to the entity. It's a very common operation, if there is
> > sub-system specific information related to an entity. I would say that if
> > you don't, the driver creating the media device must be aware of a lot of
> > the properties of the devices the media graph consists of, or use 
> > alternative means to associate subsystem specific information to entities.
> > Looping over a linked list, for instance, is bound to be less efficient.
> 
> The media device creation is handled by the core at the DVB side.
> he knows already about any subsystem-specific data.
> 
> > I don't see anything V4L2 specific here as such, even though no other
> > sub-system would (yet) need the feature.
> 
> Right now, this need happens in the case here the media entity is created
> inside I2C devices, where the I2C driver doesn't know much about the
> callers.

It's not only that, any driver or the V4L2 framework requiring functionality
which is not particular to a certain entity requires that. Link validation,
a very fundamental concept in MC, is one example. As the link_validate()
callback is only aware of the MC, the V4L2 sub-device framework does need to
dig the sub-devices there.

If a sub-system (such as V4L2) makes a difference between control nodes and
DMA engines, there's a high likelyhood this is exactly what that given
sub-system would need as well. Although that does not translate to a
conclusion that this is required by other sub-systems right now.

> Such need is specific to a few subsystems: like v4l2 and hwmon.
> 
> Ok, other systems could need to do that in the future, but such
> need assumes that the media_entities will be created by drivers
> that are connected to the main driver via some bus, with is not
> the generic case.

Laurent Pinchart March 24, 2016, 7:51 a.m. UTC | #18

On Wednesday 23 Mar 2016 13:47:42 Mauro Carvalho Chehab wrote:
> Em Wed, 23 Mar 2016 18:17:38 +0200 Sakari Ailus escreveu:
> > On Wed, Mar 23, 2016 at 12:00:59PM -0300, Mauro Carvalho Chehab wrote:
> >> Em Wed, 23 Mar 2016 15:05:41 +0100 Hans Verkuil escreveu:
> >>> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:
> >>>> Em Wed, 23 Mar 2016 10:45:55 +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 cast the entity to the proper object type. 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 an obj_type field to the media entity structure to
> >>>>> carry the information.
> >>>>> 
> >>>>> Signed-off-by: Laurent Pinchart
> >>>>> <[e-mail]>
> >>>>> Acked-by: Hans Verkuil <[e-mail]>
> >>>>> Acked-by: Sakari Ailus <[e-mail]>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/media/media-device.c          |  2 +
> >>>>>  drivers/media/v4l2-core/v4l2-dev.c    |  1 +
> >>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  1 +
> >>>>>  include/media/media-entity.h          | 79 ++++++++++++------------
> >>>>>  4 files changed, 46 insertions(+), 37 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/media/media-device.c
> >>>>> b/drivers/media/media-device.c
> >>>>> index 4a97d92a7e7d..88d8de3b7a4f 100644
> >>>>> --- a/drivers/media/media-device.c
> >>>>> +++ b/drivers/media/media-device.c
> >>>>> @@ -580,6 +580,8 @@ int __must_check
> >>>>> media_device_register_entity(struct media_device *mdev,> > > >> 
> >>>>>  			 "Entity type for entity %s was not initialized!\n",
> >>>>>  			 entity->name);
> >>>>> 
> >>>>> +	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
> >>>>> +
> >>>> 
> >>>> This is not ok. There are valid cases where the entity is not
> >>>> embedded on some other struct. That's the case of
> >>>> connectors/connections, for example: a connector/connection entity
> >>>> doesn't need anything else but struct media device.
> >>> 
> >>> Once connectors are enabled, then we do need a
> >>> MEDIA_ENTITY_TYPE_CONNECTOR or MEDIA_ENTITY_TYPE_STANDALONE or
> >>> something along those lines.
> >>> 
> >>>> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> >>>> container_of(). Actually, this won't even work there, as the entity
> >>>> is stored as a pointer, and not as an embedded data.
> >>> 
> >>> Any other subsystem that *does* embed this can use obj_type. If it
> >>> doesn't embed it in anything, then MEDIA_ENTITY_TYPE_STANDALONE
> >>> should be used (or whatever name we give it). I agree that we need a
> >>> type define for the case where it is not embedded.
> >>> 
> >>>> So, if we're willing to do this, then it should, instead, create
> >>>> something like:
> >>>> 
> >>>> struct embedded_media_entity {
> >>>> 	struct media_entity entity;
> >>>> 	enum media_entity_type obj_type;
> >>>> };
> >>> 
> >>> It's not v4l2 specific. It is just that v4l2 is the only subsystem
> >>> that requires this information at the moment. I see no reason at all
> >>> to create such an ugly struct.
> >> 
> >> At the minute we added a BUG_ON() there, it became mandatory that all
> >> struct media_entity to be embedded. This is not always true, but
> >> as the intention is to avoid the risk of embedding it without a type,
> >> it makes sense to have the above struct. This way, the obj_type
> >> usage will be enforced *only* in the places where it is needed.
> >> 
> >> We could, instead, remove BUG_ON() and make MEDIA_ENTITY_TYPE_STANDALONE
> >> the default type, but that won't enforce its usage where it is needed.
> > 
> > My understanding, based on the previous discussion, was that the
> > media_entity would in practice be always embedded in another struct. If
> > that's not the case, I think Hans proposed a few good options for type
> > enums telling the media_entity is not embedded in another struct.
> > 
> > For what it's worth, my personal favourite is "NONE".
> 
> A "standalone" type would work. "none" doesn't seem a good name
> though, but I don't intend to spend much time with naming issues.
> 
> >>> I very strongly suspect that other subsystems will also embed this in
> >>> their own internal structs.
> >> 
> >> They will if they need.
> >> 
> >>> I actually wonder why it isn't embedded in struct dvb_device,
> >>> but I have to admit that I didn't take a close look at that. The pads
> >>> are embedded there, so it is somewhat odd that the entity isn't.
> >> 
> >> The only advantage of embedding instead of using a pointer is that
> >> it would allow to use container_of() to get the struct. On the
> >> other hand, there's one drawback: both container and embedded
> >> structs will be destroyed at the same time. This can be a problem
> >> if the embedded object needs to live longer than the container.
> >> 
> >> Also, the usage of container_of() doesn't work fine if the
> >> container have embedded two objects of the same type.
> >> 
> >> In the specific case of DVB, let's imagine we would use the above
> >> solution and add a MEDIA_ENTITY_TYPE_DVB_DEVICE.
> >> 
> >> If you look into struct dvb_device, you'll see that there are
> >> actually two media_entities on it:
> >> 
> >> struct dvb_device {
> >> ...
> >>         struct media_entity *entity, *tsout_entity;
> >> ...
> >> };
> >> 
> >> If we had embedded them, just knowing that the container is
> >> struct dvb_device won't help, as the offsets for "entity"
> >> and for "tsout_entity" to get its container would be different.
> >> 
> >> OK, we could have added two types there, but all of these
> >> would be just adding uneeded complexity and wound't be error
> >> prone. Also, there's no need to use container_of(), as a pointer
> >> to the dvb_device struct is always there at the DVB code.
> > 
> > If you wanted to obtain the dvb_device here, you'd instead add another
> > struct that embeds the struct media_entity. This struct would contain a
> > pointer to struct dvb_device. Simple and efficient.
> 
> Why would someone need that? There's no need to embedding it at
> the DVB core. Adding an extra struct just to make the media core
> happy would just add extra complexity for no good reason.

Sakari's point was that *if* you need to obtain the dvb_device pointer from 
the media_entity pointer you would add another structure. He didn't say you 
have to do so if you don't have such a requirement, there's no need to change 
anything.

> >> The same happens at ALSA code: so far, there's no need to go from a
> >> media_entity to its container.
> >> 
> >> So, as I said before, the usage of container_of() and the need for an
> >> object type is currently V4L2 specific, and it is due to the way
> >> the v4l2 core and subdev framework was modeled. Don't expect or
> >> force that all subsystems would do the same.
> > 
> > What you do miss with using a pointer above is to find sub-system specific
> > information related to the entity. It's a very common operation, if there
> > is sub-system specific information related to an entity. I would say that
> > if you don't, the driver creating the media device must be aware of a lot
> > of the properties of the devices the media graph consists of, or use
> > alternative means to associate subsystem specific information to entities.
> > Looping over a linked list, for instance, is bound to be less efficient.
> 
> The media device creation is handled by the core at the DVB side.
> he knows already about any subsystem-specific data.
> 
> > I don't see anything V4L2 specific here as such, even though no other
> > sub-system would (yet) need the feature.
> 
> Right now, this need happens in the case here the media entity is created
> inside I2C devices, where the I2C driver doesn't know much about the
> callers.
> 
> Such need is specific to a few subsystems: like v4l2 and hwmon.
> 
> Ok, other systems could need to do that in the future, but such
> need assumes that the media_entities will be created by drivers
> that are connected to the main driver via some bus, with is not
> the generic case.

On the ALSA side this seems a quite common case to me, as codecs are usually 
external chips.

Laurent Pinchart March 24, 2016, 8:15 a.m. UTC | #19

On Wednesday 23 Mar 2016 14:29:35 Mauro Carvalho Chehab wrote:
> Em Wed, 23 Mar 2016 17:41:44 +0200 Laurent Pinchart escreveu:
> > On Wednesday 23 Mar 2016 12:17:30 Mauro Carvalho Chehab wrote:
> >> Em Wed, 23 Mar 2016 15:57:10 +0100 Hans Verkuil escreveu:
> >>> On 03/23/2016 03:45 PM, Laurent Pinchart wrote:
> >>>> On Wednesday 23 Mar 2016 15:05:41 Hans Verkuil wrote:
> >>>>> On 03/23/2016 11:35 AM, Mauro Carvalho Chehab wrote:

[snip]

> >>>>>> Also, this is V4L2 specific. Neither ALSA nor DVB need to use
> >>>>>> container_of(). Actually, this won't even work there, as the entity
> >>>>>> is stored as a pointer, and not as an embedded data.
> >>>> 
> >>>> That's sounds like a strange design decision at the very least. There
> >>>> can be valid cases that require creation of bare entities, but I
> >>>> don't think they should be that common.
> >> 
> >> This is where we disagree.
> >> 
> >> Basically the problem we have is that we have something like:
> >> 
> >> struct container {
> >> 	struct object obj;
> >> };
> >> 
> >> or
> >> 
> >> struct container {
> >> 	struct object *obj;
> >> };
> >> 
> >> 
> >> The normal usage is the way both DVB and ALSA currently does: they
> >> 
> >> always go from the container to the obj:
> >> 	obj = container.obj;
> >> 
> >> or
> >> 
> >> 	obj = container->obj;
> >> 
> >> Anyway, either embeeding or usin a pointer, for such usage, there's no
> >> need for an "obj_type".
> >> 
> >> At some V4L2 drivers, however, it is needed to do something like:
> >> 
> >> if (obj_type == MEDIA_TYPE_FOO)
> >> 	container_foo = container_of(obj, struct container_foo, obj);
> >> 
> >> if (obj_type == MEDIA_TYPE_BAR)
> >> 	container_bar = container_of(obj, struct container_bar, obj);
> >> 
> >> Ok, certainly there are cases where this could be unavoidable, but it is
> >> *ugly*.
> >> 
> >> The way DVB uses it is a way cleaner, as never needs to use
> >> container_of(), as the container struct is always known. Also, there's
> >> no need to embed the struct.
> > 
> > No, no, no and no. Looks like it's time for a bit of Object Oriented
> > Programming 101.
> 
> I know what you're doing. I had my usual workload of programs
> in c++ programming.
> 
> > Casting from a superclass (a.k.a. base class) type to a subclass type is a
> > basic programming concept found in most languages that deal with objects.
> > It allows creating collections of objects of different subclasses than
> > all inherit from the same base class, handle them with generic code and
> > still offer the ability for custom processing when needed.
> > 
> > C++ implements this concept with the dynamic_cast<> operator. As the
> > kernel is written in plain C we use container_of() instead for the same
> > purpose, and need explicit object types to perform RTTI.
> 
> I'm not arguing against the c++ theory, but, instead, about its
> implementation.
> 
> On (almost?) all cases where we use container_of() in the Kernel, the
> container type is already known. So, drivers just use container_of()
> without any if/switch().
> 
> That's OK.
> 
> What's different in this usecase is that the driver that needs to
> use container_of() doesn't know the type of the object that it
> is needing to reference. So, it has things like[1]:
> 
> 	while (pad) {
> 		if (!is_media_entity_v4l2_subdev(pad->entity))
> 			break;
> 
> 		subdev = container_of(pad->entity, struct v4l2_subdev, entity)
> 		entity = container_of(subdev, struct vsp1_entity, subdev);
> ...
> 	}
> 
> [1] I changed the code snippet a little bit to show the container_of()
> that would be otherwise hidden by a function call.
> 
> This is needed only because the loop doesn't know if the pad->entity
> may not be contained inside struct v4l2_subdev.
> 
> While the above *works*, it is, IMHO, ugly, as, if the type is not
> properly set, it will cause an horrible crash.
> 
> I bet you won't find too many places with similar tests at the Kernel.

I believe that the construct will be quite common at least in embedded device 
drivers, and more generically in drivers that use the subdev userspace API, as 
they have a need to go through a pipeline (and thus iterating over 
media_entity instances) and process the entities depending on their type. One 
particular example is pipeline validation code where formats on connected pads 
must be matched, and how to retrieve that format differs between subdevs and 
video nodes.

> On most places, what you'll find is just:
> 
> 	function xxx(struct foo)
> 	{
> 		struct bar = container_of(obj, foo, obj);
> 
> without any ifs.
> 
> Yet, while I don't like the if's before container_of() and I would
> avoid doing that on my own code, I see why you need it, and I'm ok
> with that.

Patch

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 4a97d92a7e7d..88d8de3b7a4f 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -580,6 +580,8 @@  int __must_check media_device_register_entity(struct media_device *mdev,
 			 "Entity type for entity %s was not initialized!\n",
 			 entity->name);
 
+	WARN_ON(entity->obj_type == MEDIA_ENTITY_TYPE_INVALID);
+
 	/* Warn if we apparently re-register an entity */
 	WARN_ON(entity->graph_obj.mdev != NULL);
 	entity->graph_obj.mdev = mdev;
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index d8e5994cccf1..70b559d7ca80 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.obj_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..0fa60801a428 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.obj_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 6dc9e4e8cbd4..5cea57955a3a 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -188,10 +188,41 @@  struct media_entity_operations {
 };
 
 /**
+ * enum media_entity_type - Media entity type
+ *
+ * @MEDIA_ENTITY_TYPE_INVALID:
+ *	Invalid type, used to catch uninitialized fields.
+ * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
+ *	The entity is embedded in a struct video_device instance.
+ * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
+ *	The entity is embedded in a struct v4l2_subdev instance.
+ *
+ * Media entity objects are not instantiated directly, but the media entity
+ * structure is inherited by (through embedding) other subsystem-specific
+ * structures. The media entity type identifies the type of the subclass
+ * structure that implements a media entity instance.
+ *
+ * This allows runtime type identification of media entities and safe casting to
+ * the correct object type. For instance, a media entity structure instance
+ * embedded in a v4l2_subdev structure instance will have the type
+ * MEDIA_ENTITY_TYPE_V4L2_SUBDEV and can safely be cast to a v4l2_subdev
+ * structure using the container_of() macro.
+ *
+ * The MEDIA_ENTITY_TYPE_INVALID type should never be set as an entity type, it
+ * only serves to catch uninitialized fields when registering entities.
+ */
+enum media_entity_type {
+	MEDIA_ENTITY_TYPE_INVALID,
+	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.
+ * @obj_type:	Type of the object that implements the media_entity.
  * @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_*)
@@ -220,6 +251,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 obj_type;
 	u32 function;
 	unsigned long flags;
 
@@ -329,56 +361,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 is a video_device
  * @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 is an instance of a video_device object and can
+ * safely be cast to a struct video_device using the container_of() macro, 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->obj_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 is a v4l2_subdev
  * @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 is an instance of a v4l2_subdev object and can
+ * safely be cast to a struct v4l2_subdev using the container_of() macro, 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->obj_type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
 }
 
 /**