| Message ID | 1PjHuckjnFWu-P_WVFjNhBem43Z22IbEVss8hepg7PwpuDCy1ZuXDSQ9Ju3lrMLaOWrjWpnFcGziK2_JaaoOP_H09ER37luwAqnPQqv4VMU=@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Dan, On 26/08/2020 17:05, Dan Scally wrote: > The setupLink function fails (ioctl returns EINVAL) when it passes the MEDIA_LNK_FL_ENABLE flag > to a link that is already flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast > to media-ctl's equivalent media_setup_link() which ORs the new flags with the existing values. > This patch modifies the behaviour of setupLink() to behave the same as media_setup_link() in > media-ctl. > Git commit messages should aim to be wrapped at 72 chars, though it can be longer when it makes sense. (the above goes to 96). We can rewrap when applying though. > ref https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210 > This is missing a Signed-off-by tag from you. Can you confirm you are happy to add the following please? (as per the DCO: https://www.libcamera.org/contributing.html) Signed-off-by: Dan Scally <djrscally@protonmail.com> With that, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Eeep, git-am had issue applying this mail, possibly due to utf formatting or such, but I've fixed up locally. With confirmation from Dan on the SoB, and another RB tag, I'll handle integration of this patch as I have it locally. -- Kieran > Changelog: > > v2 - Simplified by removing the call to link() to fetch a link that's already passed as a parameter to the function. > > --- > src/libcamera/media_device.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > index de18d57..007a45b 100644 > --- a/src/libcamera/media_device.cpp > +++ b/src/libcamera/media_device.cpp > @@ -794,7 +794,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags) > linkDesc.sink.index = sink->index(); > linkDesc.sink.flags = MEDIA_PAD_FL_SINK; > > > - linkDesc.flags = flags; > + linkDesc.flags = flags | (link->flags() & MEDIA_LNK_FL_IMMUTABLE); > > > int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc); > if (ret) { > > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
+libcamera-devel On 27/08/2020 12:11, Dan Scally wrote: > Hi Kieran > >> Git commit messages should aim to be wrapped at 72 chars, though it can >> be longer when it makes sense. (the above goes to 96). >> > >> We can rewrap when applying though. > > Mea culpa - I'll stick to the limit in future. Happy to do a V3 if you prefer, fixing that and the SoB. No worries, I think it's ok. Unless there's any comments from anyone else I have it fixed up locally already. > >> This is missing a Signed-off-by tag from you. >> > >> Can you confirm you are happy to add the following please? >> (as per the DCO: https://www.libcamera.org/contributing.html) >> > >> Signed-off-by: Dan Scally djrscally@protonmail.com > > Yes, absolutely. Sorry - first time patching anything, knew I'd forget something! no problem, Thank you for contributing! I'm still interested to know how you hit this? What platform are you testing on ? -- Kieran > > Thanks > Dan > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Thursday, 27 August 2020 11:42, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > >> Hi Dan, >> > >> On 26/08/2020 17:05, Dan Scally wrote: >> > >>> The setupLink function fails (ioctl returns EINVAL) when it passes the MEDIA_LNK_FL_ENABLE flag >>> to a link that is already flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast >>> to media-ctl's equivalent media_setup_link() which ORs the new flags with the existing values. >>> This patch modifies the behaviour of setupLink() to behave the same as media_setup_link() in >>> media-ctl. >> > >> Git commit messages should aim to be wrapped at 72 chars, though it can >> be longer when it makes sense. (the above goes to 96). >> > >> We can rewrap when applying though. >> > >>> ref https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210 >> > >> This is missing a Signed-off-by tag from you. >> > >> Can you confirm you are happy to add the following please? >> (as per the DCO: https://www.libcamera.org/contributing.html) >> > >> Signed-off-by: Dan Scally djrscally@protonmail.com >> > >> With that, >> > >> Reviewed-by: Kieran Bingham kieran.bingham@ideasonboard.com >> > >> Eeep, git-am had issue applying this mail, possibly due to utf >> formatting or such, but I've fixed up locally. >> > >> With confirmation from Dan on the SoB, and another RB tag, I'll handle >> integration of this patch as I have it locally. >> > >> ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- >> > >> Kieran >> > >>> Changelog: >>> > >>> v2 - Simplified by removing the call to link() to fetch a link that's already passed as a parameter to the function. >>> > >>> > >>> src/libcamera/media_device.cpp | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp >>> index de18d57..007a45b 100644 >>> --- a/src/libcamera/media_device.cpp >>> +++ b/src/libcamera/media_device.cpp >>> @@ -794,7 +794,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags) >>> linkDesc.sink.index = sink->index(); >>> linkDesc.sink.flags = MEDIA_PAD_FL_SINK; >>> > >>> - linkDesc.flags = flags; >>> > >>> - linkDesc.flags = flags | (link->flags() & MEDIA_LNK_FL_IMMUTABLE); >>> int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc); >>> if (ret) { >>> > >>> > >>> libcamera-devel mailing list >>> libcamera-devel@lists.libcamera.org >>> https://lists.libcamera.org/listinfo/libcamera-devel >> > >> -- >> > >> Regards >> > >> -------- >> > >> Kieran >
> I'm still interested to know how you hit this? > What platform are you testing on ? x86, specifically a Lenovo Miix-510. I'm working with the surface-linux guys trying to get webcams working on Microsoft Surface and similar devices. We're either patching DSDT or using a bridge driver to connect the sensors to the cio2 infrastructure, but trying to take images using either cam or qcam threw the error that this clears (though media-ctl got past it). With this patch in, my webcam's functional. ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 27 August 2020 12:48, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > +libcamera-devel > > On 27/08/2020 12:11, Dan Scally wrote: > > > Hi Kieran > > > > > Git commit messages should aim to be wrapped at 72 chars, though it can > > > be longer when it makes sense. (the above goes to 96). > > > > > We can rewrap when applying though. > > > > Mea culpa - I'll stick to the limit in future. Happy to do a V3 if you prefer, fixing that and the SoB. > > No worries, I think it's ok. > > Unless there's any comments from anyone else I have it fixed up locally > already. > > > > This is missing a Signed-off-by tag from you. > > > > > Can you confirm you are happy to add the following please? > > > (as per the DCO: https://www.libcamera.org/contributing.html) > > > > > Signed-off-by: Dan Scally djrscally@protonmail.com > > > > Yes, absolutely. Sorry - first time patching anything, knew I'd forget something! > > no problem, Thank you for contributing! > > I'm still interested to know how you hit this? > What platform are you testing on ? > > ----------------------------------------------------------------------------------------------------------------------------- > > Kieran > > > Thanks > > Dan > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > > On Thursday, 27 August 2020 11:42, Kieran Bingham kieran.bingham@ideasonboard.com wrote: > > > > > Hi Dan, > > > > > On 26/08/2020 17:05, Dan Scally wrote: > > > > > > The setupLink function fails (ioctl returns EINVAL) when it passes the MEDIA_LNK_FL_ENABLE flag > > > > to a link that is already flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast > > > > to media-ctl's equivalent media_setup_link() which ORs the new flags with the existing values. > > > > This patch modifies the behaviour of setupLink() to behave the same as media_setup_link() in > > > > media-ctl. > > > > > Git commit messages should aim to be wrapped at 72 chars, though it can > > > be longer when it makes sense. (the above goes to 96). > > > > > We can rewrap when applying though. > > > > > > ref https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210 > > > > > This is missing a Signed-off-by tag from you. > > > > > Can you confirm you are happy to add the following please? > > > (as per the DCO: https://www.libcamera.org/contributing.html) > > > > > Signed-off-by: Dan Scally djrscally@protonmail.com > > > > > With that, > > > > > Reviewed-by: Kieran Bingham kieran.bingham@ideasonboard.com > > > > > Eeep, git-am had issue applying this mail, possibly due to utf > > > formatting or such, but I've fixed up locally. > > > > > With confirmation from Dan on the SoB, and another RB tag, I'll handle > > > integration of this patch as I have it locally. > > > > > > > > > > Kieran > > > > > > Changelog: > > > > > > v2 - Simplified by removing the call to link() to fetch a link that's already passed as a parameter to the function. > > > > > > > > > > > > > > > > src/libcamera/media_device.cpp | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > > > > index de18d57..007a45b 100644 > > > > --- a/src/libcamera/media_device.cpp > > > > +++ b/src/libcamera/media_device.cpp > > > > @@ -794,7 +794,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags) > > > > linkDesc.sink.index = sink->index(); > > > > linkDesc.sink.flags = MEDIA_PAD_FL_SINK; > > > > > > - linkDesc.flags = flags; > > > > > > - linkDesc.flags = flags | (link->flags() & MEDIA_LNK_FL_IMMUTABLE); > > > > int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc); > > > > if (ret) { > > > > > > > > > > > > > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > -- > > > > > Regards > > > > > > > > > > Kieran > > -- > > Regards > > -------- > > Kieran
Hi Dan, On 27/08/2020 12:54, Dan Scally wrote: >> I'm still interested to know how you hit this? What platform are >> you testing on ? > > x86, specifically a Lenovo Miix-510. I'm working with the > surface-linux guys trying to get webcams working on Microsoft Surface > and similar devices. We're either patching DSDT or using a bridge > driver to connect the sensors to the cio2 infrastructure, but trying > to take images using either cam or qcam threw the error that this > clears (though media-ctl got past it). > > With this patch in, my webcam's functional. Oh! That's fantastic news, Are you able to share some of the information on the work you guys have done with the DSDT patching or bridge drivers? We've had plenty of people asking about how to make use of the IPU3 on platforms we can't otherwise support because of that. There's quite a few Dell laptops that also use it from what I recall. -- Kieran > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 27 August 2020 12:48, > Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > >> +libcamera-devel >> > >> On 27/08/2020 12:11, Dan Scally wrote: >> > >>> Hi Kieran >>> > >>>> Git commit messages should aim to be wrapped at 72 chars, >>>> though it can be longer when it makes sense. (the above goes to >>>> 96). >>> > >>>> We can rewrap when applying though. >>> > >>> Mea culpa - I'll stick to the limit in future. Happy to do a V3 >>> if you prefer, fixing that and the SoB. >> > >> No worries, I think it's ok. >> > >> Unless there's any comments from anyone else I have it fixed up >> locally already. >> > >>>> This is missing a Signed-off-by tag from you. >>> > >>>> Can you confirm you are happy to add the following please? (as >>>> per the DCO: https://www.libcamera.org/contributing.html) >>> > >>>> Signed-off-by: Dan Scally djrscally@protonmail.com >>> > >>> Yes, absolutely. Sorry - first time patching anything, knew I'd >>> forget something! >> > >> no problem, Thank you for contributing! >> > >> I'm still interested to know how you hit this? What platform are >> you testing on ? >> > >> ----------------------------------------------------------------------------------------------------------------------------- >> > >> Kieran >> > >>> Thanks Dan ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 27 >>> August 2020 11:42, Kieran Bingham kieran.bingham@ideasonboard.com >>> wrote: >>> > >>>> Hi Dan, >>> > >>>> On 26/08/2020 17:05, Dan Scally wrote: >>> > >>>>> The setupLink function fails (ioctl returns EINVAL) when it >>>>> passes the MEDIA_LNK_FL_ENABLE flag to a link that is already >>>>> flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. >>>>> Contrast to media-ctl's equivalent media_setup_link() which >>>>> ORs the new flags with the existing values. This patch >>>>> modifies the behaviour of setupLink() to behave the same as >>>>> media_setup_link() in media-ctl. >>> > >>>> Git commit messages should aim to be wrapped at 72 chars, >>>> though it can be longer when it makes sense. (the above goes to >>>> 96). >>> > >>>> We can rewrap when applying though. >>> > >>>>> ref >>>>> https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210 >>> > >>>>> >>>> This is missing a Signed-off-by tag from you. >>> > >>>> Can you confirm you are happy to add the following please? (as >>>> per the DCO: https://www.libcamera.org/contributing.html) >>> > >>>> Signed-off-by: Dan Scally djrscally@protonmail.com >>> > >>>> With that, >>> > >>>> Reviewed-by: Kieran Bingham kieran.bingham@ideasonboard.com >>> > >>>> Eeep, git-am had issue applying this mail, possibly due to utf >>>> formatting or such, but I've fixed up locally. >>> > >>>> With confirmation from Dan on the SoB, and another RB tag, I'll >>>> handle integration of this patch as I have it locally. >>> > >>>> > >>> > >>>> Kieran >>> > >>>>> Changelog: >>> > >>>>> v2 - Simplified by removing the call to link() to fetch a >>>>> link that's already passed as a parameter to the function. >>>>> > >>> > >>>>> > >>> > >>>>> src/libcamera/media_device.cpp | 2 +- 1 file changed, 1 >>>>> insertion(+), 1 deletion(-) diff --git >>>>> a/src/libcamera/media_device.cpp >>>>> b/src/libcamera/media_device.cpp index de18d57..007a45b >>>>> 100644 --- a/src/libcamera/media_device.cpp +++ >>>>> b/src/libcamera/media_device.cpp @@ -794,7 +794,7 @@ int >>>>> MediaDevice::setupLink(const MediaLink *link, unsigned int >>>>> flags) linkDesc.sink.index = sink->index(); >>>>> linkDesc.sink.flags = MEDIA_PAD_FL_SINK; >>> > >>>>> - linkDesc.flags = flags; >>> > >>>>> - linkDesc.flags = flags | (link->flags() & >>>>> MEDIA_LNK_FL_IMMUTABLE); int ret = ioctl(fd_, >>>>> MEDIA_IOC_SETUP_LINK, &linkDesc); if (ret) { >>>>> > >>> > >>>>> > >>> > >>>>> libcamera-devel mailing list >>>>> libcamera-devel@lists.libcamera.org >>>>> https://lists.libcamera.org/listinfo/libcamera-devel >>> > >>>> -- >>> > >>>> Regards >>> > >>>> > >>> > >>>> Kieran >> > >> -- >> > >> Regards >> > >> -------- >> > >> Kieran >
> Oh! That's fantastic news, > > Are you able to share some of the information on the work you guys have > done with the DSDT patching or bridge drivers? > > We've had plenty of people asking about how to make use of the IPU3 on > platforms we can't otherwise support because of that. There's quite a > few Dell laptops that also use it from what I recall. Sure, although I wouldn't call any of it production-ready yet! The DSDT and bridge driver are separate methods; myself and the one other person to successfully take an image with a Surface webcam are using the bridge driver, and that seems like the best approach. Repository here: https://github.com/jhand2/surface-camera. Patches 1-3 and 5 add all the connection functionality, though note that the surface_camera driver introduced by patch 5 hardcodes the connection values to a specific camera on that chap's device, and additionally it only connects a single device so some tweaking is needed if you want to try it at the moment. I think the author is too busy to work on it at the moment, so I'm currently writing an update to that driver that will connect any number of supported devices by parsing the information we need from the SSDB buffers in the DSDT. DSDT modifications, best example probably this repo: https://github.com/kitakar5525/surface-ipu3-cameras. I tried and failed to get that working, although it definitely worked for him. Main development thread is here if you're interested: https://github.com/linux-surface/linux-surface/issues/91 There's still a decent amount of work to do, but it's looking quite promising. Thanks Dan ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 27 August 2020 12:59, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Hi Dan, > > On 27/08/2020 12:54, Dan Scally wrote: > > > > I'm still interested to know how you hit this? What platform are > > > you testing on ? > > > > x86, specifically a Lenovo Miix-510. I'm working with the > > surface-linux guys trying to get webcams working on Microsoft Surface > > and similar devices. We're either patching DSDT or using a bridge > > driver to connect the sensors to the cio2 infrastructure, but trying > > to take images using either cam or qcam threw the error that this > > clears (though media-ctl got past it). > > With this patch in, my webcam's functional. > > Oh! That's fantastic news, > > Are you able to share some of the information on the work you guys have > done with the DSDT patching or bridge drivers? > > We've had plenty of people asking about how to make use of the IPU3 on > platforms we can't otherwise support because of that. There's quite a > few Dell laptops that also use it from what I recall. > > --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- > > Kieran > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 27 August 2020 12:48, > > Kieran Bingham kieran.bingham@ideasonboard.com wrote: > > > > > +libcamera-devel > > > > > On 27/08/2020 12:11, Dan Scally wrote: > > > > > > Hi Kieran > > > > > > > Git commit messages should aim to be wrapped at 72 chars, > > > > > though it can be longer when it makes sense. (the above goes to > > > > > 96). > > > > > > > We can rewrap when applying though. > > > > > > Mea culpa - I'll stick to the limit in future. Happy to do a V3 > > > > if you prefer, fixing that and the SoB. > > > > > No worries, I think it's ok. > > > > > Unless there's any comments from anyone else I have it fixed up > > > locally already. > > > > > > > This is missing a Signed-off-by tag from you. > > > > > > > Can you confirm you are happy to add the following please? (as > > > > > per the DCO: https://www.libcamera.org/contributing.html) > > > > > > > Signed-off-by: Dan Scally djrscally@protonmail.com > > > > > > Yes, absolutely. Sorry - first time patching anything, knew I'd > > > > forget something! > > > > > no problem, Thank you for contributing! > > > > > I'm still interested to know how you hit this? What platform are > > > you testing on ? > > > > > > > > > > Kieran > > > > > > Thanks Dan ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 27 > > > > August 2020 11:42, Kieran Bingham kieran.bingham@ideasonboard.com > > > > wrote: > > > > > > > Hi Dan, > > > > > > > On 26/08/2020 17:05, Dan Scally wrote: > > > > > > > > The setupLink function fails (ioctl returns EINVAL) when it > > > > > > passes the MEDIA_LNK_FL_ENABLE flag to a link that is already > > > > > > flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. > > > > > > Contrast to media-ctl's equivalent media_setup_link() which > > > > > > ORs the new flags with the existing values. This patch > > > > > > modifies the behaviour of setupLink() to behave the same as > > > > > > media_setup_link() in media-ctl. > > > > > > > Git commit messages should aim to be wrapped at 72 chars, > > > > > though it can be longer when it makes sense. (the above goes to > > > > > 96). > > > > > > > We can rewrap when applying though. > > > > > > > > ref > > > > > > https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210 > > > > > > > > > > > > > > > > > > This is missing a Signed-off-by tag from you. > > > > > > > Can you confirm you are happy to add the following please? (as > > > > > per the DCO: https://www.libcamera.org/contributing.html) > > > > > > > Signed-off-by: Dan Scally djrscally@protonmail.com > > > > > > > With that, > > > > > > > Reviewed-by: Kieran Bingham kieran.bingham@ideasonboard.com > > > > > > > Eeep, git-am had issue applying this mail, possibly due to utf > > > > > formatting or such, but I've fixed up locally. > > > > > > > With confirmation from Dan on the SoB, and another RB tag, I'll > > > > > handle integration of this patch as I have it locally. > > > > > > > > > > > > > > > > > > > > Kieran > > > > > > > > Changelog: > > > > > > > > v2 - Simplified by removing the call to link() to fetch a > > > > > > link that's already passed as a parameter to the function. > > > > > > > > > > > > > > > > > > > > > > > > > > > > src/libcamera/media_device.cpp | 2 +- 1 file changed, 1 > > > > > > insertion(+), 1 deletion(-) diff --git > > > > > > a/src/libcamera/media_device.cpp > > > > > > b/src/libcamera/media_device.cpp index de18d57..007a45b > > > > > > 100644 --- a/src/libcamera/media_device.cpp +++ > > > > > > b/src/libcamera/media_device.cpp @@ -794,7 +794,7 @@ int > > > > > > MediaDevice::setupLink(const MediaLink *link, unsigned int > > > > > > flags) linkDesc.sink.index = sink->index(); > > > > > > linkDesc.sink.flags = MEDIA_PAD_FL_SINK; > > > > > > > > - linkDesc.flags = flags; > > > > > > > > - linkDesc.flags = flags | (link->flags() & > > > > > > MEDIA_LNK_FL_IMMUTABLE); int ret = ioctl(fd_, > > > > > > MEDIA_IOC_SETUP_LINK, &linkDesc); if (ret) { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > libcamera-devel mailing list > > > > > > libcamera-devel@lists.libcamera.org > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > -- > > > > > > > Regards > > > > > > > > > > > > > > > > > > > > Kieran > > > > > -- > > > > > Regards > > > > > > > > > > Kieran > > -- > > Regards > > -------- > > Kieran
Hi Dan, On Thu, Aug 27, 2020 at 12:10:07PM +0000, Dan Scally wrote: > > Oh! That's fantastic news, > > > > Are you able to share some of the information on the work you guys have > > done with the DSDT patching or bridge drivers? > > > > We've had plenty of people asking about how to make use of the IPU3 on > > platforms we can't otherwise support because of that. There's quite a > > few Dell laptops that also use it from what I recall. > > Sure, although I wouldn't call any of it production-ready yet! > > The DSDT and bridge driver are separate methods; myself and the one > other person to successfully take an image with a Surface webcam are > using the bridge driver, and that seems like the best approach. > Repository here: https://github.com/jhand2/surface-camera. Patches 1-3 > and 5 add all the connection functionality, though note that the > surface_camera driver introduced by patch 5 hardcodes the connection > values to a specific camera on that chap's device, and additionally it > only connects a single device so some tweaking is needed if you want > to try it at the moment. I think the author is too busy to work on it > at the moment, so I'm currently writing an update to that driver that > will connect any number of supported devices by parsing the > information we need from the SSDB buffers in the DSDT. > > DSDT modifications, best example probably this repo: > https://github.com/kitakar5525/surface-ipu3-cameras. I tried and > failed to get that working, although it definitely worked for him. > > Main development thread is here if you're interested: > https://github.com/linux-surface/linux-surface/issues/91 > > There's still a decent amount of work to do, but it's looking quite > promising. Let me join Kieran to thank you for your efforts. This is really nice news ! As you may have noticed from the very green images, there's still plenty of work to do on IPU3 support in libcamera. This will open the door to developing libcamera on more IPU3-based platforms, I'm excited about that. > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Thursday, 27 August 2020 12:59, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > On 27/08/2020 12:54, Dan Scally wrote: > > > >>> I'm still interested to know how you hit this? What platform are > >>> you testing on ? > >> > >> x86, specifically a Lenovo Miix-510. I'm working with the > >> surface-linux guys trying to get webcams working on Microsoft Surface > >> and similar devices. We're either patching DSDT or using a bridge > >> driver to connect the sensors to the cio2 infrastructure, but trying > >> to take images using either cam or qcam threw the error that this > >> clears (though media-ctl got past it). > >> With this patch in, my webcam's functional. > > > > Oh! That's fantastic news, > > > > Are you able to share some of the information on the work you guys have > > done with the DSDT patching or bridge drivers? > > > > We've had plenty of people asking about how to make use of the IPU3 on > > platforms we can't otherwise support because of that. There's quite a > > few Dell laptops that also use it from what I recall. > > > >> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 27 August 2020 12:48, > >> Kieran Bingham kieran.bingham@ideasonboard.com wrote: > >> > >>> +libcamera-devel > >> > >>> On 27/08/2020 12:11, Dan Scally wrote: > >> > >>>> Hi Kieran > >> > >>>>> Git commit messages should aim to be wrapped at 72 chars, > >>>>> though it can be longer when it makes sense. (the above goes to > >>>>> 96). > >>>>> > >>>>> We can rewrap when applying though. > >>>> > >>>> Mea culpa - I'll stick to the limit in future. Happy to do a V3 > >>>> if you prefer, fixing that and the SoB. > >>> > >>> No worries, I think it's ok. > >>> > >>> Unless there's any comments from anyone else I have it fixed up > >>> locally already. > >>> > >>>>> This is missing a Signed-off-by tag from you. > >>>>> > >>>>> Can you confirm you are happy to add the following please? (as > >>>>> per the DCO: https://www.libcamera.org/contributing.html) > >>>>> > >>>>> Signed-off-by: Dan Scally djrscally@protonmail.com > >>>> > >>>> Yes, absolutely. Sorry - first time patching anything, knew I'd > >>>> forget something! > >>> > >>> no problem, Thank you for contributing! > >>> > >>> I'm still interested to know how you hit this? What platform are > >>> you testing on ? > >>> > >>>>> On 26/08/2020 17:05, Dan Scally wrote: > >>>>> > >>>>>> The setupLink function fails (ioctl returns EINVAL) when it > >>>>>> passes the MEDIA_LNK_FL_ENABLE flag to a link that is already > >>>>>> flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. > >>>>>> Contrast to media-ctl's equivalent media_setup_link() which > >>>>>> ORs the new flags with the existing values. This patch > >>>>>> modifies the behaviour of setupLink() to behave the same as > >>>>>> media_setup_link() in media-ctl. > >>>>> > >>>>> Git commit messages should aim to be wrapped at 72 chars, > >>>>> though it can be longer when it makes sense. (the above goes to > >>>>> 96). > >>>>> > >>>>> We can rewrap when applying though. > >>>>> > >>>>>> ref > >>>>>> https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210 > >>>>>> > >>>>> > >>>>> This is missing a Signed-off-by tag from you. > >>>>> > >>>>> Can you confirm you are happy to add the following please? (as > >>>>> per the DCO: https://www.libcamera.org/contributing.html) > >>>>> > >>>>> Signed-off-by: Dan Scally djrscally@protonmail.com > >>>>> > >>>>> With that, > >>>>> > >>>>> Reviewed-by: Kieran Bingham kieran.bingham@ideasonboard.com > >>>>> > >>>>> Eeep, git-am had issue applying this mail, possibly due to utf > >>>>> formatting or such, but I've fixed up locally. > >>>>> > >>>>> With confirmation from Dan on the SoB, and another RB tag, I'll > >>>>> handle integration of this patch as I have it locally. > >>>>> > >>>>>> Changelog: > >>>>>> > >>>>>> v2 - Simplified by removing the call to link() to fetch a > >>>>>> link that's already passed as a parameter to the function. > >>>>>> > >>>>>> src/libcamera/media_device.cpp | 2 +- > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > >>>>>> index de18d57..007a45b 100644 > >>>>>> --- a/src/libcamera/media_device.cpp > >>>>>> +++ b/src/libcamera/media_device.cpp > >>>>>> @@ -794,7 +794,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned intflags) > >>>>>> > >>>>>> linkDesc.sink.index = sink->index(); > >>>>>> linkDesc.sink.flags = MEDIA_PAD_FL_SINK; > >>>>>> - linkDesc.flags = flags; > >>>>>> + linkDesc.flags = flags | (link->flags() & MEDIA_LNK_FL_IMMUTABLE); > >>>>>> int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc); > >>>>>> if (ret) { > >>>>>>
Hi Dan, Thank you for the patch. On Wed, Aug 26, 2020 at 04:05:50PM +0000, Dan Scally wrote: > The setupLink function fails (ioctl returns EINVAL) when it passes the MEDIA_LNK_FL_ENABLE flag > to a link that is already flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast > to media-ctl's equivalent media_setup_link() which ORs the new flags with the existing values. > This patch modifies the behaviour of setupLink() to behave the same as media_setup_link() in > media-ctl. > > ref https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210 > > Changelog: > > v2 - Simplified by removing the call to link() to fetch a link that's already passed as a parameter to the function. > > --- > src/libcamera/media_device.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > index de18d57..007a45b 100644 > --- a/src/libcamera/media_device.cpp > +++ b/src/libcamera/media_device.cpp > @@ -794,7 +794,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags) > linkDesc.sink.index = sink->index(); > linkDesc.sink.flags = MEDIA_PAD_FL_SINK; > > > - linkDesc.flags = flags; > + linkDesc.flags = flags | (link->flags() & MEDIA_LNK_FL_IMMUTABLE); I wonder if this shouldn't be done in MediaLink::setEnabled() instead. The function reads as int MediaLink::setEnabled(bool enable) { unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0; int ret = dev_->setupLink(this, flags); if (ret) return ret; flags_ = flags; return 0; } and flags_ will lose MEDIA_LNK_FL_IMMUTABLE when setEnabled() is called. How about - unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0; + unsigned int flags = (flags_ & ~MEDIA_LNK_FL_ENABLED) + | (enable ? MEDIA_LNK_FL_ENABLED : 0); ? On a side note, I recommend using git-send-email to send patches, to avoid them getting mangled by the mail user agent. For a patch as simple as this one Kieran fixed the issues by hand, but for more complex patches that wouldn't be an option. > > > int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc); > if (ret) {
diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp index de18d57..007a45b 100644 --- a/src/libcamera/media_device.cpp +++ b/src/libcamera/media_device.cpp @@ -794,7 +794,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags) linkDesc.sink.index = sink->index(); linkDesc.sink.flags = MEDIA_PAD_FL_SINK; - linkDesc.flags = flags; + linkDesc.flags = flags | (link->flags() & MEDIA_LNK_FL_IMMUTABLE);
The setupLink function fails (ioctl returns EINVAL) when it passes the MEDIA_LNK_FL_ENABLE flag to a link that is already flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to media-ctl's equivalent media_setup_link() which ORs the new flags with the existing values. This patch modifies the behaviour of setupLink() to behave the same as media_setup_link() in media-ctl. ref https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210 Changelog: v2 - Simplified by removing the call to link() to fetch a link that's already passed as a parameter to the function. --- src/libcamera/media_device.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc); if (ret) {