Hi Mauro, On Thursday 15 Dec 2016 10:57:16 Mauro Carvalho Chehab wrote: > Em Thu, 15 Dec 2016 09:42:35 -0300 Javier Martinez Canillas escreveu: > > On 12/15/2016 09:37 AM, Mauro Carvalho Chehab wrote: > > > > [snip] > > > >> What happens is that omap3isp driver calls media_device_unregister() > >> too early. Right now, it is called at omap3isp_video_device_release(), > >> with happens when a driver unbind is ordered by userspace, and not after > >> the last usage of all /dev/video?? devices. > >> > >> There are two possible fixes: > >> > >> 1) at omap3isp_video_device_release(), streamoff all streams and mark > >> that the media device will be gone. > > I actually meant to say: isp_unregister_entities() here. > > >> 2) instead of using video_device_release_empty for the > >> video->video.release, create a omap3isp_video_device_release() that > >> will call media_device_unregister() when destroying the last /dev/video?? > >> devnode. > > > > There's also option (3), to have a proper refcounting to make sure that > > the media device node is not freed until all references to it are gone. > > Yes, that's another alternative. And I think that's the only one that will bring us sanity in the long term. > > I understand that's what Sakari's RFC patches do. I'll try to make some > > time tomorrow to test and review his patches. > > The biggest problem with Sakari's patches is that it starts by > reverting 3 patches, and this will cause regressions on existing > devices. > > Development should be incremental. Yes, it should, but there's also a reason git has a revert command. When patches are broken they should be reverted. Broken means that they do more harm than good. This is usually understood as introducing a bug worse than the gain the patch is supposed to bring. Broken can also mean that the patch makes incremental development of a proper solution very difficult while still failing to fix the initial problem completely. I believe the three patches in question fall into that category. And let's not take this personally, I don't care who have authored them, and there's certainly no shame getting a patch reverted. It should be considered as a review that comes after merge, it might not be the most pleasant one, but we I'm sure we all appreciate how reviews help use avoiding the same mistakes in the future and improving ourselves. > I didn't review carefully his series (as it started the wrong way), Please review it, as we need to get an agreement on the direction we want to take. Then we can discuss whether the reverts are really such a problem. I don't think inflicting ourselves the pain that would come with pure incremental development would bring us anything good, especially if we consider that we'll merge the reverts with the patch series that fixes the problem, so we're talking about bisection of unbind code paths that remained broken for years before the attempted fix, and are still broken with it as current code is racy anyway. > but I guess there's another problem on it: as OMAP3 remove entities > at isp_unregister_entities(), while adding a kref to media_device > will prevent an oops, the streamoff logic won't work anymore, as > the entities that were supposed to be at the graph will have been > removed by then. We need to fix drivers, that's for sure, and we're working on the OMAP3 ISP driver first as a proof of concept. > Ok, we can roll the snow ball and add kref's to entities and links, > but IMHO, we're trying to kill a fly with a death star: instead, > the better is to just fix the driver in a way that it would be > streaming off everything at isp_unregister_entities(), before > dropping the entities and the media controller. That won't be enough. Even if you're not entirely convinced by the reasons explained in this mail thread, remember that we will need sooner or later to implement support for media graph update at runtime. Refcounting will be needed, let's design it in the cleanest possible way. -- 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
