[libcamera-devel] [PATCH 03/12] libcamera: ipu3: Register Exposure control

[Jacopo Mondi]

Hi Laurent,

On Mon, Jan 11, 2021 at 12:22:54AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> > +	 */
> > +	float pixelRate = sensorInfo.pixelRate / 1e6f;
> > +	const ControlInfoMap &sensorControls = sensor->controls();
> > +	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> > +	int32_t minExposure = v4l2Exposure.min().get<int32_t>()
> > +			    * sensorInfo.lineLength / pixelRate;
> > +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>()
> > +			    * sensorInfo.lineLength / pixelRate;
> > +	int32_t defExposure = v4l2Exposure.def().get<int32_t>()
> > +			    * sensorInfo.lineLength / pixelRate;
>
> For long exposure time this will overflow as the first two operands are
> 32-bit integers. How about the following ?
>
> 	double lineDuration = sensorInfo.pixelRate / 1e6f /pixelRate;

did you mean:
        sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6f)
or rather
        sensorInfo.lineLength * 1e6f / sensorInfo.pixelRate

but there's an overflow risk as well here.

> 	...
> 	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
> 	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
> 	int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration;
>
> > +
> > +	/*
> > +	 * \todo Report the actual exposure time, use the default for the
> > +	 * moment.
> > +	 */
> > +	data->exposureTime_ = defExposure;
> > +
> > +	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> > +							defExposure);
> > +
> > +	data->controlInfo_ = std::move(controls);
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \brief Initialise ImgU and CIO2 devices associated with cameras
> >   *
> > @@ -776,8 +833,9 @@ int PipelineHandlerIPU3::registerCameras()
> >  		/* Initialize the camera properties. */
> >  		data->properties_ = cio2->sensor()->properties();
> >
> > -		/* Initialze the camera controls. */
> > -		data->controlInfo_ = IPU3Controls;
> > +		ret = initControls(data.get());
> > +		if (ret)
> > +			continue;
> >
> >  		/**
> >  		 * \todo Dynamically assign ImgU and output devices to each
> > @@ -842,6 +900,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >
> >  	/* Mark the request as complete. */
> >  	request->metadata().set(controls::draft::PipelineDepth, 3);
>
> 	/* \todo Move the ExposureTime control to the IPA */
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>

Thanks
  j

> > +	request->metadata().set(controls::ExposureTime, exposureTime_);
> >  	pipe_->completeRequest(request);
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart