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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Feb 22, 2019 at 01:05:15PM +0100, Jacopo Mondi wrote:
> On Fri, Feb 22, 2019 at 01:44:14PM +0200, Laurent Pinchart wrote:
> > 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.
> 
> True indeed, but if I'm not mistaken, the ImgU has a default
> programmed to be 1920x1080, not all sensors can provide that
> resolution (yes, I understand, it's a corner
> case, most modern sensors can, but I feel this is worth checking,
> isn't it?)

But the default ImgU settings don't matter at all :-) From an
application point of view the ImgU isn't visible, we have one or more
cameras, with one or more streams each, and we need to report a default
configuration for those streams. That's only dependent on the sensor,
not the ImgU (or, to be exact, the only dependencies on the ImgU are
min/max resolution and min/max scaling factor).

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