Em Tue, 11 Aug 2015 14:25:18 +0200
Hans Verkuil <hansverk@xxxxxxxxx> 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 <mchehab@xxxxxxxxxxxxxxx>
> >
>
> <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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
