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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Aug 28, 2020 at 01:04:12PM +0200, Jacopo Mondi wrote:
> On Thu, Aug 27, 2020 at 05:25:31PM +0300, Laurent Pinchart wrote:
> > 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.

As we'll have to refactor it anyway, I'd go for 2 as that's the least
intrusive change, doesn't require thinking about how to implement the
open/close semantics, or guarding against multiple opens. It's up to
you, but I don't think now is the time to already think about how it
will be refactored, we don't know yet.

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

-- 
Regards,

Laurent Pinchart