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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Feb 22, 2019 at 11:33:05AM +0100, 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.

As mentioned in a previous e-mail I think you will have to make this
dynamic. The above resolution may be fine on the hardware we use for
testing the IPU3, but it needs to be sensor-dependent.

> Does this clarify your concerns?
> 
> > -----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