[PATCH 0/3] OMAP3 ISP resizer live zoom fixes

[PATCH 0/3] OMAP3 ISP resizer live zoom fixes

[Laurent Pinchart]

Hello,

This patch set fixes two issues occuring when performing live zoom with the
OMAP3 ISP resizer.

The first issue has been observed when using the digital zoom of the live
application (http://git.ideasonboard.org/omap3-isp-live.git) on a beagleboard.
It leads to image corruption due to interrupt handling latency. Patch 2/3
fixes it.

The second issue is a race condition that I haven't observed in practice. It's
fixed by patch 3/3. As usual with race conditions and locking, careful review
will be appreciated.

Laurent Pinchart (3):
  omap3isp: resizer: Remove needless variable initializations
  omap3isp: resizer: Remove slow debugging message from interrupt
    handler
  omap3isp: resizer: Protect against races when updating crop

 drivers/media/platform/omap3isp/ispresizer.c | 70 ++++++++++++++++++----------
 drivers/media/platform/omap3isp/ispresizer.h |  3 ++
 2 files changed, 48 insertions(+), 25 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/3] omap3isp: resizer: Remove needless variable initializations

[Laurent Pinchart]

There's no need to initialize local variables to zero when they're
explicitly assigned another value right after. Remove the needless
initializations.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispresizer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispresizer.c b/drivers/media/platform/omap3isp/ispresizer.c
index 6f077c2..8515793 100644
--- a/drivers/media/platform/omap3isp/ispresizer.c
+++ b/drivers/media/platform/omap3isp/ispresizer.c
@@ -239,7 +239,7 @@ static void resizer_set_phase(struct isp_res_device *res, u32 h_phase,
 			      u32 v_phase)
 {
 	struct isp_device *isp = to_isp_device(res);
-	u32 rgval = 0;
+	u32 rgval;
 
 	rgval = isp_reg_readl(isp, OMAP3_ISP_IOMEM_RESZ, ISPRSZ_CNT) &
 	      ~(ISPRSZ_CNT_HSTPH_MASK | ISPRSZ_CNT_VSTPH_MASK);
@@ -275,7 +275,7 @@ static void resizer_set_luma(struct isp_res_device *res,
 			     struct resizer_luma_yenh *luma)
 {
 	struct isp_device *isp = to_isp_device(res);
-	u32 rgval = 0;
+	u32 rgval;
 
 	rgval  = (luma->algo << ISPRSZ_YENH_ALGO_SHIFT)
 		  & ISPRSZ_YENH_ALGO_MASK;
@@ -322,7 +322,7 @@ static void resizer_set_ratio(struct isp_res_device *res,
 {
 	struct isp_device *isp = to_isp_device(res);
 	const u16 *h_filter, *v_filter;
-	u32 rgval = 0;
+	u32 rgval;
 
 	rgval = isp_reg_readl(isp, OMAP3_ISP_IOMEM_RESZ, ISPRSZ_CNT) &
 			      ~(ISPRSZ_CNT_HRSZ_MASK | ISPRSZ_CNT_VRSZ_MASK);
@@ -365,7 +365,7 @@ static void resizer_set_output_size(struct isp_res_device *res,
 				    u32 width, u32 height)
 {
 	struct isp_device *isp = to_isp_device(res);
-	u32 rgval = 0;
+	u32 rgval;
 
 	dev_dbg(isp->dev, "Output size[w/h]: %dx%d\n", width, height);
 	rgval  = (width << ISPRSZ_OUT_SIZE_HORZ_SHIFT)
@@ -409,7 +409,7 @@ static void resizer_set_output_offset(struct isp_res_device *res, u32 offset)
 static void resizer_set_start(struct isp_res_device *res, u32 left, u32 top)
 {
 	struct isp_device *isp = to_isp_device(res);
-	u32 rgval = 0;
+	u32 rgval;
 
 	rgval = (left << ISPRSZ_IN_START_HORZ_ST_SHIFT)
 		& ISPRSZ_IN_START_HORZ_ST_MASK;
@@ -429,7 +429,7 @@ static void resizer_set_input_size(struct isp_res_device *res,
 				   u32 width, u32 height)
 {
 	struct isp_device *isp = to_isp_device(res);
-	u32 rgval = 0;
+	u32 rgval;
 
 	dev_dbg(isp->dev, "Input size[w/h]: %dx%d\n", width, height);
 
-- 
1.8.5.5

[PATCH 2/3] omap3isp: resizer: Remove slow debugging message from interrupt handler

[Laurent Pinchart]

The resizer_set_input_size() function prints a debugging message with
the input width and height values. As the function is called from
interrupt context, printing that message to the serial console could
slow down the interrupt handler and cause it to miss the start of the
next frame, causing image corruption.

Fix this by reorganizing the resizer debug messages. The driver now
prints the input size, the crop rectangle and the output size in the set
selection handler instead of scattering debug messages in various
places.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispresizer.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispresizer.c b/drivers/media/platform/omap3isp/ispresizer.c
index 8515793..c8676e1 100644
--- a/drivers/media/platform/omap3isp/ispresizer.c
+++ b/drivers/media/platform/omap3isp/ispresizer.c
@@ -367,7 +367,6 @@ static void resizer_set_output_size(struct isp_res_device *res,
 	struct isp_device *isp = to_isp_device(res);
 	u32 rgval;
 
-	dev_dbg(isp->dev, "Output size[w/h]: %dx%d\n", width, height);
 	rgval  = (width << ISPRSZ_OUT_SIZE_HORZ_SHIFT)
 		 & ISPRSZ_OUT_SIZE_HORZ_MASK;
 	rgval |= (height << ISPRSZ_OUT_SIZE_VERT_SHIFT)
@@ -431,8 +430,6 @@ static void resizer_set_input_size(struct isp_res_device *res,
 	struct isp_device *isp = to_isp_device(res);
 	u32 rgval;
 
-	dev_dbg(isp->dev, "Input size[w/h]: %dx%d\n", width, height);
-
 	rgval = (width << ISPRSZ_IN_SIZE_HORZ_SHIFT)
 		& ISPRSZ_IN_SIZE_HORZ_MASK;
 	rgval |= (height << ISPRSZ_IN_SIZE_VERT_SHIFT)
@@ -1302,12 +1299,10 @@ static int resizer_set_selection(struct v4l2_subdev *sd,
 	format_source = __resizer_get_format(res, fh, RESZ_PAD_SOURCE,
 					     sel->which);
 
-	dev_dbg(isp->dev, "%s: L=%d,T=%d,W=%d,H=%d,which=%d\n", __func__,
-		sel->r.left, sel->r.top, sel->r.width, sel->r.height,
-		sel->which);
-
-	dev_dbg(isp->dev, "%s: input=%dx%d, output=%dx%d\n", __func__,
+	dev_dbg(isp->dev, "%s(%s): req %ux%u -> (%d,%d)/%ux%u -> %ux%u\n",
+		__func__, sel->which == V4L2_SUBDEV_FORMAT_TRY ? "try" : "act",
 		format_sink->width, format_sink->height,
+		sel->r.left, sel->r.top, sel->r.width, sel->r.height,
 		format_source->width, format_source->height);
 
 	/* Clamp the crop rectangle to the bounds, and then mangle it further to
@@ -1322,6 +1317,12 @@ static int resizer_set_selection(struct v4l2_subdev *sd,
 	*__resizer_get_crop(res, fh, sel->which) = sel->r;
 	resizer_calc_ratios(res, &sel->r, format_source, &ratio);
 
+	dev_dbg(isp->dev, "%s(%s): got %ux%u -> (%d,%d)/%ux%u -> %ux%u\n",
+		__func__, sel->which == V4L2_SUBDEV_FORMAT_TRY ? "try" : "act",
+		format_sink->width, format_sink->height,
+		sel->r.left, sel->r.top, sel->r.width, sel->r.height,
+		format_source->width, format_source->height);
+
 	if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
 		return 0;
 
-- 
1.8.5.5

[PATCH 3/3] omap3isp: resizer: Protect against races when updating crop

[Laurent Pinchart]

When updating the crop rectangle during streaming, the IRQ handler will
reprogram the resizer after the current frame. A race condition
currently exists between the set selection operation and the IRQ
handler: if the set selection operation is called twice in a row and the
IRQ handler runs only during the second call, it could reprogram the
hardware with partially updated values. Use a spinlock to protect
against that.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispresizer.c | 43 ++++++++++++++++++++--------
 drivers/media/platform/omap3isp/ispresizer.h |  3 ++
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispresizer.c b/drivers/media/platform/omap3isp/ispresizer.c
index c8676e1..5fcf3cf 100644
--- a/drivers/media/platform/omap3isp/ispresizer.c
+++ b/drivers/media/platform/omap3isp/ispresizer.c
@@ -1072,10 +1072,13 @@ static void resizer_isr_buffer(struct isp_res_device *res)
 void omap3isp_resizer_isr(struct isp_res_device *res)
 {
 	struct v4l2_mbus_framefmt *informat, *outformat;
+	unsigned long flags;
 
 	if (omap3isp_module_sync_is_stopping(&res->wait, &res->stopping))
 		return;
 
+	spin_lock_irqsave(&res->lock, flags);
+
 	if (res->applycrop) {
 		outformat = __resizer_get_format(res, NULL, RESZ_PAD_SOURCE,
 					      V4L2_SUBDEV_FORMAT_ACTIVE);
@@ -1085,6 +1088,8 @@ void omap3isp_resizer_isr(struct isp_res_device *res)
 		res->applycrop = 0;
 	}
 
+	spin_unlock_irqrestore(&res->lock, flags);
+
 	resizer_isr_buffer(res);
 }
 
@@ -1287,8 +1292,10 @@ static int resizer_set_selection(struct v4l2_subdev *sd,
 {
 	struct isp_res_device *res = v4l2_get_subdevdata(sd);
 	struct isp_device *isp = to_isp_device(res);
-	struct v4l2_mbus_framefmt *format_sink, *format_source;
+	const struct v4l2_mbus_framefmt *format_sink;
+	struct v4l2_mbus_framefmt format_source;
 	struct resizer_ratio ratio;
+	unsigned long flags;
 
 	if (sel->target != V4L2_SEL_TGT_CROP ||
 	    sel->pad != RESZ_PAD_SINK)
@@ -1296,14 +1303,14 @@ static int resizer_set_selection(struct v4l2_subdev *sd,
 
 	format_sink = __resizer_get_format(res, fh, RESZ_PAD_SINK,
 					   sel->which);
-	format_source = __resizer_get_format(res, fh, RESZ_PAD_SOURCE,
-					     sel->which);
+	format_source = *__resizer_get_format(res, fh, RESZ_PAD_SOURCE,
+					      sel->which);
 
 	dev_dbg(isp->dev, "%s(%s): req %ux%u -> (%d,%d)/%ux%u -> %ux%u\n",
 		__func__, sel->which == V4L2_SUBDEV_FORMAT_TRY ? "try" : "act",
 		format_sink->width, format_sink->height,
 		sel->r.left, sel->r.top, sel->r.width, sel->r.height,
-		format_source->width, format_source->height);
+		format_source.width, format_source.height);
 
 	/* Clamp the crop rectangle to the bounds, and then mangle it further to
 	 * fulfill the TRM equations. Store the clamped but otherwise unmangled
@@ -1313,29 +1320,39 @@ static int resizer_set_selection(struct v4l2_subdev *sd,
 	 * smaller input crop rectangle every time the output size is set if we
 	 * stored the mangled rectangle.
 	 */
-	resizer_try_crop(format_sink, format_source, &sel->r);
+	resizer_try_crop(format_sink, &format_source, &sel->r);
 	*__resizer_get_crop(res, fh, sel->which) = sel->r;
-	resizer_calc_ratios(res, &sel->r, format_source, &ratio);
+	resizer_calc_ratios(res, &sel->r, &format_source, &ratio);
 
 	dev_dbg(isp->dev, "%s(%s): got %ux%u -> (%d,%d)/%ux%u -> %ux%u\n",
 		__func__, sel->which == V4L2_SUBDEV_FORMAT_TRY ? "try" : "act",
 		format_sink->width, format_sink->height,
 		sel->r.left, sel->r.top, sel->r.width, sel->r.height,
-		format_source->width, format_source->height);
+		format_source.width, format_source.height);
 
-	if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
+	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+		*__resizer_get_format(res, fh, RESZ_PAD_SOURCE, sel->which) =
+			format_source;
 		return 0;
+	}
+
+	/* Update the source format, resizing ratios and crop rectangle. If
+	 * streaming is on the IRQ handler will reprogram the resizer after the
+	 * current frame. We thus we need to protect against race conditions.
+	 */
+	spin_lock_irqsave(&res->lock, flags);
+
+	*__resizer_get_format(res, fh, RESZ_PAD_SOURCE, sel->which) =
+		format_source;
 
 	res->ratio = ratio;
 	res->crop.active = sel->r;
 
-	/*
-	 * set_selection can be called while streaming is on. In this case the
-	 * crop values will be set in the next IRQ.
-	 */
 	if (res->state != ISP_PIPELINE_STREAM_STOPPED)
 		res->applycrop = 1;
 
+	spin_unlock_irqrestore(&res->lock, flags);
+
 	return 0;
 }
 
@@ -1782,6 +1799,8 @@ int omap3isp_resizer_init(struct isp_device *isp)
 
 	init_waitqueue_head(&res->wait);
 	atomic_set(&res->stopping, 0);
+	spin_lock_init(&res->lock);
+
 	return resizer_init_entities(res);
 }
 
diff --git a/drivers/media/platform/omap3isp/ispresizer.h b/drivers/media/platform/omap3isp/ispresizer.h
index 9b01e90..e355211 100644
--- a/drivers/media/platform/omap3isp/ispresizer.h
+++ b/drivers/media/platform/omap3isp/ispresizer.h
@@ -27,6 +27,7 @@
 #ifndef OMAP3_ISP_RESIZER_H
 #define OMAP3_ISP_RESIZER_H
 
+#include <linux/spinlock.h>
 #include <linux/types.h>
 
 /*
@@ -96,6 +97,7 @@ enum resizer_input_entity {
 
 /*
  * struct isp_res_device - OMAP3 ISP resizer module
+ * @lock: Protects formats and crop rectangles between set_selection and IRQ
  * @crop.request: Crop rectangle requested by the user
  * @crop.active: Active crop rectangle (based on hardware requirements)
  */
@@ -116,6 +118,7 @@ struct isp_res_device {
 	enum isp_pipeline_stream_state state;
 	wait_queue_head_t wait;
 	atomic_t stopping;
+	spinlock_t lock;
 
 	struct {
 		struct v4l2_rect request;
-- 
1.8.5.5

Re: [PATCH 0/3] OMAP3 ISP resizer live zoom fixes

[Laurent Pinchart]

On Wednesday 23 July 2014 08:14:45 Raymond Jender wrote:
> Please remove me from this mail list!  If you cannot, please forward to
> someone who can!
> 
> This damn thing is so easy to join,  but a bitch to un-subscribe,
> 
> I did the email with the unsubscribe in the body a few days ago!

Just send an e-mail in plain text format (no HTML) to 
majordomo@vger.kernel.org. The e-mail body must contain

unsubscribe linux-media

and nothing else. You will receive a reply with instructions and a 
confirmation code.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/3] OMAP3 ISP resizer live zoom fixes

[Sakari Ailus]

On Wed, Jul 23, 2014 at 04:57:08PM +0200, Laurent Pinchart wrote:
> Hello,
> 
> This patch set fixes two issues occuring when performing live zoom with the
> OMAP3 ISP resizer.
> 
> The first issue has been observed when using the digital zoom of the live
> application (http://git.ideasonboard.org/omap3-isp-live.git) on a beagleboard.
> It leads to image corruption due to interrupt handling latency. Patch 2/3
> fixes it.
> 
> The second issue is a race condition that I haven't observed in practice. It's
> fixed by patch 3/3. As usual with race conditions and locking, careful review
> will be appreciated.
> 
> Laurent Pinchart (3):
>   omap3isp: resizer: Remove needless variable initializations
>   omap3isp: resizer: Remove slow debugging message from interrupt
>     handler
>   omap3isp: resizer: Protect against races when updating crop

For the set:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk