| Message ID | 20250814130534.1125903-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 08. 14. 15:05 keltezéssel, Kieran Bingham írta: > From: "Schulz, Andreas" <andreas.schulz2@karlstorz.com> > > For debugging purposes, threads can be assigned a name, which eases > distinguishing between them in e.g. htop or gdb. This uses a > Linux-specific API for now which is limited to 15 characters (+ null > terminator), so truncation is done and names for existing thread > instantiations were chosen to be consise. I think this is useful, and I have thought about adding this on multiple occasions. > > [Kieran: Apply checkstyle suggestions] > Signed-off-by: Schulz, Andreas <andreas.schulz2@karlstorz.com> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/base/thread.h | 4 +++- > src/libcamera/base/thread.cpp | 8 +++++++- > src/libcamera/camera_manager.cpp | 2 +- > src/libcamera/software_isp/software_isp.cpp | 6 +++--- > .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 2 +- > 5 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h > index b9284c2c0556..56055261e920 100644 > --- a/include/libcamera/base/thread.h > +++ b/include/libcamera/base/thread.h > @@ -8,6 +8,7 @@ > #pragma once > > #include <memory> > +#include <string> > #include <sys/types.h> > #include <thread> > > @@ -30,7 +31,7 @@ class ThreadMain; > class Thread > { > public: > - Thread(); > + Thread(std::string name = std::string()); = {} ? > virtual ~Thread(); > > void start(); > @@ -74,6 +75,7 @@ private: > void moveObject(Object *object, ThreadData *currentData, > ThreadData *targetData); > > + std::string name_; > std::thread thread_; > ThreadData *data_; > }; > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > index d8fe0d6971ec..d016f352a783 100644 > --- a/src/libcamera/base/thread.cpp > +++ b/src/libcamera/base/thread.cpp > @@ -10,6 +10,7 @@ > #include <atomic> > #include <list> > #include <optional> > +#include <pthread.h> > #include <sys/syscall.h> > #include <sys/types.h> > #include <unistd.h> > @@ -140,6 +141,7 @@ class ThreadMain : public Thread > { > public: > ThreadMain() > + : Thread("libcamera-main") > { > data_->running_ = true; > } > @@ -230,7 +232,8 @@ ThreadData *ThreadData::current() > /** > * \brief Create a thread > */ > -Thread::Thread() > +Thread::Thread(std::string name) > + : name_(std::move(name)) > { > data_ = new ThreadData; > data_->thread_ = this; > @@ -286,6 +289,9 @@ void Thread::startThread() > data_->tid_ = syscall(SYS_gettid); > currentThreadData = data_; > > + if (!name_.empty()) > + pthread_setname_np(thread_.native_handle(), name_.substr(0, 15).c_str()); I am not sure but I believe enforcing short prefix, e.g. "lc:", would be very useful. E.g. char n[16] = {}; snprintf(n, sizeof(n), "lc:%.12s", name_.c_str()); pthread_setname_np(...); Another small question is whether the availability of `pthread_setname_np` should be checked. Regards, Barnabás Pőcze > + > run(); > } > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index f81794bfd6fe..a42835c18739 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -38,7 +38,7 @@ LOG_DEFINE_CATEGORY(Camera) > > #ifndef __DOXYGEN_PUBLIC__ > CameraManager::Private::Private() > - : initialized_(false) > + : Thread("CameraManager"), initialized_(false) > { > ipaManager_ = std::make_unique<IPAManager>(); > } > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index 28e2a360e7e6..b2f6cf61b646 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -75,9 +75,9 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) > */ > SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > ControlInfoMap *ipaControls) > - : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | > - DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | > - DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) > + : ispWorkerThread_("SWIspWorker"), dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | > + DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | > + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) > { > /* > * debayerParams_ must be initialized because the initial value is used for > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > index beb646e2d157..7d2d510f5a79 100644 > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > @@ -46,7 +46,7 @@ namespace {{ns}} { > {%- endif %} > > {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) > - : IPAProxy(ipam), isolate_(isolate), > + : IPAProxy(ipam), thread_("{{proxy_name}}"), isolate_(isolate), > controlSerializer_(ControlSerializer::Role::Proxy), seq_(0) > { > LOG(IPAProxy, Debug)
On Thu, Aug 14, 2025 at 03:16:49PM +0200, Barnabás Pőcze wrote: > 2025. 08. 14. 15:05 keltezéssel, Kieran Bingham írta: > > From: "Schulz, Andreas" <andreas.schulz2@karlstorz.com> > > > > For debugging purposes, threads can be assigned a name, which eases > > distinguishing between them in e.g. htop or gdb. This uses a > > Linux-specific API for now which is limited to 15 characters (+ null > > terminator), so truncation is done and names for existing thread > > instantiations were chosen to be consise. > > I think this is useful, and I have thought about adding this on > multiple occasions. > > > [Kieran: Apply checkstyle suggestions] > > Signed-off-by: Schulz, Andreas <andreas.schulz2@karlstorz.com> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > include/libcamera/base/thread.h | 4 +++- > > src/libcamera/base/thread.cpp | 8 +++++++- > > src/libcamera/camera_manager.cpp | 2 +- > > src/libcamera/software_isp/software_isp.cpp | 6 +++--- > > .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 2 +- > > 5 files changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h > > index b9284c2c0556..56055261e920 100644 > > --- a/include/libcamera/base/thread.h > > +++ b/include/libcamera/base/thread.h > > @@ -8,6 +8,7 @@ > > #pragma once > > > > #include <memory> > > +#include <string> > > #include <sys/types.h> > > #include <thread> > > > > @@ -30,7 +31,7 @@ class ThreadMain; > > class Thread > > { > > public: > > - Thread(); > > + Thread(std::string name = std::string()); > > = {} > > ? > > > virtual ~Thread(); > > > > void start(); > > @@ -74,6 +75,7 @@ private: > > void moveObject(Object *object, ThreadData *currentData, > > ThreadData *targetData); > > > > + std::string name_; > > std::thread thread_; > > ThreadData *data_; > > }; > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > > index d8fe0d6971ec..d016f352a783 100644 > > --- a/src/libcamera/base/thread.cpp > > +++ b/src/libcamera/base/thread.cpp > > @@ -10,6 +10,7 @@ > > #include <atomic> > > #include <list> > > #include <optional> > > +#include <pthread.h> > > #include <sys/syscall.h> > > #include <sys/types.h> > > #include <unistd.h> > > @@ -140,6 +141,7 @@ class ThreadMain : public Thread > > { > > public: > > ThreadMain() > > + : Thread("libcamera-main") This will never appear anywhere, so I wouldn't give it a name. > > { > > data_->running_ = true; > > } > > @@ -230,7 +232,8 @@ ThreadData *ThreadData::current() > > /** > > * \brief Create a thread > > */ > > -Thread::Thread() > > +Thread::Thread(std::string name) > > + : name_(std::move(name)) > > { > > data_ = new ThreadData; > > data_->thread_ = this; > > @@ -286,6 +289,9 @@ void Thread::startThread() > > data_->tid_ = syscall(SYS_gettid); > > currentThreadData = data_; > > > > + if (!name_.empty()) > > + pthread_setname_np(thread_.native_handle(), name_.substr(0, 15).c_str()); Isn't name.substr(0, 15).c_str() unsafe ? > I am not sure but I believe enforcing short prefix, e.g. "lc:", would > be very useful. E.g. > > char n[16] = {}; > snprintf(n, sizeof(n), "lc:%.12s", name_.c_str()); > pthread_setname_np(...); I'd be tempted to use "lc:%.12s", typeid(*this).name() (probably stripping the "struct" or "class" prefixes) to make all this automatic. Names could get too long though. > Another small question is whether the availability of > `pthread_setname_np` should be checked. We already use pthread_setaffinity_np(), I suppose that either both or none would be available ? > > + > > run(); > > } > > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > index f81794bfd6fe..a42835c18739 100644 > > --- a/src/libcamera/camera_manager.cpp > > +++ b/src/libcamera/camera_manager.cpp > > @@ -38,7 +38,7 @@ LOG_DEFINE_CATEGORY(Camera) > > > > #ifndef __DOXYGEN_PUBLIC__ > > CameraManager::Private::Private() > > - : initialized_(false) > > + : Thread("CameraManager"), initialized_(false) > > { > > ipaManager_ = std::make_unique<IPAManager>(); > > } > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > > index 28e2a360e7e6..b2f6cf61b646 100644 > > --- a/src/libcamera/software_isp/software_isp.cpp > > +++ b/src/libcamera/software_isp/software_isp.cpp > > @@ -75,9 +75,9 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) > > */ > > SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > > ControlInfoMap *ipaControls) > > - : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | > > - DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | > > - DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) > > + : ispWorkerThread_("SWIspWorker"), dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | > > + DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | > > + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) Line wrap before dmaHeap_. > > { > > /* > > * debayerParams_ must be initialized because the initial value is used for > > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > > index beb646e2d157..7d2d510f5a79 100644 > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > > @@ -46,7 +46,7 @@ namespace {{ns}} { > > {%- endif %} > > > > {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) > > - : IPAProxy(ipam), isolate_(isolate), > > + : IPAProxy(ipam), thread_("{{proxy_name}}"), isolate_(isolate), > > controlSerializer_(ControlSerializer::Role::Proxy), seq_(0) > > { > > LOG(IPAProxy, Debug)
2025. 08. 14. 17:17 keltezéssel, Laurent Pinchart írta: > On Thu, Aug 14, 2025 at 03:16:49PM +0200, Barnabás Pőcze wrote: >> 2025. 08. 14. 15:05 keltezéssel, Kieran Bingham írta: >>> From: "Schulz, Andreas" <andreas.schulz2@karlstorz.com> >>> >>> For debugging purposes, threads can be assigned a name, which eases >>> distinguishing between them in e.g. htop or gdb. This uses a >>> Linux-specific API for now which is limited to 15 characters (+ null >>> terminator), so truncation is done and names for existing thread >>> instantiations were chosen to be consise. >> >> I think this is useful, and I have thought about adding this on >> multiple occasions. >> >>> [Kieran: Apply checkstyle suggestions] >>> Signed-off-by: Schulz, Andreas <andreas.schulz2@karlstorz.com> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> include/libcamera/base/thread.h | 4 +++- >>> src/libcamera/base/thread.cpp | 8 +++++++- >>> src/libcamera/camera_manager.cpp | 2 +- >>> src/libcamera/software_isp/software_isp.cpp | 6 +++--- >>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 2 +- >>> 5 files changed, 15 insertions(+), 7 deletions(-) >>> >>> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h >>> index b9284c2c0556..56055261e920 100644 >>> --- a/include/libcamera/base/thread.h >>> +++ b/include/libcamera/base/thread.h >>> @@ -8,6 +8,7 @@ >>> #pragma once >>> >>> #include <memory> >>> +#include <string> >>> #include <sys/types.h> >>> #include <thread> >>> >>> @@ -30,7 +31,7 @@ class ThreadMain; >>> class Thread >>> { >>> public: >>> - Thread(); >>> + Thread(std::string name = std::string()); >> >> = {} >> >> ? >> >>> virtual ~Thread(); >>> >>> void start(); >>> @@ -74,6 +75,7 @@ private: >>> void moveObject(Object *object, ThreadData *currentData, >>> ThreadData *targetData); >>> >>> + std::string name_; >>> std::thread thread_; >>> ThreadData *data_; >>> }; >>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp >>> index d8fe0d6971ec..d016f352a783 100644 >>> --- a/src/libcamera/base/thread.cpp >>> +++ b/src/libcamera/base/thread.cpp >>> @@ -10,6 +10,7 @@ >>> #include <atomic> >>> #include <list> >>> #include <optional> >>> +#include <pthread.h> >>> #include <sys/syscall.h> >>> #include <sys/types.h> >>> #include <unistd.h> >>> @@ -140,6 +141,7 @@ class ThreadMain : public Thread >>> { >>> public: >>> ThreadMain() >>> + : Thread("libcamera-main") > > This will never appear anywhere, so I wouldn't give it a name. I suppose it could still be useful when manually inspecting the object in a debugger. > >>> { >>> data_->running_ = true; >>> } >>> @@ -230,7 +232,8 @@ ThreadData *ThreadData::current() >>> /** >>> * \brief Create a thread >>> */ >>> -Thread::Thread() >>> +Thread::Thread(std::string name) >>> + : name_(std::move(name)) >>> { >>> data_ = new ThreadData; >>> data_->thread_ = this; >>> @@ -286,6 +289,9 @@ void Thread::startThread() >>> data_->tid_ = syscall(SYS_gettid); >>> currentThreadData = data_; >>> >>> + if (!name_.empty()) >>> + pthread_setname_np(thread_.native_handle(), name_.substr(0, 15).c_str()); > > Isn't name.substr(0, 15).c_str() unsafe ? I don't think so. `std::string::substr()` creates a temporary string, but that is only destroyed at the end of the full expression, so it will be valid during the pthread call. > >> I am not sure but I believe enforcing short prefix, e.g. "lc:", would >> be very useful. E.g. >> >> char n[16] = {}; >> snprintf(n, sizeof(n), "lc:%.12s", name_.c_str()); >> pthread_setname_np(...); > > I'd be tempted to use > > "lc:%.12s", typeid(*this).name() > > (probably stripping the "struct" or "class" prefixes) to make all this > automatic. Names could get too long though. I feel like an explicit name could be better in certain cases, especially in a more complex inheritance hierarchy with templates. I suppose the type id could be the fallback. > >> Another small question is whether the availability of >> `pthread_setname_np` should be checked. > > We already use pthread_setaffinity_np(), I suppose that either both or > none would be available ? Ahh, ok, then I guess it's fine. > >>> + >>> run(); >>> } >>> >>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp >>> index f81794bfd6fe..a42835c18739 100644 >>> --- a/src/libcamera/camera_manager.cpp >>> +++ b/src/libcamera/camera_manager.cpp >>> @@ -38,7 +38,7 @@ LOG_DEFINE_CATEGORY(Camera) >>> >>> #ifndef __DOXYGEN_PUBLIC__ >>> CameraManager::Private::Private() >>> - : initialized_(false) >>> + : Thread("CameraManager"), initialized_(false) >>> { >>> ipaManager_ = std::make_unique<IPAManager>(); >>> } >>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >>> index 28e2a360e7e6..b2f6cf61b646 100644 >>> --- a/src/libcamera/software_isp/software_isp.cpp >>> +++ b/src/libcamera/software_isp/software_isp.cpp >>> @@ -75,9 +75,9 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) >>> */ >>> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >>> ControlInfoMap *ipaControls) >>> - : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | >>> - DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | >>> - DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) >>> + : ispWorkerThread_("SWIspWorker"), dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | >>> + DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | >>> + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) > > Line wrap before dmaHeap_. > >>> { >>> /* >>> * debayerParams_ must be initialized because the initial value is used for >>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >>> index beb646e2d157..7d2d510f5a79 100644 >>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >>> @@ -46,7 +46,7 @@ namespace {{ns}} { >>> {%- endif %} >>> >>> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) >>> - : IPAProxy(ipam), isolate_(isolate), >>> + : IPAProxy(ipam), thread_("{{proxy_name}}"), isolate_(isolate), >>> controlSerializer_(ControlSerializer::Role::Proxy), seq_(0) >>> { >>> LOG(IPAProxy, Debug) >
Quoting Barnabás Pőcze (2025-08-14 16:30:09) > 2025. 08. 14. 17:17 keltezéssel, Laurent Pinchart írta: > > On Thu, Aug 14, 2025 at 03:16:49PM +0200, Barnabás Pőcze wrote: > >> 2025. 08. 14. 15:05 keltezéssel, Kieran Bingham írta: > >>> From: "Schulz, Andreas" <andreas.schulz2@karlstorz.com> > >>> > >>> For debugging purposes, threads can be assigned a name, which eases > >>> distinguishing between them in e.g. htop or gdb. This uses a > >>> Linux-specific API for now which is limited to 15 characters (+ null > >>> terminator), so truncation is done and names for existing thread > >>> instantiations were chosen to be consise. > >> > >> I think this is useful, and I have thought about adding this on > >> multiple occasions. > >> > >>> [Kieran: Apply checkstyle suggestions] > >>> Signed-off-by: Schulz, Andreas <andreas.schulz2@karlstorz.com> > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> include/libcamera/base/thread.h | 4 +++- > >>> src/libcamera/base/thread.cpp | 8 +++++++- > >>> src/libcamera/camera_manager.cpp | 2 +- > >>> src/libcamera/software_isp/software_isp.cpp | 6 +++--- > >>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 2 +- > >>> 5 files changed, 15 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h > >>> index b9284c2c0556..56055261e920 100644 > >>> --- a/include/libcamera/base/thread.h > >>> +++ b/include/libcamera/base/thread.h > >>> @@ -8,6 +8,7 @@ > >>> #pragma once > >>> > >>> #include <memory> > >>> +#include <string> > >>> #include <sys/types.h> > >>> #include <thread> > >>> > >>> @@ -30,7 +31,7 @@ class ThreadMain; > >>> class Thread > >>> { > >>> public: > >>> - Thread(); > >>> + Thread(std::string name = std::string()); > >> > >> = {} > >> > >> ? Done. > >> > >>> virtual ~Thread(); > >>> > >>> void start(); > >>> @@ -74,6 +75,7 @@ private: > >>> void moveObject(Object *object, ThreadData *currentData, > >>> ThreadData *targetData); > >>> > >>> + std::string name_; > >>> std::thread thread_; > >>> ThreadData *data_; > >>> }; > >>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > >>> index d8fe0d6971ec..d016f352a783 100644 > >>> --- a/src/libcamera/base/thread.cpp > >>> +++ b/src/libcamera/base/thread.cpp > >>> @@ -10,6 +10,7 @@ > >>> #include <atomic> > >>> #include <list> > >>> #include <optional> > >>> +#include <pthread.h> > >>> #include <sys/syscall.h> > >>> #include <sys/types.h> > >>> #include <unistd.h> > >>> @@ -140,6 +141,7 @@ class ThreadMain : public Thread > >>> { > >>> public: > >>> ThreadMain() > >>> + : Thread("libcamera-main") > > > > This will never appear anywhere, so I wouldn't give it a name. > > I suppose it could still be useful when manually inspecting the object > in a debugger. I'd like to keep this one in too. > > > > > >>> { > >>> data_->running_ = true; > >>> } > >>> @@ -230,7 +232,8 @@ ThreadData *ThreadData::current() > >>> /** > >>> * \brief Create a thread > >>> */ > >>> -Thread::Thread() > >>> +Thread::Thread(std::string name) > >>> + : name_(std::move(name)) > >>> { > >>> data_ = new ThreadData; > >>> data_->thread_ = this; > >>> @@ -286,6 +289,9 @@ void Thread::startThread() > >>> data_->tid_ = syscall(SYS_gettid); > >>> currentThreadData = data_; > >>> > >>> + if (!name_.empty()) > >>> + pthread_setname_np(thread_.native_handle(), name_.substr(0, 15).c_str()); > > > > Isn't name.substr(0, 15).c_str() unsafe ? > > I don't think so. `std::string::substr()` creates a temporary string, but that is only > destroyed at the end of the full expression, so it will be valid during the pthread call. > > > > > >> I am not sure but I believe enforcing short prefix, e.g. "lc:", would > >> be very useful. E.g. > >> > >> char n[16] = {}; > >> snprintf(n, sizeof(n), "lc:%.12s", name_.c_str()); > >> pthread_setname_np(...); > > > > I'd be tempted to use > > > > "lc:%.12s", typeid(*this).name() > > > > (probably stripping the "struct" or "class" prefixes) to make all this > > automatic. Names could get too long though. > > I feel like an explicit name could be better in certain cases, especially > in a more complex inheritance hierarchy with templates. I suppose the type > id could be the fallback. I liked the idea of automatically populating the thread name from the object - but I don't think it's going to be helpful: [367:48:04.502587188] [2207769] ERROR Thread thread.cpp:306 Setting thread name to N9libcamera13Ca from N9libcamera13CameraManager7PrivateE That might be difficult to demangle in a generic way that will always get a short and obvious type... Any suggestions? Otherwise I think I would only set them if supplied. > >> Another small question is whether the availability of > >> `pthread_setname_np` should be checked. > > > > We already use pthread_setaffinity_np(), I suppose that either both or > > none would be available ? > > Ahh, ok, then I guess it's fine. > > > > > >>> + > >>> run(); > >>> } > >>> > >>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > >>> index f81794bfd6fe..a42835c18739 100644 > >>> --- a/src/libcamera/camera_manager.cpp > >>> +++ b/src/libcamera/camera_manager.cpp > >>> @@ -38,7 +38,7 @@ LOG_DEFINE_CATEGORY(Camera) > >>> > >>> #ifndef __DOXYGEN_PUBLIC__ > >>> CameraManager::Private::Private() > >>> - : initialized_(false) > >>> + : Thread("CameraManager"), initialized_(false) > >>> { > >>> ipaManager_ = std::make_unique<IPAManager>(); > >>> } > >>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > >>> index 28e2a360e7e6..b2f6cf61b646 100644 > >>> --- a/src/libcamera/software_isp/software_isp.cpp > >>> +++ b/src/libcamera/software_isp/software_isp.cpp > >>> @@ -75,9 +75,9 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) > >>> */ > >>> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > >>> ControlInfoMap *ipaControls) > >>> - : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | > >>> - DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | > >>> - DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) > >>> + : ispWorkerThread_("SWIspWorker"), dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | > >>> + DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | > >>> + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) > > > > Line wrap before dmaHeap_. Ack. -- Kieran > > > >>> { > >>> /* > >>> * debayerParams_ must be initialized because the initial value is used for > >>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > >>> index beb646e2d157..7d2d510f5a79 100644 > >>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > >>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > >>> @@ -46,7 +46,7 @@ namespace {{ns}} { > >>> {%- endif %} > >>> > >>> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) > >>> - : IPAProxy(ipam), isolate_(isolate), > >>> + : IPAProxy(ipam), thread_("{{proxy_name}}"), isolate_(isolate), > >>> controlSerializer_(ControlSerializer::Role::Proxy), seq_(0) > >>> { > >>> LOG(IPAProxy, Debug) > > >
2025. 11. 04. 19:08 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-08-14 16:30:09) >> 2025. 08. 14. 17:17 keltezéssel, Laurent Pinchart írta: >>> On Thu, Aug 14, 2025 at 03:16:49PM +0200, Barnabás Pőcze wrote: >>>> 2025. 08. 14. 15:05 keltezéssel, Kieran Bingham írta: >>>>> From: "Schulz, Andreas" <andreas.schulz2@karlstorz.com> >>>>> >>>>> For debugging purposes, threads can be assigned a name, which eases >>>>> distinguishing between them in e.g. htop or gdb. This uses a >>>>> Linux-specific API for now which is limited to 15 characters (+ null >>>>> terminator), so truncation is done and names for existing thread >>>>> instantiations were chosen to be consise. >>>> >>>> I think this is useful, and I have thought about adding this on >>>> multiple occasions. >>>> >>>>> [Kieran: Apply checkstyle suggestions] >>>>> Signed-off-by: Schulz, Andreas <andreas.schulz2@karlstorz.com> >>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>> --- >>>>> include/libcamera/base/thread.h | 4 +++- >>>>> src/libcamera/base/thread.cpp | 8 +++++++- >>>>> src/libcamera/camera_manager.cpp | 2 +- >>>>> src/libcamera/software_isp/software_isp.cpp | 6 +++--- >>>>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 2 +- >>>>> 5 files changed, 15 insertions(+), 7 deletions(-) >>>>> > [...] >>>>> -Thread::Thread() >>>>> +Thread::Thread(std::string name) >>>>> + : name_(std::move(name)) >>>>> { >>>>> data_ = new ThreadData; >>>>> data_->thread_ = this; >>>>> @@ -286,6 +289,9 @@ void Thread::startThread() >>>>> data_->tid_ = syscall(SYS_gettid); >>>>> currentThreadData = data_; >>>>> >>>>> + if (!name_.empty()) >>>>> + pthread_setname_np(thread_.native_handle(), name_.substr(0, 15).c_str()); >>> >>> Isn't name.substr(0, 15).c_str() unsafe ? >> >> I don't think so. `std::string::substr()` creates a temporary string, but that is only >> destroyed at the end of the full expression, so it will be valid during the pthread call. >> >> >>> >>>> I am not sure but I believe enforcing short prefix, e.g. "lc:", would >>>> be very useful. E.g. >>>> >>>> char n[16] = {}; >>>> snprintf(n, sizeof(n), "lc:%.12s", name_.c_str()); >>>> pthread_setname_np(...); >>> >>> I'd be tempted to use >>> >>> "lc:%.12s", typeid(*this).name() >>> >>> (probably stripping the "struct" or "class" prefixes) to make all this >>> automatic. Names could get too long though. >> >> I feel like an explicit name could be better in certain cases, especially >> in a more complex inheritance hierarchy with templates. I suppose the type >> id could be the fallback. > > I liked the idea of automatically populating the thread name from the > object - but I don't think it's going to be helpful: > > [367:48:04.502587188] [2207769] ERROR Thread thread.cpp:306 Setting thread name to N9libcamera13Ca from N9libcamera13CameraManager7PrivateE > > > That might be difficult to demangle in a generic way that will always > get a short and obvious type... > > Any suggestions? Otherwise I think I would only set them if supplied. > [...] I am not really proposing this, but there is `__cxxabiv1::__cxa_demangle()` in the c++ runtime to demangle names. But still, the length constraint probably makes anything automatic but still simple not too useful. I was thinking about using the address of the object in absence of a name, but not entirely sure about the implications. Regards, Barnabás Pőcze
diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h index b9284c2c0556..56055261e920 100644 --- a/include/libcamera/base/thread.h +++ b/include/libcamera/base/thread.h @@ -8,6 +8,7 @@ #pragma once #include <memory> +#include <string> #include <sys/types.h> #include <thread> @@ -30,7 +31,7 @@ class ThreadMain; class Thread { public: - Thread(); + Thread(std::string name = std::string()); virtual ~Thread(); void start(); @@ -74,6 +75,7 @@ private: void moveObject(Object *object, ThreadData *currentData, ThreadData *targetData); + std::string name_; std::thread thread_; ThreadData *data_; }; diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp index d8fe0d6971ec..d016f352a783 100644 --- a/src/libcamera/base/thread.cpp +++ b/src/libcamera/base/thread.cpp @@ -10,6 +10,7 @@ #include <atomic> #include <list> #include <optional> +#include <pthread.h> #include <sys/syscall.h> #include <sys/types.h> #include <unistd.h> @@ -140,6 +141,7 @@ class ThreadMain : public Thread { public: ThreadMain() + : Thread("libcamera-main") { data_->running_ = true; } @@ -230,7 +232,8 @@ ThreadData *ThreadData::current() /** * \brief Create a thread */ -Thread::Thread() +Thread::Thread(std::string name) + : name_(std::move(name)) { data_ = new ThreadData; data_->thread_ = this; @@ -286,6 +289,9 @@ void Thread::startThread() data_->tid_ = syscall(SYS_gettid); currentThreadData = data_; + if (!name_.empty()) + pthread_setname_np(thread_.native_handle(), name_.substr(0, 15).c_str()); + run(); } diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index f81794bfd6fe..a42835c18739 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -38,7 +38,7 @@ LOG_DEFINE_CATEGORY(Camera) #ifndef __DOXYGEN_PUBLIC__ CameraManager::Private::Private() - : initialized_(false) + : Thread("CameraManager"), initialized_(false) { ipaManager_ = std::make_unique<IPAManager>(); } diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index 28e2a360e7e6..b2f6cf61b646 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -75,9 +75,9 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) */ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, ControlInfoMap *ipaControls) - : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | - DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | - DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) + : ispWorkerThread_("SWIspWorker"), dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | + DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) { /* * debayerParams_ must be initialized because the initial value is used for diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl index beb646e2d157..7d2d510f5a79 100644 --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl @@ -46,7 +46,7 @@ namespace {{ns}} { {%- endif %} {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) - : IPAProxy(ipam), isolate_(isolate), + : IPAProxy(ipam), thread_("{{proxy_name}}"), isolate_(isolate), controlSerializer_(ControlSerializer::Role::Proxy), seq_(0) { LOG(IPAProxy, Debug)