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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

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

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

[Laurent Pinchart]

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
> > 
> > Acked-by: Hans Verkuil 
> > Acked-by: Sakari Ailus 
> > ---
> > 
> >  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.
> >> 
> >> 

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

[Sakari Ailus]

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  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  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 
> > > > >> 
> > > > >> Acked-by: Hans Verkuil 
> > > > >> Acked-by: Sakari Ailus 
> > > > >> ---
> > > > >>  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 

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

[Mauro Carvalho Chehab]

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:  
> >  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
> > > 
> > > Acked-by: Hans Verkuil 
> > > Acked-by: Sakari Ailus 
> > > ---
> > > 
> > >  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 

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

[Mauro Carvalho Chehab]

Em Wed, 23 Mar 2016 18:30:47 +0200
Laurent Pinchart  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
> > >  
> > >  Acked-by: Hans Verkuil 
> > >  Acked-by: Sakari Ailus 
> > >  ---
> > >  
> > >  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 

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

[Mauro Carvalho Chehab]

Em Wed, 23 Mar 2016 18:17:38 +0200
Sakari Ailus  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  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 
> > > >> 
> > > >> Acked-by: Hans Verkuil 
> > > >> Acked-by: Sakari Ailus 
> > > >> ---
> > > >>  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 

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

[Laurent Pinchart]

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
> >  
> >  Acked-by: Hans Verkuil 
> >  Acked-by: Sakari Ailus 
> >  ---
> >  
> >  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.
> >> 

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

[Sakari Ailus]

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  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 
> > >> 
> > >> Acked-by: Hans Verkuil 
> > >> Acked-by: Sakari Ailus 
> > >> ---
> > >>  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 

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

[Laurent Pinchart]

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
> > 
> > Acked-by: Hans Verkuil 
> > Acked-by: Sakari Ailus 
> > ---
> > 
> >  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 

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

[Mauro Carvalho Chehab]

Em Wed, 23 Mar 2016 17:11:30 +0200
Laurent Pinchart  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
> > >>> 
> > >>> Acked-by: Hans Verkuil 
> > >>> Acked-by: Sakari Ailus 
> > >>> ---
> > >>> 
> > >>>  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
> > 

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

[Hans Verkuil]

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
> 
> Acked-by: Hans Verkuil 
> Acked-by: Sakari Ailus 
> ---
>
>  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mauro Carvalho Chehab]

Em Wed, 23 Mar 2016 15:57:10 +0100
Hans Verkuil  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
>  
>  Acked-by: Hans Verkuil 
>  Acked-by: Sakari Ailus 
>  ---
> 
>   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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laurent Pinchart]

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
> >>> 
> >>> Acked-by: Hans Verkuil 
> >>> Acked-by: Sakari Ailus 
> >>> ---
> >>> 
> >>>  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 

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

[Laurent Pinchart]

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
>  
>  Acked-by: Hans Verkuil 
>  Acked-by: Sakari Ailus 
>  ---
>  
>   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.

-- 
Regards,

Laurent Pinchart

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

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

[Mauro Carvalho Chehab]

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 
> >> Acked-by: Hans Verkuil 
> >> Acked-by: Sakari Ailus 
> >> ---
> >>  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 

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

[Hans Verkuil]

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
 
 Acked-by: Hans Verkuil 
 Acked-by: Sakari Ailus 
 ---

  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laurent Pinchart]

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
> >> 
> >> Acked-by: Hans Verkuil 
> >> Acked-by: Sakari Ailus 
> >> ---
> >> 
> >>  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)
> >>

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

[Hans Verkuil]

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 
>> Acked-by: Hans Verkuil 
>> Acked-by: Sakari Ailus 
>> ---
>>  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
>> @@ 

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

[Mauro Carvalho Chehab]

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 
> Acked-by: Hans Verkuil 
> Acked-by: Sakari Ailus 
> ---
>  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
> + *