Hi, On 4/10/24 11:24 PM, Kieran Bingham wrote: > 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 ? Besides a bunch of devices with the atomisp I also have 1 IPU3 based device with an ov2680 sensor. So I could capture full raw resolution there. But unless I modify the driver full raw resolution is 1616x1216 where as before this patch the driver sets v_end to 1631 so more pixels then the full size, which is the weirdness this patch corrects. Regards, Hans > >>> >>>> 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 >> >> >
