Re: [PATCH v3] media: vim2m: better handle cap/out buffers with different sizes

[Mauro Carvalho Chehab <mchehab+samsung@kernel.org>]

Em Thu, 28 Feb 2019 14:59:21 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> 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@kernel.org>
> ---
>  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