Hi Hans,
thank you for your comments!
On Mon, 2018-12-03 at 16:21 +0100, Hans Verkuil wrote:
> On 12/03/2018 12:48 PM, Philipp Zabel wrote:
> > Add a single imx-media mem2mem video device that uses the IPU IC PP
> > (image converter post processing) task for scaling and colorspace
> > conversion.
> > On i.MX6Q/DL SoCs with two IPUs currently only the first IPU is used.
> >
> > The hardware only supports writing to destination buffers up to
> > 1024x1024 pixels in a single pass, arbitrary sizes can be achieved
> > by rendering multiple tiles per frame.
> >
> > Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> > [slongerb...@gmail.com: use ipu_image_convert_adjust(), fix
> > device_run() error handling]
> > Signed-off-by: Steve Longerbeam <slongerb...@gmail.com>
> > ---
> > Changes since v4:
> > - No functional changes.
> > - Dropped deprecated TODO comment. This driver has no interaction with
> > the IC task v4l2 subdevices.
> > - Dropped ipu-v3 patches, those are merged independently via imx-drm.
>
> These land in kernel 4.21? Or are they already in 4.20?
These landed in v5.0-rc1.
I've sent a v6 that should already address most of the issues you
mentioned, except those I answer to below:
[...]
> > +static int mem2mem_queue_setup(struct vb2_queue *vq, unsigned int
> > *nbuffers,
> > + unsigned int *nplanes, unsigned int sizes[],
> > + struct device *alloc_devs[])
> > +{
> > + struct mem2mem_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct mem2mem_q_data *q_data;
> > + unsigned int count = *nbuffers;
> > + struct v4l2_pix_format *pix;
> > +
> > + q_data = get_q_data(ctx, vq->type);
> > + pix = &q_data->cur_fmt;
> > +
> > + *nplanes = 1;
> > + *nbuffers = count;
> > + sizes[0] = pix->sizeimage;
>
> This needs to be modified slightly to support PREPARE_BUF.
Do you mean CREATE_BUFS? I have fixed queue_setup to do check the size
when called from CREATE_BUFS, but I don't understand what needs to be
changed for PREPARE_BUF.
[...]
> > + ret = ipu_degrees_to_rot_mode(&rot_mode, rotate, hflip, vflip);
> > + if (ret)
> > + return ret;
> > +
> > + if (rot_mode != ctx->rot_mode) {
> > + struct vb2_queue *cap_q;
> > +
> > + cap_q = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > + V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > + if (vb2_is_streaming(cap_q))
> > + return -EBUSY;
> > +
> > + ctx->rot_mode = rot_mode;
> > + ctx->rotate = rotate;
> > + ctx->hflip = hflip;
> > + ctx->vflip = vflip;
>
> Changing the orientation should also change the format (swaps width and
> height), but I see no sign of that in the code.
A +/-90° orientation change (in addition to changing rot_mode) should
have the same effect as a S_FMT with swapped width/height?
I was unsure about what the "display window" meant in the
V4L2_CID_ROTATE description [1], but the wording sounded like the user
was expected to call S_FMT manually anyway:
V4L2_CID_ROTATE (integer)
Rotates the image by specified angle.
Common angles are 90, 270 and
180. Rotating the image to 90 and 270
will reverse the height and
width of the display window. It is
necessary to set the new height
and width of the picture using the
VIDIOC_S_FMT ioctl according to
the rotation angle selected.
[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/control.html
> Rotating also changes the buffer size for YUV 4:2:0, so you probably should
> use vb2_is_busy() in the test above.
>
> There is another issue involved. See this old patch (never got merged):
>
> https://patchwork.linuxtv.org/patch/44424/
>
> I think this was discussed as well on irc not too long ago, along there I
> suggested calling the ops queue_prepare and queue_finish.
Do you mean the V4L2_CID_ROTATE should be grabbed while the queue is
busy?
regards
Philipp
