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 45F70637; Mon, 23 May 2022 13:27:29 +0000 (UTC) Received: from pendragon.ideasonboard.com (unknown [141.113.67.45]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 9F9404A8; Mon, 23 May 2022 15:27:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1653312446; bh=le3nMsKm2R0gz2BHZLkV9dWl4Icu/CxHXa+fx5+7YwY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=FoRrlZb7UqkeXcJqjii8Y8CtSMyHK3GkQ6CI8Psr8mDyBX6zt/Zo/oLhc0zGRDxIZ lE3x9FaFJ0ggZsHT2KJpws0uHE5hKpseYKuIDzjSIW7Hom7718Ueg289VRKbAN8/lw uPVk8fr0s3WZVxy/ddlENjnjXypFIP76EQ2WtcgM= Date: Mon, 23 May 2022 16:27:22 +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 Mon, May 23, 2022 at 02:51:55PM +0200, Paul Kocialkowski wrote: > On Sun 22 May 22, 20:34, Laurent Pinchart wrote: > > 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 ? > > Currently the parameters are not mandatory (there is a default state set > by the driver) and queued parameter buffers are applied in the order they > are submitted. > > The request API would make per-frame control possible, but I don't think > there is a point in making it mandatory. It seems that the situation is very > similar to what already exists with the rkisp1 driver. You mentioned that the parameter buffers contain buffer addresses, is that the DMA address of the image buffers (input and output) ? If so, how does that work, does the kernel patch the parameters buffer provided by userspace to fill the DMA addresses in placeholders ? > > > > > 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 99A45C433F5 for ; Mon, 23 May 2022 13:29:05 +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=Bl8exhDZDt4P3MkdlEKdSUh7RhvnZGyDGGodrdQBAzY=; b=jf79X3AurMFFeA 4wxxJ0SF0VTPGH0u9k1WKEFAAewVfHeI0vwE1cO7WLDiJSuATjq3y1KDMgTdjhIz16bVZx6jcZ2Cx jhU4kxzk0GuxfmhWQ9uaL7HXegAc/sQMPdHzENitSV9s1G5NQVez0+1kPxkxGNPqh/mZobcGYfi4Z UDnpam0+03fqQK0Ld9aa1cPNECgrXqXCrZOZ4s5ftxH8UgCd+DmN0hTkDhcqk+CGQkT8NNJm8aOEw pECUKTCvN6fR/0WK5zFqBSXIjBkcsYbekDBwcIRdiFMNgZG8hwJjQ5D9g1s/u6lvDt1hf5bM5iVLi WF9y5wmiI9qm7P6gg7ng==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nt86E-004Oe1-H0; Mon, 23 May 2022 13:27:54 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nt860-004OWc-Qp for linux-arm-kernel@lists.infradead.org; Mon, 23 May 2022 13:27:42 +0000 Received: from pendragon.ideasonboard.com (unknown [141.113.67.45]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 9F9404A8; Mon, 23 May 2022 15:27:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1653312446; bh=le3nMsKm2R0gz2BHZLkV9dWl4Icu/CxHXa+fx5+7YwY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=FoRrlZb7UqkeXcJqjii8Y8CtSMyHK3GkQ6CI8Psr8mDyBX6zt/Zo/oLhc0zGRDxIZ lE3x9FaFJ0ggZsHT2KJpws0uHE5hKpseYKuIDzjSIW7Hom7718Ueg289VRKbAN8/lw uPVk8fr0s3WZVxy/ddlENjnjXypFIP76EQ2WtcgM= Date: Mon, 23 May 2022 16:27:22 +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-20220523_062741_031775_19D35491 X-CRM114-Status: GOOD ( 65.77 ) 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 Mon, May 23, 2022 at 02:51:55PM +0200, Paul Kocialkowski wrote: > On Sun 22 May 22, 20:34, Laurent Pinchart wrote: > > 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 ? > > Currently the parameters are not mandatory (there is a default state set > by the driver) and queued parameter buffers are applied in the order they > are submitted. > > The request API would make per-frame control possible, but I don't think > there is a point in making it mandatory. It seems that the situation is very > similar to what already exists with the rkisp1 driver. You mentioned that the parameter buffers contain buffer addresses, is that the DMA address of the image buffers (input and output) ? If so, how does that work, does the kernel patch the parameters buffer provided by userspace to fill the DMA addresses in placeholders ? > > > > > 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