Quoting Hans de Goede (2024-04-10 12:27:03) > Hi, > > Sorry for being very slow with replying to this. > No worries, > 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 Me neither at this point. I was probably worried about the impact of changing these values causing visible issues in the stride/line widths or such. But if you're testing and capturing images successfully I'm not worried now. > 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). Or maybe that was the part to check visually ;-) But I guess it's not easy to capture the full raw images 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 > >
