Re: [PATCH v6 3/8] [media] media: use media_gobj inside entities

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Mauro,

On Friday 21 August 2015 18:01:31 Mauro Carvalho Chehab wrote:
> Em Fri, 21 Aug 2015 20:51:10 +0300 Laurent Pinchart escreveu:
> > On Friday 21 August 2015 07:09:44 Mauro Carvalho Chehab wrote:
> >> Em Fri, 21 Aug 2015 04:10:03 +0300 Laurent Pinchart escreveu:
> >>> On Wednesday 19 August 2015 08:01:50 Mauro Carvalho Chehab wrote:
> >>>> As entities are graph elements, let's embed media_gobj
> >>>> on it. That ensures an unique ID for entities that can be
> >>>> global along the entire media controller.
> >>>> 
> >>>> For now, we'll keep the already existing entity ID. Such
> >>>> field need to be dropped at some point, but for now, let's
> >>>> not do this, to avoid needing to review all drivers and
> >>>> the userspace apps.
> >>>> 
> >>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >>>> 
> >>>> diff --git a/drivers/media/media-device.c
> >>>> b/drivers/media/media-device.c
> >>>> index e429605ca2c3..81d6a130efef 100644
> >>>> --- a/drivers/media/media-device.c
> >>>> +++ b/drivers/media/media-device.c
> >>>> @@ -379,7 +379,6 @@ int __must_check __media_device_register(struct
> >>>> media_device *mdev,
> >>>> 	if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0))
> >>>>  		return -EINVAL;
> >>>> 
> >>>> -	mdev->entity_id = 1;
> >>>>  	INIT_LIST_HEAD(&mdev->entities);
> >>>>  	spin_lock_init(&mdev->lock);
> >>>>  	mutex_init(&mdev->graph_mutex);
> >>>> @@ -433,10 +432,8 @@ int __must_check 
> >>>> media_device_register_entity(struct media_device *mdev,
> >>>> 
> >>>> 	entity->parent = mdev;
> >>>> 	
> >>>>  	spin_lock(&mdev->lock);
> >>>> 
> >>>> -	if (entity->id == 0)
> >>>> -		entity->id = mdev->entity_id++;
> >>>> -	else
> >>>> -		mdev->entity_id = max(entity->id + 1, mdev->entity_id);
> >>>> +	/* Initialize media_gobj embedded at the entity */
> >>>> +	media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
> >>> 
> >>> Graph object initialization should be moved to media_entity_init() to
> >>> keep initialization separate from registration.
> >> 
> >> Won't work. I tried. My first RFC patches were doing that.
> >> 
> >> The problem is that media_entity_init() is currently called too early
> >> at the V4L2 drivers, before having mdev assigned.
> > 
> > That looks like a problem that should be fixed in the drivers then. The
> > initialization of media devices and entities hasn't been thought of
> > correctly, let's not carry mistakes forward.
> 
> In this particular case, calling media_gobj_init() during
> media_entity_register() won't cause any troubles, and moving it latter
> to media_entity_init() is a two lines patch, once drivers got fixed
> by the drivers maintainers.

It's the "once drivers got fixed by the drivers maintainers" that worries me. 
We all know it unfortunately doesn't happen by itself, and I believe it will 
become more and more difficult as time goes by and more drivers use the MC 
framework. We're trying to refactor, clean up and extend MC, both in-kernel 
and towards userspace, to correct design and implementation mistakes. I'm 
worried that we'll make different but similarly painful mistakes if we don't 
take a bit of time to do things properly now. And while it will require a 
couple more patches, I don't think we're looking at months of work either.

It might be that assigning an ID to objects can only be done at at 
registration time, especially for standalone subdev drivers (I'm thinking 
about the I2C camera sensors in particular). Still, calling media_gobj_init() 
in media_device_register_entity() doesn't sound right. Maybe the solution is 
to use media_gobj_init() for init-time initialization, and creating a 
media_gobj_assign_id() (or similarly-named) function to call at registration 
time. What I'd really like to see is clear explicit rules regarding how 
init/cleanup and register/unregister interact and how they should be used by 
drivers. The current mess is partly caused by not having thought this out 
properly to start with.

I also have a feeling that we'll realize changes are required when 
implementing support for dynamic changes.

> >> Also, objects without PADs don't call media_entity_init(). At long
> >> term, this function may even disappear, as it only exists today to
> >> allocate arrays for pads/links. If we get rid of the pads array, it
> >> will make no sense to keep it.
> > 
> > I wouldn't do that, as if we later add a field to media_entity that
> > requires an initialization function to be called we would need to go and
> > patch all the code that instantiates media_entity to add init calls. I'd
> > prefer keeping the media_entity_init function even if all it does is call
> > media_gobj_init.
>
> OK.
> 
> >>>>  	list_add_tail(&entity->list, &mdev->entities);
> >>>>  	spin_unlock(&mdev->lock);
> >>>> 

-- 
Regards,

Laurent Pinchart