From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7C67E7C; Sun, 22 May 2022 17:42:32 +0000 (UTC) Received: from pendragon.ideasonboard.com (unknown [86.8.200.222]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 483AE45F; Sun, 22 May 2022 19:34:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1653240852; bh=7MpQyQtc2TVtgxChF+m8fe8kdSmQrvHBDgzor7SU3ZM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=b76UBaw56+3iTwuNMjpS/go4euO+izOU1tmFouq9rysNYQ9Zltpuf0/gJGefURTnc bHx15JyuHsc5CFaBVT7a2qcha1xufhVJFmqnpbxxNOqAAc1veniaDJql3+Czh/HqFs g+ky4DRRl1oPH+McPckf21DmeFO4P8OLNpMZdwI8= Date: Sun, 22 May 2022 20:34:04 +0300 From: Laurent Pinchart To: Paul Kocialkowski Cc: Sakari Ailus , linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev, Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Sakari Ailus , Hans Verkuil , Maxime Ripard , Thomas Petazzoni Subject: Re: [PATCH v3 3/4] staging: media: Add support for the Allwinner A31 ISP Message-ID: References: <20220415153708.637804-1-paul.kocialkowski@bootlin.com> <20220415153708.637804-4-paul.kocialkowski@bootlin.com> Precedence: bulk X-Mailing-List: linux-sunxi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Hi Paul, On Fri, May 20, 2022 at 04:57:35PM +0200, Paul Kocialkowski wrote: > On Fri 29 Apr 22, 00:07, Sakari Ailus wrote: > > On Thu, Apr 28, 2022 at 04:30:11PM +0200, Paul Kocialkowski wrote: > > > Hi Sakari, > > > > > > On Thu 28 Apr 22, 15:14, Sakari Ailus wrote: > > > > Hi Paul, > > > > > > > > Thanks for the set. > > > > > > > > A few comments below. > > > > > > Thanks a lot for your review! > > > > You're welcome! > > > > ... > > > > > > I understand this is an online ISP. How do you schedule the video buffer > > > > queues? Say, what happens if it's time to set up buffers for a frame and > > > > there's a buffer queued in the parameter queue but not in the image data > > > > queue? Or the other way around? > > > > > > The ISP works in a quite atypical way, with a DMA buffer that is used to > > > hold upcoming parameters (including buffer addresses) and a bit in a "direct" > > > register to schedule the update of the parameters at next vsync. > > > > > > The update (setting the bit) is triggered whenever new parameters are > > > submitted via the params video device or whenever there's a capture buffer > > > available in the capture video device. > > > > > > So you don't particularly need to have one parameter buffer matching a capture > > > buffer, the two can be updated independently. Of course, a capture buffer will > > > only be returned after another buffer becomes active. > > > > This also means it's not possible to associate a capture buffer to a > > parameter buffer by other means than timing --- which is unreliable. The > > request API would allow that but it's not free of issues either. > > Yes the request API seems like a good fit for this. Note that the returned > sequence number in dequeued buffers for the capture and meta video devices > should match though, so userspace still has a way to know which captured buffer > used parameters from which meta params buffer. > > > Alternatively, I think in this case you could always require the capture > > buffer and grab a parameter buffer when it's available. As ISPs are > > generally requiring device specific control software, this shouldn't be a > > problem really. > > I think this is pretty much what happens already. > > > I wonder what Laurent thinks. If parameters buffers are optional, I think the request API should be used, otherwise we won't be able to ensure per-frame control. The alternative is to make the parameter buffer mandatory for every frame, even if no parameters have changed. Or maybe that's the case already ? > > > I hope this answers your concern! > > > > > > [...] > > > > > > > > +static int sun6i_isp_tables_setup(struct sun6i_isp_device *isp_dev) > > > > > +{ > > > > > + struct sun6i_isp_tables *tables = &isp_dev->tables; > > > > > + int ret; > > > > > + > > > > > + /* Sizes are hardcoded for now but actually depend on the platform. */ > > > > > > > > Would it be cleaner to have them defined in a platform-specific way, e.g. > > > > in a struct you obtain using device_get_match_data()? > > > > > > Absolutely! I didn't do it at this stage since only one platform is supported > > > but we could just as well introduce a variant structure already for the table > > > sizes. > > > > I think that would be nice already, especially if you know these are going > > to be different. Otherwise macros could be an option. > > Understood! > > > ... > > > > > > > + ret = v4l2_ctrl_handler_init(&v4l2->ctrl_handler, 0); > > > > > > > > I suppose you intend to add controls later on? > > > > > > I might be wrong but I thought this was necessary to expose sensor controls > > > registered by subdevs that end up attached to this v4l2 device. > > > > > > I doubt the drivers itself will expose controls otherwise. > > > > Now that this is an MC-enabled driver, the subdev controls should be > > accessed through the subdev nodes only. Adding them to the video device's > > control handler is quite hackish and not guaranteed to even work (as e.g. > > multiple subdevs can have the same control). > > Yes I was wondering what would happen in that case. I'll drop the ctrls > handling in the next iteration then. > > Paul > > > ... > > > > > > > +{ > > > > > + struct sun6i_isp_device *isp_dev = video_drvdata(file); > > > > > + struct video_device *video_dev = &isp_dev->capture.video_dev; > > > > > + struct mutex *lock = &isp_dev->capture.lock; > > > > > + int ret; > > > > > + > > > > > + if (mutex_lock_interruptible(lock)) > > > > > + return -ERESTARTSYS; > > > > > + > > > > > + ret = v4l2_pipeline_pm_get(&video_dev->entity); > > > > > > > > Do you need this? > > > > > > > > Drivers should primarily depend on runtime PM, this is only needed for > > > > compatibility reasons. Instead I'd like to see sensor drivers being moved > > > > to runtime PM. > > > > > > Yes it's still needed to support sensor drivers that don't use rpm yet. > > > > To that I suggested adding runtime PM support for the affected sensors. > > This doesn't seem to get done otherwise. E.g. ipu3-cio2 driver does not > > call s_power() on sensor subdevs. > > > > ... > > > > > > > + ret = video_register_device(video_dev, VFL_TYPE_VIDEO, -1); > > > > > + if (ret) { > > > > > + v4l2_err(v4l2_dev, "failed to register video device: %d\n", > > > > > + ret); > > > > > + goto error_media_entity; > > > > > + } > > > > > + > > > > > + v4l2_info(v4l2_dev, "device %s registered as %s\n", video_dev->name, > > > > > + video_device_node_name(video_dev)); > > > > > > > > This isn't really driver specific. I'd drop it. > > > > > > I agree but I see that many drivers are doing it and the information can > > > actually be quite useful at times. > > > > You can get that information using media-ctl -e 'entity name'. > > > > I guess this could be also added to video_register_device() on debug level. > > > > > > > +struct sun6i_isp_params_config_bdnf { > > > > > + __u8 in_dis_min; // 8 > > > > > + __u8 in_dis_max; // 10 > > > > > > > > Are these default values or something else? Better documentation was in the > > > > TODO.txt file already. > > > > > > Yes that's the default register values, but these comments are and overlook on > > > my side and should be removed. > > > > I'm fine leaving these here. Just wondering. Up to you. -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 16563C433EF for ; Mon, 23 May 2022 08:52:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=XGjd6gryQb/ewftI3yFl0CiDnVVX7yV2MU83mFZwlJ0=; b=dh75jnJFylZ9e3 bLgKfUfP9s6twmeZGPY9wYWyoeghYqpqYYqW24uwaHUWnOE1KHKbMCDIY8xCvPEVKrRBAjaY2Olig GHGMwdoQzIjgbRuQmnTebnpblEwI9VCQBr9VPgNKeL/M2S3XNOl9H2D1Q5s1qmLLzZoieorcbmvba CK+VXPmeea1SN7lQHFdl3nSyTOv/GctOlZlsc5sSomdTareGGbRjjtaT2KS+EwB3pGBNdT5oNuwrX DJFmEITfg65L4bTR9QkUaBw7YhPyRU0VQ2tFlKIo9WUvJK8Y3n3rgQyPUQpHWrHnwr5wVvWBTHhPS A7rAuHOAINpgpUd2JNrg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nt3ln-002Zsm-6v; Mon, 23 May 2022 08:50:32 +0000 Received: from desiato.infradead.org ([2001:8b0:10b:1:d65d:64ff:fe57:4e05]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nt34y-002H3a-RF for linux-arm-kernel@bombadil.infradead.org; Mon, 23 May 2022 08:06:17 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=desiato.20200630; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=4BPpKcMAOUkYvizc7dEXVxrVTxObLVj92RBzFkeeAGA=; b=LiwgjuPCo5znwzwwrqIaC88OuZ HjuGoEfmL+yis5jYQ1laiIlJh7r3OelLXPVEBY/0sYoJUQkefK6S8IPyqj5XJFHJZWSQ2v+ipValP PvWToWGa0UPp29KTcsRAQiDYbcDQcpdPZ+9VQ5P2SBwraeSQCABU7zNdV/KLSeCK4/W6He4NKz+6Q uS+QW2Ea4zkLSAOz1fb3erbeEm61HX4DnwIhktX55UUOUuakC7/y15hzPNpZ1BsdJLMxUhPwY4IWn AQ1FfhC55OqtfQ0J1uxFVBxXYzLirsJL1eT8g737oNfC0/8p95HHTlbEFKehP4TYTO51UZ9aCfQDY oG7Gg50g==; Received: from perceval.ideasonboard.com ([213.167.242.64]) by desiato.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nspTB-000Yhf-OE for linux-arm-kernel@lists.infradead.org; Sun, 22 May 2022 17:34:26 +0000 Received: from pendragon.ideasonboard.com (unknown [86.8.200.222]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 483AE45F; Sun, 22 May 2022 19:34:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1653240852; bh=7MpQyQtc2TVtgxChF+m8fe8kdSmQrvHBDgzor7SU3ZM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=b76UBaw56+3iTwuNMjpS/go4euO+izOU1tmFouq9rysNYQ9Zltpuf0/gJGefURTnc bHx15JyuHsc5CFaBVT7a2qcha1xufhVJFmqnpbxxNOqAAc1veniaDJql3+Czh/HqFs g+ky4DRRl1oPH+McPckf21DmeFO4P8OLNpMZdwI8= Date: Sun, 22 May 2022 20:34:04 +0300 From: Laurent Pinchart To: Paul Kocialkowski Cc: Sakari Ailus , linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev, Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Sakari Ailus , Hans Verkuil , Maxime Ripard , Thomas Petazzoni Subject: Re: [PATCH v3 3/4] staging: media: Add support for the Allwinner A31 ISP Message-ID: References: <20220415153708.637804-1-paul.kocialkowski@bootlin.com> <20220415153708.637804-4-paul.kocialkowski@bootlin.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220522_183422_162039_4F1A412D X-CRM114-Status: GOOD ( 57.13 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Paul, On Fri, May 20, 2022 at 04:57:35PM +0200, Paul Kocialkowski wrote: > On Fri 29 Apr 22, 00:07, Sakari Ailus wrote: > > On Thu, Apr 28, 2022 at 04:30:11PM +0200, Paul Kocialkowski wrote: > > > Hi Sakari, > > > > > > On Thu 28 Apr 22, 15:14, Sakari Ailus wrote: > > > > Hi Paul, > > > > > > > > Thanks for the set. > > > > > > > > A few comments below. > > > > > > Thanks a lot for your review! > > > > You're welcome! > > > > ... > > > > > > I understand this is an online ISP. How do you schedule the video buffer > > > > queues? Say, what happens if it's time to set up buffers for a frame and > > > > there's a buffer queued in the parameter queue but not in the image data > > > > queue? Or the other way around? > > > > > > The ISP works in a quite atypical way, with a DMA buffer that is used to > > > hold upcoming parameters (including buffer addresses) and a bit in a "direct" > > > register to schedule the update of the parameters at next vsync. > > > > > > The update (setting the bit) is triggered whenever new parameters are > > > submitted via the params video device or whenever there's a capture buffer > > > available in the capture video device. > > > > > > So you don't particularly need to have one parameter buffer matching a capture > > > buffer, the two can be updated independently. Of course, a capture buffer will > > > only be returned after another buffer becomes active. > > > > This also means it's not possible to associate a capture buffer to a > > parameter buffer by other means than timing --- which is unreliable. The > > request API would allow that but it's not free of issues either. > > Yes the request API seems like a good fit for this. Note that the returned > sequence number in dequeued buffers for the capture and meta video devices > should match though, so userspace still has a way to know which captured buffer > used parameters from which meta params buffer. > > > Alternatively, I think in this case you could always require the capture > > buffer and grab a parameter buffer when it's available. As ISPs are > > generally requiring device specific control software, this shouldn't be a > > problem really. > > I think this is pretty much what happens already. > > > I wonder what Laurent thinks. If parameters buffers are optional, I think the request API should be used, otherwise we won't be able to ensure per-frame control. The alternative is to make the parameter buffer mandatory for every frame, even if no parameters have changed. Or maybe that's the case already ? > > > I hope this answers your concern! > > > > > > [...] > > > > > > > > +static int sun6i_isp_tables_setup(struct sun6i_isp_device *isp_dev) > > > > > +{ > > > > > + struct sun6i_isp_tables *tables = &isp_dev->tables; > > > > > + int ret; > > > > > + > > > > > + /* Sizes are hardcoded for now but actually depend on the platform. */ > > > > > > > > Would it be cleaner to have them defined in a platform-specific way, e.g. > > > > in a struct you obtain using device_get_match_data()? > > > > > > Absolutely! I didn't do it at this stage since only one platform is supported > > > but we could just as well introduce a variant structure already for the table > > > sizes. > > > > I think that would be nice already, especially if you know these are going > > to be different. Otherwise macros could be an option. > > Understood! > > > ... > > > > > > > + ret = v4l2_ctrl_handler_init(&v4l2->ctrl_handler, 0); > > > > > > > > I suppose you intend to add controls later on? > > > > > > I might be wrong but I thought this was necessary to expose sensor controls > > > registered by subdevs that end up attached to this v4l2 device. > > > > > > I doubt the drivers itself will expose controls otherwise. > > > > Now that this is an MC-enabled driver, the subdev controls should be > > accessed through the subdev nodes only. Adding them to the video device's > > control handler is quite hackish and not guaranteed to even work (as e.g. > > multiple subdevs can have the same control). > > Yes I was wondering what would happen in that case. I'll drop the ctrls > handling in the next iteration then. > > Paul > > > ... > > > > > > > +{ > > > > > + struct sun6i_isp_device *isp_dev = video_drvdata(file); > > > > > + struct video_device *video_dev = &isp_dev->capture.video_dev; > > > > > + struct mutex *lock = &isp_dev->capture.lock; > > > > > + int ret; > > > > > + > > > > > + if (mutex_lock_interruptible(lock)) > > > > > + return -ERESTARTSYS; > > > > > + > > > > > + ret = v4l2_pipeline_pm_get(&video_dev->entity); > > > > > > > > Do you need this? > > > > > > > > Drivers should primarily depend on runtime PM, this is only needed for > > > > compatibility reasons. Instead I'd like to see sensor drivers being moved > > > > to runtime PM. > > > > > > Yes it's still needed to support sensor drivers that don't use rpm yet. > > > > To that I suggested adding runtime PM support for the affected sensors. > > This doesn't seem to get done otherwise. E.g. ipu3-cio2 driver does not > > call s_power() on sensor subdevs. > > > > ... > > > > > > > + ret = video_register_device(video_dev, VFL_TYPE_VIDEO, -1); > > > > > + if (ret) { > > > > > + v4l2_err(v4l2_dev, "failed to register video device: %d\n", > > > > > + ret); > > > > > + goto error_media_entity; > > > > > + } > > > > > + > > > > > + v4l2_info(v4l2_dev, "device %s registered as %s\n", video_dev->name, > > > > > + video_device_node_name(video_dev)); > > > > > > > > This isn't really driver specific. I'd drop it. > > > > > > I agree but I see that many drivers are doing it and the information can > > > actually be quite useful at times. > > > > You can get that information using media-ctl -e 'entity name'. > > > > I guess this could be also added to video_register_device() on debug level. > > > > > > > +struct sun6i_isp_params_config_bdnf { > > > > > + __u8 in_dis_min; // 8 > > > > > + __u8 in_dis_max; // 10 > > > > > > > > Are these default values or something else? Better documentation was in the > > > > TODO.txt file already. > > > > > > Yes that's the default register values, but these comments are and overlook on > > > my side and should be removed. > > > > I'm fine leaving these here. Just wondering. Up to you. -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel