[PATCH RFC v2 09/16] media: use media_graph_obj for link endpoints

Em Tue, 11 Aug 2015 14:25:18 +0200
Hans Verkuil [off-list ref] 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:

quoted

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 <redacted>

<snip>

quoted

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?).

quoted

 
 /* 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.

quoted

 	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.

quoted

 void graph_obj_init(struct media_device *mdev,
 		    enum media_graph_type type,
 		    struct media_graph_obj *gobj);

Regards,

	Hans