Hi Kieran, On 4/15/24 12:03 AM, Kieran Bingham wrote: > Hi Hans, > > Quoting Hans de Goede (2024-04-11 13:19:15) >> 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. > > That sounds reasonable to me, thank you for all the explanations. > > I don't object to this patch, but I don't have a way to test, nor > further verify this one myself specifically. > > But, the commit does reflect what the commit message states ... so I > guess that's a ... > > Reviewed-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx> Thank you. Since this patch set is quite old by now I'll go and prepare a resend with all the collected tags soon. 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 >>>> >>>> >>> >> >
