Hi Javier,
Thank you for the patch.
On Wednesday 09 September 2015 10:48:29 Javier Martinez Canillas wrote:
> On 09/09/2015 10:03 AM, Sakari Ailus wrote:
> > On Sun, Aug 30, 2015 at 12:06:29AM -0300, Mauro Carvalho Chehab wrote:
> >> From: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> >>
> >> The omap3isp driver parses the graph endpoints to know how many
> >> subdevices needs to be registered async and register notifiers callbacks
> >> for to know when these are bound and when the async registrations are
> >> completed.
> >>
> >> Currently the entities pad are linked with the correct ISP input
> >> interface when the subdevs are bound but it happens before entitities are
> >> registered with the media device so that won't work now that the entity
> >> links list is initialized on device registration.
> >>
> >> So instead creating the pad links when the subdevice is bound, create
> >> them on the complete callback once all the subdevices have been bound but
> >> only try to create for the ones that have a bus configuration set during
> >> bound.
> >>
> >> Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
> >>
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c index b8f6f81d2db2..69e7733d36cd
> >> 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -2321,26 +2321,33 @@ static int isp_subdev_notifier_bound(struct
> >> v4l2_async_notifier *async,
> >> struct v4l2_subdev *subdev,
> >> struct v4l2_async_subdev *asd)
> >> {
> >> - struct isp_device *isp = container_of(async, struct isp_device,
> >> - notifier);
> >> struct isp_async_subdev *isd =
> >> container_of(asd, struct isp_async_subdev, asd);
> >> - int ret;
> >> -
> >> - ret = isp_link_entity(isp, &subdev->entity, isd->bus.interface);
> >> - if (ret < 0)
> >> - return ret;
> >>
> >> isd->sd = subdev;
> >> isd->sd->host_priv = &isd->bus;
> >>
> >> - return ret;
> >> + return 0;
> >> }
> >>
> >> static int isp_subdev_notifier_complete(struct v4l2_async_notifier
> >> *async)
> >> {
> >> struct isp_device *isp = container_of(async, struct isp_device,
> >> notifier);
> >> + struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
> >> + struct v4l2_subdev *sd;
> >> + struct isp_bus_cfg *bus;
> >> + int ret;
> >> +
> >> + list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
> >> + /* Only try to link entities whose interface was set on bound */
> >> + if (sd->host_priv) {
> >> + bus = (struct isp_bus_cfg *)sd->host_priv;
> >> + ret = isp_link_entity(isp, &sd->entity, bus->interface);
> >> + if (ret < 0)
> >> + return ret;
> >> + }
> >> + }
> >> return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
> >> }
> >
> > I think you're working around a problem here, not really fixing it.
> >
> > This change will create the links only after the media device is
> > registered, which means the user may obtain a partial enumeration of
> > links if the enumeration is performed too early.
> >
> > Before this set, the problem also was that the media device was registered
> > before the async entities were bound, again making it possible to obtain a
> > partial enumeration of entities.
>
> You are absolutely correct but I think these are separate issues. The
> problem here is that v4l2_async_test_notify() [0] first invokes the bound
> notifier callback and then calls v4l2_device_register_subdev() that
> register the media entity with the media device.
>
> Since now is a requirement that the entities must be registered prior
> creating pads links (because to init a MEDIA_GRAPH_LINK object a mdev has
> to be set), $SUBJECT is needed regardless of the race between subdev
> registration and the media dev node being available to user-space before
> everything is registered.
> > What I'd suggest instead is that we split media device initialisation and
> > registration to the system; that way the media device can be prepared
> > (entities registered and links created) before it becomes visible to the
> > user space. I can write a patch for that if you like.
>
> Agreed, looking at the implementation it seems that
> __media_device_register() has to be split (possibly being renamed to
> __media_device_init) so it only contains the initialization logic and all
> the media device node registration logic moved to another function (that
> would become media_device_register).
>
> I think the media dev node registration has to be made in the complete
> callback to make sure that happens when all the subdevs have been already
> registered.
>
> Is that what you had in mind? I can also write such a patch if you want.
I think I've already commented on it in my review of another patch (but can't
find it right now), I agree with you. We need to properly think about
initialization (and, for that matter, cleanup as well) order, both for the
media device and the entities. And, as a corollary, for subdevs too. The
current media entity and subdevs initialization and registration code grew in
an organic way without much design behind it, let's not repeat the same
mistake.
> [0]:
> http://lxr.free-electrons.com/source/drivers/media/v4l2-core/v4l2-async.c#L
> 96 [1]:
> http://lxr.free-electrons.com/source/drivers/media/media-device.c#L372
--
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
