Hi Steve,

thanks for the update.

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

Hi Philipp,


On 09/06/2016 02:26 AM, Philipp Zabel wrote:

quoted

Hi Steve,

Am Mittwoch, den 17.08.2016, 17:50 -0700 schrieb Steve Longerbeam:

quoted

This patch implements complete image conversion support to ipu-ic,
with tiling to support scaling to and from images up to 4096x4096.
Image rotation is also supported.

The internal API is subsystem agnostic (no V4L2 dependency except
for the use of V4L2 fourcc pixel formats).

Callers prepare for image conversion by calling
ipu_image_convert_prepare(), which initializes the parameters of
the conversion.

... and possibly allocates intermediate buffers for rotation support.
This should be documented somewhere, with a node that v4l2 users should
be doing this during REQBUFS.

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.

quoted

quoted

  The caller passes in the ipu_ic task to use for
the conversion, the input and output image formats, a rotation mode,
and a completion callback and completion context pointer:

struct image_converter_ctx *
ipu_image_convert_prepare(struct ipu_ic *ic,
                           struct ipu_image *in, struct ipu_image *out,
                           enum ipu_rotate_mode rot_mode,
                           image_converter_cb_t complete,
                           void *complete_context);

As I commented on the other patch, I think the image_convert functions
should use a separate handle for the image conversion queues that sit on
top of the ipu_ic task handles.

Here is a new prototype I came up with:

struct ipu_image_convert_ctx *
ipu_image_convert_prepare(struct ipu_soc *ipu, enum ipu_ic_task ic_task,
               struct ipu_image *in, struct ipu_image *out,
               enum ipu_rotate_mode rot_mode,
               ipu_image_convert_cb_t complete,
               void *complete_context);

In other words, the ipu_ic handle is replaced by the IPU handle and IC task
that are requested for carrying out the conversion.

Looks good to me for now.

The image converter will acquire the ipu_ic handle internally, whenever 
there
are queued contexts to that IC task (which I am calling a 'struct 
ipu_image_convert_chan').
This way the IC handle can be shared by all contexts using that IC task. 
After all
contexts have been freed from the (struct 
ipu_image_convert_chan)->ctx_list queue,
the ipu_ic handle is freed.

The ipu_ic handle is acquired in get_ipu_resources() and freed in 
release_ipu_resources(),
along with all the other IPU resources that *could possibly be needed* 
in that
ipu_image_convert_chan by future contexts (*all* idmac channels, *all* 
irqs).

Ok.

[...]

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?

quoted

quoted

+struct ic_task_channels {
+	int in;
+	int out;
+	int rot_in;
+	int rot_out;
+	int vdi_in_p;
+	int vdi_in;
+	int vdi_in_n;

The vdi channels are unused.

Well, I'd prefer to keep the VDI channels. It's quite possible we
can add motion compensated deinterlacing support using the
PRP_VF task to the image converter in the future.

Indeed.

quoted

quoted

+struct image_converter_ctx {
+	struct image_converter *cvt;
+
+	image_converter_cb_t complete;
+	void *complete_context;
+
+	/* Source/destination image data and rotation mode */
+	struct ipu_ic_image in;
+	struct ipu_ic_image out;
+	enum ipu_rotate_mode rot_mode;
+
+	/* intermediate buffer for rotation */
+	struct ipu_ic_dma_buf rot_intermediate[2];

No need to change it now, but I assume these could be per IC task
instead of per context.

Actually no. The rotation intermediate buffers have the dimension
of a single tile, so they must remain in the context struct.

I see. The per task intermediate buffer would have to be the maximum
size, so this would only ever make sense when rotating multiple large
RGB streams simultaneously. I think we can reasonably ignore this use
case.

[...]

quoted

quoted

+		.fourcc	= V4L2_PIX_FMT_RGB565,
+		.bpp    = 16,

bpp is only ever used in bytes, not bits (always divided by 8).
Why not make this bytes_per_pixel or pixel_stride = 2.

Actually bpp is used to calculate *total* tile sizes and *total* bytes
per line. For the planar 4:2:0 formats that means it must be specified
in bits.

Ok for total size of chroma subsampled planar formats.

[...]

quoted

Most of the following code seems to be running under one big spinlock.
Is this really necessary?

You're right, convert_stop(), convert_start(), and init_idmac_channel() are
only calling the ipu_ic lower level primitives. So they don't require 
the irqlock.
I did remove the "hold irqlock when calling" comment for those. However
they are called embedded in the irq handling, so it would be cumbersome
to drop the lock there only because they don't need it. We can revisit the
lock handling later if you see some room for optimization there.

Alright, let's call it future performance optimization potential.

quoted

quoted

+static irqreturn_t ipu_ic_norotate_irq(int irq, void *data)
+{

[...]

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.

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.

The remaining issues are minor and can be fixed later.
I'll apply this as is.

regards
Philipp