| Message ID | 20241021133718.894374-2-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2024-10-21 14:37:16) > Let's extract the two occurrences of canceling a request to a common > helper. This is especially useful for the followup patch, which needs > to cancel a request from outside. Same as v4: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> This series fixes the assertion at shutdown with a large buffer count: Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > include/libcamera/internal/pipeline_handler.h | 1 + > src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------ > 2 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 0d3808036..fb28a18d0 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -60,6 +60,7 @@ public: > > bool completeBuffer(Request *request, FrameBuffer *buffer); > void completeRequest(Request *request); > + void cancelRequest(Request *request); > > std::string configurationFile(const std::string &subdir, > const std::string &name) const; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index e59404691..c9cb11f0f 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera) > while (!waitingRequests_.empty()) { > Request *request = waitingRequests_.front(); > waitingRequests_.pop(); > - > - request->_d()->cancel(); > - completeRequest(request); > + cancelRequest(request); > } > > /* Make sure no requests are pending. */ > @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request) > } > > int ret = queueRequestDevice(camera, request); > - if (ret) { > - request->_d()->cancel(); > - completeRequest(request); > - } > + if (ret) > + cancelRequest(request); > } > > /** > @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request) > } > } > > +/** > + * \brief Cancel request and signal its completion > + * \param[in] request The request to cancel > + * > + * This function cancels the request in addition to its completion. The same > + * rules as for completeRequest() apply. > + */ > +void PipelineHandler::cancelRequest(Request *request) > +{ > + request->_d()->cancel(); > + completeRequest(request); > +} > + > /** > * \brief Retrieve the absolute path to a platform configuration file > * \param[in] subdir The pipeline handler specific subdirectory name > -- > 2.44.1 >
Hi, On 21-Oct-24 3:37 PM, Milan Zamazal wrote: > Let's extract the two occurrences of canceling a request to a common > helper. This is especially useful for the followup patch, which needs > to cancel a request from outside. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > include/libcamera/internal/pipeline_handler.h | 1 + > src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------ > 2 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 0d3808036..fb28a18d0 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -60,6 +60,7 @@ public: > > bool completeBuffer(Request *request, FrameBuffer *buffer); > void completeRequest(Request *request); > + void cancelRequest(Request *request); > > std::string configurationFile(const std::string &subdir, > const std::string &name) const; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index e59404691..c9cb11f0f 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera) > while (!waitingRequests_.empty()) { > Request *request = waitingRequests_.front(); > waitingRequests_.pop(); > - > - request->_d()->cancel(); > - completeRequest(request); > + cancelRequest(request); > } > > /* Make sure no requests are pending. */ > @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request) > } > > int ret = queueRequestDevice(camera, request); > - if (ret) { > - request->_d()->cancel(); > - completeRequest(request); > - } > + if (ret) > + cancelRequest(request); > } > > /** > @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request) > } > } > > +/** > + * \brief Cancel request and signal its completion > + * \param[in] request The request to cancel > + * > + * This function cancels the request in addition to its completion. The same > + * rules as for completeRequest() apply. > + */ > +void PipelineHandler::cancelRequest(Request *request) > +{ > + request->_d()->cancel(); > + completeRequest(request); > +} > + > /** > * \brief Retrieve the absolute path to a platform configuration file > * \param[in] subdir The pipeline handler specific subdirectory name
Hi Milan, Thank you for the patch. On Mon, Oct 21, 2024 at 03:37:16PM +0200, Milan Zamazal wrote: > Let's extract the two occurrences of canceling a request to a common > helper. This is especially useful for the followup patch, which needs > to cancel a request from outside. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > include/libcamera/internal/pipeline_handler.h | 1 + > src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------ > 2 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 0d3808036..fb28a18d0 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -60,6 +60,7 @@ public: > > bool completeBuffer(Request *request, FrameBuffer *buffer); > void completeRequest(Request *request); > + void cancelRequest(Request *request); > > std::string configurationFile(const std::string &subdir, > const std::string &name) const; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index e59404691..c9cb11f0f 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera) > while (!waitingRequests_.empty()) { > Request *request = waitingRequests_.front(); > waitingRequests_.pop(); > - > - request->_d()->cancel(); > - completeRequest(request); > + cancelRequest(request); > } > > /* Make sure no requests are pending. */ > @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request) > } > > int ret = queueRequestDevice(camera, request); > - if (ret) { > - request->_d()->cancel(); > - completeRequest(request); > - } > + if (ret) > + cancelRequest(request); > } > > /** > @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request) > } > } > > +/** > + * \brief Cancel request and signal its completion > + * \param[in] request The request to cancel > + * > + * This function cancels the request in addition to its completion. The same "in additiona to its completion" confuses me. I would simply write * This function cancels and completes the request. The same rules as for * completeRequest() apply. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Although, I wonder if we could instead add a Request::Status argument to completeRequest(), default it to Request::Status::RequestComplete, and avoid adding a new function. I don't mind much either way. > + * rules as for completeRequest() apply. > + */ > +void PipelineHandler::cancelRequest(Request *request) > +{ > + request->_d()->cancel(); > + completeRequest(request); > +} > + > /** > * \brief Retrieve the absolute path to a platform configuration file > * \param[in] subdir The pipeline handler specific subdirectory name
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Mon, Oct 21, 2024 at 03:37:16PM +0200, Milan Zamazal wrote: >> Let's extract the two occurrences of canceling a request to a common >> helper. This is especially useful for the followup patch, which needs >> to cancel a request from outside. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> include/libcamera/internal/pipeline_handler.h | 1 + >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------ >> 2 files changed, 17 insertions(+), 7 deletions(-) >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h >> index 0d3808036..fb28a18d0 100644 >> --- a/include/libcamera/internal/pipeline_handler.h >> +++ b/include/libcamera/internal/pipeline_handler.h >> @@ -60,6 +60,7 @@ public: >> >> bool completeBuffer(Request *request, FrameBuffer *buffer); >> void completeRequest(Request *request); >> + void cancelRequest(Request *request); >> >> std::string configurationFile(const std::string &subdir, >> const std::string &name) const; >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >> index e59404691..c9cb11f0f 100644 >> --- a/src/libcamera/pipeline_handler.cpp >> +++ b/src/libcamera/pipeline_handler.cpp >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera) >> while (!waitingRequests_.empty()) { >> Request *request = waitingRequests_.front(); >> waitingRequests_.pop(); >> - >> - request->_d()->cancel(); >> - completeRequest(request); >> + cancelRequest(request); >> } >> >> /* Make sure no requests are pending. */ >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request) >> } >> >> int ret = queueRequestDevice(camera, request); >> - if (ret) { >> - request->_d()->cancel(); >> - completeRequest(request); >> - } >> + if (ret) >> + cancelRequest(request); >> } >> >> /** >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request) >> } >> } >> >> +/** >> + * \brief Cancel request and signal its completion >> + * \param[in] request The request to cancel >> + * >> + * This function cancels the request in addition to its completion. The same > > "in additiona to its completion" confuses me. I would simply write > > * This function cancels and completes the request. The same rules as for > * completeRequest() apply. Indeed, this sounds better. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Although, I wonder if we could instead add a Request::Status argument to > completeRequest(), default it to Request::Status::RequestComplete, and > avoid adding a new function. I don't mind much either way. A Request::Status argument could cause questions about possible Request::Status::Pending value. Which could be avoided by using something like `bool cancel' argument instead but I think it's better to stop here and leave things as they are; other than that I also don't mind much either way. >> + * rules as for completeRequest() apply. >> + */ >> +void PipelineHandler::cancelRequest(Request *request) >> +{ >> + request->_d()->cancel(); >> + completeRequest(request); >> +} >> + >> /** >> * \brief Retrieve the absolute path to a platform configuration file >> * \param[in] subdir The pipeline handler specific subdirectory name
On Wed, Nov 06, 2024 at 09:25:41PM +0100, Milan Zamazal wrote: > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > > On Mon, Oct 21, 2024 at 03:37:16PM +0200, Milan Zamazal wrote: > >> Let's extract the two occurrences of canceling a request to a common > >> helper. This is especially useful for the followup patch, which needs > >> to cancel a request from outside. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> include/libcamera/internal/pipeline_handler.h | 1 + > >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------ > >> 2 files changed, 17 insertions(+), 7 deletions(-) > >> > >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > >> index 0d3808036..fb28a18d0 100644 > >> --- a/include/libcamera/internal/pipeline_handler.h > >> +++ b/include/libcamera/internal/pipeline_handler.h > >> @@ -60,6 +60,7 @@ public: > >> > >> bool completeBuffer(Request *request, FrameBuffer *buffer); > >> void completeRequest(Request *request); > >> + void cancelRequest(Request *request); > >> > >> std::string configurationFile(const std::string &subdir, > >> const std::string &name) const; > >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > >> index e59404691..c9cb11f0f 100644 > >> --- a/src/libcamera/pipeline_handler.cpp > >> +++ b/src/libcamera/pipeline_handler.cpp > >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera) > >> while (!waitingRequests_.empty()) { > >> Request *request = waitingRequests_.front(); > >> waitingRequests_.pop(); > >> - > >> - request->_d()->cancel(); > >> - completeRequest(request); > >> + cancelRequest(request); > >> } > >> > >> /* Make sure no requests are pending. */ > >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request) > >> } > >> > >> int ret = queueRequestDevice(camera, request); > >> - if (ret) { > >> - request->_d()->cancel(); > >> - completeRequest(request); > >> - } > >> + if (ret) > >> + cancelRequest(request); > >> } > >> > >> /** > >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request) > >> } > >> } > >> > >> +/** > >> + * \brief Cancel request and signal its completion > >> + * \param[in] request The request to cancel > >> + * > >> + * This function cancels the request in addition to its completion. The same > > > > "in additiona to its completion" confuses me. I would simply write > > > > * This function cancels and completes the request. The same rules as for > > * completeRequest() apply. > > Indeed, this sounds better. > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Although, I wonder if we could instead add a Request::Status argument to > > completeRequest(), default it to Request::Status::RequestComplete, and > > avoid adding a new function. I don't mind much either way. > > A Request::Status argument could cause questions about possible > Request::Status::Pending value. Which could be avoided by using > something like `bool cancel' argument instead but I think it's better to > stop here and leave things as they are; other than that I also don't > mind much either way. I was also considering the Pending issue, and I don't like boolean arguments much in this case, as they're not very readable. Does completeRequest(request, true); cancel the request or complete it successfully ? Let's leave things as-is. > >> + * rules as for completeRequest() apply. > >> + */ > >> +void PipelineHandler::cancelRequest(Request *request) > >> +{ > >> + request->_d()->cancel(); > >> + completeRequest(request); > >> +} > >> + > >> /** > >> * \brief Retrieve the absolute path to a platform configuration file > >> * \param[in] subdir The pipeline handler specific subdirectory name
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index 0d3808036..fb28a18d0 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -60,6 +60,7 @@ public: bool completeBuffer(Request *request, FrameBuffer *buffer); void completeRequest(Request *request); + void cancelRequest(Request *request); std::string configurationFile(const std::string &subdir, const std::string &name) const; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index e59404691..c9cb11f0f 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera) while (!waitingRequests_.empty()) { Request *request = waitingRequests_.front(); waitingRequests_.pop(); - - request->_d()->cancel(); - completeRequest(request); + cancelRequest(request); } /* Make sure no requests are pending. */ @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request) } int ret = queueRequestDevice(camera, request); - if (ret) { - request->_d()->cancel(); - completeRequest(request); - } + if (ret) + cancelRequest(request); } /** @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request) } } +/** + * \brief Cancel request and signal its completion + * \param[in] request The request to cancel + * + * This function cancels the request in addition to its completion. The same + * rules as for completeRequest() apply. + */ +void PipelineHandler::cancelRequest(Request *request) +{ + request->_d()->cancel(); + completeRequest(request); +} + /** * \brief Retrieve the absolute path to a platform configuration file * \param[in] subdir The pipeline handler specific subdirectory name
Let's extract the two occurrences of canceling a request to a common helper. This is especially useful for the followup patch, which needs to cancel a request from outside. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- include/libcamera/internal/pipeline_handler.h | 1 + src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------ 2 files changed, 17 insertions(+), 7 deletions(-)