| Message ID | 20240216223237.326523-2-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: ov2680: Add all controls required by libcamera | expand |
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? Or is it an optical black region? or padding? (I hit issues around that on the IMX283 lately). Is this a sensor you have and can visually check? > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > 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. > -- > 2.43.0 >
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@redhat.com> >> --- >> 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
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@redhat.com> > >> --- > >> 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 > >
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@redhat.com> >>>> --- >>>> 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 >> >> >
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@ideasonboard.com> > Regards, > > Hans > > > > > > > >>> > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>> --- > >>>> 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 > >> > >> > > >
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;
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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/media/i2c/ov2680.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)