Em Thu, 28 Feb 2019 14:59:21 -0300
Mauro Carvalho Chehab <mchehab+samsung@xxxxxxxxxx> escreveu:
> The vim2m driver doesn't enforce that the capture and output
> buffers would have the same size. Do the right thing if the
> buffers are different, zeroing the buffer before writing,
> ensuring that lines will be aligned and it won't write past
> the buffer area.
>
> This is a temporary fix.
Please ignore this. Forgot to change one line (the one with a
comment).
Sending version 4.
>
> A proper fix is to either implement a simple scaler at vim2m,
> or to better define the behaviour of M2M transform drivers
> at V4L2 API with regards to its capability of scaling the
> image or not.
>
> In any case, such changes would deserve a separate patch
> anyway, as it would imply on some behavoral change.
>
> Also, as we have an actual bug of writing data at wrong
> places, let's fix this here, and add a mental note that
> we need to properly address it.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@xxxxxxxxxx>
> ---
> drivers/media/platform/vim2m.c | 117 +++++++++++++++++++++++----------
> 1 file changed, 81 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
> index 5157a59aeb58..1c55c47b151a 100644
> --- a/drivers/media/platform/vim2m.c
> +++ b/drivers/media/platform/vim2m.c
> @@ -267,46 +267,66 @@ static const char *type_name(enum v4l2_buf_type type)
> #define CLIP(__color) \
> (u8)(((__color) > 0xff) ? 0xff : (((__color) < 0) ? 0 : (__color)))
>
> -static void copy_two_pixels(struct vim2m_fmt *in, struct vim2m_fmt *out,
> +static int fast_copy_two_pixels(struct vim2m_q_data *q_data_in,
> + struct vim2m_q_data *q_data_out,
> + u8 **src, u8 **dst, int ypos, bool reverse)
> +{
> + int depth = q_data_out->fmt->depth >> 3;
> +
> + /* Only do fast copy when format and resolution are identical */
> + if (q_data_in->fmt->fourcc != q_data_out->fmt->fourcc ||
> + q_data_in->width != q_data_out->width ||
> + q_data_in->height != q_data_out->height)
> + return 0;
> +
> + if (!reverse) {
> + memcpy(*dst, *src, depth << 1);
> + *src += depth << 1;
> + *dst += depth << 1;
> + return 1;
> + }
> +
> + /* Copy line at reverse order - YUYV format */
> + if (q_data_in->fmt->fourcc == V4L2_PIX_FMT_YUYV) {
> + int u, v, y, y1;
> +
> + *src -= 2;
> +
> + y1 = (*src)[0]; /* copy as second point */
> + u = (*src)[1];
> + y = (*src)[2]; /* copy as first point */
> + v = (*src)[3];
> +
> + *src -= 2;
> +
> + *(*dst)++ = y;
> + *(*dst)++ = u;
> + *(*dst)++ = y1;
> + *(*dst)++ = v;
> + return 1;
> + }
> +
> + /* copy RGB formats in reverse order */
> + memcpy(*dst, *src, depth);
> + memcpy(*dst + depth, *src - depth, depth);
> + *src -= depth << 1;
> + *dst += depth << 1;
> + return 1;
> +}
> +
> +static void copy_two_pixels(struct vim2m_q_data *q_data_in,
> + struct vim2m_q_data *q_data_out,
> u8 **src, u8 **dst, int ypos, bool reverse)
> {
> + struct vim2m_fmt *out = q_data_out->fmt;
> + struct vim2m_fmt *in = q_data_in->fmt;
> u8 _r[2], _g[2], _b[2], *r, *g, *b;
> int i, step;
>
> // If format is the same just copy the data, respecting the width
> - if (in->fourcc == out->fourcc) {
> - int depth = out->depth >> 3;
> -
> - if (reverse) {
> - if (in->fourcc == V4L2_PIX_FMT_YUYV) {
> - int u, v, y, y1;
> -
> - *src -= 2;
> -
> - y1 = (*src)[0]; /* copy as second point */
> - u = (*src)[1];
> - y = (*src)[2]; /* copy as first point */
> - v = (*src)[3];
> -
> - *src -= 2;
> -
> - *(*dst)++ = y;
> - *(*dst)++ = u;
> - *(*dst)++ = y1;
> - *(*dst)++ = v;
> - return;
> - }
> -
> - memcpy(*dst, *src, depth);
> - memcpy(*dst + depth, *src - depth, depth);
> - *src -= depth << 1;
> - } else {
> - memcpy(*dst, *src, depth << 1);
> - *src += depth << 1;
> - }
> - *dst += depth << 1;
> - return;
> - }
> + if (fast_copy_two_pixels(q_data_in, q_data_out,
> + src, dst, ypos, reverse))
> + return;
>
> /* Step 1: read two consecutive pixels from src pointer */
>
> @@ -506,7 +526,9 @@ static int device_process(struct vim2m_ctx *ctx,
> struct vim2m_dev *dev = ctx->dev;
> struct vim2m_q_data *q_data_in, *q_data_out;
> u8 *p_in, *p, *p_out;
> - int width, height, bytesperline, x, y, y_out, start, end, step;
> + unsigned int width, height, bytesperline, bytesperline_out;
> + unsigned int x, y, y_out;
> + int start, end, step;
> struct vim2m_fmt *in, *out;
>
> q_data_in = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> @@ -516,8 +538,15 @@ static int device_process(struct vim2m_ctx *ctx,
> bytesperline = (q_data_in->width * q_data_in->fmt->depth) >> 3;
>
> q_data_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> + bytesperline_out = (q_data_out->width * q_data_out->fmt->depth) >> 3;
> out = q_data_out->fmt;
>
> + /* Crop to the limits of the destination image */
> + if (width > q_data_out->width)
> + width = q_data_out->width;
> + if (height > q_data_out->height)
> + height = q_data_out->height;
> +
> p_in = vb2_plane_vaddr(&in_vb->vb2_buf, 0);
> p_out = vb2_plane_vaddr(&out_vb->vb2_buf, 0);
> if (!p_in || !p_out) {
> @@ -526,6 +555,17 @@ static int device_process(struct vim2m_ctx *ctx,
> return -EFAULT;
> }
>
> + /*
> + * FIXME: instead of cropping the image and zeroing any
> + * extra data, the proper behavior is to either scale the
> + * data or report that scale is not supported (with depends
> + * on some API for such purpose).
> + */
> +
> + /* Image size is different. Zero buffer first */
> + if (q_data_in->width != q_data_out->width ||
> + q_data_in->height != q_data_out->height)
> + memset(p_out, 0, q_data_out->sizeimage);
> out_vb->sequence = get_q_data(ctx,
> V4L2_BUF_TYPE_VIDEO_CAPTURE)->sequence++;
> in_vb->sequence = q_data_in->sequence++;
> @@ -547,8 +587,13 @@ static int device_process(struct vim2m_ctx *ctx,
> p += bytesperline - (q_data_in->fmt->depth >> 3);
>
> for (x = 0; x < width >> 1; x++)
> - copy_two_pixels(in, out, &p, &p_out, y_out,
> + copy_two_pixels(q_data_in, q_data_out, &p, &p_out, y_out,
> ctx->mode & MEM2MEM_HFLIP);
> +
> + /* Go to the next line at the out buffer*/
> + if (width < q_data_out->width)
> + p_out += ((q_data_out->width - width)
> + * q_data_out->fmt->depth) >> 3;
> }
>
> return 0;
Thanks,
Mauro
