Message ID | d315073f004ce46e0198fd614398e046ffe649e7.1487111824.git.pavel@ucw.cz (mailing list archive) |
---|---|
State | Accepted, archived |
Delegated to: | Sakari Ailus |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1cdllK-0003q2-Ba; Tue, 14 Feb 2017 22:39:54 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.84_2/mailfrontend-7) with esmtp id 1cdllI-0004du-1w; Tue, 14 Feb 2017 23:39:54 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751422AbdBNWiw (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 14 Feb 2017 17:38:52 -0500 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:44784 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750810AbdBNWiv (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 14 Feb 2017 17:38:51 -0500 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 512) id B8F1981860; Tue, 14 Feb 2017 23:38:49 +0100 (CET) Date: Tue, 14 Feb 2017 23:38:49 +0100 From: Pavel Machek <pavel@ucw.cz> To: sakari.ailus@iki.fi Cc: sre@kernel.org, pali.rohar@gmail.com, pavel@ucw.cz, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, laurent.pinchart@ideasonboard.com, mchehab@kernel.org, ivo.g.dimitrov.75@gmail.com Subject: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times Message-ID: <d315073f004ce46e0198fd614398e046ffe649e7.1487111824.git.pavel@ucw.cz> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="u3/rZRmxL6MmkK24" Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2017.2.14.223316 X-PMX-Spam: Gauge=IIIIIIIII, Probability=9%, Report=' BODY_PARA_IS_SENTENCE_URL 0.1, MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, KNOWN_FREEWEB_URI 0.05, MSGID_ADDED_BY_MTA 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1500_1599 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, LEGITIMATE_SIGNS 0, MULTIPLE_REAL_RCPTS 0, NO_URI_HTTPS 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __ATTACHMENT_SIZE_0_10K 0, __CD 0, __CP_URI_IN_BODY 0, __CT 0, __CTYPE_HAS_BOUNDARY 0, __CTYPE_MULTIPART 0, __FRAUD_BODY_WEBMAIL 0, __FRAUD_WEBMAIL 0, __FROM_DOMAIN_IN_ANY_CC2 0, __FROM_DOMAIN_IN_RCPT 0, __HAS_ATTACHMENT 0, __HAS_ATTACHMENT1 0, __HAS_ATTACHMENT2 0, __HAS_CC_HDR 0, __HAS_FROM 0, __HAS_LIST_ID 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __KNOWN_FREEWEB_URI2 0, __MIME_TEXT_P 0, __MIME_TEXT_P1 0, __MIME_TEXT_P2 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __MULTIPLE_URI_TEXT 0, __NO_HTML_TAG_RAW 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_IN_BODY 0, __URI_NS , __URI_WITH_PATH 0, __USER_AGENT 0' |
Commit Message
Pavel Machek
Feb. 14, 2017, 10:38 p.m. UTC
From: Sebastian Reichel <sre@kernel.org> If v4l2_device_register_subdev_nodes() is called multiple times, it is better to return early than corrupt memory. Without this, exposure / gain controls do not work in the camera application on N900. Signed-off-by: Sebastian Reichel <sre@kernel.org> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> Signed-off-by: Pavel Machek <pavel@ucw.cz> --- drivers/media/v4l2-core/v4l2-device.c | 3 +++ 1 file changed, 3 insertions(+)
Comments
Hi! On Tue 2017-02-14 23:38:49, Pavel Machek wrote: > From: Sebastian Reichel <sre@kernel.org> > > If v4l2_device_register_subdev_nodes() is called multiple times, it is > better to return early than corrupt memory. > > Without this, exposure / gain controls do not work in the camera > application on N900. > > Signed-off-by: Sebastian Reichel <sre@kernel.org> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> > Signed-off-by: Pavel Machek <pavel@ucw.cz> Can I get some updates/feedback here? You liked this one and whole series should be ready... Best regards, Pavel
Hi Pavel, On Mon, Feb 20, 2017 at 11:31:14AM +0100, Pavel Machek wrote: > Hi! > > On Tue 2017-02-14 23:38:49, Pavel Machek wrote: > > From: Sebastian Reichel <sre@kernel.org> > > > > If v4l2_device_register_subdev_nodes() is called multiple times, it is > > better to return early than corrupt memory. > > > > Without this, exposure / gain controls do not work in the camera > > application on N900. > > > > Signed-off-by: Sebastian Reichel <sre@kernel.org> > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > Can I get some updates/feedback here? > > You liked this one and whole series should be ready... :-) I was just rebasing the CCP2 support on the fwnode patchset. I'm just pushing the result here: <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2> I've tested ACPI, will test DT soon...
On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> I've tested ACPI, will test DT soon...
DT case works, too (Nokia N9).
On Mon 2017-02-20 15:56:36, Sakari Ailus wrote: > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote: > > I've tested ACPI, will test DT soon... > > DT case works, too (Nokia N9). Hmm. Good to know. Now to figure out how to get N900 case to work... AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes seem to be in, so I'll need to figure out which, and will still need omap3isp modifications... Best regards, Pavel
On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote: > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote: > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote: > > > I've tested ACPI, will test DT soon... > > > > DT case works, too (Nokia N9). > > Hmm. Good to know. Now to figure out how to get N900 case to work... > > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes > seem to be in, so I'll need to figure out which, and will still need > omap3isp modifications... Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices. It's essentially the functionality in the four patches. The data-lane and clock-name properties have been renamed as data-lanes and clock-lanes (i.e. plural) to match the property documentation.
Hi! On Tue 2017-02-21 13:11:04, Sakari Ailus wrote: > On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote: > > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote: > > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote: > > > > I've tested ACPI, will test DT soon... > > > > > > DT case works, too (Nokia N9). > > > > Hmm. Good to know. Now to figure out how to get N900 case to work... > > > > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes > > seem to be in, so I'll need to figure out which, and will still need > > omap3isp modifications... > > Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices. > > It's essentially the functionality in the four patches. The data-lane and > clock-name properties have been renamed as data-lanes and clock-lanes (i.e. > plural) to match the property documentation. Ok, thanks, I got CSI-1 support to compile. I'm now fighting with subdevices support. Camera flash and autofocus coil really should be subdevices of the ISP, right? Do you have any solution for that? [I need it for my userspace to work, and porting the old one looks like lot of fun (tm) :-(]. Best regards, Pavel
On Tue 2017-02-21 13:11:04, Sakari Ailus wrote: > On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote: > > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote: > > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote: > > > > I've tested ACPI, will test DT soon... > > > > > > DT case works, too (Nokia N9). > > > > Hmm. Good to know. Now to figure out how to get N900 case to work... > > > > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes > > seem to be in, so I'll need to figure out which, and will still need > > omap3isp modifications... > > Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices. > > It's essentially the functionality in the four patches. The data-lane and > clock-name properties have been renamed as data-lanes and clock-lanes (i.e. > plural) to match the property documentation. Ok, I got the camera sensor to work. No subdevices support, so I don't have focus (etc) working, but that's a start. I also had to remove video-bus-switch support; but I guess it will be easier to use video-multiplexer patches... I'll have patches over weekend. Best regards, Pavel
On Sat, Feb 25, 2017 at 01:09:18AM +0100, Pavel Machek wrote: > On Tue 2017-02-21 13:11:04, Sakari Ailus wrote: > > On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote: > > > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote: > > > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote: > > > > > I've tested ACPI, will test DT soon... > > > > > > > > DT case works, too (Nokia N9). > > > > > > Hmm. Good to know. Now to figure out how to get N900 case to work... > > > > > > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes > > > seem to be in, so I'll need to figure out which, and will still need > > > omap3isp modifications... > > > > Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices. > > > > It's essentially the functionality in the four patches. The data-lane and > > clock-name properties have been renamed as data-lanes and clock-lanes (i.e. > > plural) to match the property documentation. > > Ok, I got the camera sensor to work. No subdevices support, so I don't > have focus (etc) working, but that's a start. I also had to remove > video-bus-switch support; but I guess it will be easier to use > video-multiplexer patches... > > I'll have patches over weekend. I briefly looked at what's there --- you do miss the video nodes for the non-sensor sub-devices, and they also don't show up in the media graph, right? I guess they don't end up matching in the async list. I think we need to make the non-sensor sub-device support more generic; it's not just the OMAP 3 ISP that needs it. I think we need to document the property for the flash phandle as well; I can write one, or refresh an existing one that I believe already exists. How about calling it either simply "flash" or "camera-flash"? Similarly for lens: "lens" or "camera-lens". I have a vague feeling the "camera-" prefix is somewhat redundant, so I'd just go for "flash" or "lens". At the very least the property names must be generic (not hardware dependent) as this kind of functionality should be present in the framework rather than in individual drivers. That'll be for later though. Making the sub-device bus configuration a pointer should be in a separate patch. It makes sense since the entire configuration is not valid for all sub-devices attached to the ISP anymore. I think it originally was a separate patch, but they probably have been merged at some point. I can't find it right now anyway.
Hi! > > Ok, I got the camera sensor to work. No subdevices support, so I don't > > have focus (etc) working, but that's a start. I also had to remove > > video-bus-switch support; but I guess it will be easier to use > > video-multiplexer patches... > > > > I'll have patches over weekend. > > I briefly looked at what's there --- you do miss the video nodes for the > non-sensor sub-devices, and they also don't show up in the media graph, > right? Yes. > I guess they don't end up matching in the async list. How should they get to the async list? > I think we need to make the non-sensor sub-device support more generic; > it's not just the OMAP 3 ISP that needs it. I think we need to document > the property for the flash phandle as well; I can write one, or refresh > an existing one that I believe already exists. > > How about calling it either simply "flash" or "camera-flash"? Similarly > for lens: "lens" or "camera-lens". I have a vague feeling the "camera-" > prefix is somewhat redundant, so I'd just go for "flash" or "lens". Actually, I'd go for "flash" and "focus-coil". There may be other lens properties, such as zoom, mirror movement, lens identification, ... > At the very least the property names must be generic (not hardware > dependent) as this kind of functionality should be present in the > framework rather than in individual drivers. That'll be for later > though. Agreed, that would be nice. > Making the sub-device bus configuration a pointer should be in a separate > patch. It makes sense since the entire configuration is not valid for all > sub-devices attached to the ISP anymore. I think it originally was a > separate patch, but they probably have been merged at some point. I can't > find it right now anyway. I believe I can find the patch. But I'm not sure if I can port it to the fwnode infrastructure anytime soon... Pavel
Hi! > > Making the sub-device bus configuration a pointer should be in a separate > > patch. It makes sense since the entire configuration is not valid for all > > sub-devices attached to the ISP anymore. I think it originally was a > > separate patch, but they probably have been merged at some point. I can't > > find it right now anyway. > > I believe I can find the patch. But I'm not sure if I can port it to > the fwnode infrastructure anytime soon... Here is the (unported) patch. https://git.kernel.org/cgit/linux/kernel/git/pavel/linux-n900.git/commit/?h=camera-sa-5&id=e705712683b99fec282f87ed3e80bb58c95cf726 Maybe this helps, Pavel
Moi! :-) On Sat, Feb 25, 2017 at 10:53:22PM +0100, Pavel Machek wrote: > Hi! > > > > Ok, I got the camera sensor to work. No subdevices support, so I don't > > > have focus (etc) working, but that's a start. I also had to remove > > > video-bus-switch support; but I guess it will be easier to use > > > video-multiplexer patches... > > > > > > I'll have patches over weekend. > > > > I briefly looked at what's there --- you do miss the video nodes for the > > non-sensor sub-devices, and they also don't show up in the media graph, > > right? > > Yes. > > > I guess they don't end up matching in the async list. > > How should they get to the async list? The patch you referred to does that. The problem is, it does make the bus configuration a pointer as well. There should be two patches. That's not a lot of work to separate them though. But it should be done. > > > I think we need to make the non-sensor sub-device support more generic; > > it's not just the OMAP 3 ISP that needs it. I think we need to document > > the property for the flash phandle as well; I can write one, or refresh > > an existing one that I believe already exists. > > > > How about calling it either simply "flash" or "camera-flash"? Similarly > > for lens: "lens" or "camera-lens". I have a vague feeling the "camera-" > > prefix is somewhat redundant, so I'd just go for "flash" or "lens". > > Actually, I'd go for "flash" and "focus-coil". There may be other > lens properties, such as zoom, mirror movement, lens identification, > ... Good point. Still there may be other ways to move the lens than the voice coil (which sure is cheap), so how about "flash" and "lens-focus"?
Ahoj! :-) > > > > Ok, I got the camera sensor to work. No subdevices support, so I don't > > > > have focus (etc) working, but that's a start. I also had to remove > > > > video-bus-switch support; but I guess it will be easier to use > > > > video-multiplexer patches... > > > > > > > > I'll have patches over weekend. > > > > > > I briefly looked at what's there --- you do miss the video nodes for the > > > non-sensor sub-devices, and they also don't show up in the media graph, > > > right? > > > > Yes. > > > > > I guess they don't end up matching in the async list. > > > > How should they get to the async list? > > The patch you referred to does that. The problem is, it does make the bus > configuration a pointer as well. There should be two patches. That's not a > lot of work to separate them though. But it should be done. Well... This is the line I'm fighting with: + of_parse_phandle(dev->of_node, "ti,camera-flashes", + flash++) If someone told me its fwnode equivalent, I might be able to get it to work. Knowing what group_id is and if I could ignore it would help a bit, too :-). (Also, I'll be glad to test patches :-)) > > > I think we need to make the non-sensor sub-device support more generic; > > > it's not just the OMAP 3 ISP that needs it. I think we need to document > > > the property for the flash phandle as well; I can write one, or refresh > > > an existing one that I believe already exists. > > > > > > How about calling it either simply "flash" or "camera-flash"? Similarly > > > for lens: "lens" or "camera-lens". I have a vague feeling the "camera-" > > > prefix is somewhat redundant, so I'd just go for "flash" or "lens". > > > > Actually, I'd go for "flash" and "focus-coil". There may be other > > lens properties, such as zoom, mirror movement, lens identification, > > ... > > Good point. Still there may be other ways to move the lens than the voice > coil (which sure is cheap), so how about "flash" and "lens-focus"? Sounds good to me. Pavel
Hyvää iltaa! On Sun, Feb 26, 2017 at 09:38:51AM +0100, Pavel Machek wrote: > Ahoj! :-) > > > > > > Ok, I got the camera sensor to work. No subdevices support, so I don't > > > > > have focus (etc) working, but that's a start. I also had to remove > > > > > video-bus-switch support; but I guess it will be easier to use > > > > > video-multiplexer patches... > > > > > > > > > > I'll have patches over weekend. > > > > > > > > I briefly looked at what's there --- you do miss the video nodes for the > > > > non-sensor sub-devices, and they also don't show up in the media graph, > > > > right? > > > > > > Yes. > > > > > > > I guess they don't end up matching in the async list. > > > > > > How should they get to the async list? > > > > The patch you referred to does that. The problem is, it does make the bus > > configuration a pointer as well. There should be two patches. That's not a > > lot of work to separate them though. But it should be done. > > Well... This is the line I'm fighting with: > > + of_parse_phandle(dev->of_node, "ti,camera-flashes", > + flash++) > > If someone told me its fwnode equivalent, I might be able to get it to > work. Knowing what group_id is and if I could ignore it would help a > bit, too :-). Right. ACPI does not have equivalents for OF phandles. That's the background of the problem. The port and endpoint references are a bit special: there'a a device reference and indices of the port and the endpoint nodes. I think you can just use the OF API for the time being until we define how to describe flash devices with ACPI. The difference with ACPI will be visible there almost no matter what do you do there, which is one more reason to have that functionality in the framework (and not drivers).
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c index f364cc1..937c6de 100644 --- a/drivers/media/v4l2-core/v4l2-device.c +++ b/drivers/media/v4l2-core/v4l2-device.c @@ -235,6 +235,9 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev) if (!(sd->flags & V4L2_SUBDEV_FL_HAS_DEVNODE)) continue; + if (sd->devnode) + continue; + vdev = kzalloc(sizeof(*vdev), GFP_KERNEL); if (!vdev) { err = -ENOMEM;