On 09/16/2016 07:16 AM, Philipp Zabel wrote:

Hi Steve,

thanks for the update.

Am Mittwoch, den 14.09.2016, 18:45 -0700 schrieb Steve Longerbeam:

quoted

I added comment headers for all the image conversion prototypes.
It caused bloat in imx-ipu-v3.h, so I moved it to a new header:
include/video/imx-image-convert.h, but let me know if we should put
this somewhere else and/or under Documentation/ somewhere.

I think that is the right place already. imx-image-convert.h could be
renamed to imx-ipu-image-convert.h, to make clear that this is about the
IPU image converter.

Ok, I'll send another update with the name change in the next
version (v7).

quoted

quoted

quoted

+#define MIN_W     128
+#define MIN_H     128

Where does this minimum come from?

Nowhere really :) This is just some sane minimums, to pass
to clamp_align() when aligning input/output width/height in
ipu_image_convert_adjust().

Let's use hardware minimum in the low level code. Sane defaults are for
the V4L2 API. Would that be 8x2 pixels per input tile?

I searched the imx6 reference manual, I can't find any mention
of width/height minimums for the IC. So I suppose 8x2 would be fine,
or maybe 16x8, to account for planar and IRT conversions.

quoted

quoted

quoted

+	if (ipu_rot_mode_is_irt(ctx->rot_mode)) {
+		/* this is a rotation operation, just ignore */
+		spin_unlock_irqrestore(&cvt->irqlock, flags);
+		return IRQ_HANDLED;
+	}

Why enable the out_chan EOF irq at all when using the IRT mode?

Because (see above), all the IPU resources that might be needed
for any conversion context that is queued to a image conversion
channel (IC task) are acquired when the first context is queued,
including rotation resources. So by acquiring the non-rotation EOF
irq, it will get fielded even for rotation conversions, so we have to
handle it.

There is nothing wrong with acquiring the irq. It could still be
disabled while it is not needed.

It would be difficult to disable the irq. Remember the irq handlers must
field all EOF interrupts in an ipu_image_convert_chan (IC task). If one
context in that channel disables the irq, it will break other runnings
contexts in that channel that are using it.

quoted

quoted

quoted

+/* Adjusts input/output images to IPU restrictions */
+int ipu_image_convert_adjust(struct ipu_image *in, struct ipu_image *out,
+			     enum ipu_rotate_mode rot_mode)
+{
+	const struct ipu_ic_pixfmt *infmt, *outfmt;
+	unsigned int num_in_rows, num_in_cols;
+	unsigned int num_out_rows, num_out_cols;
+	u32 w_align, h_align;
+
+	infmt = ipu_ic_get_format(in->pix.pixelformat);
+	outfmt = ipu_ic_get_format(out->pix.pixelformat);
+
+	/* set some defaults if needed */

Is this our task at all?

ipu_image_convert_adjust() is meant to be called by v4l2 try_format(),
which should never return EINVAL but should return a supported format
when the passed format is not supported. So I added this here to return
some default pixel formats and width/heights if needed.

I'd prefer to move this into the mem2mem driver try_format, then.

We could move the 0 width/height checks to v4l2, but the pixel
format defaults should probably remain in ipu-image-convert, since
it knows what formats it supports converting to/from.

Steve