| Message ID | 20190418164638.400-6-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Apr 18, 2019 at 06:46:37PM +0200, Jacopo Mondi wrote: > The viewfinder and main output require identical logic for buffer and > request completion. Connect the viewfinder bufferReady signal to the slot > and handle requests for both main output and viewfinder there. > > Update the slot logic to ignore internal buffers that are not part of > the request, Is this still needed ? > and to complete the request only when the last buffer completes. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 32 +++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 8353272642bd..647b4f4f365f 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -729,6 +729,8 @@ int PipelineHandlerIPU3::registerCameras() > &IPU3CameraData::imguInputBufferReady); > data->imgu_->output_.dev->bufferReady.connect(data.get(), > &IPU3CameraData::imguOutputBufferReady); > + data->imgu_->viewfinder_.dev->bufferReady.connect(data.get(), > + &IPU3CameraData::imguOutputBufferReady); > > /* Create and register the Camera instance. */ > std::string cameraName = cio2->sensor_->entity()->name() + " " > @@ -774,10 +776,34 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer) > */ > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer) > { > - Request *request = queuedRequests_.front(); > + Request *request = buffer->request(); > + if (!request) > + /* Completed buffers not part of a request are ignored. */ > + return; Now that you don't queue dummy buffers anymore, can this still happen ? > + > + if (!pipe_->completeBuffer(camera_, request, buffer)) > + /* Request not completed yet, return here. */ > + return; > + > + /* > + * Complete requests in queuing order: if some other request is > + * pending, post-pone completion. > + */ > + Request *front = queuedRequests_.front(); > + if (front != request) > + return; > > - pipe_->completeBuffer(camera_, request, buffer); > - pipe_->completeRequest(camera_, request); > + /* > + * Complete the current request, and all the other pending ones, > + * in queuing order. > + */ > + while (1) { I think you've missed my review of v5. > + if (front->hasPendingBuffers()) > + break; > + > + pipe_->completeRequest(camera_, front); > + front = queuedRequests_.front(); > + } > } > > /**
Hi Laurent, On Thu, Apr 18, 2019 at 11:43:35PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Apr 18, 2019 at 06:46:37PM +0200, Jacopo Mondi wrote: > > The viewfinder and main output require identical logic for buffer and > > request completion. Connect the viewfinder bufferReady signal to the slot > > and handle requests for both main output and viewfinder there. > > > > Update the slot logic to ignore internal buffers that are not part of > > the request, > > Is this still needed ? > Again no, as the below code handling this case. > > and to complete the request only when the last buffer completes. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 32 +++++++++++++++++++++++++--- > > 1 file changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 8353272642bd..647b4f4f365f 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -729,6 +729,8 @@ int PipelineHandlerIPU3::registerCameras() > > &IPU3CameraData::imguInputBufferReady); > > data->imgu_->output_.dev->bufferReady.connect(data.get(), > > &IPU3CameraData::imguOutputBufferReady); > > + data->imgu_->viewfinder_.dev->bufferReady.connect(data.get(), > > + &IPU3CameraData::imguOutputBufferReady); > > > > /* Create and register the Camera instance. */ > > std::string cameraName = cio2->sensor_->entity()->name() + " " > > @@ -774,10 +776,34 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer) > > */ > > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer) > > { > > - Request *request = queuedRequests_.front(); > > + Request *request = buffer->request(); > > + if (!request) > > + /* Completed buffers not part of a request are ignored. */ > > + return; > > Now that you don't queue dummy buffers anymore, can this still happen ? > It should not. > > + > > + if (!pipe_->completeBuffer(camera_, request, buffer)) > > + /* Request not completed yet, return here. */ > > + return; > > + > > + /* > > + * Complete requests in queuing order: if some other request is > > + * pending, post-pone completion. > > + */ > > + Request *front = queuedRequests_.front(); > > + if (front != request) > > + return; > > > > - pipe_->completeBuffer(camera_, request, buffer); > > - pipe_->completeRequest(camera_, request); > > + /* > > + * Complete the current request, and all the other pending ones, > > + * in queuing order. > > + */ > > + while (1) { > > I think you've missed my review of v5. > This one on v4 you mean? ------------------------------------------------------------------------- I think you can simplify all this with /* Complete requests in queuing order. */ while (1) { request = queuedRequest_.front(); if (!request->empty()) break; pipe_->completeRequest(camera_, request); ------------------------------------------------------------------------ I might have missed why it is better, considering here above I have to check (front == request) to proceed to complete it. Thanks j > > + if (front->hasPendingBuffers()) > > + break; > > + > > + pipe_->completeRequest(camera_, front); > > + front = queuedRequests_.front(); > > + } > > } > > > > /** > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Fri, Apr 19, 2019 at 08:51:15AM +0200, Jacopo Mondi wrote: > On Thu, Apr 18, 2019 at 11:43:35PM +0300, Laurent Pinchart wrote: > > On Thu, Apr 18, 2019 at 06:46:37PM +0200, Jacopo Mondi wrote: > >> The viewfinder and main output require identical logic for buffer and > >> request completion. Connect the viewfinder bufferReady signal to the slot > >> and handle requests for both main output and viewfinder there. > >> > >> Update the slot logic to ignore internal buffers that are not part of > >> the request, > > > > Is this still needed ? > > Again no, as the below code handling this case. > > >> and to complete the request only when the last buffer completes. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 32 +++++++++++++++++++++++++--- > >> 1 file changed, 29 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 8353272642bd..647b4f4f365f 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -729,6 +729,8 @@ int PipelineHandlerIPU3::registerCameras() > >> &IPU3CameraData::imguInputBufferReady); > >> data->imgu_->output_.dev->bufferReady.connect(data.get(), > >> &IPU3CameraData::imguOutputBufferReady); > >> + data->imgu_->viewfinder_.dev->bufferReady.connect(data.get(), > >> + &IPU3CameraData::imguOutputBufferReady); > >> > >> /* Create and register the Camera instance. */ > >> std::string cameraName = cio2->sensor_->entity()->name() + " " > >> @@ -774,10 +776,34 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer) > >> */ > >> void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer) > >> { > >> - Request *request = queuedRequests_.front(); > >> + Request *request = buffer->request(); > >> + if (!request) > >> + /* Completed buffers not part of a request are ignored. */ > >> + return; > > > > Now that you don't queue dummy buffers anymore, can this still happen ? > > It should not. > > >> + > >> + if (!pipe_->completeBuffer(camera_, request, buffer)) > >> + /* Request not completed yet, return here. */ > >> + return; > >> + > >> + /* > >> + * Complete requests in queuing order: if some other request is > >> + * pending, post-pone completion. > >> + */ > >> + Request *front = queuedRequests_.front(); > >> + if (front != request) > >> + return; > >> > >> - pipe_->completeBuffer(camera_, request, buffer); > >> - pipe_->completeRequest(camera_, request); > >> + /* > >> + * Complete the current request, and all the other pending ones, > >> + * in queuing order. > >> + */ > >> + while (1) { > > > > I think you've missed my review of v5. > > This one on v4 you mean? > > ------------------------------------------------------------------------- > > I think you can simplify all this with > > /* Complete requests in queuing order. */ > while (1) { > request = queuedRequest_.front(); > if (!request->empty()) > break; > > pipe_->completeRequest(camera_, request); > ------------------------------------------------------------------------ > > I might have missed why it is better, considering here above I have to > check (front == request) to proceed to complete it. That's my point, I don't think you need that check, you can complete all requests that have no pending buffer in order until you reach a request that still has pending buffers. > >> + if (front->hasPendingBuffers()) > >> + break; > >> + > >> + pipe_->completeRequest(camera_, front); > >> + front = queuedRequests_.front(); > >> + } > >> } > >> > >> /**
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 8353272642bd..647b4f4f365f 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -729,6 +729,8 @@ int PipelineHandlerIPU3::registerCameras() &IPU3CameraData::imguInputBufferReady); data->imgu_->output_.dev->bufferReady.connect(data.get(), &IPU3CameraData::imguOutputBufferReady); + data->imgu_->viewfinder_.dev->bufferReady.connect(data.get(), + &IPU3CameraData::imguOutputBufferReady); /* Create and register the Camera instance. */ std::string cameraName = cio2->sensor_->entity()->name() + " " @@ -774,10 +776,34 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer) */ void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer) { - Request *request = queuedRequests_.front(); + Request *request = buffer->request(); + if (!request) + /* Completed buffers not part of a request are ignored. */ + return; + + if (!pipe_->completeBuffer(camera_, request, buffer)) + /* Request not completed yet, return here. */ + return; + + /* + * Complete requests in queuing order: if some other request is + * pending, post-pone completion. + */ + Request *front = queuedRequests_.front(); + if (front != request) + return; - pipe_->completeBuffer(camera_, request, buffer); - pipe_->completeRequest(camera_, request); + /* + * Complete the current request, and all the other pending ones, + * in queuing order. + */ + while (1) { + if (front->hasPendingBuffers()) + break; + + pipe_->completeRequest(camera_, front); + front = queuedRequests_.front(); + } } /**
The viewfinder and main output require identical logic for buffer and request completion. Connect the viewfinder bufferReady signal to the slot and handle requests for both main output and viewfinder there. Update the slot logic to ignore internal buffers that are not part of the request, and to complete the request only when the last buffer completes. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 32 +++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)