[libcamera-devel] [PATCH v5 3/5] ipa: raspberrypi: Add move/copy ctors and operators to Metadata class

[David Plowman]

Hi Laurent

Thanks for the comments. Just to add my explanation to your question...

On Tue, 27 Apr 2021 at 08:17, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Apr 19, 2021 at 02:34:49PM +0100, Naushir Patuck wrote:
> > Add a default, move and copy constructor as well as a move operator
> > implementation RPiController::Metadata class.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/metadata.hpp | 24 +++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp
> > index 07dd28ed9e0a..319f2320fc70 100644
> > --- a/src/ipa/raspberrypi/controller/metadata.hpp
> > +++ b/src/ipa/raspberrypi/controller/metadata.hpp
> > @@ -19,6 +19,21 @@ namespace RPiController {
> >  class Metadata
> >  {
> >  public:
> > +     Metadata() = default;
> > +
> > +     Metadata(Metadata const &other)
> > +     {
> > +             std::lock_guard<std::mutex> other_lock(other.mutex_);
> > +             data_ = other.data_;
> > +     }
> > +
> > +     Metadata(Metadata &&other)
> > +     {
> > +             std::lock_guard<std::mutex> other_lock(other.mutex_);
> > +             data_ = std::move(other.data_);
> > +             other.data_.clear();
> > +     }
> > +
> >       template<typename T>
> >       void Set(std::string const &tag, T const &value)
> >       {
> > @@ -51,6 +66,15 @@ public:
> >               return *this;
> >       }
> >
> > +     Metadata &operator=(Metadata &&other)
> > +     {
> > +             std::lock_guard<std::mutex> lock(mutex_);
> > +             std::lock_guard<std::mutex> other_lock(other.mutex_);
>
> This could cause a deadlock if two threads we to move A to B and B to A.
> I suppose such a usage pattern never happens ? We may be able to improve
> locking a bit, but that's out of scope for this series.

Indeed, that's true. But I think any situation where one thread does
"A = B" and another, in parallel, does "B = A" is fundamentally
problematic. I can't imagine what one would expect to happen...

Actually, you may recall how there's been a general process of
removing locks from our IPAs because in the libcamera world everything
is more synchronous than the place where that code originated. I do
wonder whether these locks fall into the same category, but that's a
separate investigation for a separate patch series.

Thanks!
David

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +             data_ = std::move(other.data_);
> > +             other.data_.clear();
> > +             return *this;
> > +     }
> > +
> >       template<typename T>
> >       T *GetLocked(std::string const &tag)
> >       {
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel