[libcamera-devel] [PATCH V2] libcamera: MediaDevice: Make MediaDevice::setupLink account for existing link flags

[Laurent Pinchart]

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 at 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 at 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 at 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 at protonmail.com
> >>>>>
> >>>>> With that,
> >>>>>
> >>>>> Reviewed-by: Kieran Bingham kieran.bingham at 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) {
> >>>>>>     

-- 
Regards,

Laurent Pinchart