| Message ID | 20200827082038.40758-4-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 27/08/2020 09:20, 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@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(); > +} > + Should we explicitly close in RPiCameraData destructor? Perhaps not in fact, as the dmaHeap destructor already does that. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > void RPiCameraData::frameStarted(uint32_t sequence) > { > LOG(RPI, Debug) << "frame start " << sequence; >
On Thu, Aug 27, 2020 at 12:52:27PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 27/08/2020 09:20, 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@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(); > > +} > > + > > Should we explicitly close in RPiCameraData destructor? Perhaps not in > fact, as the dmaHeap destructor already does that. > I had added a ~RPiCameraData destructor for that, but since the DmaHeaps class destructor does the same, I then removed it. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Thanks j > > void RPiCameraData::frameStarted(uint32_t sequence) > > { > > LOG(RPI, Debug) << "frame start " << sequence; > > > > -- > Regards > -- > Kieran
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@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, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +} > + > void RPiCameraData::frameStarted(uint32_t sequence) > { > LOG(RPI, Debug) << "frame start " << sequence;
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@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@ideasonboard.com> > > > +} > > + > > void RPiCameraData::frameStarted(uint32_t sequence) > > { > > LOG(RPI, Debug) << "frame start " << sequence; > > -- > Regards, > > 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@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@ideasonboard.com> > > > > > +} > > > + > > > void RPiCameraData::frameStarted(uint32_t sequence) > > > { > > > LOG(RPI, Debug) << "frame start " << sequence;
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(); +} + void RPiCameraData::frameStarted(uint32_t sequence) { LOG(RPI, Debug) << "frame start " << sequence;
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@jmondi.org> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)