IMX Scaler / CSC m2m driver.

[Ian Molton]

Hi Philipp!

I've located the cause of the "giant oops" I noted a couple of days ago.

because ctx->icc is recycled, it must be a valid or NULL pointer for kfree().

When an error occurs, the pointer is not reset to NULL, and kfree() crashes.

This helps:

@@ -208,10 +208,11 @@ static void ipu_scaler_work(struct work_struct *work)
  		kfree(ctx->icc);
  		ctx->icc = ipu_image_convert_prepare(ipu_scaler->ipu, &in,
  						     &out, ctx->ctrl,
  						     &ctx->num_tiles);
  		if (IS_ERR(ctx->icc)) {
+			ctx->icc = NULL;
  			schedule_work(&ctx->skip_run);
  			return;
  		}
  	}

This fix "band-aids" it, but I don't think its complete, as IMO, this really
should result in gstreamer giving up, not displaying blank frames.

On the plus side, it does make the whole thing a lot more reliable.

It seems also like this case should be caught a lot earlier.

I've also noticed odd behaviour below width=480;  (height=272)

479: works
478: works
477: gstreamer errors out
476: works (but no video output)
475: ditto
747: ditto
743: gstreamer errors out

The error is:

0:00:00.284476334  1216   0xd01e30 ERROR          v4l2transform gstv4l2transform.c:239:gst_v4l2_transform_set_caps:<v4l2video0convert0> failed to set output caps: video/x-raw, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, framerate=(fraction)24/1, format=(string)BGRx, width=(int)473, height=(int)272
ERROR: from element /GstPipeline:pipeline0/v4l2video0convert:v4l2video0convert0: Device '/dev/video0' cannot capture at 473x272
Additional debug info:
gstv4l2object.c(2967): gst_v4l2_object_set_format (): /GstPipeline:pipeline0/v4l2video0convert:v4l2video0convert0:
Tried to capture at 473x272, but device returned size 472x272
ERROR: pipeline doesn't want to preroll.

which smells like a shift or bitwise test thats not right.

I'm guessing 478 and 479 are rounding up to 480, but the others aren't.

More comments below:

On 23/03/15 10:22, Philipp Zabel wrote:
>> So would I be right in thinking that this test (and comment) are wrong:
>>
>>      /* Cannot downsize more than 8:1 */
>>           if ((out_size << 3) < in_size) {
>>                   dev_err(ipu->dev, "Unsupported downsize\n");
>>                   return -EINVAL;
>>           }
>>
>> (drivers/gpu/ipu-v3/ipu-ic.c)

Ok, so I've changed this thus:

@@ -360,12 +360,12 @@ static int calc_resize_coeffs(struct ipu_ic *ic,
  	if (out_size > 1024) {
  		dev_err(ipu->dev, "Unsupported resize (out_size > 1024)\n");
  		return -EINVAL;
  	}
  
-	/* Cannot downsize more than 8:1 */
-	if ((out_size << 3) < in_size) {
+	/* Cannot downsize more than 4:1 */
+	if ((out_size << 2) < in_size) {
  		dev_err(ipu->dev, "Unsupported downsize\n");
  		return -EINVAL;
  	}

> Exactly. And the error you are hitting is here:
>
> --- a/drivers/gpu/ipu-v3/ipu-ic.c
> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
> @@ -1092,7 +1092,7 @@ struct image_convert_ctx *ipu_image_convert_prepare(struct ipu_soc *ipu,
>
>          /* validate input */
>          if (in->rect.width < 16 || out->rect.width < 16 ||
> -           (in->rect.width / 8) > out->rect.width)
> +           (in->rect.width / 4) > out->rect.width)
>                  return ERR_PTR(-EINVAL);
>
>          /* tile setup */
>
> And obviously the v4l2 mem2mem scaler driver should catch this earlier.

This test looks broken still - surely it should check height too?

Something like:

  @@ -896,11 +896,12 @@ struct image_convert_ctx *ipu_image_convert_prepare(struct ipu_soc *ipu,
  	u32 v_downsize_coeff = 0, h_downsize_coeff = 0;
  	u32 v_resize_coeff, h_resize_coeff;
  
  	/* validate input */
  	if (in->rect.width < 16 || out->rect.width < 16 ||
-	    (in->rect.width / 8) > out->rect.width)
+	    (in->rect.width / 4) > out->rect.width ||
+	    (in->rect.height / 4) > out->rect.height)
  		return ERR_PTR(-EINVAL);

-Ian