Hi Mauro,
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@xxxxxxxxxxxxxxx>
> >> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
> >>
> >> 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.
> 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.
> >> list_add_tail(&entity->list, &mdev->entities);
> >> spin_unlock(&mdev->lock);
> >>
> >> @@ -459,6 +456,7 @@ void media_device_unregister_entity(struct
> >> media_entity *entity)
> >> return;
> >>
> >> spin_lock(&mdev->lock);
> >> + media_gobj_remove(&entity->graph_obj);
> >> list_del(&entity->list);
> >> spin_unlock(&mdev->lock);
> >> entity->parent = NULL;
> >> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> >> index 4834172bf6f8..888cb88e19bf 100644
> >> --- a/drivers/media/media-entity.c
> >> +++ b/drivers/media/media-entity.c
> >> @@ -43,7 +43,12 @@ void media_gobj_init(struct media_device *mdev,
> >> enum media_gobj_type type,
> >> struct media_gobj *gobj)
> >> {
> >> - /* For now, nothing to do */
> >> + /* Create a per-type unique object ID */
> >> + switch (type) {
> >> + case MEDIA_GRAPH_ENTITY:
> >> + gobj->id = media_gobj_gen_id(type, ++mdev->entity_id);
> >> + break;
> >> + }
> >> }
> >>
> >> /**
> >> diff --git a/include/media/media-device.h b/include/media/media-device.h
> >> index a44f18fdf321..f6deef6e5820 100644
> >> --- a/include/media/media-device.h
> >> +++ b/include/media/media-device.h
> >> @@ -41,7 +41,7 @@ struct device;
> >> * @bus_info: Unique and stable device location identifier
> >> * @hw_revision: Hardware device revision
> >> * @driver_version: Device driver version
> >> - * @entity_id: ID of the next entity to be registered
> >> + * @entity_id: Unique ID used on the last entity registered
> >> * @entities: List of registered entities
> >> * @lock: Entities list lock
> >> * @graph_mutex: Entities graph operation lock
> >> @@ -69,6 +69,7 @@ struct media_device {
> >> u32 driver_version;
> >>
> >> u32 entity_id;
> >> +
> >> struct list_head entities;
> >>
> >> /* Protects the entities list */
> >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> >> index c1cd4fba051d..9ca366334bcf 100644
> >> --- a/include/media/media-entity.h
> >> +++ b/include/media/media-entity.h
> >> @@ -33,10 +33,10 @@
> >> /**
> >> * enum media_gobj_type - type of a graph element
> >> *
> >> + * @MEDIA_GRAPH_ENTITY: Identify a media entity
> >
> > I think we should explicitly define here what an entity is.
>
> I'm happy to add a new text here if you have a better idea.
>
> Anyway, for now, I'm more concerned on getting things done than to spend
> lots of time with the comments. As pointed on patch 1/8, while we're
> changing a lot the code, those comments tend to become obsolete very
> quick. The comments there is more like a boilerplate, as, once we finish
> touching at the core, it makes sense to review the comments at media *.h
> files, converting all descriptions to kernel-doc-nano format (there are
> several not using it) and review Documentation/media-framework.txt, adding
> the missing parts.
Yes, we'll need to update Documentation/media-framework.txt. It doesn't have
to be done right now, but should be ready by the time patches get merged. And
from my experience, even though writing documentation feels like wasting time
when you develop a patch series such as this one, it gets even more painful if
you delay it.
> >> */
> >> enum media_gobj_type {
> >> - /* FIXME: add the types here, as we embed media_gobj */
> >> - MEDIA_GRAPH_NONE
> >> + MEDIA_GRAPH_ENTITY,
> >> };
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
