| Message ID | 20251002080952.3077149-1-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2025-10-02 09:09:22) > The PixelArraySize property is incorrectly deduced from > V4L2_SEL_TGT_CROP_BOUNDS instead of V4L2_SEL_TGT_NATIVE_SIZE. This is > sometimes the same value, but for newer sensor drivers > V4L2_SEL_TGT_NATIVE_SIZE is often larger than crop bounds. Most sensor > drivers already support V4L2_SEL_TGT_NATIVE_SIZE and for the ones that > don't, the fallback to the largest supported size is acceptable. I don't see where the fallback happens if V4L2_SEL_TGT_NATIVE_SIZE isn't available on the sensor. Is that target always guaranteed now to be provided ? Or do we mandate it now ? > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > > Hi all, > > On the imx335 we had an issue that we got the following reports: > PixelArrayActiveAreas: [ (36, 50)/2624x1944 ] > PixelArraySize: (2624,1944) > > As you can see, the PixelArraySize doesn't hold the active area which > must be the case according to https://www.libcamera.org/api-html/namespacelibcamera_1_1properties.html#acec5675f79b6c456aca72c7532a263a4 > > The native_size reported by the driver is [ (0, 0)/2696x2044 ] I think we should be reporting this indeed to be correct. > So I think this is the right thing to do :-) > Could you verify that this doesn't negatively impact the ScalerCropMaximum handling ? > Best regards, > Stefan > > > --- > src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +- > src/libcamera/sensor/camera_sensor_raw.cpp | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > index a84f084ceeeb..c0adc393950f 100644 > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > @@ -399,7 +399,7 @@ int CameraSensorLegacy::validateSensorDriver() > * test platforms have been updated. > */ > Rectangle rect; > - int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > + int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_NATIVE_SIZE, &rect); > if (ret) { > /* > * Default the pixel array size to the largest size supported > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > index 759cccafe4a9..a975028a5e70 100644 > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > @@ -420,10 +420,10 @@ std::optional<int> CameraSensorRaw::init() > */ > > Rectangle rect; > - ret = subdev_->getSelection(streams_.image.sink, V4L2_SEL_TGT_CROP_BOUNDS, > + ret = subdev_->getSelection(streams_.image.sink, V4L2_SEL_TGT_NATIVE_SIZE, Both of these remove the crop bounds. > &rect); > if (ret) { > - LOG(CameraSensor, Error) << "No pixel array crop bounds"; > + LOG(CameraSensor, Error) << "No pixel array native size"; > return { ret }; > } > > -- > 2.48.1 >
Quoting Kieran Bingham (2025-10-02 10:36:42) > Quoting Stefan Klug (2025-10-02 09:09:22) > > The PixelArraySize property is incorrectly deduced from > > V4L2_SEL_TGT_CROP_BOUNDS instead of V4L2_SEL_TGT_NATIVE_SIZE. This is > > sometimes the same value, but for newer sensor drivers > > V4L2_SEL_TGT_NATIVE_SIZE is often larger than crop bounds. Most sensor > > drivers already support V4L2_SEL_TGT_NATIVE_SIZE and for the ones that > > don't, the fallback to the largest supported size is acceptable. > > I don't see where the fallback happens if V4L2_SEL_TGT_NATIVE_SIZE isn't > available on the sensor. Is that target always guaranteed now to be > provided ? Or do we mandate it now ? > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > > > Hi all, > > > > On the imx335 we had an issue that we got the following reports: > > PixelArrayActiveAreas: [ (36, 50)/2624x1944 ] > > PixelArraySize: (2624,1944) > > > > As you can see, the PixelArraySize doesn't hold the active area which > > must be the case according to https://www.libcamera.org/api-html/namespacelibcamera_1_1properties.html#acec5675f79b6c456aca72c7532a263a4 > > > > The native_size reported by the driver is [ (0, 0)/2696x2044 ] > > I think we should be reporting this indeed to be correct. > > > So I think this is the right thing to do :-) > > > > Could you verify that this doesn't negatively impact the > ScalerCropMaximum handling ? > > > Best regards, > > Stefan > > > > > > --- > > src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +- > > src/libcamera/sensor/camera_sensor_raw.cpp | 4 ++-- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > > index a84f084ceeeb..c0adc393950f 100644 > > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > > @@ -399,7 +399,7 @@ int CameraSensorLegacy::validateSensorDriver() > > * test platforms have been updated. > > */ > > Rectangle rect; > > - int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > > + int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_NATIVE_SIZE, &rect); > > if (ret) { > > /* > > * Default the pixel array size to the largest size supported > > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > > index 759cccafe4a9..a975028a5e70 100644 > > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > > @@ -420,10 +420,10 @@ std::optional<int> CameraSensorRaw::init() > > */ > > > > Rectangle rect; > > - ret = subdev_->getSelection(streams_.image.sink, V4L2_SEL_TGT_CROP_BOUNDS, > > + ret = subdev_->getSelection(streams_.image.sink, V4L2_SEL_TGT_NATIVE_SIZE, > > Both of these remove the crop bounds. This was supposed to say: Both of these remove the crop bounds. Do we need that one for anything else now ? Should we report each rectangle in the sensor properties ? > > > &rect); > > if (ret) { > > - LOG(CameraSensor, Error) << "No pixel array crop bounds"; > > + LOG(CameraSensor, Error) << "No pixel array native size"; > > return { ret }; > > } > > > > -- > > 2.48.1 > >
Hi Stefan On Thu, Oct 02, 2025 at 10:09:22AM +0200, Stefan Klug wrote: > The PixelArraySize property is incorrectly deduced from > V4L2_SEL_TGT_CROP_BOUNDS instead of V4L2_SEL_TGT_NATIVE_SIZE. This is Is it incorrect for real ? The definition of the PixelArraySize property can be summarized as: "The readable pixel area, including dummy, optically black and active pixels". Now, be particulary careful about this part: The property describes the maximum size of the raw data captured by the camera, which might not correspond to the physical size of the sensor pixel array matrix, as some portions of the physical pixel array matrix are not accessible and cannot be transmitted out. So "full pixel array size" != "readable pixel array size" and the "readable" part contains visible, non-visible and non-valid pixels. The definition the the TGT_CROP_BOUNDS target in Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst reads as: Bounds of the crop rectangle. All valid crop rectangles fit inside the crop bounds rectangle. which to me means that it represent the readable part of the pixel array (which might be smaller than the full pixel array reported by NATIVE as above suggested by the definition of the PixelArraySize property). > sometimes the same value, but for newer sensor drivers > V4L2_SEL_TGT_NATIVE_SIZE is often larger than crop bounds. Most sensor > drivers already support V4L2_SEL_TGT_NATIVE_SIZE and for the ones that > don't, the fallback to the largest supported size is acceptable. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > > Hi all, > > On the imx335 we had an issue that we got the following reports: > PixelArrayActiveAreas: [ (36, 50)/2624x1944 ] > PixelArraySize: (2624,1944) I see the mainline version registering /* IMX335 native and active pixel array size. */ #define IMX335_NATIVE_WIDTH 2616U #define IMX335_NATIVE_HEIGHT 1964U #define IMX335_PIXEL_ARRAY_LEFT 12U #define IMX335_PIXEL_ARRAY_TOP 12U #define IMX335_PIXEL_ARRAY_WIDTH 2592U #define IMX335_PIXEL_ARRAY_HEIGHT 1944U case V4L2_SEL_TGT_NATIVE_SIZE: sel->r.top = 0; sel->r.left = 0; sel->r.width = IMX335_NATIVE_WIDTH; sel->r.height = IMX335_NATIVE_HEIGHT; return 0; case V4L2_SEL_TGT_CROP: case V4L2_SEL_TGT_CROP_DEFAULT: case V4L2_SEL_TGT_CROP_BOUNDS: sel->r.top = IMX335_PIXEL_ARRAY_TOP; sel->r.left = IMX335_PIXEL_ARRAY_LEFT; sel->r.width = IMX335_PIXEL_ARRAY_WIDTH; sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT; So you might be running a different driver version ? Now, I think the problem lies with the fact we don't offset the activeArea_ Rectangle when we use it to register the PixelArrayActiveAreas[0] property. V4L2_SEL_TGT_CROP_DEFAULT is defined as an offset from V4L2_SEL_TGT_NATIVE_SIZE, while we want to offset the active area from the readable bounds. Should we translate the active area area rectangle by V4L2_SEL_TGT_CROP_BOUNDS[x, y] ? I'm just surprised we never noticed as this code has been there since forever. > > As you can see, the PixelArraySize doesn't hold the active area which > must be the case according to https://www.libcamera.org/api-html/namespacelibcamera_1_1properties.html#acec5675f79b6c456aca72c7532a263a4 > > The native_size reported by the driver is [ (0, 0)/2696x2044 ] > > So I think this is the right thing to do :-) > > Best regards, > Stefan > As a note the long awaited series (very close to land) that introduce the RAW camera sensor model in Linux changes this https://patchwork.linuxtv.org/project/linux-media/patch/20250825095107.1332313-40-sakari.ailus@linux.intel.com/ for sensor drivers conforming with the RAW camera sensor model pad/stream 1/0 will represent the pixel array with: format on 1/0: The width and the height fields indicates the full size of the pixel array, including non-visible pixels V4L2_SEL_TGT_CROP_DEFAULT on 1/0: The visible pixel area. This rectangle is relative to the format V4L2_SEL_TGT_CROP on 1/0: The analogue crop so src/libcamera/sensor/camera_sensor_raw.cpp will have to be updated accordingly. > > --- > src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +- > src/libcamera/sensor/camera_sensor_raw.cpp | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > index a84f084ceeeb..c0adc393950f 100644 > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > @@ -399,7 +399,7 @@ int CameraSensorLegacy::validateSensorDriver() > * test platforms have been updated. > */ > Rectangle rect; > - int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > + int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_NATIVE_SIZE, &rect); > if (ret) { > /* > * Default the pixel array size to the largest size supported > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > index 759cccafe4a9..a975028a5e70 100644 > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > @@ -420,10 +420,10 @@ std::optional<int> CameraSensorRaw::init() > */ > > Rectangle rect; > - ret = subdev_->getSelection(streams_.image.sink, V4L2_SEL_TGT_CROP_BOUNDS, > + ret = subdev_->getSelection(streams_.image.sink, V4L2_SEL_TGT_NATIVE_SIZE, > &rect); > if (ret) { > - LOG(CameraSensor, Error) << "No pixel array crop bounds"; > + LOG(CameraSensor, Error) << "No pixel array native size"; > return { ret }; > } > > -- > 2.48.1 >
diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp index a84f084ceeeb..c0adc393950f 100644 --- a/src/libcamera/sensor/camera_sensor_legacy.cpp +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp @@ -399,7 +399,7 @@ int CameraSensorLegacy::validateSensorDriver() * test platforms have been updated. */ Rectangle rect; - int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); + int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_NATIVE_SIZE, &rect); if (ret) { /* * Default the pixel array size to the largest size supported diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp index 759cccafe4a9..a975028a5e70 100644 --- a/src/libcamera/sensor/camera_sensor_raw.cpp +++ b/src/libcamera/sensor/camera_sensor_raw.cpp @@ -420,10 +420,10 @@ std::optional<int> CameraSensorRaw::init() */ Rectangle rect; - ret = subdev_->getSelection(streams_.image.sink, V4L2_SEL_TGT_CROP_BOUNDS, + ret = subdev_->getSelection(streams_.image.sink, V4L2_SEL_TGT_NATIVE_SIZE, &rect); if (ret) { - LOG(CameraSensor, Error) << "No pixel array crop bounds"; + LOG(CameraSensor, Error) << "No pixel array native size"; return { ret }; }
The PixelArraySize property is incorrectly deduced from V4L2_SEL_TGT_CROP_BOUNDS instead of V4L2_SEL_TGT_NATIVE_SIZE. This is sometimes the same value, but for newer sensor drivers V4L2_SEL_TGT_NATIVE_SIZE is often larger than crop bounds. Most sensor drivers already support V4L2_SEL_TGT_NATIVE_SIZE and for the ones that don't, the fallback to the largest supported size is acceptable. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Hi all, On the imx335 we had an issue that we got the following reports: PixelArrayActiveAreas: [ (36, 50)/2624x1944 ] PixelArraySize: (2624,1944) As you can see, the PixelArraySize doesn't hold the active area which must be the case according to https://www.libcamera.org/api-html/namespacelibcamera_1_1properties.html#acec5675f79b6c456aca72c7532a263a4 The native_size reported by the driver is [ (0, 0)/2696x2044 ] So I think this is the right thing to do :-) Best regards, Stefan --- src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +- src/libcamera/sensor/camera_sensor_raw.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)