| Message ID | 20200627030043.2088585-10-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Sat, Jun 27, 2020 at 05:00:39AM +0200, Niklas Söderlund wrote: > Do not keep the duplicated ImgUDevice::ImgUOutput information in both > the stream and camera data classes. Remove it from the stream and only > access it from the camera data class. > > Which stream is which can instead be checked by comparing it to the > known streams in camera data. This match how streams are checked in > other parts of the code making the driver more coherent. not a driver :) > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 31 +++++++++++++++------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index ae7a01b81dacf498..1f320dc166767bab 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -35,13 +35,11 @@ class IPU3Stream : public Stream > { > public: > IPU3Stream() > - : active_(false), raw_(false), device_(nullptr) > + : active_(false) > { > } > > bool active_; > - bool raw_; > - ImgUDevice::ImgUOutput *device_; > }; > > class IPU3CameraData : public CameraData > @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > const StreamConfiguration oldCfg = cfg; > const IPU3Stream *stream = streams_[i]; > > - if (stream->raw_) { > + if (stream == &data_->rawStream_) { > cfg = cio2Configuration_; > } else { > bool scale = stream == &data_->vfStream_; > @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > IPU3CameraData *data = cameraData(camera); > - IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream); > unsigned int count = stream->configuration().bufferCount; > > - if (ipu3stream->raw_) > + if (stream == &data->outStream_) > + return data->imgu_->output_.dev->exportBuffers(count, buffers); > + else if (stream == &data->vfStream_) > + return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers); > + else if (stream == &data->rawStream_) > return data->cio2_.exportBuffers(count, buffers); > > - return ipu3stream->device_->dev->exportBuffers(count, buffers); > + return -EINVAL; > } > > /** > @@ -685,11 +686,16 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) > /* Queue all buffers from the request aimed for the ImgU. */ > for (auto it : request->buffers()) { > IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); > - if (stream->raw_) > - continue; > - > FrameBuffer *buffer = it.second; > - int ret = stream->device_->dev->queueBuffer(buffer); > + int ret; > + > + if (stream == &data->outStream_) > + ret = data->imgu_->output_.dev->queueBuffer(buffer); > + else if (stream == &data->vfStream_) > + ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer); > + else > + continue; > + I would keep the previous code flow. Up to you if (stream == &data->rawStream_) continue; int ret = 0; if (stream == &data->outStream_) ret = data->imgu_->output_.dev->queueBuffer(buffer); else if (stream == &data->vfStream_) ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer); > if (ret < 0) > error = ret; > } > @@ -801,9 +807,6 @@ int PipelineHandlerIPU3::registerCameras() > * second. > */ > data->imgu_ = numCameras ? &imgu1_ : &imgu0_; > - data->outStream_.device_ = &data->imgu_->output_; > - data->vfStream_.device_ = &data->imgu_->viewfinder_; > - data->rawStream_.raw_ = true; > > /* > * Connect video devices' 'bufferReady' signals to their > -- > 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:39AM +0200, Niklas Söderlund wrote: > Do not keep the duplicated ImgUDevice::ImgUOutput information in both > the stream and camera data classes. Remove it from the stream and only > access it from the camera data class. > > Which stream is which can instead be checked by comparing it to the > known streams in camera data. This match how streams are checked in > other parts of the code making the driver more coherent. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 31 +++++++++++++++------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index ae7a01b81dacf498..1f320dc166767bab 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -35,13 +35,11 @@ class IPU3Stream : public Stream > { > public: > IPU3Stream() > - : active_(false), raw_(false), device_(nullptr) > + : active_(false) > { > } > > bool active_; > - bool raw_; > - ImgUDevice::ImgUOutput *device_; I would have preferred to add an enum to identify the stream type and store it in IPU3Stream. This would allow using switch...case below. I understand from patch 11/13 that you want to remove the IPU3Stream class, and I wonder if it wouldn't be best to keep it with an enum id, as it could be useful for later. Up to you. > }; > > class IPU3CameraData : public CameraData > @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > const StreamConfiguration oldCfg = cfg; > const IPU3Stream *stream = streams_[i]; > > - if (stream->raw_) { > + if (stream == &data_->rawStream_) { > cfg = cio2Configuration_; > } else { > bool scale = stream == &data_->vfStream_; > @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > IPU3CameraData *data = cameraData(camera); > - IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream); > unsigned int count = stream->configuration().bufferCount; > > - if (ipu3stream->raw_) > + if (stream == &data->outStream_) > + return data->imgu_->output_.dev->exportBuffers(count, buffers); > + else if (stream == &data->vfStream_) > + return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers); > + else if (stream == &data->rawStream_) > return data->cio2_.exportBuffers(count, buffers); > > - return ipu3stream->device_->dev->exportBuffers(count, buffers); > + return -EINVAL; > } > > /** > @@ -685,11 +686,16 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) > /* Queue all buffers from the request aimed for the ImgU. */ > for (auto it : request->buffers()) { > IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); > - if (stream->raw_) > - continue; > - > FrameBuffer *buffer = it.second; > - int ret = stream->device_->dev->queueBuffer(buffer); > + int ret; > + > + if (stream == &data->outStream_) > + ret = data->imgu_->output_.dev->queueBuffer(buffer); > + else if (stream == &data->vfStream_) > + ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer); > + else > + continue; > + > if (ret < 0) > error = ret; > } > @@ -801,9 +807,6 @@ int PipelineHandlerIPU3::registerCameras() > * second. > */ > data->imgu_ = numCameras ? &imgu1_ : &imgu0_; > - data->outStream_.device_ = &data->imgu_->output_; > - data->vfStream_.device_ = &data->imgu_->viewfinder_; > - data->rawStream_.raw_ = true; > > /* > * Connect video devices' 'bufferReady' signals to their
Hi Jacopo, Thanks for your feedback. On 2020-06-27 12:06:16 +0200, Jacopo Mondi wrote: > > On Sat, Jun 27, 2020 at 05:00:39AM +0200, Niklas Söderlund wrote: > > Do not keep the duplicated ImgUDevice::ImgUOutput information in both > > the stream and camera data classes. Remove it from the stream and only > > access it from the camera data class. > > > > Which stream is which can instead be checked by comparing it to the > > known streams in camera data. This match how streams are checked in > > other parts of the code making the driver more coherent. > > > not a driver :) wops :-) > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 31 +++++++++++++++------------- > > 1 file changed, 17 insertions(+), 14 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index ae7a01b81dacf498..1f320dc166767bab 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -35,13 +35,11 @@ class IPU3Stream : public Stream > > { > > public: > > IPU3Stream() > > - : active_(false), raw_(false), device_(nullptr) > > + : active_(false) > > { > > } > > > > bool active_; > > - bool raw_; > > - ImgUDevice::ImgUOutput *device_; > > }; > > > > class IPU3CameraData : public CameraData > > @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > const StreamConfiguration oldCfg = cfg; > > const IPU3Stream *stream = streams_[i]; > > > > - if (stream->raw_) { > > + if (stream == &data_->rawStream_) { > > cfg = cio2Configuration_; > > } else { > > bool scale = stream == &data_->vfStream_; > > @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > { > > IPU3CameraData *data = cameraData(camera); > > - IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream); > > unsigned int count = stream->configuration().bufferCount; > > > > - if (ipu3stream->raw_) > > + if (stream == &data->outStream_) > > + return data->imgu_->output_.dev->exportBuffers(count, buffers); > > + else if (stream == &data->vfStream_) > > + return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers); > > + else if (stream == &data->rawStream_) > > return data->cio2_.exportBuffers(count, buffers); > > > > - return ipu3stream->device_->dev->exportBuffers(count, buffers); > > + return -EINVAL; > > } > > > > /** > > @@ -685,11 +686,16 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) > > /* Queue all buffers from the request aimed for the ImgU. */ > > for (auto it : request->buffers()) { > > IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); > > - if (stream->raw_) > > - continue; > > - > > FrameBuffer *buffer = it.second; > > - int ret = stream->device_->dev->queueBuffer(buffer); > > + int ret; > > + > > + if (stream == &data->outStream_) > > + ret = data->imgu_->output_.dev->queueBuffer(buffer); > > + else if (stream == &data->vfStream_) > > + ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer); > > + else > > + continue; > > + > > I would keep the previous code flow. Up to you > > if (stream == &data->rawStream_) > continue; > > int ret = 0; > if (stream == &data->outStream_) > ret = data->imgu_->output_.dev->queueBuffer(buffer); > else if (stream == &data->vfStream_) > ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer); I have no strong preference so I will go with your suggestion in v2. > > > if (ret < 0) > > error = ret; > > } > > @@ -801,9 +807,6 @@ int PipelineHandlerIPU3::registerCameras() > > * second. > > */ > > data->imgu_ = numCameras ? &imgu1_ : &imgu0_; > > - data->outStream_.device_ = &data->imgu_->output_; > > - data->vfStream_.device_ = &data->imgu_->viewfinder_; > > - data->rawStream_.raw_ = true; > > > > /* > > * Connect video devices' 'bufferReady' signals to their > > -- > > 2.27.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for your feedback. On 2020-06-27 19:36:36 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Sat, Jun 27, 2020 at 05:00:39AM +0200, Niklas Söderlund wrote: > > Do not keep the duplicated ImgUDevice::ImgUOutput information in both > > the stream and camera data classes. Remove it from the stream and only > > access it from the camera data class. > > > > Which stream is which can instead be checked by comparing it to the > > known streams in camera data. This match how streams are checked in > > other parts of the code making the driver more coherent. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 31 +++++++++++++++------------- > > 1 file changed, 17 insertions(+), 14 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index ae7a01b81dacf498..1f320dc166767bab 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -35,13 +35,11 @@ class IPU3Stream : public Stream > > { > > public: > > IPU3Stream() > > - : active_(false), raw_(false), device_(nullptr) > > + : active_(false) > > { > > } > > > > bool active_; > > - bool raw_; > > - ImgUDevice::ImgUOutput *device_; > > I would have preferred to add an enum to identify the stream type and > store it in IPU3Stream. This would allow using switch...case below. I > understand from patch 11/13 that you want to remove the IPU3Stream > class, and I wonder if it wouldn't be best to keep it with an enum id, > as it could be useful for later. Up to you. I think I would like to keep the change as it is unless someone have strong feelings opposing it. Keeping IPU3Stream just for the numerical id feels a bit bloated. But if we going forward need to again subclass Stream for some reason I might be persuaded. For now lets keep it simple so we can add complexity in later patches with added functionality more easily :-) > > > }; > > > > class IPU3CameraData : public CameraData > > @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > const StreamConfiguration oldCfg = cfg; > > const IPU3Stream *stream = streams_[i]; > > > > - if (stream->raw_) { > > + if (stream == &data_->rawStream_) { > > cfg = cio2Configuration_; > > } else { > > bool scale = stream == &data_->vfStream_; > > @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > { > > IPU3CameraData *data = cameraData(camera); > > - IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream); > > unsigned int count = stream->configuration().bufferCount; > > > > - if (ipu3stream->raw_) > > + if (stream == &data->outStream_) > > + return data->imgu_->output_.dev->exportBuffers(count, buffers); > > + else if (stream == &data->vfStream_) > > + return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers); > > + else if (stream == &data->rawStream_) > > return data->cio2_.exportBuffers(count, buffers); > > > > - return ipu3stream->device_->dev->exportBuffers(count, buffers); > > + return -EINVAL; > > } > > > > /** > > @@ -685,11 +686,16 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) > > /* Queue all buffers from the request aimed for the ImgU. */ > > for (auto it : request->buffers()) { > > IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); > > - if (stream->raw_) > > - continue; > > - > > FrameBuffer *buffer = it.second; > > - int ret = stream->device_->dev->queueBuffer(buffer); > > + int ret; > > + > > + if (stream == &data->outStream_) > > + ret = data->imgu_->output_.dev->queueBuffer(buffer); > > + else if (stream == &data->vfStream_) > > + ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer); > > + else > > + continue; > > + > > if (ret < 0) > > error = ret; > > } > > @@ -801,9 +807,6 @@ int PipelineHandlerIPU3::registerCameras() > > * second. > > */ > > data->imgu_ = numCameras ? &imgu1_ : &imgu0_; > > - data->outStream_.device_ = &data->imgu_->output_; > > - data->vfStream_.device_ = &data->imgu_->viewfinder_; > > - data->rawStream_.raw_ = true; > > > > /* > > * Connect video devices' 'bufferReady' signals to their > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index ae7a01b81dacf498..1f320dc166767bab 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -35,13 +35,11 @@ class IPU3Stream : public Stream { public: IPU3Stream() - : active_(false), raw_(false), device_(nullptr) + : active_(false) { } bool active_; - bool raw_; - ImgUDevice::ImgUOutput *device_; }; class IPU3CameraData : public CameraData @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() const StreamConfiguration oldCfg = cfg; const IPU3Stream *stream = streams_[i]; - if (stream->raw_) { + if (stream == &data_->rawStream_) { cfg = cio2Configuration_; } else { bool scale = stream == &data_->vfStream_; @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, std::vector<std::unique_ptr<FrameBuffer>> *buffers) { IPU3CameraData *data = cameraData(camera); - IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream); unsigned int count = stream->configuration().bufferCount; - if (ipu3stream->raw_) + if (stream == &data->outStream_) + return data->imgu_->output_.dev->exportBuffers(count, buffers); + else if (stream == &data->vfStream_) + return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers); + else if (stream == &data->rawStream_) return data->cio2_.exportBuffers(count, buffers); - return ipu3stream->device_->dev->exportBuffers(count, buffers); + return -EINVAL; } /** @@ -685,11 +686,16 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) /* Queue all buffers from the request aimed for the ImgU. */ for (auto it : request->buffers()) { IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); - if (stream->raw_) - continue; - FrameBuffer *buffer = it.second; - int ret = stream->device_->dev->queueBuffer(buffer); + int ret; + + if (stream == &data->outStream_) + ret = data->imgu_->output_.dev->queueBuffer(buffer); + else if (stream == &data->vfStream_) + ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer); + else + continue; + if (ret < 0) error = ret; } @@ -801,9 +807,6 @@ int PipelineHandlerIPU3::registerCameras() * second. */ data->imgu_ = numCameras ? &imgu1_ : &imgu0_; - data->outStream_.device_ = &data->imgu_->output_; - data->vfStream_.device_ = &data->imgu_->viewfinder_; - data->rawStream_.raw_ = true; /* * Connect video devices' 'bufferReady' signals to their
Do not keep the duplicated ImgUDevice::ImgUOutput information in both the stream and camera data classes. Remove it from the stream and only access it from the camera data class. Which stream is which can instead be checked by comparing it to the known streams in camera data. This match how streams are checked in other parts of the code making the driver more coherent. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 31 +++++++++++++++------------- 1 file changed, 17 insertions(+), 14 deletions(-)