Re: [PATCH v7 18/44] [media] media: make media_link more generic to handle interace links

[Mauro Carvalho Chehab <mchehab@osg.samsung.com>]

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