| Message ID | 20210618220430.31457-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
On 18/06/2021 23:04, David Plowman wrote: > Hi again Hello ;-) > Here's a v3 set which addresses Kieran's comments, so thanks for > those! Otherwise everything is functionally the same as before. > > To answer Kieran's other questions: > > * Yes, a monochrome raw sensor is very different to a monochrome > non-raw sensor! No black levels, vignetting to worry about on those, > among other things. Understood. > * On the question of warning about missing AWB results only for > monochrome sensors. Yes, that's a reasonable thought, though at the > moment I don't think the IPA side of the world even knows if a > sensor is monochrome. We'd have to pass that somehow, maybe in the > IPACameraSesnsorInfo? That's perhaps a slightly longer discussion, > I'm not entirely sure I want to open that right now... I don't mind either at the moment. I wasn't sure if it was directly feasible, so it could be something to optionally consider later if it became helpful. The print is still there - it might just then needlessly spam the debug logs for monochrome sensors. But I don't think that will be a big impact currently. > * We need to be very nice to Dave, but then the plan would be to start > upstreaming new drivers, as we've been doing previously. There are a > couple more I'd like to get going too. Some of them would need a > "Y12P" format, which seems to be missing from the kernel, currently. I'll leave Laurent to the decisions there, but as long as we know there's intent to get the support upstream, I think the IPA can get in place earlier. I don't think there's particular risk on this of things changing on the way in, as we use the standard interfaces ... > * The tuning I've given for the OV9281 is for an Innomaker module, or > maybe not, I'm not sure what it is. But it actually makes > surprisingly little difference as there is no colour shading, AWB or > colour matrices to worry about. Well I don't think anyone can complain about less things to do. The ISP gets to take a break ;-) I guess things like lens shading will still apply here and be specific to a module - but that needs to be per-device anyway ... and I think I recall you mentioned that the ALSC has a good go at it anyway. > Thanks > David > > > David Plowman (3): > libcamera: Add support for monochrome sensors > libcamera: ipa: raspberrypi: Demote warnings about lack of AWB results > libcamera: ipa: raspberrypi: Add support for ov9281 sensor > > include/libcamera/internal/bayer_format.h | 3 +- > src/ipa/raspberrypi/cam_helper_mov9281.cpp | 65 +++++++++++++++ > src/ipa/raspberrypi/controller/rpi/agc.cpp | 2 +- > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 +- > src/ipa/raspberrypi/data/meson.build | 1 + > src/ipa/raspberrypi/data/mov9281.json | 92 +++++++++++++++++++++ > src/ipa/raspberrypi/meson.build | 1 + > src/libcamera/bayer_format.cpp | 14 +++- > src/libcamera/camera_sensor.cpp | 3 + > src/libcamera/property_ids.yaml | 4 + > 10 files changed, 183 insertions(+), 6 deletions(-) > create mode 100644 src/ipa/raspberrypi/cam_helper_mov9281.cpp > create mode 100644 src/ipa/raspberrypi/data/mov9281.json >
Hi Kieran On Fri, 18 Jun 2021 at 23:18, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > On 18/06/2021 23:04, David Plowman wrote: > > Hi again > > Hello ;-) > > > Here's a v3 set which addresses Kieran's comments, so thanks for > > those! Otherwise everything is functionally the same as before. > > > > To answer Kieran's other questions: > > > > * Yes, a monochrome raw sensor is very different to a monochrome > > non-raw sensor! No black levels, vignetting to worry about on those, > > among other things. > > Understood. > > > > * On the question of warning about missing AWB results only for > > monochrome sensors. Yes, that's a reasonable thought, though at the > > moment I don't think the IPA side of the world even knows if a > > sensor is monochrome. We'd have to pass that somehow, maybe in the > > IPACameraSesnsorInfo? That's perhaps a slightly longer discussion, > > I'm not entirely sure I want to open that right now... > > I don't mind either at the moment. I wasn't sure if it was directly > feasible, so it could be something to optionally consider later if it > became helpful. The print is still there - it might just then needlessly > spam the debug logs for monochrome sensors. But I don't think that will > be a big impact currently. > > > > > * We need to be very nice to Dave, but then the plan would be to start > > upstreaming new drivers, as we've been doing previously. There are a > > couple more I'd like to get going too. Some of them would need a > > "Y12P" format, which seems to be missing from the kernel, currently. > > I'll leave Laurent to the decisions there, but as long as we know > there's intent to get the support upstream, I think the IPA can get in > place earlier. I don't think there's particular risk on this of things > changing on the way in, as we use the standard interfaces ... You know the way I tend to work is to write patches that should be acceptable to upstream, but rarely have the energy to do battle there. You're aware that Gordon (software boss at Pi Towers) is generally happy to contract out upstreaming work if there are people willing to do it.... The two patches for Y12P and Y14P exist in our rpi-5.10.y tree with docs, so it should just be a case of firing them off. c79333a30676 media: Add a pixel format for MIPI packed 14bit luma only. ec127d471e84 media: Add a pixel format for MIPI packed 12bit luma only. Sensor drivers are a little more complex, but hopefully this OV9281 driver is in reasonable shape for upstream. There are also: OV7251 (VGA mono global shutter) is very similar. I meant to drop my module into the office as another one for David to do a rough tune on, but forgot. OV2311 (1600x1300 mono global shutter) isn't merged to our tree quite yet as it needs some more work. IMX290/IMX327 (1080p 60fps in colour or mono variants) I sorted last week to have all the relevant controls working correctly for libcamera, extending the existing mainline driver. > > * The tuning I've given for the OV9281 is for an Innomaker module, or > > maybe not, I'm not sure what it is. But it actually makes > > surprisingly little difference as there is no colour shading, AWB or > > colour matrices to worry about. Almost all of the ones we have are Innomaker (available via Amazon [1]). I do have 2 Arducam ones [2] [3], and a Vision Components[4] one, but I have all those variants in my big box of sensors. [2] only has a single data lane wired up and currently doesn't work with the driver, but the other 2 work fine. [1] https://www.amazon.co.uk/Raspberry-Industrial-Monochrome-Shutter-1MPixel/dp/B085VQ9BBY [2] Integrated lens https://www.arducam.com/product/arducam-ov9281-mipi-1mp-monochrome-global-shutter-camera-module-raspberry-pi/ [3] M12 lens https://www.arducam.com/product/arducam-ov9281-1mp-mono-global-shutter-mipi-camera-with-130degree-850nm-only-m12-mount-for-raspberry-pi/ [4] https://www.vision-components.com/en/products/oem-embedded-vision-systems/mipi-camera-modules/ > Well I don't think anyone can complain about less things to do. The ISP > gets to take a break ;-) > > I guess things like lens shading will still apply here and be specific > to a module - but that needs to be per-device anyway ... and I think I > recall you mentioned that the ALSC has a good go at it anyway. Lens shading will apply here as most of these OV9281 modules have M12 mounts so could have any lens. I have a selection of M12 lenses from a 1.7mm fisheye up to I think it's a 25mm. At least there isn't colour variation due to the lens, just overall vignetting. Dave > > Thanks > > David > > > > > > David Plowman (3): > > libcamera: Add support for monochrome sensors > > libcamera: ipa: raspberrypi: Demote warnings about lack of AWB results > > libcamera: ipa: raspberrypi: Add support for ov9281 sensor > > > > include/libcamera/internal/bayer_format.h | 3 +- > > src/ipa/raspberrypi/cam_helper_mov9281.cpp | 65 +++++++++++++++ > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 2 +- > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 +- > > src/ipa/raspberrypi/data/meson.build | 1 + > > src/ipa/raspberrypi/data/mov9281.json | 92 +++++++++++++++++++++ > > src/ipa/raspberrypi/meson.build | 1 + > > src/libcamera/bayer_format.cpp | 14 +++- > > src/libcamera/camera_sensor.cpp | 3 + > > src/libcamera/property_ids.yaml | 4 + > > 10 files changed, 183 insertions(+), 6 deletions(-) > > create mode 100644 src/ipa/raspberrypi/cam_helper_mov9281.cpp > > create mode 100644 src/ipa/raspberrypi/data/mov9281.json > > > > -- > Regards > -- > Kieran