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

12004 diff mbox series

Add a default, move and copy constructor as well as a move operator
implementation RPiController::Metadata class.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/metadata.hpp | 24 +++++++++++++++++++++
 1 file changed, 24 insertions(+)

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@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		data_ = std::move(other.data_);
> +		other.data_.clear();
> +		return *this;
> +	}
> +
>  	template<typename T>
>  	T *GetLocked(std::string const &tag)
>  	{

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@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@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel