| Message ID | 20190326083902.26121-15-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Mar 26, 2019 at 09:38:57AM +0100, Jacopo Mondi wrote: > Start and stop video devices in the pipeline. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++++++++++++++++++++-- > 1 file changed, 87 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index d3519bb1d536..5b3c44174566 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -111,6 +111,9 @@ public: > int exportBuffers(enum OutputId id, unsigned int count); > void freeBuffers(); > > + int start(); > + void stop(); > + > unsigned int index_; > std::string name_; > MediaDevice *media_; > @@ -154,6 +157,9 @@ public: > BufferPool *exportBuffers(); > void freeBuffers(); > > + int start(); > + void stop(); > + > V4L2Device *output_; > V4L2Subdevice *csi2_; > V4L2Subdevice *sensor_; > @@ -371,12 +377,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) > int PipelineHandlerIPU3::start(Camera *camera) > { > IPU3CameraData *data = cameraData(camera); > - V4L2Device *cio2 = data->cio2_.output_; > + CIO2Device *cio2 = &data->cio2_; > + ImgUDevice *imgu = data->imgu_; > int ret; > > - ret = cio2->streamOn(); > + /* > + * Start the ImgU video devices, buffers will be queued to the > + * ImgU output and viewfinder when requests will be queued. > + */ > + ret = cio2->start(); > + if (ret) { > + cio2->stop(); Is this needed ? > + return ret; > + } > + > + ret = imgu->start(); > if (ret) { > - LOG(IPU3, Info) << "Failed to start camera " << camera->name(); I think this message would be useful to keep. The start() functions log the detailed error cause (either directly, or in the functions they call), but an overall message stating that the camera failed to start is useful. You can use a goto error to avoid duplicating the message in the cio2 and imgu start error paths. > + imgu->stop(); > + cio2->stop(); > return ret; > } > > @@ -386,10 +404,9 @@ int PipelineHandlerIPU3::start(Camera *camera) > void PipelineHandlerIPU3::stop(Camera *camera) > { > IPU3CameraData *data = cameraData(camera); > - V4L2Device *cio2 = data->cio2_.output_; > > - if (cio2->streamOff()) > - LOG(IPU3, Info) << "Failed to stop camera " << camera->name(); > + data->cio2_.stop(); > + data->imgu_->stop(); Similarly I think an error message would be useful here too (but without returning early, we should stop the imgu even if the cio2 fails to stop). As we don't care about the error code the code below should o. ret = data->cio2_.stop(); ret |= data->imgu_->stop(); if (ret) LOG(IPU3, Warning) << "Failed to stop camera " << camera->name(); > > PipelineHandler::stop(camera); > } > @@ -833,6 +850,46 @@ void ImgUDevice::freeBuffers() > LOG(IPU3, Error) << "Failed to release ImgU input buffers"; > } > > +int ImgUDevice::start() > +{ > + int ret; > + > + /* Start the ImgU video devices. */ > + ret = output_->streamOn(); > + if (ret) { > + LOG(IPU3, Error) << "Failed to start ImgU output"; > + return ret; > + } > + > + ret = viewfinder_->streamOn(); > + if (ret) { > + LOG(IPU3, Error) << "Failed to start ImgU viewfinder"; > + return ret; > + } > + > + ret = stat_->streamOn(); > + if (ret) { > + LOG(IPU3, Error) << "Failed to start ImgU stat"; > + return ret; > + } > + > + ret = input_->streamOn(); > + if (ret) { > + LOG(IPU3, Error) << "Failed to start ImgU input"; > + return ret; > + } > + > + return 0; > +} > + > +void ImgUDevice::stop() > +{ > + output_->streamOff(); > + viewfinder_->streamOff(); > + stat_->streamOff(); > + input_->streamOff(); > +} > + > /*------------------------------------------------------------------------------ > * CIO2 Device > */ > @@ -1026,6 +1083,30 @@ void CIO2Device::freeBuffers() > LOG(IPU3, Error) << "Failed to release CIO2 buffers"; > } > > +int CIO2Device::start() > +{ > + int ret; > + > + for (Buffer &buffer : pool_.buffers()) { > + ret = output_->queueBuffer(&buffer); > + if (ret) > + return ret; > + } > + > + ret = output_->streamOn(); > + if (ret) { > + LOG(IPU3, Error) << "Failed to start CIO2"; The streamOn() function already logs a message, I think you can remove this one. With those small issues fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + return ret; > + } > + > + return 0; > +} > + > +void CIO2Device::stop() > +{ > + output_->streamOff(); > +} > + > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3); > > } /* namespace libcamera */
Hi Jacopo, Thanks for your work. On 2019-03-26 09:38:57 +0100, Jacopo Mondi wrote: > Start and stop video devices in the pipeline. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++++++++++++++++++++-- > 1 file changed, 87 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index d3519bb1d536..5b3c44174566 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -111,6 +111,9 @@ public: > int exportBuffers(enum OutputId id, unsigned int count); > void freeBuffers(); > > + int start(); > + void stop(); > + > unsigned int index_; > std::string name_; > MediaDevice *media_; > @@ -154,6 +157,9 @@ public: > BufferPool *exportBuffers(); > void freeBuffers(); > > + int start(); > + void stop(); > + > V4L2Device *output_; > V4L2Subdevice *csi2_; > V4L2Subdevice *sensor_; > @@ -371,12 +377,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) > int PipelineHandlerIPU3::start(Camera *camera) > { > IPU3CameraData *data = cameraData(camera); > - V4L2Device *cio2 = data->cio2_.output_; > + CIO2Device *cio2 = &data->cio2_; > + ImgUDevice *imgu = data->imgu_; > int ret; > > - ret = cio2->streamOn(); > + /* > + * Start the ImgU video devices, buffers will be queued to the > + * ImgU output and viewfinder when requests will be queued. > + */ > + ret = cio2->start(); > + if (ret) { > + cio2->stop(); > + return ret; > + } > + > + ret = imgu->start(); > if (ret) { > - LOG(IPU3, Info) << "Failed to start camera " << camera->name(); > + imgu->stop(); > + cio2->stop(); > return ret; > } > > @@ -386,10 +404,9 @@ int PipelineHandlerIPU3::start(Camera *camera) > void PipelineHandlerIPU3::stop(Camera *camera) > { > IPU3CameraData *data = cameraData(camera); > - V4L2Device *cio2 = data->cio2_.output_; > > - if (cio2->streamOff()) > - LOG(IPU3, Info) << "Failed to stop camera " << camera->name(); > + data->cio2_.stop(); > + data->imgu_->stop(); Not complaining only pointing out it looks odd that one is a pointer and the other one not :-) With Laurents comments addressed feel free to add, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > PipelineHandler::stop(camera); > } > @@ -833,6 +850,46 @@ void ImgUDevice::freeBuffers() > LOG(IPU3, Error) << "Failed to release ImgU input buffers"; > } > > +int ImgUDevice::start() > +{ > + int ret; > + > + /* Start the ImgU video devices. */ > + ret = output_->streamOn(); > + if (ret) { > + LOG(IPU3, Error) << "Failed to start ImgU output"; > + return ret; > + } > + > + ret = viewfinder_->streamOn(); > + if (ret) { > + LOG(IPU3, Error) << "Failed to start ImgU viewfinder"; > + return ret; > + } > + > + ret = stat_->streamOn(); > + if (ret) { > + LOG(IPU3, Error) << "Failed to start ImgU stat"; > + return ret; > + } > + > + ret = input_->streamOn(); > + if (ret) { > + LOG(IPU3, Error) << "Failed to start ImgU input"; > + return ret; > + } > + > + return 0; > +} > + > +void ImgUDevice::stop() > +{ > + output_->streamOff(); > + viewfinder_->streamOff(); > + stat_->streamOff(); > + input_->streamOff(); > +} > + > /*------------------------------------------------------------------------------ > * CIO2 Device > */ > @@ -1026,6 +1083,30 @@ void CIO2Device::freeBuffers() > LOG(IPU3, Error) << "Failed to release CIO2 buffers"; > } > > +int CIO2Device::start() > +{ > + int ret; > + > + for (Buffer &buffer : pool_.buffers()) { > + ret = output_->queueBuffer(&buffer); > + if (ret) > + return ret; > + } > + > + ret = output_->streamOn(); > + if (ret) { > + LOG(IPU3, Error) << "Failed to start CIO2"; > + return ret; > + } > + > + return 0; > +} > + > +void CIO2Device::stop() > +{ > + output_->streamOff(); > +} > + > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3); > > } /* namespace libcamera */ > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index d3519bb1d536..5b3c44174566 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -111,6 +111,9 @@ public: int exportBuffers(enum OutputId id, unsigned int count); void freeBuffers(); + int start(); + void stop(); + unsigned int index_; std::string name_; MediaDevice *media_; @@ -154,6 +157,9 @@ public: BufferPool *exportBuffers(); void freeBuffers(); + int start(); + void stop(); + V4L2Device *output_; V4L2Subdevice *csi2_; V4L2Subdevice *sensor_; @@ -371,12 +377,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) int PipelineHandlerIPU3::start(Camera *camera) { IPU3CameraData *data = cameraData(camera); - V4L2Device *cio2 = data->cio2_.output_; + CIO2Device *cio2 = &data->cio2_; + ImgUDevice *imgu = data->imgu_; int ret; - ret = cio2->streamOn(); + /* + * Start the ImgU video devices, buffers will be queued to the + * ImgU output and viewfinder when requests will be queued. + */ + ret = cio2->start(); + if (ret) { + cio2->stop(); + return ret; + } + + ret = imgu->start(); if (ret) { - LOG(IPU3, Info) << "Failed to start camera " << camera->name(); + imgu->stop(); + cio2->stop(); return ret; } @@ -386,10 +404,9 @@ int PipelineHandlerIPU3::start(Camera *camera) void PipelineHandlerIPU3::stop(Camera *camera) { IPU3CameraData *data = cameraData(camera); - V4L2Device *cio2 = data->cio2_.output_; - if (cio2->streamOff()) - LOG(IPU3, Info) << "Failed to stop camera " << camera->name(); + data->cio2_.stop(); + data->imgu_->stop(); PipelineHandler::stop(camera); } @@ -833,6 +850,46 @@ void ImgUDevice::freeBuffers() LOG(IPU3, Error) << "Failed to release ImgU input buffers"; } +int ImgUDevice::start() +{ + int ret; + + /* Start the ImgU video devices. */ + ret = output_->streamOn(); + if (ret) { + LOG(IPU3, Error) << "Failed to start ImgU output"; + return ret; + } + + ret = viewfinder_->streamOn(); + if (ret) { + LOG(IPU3, Error) << "Failed to start ImgU viewfinder"; + return ret; + } + + ret = stat_->streamOn(); + if (ret) { + LOG(IPU3, Error) << "Failed to start ImgU stat"; + return ret; + } + + ret = input_->streamOn(); + if (ret) { + LOG(IPU3, Error) << "Failed to start ImgU input"; + return ret; + } + + return 0; +} + +void ImgUDevice::stop() +{ + output_->streamOff(); + viewfinder_->streamOff(); + stat_->streamOff(); + input_->streamOff(); +} + /*------------------------------------------------------------------------------ * CIO2 Device */ @@ -1026,6 +1083,30 @@ void CIO2Device::freeBuffers() LOG(IPU3, Error) << "Failed to release CIO2 buffers"; } +int CIO2Device::start() +{ + int ret; + + for (Buffer &buffer : pool_.buffers()) { + ret = output_->queueBuffer(&buffer); + if (ret) + return ret; + } + + ret = output_->streamOn(); + if (ret) { + LOG(IPU3, Error) << "Failed to start CIO2"; + return ret; + } + + return 0; +} + +void CIO2Device::stop() +{ + output_->streamOff(); +} + REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3); } /* namespace libcamera */
Start and stop video devices in the pipeline. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++++++++++++++++++++-- 1 file changed, 87 insertions(+), 6 deletions(-)