[libcamera-devel] [PATCH v2 8/8] libcamera: pipeline: ipu3: Fail if the FrameInfo can't be found

[Kieran Bingham]

Hi Laurent,

On 13/03/2021 23:13, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Mar 12, 2021 at 05:47:27AM +0000, Kieran Bingham wrote:
>> The FrameInfo structure associates the data sent to the IPA
>> and is essential for handling events.
>>
>> If it can not be found, this is a fatal error which must be fixed.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index c5facaea16dd..a61e2b51ef9e 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -1168,8 +1168,10 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
>>  	}
>>  	case ipa::ipu3::ActionParamFilled: {
>>  		IPU3Frames::Info *info = frameInfos_.find(id);
>> -		if (!info)
>> +		if (!info) {
>> +			LOG(IPU3, Fatal) << "No frameInfo for ActionParamFilled on ID: " << id;
> 
> s/ID:/ID/ ?
> 
> Let's shorten lines a bit.
> 
> 			LOG(IPU3, Fatal)
> 				<< "No frameInfo for ActionParamFilled on ID "
> 				<< id;
> 
> Same below.
> 
> This being said, couldn't we more simply turn the existing Error message
> in IPU3Frames::find(unsigned int id) into a Fatal message ?

I think I hadn't done that because I thought that there were legitimate
reasons for not being able to find a FrameInfo for a buffer. But I don't
believe there is.

FrameInfo is not removed until the request is completed, so any failure
to find it is fatal.

Having the internal structure associated with a Request::Private
internal pointer would therefore save lookups and be a (nearish) future
improvement.

I'll make this fully fatal and retest everything.


> I also wonder if the same shouldn't be done for
> IPU3Frames::find(FrameBuffer *buffer).
> 
>>  			break;
>> +		}
>>  
>>  		/* Queue all buffers from the request aimed for the ImgU. */
>>  		for (auto it : info->request->buffers()) {
>> @@ -1190,8 +1192,10 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
>>  	}
>>  	case ipa::ipu3::ActionMetadataReady: {
>>  		IPU3Frames::Info *info = frameInfos_.find(id);
>> -		if (!info)
>> +		if (!info) {
>> +			LOG(IPU3, Fatal) << "No frameInfo for ActionMetadataReady on ID: " << id;
>>  			break;
>> +		}
>>  
>>  		/*
>>  		 * \todo Parse the value of the controls returned by the IPA
> 

-- 
Regards
--
Kieran