[libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration

[Kieran Bingham]

Hi Laurent, Jacopo,

On 22/02/2019 11:44, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Fri, Feb 22, 2019 at 12:08:42PM +0100, Jacopo Mondi wrote:
>> On Fri, Feb 22, 2019 at 10:57:35AM +0000, Kieran Bingham wrote:
>>> On 22/02/2019 10:33, Jacopo Mondi wrote:
>>>> On Fri, Feb 22, 2019 at 02:30:09AM +0000, Hu, Jerry W wrote:
>>>>> Hi,
>>>>>
>>>>> May I know why hard coded the width and height?
>>>>>>> +	config.width = 2560;
>>>>>>> +	config.height = 1920;
>>>>>>> +	config.pixelFormat = V4L2_PIX_FMT_NV12;
>>>>
>>>> This happens in the 'streamConfiguration()' methods, which
>>>> applications use to obtain a known good default resolution and image
>>>> format the camera can provide. In this case I've selected 2560x1920
>>>> NV12 format as "default" for the single stream the IPU3 camera
>>>> provides at the moment.
>>>>
>>>> Once application receives a known default format it is then free to
>>>> change any parameter of that, and provide it to libcamera using the
>>>> Camera::configureStreams() methods. The Camera instance will pass the
>>>> configurations to the pipeline handlers, that wil try to apply it to
>>>> the hardware.
>>>>
>>>> To sum up: we need to provide to applications a default configuration
>>>> suitable for the hardware, and that's why we hardcode those values
>>>> here. Starting from that default configuration, applications can
>>>> configure streams with the desired width, height and pixel format and
>>>> ask libcamera to apply it to the hardware.
>>>>
>>>> Does this clarify your concerns?
>>>
>>> I think it was discussed somewhere but I'm not sure where, but what is
>>> the reason why we can't ask the hardware for it's "current" format to
>>> set the 'default' ?
>>
>> Because at the current stage the 'hardware' is both the sensor
>> provided resolution and the output resolution provided by the ImgU,
>> which do not match, if we use their defaults. And as images will be
>> provided from ImgU not the CIO2 as before, I cannot just claim the
>> format applied on the sensor and report that.
> 
> The ImgU is a memory to memory device, it can operate on pretty much any
> input resolution, includes a scaler, and can crop. The default
> resolution should come from the sensor, not the ImgU.

How about to provide a default, we obtain the resolution from the
Sensor, (I assume this is constant-ish?) and the rest from the ImgU ?

So we would return a default of a non-scaled capture but with a pixel
format supported by the ImgU output?


>> That resolution is known to work for the current use case, and I put
>> it there not to break capture. Later on, with full ImgU support should
>> westart from the sensor resolution and adjust it to one the ImgU can
>> provide, or start from the default one applied to the ImgU unit, and
>> make sure the sensor can provide something big enough to fit into
>> that?
>>
>> Also, I know realize this was probably what Jerry was asking in is
>> email, not an explanation of the stream format negotiation
>> procedure...
>>
>>> I feel like anything we define statically in the pipeline is just as
>>> wrong as any initial default provided by the hardware ..
>>
>> Why do you consider initial defaults provided by the hardware wrong? I
>> mean, the issue here to me is "which default", but somewhere it has to
>> be defined, right?

Yes, I think I was essentially saying the same thing as Laurent in
another e-mail ...

(
> The current format isn't a good idea, as it can vary based on the
> history of device usage, and is thus not suitable as a default. I
> however agree that we should compute a reasonable default based on the
> hardware capabilities.
)




>>
>>>>> -----Original Message-----
>>>>> From: libcamera-devel [mailto:libcamera-devel-bounces at lists.libcamera.org] On Behalf Of Jacopo Mondi
>>>>> Sent: Friday, February 22, 2019 12:21 AM
>>>>> To: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>>>> Cc: libcamera-devel at lists.libcamera.org
>>>>> Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration
>>>>>
>>>>> Hi Niklas,
>>>>>
>>>>> On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote:
>>>>>> Hi Jacopo,
>>>>>>
>>>>>> Thanks for your patch.
>>>>>>
>>>>>> On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote:
>>>>>>> Return default configuration for the output stream produced by the
>>>>>>> imgu 'output' video node.
>>>>>>>
>>>>>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>>>>>> ---
>>>>>>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++-------------
>>>>>>>  1 file changed, 3 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>>>> index 9694d0ce51ab..9065073913a2 100644
>>>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>>>> @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera
>>>>>>> *camera,  {
>>>>>>>  	IPU3CameraData *data = cameraData(camera);
>>>>>>>  	std::map<Stream *, StreamConfiguration> configs;
>>>>>>> -	V4L2SubdeviceFormat format = {};
>>>>>>> -
>>>>>>> -	/*
>>>>>>> -	 * FIXME: As of now, return the image format reported by the sensor.
>>>>>>> -	 * In future good defaults should be provided for each stream.
>>>>>>> -	 */
>>>>>>> -	if (data->sensor_->getFormat(0, &format)) {
>>>>>>> -		LOG(IPU3, Error) << "Failed to create stream configurations";
>>>>>>> -		return configs;
>>>>>>> -	}
>>>>>>>
>>>>>>>  	StreamConfiguration config = {};
>>>>>>> -	config.width = format.width;
>>>>>>> -	config.height = format.height;
>>>>>>> -	config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
>>>>>>> +	config.width = 2560;
>>>>>>> +	config.height = 1920;
>>>>>>> +	config.pixelFormat = V4L2_PIX_FMT_NV12;
>>>>>>
>>>>>> This is a good change and trust that the format you selected are the
>>>>>> correct one so I have not verified it by reading or testing on the IPU3.
>>>>>>
>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>>>>>
>>>>>> I'm just curious why the pixelformat changes, did something change in
>>>>>> the upstream driver or is this in preparation of some later change?
>>>>>>
>>>>>
>>>>> We're preparing to provide frames to the application from the IMGU not from the CIO2 unit. The output format of the two is different.
>>>>>
>>>>>>>  	config.bufferCount = 4;
>>>>>>>
>>>>>>>  	configs[&data->stream_] = config;
> 

-- 
Regards
--
Kieran