| Message ID | 20210427130844.11357-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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 ;-) -- Kieran > Thanks in advance > > David > > 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(-) >
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(-)
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@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
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? 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. Thanks in advance David 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(-)