Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding new image sensor support (IMX585/IMX283/IMX294) #118

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

will127534
Copy link

Hi everyone,

I want to add the following sensor support for:

  1. IMX283
    With driver from https://github.com/will127534/imx283-v4l2-driver + Tuning files
  2. IMX585
    With driver from https://github.com/will127534/imx585-v4l2-driver + Tuning files
  3. IMX294
    With driver from https://github.com/will127534/imx294-v4l2-driver, but the tuning files haven't collected yet.

I'm not sure what is the requirements for adding new sensor support, so please let me know anything else I need to merge this pull request.

naushir and others added 15 commits February 15, 2024 12:39
Add support for 16-bps (48-bpp) RGB output formats. These new formats
are define for the RGB and BGR ordering.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Add the Raspberry Pi 5 PiSP specific verification format:
- V4L2_PIX_FMT_RPI_BE

Add the Raspberry Pi 5 PiSP specific compressed Bayer format types 1/2:
- V4L2_PIX_FMT_PISP_COMP1_xxx
- V4L2_PIX_FMT_PISP_COMP2_xxx

Add the Raspberry Pi 5 PiSP Frontend and Backend config formats:
- V4L2_META_FMT_RPI_FE_CFG
- V4L2_META_FMT_RPI_BE_CFG

Add the Raspberry Pi 5 PiSP Frontend statistics format:
- V4L2_META_FMT_RPI_FE_STATS

Add 16-bit Bayer formats:
- MEDIA_BUS_FMT_Sxxx16_1X16

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Add support for the following HDR modes in the Raspberry Pi IPA:
- Night mode
- Single exposure mode
- Multi-exposure (merged and unmerged)

The algorithm is updated to expect the HDR short channel to meter
explicitly for highlights. This means that it will not in general
under-expose the short channel more than is actually necessary.

When images don't have much saturation, it's good to detect this so
that some of the boost we want to apply to the dark areas can be
implemented as regular gain. This means we can then adjust the tone
curve less, leading to less flat looking images.

The impact on the HDR algorithm is then that this determines how we
build tonemaps dynamically. The highlights are more-or-less correct
now, so we have to build a power-type curve that gives us the
appropriately configured targets in the lower part of the histogram.

We allow the tuning file to supply the maximum spatial gain value,
rather than the whole curve (though it can supply this if it
wants). Some parameter defaults are tweaked to be generally better
across the range of our cameras.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
The following member variable will be used by the derived IPAs in an
upcoming commit, so make them protected:

lensPresent_
monoSensor_

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Add a new subpoject wrap file for the libpisp library located at
https://github.com/raspberrypi/libpisp

The libpisp library is used to configure the Raspberry Pi 5 Frontend
and Backend ISP components.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Add the Raspberry Pi 5 ISP (PiSP) pipeline handler to libcamera. To
include this pipeline handler in the build, set the following meson
option:

meson configure -Dpipelines=rpi/pisp

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Add the Raspberry Pi 5 ISP (PiSP) IPA to libcamera. To include this IPA
in the build, set the following meson option:

meson configure -Dipas=rpi/pisp

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
The old ctt.py and alsc_only.py scripts are removed.

Instead of ctt.py use ctt_vc4.py or ctt_pisp.py, depending on your
target platform.

Instead of alsc_only.py use alsc_vc4.py or alsc_pisp.py, again
according to your platform.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Added the ability to tune the chromatic aberration correction
within the ctt. There are options for cac_only or to tune as part
of a larger tuning process. CTT will now recognise any files that
begin with "cac" as being chromatic aberration tuning files.

Signed-off-by: Ben Benson <ben.benson@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Changed how users select which platform to tune for. Now users
specify a command line argument, '-t', to specify which target
platform.

Signed-off-by: Ben Benson <ben.benson@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
The various boilerplate parts of the tuning file are extended to
include the necessary extra bits for HDR, specifically:

* rpi.denoise has different configurations for HDR modes
* rpi.agc now has extra channels for HDR
* rpi.hdr parameters are added.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
This change adds functionality to the convert_tuning.py script to
convert between vc4 and pisp target tuning files. The conversion is
done on a best effort basis, and should provide functional tuning files.
However, a full tuning for the target platform is always preferred.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
The IMX708 sensor driver advertises its module variants (narrow/wide angle lens,
IR block/pass) by modifying the media entity name string. So add duplicate
entries for each variant.

Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Look for the RAW mandatory stream flag in the pipeline handler config
file. If this flag is set, it guarantees that the application will
provide buffers for Unicam Image, so override the minUnicamBuffers and
minTotalUnicamBuffers config parameters in the following way:

- If startup drop frames are required, allocate at least 1 internal buffer.
- If no startup drop frames are required, do not allocate any internal buffers.

Look for the Output 0 mandatory stream flag in in the pipeline handler
config file. If this flag is set, it guarantees that the application
will provide buffers for the ISP, do not allocate any internal buffers
for the device.

Add a new rpi_apps.yaml pipeline handler config file that enables both
these flags.  To use the file, set the following env variable for a
custom build:

export LIBCAMERA_RPI_CONFIG_FILE=/usr/local/share/libcamera/pipeline/rpi/vc4/rpi_apps.yaml

or for a packaged install:

export LIBCAMERA_RPI_CONFIG_FILE=/usr/share/libcamera/pipeline/rpi/vc4/rpi_apps.yaml

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
@kbingham
Copy link
Collaborator

kbingham commented Mar 4, 2024

Could you squash the fixes and split this into one commit per sensor please? Then I think it would be far closer to being able to merge directly to mainline libcamera.

@6by9
Copy link
Collaborator

6by9 commented Mar 4, 2024

Could you squash the fixes and split this into one commit per sensor please? Then I think it would be far closer to being able to merge directly to mainline libcamera.

Have the drivers for these sensors been submitted to linux-media then? I don't recall seeing them.
Or has that requirement been dropped from libcamera?

@kbingham
Copy link
Collaborator

kbingham commented Mar 4, 2024

It's still a requirement.

IMX283 has already been posted [0], and splitting this would let us merge the IMX283 part already upstream in libcamera, while keeping this together keeps it stalled or lost.

[0] https://lore.kernel.org/linux-media/20240215204436.9194-1-umang.jain@ideasonboard.com/

I hope IMX585 and IMX294 would also get posted upstream, but those are not sensors Ideas on Board are presently working on, so someone needs to take actions there.


unsigned int CamHelperImx294::hideFramesModeSwitch() const
{
/* After a mode switch, we seem to get 1 bad frame. */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you done anything to verify the numbers in the helpers on imx585 and imx294?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did verify the numbers for imx585 but not on imx294 yet...

@will127534
Copy link
Author

Could you squash the fixes and split this into one commit per sensor please? Then I think it would be far closer to being able to merge directly to mainline libcamera.

Done.

@kbingham
Copy link
Collaborator

kbingham commented Mar 5, 2024

Could you make it one commit per sensor please? - sorry - I meant as in a single commit for IMX283, a single commit for IMX585 and a single commit for IMX294. They can all be on the same branch - just distinct commits for each sensor topic.

@will127534
Copy link
Author

I see, sounds good!
Just separated the commits for IMX283 and IMX585, let me drop the IMX294 since it is probably not polished yet.

Thanks!

@naushir
Copy link
Collaborator

naushir commented Mar 6, 2024

I think we should send this directly to the libcamera mailing list when ready. Once it's merged upstream, I'll pull the changes down to our tree ready for the next release.

@naushir naushir closed this Mar 6, 2024
@naushir naushir reopened this Mar 6, 2024
@naushir
Copy link
Collaborator

naushir commented Mar 6, 2024

Feel free to use this space to "pre-review" the changes in the meantime.

@kbingham
Copy link
Collaborator

kbingham commented Mar 9, 2024

@will127534 , thanks for splitting. Next steps to getting upstream are updating the commit message. The most important part is adding your signed-off-by tag to your commits. We can't take anything without that.

Then a nice to have is to write a (small/brief) bit in the commit message about what the tuning file has been generated from (what lights, scene, anything relevant but it's mostly just for reference to help know what the tuning file contains as the numbers are otherwise "raw" to interpret.)

With that, as @6by9 said, we can post for review+merge with drivers posted to Linux media, and imx283 qualifies already though we'll have to figure a plan for the imx585.

With a tag I can handle some clean up and posting to the list if you prefer, but only when your tag is added, or feel free to take the next step to posting to libcamera-devel too yourself.

@kbingham
Copy link
Collaborator

Hi @will127534. Would you be able to add your Signed-off-by: tag to the commits please? Then I'll be able to assist more in upstreaming your IMX283 helpers.

@will127534 will127534 force-pushed the master branch 3 times, most recently from b1202ed to 2c775f6 Compare March 19, 2024 05:47
Signed-off-by: Will Whang <will@willwhang.com>
Signed-off-by: Will Whang <will@willwhang.com>
@will127534
Copy link
Author

Hi @kbingham, I've updated the commits.

Thanks!

@naushir naushir force-pushed the main branch 2 times, most recently from ebda3b7 to eb00c13 Compare April 18, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants