[libcamera-devel] [PATCH v4 0/4] Raspberry Pi: handle sensors more flexibly

[David Plowman]

Hi

Thanks for the feedback everyone.

I think it probably makes sense to drop this entire set on the
following grounds:

* We're not doing the virtual exposure method thing, at least, not
unless we find ourselves boxed into a corner with no other way out!
But watch out for another looming metadiscussion on what to do about
per-mode sensitivities instead... :)

* The V4L2_CID_NOTIFY_GAIN stuff wants to wait until there's been some
progress on the linux media mailing list. Hopefully I'll get round to
that soon now.

* I don't see any benefit adding the ColourGainCode method until we're using it.

Best regards

David

On Wed, 28 Apr 2021 at 12:35, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Tue, Apr 27, 2021 at 09:17:55PM +0100, Kieran Bingham wrote:
> > Hi David,
> >
> > On 27/04/2021 14:08, David Plowman wrote:
> > > Hi
> > >
> > > Here's a version 4 of this patch set. The only difference over v3 is a
> > > new penultimate patch that adds new V4L2 controls for informing the
> > > sensor of the colour gains.
> > >
> > > I've actually added a control for each of the 4 Bayer colour channels
> > > even though I use only two of them. It feels like one of those things
> > > where some sensor may eventually want the ones that I might have been
> > > tempted to miss out, at which point why not just include them? Though
> > > I still wonder if having both GREENR and GREENB is overkill, and just
> > > GREEN would have sufficed. Any thoughts on that?
> >
> > I think adding them all in now makes more sense than adding some in now,
> > and then finding the others are needed but the numbering is no longer
> > consecutive because a control has been added in the meanwhile.
> >
> > Would there ever be a (non-bayer?) sensor that would use only R,G,B? In
> > which case, I would wonder which GREEN should be used... but I suspect
> > that's not really an issue that will occur if sensors that need this
> > data are always going to be some form of bayer.
> >
> > > Once we're happy with the changes here I can apply them in the
> > > Raspberry Pi Linux distribution, and then we can look to upstream them
> > > too.
> >
> > I would suggest doing this the other way around.
> >
> > The V4L2 patch should already be posted to the V4L2 mailing list, to get
> > early review comments.
> >
> > "Upstream first" is always better ;-)
>
> +1. We can merge things in libcamera without waiting for upstream in
> some cases, but the code has to be on its way to upstream. In this
> particular case, it means that the patches have to be posted to the
> linux-media mailign list, with at least a general agreement on the
> direction. Documentation for the new controls is thus needed.
>
> Small details and upstreaming delays due to the schedule of the upstream
> kernel merge windows are not blockers, we can add the controls to the
> libcamera copy of the kernel headers once they're approved in principle
> by upstream.
>
> > > David Plowman (4):
> > >   ipa: raspberrypi: Make CamHelper exposure methods virtual
> > >   ipa: raspberrypi: Add CamHelper::ColourGainCode method
> > >   include: linux: Add V4L2_CID_NOTIFY_GAIN_XXX controls
> > >   ipa: raspberrypi: Update sensor's V4L2_CID_NOTIFY_GAIN_RED/BLUE
> > >     controls when present
> > >
> > >  include/libcamera/ipa/raspberrypi.mojom        |  1 +
> > >  include/linux/v4l2-controls.h                  |  4 ++++
> > >  src/ipa/raspberrypi/cam_helper.cpp             | 18 ++++++++++++++++++
> > >  src/ipa/raspberrypi/cam_helper.hpp             |  8 +++++---
> > >  src/ipa/raspberrypi/raspberrypi.cpp            | 13 +++++++++++++
> > >  .../pipeline/raspberrypi/raspberrypi.cpp       | 10 ++++++++++
> > >  6 files changed, 51 insertions(+), 3 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart