| Message ID | cec7a29d26c1abc95bd0df9ca6a92910ec1561ad.1440359643.git.mchehab@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/23/2015 10:17 PM, Mauro Carvalho Chehab wrote: > By adding an union at media_link, we get for free a way to > represent interface->entity links. > > No need to change anything at the code, just at the internal > header file. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index 17bb5cbbd67d..f6e8fa801cf9 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -75,14 +75,20 @@ struct media_pipeline { > struct media_link { > struct media_gobj graph_obj; > struct list_head list; > - struct media_pad *source; /* Source pad */ > - struct media_pad *sink; /* Sink pad */ > + union { > + struct media_gobj *port0; > + struct media_pad *source; > + }; > + union { > + struct media_gobj *port1; > + struct media_pad *sink; > + }; > struct media_link *reverse; /* Link in the reverse direction */ > unsigned long flags; /* Link flags (MEDIA_LNK_FL_*) */ > }; > > struct media_pad { > - struct media_gobj graph_obj; > + struct media_gobj graph_obj; /* should be the first object */ > struct media_entity *entity; /* Entity this pad belongs to */ > u16 index; /* Pad index in the entity pads array */ > unsigned long flags; /* Pad flags (MEDIA_PAD_FL_*) */ > @@ -105,7 +111,7 @@ struct media_entity_operations { > }; > > struct media_entity { > - struct media_gobj graph_obj; > + struct media_gobj graph_obj; /* should be the first object */ Does adding these "/* should be the first object */" comments in media_entity and media_pad belong to this patch? It doesn't seem to be related to the union change. I would also suggest that "/* must be first field in struct */" is a better phrase. Regards, Hans > struct list_head list; > const char *name; /* Entity name */ > u32 type; /* Entity type (MEDIA_ENT_T_*) */ > @@ -119,7 +125,7 @@ struct media_entity { > u16 num_backlinks; /* Number of backlinks */ > > struct media_pad *pads; /* Pads array (num_pads objects) */ > - struct list_head links; /* Links list */ > + struct list_head links; /* Pad-to-pad links list */ > > const struct media_entity_operations *ops; /* Entity operations */ > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/23/2015 10:17 PM, Mauro Carvalho Chehab wrote: > By adding an union at media_link, we get for free a way to > represent interface->entity links. > > No need to change anything at the code, just at the internal > header file. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index 17bb5cbbd67d..f6e8fa801cf9 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -75,14 +75,20 @@ struct media_pipeline { > struct media_link { > struct media_gobj graph_obj; > struct list_head list; > - struct media_pad *source; /* Source pad */ > - struct media_pad *sink; /* Sink pad */ > + union { > + struct media_gobj *port0; > + struct media_pad *source; > + }; > + union { > + struct media_gobj *port1; Why add port0 and port1 here instead of intf and entity (now added in patch 20)? port0/port1 isn't used, so I'd postpone adding that until it is needed. Part of the reason is also that I am not convinced about the 'port' name, so let's not add this yet. Regards, Hans > + struct media_pad *sink; > + }; > struct media_link *reverse; /* Link in the reverse direction */ > unsigned long flags; /* Link flags (MEDIA_LNK_FL_*) */ > }; > > struct media_pad { > - struct media_gobj graph_obj; > + struct media_gobj graph_obj; /* should be the first object */ > struct media_entity *entity; /* Entity this pad belongs to */ > u16 index; /* Pad index in the entity pads array */ > unsigned long flags; /* Pad flags (MEDIA_PAD_FL_*) */ > @@ -105,7 +111,7 @@ struct media_entity_operations { > }; > > struct media_entity { > - struct media_gobj graph_obj; > + struct media_gobj graph_obj; /* should be the first object */ > struct list_head list; > const char *name; /* Entity name */ > u32 type; /* Entity type (MEDIA_ENT_T_*) */ > @@ -119,7 +125,7 @@ struct media_entity { > u16 num_backlinks; /* Number of backlinks */ > > struct media_pad *pads; /* Pads array (num_pads objects) */ > - struct list_head links; /* Links list */ > + struct list_head links; /* Pad-to-pad links list */ > > const struct media_entity_operations *ops; /* Entity operations */ > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Tue, 25 Aug 2015 09:28:55 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 08/23/2015 10:17 PM, Mauro Carvalho Chehab wrote: > > By adding an union at media_link, we get for free a way to > > represent interface->entity links. > > > > No need to change anything at the code, just at the internal > > header file. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > > > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > > index 17bb5cbbd67d..f6e8fa801cf9 100644 > > --- a/include/media/media-entity.h > > +++ b/include/media/media-entity.h > > @@ -75,14 +75,20 @@ struct media_pipeline { > > struct media_link { > > struct media_gobj graph_obj; > > struct list_head list; > > - struct media_pad *source; /* Source pad */ > > - struct media_pad *sink; /* Sink pad */ > > + union { > > + struct media_gobj *port0; > > + struct media_pad *source; > > + }; > > + union { > > + struct media_gobj *port1; > > + struct media_pad *sink; > > + }; > > struct media_link *reverse; /* Link in the reverse direction */ > > unsigned long flags; /* Link flags (MEDIA_LNK_FL_*) */ > > }; > > > > struct media_pad { > > - struct media_gobj graph_obj; > > + struct media_gobj graph_obj; /* should be the first object */ > > struct media_entity *entity; /* Entity this pad belongs to */ > > u16 index; /* Pad index in the entity pads array */ > > unsigned long flags; /* Pad flags (MEDIA_PAD_FL_*) */ > > @@ -105,7 +111,7 @@ struct media_entity_operations { > > }; > > > > struct media_entity { > > - struct media_gobj graph_obj; > > + struct media_gobj graph_obj; /* should be the first object */ > > Does adding these "/* should be the first object */" comments in media_entity > and media_pad belong to this patch? It doesn't seem to be related to the union > change. It is related. See: union { struct media_gobj *port0; struct media_pad *source; }; If we keep the graph_obj as the first part of media_pad, the pad object can be accessed as either: link->port0 or link->source->graph_obj. In practice, it means that, if one wants to know the type of the object, it could do: type = media_type(link->port0) Technically, we don't need yet to put the comment at the media_entity, as the patch that will create interface->entity links is patch 20. But I opted to do the comments change altogether. > > I would also suggest that "/* must be first field in struct */" is a better > phrase. OK. > > Regards, > > Hans > > > struct list_head list; > > const char *name; /* Entity name */ > > u32 type; /* Entity type (MEDIA_ENT_T_*) */ > > @@ -119,7 +125,7 @@ struct media_entity { > > u16 num_backlinks; /* Number of backlinks */ > > > > struct media_pad *pads; /* Pads array (num_pads objects) */ > > - struct list_head links; /* Links list */ > > + struct list_head links; /* Pad-to-pad links list */ > > > > const struct media_entity_operations *ops; /* Entity operations */ > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Tue, 25 Aug 2015 09:38:57 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 08/23/2015 10:17 PM, Mauro Carvalho Chehab wrote: > > By adding an union at media_link, we get for free a way to > > represent interface->entity links. > > > > No need to change anything at the code, just at the internal > > header file. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > > > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > > index 17bb5cbbd67d..f6e8fa801cf9 100644 > > --- a/include/media/media-entity.h > > +++ b/include/media/media-entity.h > > @@ -75,14 +75,20 @@ struct media_pipeline { > > struct media_link { > > struct media_gobj graph_obj; > > struct list_head list; > > - struct media_pad *source; /* Source pad */ > > - struct media_pad *sink; /* Sink pad */ > > + union { > > + struct media_gobj *port0; > > + struct media_pad *source; > > + }; > > + union { > > + struct media_gobj *port1; > > Why add port0 and port1 here instead of intf and entity (now added in patch 20)? > port0/port1 isn't used, so I'd postpone adding that until it is needed. Because we need to use it to be able to identify the object type. This is used (actually, it should be used - see my comments for 19/44) on the next patch. > Part of the reason is also that I am not convinced about the 'port' name, so > let's not add this yet. I'm not bound to "port" name. If you have a better suggestion, this could easily be fixed. However, we can't use here source/sink, as this means something directional, but the interface<->entity link is bidirectional. I chose "port" because it is the ITU-T name for the graph element that represents the connection points at the entities. > > Regards, > > Hans > > > + struct media_pad *sink; > > + }; > > struct media_link *reverse; /* Link in the reverse direction */ > > unsigned long flags; /* Link flags (MEDIA_LNK_FL_*) */ > > }; > > > > struct media_pad { > > - struct media_gobj graph_obj; > > + struct media_gobj graph_obj; /* should be the first object */ > > struct media_entity *entity; /* Entity this pad belongs to */ > > u16 index; /* Pad index in the entity pads array */ > > unsigned long flags; /* Pad flags (MEDIA_PAD_FL_*) */ > > @@ -105,7 +111,7 @@ struct media_entity_operations { > > }; > > > > struct media_entity { > > - struct media_gobj graph_obj; > > + struct media_gobj graph_obj; /* should be the first object */ > > struct list_head list; > > const char *name; /* Entity name */ > > u32 type; /* Entity type (MEDIA_ENT_T_*) */ > > @@ -119,7 +125,7 @@ struct media_entity { > > u16 num_backlinks; /* Number of backlinks */ > > > > struct media_pad *pads; /* Pads array (num_pads objects) */ > > - struct list_head links; /* Links list */ > > + struct list_head links; /* Pad-to-pad links list */ > > > > const struct media_entity_operations *ops; /* Entity operations */ > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/25/15 11:53, Mauro Carvalho Chehab wrote: > Em Tue, 25 Aug 2015 09:38:57 +0200 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> On 08/23/2015 10:17 PM, Mauro Carvalho Chehab wrote: >>> By adding an union at media_link, we get for free a way to >>> represent interface->entity links. >>> >>> No need to change anything at the code, just at the internal >>> header file. >>> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> >>> >>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h >>> index 17bb5cbbd67d..f6e8fa801cf9 100644 >>> --- a/include/media/media-entity.h >>> +++ b/include/media/media-entity.h >>> @@ -75,14 +75,20 @@ struct media_pipeline { >>> struct media_link { >>> struct media_gobj graph_obj; >>> struct list_head list; >>> - struct media_pad *source; /* Source pad */ >>> - struct media_pad *sink; /* Sink pad */ >>> + union { >>> + struct media_gobj *port0; >>> + struct media_pad *source; >>> + }; >>> + union { >>> + struct media_gobj *port1; >> >> Why add port0 and port1 here instead of intf and entity (now added in patch 20)? >> port0/port1 isn't used, so I'd postpone adding that until it is needed. > > Because we need to use it to be able to identify the object type. > This is used (actually, it should be used - see my comments for 19/44) > on the next patch. Ah, right. It makes much more sense when the debug function uses it. The fact that it didn't confused me. > >> Part of the reason is also that I am not convinced about the 'port' name, so >> let's not add this yet. > > I'm not bound to "port" name. If you have a better suggestion, this could > easily be fixed. > > However, we can't use here source/sink, as this means something > directional, but the interface<->entity link is bidirectional. > > I chose "port" because it is the ITU-T name for the graph element that > represents the connection points at the entities. Since it is a media_gobj pointer, I would think 'gobj0/1' would be more sensible since that doesn't introduce a new name ('port'). Regards, Hans > >> >> Regards, >> >> Hans >> >>> + struct media_pad *sink; >>> + }; >>> struct media_link *reverse; /* Link in the reverse direction */ >>> unsigned long flags; /* Link flags (MEDIA_LNK_FL_*) */ >>> }; >>> >>> struct media_pad { >>> - struct media_gobj graph_obj; >>> + struct media_gobj graph_obj; /* should be the first object */ >>> struct media_entity *entity; /* Entity this pad belongs to */ >>> u16 index; /* Pad index in the entity pads array */ >>> unsigned long flags; /* Pad flags (MEDIA_PAD_FL_*) */ >>> @@ -105,7 +111,7 @@ struct media_entity_operations { >>> }; >>> >>> struct media_entity { >>> - struct media_gobj graph_obj; >>> + struct media_gobj graph_obj; /* should be the first object */ >>> struct list_head list; >>> const char *name; /* Entity name */ >>> u32 type; /* Entity type (MEDIA_ENT_T_*) */ >>> @@ -119,7 +125,7 @@ struct media_entity { >>> u16 num_backlinks; /* Number of backlinks */ >>> >>> struct media_pad *pads; /* Pads array (num_pads objects) */ >>> - struct list_head links; /* Links list */ >>> + struct list_head links; /* Pad-to-pad links list */ >>> >>> const struct media_entity_operations *ops; /* Entity operations */ >>> >>> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/media/media-entity.h b/include/media/media-entity.h index 17bb5cbbd67d..f6e8fa801cf9 100644 --- a/include/media/media-entity.h +++ b/include/media/media-entity.h @@ -75,14 +75,20 @@ struct media_pipeline { struct media_link { struct media_gobj graph_obj; struct list_head list; - struct media_pad *source; /* Source pad */ - struct media_pad *sink; /* Sink pad */ + union { + struct media_gobj *port0; + struct media_pad *source; + }; + union { + struct media_gobj *port1; + struct media_pad *sink; + }; struct media_link *reverse; /* Link in the reverse direction */ unsigned long flags; /* Link flags (MEDIA_LNK_FL_*) */ }; struct media_pad { - struct media_gobj graph_obj; + struct media_gobj graph_obj; /* should be the first object */ struct media_entity *entity; /* Entity this pad belongs to */ u16 index; /* Pad index in the entity pads array */ unsigned long flags; /* Pad flags (MEDIA_PAD_FL_*) */ @@ -105,7 +111,7 @@ struct media_entity_operations { }; struct media_entity { - struct media_gobj graph_obj; + struct media_gobj graph_obj; /* should be the first object */ struct list_head list; const char *name; /* Entity name */ u32 type; /* Entity type (MEDIA_ENT_T_*) */ @@ -119,7 +125,7 @@ struct media_entity { u16 num_backlinks; /* Number of backlinks */ struct media_pad *pads; /* Pads array (num_pads objects) */ - struct list_head links; /* Links list */ + struct list_head links; /* Pad-to-pad links list */ const struct media_entity_operations *ops; /* Entity operations */
By adding an union at media_link, we get for free a way to represent interface->entity links. No need to change anything at the code, just at the internal header file. Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>