Hi Laurent On Tue, 4 Jul 2023 at 15:56, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > On Tue, Jul 04, 2023 at 12:03:00PM +0100, Dave Stevenson wrote: > > On Tue, 4 Jul 2023 at 11:24, Jacopo Mondi wrote: > > > On Tue, Jul 04, 2023 at 10:58:11AM +0100, Dave Stevenson wrote: > > > > Hi Jacopo > > > > > > > > Thanks for adding documentation. > > > > Sorry I couldn't be at your presentation, but I'll find the slides > > > > (and recording if there is one). > > > > > > videos and slides should be already available for attendees. If not I > > > can send you the slide deck, but trust me, there is nothing new for > > > you there. > > > > > > > On Mon, 3 Jul 2023 at 21:29, Jacopo Mondi wrote: > > > > > > > > > > Document the requirement of notifying to userspace the possible > > > > > re-ordering of the color sample components when a vertical or horizontal > > > > > flip is applied to a RAW camera sensor. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> > > > > > --- > > > > > Documentation/driver-api/media/camera-sensor.rst | 16 ++++++++++++++++ > > > > > 1 file changed, 16 insertions(+) > > > > > > > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst > > > > > index 93f4f2536c25..ee4a7fe5f72a 100644 > > > > > --- a/Documentation/driver-api/media/camera-sensor.rst > > > > > +++ b/Documentation/driver-api/media/camera-sensor.rst > > > > > @@ -173,3 +173,19 @@ V4L2_CID_VFLIP controls with the values programmed by the register sequences. > > > > > The default values of these controls shall be 0 (disabled). Especially these > > > > > controls shall not be inverted, independently of the sensor's mounting > > > > > rotation. > > > > > + > > > > > +Flip handling for raw camera sensors > > > > > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > Possibly "for colour raw camera sensors". > > > > Mono sensors are still raw in that they need processing for black > > > > level, lens shading, etc, but they won't change the colour ordering. > > > > > > Thanks, good point > > > > > > > > + > > > > > +Applying vertical and horizontal flips on raw camera sensors inverts the color > > > > > +sample reading direction on the sensor's pixel array. This causes the > > > > > +re-ordering of the color samples on the sensor's output frame. > > > > > > > > This *may* cause the re-ordering.... > > > > > > > > Not all sensors do. Some shift the readout by one line/column to keep > > > > the Bayer order the same, and technically should update the selection. > > > > OnSemi sensors in particular seem to do this, as do the Sony > > > > IMX327/290/462 family. > > > > > > Is it the driver doing the shift or is it the sensor automatically > > > adjusting ? > > > > The sensor does it. > > Both cases exist, some drivers do the adjustments internally. That's not > something I would recommend, but it was implemented to allow changing > the flip controls during streaming as we lacked (and still lack) a > solution to this problem. I'll believe you, but had never noticed it in a mainline driver. > There are also sensors that make this behaviour configurable, with a > register bit to indicate if the crop rectangle should be adjusted > automatically. > > While moving the crop rectangle by one pixel in a large resolution > sensor could seem difficult to notice, it can have very noticeable > effects. For instance, defective pixel correction that operates on a > list of known bad pixels will require adjustement. > > > For IMX290, the array is defined horizontally as: > > 4 ignored pixels > > 8 pixels margin for colour processing > > 1920 recording pixel area > > 9 pixels margin for colour processing > > 4 ignored pixels > > (3 dummy pixels) > > Do you know what those dummy pixels are, and how they differ from the > ignored pixels that are not supposed to be read out either ? Going off topic for this documentation thread, but c'est la vie. I'm assuming you have a datasheet for IMX290 or 327 as you've previously worked on the driver. "Drive Timing Chart for Full HD 1080p mode (CSI-2 serial output)" on (for me) page 62 says that there are1948 pixels are being produced per line - 4 side ignored area, 8 margin for colour processing, 1920 active, 9 margin for colour processing, 4 side ignored area, and 3 dummy. The dummy ones are not numbered, so presumably aren't actually read from the pixel array. Vertically it says you're getting lines 3-12 (optical black), and then 13-1109 (8 + 1080 + 9) as active lines. The Drive Timing Chart for the 720p mode (page 76) says that it's sending pixels 321 to 1625 (inclusive) per line as 4 ignored, 8 margin, 1280 active, 9 margin, 4 ignored, and then 3 dummy area samples (again not numbered). Vertically it says you're getting lines 3-6 (optical black), and then 197-925 as active lines. Looking again at that diagram, it lists out exactly which pixels/lines are sent in normal and inverted modes, so the shift-by-1 is explicitly stated. The OB lines always stay at the top of the image. OB data appears to be sent with data type 0x37, so at least for Unicam it won't end up in the image buffer. Further comparison between the datasheet and driver show that for 1080p the datasheet intends X_OUT_SIZE = 0x79c (1948) and Y_OUT_SIZE = 0x449 (1097), and for the 720p mode 0x51c (1308) x 0x2d9 (729). The driver is setting those to 1920x1080 and 1280x720 respectively, therefore the output FOV may well not be that which is expected. I seem to recall making a comment along those lines when I upstreamed the last load of patches for imx290, but have never had the time to fully investigate. Dave > > Ignoring the dummy ones, that gives a width of 1945 pixels. > > > > Vertically is similar with 1109 pixels total (9 colour margin / 1080 > > active / 8 colour margin). > > > > That means it is a red pixel in all 4 corners of the full 1945x1109 > > array, and it keeps the 8 pixels margin on the left of the image read > > out in all circumstances, and 8 at the top too so that readout always > > follows that. > > It's slightly weird as a sensor in that it has presets for 1080p and > > 720p, as well as a windowed mode where you can arbitrarily crop from > > the whole array. The driver only uses the two presets, so exactly what > > is going on is partly reverse engineering. > > > > With regard OnSemi, we asked them about it several years ago, and the > > response was that there was no way to change that behaviour for the > > sensor we were looking at. > > From that datasheet: > > "While the sensor’s format is W × H, the additional active columns and > > active rows are included for use when horizontal or vertical mirrored > > readout is enabled, to allow readout to start on the same pixel. The > > pixel adjustment is always performed for monochrome or color versions" > > Definition of the VERT_FLIP bit > > "1: Readout is flipped (mirrored) vertically so that the row specified > > by y_addr_end_ (+1) is read out of the sensor first" > > and HORIZ_MIRROR bit > > "1: Readout is mirrored horizontally so that the column specified by > > x_addr_end_ (+1) is read out of the sensor first." > > > > Having just checked a different OnSemi datasheet, that doesn't state > > that it alters the pixel read out first when flipped. It possibly > > depends on the target market for the sensor, and how configurable they > > anticipate the ISPs to be. > > > > > > > As an example, a > > > > > +raw camera sensor with a Bayer pattern color filter array and a native RGGB > > > > > +Bayer order will produce frames with GRBG component ordering when a vertical > > > > > +flip is applied. > > > > > > > > Vertical flip of RGGB would be GBRG as the RG and GB get swapped, not > > > > GRBG (which would be horizontal flip). > > > > > > I clearly mean horizontal sorry. I'll fix. > > > > > > > > Camera sensor drivers where inverting the reading order > > > > > +direction causes a re-ordering of the color components are requested to register > > > > > +the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the > > > > > +``V4L2_CTRL_FLAG_MODIFY_LAYOUT`` flag enabled to notify userspace that enabling > > > > > +a flip can potentially change the output buffer content layout. Flips should > > > > > +also be taken into account when enumerating and handling media bus formats > > > > > +on the camera sensor source pads. > > -- > Regards, > > Laurent Pinchart
