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

From: <Eugen.Hristev@microchip.com>
To: <hverkuil@xs4all.nl>, <linux-media@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>
Cc: <Nicolas.Ferre@microchip.com>, <mchehab@kernel.org>,
	<ksloat@aampglobal.com>
Subject: Re: [PATCH 4/7] media: atmel: atmel-isc: add support for DO_WHITE_BALANCE
Date: Tue, 23 Apr 2019 13:19:38 +0000	[thread overview]
Message-ID: <a891b3ea-6ab4-30c2-04ff-152d59530509@microchip.com> (raw)
In-Reply-To: <2c7b069e-f81c-a643-4feb-0839277752e1@xs4all.nl>

On 23.04.2019 16:11, Hans Verkuil wrote:
> On 4/15/19 8:43 AM, Eugen.Hristev@microchip.com wrote:
>>
>>
>> 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.
> 
> No, you are correct. I got confused with FLAG_GRABBED.
> 
> In any case, the idea was right, but you do have to add code to s_ctrl
> to handle this (e.g. if the INACTIVE flag is set, then just do nothing).
> 
> The INACTIVE flag is meant to communicate that the control can still be
> set, but it just doesn't do anything. qv4l2 will disable the control if
> this flag is set.
> 
> Note that when you set an inactive control, the control value should
> still be updated even if it isn't used at the moment. If the configuration
> changes so that the control becomes active again, then that last set
> value should be used by the hardware.
> 
> This is the reason why s_ctrl is still called.

Hello Hans,

Thanks for the explanation. I noticed what you say, and saw other 
drivers just exit the s_ctrl if the flag is INACTIVE.
Thus, my latest patch revision does exactly that 
https://patchwork.linuxtv.org/patch/55682/

Regarding my specific control (DO_WHITE_BALANCE), it's a button, so it 
should really do nothing if control is inactive (no state to save).

Thanks,
Eugen

> 
> Regards,
> 
> 	Hans
> 
>>
>> 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
>>>
> 
> 