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> > 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 > >> > >> > > >
