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

[Laurent Pinchart]

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.

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