Re: [PATCH RFC v5 7/8] [media] media: add a debug message to warn about gobj creation/removal

[Hans Verkuil <hverkuil@xs4all.nl>]

On 08/19/15 11:50, Mauro Carvalho Chehab wrote:
> Em Wed, 19 Aug 2015 10:36:51 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 08/18/2015 10:04 PM, Mauro Carvalho Chehab wrote:
>>> It helps to check if the media controller is doing the
>>> right thing with the object creation and removal.
>>>
>>> No extra code/data will be produced if DEBUG or
>>> CONFIG_DYNAMIC_DEBUG is not enabled.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>>
>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>>> index a6be50e04736..9e5ebc83ff76 100644
>>> --- a/drivers/media/media-entity.c
>>> +++ b/drivers/media/media-entity.c
>>> @@ -27,6 +27,67 @@
>>>  #include <media/media-device.h>
>>>  
>>>  /**
>>> + *  dev_dbg_obj - Prints in debug mode a change on some object
>>> + *
>>> + * @event_name:	Name of the event to report. Could be __func__
>>> + * @gobj:	Pointer to the object
>>> + *
>>> + * Enabled only if DEBUG or CONFIG_DYNAMIC_DEBUG. Otherwise, it
>>> + * won't produce any code.
>>> + */
>>> +static void dev_dbg_obj(const char *event_name,  struct media_gobj *gobj)
>>> +{
>>> +#if defined(DEBUG) || defined (CONFIG_DYNAMIC_DEBUG)
>>> +	static const char *gobj_type[] = {
>>> +		[MEDIA_GRAPH_ENTITY] = "entity",
>>> +		[MEDIA_GRAPH_PAD] = "pad",
>>> +		[MEDIA_GRAPH_LINK] = "link",
>>> +	};
>>> +
>>> +	switch (media_type(gobj)) {
>>> +	case MEDIA_GRAPH_ENTITY:
>>> +		dev_dbg(gobj->mdev->dev,
>>> +			"%s: id 0x%08x %s#%d '%s'\n",
>>> +			event_name, gobj->id,
>>> +			gobj_type[media_type(gobj)],
>>
>> You can just fill in "entity" in the string since this is always an entity in
>> this case.
> 
> Yes, I know that we actually don't need to use gobj_type[] on the
> macros anymore. On my first version, there was no switch(), and just
> a generic function. I would prefer this way, though. We'll end by
> using the arrays for the links.
> 
> So, I'll do such cleanup when adding interfaces and interface links.

OK.

> 
>>
>>> +			media_localid(gobj),
>>> +			gobj_to_entity(gobj)->name);
>>> +		break;
>>> +	case MEDIA_GRAPH_LINK:
>>> +	{
>>> +		struct media_link *link = gobj_to_link(gobj);
>>> +
>>> +		dev_dbg(gobj->mdev->dev,
>>> +			"%s: id 0x%08x %s#%d: '%s' %s#%d ==> '%s' %s#%d\n",
>>> +			event_name, gobj->id,
>>> +			gobj_type[media_type(gobj)],
>>
>> Ditto.
>>
>>> +			media_localid(gobj),
>>> +
>>> +			link->source->entity->name,
>>> +			gobj_type[media_type(&link->source->graph_obj)],
>>> +			media_localid(&link->source->graph_obj),
>>> +
>>> +			link->sink->entity->name,
>>> +			gobj_type[media_type(&link->sink->graph_obj)],
>>> +			media_localid(&link->sink->graph_obj));
>>> +		break;
>>> +	}
>>> +	case MEDIA_GRAPH_PAD:
>>> +	{
>>> +		struct media_pad *pad = gobj_to_pad(gobj);
>>> +
>>> +		dev_dbg(gobj->mdev->dev,
>>> +			"%s: id 0x%08x %s %s#%d\n",
>>> +			event_name, gobj->id,
>>> +			pad->entity->name,
>>> +			gobj_type[media_type(gobj)],
>>
>> Ditto.
>>
>>> +			media_localid(gobj));
>>> +	}
>>
>> I would add a default case here with a WARN(1) or something like that to make
>> it very clear that someone forgot to extend this switch after adding a new
>> object type.
> 
> Well, newer gcc versions will print a warning if we add a new type at
> the enum and we forget to add here. So, I guess a default case is an
> overkill, and will just add extra code that will never be used.

Ah, that's true.

> 
>> In the LINK case there is no check for unknown object types in the gobj_type[]
>> lookups. I think it is overkill to add a check for that since the objects that
>> are linked are going to be created first, so they would trigger the WARN(1) in
>> the default case first if they are using unhandled types. That should give
>> enough information for the developer.
> 
> Well, I can convert gobj_type into a function and add a default case
> there, as going past the array is something that makes sense to avoid.
> 
> I'll do such change when writing the support for interface links.

OK.

> 
>>
>>> +	}
>>> +#endif
>>> +}
>>> +
>>> +/**
>>>   *  media_gobj_init - Initialize a graph object
>>>   *
>>>   * @mdev:	Pointer to the media_device that contains the object
>>> @@ -43,6 +104,8 @@ void media_gobj_init(struct media_device *mdev,
>>>  			   enum media_gobj_type type,
>>>  			   struct media_gobj *gobj)
>>>  {
>>> +	gobj->mdev = mdev;
>>> +
>>>  	/* Create a per-type unique object ID */
>>>  	switch (type) {
>>>  	case MEDIA_GRAPH_ENTITY:
>>> @@ -55,6 +118,7 @@ void media_gobj_init(struct media_device *mdev,
>>>  		gobj->id = media_gobj_gen_id(type, ++mdev->link_id);
>>>  		break;
>>>  	}
>>> +	dev_dbg_obj(__func__, gobj);
>>>  }
>>>  
>>>  /**
>>> @@ -66,7 +130,7 @@ void media_gobj_init(struct media_device *mdev,
>>>   */
>>>  void media_gobj_remove(struct media_gobj *gobj)
>>>  {
>>> -	/* For now, nothing to do */
>>> +	dev_dbg_obj(__func__, gobj);
>>>  }
>>>  
>>>  /**
>>> @@ -472,13 +536,6 @@ static struct media_link *media_entity_add_link(struct media_entity *entity)
>>>  		entity->links = links;
>>>  	}
>>>  
>>> -	/* Initialize graph object embedded at the new link */
>>> -	if (entity->parent)
>>> -		media_gobj_init(entity->parent, MEDIA_GRAPH_LINK,
>>> -				&entity->links[entity->num_links].graph_obj);
>>> -	else
>>> -		pr_warn("Link created before entity register!\n");
>>> -
>>
>> This is confusing: this was added in patch 5 and now it is removed again. This doesn't
>> seem to have anything to do with adding this debugging facility.
> 
> The warning is a left over from the test phase. I was afraid that such
> condition might be happening. 
> 
> With regards to media_gobj_init(), we can't call it here anymore, as
> it is too early to call dev_dbg_obj(), as the link data is not filled.
> So, this got moved to media_entity_create_link(), to happen after
> filling link->source and link->sink.
> 
>>
>>>  	return &entity->links[entity->num_links++];
>>>  }
>>>  
>>> @@ -501,6 +558,8 @@ )struct media_entity *source, u16 source_pad,
>>>  	link->sink = &sink->pads[sink_pad];
>>>  	link->flags = flags;
>>>  
>>> +	media_gobj_init(source->parent, MEDIA_GRAPH_LINK, &link->graph_obj);
>>> +
>>
>> Same for this,
>>
>>>  	/* Create the backlink. Backlinks are used to help graph traversal and
>>>  	 * are not reported to userspace.
>>>  	 */
>>> @@ -514,6 +573,8 @@ media_entity_create_link(struct media_entity *source, u16 source_pad,
>>>  	backlink->sink = &sink->pads[sink_pad];
>>>  	backlink->flags = flags;
>>>  
>>> +	media_gobj_init(sink->parent, MEDIA_GRAPH_LINK, &backlink->graph_obj);
>>> +
>>
>> and this.
>>
>> Shouldn't these three changes be moved to patch 5?
> 
> Well, I can move this hunk to patch 5, but that will just be hiding 
> the reason why they should be here in the first place: due to the new
> debug function.

As a reviewer it is confusing when something is added in one patch and promptly
removed again in the next. Just saying...

> 
>>
>>>  	link->reverse = backlink;
>>>  	backlink->reverse = link;
>>>  
>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>> index 2ffe015629fa..db1f54f09723 100644
>>> --- a/include/media/media-entity.h
>>> +++ b/include/media/media-entity.h
>>> @@ -59,6 +59,7 @@ enum media_gobj_type {
>>>   * All elements on the media graph should have this struct embedded
>>>   */
>>>  struct media_gobj {
>>> +	struct media_device	*mdev;
>>
>> If you add this here, then the parent pointer in struct media_entity should be
>> removed to avoid having duplicate fields.
> 
> Yes, I know, but I don't want to do it on just one patch. I'm trying to
> do the minimal amount of changes to introduce interface and interface
> links. My plan is to actually remove entity->parent latter, as such change
> will touch inside the drivers. It could be a big patch that won't cause
> any real gain, except for 4 bytes saved. So, I really prefer to postpone
> this to a cleanup series. I'm also hoping to merge vimc driver before that,
> as this would mean one less thing to be rebased on it ;)

OK, but then please add a comment before the parent field that mentions that
it is now a duplicate of mdev and will be removed in a future patch (or
something along those lines).

Regards,

	Hans

> 
>>
>> I understand why you add this and I don't object, but I would split this change
>> off into a separate patch that also includes removing the media_entity parent
>> field.
> 
> Well, you didn't complain about that on the previous versions (and mdev
> were on all other patches) :-) 
> 
> I actually think that this way is easier to review, as one of the usages
> of mdev is explicit on this patch, and, as I said, the removal of the
> entity->parent is just a cleanup that can be done latter.
> 
> Let's focus on adding the new functionality first.
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>  	u32			id;
>>>  };
>>>  
>>> @@ -190,6 +191,12 @@ struct media_entity_graph {
>>>  #define gobj_to_entity(gobj) \
>>>  		container_of(gobj, struct media_entity, graph_obj)
>>>  
>>> +#define gobj_to_pad(gobj) \
>>> +		container_of(gobj, struct media_pad, graph_obj)
>>> +
>>> +#define gobj_to_link(gobj) \
>>> +		container_of(gobj, struct media_link, graph_obj)
>>> +
>>>  void media_gobj_init(struct media_device *mdev,
>>>  		    enum media_gobj_type type,
>>>  		    struct media_gobj *gobj);
>>>
>>
> --
> 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
>