Hi, Sorry for being very slow with replying to this. On 2/17/24 4:38 PM, Kieran Bingham wrote: > Quoting Hans de Goede (2024-02-16 22:32:33) >> There is no reason to send OV2680_END_MARGIN extra columns on top of >> the mode width and the same for sending extra lines over the mode height. >> >> This sending of extra lines/columns was inherited from the atomisp >> ov2680 driver, it is unclear why this was done and this complicates >> adding V4L2_CID_VBLANK support, so remove it. > > Was this some niave way of adding some HBLANK to let the AtomISP keep up > with processing each line? The total amount of pixels per line and of lines per frame are fixed: #define OV2680_PIXELS_PER_LINE 1704 #define OV2680_LINES_PER_FRAME 1294 This patch only changes the h_end and v_end registers which before AFAIK configure when to stop sending actual pixel data (and move over to sending blanking data). So this was actually requesting for more pixels to be send then there are. > Or is it an optical black region? or padding? (I hit issues around that > on the IMX283 lately). AFAICT (from the datasheet) there is no optical black region, so this really just seemed some weirdness in the original Android kernel driver where this is derived from. The datasheet says: "2.4 pixel array addresses The addressable pixel array of the OV2680 sensor is 1616 x 1216. The addressed region of the pixel array is controlled by the horizontal_start, vertical_start, horizontal_end and vertical_end registers. The start and end addresses are limited to even and odd numbers, respectively, to ensure that there is always an even number of pixels read out in x and y." > Is this a sensor you have and can visually check? Yes this is a sensor which I have, not sure what you mean with visually check. The atomisp driver does not allow getting the full resolution as the ISP needs some padding. So the max I can get is 1600x1200. I think the original Android code just added the 16 extra pixels needed by the ISP to h_end and v_end twice. Starting with the extra 16 pixels which are actually there on the sensor and then adding an extra 16 which are fully made up by the driver author and somehow this still works (I guess the sensor just sends all 0 data for these). > >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> --- >> drivers/media/i2c/ov2680.c | 9 ++------- >> 1 file changed, 2 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c >> index 39d321e2b7f9..5b04c6c0554a 100644 >> --- a/drivers/media/i2c/ov2680.c >> +++ b/drivers/media/i2c/ov2680.c >> @@ -86,9 +86,6 @@ >> #define OV2680_PIXELS_PER_LINE 1704 >> #define OV2680_LINES_PER_FRAME 1294 >> >> -/* If possible send 16 extra rows / lines to the ISP as padding */ >> -#define OV2680_END_MARGIN 16 >> - >> /* Max exposure time is VTS - 8 */ >> #define OV2680_INTEGRATION_TIME_MARGIN 8 >> >> @@ -359,11 +356,9 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor) >> sensor->mode.v_start = (sensor->mode.crop.top + >> (sensor->mode.crop.height - height) / 2) & ~1; >> sensor->mode.h_end = >> - min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1, >> - OV2680_NATIVE_WIDTH - 1); >> + min(sensor->mode.h_start + width - 1, OV2680_NATIVE_WIDTH - 1); >> sensor->mode.v_end = >> - min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1, >> - OV2680_NATIVE_HEIGHT - 1); >> + min(sensor->mode.v_start + height - 1, OV2680_NATIVE_HEIGHT - 1); >> sensor->mode.h_output_size = orig_width; >> sensor->mode.v_output_size = orig_height; >> sensor->mode.hts = OV2680_PIXELS_PER_LINE; > > Especially as seeing hts = OV2680_PIXELS_PER_LINE it does sound like the > margin is superfluous. Right. Regards, Hans
