Em Tue, 11 Aug 2015 14:25:18 +0200 Hans Verkuil <hansv...@cisco.com> escreveu:
> Hi Mauro, > > Thanks for posting the missing patches. Thanks for reviewing this patch series! > > On 08/11/15 14:09, Mauro Carvalho Chehab wrote: > > As we'll need to create links between entities and interfaces, > > we need to identify the link endpoints by the media_graph_obj. > > > > Most of the changes here was done by this small script: > > > > for i in `find drivers/media -type f` `find drivers/staging/media -type f`; > > do > > perl -ne > > 's,([\w]+)\-\>(source|sink)\-\>entity,gobj_to_pad($1->$2)->entity,; print > > $_;' <$i >a && mv a $i > > done > > > > Please note that, while we're now using graph_obj to reference > > the link endpoints, we're still assuming that all endpoints are > > pads. This is true for all existing links, so no problems > > are expected so far. > > > > Yet, as we introduce links between entities and interfaces, > > we may need to change some existing code to work with links > > that aren't pad to pad. > > > > Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com> > > > > <snip> > > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > > index 403019035424..f6e2136480f1 100644 > > --- a/include/media/media-entity.h > > +++ b/include/media/media-entity.h > > @@ -43,6 +43,17 @@ enum media_graph_type { > > MEDIA_GRAPH_LINK, > > }; > > > > +/** > > + * enum media_graph_link_dir - direction of a link > > + * > > + * @MEDIA_LINK_DIR_BIDIRECTIONAL Link is bidirectional > > + * @MEDIA_LINK_DIR_PAD_0_TO_1 Link is unidirectional, > > + * from port 0 (source) to port 1 (sink) > > + */ > > +enum media_graph_link_dir { > > + MEDIA_LINK_DIR_BIDIRECTIONAL, > > + MEDIA_LINK_DIR_PORT0_TO_PORT1, > > +}; > > 1) the comment and the actual enum are out-of-sync Ah, yes. I was in doubt about using PAD or PORT here. I ended by using port at the links, as the endpoints can either be an interface/entity or a pad. So, I decided to use port. > 2) why not just make a 'BIRECTIONAL' link flag instead of inventing > a new enum? Adding yet another field seems overkill to me. Have a > 'BIDIRECTIONAL' flag seems perfectly OK to me (and useful for the > application as well). Yeah, we can use flags, instead. I decided to use an enum here just to make it clearer about the two possible options. I was actually considering to rename media_link source/sink to port0/port1, as using "source"/"sink" names on a bidirection link doesn't make sense. I'm still in doubt about such rename, though, as it would make harder to inspect the graph traversal routines. Also, I want to force all places that create a link to choose between either BIRECTIONAL or PORT0_TO_PORT1, as this makes easier to review if the code is doing the right thing when inspecting it. In summary, I would prefer to keep this internally as a separate enum, at least for now. We can latter simplify it and use a flag for that (or maybe two flags?). > > > > > /* Structs to represent the objects that belong to a media graph */ > > > > @@ -72,9 +83,9 @@ struct media_pipeline { > > > > struct media_link { > > struct list_head list; > > - struct media_graph_obj graph_obj; > > - struct media_pad *source; /* Source pad */ > > - struct media_pad *sink; /* Sink pad */ > > + struct media_graph_obj graph_obj; > > + enum media_graph_link_dir dir; > > + struct media_graph_obj *source, *sink; > > I'm not too keen about all the gobj_to_foo(obj) macros that this requires. It > is rather ugly code. > > What about this: > > union { > struct media_graph_obj *source; > struct media_pad *source_pad; > struct media_interface *source_intf; > }; > union { > struct media_graph_obj *sink; > struct media_pad *sink_pad; > struct media_entity *sink_ent; > }; > > Now the code can just use ->source_pad etc. good idea. Will do that on a version 3. I think that, in this case, the best is to write a note that the first element at pad/entity/interface should be the graph_obj. I would actually call port0_intf and port1_ent on the above structs, as it makes no sense to call sink/source for interface->entity links. > > struct media_link *reverse; /* Link in the reverse direction */ > > unsigned long flags; /* Link flags (MEDIA_LNK_FL_*) */ > > }; > > @@ -115,6 +126,11 @@ struct media_entity { > > u32 group_id; /* Entity group ID */ > > > > u16 num_pads; /* Number of sink and source pads */ > > + > > + /* > > + * Both num_links and num_backlinks are used only to report > > + * the number of links via MEDIA_IOC_ENUM_ENTITIES at media_device.c > > + */ > > u16 num_links; /* Number of existing links, both > > * enabled and disabled */ > > u16 num_backlinks; /* Number of backlinks */ > > @@ -171,6 +187,12 @@ struct media_entity_graph { > > #define gobj_to_entity(gobj) \ > > container_of(gobj, struct media_entity, graph_obj) > > > > +#define gobj_to_link(gobj) \ > > + container_of(gobj, struct media_link, graph_obj) > > + > > +#define gobj_to_pad(gobj) \ > > + container_of(gobj, struct media_pad, graph_obj) > > + > > I saw a lot of type checks (if (link->sink.type != MEDIA_GRAPH_PAD)) > that I think would look cleaner if there was a simple static inline > helper: > > static inline bool is_pad(const struct media_graph_obj *obj) > { > return obj->type == MEDIA_GRAPH_PAD; > } > > media_is_pad() will work as well, but I think brevity is more useful > in this case. Personal opinion, though. I would use media_is_pad() instead, to avoid namespace conflicts. We may eventually need other inline functions for other types in the future. > > > void graph_obj_init(struct media_device *mdev, > > enum media_graph_type type, > > struct media_graph_obj *gobj); > > > > Regards, > > Hans -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html