[PATCH 0/7] media: atmel: atmel-isc: new features

[PATCH 0/7] media: atmel: atmel-isc: new features

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

This series includes feature rework, feature additions and bug fixes for the
atmel-isc driver.
It applies only on top of my previous patchset:
media: atmel: atmel-isc: removed ARGB32 added ABGR32 and XBGR32
media: atmel: atmel-isc: reworked driver and formats
available at:
https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=for-v5.2b&id=03ef1b56cba6ad17f6ead13c85a81e0e80fbc9d1

One open question is regarding the WHITE_BALANCE error returns: it would seem
logical to return EAGAIN or EBUSY, but this is not compliant with
v4l2-compliance.
Does it make sense to return success on every occasion? even if the
DO_WHITE_BALANCE does nothing?
In this series I used the return EAGAIN or EBUSY from the v4l2-ctrls, but I
can change if always success is a better way of returning (even if normally
a return value serves this exact purpose - return some code )

Eugen Hristev (7):
  media: atmel: atmel-isc: add safe checks and fixed wrong ISC state in
    error case
  media: atmel: atmel-isc: reworked white balance feature
  media: v4l2-ctrl: fix flags for DO_WHITE_BALANCE
  media: atmel: atmel-isc: add support for DO_WHITE_BALANCE
  media: atmel: atmel-isc: limit incoming pixels per frame
  media: atmel: atmel-isc: fix INIT_WORK misplacement
  media: atmel: atmel-isc: fix asd memory allocation

 drivers/media/platform/atmel/atmel-isc-regs.h |  25 +-
 drivers/media/platform/atmel/atmel-isc.c      | 346 +++++++++++++++++++++++---
 drivers/media/v4l2-core/v4l2-ctrls.c          |   1 +
 3 files changed, 336 insertions(+), 36 deletions(-)

-- 
2.7.4

[PATCH 1/7] media: atmel: atmel-isc: add safe checks and fixed wrong ISC state in error case

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

This adds safety checks on some scenarios:
- start streaming but streaming is already started
- start streaming but no buffer in the dma queue
- spin lock is not released in error scenario
- no frame is configured but dma is requested to start
- configure ISC may have been called without need, before checking if buffer is
ok.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/media/platform/atmel/atmel-isc.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index a10db16..3c19761 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -722,6 +722,11 @@ static void isc_start_dma(struct isc_device *isc)
 	u32 dctrl_dview;
 	dma_addr_t addr0;
 
+	if (!isc->cur_frm) {
+		v4l2_err(&isc->v4l2_dev, "Video buffer not available\n");
+		return;
+	}
+
 	addr0 = vb2_dma_contig_plane_dma_addr(&isc->cur_frm->vb.vb2_buf, 0);
 	regmap_write(regmap, ISC_DAD0, addr0);
 
@@ -886,6 +891,9 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 	unsigned long flags;
 	int ret;
 
+	if (vb2_is_streaming(&isc->vb2_vidq))
+		return -EBUSY;
+
 	/* Enable stream on the sub device */
 	ret = v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 1);
 	if (ret && ret != -ENOIOCTLCMD) {
@@ -896,6 +904,20 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	pm_runtime_get_sync(isc->dev);
 
+	spin_lock_irqsave(&isc->dma_queue_lock, flags);
+
+	isc->sequence = 0;
+	isc->stop = false;
+	reinit_completion(&isc->comp);
+
+	if (list_empty(&isc->dma_queue)) {
+		v4l2_err(&isc->v4l2_dev, "dma queue empty\n");
+		ret = -EINVAL;
+		goto err_configure_unlock;
+	}
+
+	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
+
 	ret = isc_configure(isc);
 	if (unlikely(ret))
 		goto err_configure;
@@ -905,10 +927,6 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	spin_lock_irqsave(&isc->dma_queue_lock, flags);
 
-	isc->sequence = 0;
-	isc->stop = false;
-	reinit_completion(&isc->comp);
-
 	isc->cur_frm = list_first_entry(&isc->dma_queue,
 					struct isc_buffer, list);
 	list_del(&isc->cur_frm->list);
@@ -919,8 +937,11 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	return 0;
 
+err_configure_unlock:
+	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
+
 err_configure:
-	pm_runtime_put_sync(isc->dev);
+	isc->stop = true;
 
 	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
 
@@ -931,6 +952,7 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 	INIT_LIST_HEAD(&isc->dma_queue);
 	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
 
+	pm_runtime_put_sync(isc->dev);
 	return ret;
 }
 
-- 
2.7.4

[PATCH 2/7] media: atmel: atmel-isc: reworked white balance feature

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

Reworked auto white balance feature (awb) to cope with all four channels.
Implemented stretching and grey world algorithms.
Using the histogram, the ISC will auto adjust the white balance during
frame captures.
Because each histogram needs a frame, it will take 4 frames for one adjustment.
When the gains were updated by previous code, the registers for the gains
were updated only on new streaming start. Now, after each full histogram the
registers are updated with new gains.
Also, on previous code, if the streaming stopped but not all 3 histograms
finished, a new histogram was started either way. This used to lead to an
error "timeout to update profile" when streaming was stopped.
According to the hardware, histogram can only work together with the capture,
not independently.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/media/platform/atmel/atmel-isc-regs.h |   6 +-
 drivers/media/platform/atmel/atmel-isc.c      | 200 +++++++++++++++++++++++---
 2 files changed, 181 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h b/drivers/media/platform/atmel/atmel-isc-regs.h
index 2aadc19..c720b1b 100644
--- a/drivers/media/platform/atmel/atmel-isc-regs.h
+++ b/drivers/media/platform/atmel/atmel-isc-regs.h
@@ -79,13 +79,15 @@
 #define ISC_WB_O_RGR	0x00000060
 
 /* ISC White Balance Offset for B, GB Register */
-#define ISC_WB_O_BGR	0x00000064
+#define ISC_WB_O_BGB	0x00000064
 
 /* ISC White Balance Gain for R, GR Register */
 #define ISC_WB_G_RGR	0x00000068
 
 /* ISC White Balance Gain for B, GB Register */
-#define ISC_WB_G_BGR	0x0000006c
+#define ISC_WB_G_BGB	0x0000006c
+
+#define ISC_WB_O_ZERO_VAL	(1 << 13)
 
 /* ISC Color Filter Array Control Register */
 #define ISC_CFA_CTRL    0x00000070
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 3c19761..f6b8b00e 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -169,13 +169,17 @@ struct isc_ctrls {
 	u8 gamma_index;
 	u8 awb;
 
-	u32 r_gain;
-	u32 b_gain;
+	/* one for each component : GR, R, GB, B */
+	u32 gain[HIST_BAYER];
+	u32 offset[HIST_BAYER];
 
 	u32 hist_entry[HIST_ENTRIES];
 	u32 hist_count[HIST_BAYER];
 	u8 hist_id;
 	u8 hist_stat;
+#define HIST_MIN_INDEX		0
+#define HIST_MAX_INDEX		1
+	u32 hist_minmax[HIST_BAYER][2];
 };
 
 #define ISC_PIPE_LINE_NODE_NUM	11
@@ -209,6 +213,7 @@ struct isc_device {
 	struct work_struct	awb_work;
 
 	struct mutex		lock;
+	spinlock_t		awb_lock;
 
 	struct regmap_field	*pipeline[ISC_PIPE_LINE_NODE_NUM];
 
@@ -395,6 +400,40 @@ module_param(sensor_preferred, uint, 0644);
 MODULE_PARM_DESC(sensor_preferred,
 		 "Sensor is preferred to output the specified format (1-on 0-off), default 1");
 
+static inline void isc_update_awb_ctrls(struct isc_device *isc)
+{
+	struct isc_ctrls *ctrls = &isc->ctrls;
+
+	regmap_write(isc->regmap, ISC_WB_O_RGR,
+		     (ISC_WB_O_ZERO_VAL - (ctrls->offset[ISC_HIS_CFG_MODE_R])) |
+		     ((ISC_WB_O_ZERO_VAL - ctrls->offset[ISC_HIS_CFG_MODE_GR]) << 16));
+	regmap_write(isc->regmap, ISC_WB_O_BGB,
+		     (ISC_WB_O_ZERO_VAL - (ctrls->offset[ISC_HIS_CFG_MODE_B])) |
+		     ((ISC_WB_O_ZERO_VAL - ctrls->offset[ISC_HIS_CFG_MODE_GB]) << 16));
+	regmap_write(isc->regmap, ISC_WB_G_RGR,
+		     ctrls->gain[ISC_HIS_CFG_MODE_R] |
+		     (ctrls->gain[ISC_HIS_CFG_MODE_GR] << 16));
+	regmap_write(isc->regmap, ISC_WB_G_BGB,
+		     ctrls->gain[ISC_HIS_CFG_MODE_B] |
+		     (ctrls->gain[ISC_HIS_CFG_MODE_GB] << 16));
+}
+
+static inline void isc_reset_awb_ctrls(struct isc_device *isc)
+{
+	int c;
+
+	for (c = ISC_HIS_CFG_MODE_GR; c <= ISC_HIS_CFG_MODE_B; c++) {
+		/* gains have a fixed point at 9 decimals */
+		isc->ctrls.gain[c] = 1 << 9;
+		/* offsets are in 2's complements, the value
+		 * will be substracted from ISC_WB_O_ZERO_VAL to obtain
+		 * 2's complement of a value between 0 and
+		 * ISC_WB_O_ZERO_VAL >> 1
+		 */
+		isc->ctrls.offset[c] = ISC_WB_O_ZERO_VAL;
+	}
+}
+
 static int isc_wait_clk_stable(struct clk_hw *hw)
 {
 	struct isc_clk *isc_clk = to_isc_clk(hw);
@@ -746,7 +785,9 @@ static void isc_start_dma(struct isc_device *isc)
 	dctrl_dview = isc->config.dctrl_dview;
 
 	regmap_write(regmap, ISC_DCTRL, dctrl_dview | ISC_DCTRL_IE_IS);
+	spin_lock(&isc->awb_lock);
 	regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_CAPTURE);
+	spin_unlock(&isc->awb_lock);
 }
 
 static void isc_set_pipeline(struct isc_device *isc, u32 pipeline)
@@ -768,11 +809,11 @@ static void isc_set_pipeline(struct isc_device *isc, u32 pipeline)
 
 	bay_cfg = isc->config.sd_format->cfa_baycfg;
 
+	if (!ctrls->awb)
+		isc_reset_awb_ctrls(isc);
+
 	regmap_write(regmap, ISC_WB_CFG, bay_cfg);
-	regmap_write(regmap, ISC_WB_O_RGR, 0x0);
-	regmap_write(regmap, ISC_WB_O_BGR, 0x0);
-	regmap_write(regmap, ISC_WB_G_RGR, ctrls->r_gain | (0x1 << 25));
-	regmap_write(regmap, ISC_WB_G_BGR, ctrls->b_gain | (0x1 << 25));
+	isc_update_awb_ctrls(isc);
 
 	regmap_write(regmap, ISC_CFA_CFG, bay_cfg | ISC_CFA_CFG_EITPOL);
 
@@ -822,13 +863,13 @@ static void isc_set_histogram(struct isc_device *isc, bool enable)
 
 	if (enable) {
 		regmap_write(regmap, ISC_HIS_CFG,
-			     ISC_HIS_CFG_MODE_R |
+			     ISC_HIS_CFG_MODE_GR |
 			     (isc->config.sd_format->cfa_baycfg
 					<< ISC_HIS_CFG_BAYSEL_SHIFT) |
 					ISC_HIS_CFG_RAR);
 		regmap_write(regmap, ISC_HIS_CTRL, ISC_HIS_CTRL_EN);
 		regmap_write(regmap, ISC_INTEN, ISC_INT_HISDONE);
-		ctrls->hist_id = ISC_HIS_CFG_MODE_R;
+		ctrls->hist_id = ISC_HIS_CFG_MODE_GR;
 		isc_update_profile(isc);
 		regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ);
 
@@ -870,7 +911,7 @@ static int isc_configure(struct isc_device *isc)
 	isc_set_pipeline(isc, pipeline);
 
 	/*
-	 * The current implemented histogram is available for RAW R, B, GB
+	 * The current implemented histogram is available for RAW R, B, GB, GR
 	 * channels. We need to check if sensor is outputting RAW BAYER
 	 */
 	if (isc->ctrls.awb &&
@@ -1462,6 +1503,12 @@ static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
 		return ret;
 
 	isc->fmt = *f;
+
+	if (isc->try_config.sd_format && isc->config.sd_format &&
+	    isc->try_config.sd_format != isc->config.sd_format) {
+		isc->ctrls.hist_stat = HIST_INIT;
+		isc_reset_awb_ctrls(isc);
+	}
 	/* make the try configuration active */
 	isc->config = isc->try_config;
 
@@ -1745,7 +1792,7 @@ static irqreturn_t isc_interrupt(int irq, void *dev_id)
 	return ret;
 }
 
-static void isc_hist_count(struct isc_device *isc)
+static void isc_hist_count(struct isc_device *isc, u32 *min, u32 *max)
 {
 	struct regmap *regmap = isc->regmap;
 	struct isc_ctrls *ctrls = &isc->ctrls;
@@ -1753,25 +1800,99 @@ static void isc_hist_count(struct isc_device *isc)
 	u32 *hist_entry = &ctrls->hist_entry[0];
 	u32 i;
 
+	*min = 0;
+	*max = HIST_ENTRIES;
+
 	regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);
 
 	*hist_count = 0;
-	for (i = 0; i < HIST_ENTRIES; i++)
+	/*
+	 * we deliberately ignore the end of the histogram,
+	 * the most white pixels
+	 */
+	for (i = 1; i < HIST_ENTRIES; i++) {
+		if (*hist_entry && !*min)
+			*min = i;
+		if (*hist_entry)
+			*max = i;
 		*hist_count += i * (*hist_entry++);
+	}
+
+	if (!*min)
+		*min = 1;
 }
 
 static void isc_wb_update(struct isc_ctrls *ctrls)
 {
 	u32 *hist_count = &ctrls->hist_count[0];
-	u64 g_count = (u64)hist_count[ISC_HIS_CFG_MODE_GB] << 9;
-	u32 hist_r = hist_count[ISC_HIS_CFG_MODE_R];
-	u32 hist_b = hist_count[ISC_HIS_CFG_MODE_B];
+	u32 c, offset[4];
+	u64 avg = 0;
+	/* We compute two gains, stretch gain and grey world gain */
+	u32 s_gain[4], gw_gain[4];
+
+	/*
+	 * According to Grey World, we need to set gains for R/B to normalize
+	 * them towards the green channel.
+	 * Thus we want to keep Green as fixed and adjust only Red/Blue
+	 * Compute the average of the both green channels first
+	 */
+	avg = (u64)hist_count[ISC_HIS_CFG_MODE_GR] +
+		(u64)hist_count[ISC_HIS_CFG_MODE_GB];
+	avg >>= 1;
 
-	if (hist_r)
-		ctrls->r_gain = div_u64(g_count, hist_r);
+	/* Green histogram is null, nothing to do */
+	if (!avg)
+		return;
 
-	if (hist_b)
-		ctrls->b_gain = div_u64(g_count, hist_b);
+	for (c = ISC_HIS_CFG_MODE_GR; c <= ISC_HIS_CFG_MODE_B; c++) {
+		/*
+		 * the color offset is the minimum value of the histogram.
+		 * we stretch this color to the full range by substracting
+		 * this value from the color component.
+		 */
+		offset[c] = ctrls->hist_minmax[c][HIST_MIN_INDEX];
+		/*
+		 * The offset is always at least 1. If the offset is 1, we do
+		 * not need to adjust it, so our result must be zero.
+		 * the offset is computed in a histogram on 9 bits (0..512)
+		 * but the offset in register is based on
+		 * 12 bits pipeline (0..4096).
+		 * we need to shift with the 3 bits that the histogram is
+		 * ignoring
+		 */
+		ctrls->offset[c] = (offset[c] - 1) << 3;
+
+		/* the offset is then taken and converted to 2's complements */
+		if (!ctrls->offset[c])
+			ctrls->offset[c] = ISC_WB_O_ZERO_VAL;
+
+		/*
+		 * the stretch gain is the total number of histogram bins
+		 * divided by the actual range of color component (Max - Min)
+		 * If we compute gain like this, the actual color component
+		 * will be stretched to the full histogram.
+		 * We need to shift 9 bits for precision, we have 9 bits for
+		 * decimals
+		 */
+		s_gain[c] = (HIST_ENTRIES << 9) /
+			(ctrls->hist_minmax[c][HIST_MAX_INDEX] -
+			ctrls->hist_minmax[c][HIST_MIN_INDEX] + 1);
+
+		/*
+		 * Now we have to compute the gain w.r.t. the average.
+		 * Add/lose gain to the component towards the average.
+		 * If it happens that the component is zero, use the
+		 * fixed point value : 1.0 gain.
+		 */
+		if (hist_count[c])
+			gw_gain[c] = div_u64(avg << 9, hist_count[c]);
+		else
+			gw_gain[c] = 1 << 9;
+
+		/* multiply both gains and adjust for decimals */
+		ctrls->gain[c] = s_gain[c] * gw_gain[c];
+		ctrls->gain[c] >>= 9;
+	}
 }
 
 static void isc_awb_work(struct work_struct *w)
@@ -1782,27 +1903,56 @@ static void isc_awb_work(struct work_struct *w)
 	struct isc_ctrls *ctrls = &isc->ctrls;
 	u32 hist_id = ctrls->hist_id;
 	u32 baysel;
+	unsigned long flags;
+	u32 min, max;
+
+	/* streaming is not active anymore */
+	if (isc->stop)
+		return;
 
 	if (ctrls->hist_stat != HIST_ENABLED)
 		return;
 
-	isc_hist_count(isc);
+	isc_hist_count(isc, &min, &max);
+	ctrls->hist_minmax[hist_id][HIST_MIN_INDEX] = min;
+	ctrls->hist_minmax[hist_id][HIST_MAX_INDEX] = max;
 
 	if (hist_id != ISC_HIS_CFG_MODE_B) {
 		hist_id++;
 	} else {
 		isc_wb_update(ctrls);
-		hist_id = ISC_HIS_CFG_MODE_R;
+		hist_id = ISC_HIS_CFG_MODE_GR;
 	}
 
 	ctrls->hist_id = hist_id;
 	baysel = isc->config.sd_format->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
 
+	/* if no more auto white balance, reset controls. */
+	if (!ctrls->awb)
+		isc_reset_awb_ctrls(isc);
+
 	pm_runtime_get_sync(isc->dev);
 
+	/*
+	 * 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) {
+		/*
+		 * It may happen that DMA Done IRQ will trigger while we are
+		 * updating white balance registers here.
+		 * In that case, only parts of the controls have been updated.
+		 * We can avoid that by locking the section.
+		 */
+		spin_lock_irqsave(&isc->awb_lock, flags);
+		isc_update_awb_ctrls(isc);
+		spin_unlock_irqrestore(&isc->awb_lock, flags);
+	}
 	regmap_write(regmap, ISC_HIS_CFG, hist_id | baysel | ISC_HIS_CFG_RAR);
 	isc_update_profile(isc);
-	regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ);
+	/* if awb has been disabled, we don't need to start another histogram */
+	if (ctrls->awb)
+		regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ);
 
 	pm_runtime_put_sync(isc->dev);
 }
@@ -1826,8 +1976,7 @@ static int isc_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_AUTO_WHITE_BALANCE:
 		ctrls->awb = ctrl->val;
 		if (ctrls->hist_stat != HIST_ENABLED) {
-			ctrls->r_gain = 0x1 << 9;
-			ctrls->b_gain = 0x1 << 9;
+			isc_reset_awb_ctrls(isc);
 		}
 		break;
 	default:
@@ -1849,11 +1998,15 @@ static int isc_ctrl_init(struct isc_device *isc)
 	int ret;
 
 	ctrls->hist_stat = HIST_INIT;
+	isc_reset_awb_ctrls(isc);
 
 	ret = v4l2_ctrl_handler_init(hdl, 4);
 	if (ret < 0)
 		return ret;
 
+	ctrls->brightness = 0;
+	ctrls->contrast = 256;
+
 	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -1024, 1023, 1, 0);
 	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -2048, 2047, 1, 256);
 	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAMMA, 0, GAMMA_MAX, 1, 2);
@@ -2019,6 +2172,7 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
 	/* Init video dma queues */
 	INIT_LIST_HEAD(&isc->dma_queue);
 	spin_lock_init(&isc->dma_queue_lock);
+	spin_lock_init(&isc->awb_lock);
 
 	ret = isc_formats_init(isc);
 	if (ret < 0) {
-- 
2.7.4

[PATCH 3/7] media: v4l2-ctrl: fix flags for DO_WHITE_BALANCE

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

Control DO_WHITE_BALANCE is a button, with read only and execute-on-write flags.
Adding this control in the proper list in the fill function.

After adding it here, we can see output of v4l2-ctl -L
do_white_balance 0x0098090d (button) : flags=write-only, execute-on-write

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index b79d3bb..dc32fb7 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1150,6 +1150,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_FLASH_STROBE_STOP:
 	case V4L2_CID_AUTO_FOCUS_START:
 	case V4L2_CID_AUTO_FOCUS_STOP:
+	case V4L2_CID_DO_WHITE_BALANCE:
 		*type = V4L2_CTRL_TYPE_BUTTON;
 		*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
 			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
-- 
2.7.4

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

[Eugen.Hristev]

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");
+			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");
+			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");
+			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");
+			return -EAGAIN;
+		}
+		ctrls->awb = ISC_WB_ONETIME;
+		isc_set_histogram(isc, true);
+		v4l2_info(&isc->v4l2_dev, "One time white-balance started.\n");
 		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;
-- 
2.7.4

[PATCH 5/7] media: atmel: atmel-isc: limit incoming pixels per frame

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

This will limit the incoming pixels per frame from the sensor.
Currently, the ISC will stop sampling the frame only when the vsync/hsync
are detected.
If we misconfigure the resolution in the sensor w.r.t. resolution in the ISC,
the buffer used for DMA in the ISC will be smaller than the number of pixels
that the ISC DMA engine will copy.
In this case it happens that the DMA will overwrite parts of the memory which
should not be written, leading to memory corruption.
To avoid this situation, use the PFE CFG1 and PFE CFG2 registers, which crop
the incoming frame to the resolution that we configure.
This way the DMA engine will never write more data than we expect it to.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/media/platform/atmel/atmel-isc-regs.h | 19 +++++++++++++++
 drivers/media/platform/atmel/atmel-isc.c      | 34 +++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h b/drivers/media/platform/atmel/atmel-isc-regs.h
index c720b1b..9b3a60b 100644
--- a/drivers/media/platform/atmel/atmel-isc-regs.h
+++ b/drivers/media/platform/atmel/atmel-isc-regs.h
@@ -35,6 +35,25 @@
 #define ISC_PFG_CFG0_BPS_TWELVE (0x0 << 28)
 #define ISC_PFE_CFG0_BPS_MASK   GENMASK(30, 28)
 
+#define ISC_PFE_CFG0_COLEN	BIT(12)
+#define ISC_PFE_CFG0_ROWEN	BIT(13)
+
+/* ISC Parallel Front End Configuration 1 Register */
+#define ISC_PFE_CFG1    0x00000010
+
+#define ISC_PFE_CFG1_COLMIN(v)		((v))
+#define ISC_PFE_CFG1_COLMIN_MASK	GENMASK(15, 0)
+#define ISC_PFE_CFG1_COLMAX(v)		((v) << 16)
+#define ISC_PFE_CFG1_COLMAX_MASK	GENMASK(31, 16)
+
+/* ISC Parallel Front End Configuration 2 Register */
+#define ISC_PFE_CFG2    0x00000014
+
+#define ISC_PFE_CFG2_ROWMIN(v)		((v))
+#define ISC_PFE_CFG2_ROWMIN_MASK	GENMASK(15, 0)
+#define ISC_PFE_CFG2_ROWMAX(v)		((v) << 16)
+#define ISC_PFE_CFG2_ROWMAX_MASK	GENMASK(31, 16)
+
 /* ISC Clock Enable Register */
 #define ISC_CLKEN               0x00000018
 
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index e516805..902bd7e 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -764,12 +764,46 @@ static void isc_start_dma(struct isc_device *isc)
 	u32 sizeimage = isc->fmt.fmt.pix.sizeimage;
 	u32 dctrl_dview;
 	dma_addr_t addr0;
+	u32 h, w;
 
 	if (!isc->cur_frm) {
 		v4l2_err(&isc->v4l2_dev, "Video buffer not available\n");
 		return;
 	}
 
+	h = isc->fmt.fmt.pix.height;
+	w = isc->fmt.fmt.pix.width;
+
+	/*
+	 * In case the sensor is not RAW, it will output a pixel (12-16 bits)
+	 * with two samples on the ISC Data bus (which is 8-12)
+	 * ISC will count each sample, so, we need to multiply these values
+	 * by two, to get the real number of samples for the required pixels.
+	 */
+	if (!ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code)) {
+		h <<= 1;
+		w <<= 1;
+	}
+
+	/*
+	 * We limit the column/row count that the ISC will output according
+	 * to the configured resolution that we want.
+	 * This will avoid the situation where the sensor is misconfigured,
+	 * sending more data, and the ISC will just take it and DMA to memory,
+	 * causing corruption.
+	 */
+	regmap_write(regmap, ISC_PFE_CFG1,
+		     (ISC_PFE_CFG1_COLMIN(0) & ISC_PFE_CFG1_COLMIN_MASK) |
+		     (ISC_PFE_CFG1_COLMAX(w - 1) & ISC_PFE_CFG1_COLMAX_MASK));
+
+	regmap_write(regmap, ISC_PFE_CFG2,
+		     (ISC_PFE_CFG2_ROWMIN(0) & ISC_PFE_CFG2_ROWMIN_MASK) |
+		     (ISC_PFE_CFG2_ROWMAX(h - 1) & ISC_PFE_CFG2_ROWMAX_MASK));
+
+	regmap_update_bits(regmap, ISC_PFE_CFG0,
+			   ISC_PFE_CFG0_COLEN | ISC_PFE_CFG0_ROWEN,
+			   ISC_PFE_CFG0_COLEN | ISC_PFE_CFG0_ROWEN);
+
 	addr0 = vb2_dma_contig_plane_dma_addr(&isc->cur_frm->vb.vb2_buf, 0);
 	regmap_write(regmap, ISC_DAD0, addr0);
 
-- 
2.7.4

[PATCH 6/7] media: atmel: atmel-isc: fix INIT_WORK misplacement

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

In case the completion function failes, unbind will be called
which will call cancel_work for awb_work.
This will trigger a WARN message from the workqueue.
To avoid this, move the INIT_WORK call at the start of the completion
function. This way the work is always initialized, which corresponds
to the 'always canceled' unbind code.

Fixes: 93d4a26c3d ("[media] atmel-isc: add the isc pipeline function")
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/media/platform/atmel/atmel-isc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 902bd7e..48652c0 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -2237,6 +2237,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
 	struct vb2_queue *q = &isc->vb2_vidq;
 	int ret;
 
+	INIT_WORK(&isc->awb_work, isc_awb_work);
+
 	ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev);
 	if (ret < 0) {
 		v4l2_err(&isc->v4l2_dev, "Failed to register subdev nodes\n");
@@ -2291,8 +2293,6 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
 		return ret;
 	}
 
-	INIT_WORK(&isc->awb_work, isc_awb_work);
-
 	/* Register video device */
 	strscpy(vdev->name, ATMEL_ISC_NAME, sizeof(vdev->name));
 	vdev->release		= video_device_release_empty;
-- 
2.7.4

[PATCH 7/7] media: atmel: atmel-isc: fix asd memory allocation

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

The subsystem will free the asd memory on notifier cleanup, if the asd is
added to the notifier.
However the memory is freed using kfree.
Thus, we cannot allocate the asd using devm_*
This can lead to crashes and problems.
To test this issue, just return an error at probe, but cleanup the
notifier beforehand.

Fixes: 106267444f ("[media] atmel-isc: add the Image Sensor Controller code")
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/media/platform/atmel/atmel-isc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 48652c0..8d8cce5 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -2408,8 +2408,11 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 			break;
 		}
 
-		subdev_entity->asd = devm_kzalloc(dev,
-				     sizeof(*subdev_entity->asd), GFP_KERNEL);
+		/* asd will be freed by the subsystem once it's added to the
+		 * notifier list
+		 */
+		subdev_entity->asd = kzalloc(sizeof(*subdev_entity->asd),
+					     GFP_KERNEL);
 		if (!subdev_entity->asd) {
 			of_node_put(rem);
 			ret = -ENOMEM;
@@ -2553,6 +2556,7 @@ static int atmel_isc_probe(struct platform_device *pdev)
 						     subdev_entity->asd);
 		if (ret) {
 			fwnode_handle_put(subdev_entity->asd->match.fwnode);
+			kfree(subdev_entity->asd);
 			goto cleanup_subdev;
 		}
 
-- 
2.7.4

Re: [PATCH 1/7] media: atmel: atmel-isc: add safe checks and fixed wrong ISC state in error case

[Hans Verkuil]

On 4/9/19 1:07 PM, Eugen.Hristev@microchip.com wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> This adds safety checks on some scenarios:
> - start streaming but streaming is already started

Can't happen. vb2 checks for that.

> - start streaming but no buffer in the dma queue

Can't happen since min_buffers_needed is > 0. So start_streaming will
never be called unless at least one buffer is queued.

> - spin lock is not released in error scenario
> - no frame is configured but dma is requested to start

Can this ever happen? It's set by start_streaming when you know there is
at least one buffer, so this seems overkill.

Regards,

	Hans

> - configure ISC may have been called without need, before checking if buffer is
> ok.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  drivers/media/platform/atmel/atmel-isc.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> index a10db16..3c19761 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -722,6 +722,11 @@ static void isc_start_dma(struct isc_device *isc)
>  	u32 dctrl_dview;
>  	dma_addr_t addr0;
>  
> +	if (!isc->cur_frm) {
> +		v4l2_err(&isc->v4l2_dev, "Video buffer not available\n");
> +		return;
> +	}
> +
>  	addr0 = vb2_dma_contig_plane_dma_addr(&isc->cur_frm->vb.vb2_buf, 0);
>  	regmap_write(regmap, ISC_DAD0, addr0);
>  
> @@ -886,6 +891,9 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	unsigned long flags;
>  	int ret;
>  
> +	if (vb2_is_streaming(&isc->vb2_vidq))
> +		return -EBUSY;
> +
>  	/* Enable stream on the sub device */
>  	ret = v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 1);
>  	if (ret && ret != -ENOIOCTLCMD) {
> @@ -896,6 +904,20 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	pm_runtime_get_sync(isc->dev);
>  
> +	spin_lock_irqsave(&isc->dma_queue_lock, flags);
> +
> +	isc->sequence = 0;
> +	isc->stop = false;
> +	reinit_completion(&isc->comp);
> +
> +	if (list_empty(&isc->dma_queue)) {
> +		v4l2_err(&isc->v4l2_dev, "dma queue empty\n");
> +		ret = -EINVAL;
> +		goto err_configure_unlock;
> +	}
> +
> +	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
> +
>  	ret = isc_configure(isc);
>  	if (unlikely(ret))
>  		goto err_configure;
> @@ -905,10 +927,6 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	spin_lock_irqsave(&isc->dma_queue_lock, flags);
>  
> -	isc->sequence = 0;
> -	isc->stop = false;
> -	reinit_completion(&isc->comp);
> -
>  	isc->cur_frm = list_first_entry(&isc->dma_queue,
>  					struct isc_buffer, list);
>  	list_del(&isc->cur_frm->list);
> @@ -919,8 +937,11 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	return 0;
>  
> +err_configure_unlock:
> +	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
> +
>  err_configure:
> -	pm_runtime_put_sync(isc->dev);
> +	isc->stop = true;
>  
>  	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
>  
> @@ -931,6 +952,7 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	INIT_LIST_HEAD(&isc->dma_queue);
>  	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
>  
> +	pm_runtime_put_sync(isc->dev);
>  	return ret;
>  }
>  
> 

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

[Hans Verkuil]

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.

> +			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

Re: [PATCH 0/7] media: atmel: atmel-isc: new features

[Hans Verkuil]

On 4/9/19 1:07 PM, Eugen.Hristev@microchip.com wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> This series includes feature rework, feature additions and bug fixes for the
> atmel-isc driver.
> It applies only on top of my previous patchset:
> media: atmel: atmel-isc: removed ARGB32 added ABGR32 and XBGR32
> media: atmel: atmel-isc: reworked driver and formats
> available at:
> https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=for-v5.2b&id=03ef1b56cba6ad17f6ead13c85a81e0e80fbc9d1
> 
> One open question is regarding the WHITE_BALANCE error returns: it would seem
> logical to return EAGAIN or EBUSY, but this is not compliant with
> v4l2-compliance.
> Does it make sense to return success on every occasion? even if the
> DO_WHITE_BALANCE does nothing?
> In this series I used the return EAGAIN or EBUSY from the v4l2-ctrls, but I
> can change if always success is a better way of returning (even if normally
> a return value serves this exact purpose - return some code )

See my comments. Use v4l2_ctrl_activate() instead to mark controls active
or inactive and the control framework will do the rest.

I think it would speed up matters if you split up your series: one series
containing just fixes (I can merged those quickly) and one for the new
functionality.

Regards,

	Hans

> 
> Eugen Hristev (7):
>   media: atmel: atmel-isc: add safe checks and fixed wrong ISC state in
>     error case
>   media: atmel: atmel-isc: reworked white balance feature
>   media: v4l2-ctrl: fix flags for DO_WHITE_BALANCE
>   media: atmel: atmel-isc: add support for DO_WHITE_BALANCE
>   media: atmel: atmel-isc: limit incoming pixels per frame
>   media: atmel: atmel-isc: fix INIT_WORK misplacement
>   media: atmel: atmel-isc: fix asd memory allocation
> 
>  drivers/media/platform/atmel/atmel-isc-regs.h |  25 +-
>  drivers/media/platform/atmel/atmel-isc.c      | 346 +++++++++++++++++++++++---
>  drivers/media/v4l2-core/v4l2-ctrls.c          |   1 +
>  3 files changed, 336 insertions(+), 36 deletions(-)
> 

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

[Eugen.Hristev]

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
> 

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

[Hans Verkuil]

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.

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
>>

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

[Eugen.Hristev]

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
>>>
> 
>