Re: [PATCH 4/7] media: atmel: atmel-isc: add support for DO_WHITE_BALANCE

[<Eugen.Hristev@microchip.com>]

On 10.04.2019 17:26, Hans Verkuil wrote:

> 
> On 4/9/19 1:07 PM, Eugen.Hristev@microchip.com wrote:
>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>
>> This adds support for the 'button' control DO_WHITE_BALANCE
>> This feature will enable the ISC to compute the white balance coefficients
>> in a one time shot, at the user discretion.
>> This can be used if a color chart/grey chart is present in front of the camera.
>> The ISC will adjust the coefficients and have them fixed until next balance
>> or until sensor mode is changed.
>> This is particularly useful for white balance adjustment in different
>> lighting scenarios, and then taking photos to similar scenery.
>> The old auto white balance stays in place, where the ISC will adjust every
>> 4 frames to the current scenery lighting, if the scenery is approximately
>> grey in average, otherwise grey world algorithm fails.
>> One time white balance adjustments needs streaming to be enabled, such that
>> capture is enabled and the histogram has data to work with.
>> Histogram without capture does not work in this hardware module.
>>
>> To disable auto white balance feature (first step)
>> v4l2-ctl --set-ctrl=white_balance_automatic=0
>>
>> To start the one time white balance procedure:
>> v4l2-ctl --set-ctrl=do_white_balance=1
>>
>> User controls now include the do_white_balance ctrl:
>> User Controls
>>
>>                       brightness 0x00980900 (int)    : min=-1024 max=1023 step=1 default=0 value=0 flags=slider
>>                         contrast 0x00980901 (int)    : min=-2048 max=2047 step=1 default=256 value=256 flags=slider
>>          white_balance_automatic 0x0098090c (bool)   : default=1 value=1
>>                 do_white_balance 0x0098090d (button) : flags=write-only, execute-on-write
>>                            gamma 0x00980910 (int)    : min=0 max=2 step=1 default=2 value=2 flags=slider
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>>   drivers/media/platform/atmel/atmel-isc.c | 74 +++++++++++++++++++++++++++++---
>>   1 file changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
>> index f6b8b00e..e516805 100644
>> --- a/drivers/media/platform/atmel/atmel-isc.c
>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>> @@ -167,6 +167,9 @@ struct isc_ctrls {
>>   	u32 brightness;
>>   	u32 contrast;
>>   	u8 gamma_index;
>> +#define ISC_WB_NONE	0
>> +#define ISC_WB_AUTO	1
>> +#define ISC_WB_ONETIME	2
>>   	u8 awb;
>>   
>>   	/* one for each component : GR, R, GB, B */
>> @@ -210,6 +213,7 @@ struct isc_device {
>>   	struct fmt_config	try_config;
>>   
>>   	struct isc_ctrls	ctrls;
>> +	struct v4l2_ctrl	*do_wb_ctrl;
>>   	struct work_struct	awb_work;
>>   
>>   	struct mutex		lock;
>> @@ -809,7 +813,7 @@ static void isc_set_pipeline(struct isc_device *isc, u32 pipeline)
>>   
>>   	bay_cfg = isc->config.sd_format->cfa_baycfg;
>>   
>> -	if (!ctrls->awb)
>> +	if (ctrls->awb == ISC_WB_NONE)
>>   		isc_reset_awb_ctrls(isc);
>>   
>>   	regmap_write(regmap, ISC_WB_CFG, bay_cfg);
>> @@ -1928,7 +1932,7 @@ static void isc_awb_work(struct work_struct *w)
>>   	baysel = isc->config.sd_format->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
>>   
>>   	/* if no more auto white balance, reset controls. */
>> -	if (!ctrls->awb)
>> +	if (ctrls->awb == ISC_WB_NONE)
>>   		isc_reset_awb_ctrls(isc);
>>   
>>   	pm_runtime_get_sync(isc->dev);
>> @@ -1937,7 +1941,7 @@ static void isc_awb_work(struct work_struct *w)
>>   	 * only update if we have all the required histograms and controls
>>   	 * if awb has been disabled, we need to reset registers as well.
>>   	 */
>> -	if (hist_id == ISC_HIS_CFG_MODE_GR || !ctrls->awb) {
>> +	if (hist_id == ISC_HIS_CFG_MODE_GR || ctrls->awb == ISC_WB_NONE) {
>>   		/*
>>   		 * It may happen that DMA Done IRQ will trigger while we are
>>   		 * updating white balance registers here.
>> @@ -1947,6 +1951,16 @@ static void isc_awb_work(struct work_struct *w)
>>   		spin_lock_irqsave(&isc->awb_lock, flags);
>>   		isc_update_awb_ctrls(isc);
>>   		spin_unlock_irqrestore(&isc->awb_lock, flags);
>> +
>> +		/*
>> +		 * if we are doing just the one time white balance adjustment,
>> +		 * we are basically done.
>> +		 */
>> +		if (ctrls->awb == ISC_WB_ONETIME) {
>> +			v4l2_info(&isc->v4l2_dev,
>> +				  "Completed one time white-balance adjustment.\n");
>> +			ctrls->awb = ISC_WB_NONE;
>> +		}
>>   	}
>>   	regmap_write(regmap, ISC_HIS_CFG, hist_id | baysel | ISC_HIS_CFG_RAR);
>>   	isc_update_profile(isc);
>> @@ -1974,10 +1988,56 @@ static int isc_s_ctrl(struct v4l2_ctrl *ctrl)
>>   		ctrls->gamma_index = ctrl->val;
>>   		break;
>>   	case V4L2_CID_AUTO_WHITE_BALANCE:
>> -		ctrls->awb = ctrl->val;
>> +		if (ctrl->val == 1) {
>> +			ctrls->awb = ISC_WB_AUTO;
>> +			v4l2_ctrl_activate(isc->do_wb_ctrl, false);
>> +		} else {
>> +			ctrls->awb = ISC_WB_NONE;
>> +			v4l2_ctrl_activate(isc->do_wb_ctrl, true);
>> +		}
>> +		/* we did not configure ISC yet */
>> +		if (!isc->config.sd_format)
>> +			break;
>> +
>> +		if (!ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code)) {
>> +			v4l2_err(&isc->v4l2_dev,
>> +				 "White balance adjustments available only if sensor is in RAW mode.\n");
> 
> This isn't an error, instead if the format isn't raw, then deactivate
> the control (see v4l2_ctrl_activate()). That way the control framework
> will handle this.
> 
>> +			return 0;
>> +		}
>> +
>>   		if (ctrls->hist_stat != HIST_ENABLED) {
>>   			isc_reset_awb_ctrls(isc);
>>   		}
>> +
>> +		if (isc->ctrls.awb && vb2_is_streaming(&isc->vb2_vidq) &&
>> +		    ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code))
>> +			isc_set_histogram(isc, true);
>> +
>> +		break;
>> +	case V4L2_CID_DO_WHITE_BALANCE:
>> +		/* we did not configure ISC yet */
>> +		if (!isc->config.sd_format)
>> +			break;
>> +
>> +		if (ctrls->awb == ISC_WB_AUTO) {
>> +			v4l2_err(&isc->v4l2_dev,
>> +				 "To use one time white-balance adjustment, disable auto white balance first.\n");
> 
> I'd do this differently: if auto whitebalance is already on, then just do
> nothing for V4L2_CID_DO_WHITE_BALANCE.
> 
>> +			return -EAGAIN;
>> +		}
>> +		if (!vb2_is_streaming(&isc->vb2_vidq)) {
>> +			v4l2_err(&isc->v4l2_dev,
>> +				 "One time white-balance adjustment requires streaming to be enabled.\n");
> 
> This too should use v4l2_ctrl_activate(): activate the control in start_streaming,
> deactivate in stop_streaming (and when the control is created).
> 
>> +			return -EAGAIN;
>> +		}
>> +
>> +		if (!ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code)) {
>> +			v4l2_err(&isc->v4l2_dev,
>> +				 "White balance adjustments available only if sensor is in RAW mode.\n");
> 
> Same note as above: use v4l2_ctrl_activate() for this.

Hello Hans,

I used v4l2_ctrl_activate with false parameter, and the v4l2-ctl -l 
looks like this:


            do_white_balance (button) : flags=inactive, write-only, 
execute-on-write

But the inactive flag looks to be only for display purposes, as issuing :

v4l2-ctl --set-ctrl=do_white_balance=1

will continue to call my ctrl callback as if the control is still active.

Am I missing something here ? v4l2_s_ctrl does not check for INACTIVE 
status.

Thanks

> 
>> +			return -EAGAIN;
>> +		}
>> +		ctrls->awb = ISC_WB_ONETIME;
>> +		isc_set_histogram(isc, true);
>> +		v4l2_info(&isc->v4l2_dev, "One time white-balance started.\n");
> 
> Make this v4l2_dbg.
> 
>>   		break;
>>   	default:
>>   		return -EINVAL;
>> @@ -2000,7 +2060,7 @@ static int isc_ctrl_init(struct isc_device *isc)
>>   	ctrls->hist_stat = HIST_INIT;
>>   	isc_reset_awb_ctrls(isc);
>>   
>> -	ret = v4l2_ctrl_handler_init(hdl, 4);
>> +	ret = v4l2_ctrl_handler_init(hdl, 5);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> @@ -2012,6 +2072,10 @@ static int isc_ctrl_init(struct isc_device *isc)
>>   	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAMMA, 0, GAMMA_MAX, 1, 2);
>>   	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
>>   
>> +	/* do_white_balance is a button, so min,max,step,default are ignored */
>> +	isc->do_wb_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DO_WHITE_BALANCE,
>> +					    0, 0, 0, 0);
>> +
>>   	v4l2_ctrl_handler_setup(hdl);
>>   
>>   	return 0;
>>
> 
> Regards,
> 
> 	Hans
>