[v3,1/2] libcamera: pipeline_handler: Provide cancelRequest

21564 diff mbox series

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(-)

Quoting Milan Zamazal (2024-10-09 18:21:07)
> 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
> + * rules as for completeRequest() apply.



> + */
> +void PipelineHandler::cancelRequest(Request *request)
> +{

I think I saw a comment from Laurent about what happens if the request
is already cancelled...

Is this an opportune location that should do any extra checks? Or
actually - I think completeRequest() probably already does that in it's
call to request->_d()->complete(), where it has an
ASSERT(request->status() == RequestPending); so its' already covered.


And anyway - as a clean up - it still stands alone for the above
duplications so I think:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +       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
>

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Milan Zamazal (2024-10-09 18:21:07)
>> 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
>> + * rules as for completeRequest() apply.
>
>
>
>> + */
>> +void PipelineHandler::cancelRequest(Request *request)
>> +{
>
> I think I saw a comment from Laurent about what happens if the request
> is already cancelled...

This has been addressed in the followup patch in v3.

> Is this an opportune location that should do any extra checks? Or
> actually - I think completeRequest() probably already does that in it's
> call to request->_d()->complete(), where it has an
> ASSERT(request->status() == RequestPending); so its' already covered.

Yes, that check is enough IMO.

> And anyway - as a clean up - it still stands alone for the above
> duplications so I think:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>> +       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
>>