[libcamera-devel,v2,7/7] libcamera: pipeline: ipu3: Pass request metadata to IPA
diff mbox series

Message ID 20210519101954.77711-8-umang.jain@ideasonboard.com
State Superseded
Headers show
Series
  • External IPU3 IPA support
Related show

Commit Message

Umang Jain May 19, 2021, 10:19 a.m. UTC 
From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Pass the request metadata control list to the IPA when setting
passing the statistics buffers, or after the raw buffer completion.

This allows us to pass in the timestamp and other data from the request
which will be used by the IPA.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart May 19, 2021, 4:04 p.m. UTC | #1 
Hi Umang (and Kieran),

Thank you for the patch.

On Wed, May 19, 2021 at 03:49:54PM +0530, Umang Jain wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Pass the request metadata control list to the IPA when setting
> passing the statistics buffers, or after the raw buffer completion.
> 
> This allows us to pass in the timestamp and other data from the request
> which will be used by the IPA.

As commented on the previous version, I think it would be better to pass
the timestamp explicitly.

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 5b15ca90..38f66919 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1285,6 +1285,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>  	ev.op = ipa::ipu3::EventFillParams;
>  	ev.frame = info->id;
>  	ev.bufferId = info->paramBuffer->cookie();
> +	ev.controls = request->metadata();
>  	ipa_->processEvent(ev);
>  }
>  
> @@ -1314,6 +1315,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  	if (!info)
>  		return;
>  
> +	Request *request = info->request;
> +
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
>  		info->metadataProcessed = true;
>  
> @@ -1321,7 +1324,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  		* tryComplete() will delete info if it completes the IPU3Frame.
>  		* In that event, we must have obtained the Request before hand.
>  		*/
> -		Request *request = info->request;
>  
>  		if (frameInfos_.tryComplete(info))
>  			pipe_->completeRequest(request);
> @@ -1333,6 +1335,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  	ev.op = ipa::ipu3::EventStatReady;
>  	ev.frame = info->id;
>  	ev.bufferId = info->statBuffer->cookie();
> +	ev.controls = request->metadata();
>  	ipa_->processEvent(ev);
>  }
>
Kieran Bingham May 21, 2021, 9:17 a.m. UTC | #2 
Hi Me and Others,

On 19/05/2021 17:04, Laurent Pinchart wrote:
> Hi Umang (and Kieran),
> 
> Thank you for the patch.
> 
> On Wed, May 19, 2021 at 03:49:54PM +0530, Umang Jain wrote:
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> Pass the request metadata control list to the IPA when setting
>> passing the statistics buffers, or after the raw buffer completion.
>>
>> This allows us to pass in the timestamp and other data from the request
>> which will be used by the IPA.
> 
> As commented on the previous version, I think it would be better to pass
> the timestamp explicitly.

Yes, that could be ok.

If there are specific things that always need to be passed, this should
be added as an IPA interface sturcture like is done for the
IPAConfigInfo perhaps so it's clear and common.

I don't mind if this patch is just dropped from this series to get the
rest moving.

From what I can tell, only 1/7 needs a small change or a check to
unblock the rest.

And even that could also be done on top perhaps too.

--
Kieran


> 
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 5b15ca90..38f66919 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -1285,6 +1285,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>>  	ev.op = ipa::ipu3::EventFillParams;
>>  	ev.frame = info->id;
>>  	ev.bufferId = info->paramBuffer->cookie();
>> +	ev.controls = request->metadata();
>>  	ipa_->processEvent(ev);
>>  }
>>  
>> @@ -1314,6 +1315,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>  	if (!info)
>>  		return;
>>  
>> +	Request *request = info->request;
>> +
>>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
>>  		info->metadataProcessed = true;
>>  
>> @@ -1321,7 +1324,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>  		* tryComplete() will delete info if it completes the IPU3Frame.
>>  		* In that event, we must have obtained the Request before hand.
>>  		*/
>> -		Request *request = info->request;
>>  
>>  		if (frameInfos_.tryComplete(info))
>>  			pipe_->completeRequest(request);
>> @@ -1333,6 +1335,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>  	ev.op = ipa::ipu3::EventStatReady;
>>  	ev.frame = info->id;
>>  	ev.bufferId = info->statBuffer->cookie();
>> +	ev.controls = request->metadata();
>>  	ipa_->processEvent(ev);
>>  }
>>  
>
Umang Jain May 21, 2021, 12:50 p.m. UTC | #3 
Hi Kieran

On 5/21/21 2:47 PM, Kieran Bingham wrote:
> Hi Me and Others,
>
> On 19/05/2021 17:04, Laurent Pinchart wrote:
>> Hi Umang (and Kieran),
>>
>> Thank you for the patch.
>>
>> On Wed, May 19, 2021 at 03:49:54PM +0530, Umang Jain wrote:
>>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>
>>> Pass the request metadata control list to the IPA when setting
>>> passing the statistics buffers, or after the raw buffer completion.
>>>
>>> This allows us to pass in the timestamp and other data from the request
>>> which will be used by the IPA.
>> As commented on the previous version, I think it would be better to pass
>> the timestamp explicitly.
> Yes, that could be ok.
>
> If there are specific things that always need to be passed, this should
> be added as an IPA interface sturcture like is done for the
> IPAConfigInfo perhaps so it's clear and common.
>
> I don't mind if this patch is just dropped from this series to get the
> rest moving.
>
>  From what I can tell, only 1/7 needs a small change or a check to
> unblock the rest.
>
> And even that could also be done on top perhaps too.
Agreed. Because would also need to work out ipu3-ipa changes along with 
this. It would be better if we address it as a separate task 
altogether.  I am dropping this patch in next iteration
>
> --
> Kieran
>
>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> ---
>>>   src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index 5b15ca90..38f66919 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -1285,6 +1285,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>>>   	ev.op = ipa::ipu3::EventFillParams;
>>>   	ev.frame = info->id;
>>>   	ev.bufferId = info->paramBuffer->cookie();
>>> +	ev.controls = request->metadata();
>>>   	ipa_->processEvent(ev);
>>>   }
>>>   
>>> @@ -1314,6 +1315,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>>   	if (!info)
>>>   		return;
>>>   
>>> +	Request *request = info->request;
>>> +
>>>   	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
>>>   		info->metadataProcessed = true;
>>>   
>>> @@ -1321,7 +1324,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>>   		* tryComplete() will delete info if it completes the IPU3Frame.
>>>   		* In that event, we must have obtained the Request before hand.
>>>   		*/
>>> -		Request *request = info->request;
>>>   
>>>   		if (frameInfos_.tryComplete(info))
>>>   			pipe_->completeRequest(request);
>>> @@ -1333,6 +1335,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>>   	ev.op = ipa::ipu3::EventStatReady;
>>>   	ev.frame = info->id;
>>>   	ev.bufferId = info->statBuffer->cookie();
>>> +	ev.controls = request->metadata();
>>>   	ipa_->processEvent(ev);
>>>   }
>>>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 5b15ca90..38f66919 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1285,6 +1285,7 @@  void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
 	ev.op = ipa::ipu3::EventFillParams;
 	ev.frame = info->id;
 	ev.bufferId = info->paramBuffer->cookie();
+	ev.controls = request->metadata();
 	ipa_->processEvent(ev);
 }
 
@@ -1314,6 +1315,8 @@  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
 	if (!info)
 		return;
 
+	Request *request = info->request;
+
 	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
 		info->metadataProcessed = true;
 
@@ -1321,7 +1324,6 @@  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
 		* tryComplete() will delete info if it completes the IPU3Frame.
 		* In that event, we must have obtained the Request before hand.
 		*/
-		Request *request = info->request;
 
 		if (frameInfos_.tryComplete(info))
 			pipe_->completeRequest(request);
@@ -1333,6 +1335,7 @@  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
 	ev.op = ipa::ipu3::EventStatReady;
 	ev.frame = info->id;
 	ev.bufferId = info->statBuffer->cookie();
+	ev.controls = request->metadata();
 	ipa_->processEvent(ev);
 }