[libcamera-devel] [PATCH 0/5] Remove Raspberry Pi logging

[Laurent Pinchart]

Hi David,

On Mon, Jan 25, 2021 at 10:58:37AM +0000, David Plowman wrote:
> Hi Laurent
> 
> Thanks for the review of all these patches.
> 
> I guess I was just swapping out the macros, but I think your
> observations are fair enough:
> 
> * Debug that is just tracing can probably just be removed. It is
> inherited from the platform where this code originally came from, and
> back then it was all new and I probably liked the reassurance about
> what was happening, especially as we were just processing single
> images.
> 
> * Things like hierarchical categories sound nice, but I'm not sure
> it's that important for us. The most likely use case for us will be
> when some particular algorithm (e.g. AWB) is mis-behaviing and we want
> to see what that single algorithm is doing.
> 
> * Yes, there seem to be some messages that are warnings but were not
> categorised initially with RPI_WARN. Not sure why, but they probably
> should be changed.
> 
> As regards making these changes, would you fold them into the existing
> commits, or would you rather have them as additional changes (i.e.
> keep the macro-swapping separate from anything else)? For myself, I
> don't mind either way!

Whatever is easier for you :-) Having separate changes produces a bit of
a cleaner history, but I doubt anyone will need to read the history of
these changes.

> Let me know what you think, and I can do a v2 set accordingly.
> 
> On Sat, 23 Jan 2021 at 11:16, Laurent Pinchart wrote:
> > On Fri, Jan 22, 2021 at 11:31:58AM +0000, Kieran Bingham wrote:
> > > On 22/01/2021 10:22, David Plowman wrote:
> > > > This patch set removes the old Raspberry Pi logging from all our
> > > > control algorithms and replaces it with libcamera logging. There is
> > > > literally nothing in this patch set except for the necessary macro
> > > > replacements and a few whitespace adjustments to keep the style
> > > > checker happy.
> > > >
> > > > This is actually quite an important change because, now that we're
> > > > about to publish libcamera versions of our legacy applications, it
> > > > makes it much easier to get debug information from our customers.
> > >
> > > Aha, great, indeed it will tie into the existing logging infrastructure
> > > nicely.
> > >
> > > > Perhaps the biggest question is whether to squish all the patches
> > > > together? I've not done this yet as it's easier to squish than to
> > > > un-squish, though I did roll up all the "minor" algorithms into a
> > > > single commit. But I'll happily do that if it's tidier!
> > >
> > > I don't think that matters too much here. Indeed we could squash 1-4, or
> > > split 4/5 further - but I don't think any of that effort is required here.
> >
> > Agreed, we can keep it as-is. It's certainly easier to review with 5
> > patches than with a single one.
> >
> > > It's interesting that you can now enabled/disable specific algorithm
> > > debug explictily. I wonder if sometime later we might want to be able to
> > > 'group' multiple debug categories to enable all IPA debug without
> > > enabling V4L2 debug for instance, or perhaps to have a way to 'disable'
> > > some categories.
> >
> > We can use a wildcard * at the end of a category name, so the feature is
> > already here.
> >
> > > But that's speculation on future features - for this whole series
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > > Thanks and best regards
> > > > David
> > > >
> > > > David Plowman (5):
> > > >   ipa: raspberrypi: controller: Replace Raspberry Pi debug with
> > > >     libcamera debug
> > > >   ipa: raspberrypi: alsc: Replace Raspberry Pi debug with libcamera
> > > >     debug
> > > >   ipa: raspberrypi: awb: Replace Raspberry Pi debug with libcamera debug
> > > >   ipa: raspberrypi: Replace Raspberry Pi debug with libcamera debug
> > > >   ipa: raspberrypi: Remove legacy Rasberry Pi logging
> > > >
> > > >  src/ipa/raspberrypi/controller/algorithm.hpp  |  1 -
> > > >  src/ipa/raspberrypi/controller/controller.cpp | 29 +++---
> > > >  src/ipa/raspberrypi/controller/logging.hpp    | 30 ------
> > > >  src/ipa/raspberrypi/controller/rpi/alsc.cpp   | 59 ++++++------
> > > >  src/ipa/raspberrypi/controller/rpi/awb.cpp    | 92 ++++++++++---------
> > > >  .../controller/rpi/black_level.cpp            |  8 +-
> > > >  src/ipa/raspberrypi/controller/rpi/ccm.cpp    | 24 +++--
> > > >  .../raspberrypi/controller/rpi/contrast.cpp   | 15 ++-
> > > >  src/ipa/raspberrypi/controller/rpi/dpc.cpp    |  8 +-
> > > >  src/ipa/raspberrypi/controller/rpi/geq.cpp    | 18 ++--
> > > >  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 12 ++-
> > > >  src/ipa/raspberrypi/controller/rpi/noise.cpp  | 14 ++-
> > > >  src/ipa/raspberrypi/controller/rpi/sdn.cpp    | 21 +++--
> > > >  .../raspberrypi/controller/rpi/sharpen.cpp    |  8 +-
> > > >  14 files changed, 179 insertions(+), 160 deletions(-)
> > > >  delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp

-- 
Regards,

Laurent Pinchart