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

[Laurent Pinchart]

Hi Kieran,

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' ?

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.

> I feel like anything we define statically in the pipeline is just as
> wrong as any initial default provided by the hardware ..
> 
> >> -----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,

Laurent Pinchart