Re: [PATCH v4 3/4] gpu: ipu-ic: Add complete image conversion support with tiling

[Steve Longerbeam]

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:
> > 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).
> > > > +#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.

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

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