[libcamera-devel] [PATCH v1 3/3] ipa: rpi: agc: Gracefully handle missing agc modes

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, Jun 07, 2023 at 11:00:54AM +0100, Naushir Patuck via libcamera-devel wrote:
> If a metering/exposure/constraint mode is not listed in the sensor
> tuning file, and a control for the missing mode is set on the agc, we
> terminate the application with a fatal log message.
> 
> Instead of this fatal termination, log a warning message and switch to
> the appropriate default mode so that the application continues running.
> 
> Reported-on: https://github.com/raspberrypi/libcamera/issues/59
> Reported-on: https://github.com/ayufan/camera-streamer/issues/67

We haven't used "Reported-on" before, and grepping in the whole Linux
kernel history there's a single occurrence. The tags used in the kernel
with github issues links are

      1 Ref.
      1 regression
      1 Reported-on
      1 see
      1 URL
      2 Issue
      2 link
      2 Ref
      3 Buglink
      3 Related
      3 Reported-by
      3 Resolves
      5 Bug
      5 References
      6 Fixes
      7 Reference
     17 Addresses-KSPP-ID
     43 See
     89 Closes
    158 BugLink
   1426 Link

We don't use BugLink and have used Link once only to refer to a mailing
list entry, while we use Bug for bugzilla entries. I would vote for Bug
or Link in this case.

> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index b611157af1f0..1b05d478818e 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -540,24 +540,36 @@ void Agc::housekeepConfig()
>  	 */
>  	if (meteringModeName_ != status_.meteringMode) {
>  		auto it = config_.meteringModes.find(meteringModeName_);
> -		if (it == config_.meteringModes.end())
> -			LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_;
> -		meteringMode_ = &it->second;
> +		if (it == config_.meteringModes.end()) {
> +			LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_
> +					     << ", defaulting to " << config_.defaultMeteringMode;
> +			meteringModeName_ = config_.defaultMeteringMode;
> +			meteringMode_ = &config_.meteringModes[meteringModeName_];
> +		} else
> +			meteringMode_ = &it->second;

Please use curly braces here and below. This can be updated when
applying the patch, no need to resubmit.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  		status_.meteringMode = meteringModeName_;
>  	}
>  	if (exposureModeName_ != status_.exposureMode) {
>  		auto it = config_.exposureModes.find(exposureModeName_);
> -		if (it == config_.exposureModes.end())
> -			LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_;
> -		exposureMode_ = &it->second;
> +		if (it == config_.exposureModes.end()) {
> +			LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_
> +					     << ", defaulting to " << config_.defaultExposureMode;
> +			exposureModeName_ = config_.defaultExposureMode;
> +			exposureMode_ = &config_.exposureModes[exposureModeName_];
> +		} else
> +			exposureMode_ = &it->second;
>  		status_.exposureMode = exposureModeName_;
>  	}
>  	if (constraintModeName_ != status_.constraintMode) {
>  		auto it =
>  			config_.constraintModes.find(constraintModeName_);
> -		if (it == config_.constraintModes.end())
> -			LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_;
> -		constraintMode_ = &it->second;
> +		if (it == config_.constraintModes.end()) {
> +			LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_
> +					     << ", defaulting to " << config_.defaultConstraintMode;
> +			constraintModeName_ = config_.defaultConstraintMode;
> +			constraintMode_ = &config_.constraintModes[constraintModeName_];
> +		} else
> +			constraintMode_ = &it->second;
>  		status_.constraintMode = constraintModeName_;
>  	}
>  	LOG(RPiAgc, Debug) << "exposureMode "

-- 
Regards,

Laurent Pinchart