[libcamera-devel] [PATCH] ipa: ipu3: List the events in their order of calling

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-11-05 15:20:31)
> This is a cleanup patch, no functionnal behaviour changes. It makes it
> clearer when an event is called.

How about:
"""
The IPU3 IPA has three events which are handled from the pipeline
handler.

The events are received in the sequence, EventProcessControls,
EventFillParams, and finally EventStatReady, while the code lists these
in a different order.

Update the flow of IPAIPU3::processEvent() to match the expected
sequence of events, to help support the reader in interpreting the flow
of events through the IPA.
"""
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 27 +++++++++++++++++----------

How about updating include/libcamera/ipa/ipu3.mojom to re-order the enum
definitions as well:

	EventProcessControls = 4,
	EventStatReady = 5,
	EventFillParams = 6,

may as well become:

	EventProcessControls = 4,
	EventFillParams = 5,
	EventStatReady = 6,


>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 5c51607d..d97b2f9e 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -509,6 +509,13 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>  /**
>   * \brief Process an event generated by the pipeline handler
>   * \param[in] event The event sent from pipeline handler
> + *
> + * The expected event handling over the lifetime of a Request has
> + * the following sequence:
> + *
> + *   - EventProcessControls : Handle controls from a new Request
> + *   - EventFillParams : Prepare the ISP to process the request

Either s/request/Request/ or s/Request/request/ in the line above it.

With those.


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

> + *   - EventStatReady : Process statistics after ISP completion
>   */
>  void IPAIPU3::processEvent(const IPU3Event &event)
>  {
> @@ -517,32 +524,32 @@ void IPAIPU3::processEvent(const IPU3Event &event)
>                 processControls(event.frame, event.controls);
>                 break;
>         }
> -       case EventStatReady: {
> +       case EventFillParams: {
>                 auto it = buffers_.find(event.bufferId);
>                 if (it == buffers_.end()) {
> -                       LOG(IPAIPU3, Error) << "Could not find stats buffer!";
> +                       LOG(IPAIPU3, Error) << "Could not find param buffer!";
>                         return;
>                 }
>  
>                 Span<uint8_t> mem = it->second.planes()[0];
> -               const ipu3_uapi_stats_3a *stats =
> -                       reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
> +               ipu3_uapi_params *params =
> +                       reinterpret_cast<ipu3_uapi_params *>(mem.data());
>  
> -               parseStatistics(event.frame, event.frameTimestamp, stats);
> +               fillParams(event.frame, params);
>                 break;
>         }
> -       case EventFillParams: {
> +       case EventStatReady: {
>                 auto it = buffers_.find(event.bufferId);
>                 if (it == buffers_.end()) {
> -                       LOG(IPAIPU3, Error) << "Could not find param buffer!";
> +                       LOG(IPAIPU3, Error) << "Could not find stats buffer!";
>                         return;
>                 }
>  
>                 Span<uint8_t> mem = it->second.planes()[0];
> -               ipu3_uapi_params *params =
> -                       reinterpret_cast<ipu3_uapi_params *>(mem.data());
> +               const ipu3_uapi_stats_3a *stats =
> +                       reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>  
> -               fillParams(event.frame, params);
> +               parseStatistics(event.frame, event.frameTimestamp, stats);
>                 break;
>         }
>         default:
> -- 
> 2.32.0
>