raspberrypi / libcamera

Other
196 stars 74 forks source link

Adding new image sensor support (IMX585/IMX283/IMX294) #118

Open will127534 opened 4 months ago

will127534 commented 4 months ago

Hi everyone,

I want to add the following sensor support for:

  1. IMX283
    With driver from https://github.com/will127534/imx283-v4l2-driver + Tuning files
  2. IMX585 With driver from https://github.com/will127534/imx585-v4l2-driver + Tuning files
  3. IMX294 With driver from https://github.com/will127534/imx294-v4l2-driver, but the tuning files haven't collected yet.

I'm not sure what is the requirements for adding new sensor support, so please let me know anything else I need to merge this pull request.

kbingham commented 4 months ago

Could you squash the fixes and split this into one commit per sensor please? Then I think it would be far closer to being able to merge directly to mainline libcamera.

6by9 commented 4 months ago

Could you squash the fixes and split this into one commit per sensor please? Then I think it would be far closer to being able to merge directly to mainline libcamera.

Have the drivers for these sensors been submitted to linux-media then? I don't recall seeing them. Or has that requirement been dropped from libcamera?

kbingham commented 4 months ago

It's still a requirement.

IMX283 has already been posted [0], and splitting this would let us merge the IMX283 part already upstream in libcamera, while keeping this together keeps it stalled or lost.

[0] https://lore.kernel.org/linux-media/20240215204436.9194-1-umang.jain@ideasonboard.com/

I hope IMX585 and IMX294 would also get posted upstream, but those are not sensors Ideas on Board are presently working on, so someone needs to take actions there.

will127534 commented 4 months ago

Could you squash the fixes and split this into one commit per sensor please? Then I think it would be far closer to being able to merge directly to mainline libcamera.

Done.

kbingham commented 4 months ago

Could you make it one commit per sensor please? - sorry - I meant as in a single commit for IMX283, a single commit for IMX585 and a single commit for IMX294. They can all be on the same branch - just distinct commits for each sensor topic.

will127534 commented 4 months ago

I see, sounds good! Just separated the commits for IMX283 and IMX585, let me drop the IMX294 since it is probably not polished yet.

Thanks!

naushir commented 4 months ago

I think we should send this directly to the libcamera mailing list when ready. Once it's merged upstream, I'll pull the changes down to our tree ready for the next release.

naushir commented 4 months ago

Feel free to use this space to "pre-review" the changes in the meantime.

kbingham commented 4 months ago

@will127534 , thanks for splitting. Next steps to getting upstream are updating the commit message. The most important part is adding your signed-off-by tag to your commits. We can't take anything without that.

Then a nice to have is to write a (small/brief) bit in the commit message about what the tuning file has been generated from (what lights, scene, anything relevant but it's mostly just for reference to help know what the tuning file contains as the numbers are otherwise "raw" to interpret.)

With that, as @6by9 said, we can post for review+merge with drivers posted to Linux media, and imx283 qualifies already though we'll have to figure a plan for the imx585.

With a tag I can handle some clean up and posting to the list if you prefer, but only when your tag is added, or feel free to take the next step to posting to libcamera-devel too yourself.

kbingham commented 4 months ago

Hi @will127534. Would you be able to add your Signed-off-by: tag to the commits please? Then I'll be able to assist more in upstreaming your IMX283 helpers.

will127534 commented 4 months ago

Hi @kbingham, I've updated the commits.

Thanks!

naushir commented 1 month ago

Hi guys, what state is this work in? Are we able to merge it into upstream libcamera?

kbingham commented 1 month ago

IMX283 driver is upstream in the kernel now, so that's definitely a candidate to get merged!