Re: RFC: Arducam 64 MP (Hawkeye)

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

On Mon, Sep 26, 2022 at 01:19:47PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-09-23 14:42:52)
> > On Fri, Sep 23, 2022 at 10:03:19AM +0000, Sakari Ailus wrote:
> > > On Fri, Sep 23, 2022 at 10:49:29AM +0100, Kieran Bingham wrote:
> > > > Hi Sakari, Laurent,
> > > > 
> > > > Last night I hooked up the Arducam 64 MP Hawkeye camera [0], to an RPi4,
> > > > and added the camera helper for the RaspberryPi IPA to libcamera, and
> > > > was able to use the camera directly with dtoverlay=arducam_64mp in the
> > > > Raspberry Pi /boot/config.txt using the latest release from Raspberry
> > > > Pi.
> > > > 
> > > > Raspberry Pi have already added the driver for this camera [1] to their
> > > > tree, and I have been given the tuning files from Arducam.
> > > > 
> > > > This has allowed me to capture an image with pretty good response to
> > > > changing lighting conditions, and colours. [2] ... The AF isn't yet
> > > > enabled, so that shot is out of focus a little. (That's for later).
> > > > 
> > > > Arducam do not wish to name the sensor used in the module, and have
> > > > called it the 'arducam_64mp'. But attempting to upstreaming this with
> > > > that name worries me.
> > > > 
> > > > Furthermore, we would like to maintain libcamera as supporting cameras
> > > > that have 'upstream' drivers (or drivers that are on their way
> > > > upstream), so I'm keen to identify how we can upstream the drivers to
> > > > create a better experience for users who are currently finding that they
> > > > need to run a fork of libcamera to operate the module.
> > > > 
> > > > So ultimately - my open question is ... Is it acceptable to have camera
> > > > drivers that are named by their 'module/integration' rather than their
> > > > sensor?
> > > 
> > > A lot of users (especially the existing ones) depend on the entity name
> > > currently. I don't think we could change it. For new ones I guess that
> > > would be possible.
> > > 
> > > Alternatively this could be a string control I think. That could be added
> > > to existing drivers as well.
> > 
> > The question here, as far as I understand, isn't about the entity name
> > exposed to userspace, but about the driver name. The entity name
> > certainly matters too.
> 
> Yes, it's "Can we support a module vendor abstracting an underlying
> device".
> 
> Ordinarily we'd expect this device to be supported through a compatible
> that directly references the component it supports.
> 
> Of course it's not the first time that vendors/ODM/OEM/SVs abstract a
> component to their own brand (There's plenty of in-silicon IP blocks
> with common bases, but separate implementations) but this case is a bit
> different because it's a direct integration of a sensor into a module.
> 
> A quick chat with Sakari tells me that we could potentially support this
> as a CCS/SMIA device. But that will probably just push the problem
> further into libcamera, as to how we directly identify which tuning file
> to use anyway! (So still an important part to consider, as matching on
> the sensor name for tuning data isn't sufficient in the first place).

Absolutely, and that's an issue that has been discussed already. We need
a way to identify camera modules in addition to camera sensors, with the
information provided in DT. Naming the module after Arducam, and
shipping a corresponding tuning file, is totally fine with me.

> > Camera sensor drivers we have in the mainline kernel are named after the
> > camera sensor model, and I think we should continue that. If this were a
> > custom silicon made by Arducam they could name it any way they want, but
> > if it's a sensor from a known sensor manufacturer, I don't think the
> > name should be hidden. I wouldn't be surprised if it would be possible
> > to identify the camera sensor relatively easily from the register set
> > anyway, which would render this whole game pointless.
> 
> We're already seeing Sony IMX drivers with support for more than one
> sensor, and multiple compatible strings. (Bryan O'Donoghue's recent work
> towards supporting IMX412, IMX577, and I think IMX477...)
> 
> I would like to see the sensor drivers becoming more generalised where
> possible, not more specific to individual modules. Of course there are
> differences between the different sensor models - but at the moment
> we're generating lots of drivers with huge distinct tables of register
> data...
> 
> The Sensor itself is common, and likely to be used in other products -
> so masking this as 'arducam_64mp' seems the wrong way forwards to me.
> 
> At some point we're going to want to support a mobile phone camera with
> the same sensor as this Arducam Module (which already exist) - and I
> don't think we want the phone to state it has an arducam module in those
> instances.
> 
> > One of the major advantages of upstreaming driver is the community
> > maintenance that you get from free, including improvements to the driver
> > from other developers who use the same camera sensor in a different
> > product. We don't want to have multiple drivers for the same hardware in
> > the mainline kernel, there are precedents for that due to historical
> > reasons (mostly people not realizing that the same IP core was used in
> > different SoCs), and there are efforts to fix that. I don't see a reason
> > to go the opposite way.
> > 
> > TL;DR: Unless there's a very compeling reason not to follow the usual
> > practice, I don't see why we should make an exception in this case.

-- 
Regards,

Laurent Pinchart