[libcamera-devel] [PATCH 3/3] libcamera: raspberrypi: Fail on dmaHeaps_ open error

[Jacopo Mondi]

Hi Laurent,

On Thu, Aug 27, 2020 at 05:25:31PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Aug 27, 2020 at 10:20:38AM +0200, Jacopo Mondi wrote:
> > Provide an RPiCameraData::init() method where the dmaHeaps_ member
> > is opened.
> >
> > This allows to fail earlier in case the allocator fails to open.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 42c9caa03e2e..38da45607d4b 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -291,6 +291,7 @@ public:
> >  	{
> >  	}
> >
> > +	int init();
> >  	void frameStarted(uint32_t sequence);
> >
> >  	int loadIPA();
> > @@ -904,6 +905,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >  		return false;
> >
> >  	std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
> > +	if (data->init())
> > +		return false;
> >
> >  	/* Locate and open the unicam video streams. */
> >  	data->unicam_[Unicam::Embedded] = RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
> > @@ -1084,6 +1087,11 @@ void PipelineHandlerRPi::freeBuffers(Camera *camera)
> >  		stream->releaseBuffers();
> >  }
> >
> > +int RPiCameraData::init()
> > +{
> > +	return dmaHeap_.open();
>
> This looks goot do me. With the isValid() (or similar) comment from 1/3
> addressed,
>

Considering this might move to become a standard component what API do
you think it's best ?
1) One that forces you to open the allocator, and make sure it
succeeds
2) One that allows you to check if the allocator is valid but silently
fail at construction time ?

I would pick 1, of course I should guard against double opens if
that's meant to become a standard component, but opening in
constructor + optional isValid() is not the best choice here in my
opinion.

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +}
> > +
> >  void RPiCameraData::frameStarted(uint32_t sequence)
> >  {
> >  	LOG(RPI, Debug) << "frame start " << sequence;
>
> --
> Regards,
>
> Laurent Pinchart