From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-pl0-f67.google.com ([209.85.160.67]:44727 "EHLO mail-pl0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965947AbeEIXGh (ORCPT ); Wed, 9 May 2018 19:06:37 -0400 Received: by mail-pl0-f67.google.com with SMTP id e6-v6so114795plt.11 for ; Wed, 09 May 2018 16:06:37 -0700 (PDT) From: Steve Longerbeam Subject: Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev To: Sakari Ailus Cc: Yong Zhi , Mauro Carvalho Chehab , Laurent Pinchart , niklas.soderlund@ragnatech.se, Sebastian Reichel , Hans Verkuil , Philipp Zabel , linux-media@vger.kernel.org References: <1521592649-7264-1-git-send-email-steve_longerbeam@mentor.com> <1521592649-7264-4-git-send-email-steve_longerbeam@mentor.com> <20180420122450.j3wkyoardgpyzbh2@paasikivi.fi.intel.com> <854dab64-caf7-be8e-e5b6-10ff78aa811a@gmail.com> <20180508101235.wctorewkzt3jgxnh@kekkonen.localdomain> Message-ID: <90f52736-1a4d-f409-c553-d59901e413fa@gmail.com> Date: Wed, 9 May 2018 16:06:32 -0700 MIME-Version: 1.0 In-Reply-To: <20180508101235.wctorewkzt3jgxnh@kekkonen.localdomain> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-media-owner@vger.kernel.org List-ID: On 05/08/2018 03:12 AM, Sakari Ailus wrote: > On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote: >> Hi Sakari, >> >> >> On 04/20/2018 05:24 AM, Sakari Ailus wrote: >>> Hi Steve, >>> >>> Thanks for the patchset. >>> >>> On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote: >>>> v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks >>>> that the asd's match_type is valid and that no other equivalent asd's >>>> have already been added to this notifier's asd list, or to other >>>> registered notifier's waiting or done lists, and increments num_subdevs. >>>> >>>> v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs >>>> array, otherwise it would have to re-allocate the array every time the >>>> function was called. In place of the subdevs array, the function adds >>>> the asd to a new master asd_list. The function will return error with a >>>> WARN() if it is ever called with the subdevs array allocated. >>>> >>>> In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(), >>>> and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs >>>> array or a non-empty notifier->asd_list. >>> I do agree with the approach, but this patch leaves the remaining users of >>> the subdevs array in the notifier intact. Could you rework them to use the >>> v4l2_async_notifier_add_subdev() as well? >>> >>> There seem to be just a few of them --- exynos4-is and rcar_drif. >> I count more than a few :) >> >> % cd drivers/media && grep -l -r --include "*.[ch]" >> 'notifier[\.\-]>*subdevs[   ]*=' >> v4l2-core/v4l2-async.c >> platform/pxa_camera.c >> platform/ti-vpe/cal.c >> platform/exynos4-is/media-dev.c >> platform/qcom/camss-8x16/camss.c >> platform/soc_camera/soc_camera.c >> platform/atmel/atmel-isi.c >> platform/atmel/atmel-isc.c >> platform/stm32/stm32-dcmi.c >> platform/davinci/vpif_capture.c >> platform/davinci/vpif_display.c >> platform/renesas-ceu.c >> platform/am437x/am437x-vpfe.c >> platform/xilinx/xilinx-vipp.c >> platform/rcar_drif.c >> >> >> So not including v4l2-core, the list is: >> >> pxa_camera >> ti-vpe >> exynos4-is >> qcom >> soc_camera >> atmel >> stm32 >> davinci >> renesas-ceu >> am437x >> xilinx >> rcar_drif >> >> >> Given such a large list of the users of the notifier->subdevs array, >> I think this should be done is two steps: submit this patchset first, >> that keeps the backward compatibility, and then a subsequent patchset >> that converts all drivers to use v4l2_async_notifier_add_subdev(), at >> which point the subdevs array can be removed from struct >> v4l2_async_notifier. >> >> Or, do you still think this should be done all at once? I could add a >> large patch to this patchset that does the conversion and removes >> the subdevs array. > Sorry for the delay. I grepped for this, too, but I guess I ended up using > an expression that missed most of the users. :-( > > I think it'd be very good to perform that conversion --- the code in the > framework would be quite a bit cleaner and easier to maintain whereas the > per-driver conversions seem pretty simple, such as this on in > drivers/media/platform/atmel/atmel-isi.c : > > /* Register the subdevices notifier. */ > subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL); > if (!subdevs) { > of_node_put(isi->entity.node); > return -ENOMEM; > } > > subdevs[0] = &isi->entity.asd; > > isi->notifier.subdevs = subdevs; > isi->notifier.num_subdevs = 1; > isi->notifier.ops = &isi_graph_notify_ops; Yes, the conversions are fairly straightforward. I've completed that work, but it was a very manual conversion, every platform is different and needed careful study. Although I was careful about getting the error-out paths correct, there could be mistakes there, which would result in memory leaks. And obviously I can't re-test all these platforms. So this patch is very high-risk. More eyes are needed on it, ideally the maintainer(s) of each affected platform. I just submitted v4 of this series, but did not include this large un-tested patch in v4 for those reasons. Instead, this patch, and follow-up patches that strips support for subdevs array altogether from v4l2-async.c, and updates rst docs, are available at my media-tree mirror on github: git@github.com:slongerbeam/mediatree.git in the branch 'remove-subdevs-array'. The branch is based off this series (branch 'imx-subdev-notifiers.6'). Steve