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

[Jacopo Mondi]

Hi Laurent,

On Fri, Feb 22, 2019 at 01:44:14PM +0200, 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.
>

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

Thanks
  j


> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190222/951c9296/attachment-0001.sig>