| Message ID | 20200627030043.2088585-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Sat, Jun 27, 2020 at 05:00:33AM +0200, Niklas Söderlund wrote: > When the IPU3 pipeline was first developed buffers of sinks from the > ImgU that where not active still needed to have allocated buffers I think somewhere there's a "buffer" too much, I get what you mean, but is slightly confused. > associated with them or streaming was not allowed to start. This is no > longer true, it's enough that the sinks have imported buffers for > streaming to start. As we already need to import buffers for streams > that are active we can align the two cases and always import buffers. as this changed in the driver since we first developed this, or has this always been the case? Also, as asked in 2/13, do we know which output has to have buffers mandatorily allocated ? > > With this there is no longer a reason to store the allocated > FrameBuffers to keep them alive and the vector tracking them can be > removed. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 25 ++++--------------------- > 1 file changed, 4 insertions(+), 21 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 405550b1302fb370..5a473e18c082cee8 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -49,7 +49,6 @@ public: > V4L2VideoDevice *dev; > unsigned int pad; > std::string name; > - std::vector<std::unique_ptr<FrameBuffer>> buffers; > }; > > ImgUDevice() > @@ -1124,9 +1123,6 @@ int ImgUDevice::configureOutput(ImgUOutput *output, > */ > int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount) > { > - IPU3Stream *outStream = &data->outStream_; > - IPU3Stream *vfStream = &data->vfStream_; > - > /* Share buffers between CIO2 output and ImgU input. */ > int ret = input_->importBuffers(bufferCount); > if (ret) { > @@ -1147,28 +1143,15 @@ int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount) > goto error; > } > > - /* > - * Allocate buffers for both outputs. If an output is active, prepare > - * for buffer import, otherwise allocate internal buffers. Use the same > - * number of buffers in either case. > - */ > - if (outStream->active_) > - ret = output_.dev->importBuffers(bufferCount); > - else > - ret = output_.dev->allocateBuffers(bufferCount, > - &output_.buffers); > + ret = output_.dev->importBuffers(bufferCount); > if (ret < 0) { > - LOG(IPU3, Error) << "Failed to allocate ImgU output buffers"; > + LOG(IPU3, Error) << "Failed to import ImgU output buffers"; Nit: I would leave the same error message, as the function that fails is called ImgUDevice::allocateBuffers(). The video device will complain anyway for the import error. > goto error; > } > > - if (vfStream->active_) > - ret = viewfinder_.dev->importBuffers(bufferCount); > - else > - ret = viewfinder_.dev->allocateBuffers(bufferCount, > - &viewfinder_.buffers); > + ret = viewfinder_.dev->importBuffers(bufferCount); > if (ret < 0) { > - LOG(IPU3, Error) << "Failed to allocate ImgU viewfinder buffers"; > + LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers"; > goto error; > } > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2020-06-27 11:56:51 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Sat, Jun 27, 2020 at 05:00:33AM +0200, Niklas Söderlund wrote: > > When the IPU3 pipeline was first developed buffers of sinks from the > > ImgU that where not active still needed to have allocated buffers > > I think somewhere there's a "buffer" too much, I get what you mean, > but is slightly confused. Me too, will fix. > > > associated with them or streaming was not allowed to start. This is no > > longer true, it's enough that the sinks have imported buffers for > > streaming to start. As we already need to import buffers for streams > > that are active we can align the two cases and always import buffers. > > as this changed in the driver since we first developed this, or has > this always been the case? No idea, I recall trying something like this a year back on the old kernel. But I have not installed the kernel or diffed the source code between the kernel now and then nor the libcamera sources between now and the state it was around Automotive summit last year. It felt like a bit of busy work as we know the IPU3 kernel driver is not the best quality. > > Also, as asked in 2/13, do we know which output has to have buffers > mandatorily allocated ? Only thru experimentation and reading the code. I have no official documentation. > > > > > With this there is no longer a reason to store the allocated > > FrameBuffers to keep them alive and the vector tracking them can be > > removed. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 25 ++++--------------------- > > 1 file changed, 4 insertions(+), 21 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 405550b1302fb370..5a473e18c082cee8 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -49,7 +49,6 @@ public: > > V4L2VideoDevice *dev; > > unsigned int pad; > > std::string name; > > - std::vector<std::unique_ptr<FrameBuffer>> buffers; > > }; > > > > ImgUDevice() > > @@ -1124,9 +1123,6 @@ int ImgUDevice::configureOutput(ImgUOutput *output, > > */ > > int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount) > > { > > - IPU3Stream *outStream = &data->outStream_; > > - IPU3Stream *vfStream = &data->vfStream_; > > - > > /* Share buffers between CIO2 output and ImgU input. */ > > int ret = input_->importBuffers(bufferCount); > > if (ret) { > > @@ -1147,28 +1143,15 @@ int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount) > > goto error; > > } > > > > - /* > > - * Allocate buffers for both outputs. If an output is active, prepare > > - * for buffer import, otherwise allocate internal buffers. Use the same > > - * number of buffers in either case. > > - */ > > - if (outStream->active_) > > - ret = output_.dev->importBuffers(bufferCount); > > - else > > - ret = output_.dev->allocateBuffers(bufferCount, > > - &output_.buffers); > > + ret = output_.dev->importBuffers(bufferCount); > > if (ret < 0) { > > - LOG(IPU3, Error) << "Failed to allocate ImgU output buffers"; > > + LOG(IPU3, Error) << "Failed to import ImgU output buffers"; > > Nit: I would leave the same error message, as the function that fails > is called ImgUDevice::allocateBuffers(). The video device will > complain anyway for the import error. I would not, as this aligns to others messages in this function which have done the switch allocate -> import with similar changes. Regarding the function name be at ease I hope to refactor it away in a future series :-) > > > goto error; > > } > > > > - if (vfStream->active_) > > - ret = viewfinder_.dev->importBuffers(bufferCount); > > - else > > - ret = viewfinder_.dev->allocateBuffers(bufferCount, > > - &viewfinder_.buffers); > > + ret = viewfinder_.dev->importBuffers(bufferCount); > > if (ret < 0) { > > - LOG(IPU3, Error) << "Failed to allocate ImgU viewfinder buffers"; > > + LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers"; > > goto error; > > } > > > > -- > > 2.27.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Sat, Jun 27, 2020 at 05:00:33AM +0200, Niklas Söderlund wrote: > When the IPU3 pipeline was first developed buffers of sinks from the > ImgU that where not active still needed to have allocated buffers > associated with them or streaming was not allowed to start. This is no > longer true, it's enough that the sinks have imported buffers for > streaming to start. As we already need to import buffers for streams > that are active we can align the two cases and always import buffers. > > With this there is no longer a reason to store the allocated > FrameBuffers to keep them alive and the vector tracking them can be > removed. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 25 ++++--------------------- > 1 file changed, 4 insertions(+), 21 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 405550b1302fb370..5a473e18c082cee8 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -49,7 +49,6 @@ public: > V4L2VideoDevice *dev; > unsigned int pad; > std::string name; > - std::vector<std::unique_ptr<FrameBuffer>> buffers; > }; > > ImgUDevice() > @@ -1124,9 +1123,6 @@ int ImgUDevice::configureOutput(ImgUOutput *output, > */ > int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount) > { > - IPU3Stream *outStream = &data->outStream_; > - IPU3Stream *vfStream = &data->vfStream_; > - > /* Share buffers between CIO2 output and ImgU input. */ > int ret = input_->importBuffers(bufferCount); > if (ret) { > @@ -1147,28 +1143,15 @@ int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount) > goto error; > } > > - /* > - * Allocate buffers for both outputs. If an output is active, prepare > - * for buffer import, otherwise allocate internal buffers. Use the same > - * number of buffers in either case. > - */ I'd keep a small comment here, maybe /* Import buffers for all outputs, regardless of whether the * corresponding stream is active or inactive, as the driver needs * buffers to be requested on the V4L2 devices in order to operate. */ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > - if (outStream->active_) > - ret = output_.dev->importBuffers(bufferCount); > - else > - ret = output_.dev->allocateBuffers(bufferCount, > - &output_.buffers); > + ret = output_.dev->importBuffers(bufferCount); > if (ret < 0) { > - LOG(IPU3, Error) << "Failed to allocate ImgU output buffers"; > + LOG(IPU3, Error) << "Failed to import ImgU output buffers"; > goto error; > } > > - if (vfStream->active_) > - ret = viewfinder_.dev->importBuffers(bufferCount); > - else > - ret = viewfinder_.dev->allocateBuffers(bufferCount, > - &viewfinder_.buffers); > + ret = viewfinder_.dev->importBuffers(bufferCount); > if (ret < 0) { > - LOG(IPU3, Error) << "Failed to allocate ImgU viewfinder buffers"; > + LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers"; > goto error; > } >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 405550b1302fb370..5a473e18c082cee8 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -49,7 +49,6 @@ public: V4L2VideoDevice *dev; unsigned int pad; std::string name; - std::vector<std::unique_ptr<FrameBuffer>> buffers; }; ImgUDevice() @@ -1124,9 +1123,6 @@ int ImgUDevice::configureOutput(ImgUOutput *output, */ int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount) { - IPU3Stream *outStream = &data->outStream_; - IPU3Stream *vfStream = &data->vfStream_; - /* Share buffers between CIO2 output and ImgU input. */ int ret = input_->importBuffers(bufferCount); if (ret) { @@ -1147,28 +1143,15 @@ int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount) goto error; } - /* - * Allocate buffers for both outputs. If an output is active, prepare - * for buffer import, otherwise allocate internal buffers. Use the same - * number of buffers in either case. - */ - if (outStream->active_) - ret = output_.dev->importBuffers(bufferCount); - else - ret = output_.dev->allocateBuffers(bufferCount, - &output_.buffers); + ret = output_.dev->importBuffers(bufferCount); if (ret < 0) { - LOG(IPU3, Error) << "Failed to allocate ImgU output buffers"; + LOG(IPU3, Error) << "Failed to import ImgU output buffers"; goto error; } - if (vfStream->active_) - ret = viewfinder_.dev->importBuffers(bufferCount); - else - ret = viewfinder_.dev->allocateBuffers(bufferCount, - &viewfinder_.buffers); + ret = viewfinder_.dev->importBuffers(bufferCount); if (ret < 0) { - LOG(IPU3, Error) << "Failed to allocate ImgU viewfinder buffers"; + LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers"; goto error; }
When the IPU3 pipeline was first developed buffers of sinks from the ImgU that where not active still needed to have allocated buffers associated with them or streaming was not allowed to start. This is no longer true, it's enough that the sinks have imported buffers for streaming to start. As we already need to import buffers for streams that are active we can align the two cases and always import buffers. With this there is no longer a reason to store the allocated FrameBuffers to keep them alive and the vector tracking them can be removed. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-)