Hi Steve,
On Thu, 2018-07-05 at 15:09 -0700, Steve Longerbeam wrote:
[...]
> > +static int mem2mem_try_fmt(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
[...]
> > + /*
> > + * Horizontally/vertically chroma subsampled formats must have even
> > + * width/height.
> > + */
> > + switch (f->fmt.pix.pixelformat) {
> > + case V4L2_PIX_FMT_YUV420:
> > + case V4L2_PIX_FMT_YVU420:
> > + case V4L2_PIX_FMT_NV12:
> > + walign = 1;
> > + halign = 1;
> > + break;
> > + case V4L2_PIX_FMT_YUV422P:
> > + case V4L2_PIX_FMT_NV16:
> > + walign = 1;
> > + halign = 0;
> > + break;
> > + default:
>
> The default case should init walign, otherwise for OUTPUT direction,
> walign may not get initialized at all, see below...
Yes, thank you.
> > + halign = 0;
> > + break;
> > + }
> > + if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > + /*
> > + * The IC burst reads 8 pixels at a time. Reading beyond the
> > + * end of the line is usually acceptable. Those pixels are
> > + * ignored, unless the IC has to write the scaled line in
> > + * reverse.
> > + */
> > + if (!ipu_rot_mode_is_irt(ctx->rot_mode) &&
> > + ctx->rot_mode && IPU_ROT_BIT_HFLIP)
> > + walign = 3;
>
> This looks wrong. Do you mean:
>
> if (ipu_rot_mode_is_irt(ctx->rot_mode) || (ctx->rot_mode & IPU_ROT_BIT_HFLIP))
> walign = 3;
> else
> walign = 1;
The input DMA burst width alignment is only necessary if the lines are
scanned from right to left (that is, if HF is enabled) in the scaling
step.
If the rotator is used, the flipping is done in the rotation step
instead, so the alignment restriction would be on the width of the
intermediate tile (and thus on the output height). This is already
covered by the rotator 8x8 pixel block alignment.
> That is, require 8 byte width alignment for IRT or if HFLIP is enabled.
No, I specifically meant (!IRT && HFLIP).
The rotator itself doesn't cause any input alignment restrictions, we
just have to make sure that the intermediate tiles after scaling are 8x8
aligned.
> Also, why not simply call ipu_image_convert_adjust() in
> mem2mem_try_fmt()? If there is something missing in the former
> function, then it should be added there, instead of adding the
> missing checks in mem2mem_try_fmt().
ipu_image_convert_adjust tries to adjust both input and output image at
the same time, here we just have the format of either input or output
image. Do you suggest to split this function into an input and an output
version?
regards
Philipp
