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

[Kieran Bingham]

Hi Jacopo,

On 22/02/2019 10:33, Jacopo Mondi wrote:
> Hi Jerry,
>         thanks for your comment
> 
> 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' ?

I feel like anything we define statically in the pipeline is just as
wrong as any initial default provided by the hardware ..

--
Kieran




> Thanks
>    j
> 
>> Thanks,
>> -Jerry
>>
>> -----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.
>>
>> Thanks
>>   j
>>
>>>>  	config.bufferCount = 4;
>>>>
>>>>  	configs[&data->stream_] = config;
>>>> --
>>>> 2.20.1
>>>>
>>>> _______________________________________________
>>>> libcamera-devel mailing list
>>>> libcamera-devel at lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>>
>>> --
>>> Regards,
>>> Niklas Söderlund
>>>
>>> _______________________________________________
>>> libcamera-devel mailing list
>>> libcamera-devel at lists.libcamera.org
>>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran