| Message ID | 20210130182313.32903-1-sebastian.fricke@posteo.net |
|---|---|
| Headers | show |
| Series | OV13850 image sensor driver | expand |
Hi Sebastian, Thank you for your patch! I only have a few generic comments at the moment. It looks like the DT bindings (together with the MAINTAINERS changes) should be a separate patch in the patchset. ov13850_g_frame_interval() should be dropped - you have the blanking controls already: https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/camera-sensor.html?#frame-interval-configuration I've checked the Rockchip BSP kernel from which you ported this driver from, and it is fixed to use 2 data lanes only too. This puzzles me, as CAM1320 module, NanoPi M4, NanoPC-T4 - they all have 4-lane interfaces... It's OK to have only 2 lanes supported in the this patchset though. Couple more comments below. On 30.01.2021 21:23, Sebastian Fricke wrote: > Hello, > > I would like to start a discussion about how I should proceed with the OV13850 driver, which I ported from the Rockchip BSP kernel [1], added new features, and started to refactor. My goal is to merge the driver with the upstream kernel and I would like to maintain it as well. > One of the biggest problems that I am currently facing is the lack of information from Omnivision, I have requested a datasheet but was just brushed aside, as I don't provide enough financial incentive for them :(. > > This means there is a list of registers, that I found in the downstream driver, that I cannot locate on the leaked datasheet [2]. I don't have too many experiences in reverse-engineering and so before I spend too much time guessing what these registers do, I thought I might ask here for advice. This is also the major reason for this driver still being register list based, I would really like to change that as register list based drivers feel like big black boxes, especially when access to data-sheets is restricted. > > My ultimate goal for this driver would be to switch to register maps instead of raw i2c access, to change the driver from a register list based driver to a freely configurable sensor driver, That would be great! I've take a very quick look at the datasheet you've used, and at first glance getting rid of the fixed modes is doable (e.g. as a follow up patch set). > figure out what the unknown registers do, and drop everything that is not required, and overall improve the driver to correspond with the latest kernel APIs. > > Here is a list of things that I currently adjusted: > - Label all registers, that are located on the data-sheet in order to make the code more descriptive on its own > - Add the get_selection subdev IOCTL > - Enable event handling > - Add controls for toggling horizontal mirror and vertical flip Quick question: does this sensor changes the output Bayer order on these flips? E.g. this is the case for imx219: https://git.linuxtv.org/media_tree.git/tree/drivers/media/i2c/imx219.c#n646 But there exist other sensors for which the Bayer order doesn't change on [hv]flips. > - Remove the Rockchip camera module header file, that was used to add further information about the camera in the device tree, as well user-configurations for AWB. I removed this feature after comparing my driver to multiple drivers and none of the others used a similar mechanism. > - Remove multiple if-guards around code blocks, this change was again inspired by the comparison to other drivers > > There are a few things, that I have not been able to test so far, because I lack the proper hardware: > - The full-resolution 4224x3136 as the platform I am working on (rkisp1) only permits resolutions until: 4032x3024 > - The r2a register list as my device only fits to r1a > > And here is a list of questions, where I hope to find an answer here: > - The datasheet mentions that the FORMAT_0 register can be set to `vbin` and `vbinf`, what does the `f` mean? I have already searched for acronymns but couldn't find anything. > - The datasheet mentions a few registers to configure the white balance manually, I played around with these but couldn't see any visual difference. Does that automatically mean that the datasheet doesn't match the device or is there maybe another mechanism that overwrites my changes? [2] (section 5-9) > > [1] - https://github.com/friendlyarm/kernel-rockchip/blob/nanopi4-linux-v4.4.y/drivers/media/i2c/ov13850.c > [2] - http://download.tfirefly.com/product/RK3288/Docs/Peripherals/OV13850%20datasheet/Sensor_OV13850-G04A_OmniVision_SpecificationV1.pdf > > By the way, if anyone is interested, I write about my progress with the driver here: https://sebastianfricke.me/porting-the-ov13850-camera/ Thanks! This looks quite useful! Thanks, Andrey > Sebastian Fricke (1): > media: i2c: OV13850 image sensor support > > .../bindings/media/i2c/ovti,ov13850.yaml | 125 ++ > MAINTAINERS | 8 + > drivers/media/i2c/Kconfig | 12 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov13850.c | 1774 +++++++++++++++++ > 5 files changed, 1920 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov13850.yaml > create mode 100644 drivers/media/i2c/ov13850.c >
Hey Andrey, Thank you for your review and sorry for taking ages to give feedback to your review, I had a lot of other tasks flying in, and additionally your question about the Bayer pattern order required some analysis from my side more about this below. On 01.02.2021 14:56, Andrey Konovalov wrote: >Hi Sebastian, > >Thank you for your patch! > >I only have a few generic comments at the moment. > >It looks like the DT bindings (together with the MAINTAINERS changes) should be a separate >patch in the patchset. Yes, I will definitly do this, I wanted to show where I am currently at with this first patch and I will clean this up in the next patch. > >ov13850_g_frame_interval() should be dropped - you have the blanking controls already: >https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/camera-sensor.html?#frame-interval-configuration Thank you for the resource and thanks for the hint, I will address this in the next version. > >I've checked the Rockchip BSP kernel from which you ported this driver from, and it is fixed to use >2 data lanes only too. >This puzzles me, as CAM1320 module, NanoPi M4, NanoPC-T4 - they all have 4-lane interfaces... >It's OK to have only 2 lanes supported in the this patchset though. I will address this in a future version, for now I would like to have the clean basic version. > >Couple more comments below. > >On 30.01.2021 21:23, Sebastian Fricke wrote: >>Hello, >> >>I would like to start a discussion about how I should proceed with the OV13850 driver, which I ported from the Rockchip BSP kernel [1], added new features, and started to refactor. My goal is to merge the driver with the upstream kernel and I would like to maintain it as well. >>One of the biggest problems that I am currently facing is the lack of information from Omnivision, I have requested a datasheet but was just brushed aside, as I don't provide enough financial incentive for them :(. >> >>This means there is a list of registers, that I found in the downstream driver, that I cannot locate on the leaked datasheet [2]. I don't have too many experiences in reverse-engineering and so before I spend too much time guessing what these registers do, I thought I might ask here for advice. This is also the major reason for this driver still being register list based, I would really like to change that as register list based drivers feel like big black boxes, especially when access to data-sheets is restricted. >> >>My ultimate goal for this driver would be to switch to register maps instead of raw i2c access, to change the driver from a register list based driver to a freely configurable sensor driver, > >That would be great! >I've take a very quick look at the datasheet you've used, and at first >glance getting rid of the fixed modes is doable (e.g. as a follow up >patch set). The current code takes a different register list for the two modes and I found out that a lot of those registers care about the crop location and size. But there is a set of register 0x37* & 0x38* & 0x40* & 0x54* (total 15 registers per mode), that are unknown to me as they are not declared in the data-sheet. Both modes use completely different values for those registers, so my current thought was: "I guess I can only get rid of the fixed modes once I know what those registers do.". But I might be wrong and maybe I can make a subset of those registers freely configurable. Still, as you said already, this will probably be a future patch set. > >>figure out what the unknown registers do, and drop everything that is not required, and overall improve the driver to correspond with the latest kernel APIs. >> >>Here is a list of things that I currently adjusted: >>- Label all registers, that are located on the data-sheet in order to make the code more descriptive on its own >>- Add the get_selection subdev IOCTL >>- Enable event handling >>- Add controls for toggling horizontal mirror and vertical flip > >Quick question: does this sensor changes the output Bayer order on these flips? >E.g. this is the case for imx219: >https://git.linuxtv.org/media_tree.git/tree/drivers/media/i2c/imx219.c#n646 >But there exist other sensors for which the Bayer order doesn't change >on [hv]flips. I thought quite a bit about this question and I've written about my analysis and thought process here: https://sebastianfricke.me/effects-of-image-mirroring-on-bayer-pattern-order/ In the end, I was still not entirely sure and maybe you can provide me further hints as I have not worked too much with raw videos before. With my current point of view, I believe that the order does not change, then again as written in my analysis the data-sheet is not really clear about this point and the sentence in the flip section leads more to the conclusion that the order does change. > >>- Remove the Rockchip camera module header file, that was used to add further information about the camera in the device tree, as well user-configurations for AWB. I removed this feature after comparing my driver to multiple drivers and none of the others used a similar mechanism. >>- Remove multiple if-guards around code blocks, this change was again inspired by the comparison to other drivers >> >>There are a few things, that I have not been able to test so far, because I lack the proper hardware: >>- The full-resolution 4224x3136 as the platform I am working on (rkisp1) only permits resolutions until: 4032x3024 >>- The r2a register list as my device only fits to r1a >> >>And here is a list of questions, where I hope to find an answer here: >>- The datasheet mentions that the FORMAT_0 register can be set to `vbin` and `vbinf`, what does the `f` mean? I have already searched for acronymns but couldn't find anything. >>- The datasheet mentions a few registers to configure the white balance manually, I played around with these but couldn't see any visual difference. Does that automatically mean that the datasheet doesn't match the device or is there maybe another mechanism that overwrites my changes? [2] (section 5-9) >> >>[1] - https://github.com/friendlyarm/kernel-rockchip/blob/nanopi4-linux-v4.4.y/drivers/media/i2c/ov13850.c >>[2] - http://download.tfirefly.com/product/RK3288/Docs/Peripherals/OV13850%20datasheet/Sensor_OV13850-G04A_OmniVision_SpecificationV1.pdf >> >>By the way, if anyone is interested, I write about my progress with the driver here: https://sebastianfricke.me/porting-the-ov13850-camera/ > >Thanks! This looks quite useful! Great good to hear, I will extend it further with the new features I add and problems that I encounter. > > >Thanks, >Andrey Thank you Sebastian > >>Sebastian Fricke (1): >> media: i2c: OV13850 image sensor support >> >> .../bindings/media/i2c/ovti,ov13850.yaml | 125 ++ >> MAINTAINERS | 8 + >> drivers/media/i2c/Kconfig | 12 + >> drivers/media/i2c/Makefile | 1 + >> drivers/media/i2c/ov13850.c | 1774 +++++++++++++++++ >> 5 files changed, 1920 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov13850.yaml >> create mode 100644 drivers/media/i2c/ov13850.c >>
Hi Sebastian, On 27.02.2021 09:54, Sebastian Fricke wrote: > Hey Andrey, > > Thank you for your review and sorry for taking ages to give feedback to > your review, I had a lot of other tasks flying in, and additionally your > question about the Bayer pattern order required some analysis from my > side more about this below. > > On 01.02.2021 14:56, Andrey Konovalov wrote: >> Hi Sebastian, >> >> Thank you for your patch! >> >> I only have a few generic comments at the moment. >> >> It looks like the DT bindings (together with the MAINTAINERS changes) should be a separate >> patch in the patchset. > > Yes, I will definitly do this, I wanted to show where I am currently at > with this first patch and I will clean this up in the next patch. > >> >> ov13850_g_frame_interval() should be dropped - you have the blanking controls already: >> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/camera-sensor.html?#frame-interval-configuration > > Thank you for the resource and thanks for the hint, I will address this > in the next version. > >> >> I've checked the Rockchip BSP kernel from which you ported this driver from, and it is fixed to use >> 2 data lanes only too. >> This puzzles me, as CAM1320 module, NanoPi M4, NanoPC-T4 - they all have 4-lane interfaces... >> It's OK to have only 2 lanes supported in the this patchset though. > > I will address this in a future version, for now I would like to have > the clean basic version. > >> >> Couple more comments below. >> >> On 30.01.2021 21:23, Sebastian Fricke wrote: >>> Hello, >>> >>> I would like to start a discussion about how I should proceed with the OV13850 driver, which I ported from the Rockchip BSP >>> kernel [1], added new features, and started to refactor. My goal is to merge the driver with the upstream kernel and I would >>> like to maintain it as well. >>> One of the biggest problems that I am currently facing is the lack of information from Omnivision, I have requested a >>> datasheet but was just brushed aside, as I don't provide enough financial incentive for them :(. >>> >>> This means there is a list of registers, that I found in the downstream driver, that I cannot locate on the leaked datasheet >>> [2]. I don't have too many experiences in reverse-engineering and so before I spend too much time guessing what these >>> registers do, I thought I might ask here for advice. This is also the major reason for this driver still being register list >>> based, I would really like to change that as register list based drivers feel like big black boxes, especially when access to >>> data-sheets is restricted. >>> >>> My ultimate goal for this driver would be to switch to register maps instead of raw i2c access, to change the driver from a >>> register list based driver to a freely configurable sensor driver, >> >> That would be great! >> I've take a very quick look at the datasheet you've used, and at first glance getting rid of the fixed modes is doable (e.g. >> as a follow up patch set). > > The current code takes a different register list for the two modes and I > found out that a lot of those registers care about the crop location and > size. But there is a set of register 0x37* & 0x38* & 0x40* & 0x54* > (total 15 registers per mode), that are unknown to me as they are not > declared in the data-sheet. Both modes use completely different values > for those registers, so my current thought was: "I guess I can only get > rid of the fixed modes once I know what those registers do.". But I > might be wrong and maybe I can make a subset of those registers freely > configurable. Still, as you said already, this will probably be a future > patch set. > >> >>> figure out what the unknown registers do, and drop everything that is not required, and overall improve the driver to >>> correspond with the latest kernel APIs. >>> >>> Here is a list of things that I currently adjusted: >>> - Label all registers, that are located on the data-sheet in order to make the code more descriptive on its own >>> - Add the get_selection subdev IOCTL >>> - Enable event handling >>> - Add controls for toggling horizontal mirror and vertical flip >> >> Quick question: does this sensor changes the output Bayer order on these flips? >> E.g. this is the case for imx219: >> https://git.linuxtv.org/media_tree.git/tree/drivers/media/i2c/imx219.c#n646 >> But there exist other sensors for which the Bayer order doesn't change >> on [hv]flips. > > I thought quite a bit about this question and I've written about my > analysis and thought process here: https://sebastianfricke.me/effects-of-image-mirroring-on-bayer-pattern-order/ > > In the end, I was still not entirely sure and maybe you can provide me > further hints as I have not worked too much with raw videos before. > With my current point of view, I believe that the order does not change, > then again as written in my analysis the data-sheet is not really clear > about this point and the sentence in the flip section leads more to the > conclusion that the order does change. I've checked the captured images (thank you Sebastian for providing them), and indeed, the bayer order doesn't change on h/v flips - it is always BGGR. Thanks, Andrey >>> - Remove the Rockchip camera module header file, that was used to add further information about the camera in the device >>> tree, as well user-configurations for AWB. I removed this feature after comparing my driver to multiple drivers and none of >>> the others used a similar mechanism. >>> - Remove multiple if-guards around code blocks, this change was again inspired by the comparison to other drivers >>> >>> There are a few things, that I have not been able to test so far, because I lack the proper hardware: >>> - The full-resolution 4224x3136 as the platform I am working on (rkisp1) only permits resolutions until: 4032x3024 >>> - The r2a register list as my device only fits to r1a >>> >>> And here is a list of questions, where I hope to find an answer here: >>> - The datasheet mentions that the FORMAT_0 register can be set to `vbin` and `vbinf`, what does the `f` mean? I have already >>> searched for acronymns but couldn't find anything. >>> - The datasheet mentions a few registers to configure the white balance manually, I played around with these but couldn't see >>> any visual difference. Does that automatically mean that the datasheet doesn't match the device or is there maybe another >>> mechanism that overwrites my changes? [2] (section 5-9) >>> >>> [1] - https://github.com/friendlyarm/kernel-rockchip/blob/nanopi4-linux-v4.4.y/drivers/media/i2c/ov13850.c >>> [2] - >>> http://download.tfirefly.com/product/RK3288/Docs/Peripherals/OV13850%20datasheet/Sensor_OV13850-G04A_OmniVision_SpecificationV1.pdf >>> >>> >>> By the way, if anyone is interested, I write about my progress with the driver here: >>> https://sebastianfricke.me/porting-the-ov13850-camera/ >> >> Thanks! This looks quite useful! > > Great good to hear, I will extend it further with the new features I add > and problems that I encounter. > >> >> >> Thanks, >> Andrey > > Thank you > > Sebastian > >> >>> Sebastian Fricke (1): >>> media: i2c: OV13850 image sensor support >>> >>> .../bindings/media/i2c/ovti,ov13850.yaml | 125 ++ >>> MAINTAINERS | 8 + >>> drivers/media/i2c/Kconfig | 12 + >>> drivers/media/i2c/Makefile | 1 + >>> drivers/media/i2c/ov13850.c | 1774 +++++++++++++++++ >>> 5 files changed, 1920 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov13850.yaml >>> create mode 100644 drivers/media/i2c/ov13850.c >>>