media: i.MX27 camera: Add resizing support.

Message ID 1329219332-27620-1-git-send-email-javier.martin@vista-silicon.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Javier Martin Feb. 14, 2012, 11:35 a.m. UTC
  If the attached video sensor cannot provide the
requested image size, try to use resizing engine
included in the eMMa-PrP IP.

This patch supports both averaging and bilinear
algorithms.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/video/mx2_camera.c |  251 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 249 insertions(+), 2 deletions(-)
  

Comments

Guennadi Liakhovetski Feb. 20, 2012, 2:13 p.m. UTC | #1
On Tue, 14 Feb 2012, Javier Martin wrote:

> If the attached video sensor cannot provide the
> requested image size, try to use resizing engine
> included in the eMMa-PrP IP.
> 
> This patch supports both averaging and bilinear
> algorithms.
> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  drivers/media/video/mx2_camera.c |  251 +++++++++++++++++++++++++++++++++++++-
>  1 files changed, 249 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index 9e84368..6516ee4 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -19,6 +19,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/errno.h>
>  #include <linux/fs.h>
> +#include <linux/gcd.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
> @@ -204,10 +205,25 @@
>  #define PRP_INTR_LBOVF		(1 << 7)
>  #define PRP_INTR_CH2OVF		(1 << 8)
>  
> +/* Resizing registers */
> +#define PRP_RZ_VALID_TBL_LEN(x)	((x) << 24)
> +#define PRP_RZ_VALID_BILINEAR	(1 << 31)
> +
>  #define mx27_camera_emma(pcdev)	(cpu_is_mx27() && pcdev->use_emma)
>  
>  #define MAX_VIDEO_MEM	16
>  
> +#define RESIZE_NUM_MIN	1
> +#define RESIZE_NUM_MAX	20
> +#define BC_COEF		3
> +#define SZ_COEF		(1 << BC_COEF)
> +
> +#define RESIZE_DIR_H	0
> +#define RESIZE_DIR_V	1
> +
> +#define RESIZE_ALGO_BILINEAR 0
> +#define RESIZE_ALGO_AVERAGING 1
> +
>  struct mx2_prp_cfg {
>  	int channel;
>  	u32 in_fmt;
> @@ -217,6 +233,13 @@ struct mx2_prp_cfg {
>  	u32 irq_flags;
>  };
>  
> +/* prp resizing parameters */
> +struct emma_prp_resize {
> +	int		algo; /* type of algorithm used */
> +	int		len; /* number of coefficients */
> +	unsigned char	s[RESIZE_NUM_MAX]; /* table of coefficients */
> +};
> +
>  /* prp configuration for a client-host fmt pair */
>  struct mx2_fmt_cfg {
>  	enum v4l2_mbus_pixelcode	in_fmt;
> @@ -278,6 +301,8 @@ struct mx2_camera_dev {
>  	dma_addr_t		discard_buffer_dma;
>  	size_t			discard_size;
>  	struct mx2_fmt_cfg	*emma_prp;
> +	struct emma_prp_resize	resizing[2];
> +	unsigned int		s_width, s_height;
>  	u32			frame_count;
>  	struct vb2_alloc_ctx	*alloc_ctx;
>  };
> @@ -687,7 +712,7 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
>  	struct mx2_camera_dev *pcdev = ici->priv;
>  	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
>  
> -	writel((icd->user_width << 16) | icd->user_height,
> +	writel((pcdev->s_width << 16) | pcdev->s_height,
>  	       pcdev->base_emma + PRP_SRC_FRAME_SIZE);
>  	writel(prp->cfg.src_pixel,
>  	       pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
> @@ -707,6 +732,74 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
>  	writel(prp->cfg.irq_flags, pcdev->base_emma + PRP_INTR_CNTL);
>  }
>  
> +static void mx2_prp_resize_commit(struct mx2_camera_dev *pcdev)
> +{
> +	int dir;
> +
> +	for (dir = RESIZE_DIR_H; dir <= RESIZE_DIR_V; dir++) {
> +		unsigned char *s = pcdev->resizing[dir].s;
> +		int len = pcdev->resizing[dir].len;
> +		unsigned int coeff[2] = {0, 0};
> +		unsigned int valid  = 0;
> +		int i;
> +
> +		if (len == 0)
> +			continue;
> +
> +		for (i = RESIZE_NUM_MAX - 1; i >= 0; i--) {
> +			int j;
> +
> +			j = i > 9 ? 1 : 0;
> +			coeff[j] = (coeff[j] << BC_COEF) |
> +			(s[i] & (SZ_COEF - 1));

Please, remember to indent line continuations.

> +
> +			if (i == 5 || i == 15)
> +				coeff[j] <<= 1;
> +
> +			valid = (valid << 1) | (s[i] >> BC_COEF);
> +		}
> +
> +		valid |= PRP_RZ_VALID_TBL_LEN(len);
> +
> +		if (pcdev->resizing[dir].algo == RESIZE_ALGO_BILINEAR)
> +			valid |= PRP_RZ_VALID_BILINEAR;
> +
> +		if (pcdev->emma_prp->cfg.channel == 1) {

Maybe put horizontal and vertical register addresses in an array too to 
avoid this "if?" Just an idea - if you don't think, the code would look 
nicer, just leave as is.

> +			if (dir == RESIZE_DIR_H) {
> +				writel(coeff[0], pcdev->base_emma +
> +							PRP_CH1_RZ_HORI_COEF1);
> +				writel(coeff[1], pcdev->base_emma +
> +							PRP_CH1_RZ_HORI_COEF2);
> +				writel(valid, pcdev->base_emma +
> +							PRP_CH1_RZ_HORI_VALID);
> +			} else {
> +				writel(coeff[0], pcdev->base_emma +
> +							PRP_CH1_RZ_VERT_COEF1);
> +				writel(coeff[1], pcdev->base_emma +
> +							PRP_CH1_RZ_VERT_COEF2);
> +				writel(valid, pcdev->base_emma +
> +							PRP_CH1_RZ_VERT_VALID);
> +			}
> +		} else {
> +			if (dir == RESIZE_DIR_H) {
> +				writel(coeff[0], pcdev->base_emma +
> +							PRP_CH2_RZ_HORI_COEF1);
> +				writel(coeff[1], pcdev->base_emma +
> +							PRP_CH2_RZ_HORI_COEF2);
> +				writel(valid, pcdev->base_emma +
> +							PRP_CH2_RZ_HORI_VALID);
> +			} else {
> +				writel(coeff[0], pcdev->base_emma +
> +							PRP_CH2_RZ_VERT_COEF1);
> +				writel(coeff[1], pcdev->base_emma +
> +							PRP_CH2_RZ_VERT_COEF2);
> +				writel(valid, pcdev->base_emma +
> +							PRP_CH2_RZ_VERT_VALID);
> +			}
> +		}
> +	}
> +}
> +
>  static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>  {
>  	struct soc_camera_device *icd = soc_camera_from_vb2q(q);
> @@ -773,6 +866,8 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>  		list_add_tail(&pcdev->buf_discard[1].queue,
>  				      &pcdev->discard);
>  
> +		mx2_prp_resize_commit(pcdev);
> +
>  		mx27_camera_emma_buf_init(icd, bytesperline);
>  
>  		if (prp->cfg.channel == 1) {
> @@ -1059,6 +1154,119 @@ static int mx2_camera_get_formats(struct soc_camera_device *icd,
>  	return formats;
>  }
>  
> +static int mx2_emmaprp_resize(struct mx2_camera_dev *pcdev,
> +			      struct v4l2_mbus_framefmt *mf_in,
> +			      struct v4l2_pix_format *pix_out)
> +{
> +	int num, den;
> +	unsigned long m;
> +	int i, dir;
> +
> +	for (dir = RESIZE_DIR_H; dir <= RESIZE_DIR_V; dir++) {
> +		unsigned char *s = pcdev->resizing[dir].s;
> +		int len = 0;
> +		int in, out;
> +
> +		if (dir == RESIZE_DIR_H) {
> +			in = mf_in->width;
> +			out = pix_out->width;
> +		} else {
> +			in = mf_in->height;
> +			out = pix_out->height;
> +		}
> +
> +		if (in < out)
> +			return -EINVAL;
> +		else if (in == out)
> +			continue;
> +
> +		/* Calculate ratio */
> +		m = gcd(in, out);
> +		num = in / m;
> +		den = out / m;
> +		if (num > RESIZE_NUM_MAX)
> +			return -EINVAL;
> +
> +		if ((num >= 2 * den) && (den == 1) &&
> +		    (num < 9) && (!(num & 0x01))) {
> +			int sum = 0;
> +			int j;
> +
> +			/* Average scaling for > 2:1 ratios */

">=" rather than ">"

> +			/* Support can be added for num >=9 and odd values */

So, this is only used for downscaling by 1/2, 1/4, 1/6, and 1/8?

> +
> +			pcdev->resizing[dir].algo = RESIZE_ALGO_AVERAGING;
> +			len = num;
> +
> +			for (i = 0; i < (len / 2); i++)
> +				s[i] = 8;
> +
> +			do {
> +				for (i = 0; i < (len / 2); i++) {
> +					s[i] = s[i] >> 1;
> +					sum = 0;
> +					for (j = 0; j < (len / 2); j++)
> +						sum += s[j];
> +					if (sum == 4)
> +						break;
> +				}
> +			} while (sum != 4);
> +
> +			for (i = (len / 2); i < len; i++)
> +				s[i] = s[len - i - 1];
> +
> +			s[len - 1] |= SZ_COEF;
> +		} else {
> +			/* bilinear scaling for < 2:1 ratios */
> +			int v; /* overflow counter */
> +			int coeff, nxt; /* table output */
> +			int in_pos_inc = 2 * den;
> +			int out_pos = num;
> +			int out_pos_inc = 2 * num;
> +			int init_carry = num - den;
> +			int carry = init_carry;
> +
> +			pcdev->resizing[dir].algo = RESIZE_ALGO_BILINEAR;
> +			v = den + in_pos_inc;
> +			do {
> +				coeff = v - out_pos;
> +				out_pos += out_pos_inc;
> +				carry += out_pos_inc;
> +				for (nxt = 0; v < out_pos; nxt++) {
> +					v += in_pos_inc;
> +					carry -= in_pos_inc;
> +				}
> +
> +				if (len > RESIZE_NUM_MAX)
> +					return -EINVAL;
> +
> +				coeff = ((coeff << BC_COEF) +
> +					(in_pos_inc >> 1)) / in_pos_inc;
> +
> +				if (coeff >= (SZ_COEF - 1))
> +					coeff--;
> +
> +				coeff |= SZ_COEF;
> +				s[len] = (unsigned char)coeff;
> +				len++;
> +
> +				for (i = 1; i < nxt; i++) {
> +					if (len >= RESIZE_NUM_MAX)
> +						return -EINVAL;
> +					s[len] = 0;
> +					len++;
> +				}
> +			} while (carry != init_carry);
> +		}
> +		pcdev->resizing[dir].len = len;
> +		if (dir == RESIZE_DIR_H)
> +			mf_in->width = mf_in->width * den / num;
> +		else
> +			mf_in->height = mf_in->height * den / num;

Aren't your calculations exact, so that here you can just use 
pix_out->width and pix_out->height?

> +	}
> +	return 0;
> +}
> +
>  static int mx2_camera_set_fmt(struct soc_camera_device *icd,
>  			       struct v4l2_format *f)
>  {
> @@ -1070,6 +1278,9 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
>  	struct v4l2_mbus_framefmt mf;
>  	int ret;
>  
> +	dev_dbg(icd->parent, "%s: requested params: width = %d, height = %d\n",
> +		__func__, pix->width, pix->height);
> +
>  	xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
>  	if (!xlate) {
>  		dev_warn(icd->parent, "Format %x not found\n",
> @@ -1087,6 +1298,18 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
>  	if (ret < 0 && ret != -ENOIOCTLCMD)
>  		return ret;
>  
> +	/* Store width and height returned by the sensor for resizing */
> +	pcdev->s_width = mf.width;
> +	pcdev->s_height = mf.height;
> +	dev_dbg(icd->parent, "%s: sensor params: width = %d, height = %d\n",
> +		__func__, pcdev->s_width, pcdev->s_height);
> +
> +	memset(pcdev->resizing, 0, sizeof(struct emma_prp_resize) << 1);

I think, just sizeof(pcdev->resizing) will do the trick.

> +	if (mf.width != pix->width || mf.height != pix->height) {
> +		if (mx2_emmaprp_resize(pcdev, &mf, pix) < 0)
> +			return -EINVAL;

Hmmm... This looks like a regression, not an improvement - you return more 
errors now, than without this resizing support. Wouldn't it be possible to 
fall back to the current behaviour if prp resizing is impossible?

> +	}
> +
>  	if (mf.code != xlate->code)
>  		return -EINVAL;
>  
> @@ -1100,6 +1323,9 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
>  		pcdev->emma_prp = mx27_emma_prp_get_format(xlate->code,
>  						xlate->host_fmt->fourcc);
>  
> +	dev_dbg(icd->parent, "%s: returned params: width = %d, height = %d\n",
> +		__func__, pix->width, pix->height);
> +
>  	return 0;
>  }
>  
> @@ -1109,11 +1335,16 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,
>  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>  	const struct soc_camera_format_xlate *xlate;
>  	struct v4l2_pix_format *pix = &f->fmt.pix;
> -	struct v4l2_mbus_framefmt mf;
>  	__u32 pixfmt = pix->pixelformat;
> +	struct v4l2_mbus_framefmt mf;

This swap doesn't seem to be needed.

> +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> +	struct mx2_camera_dev *pcdev = ici->priv;
>  	unsigned int width_limit;
>  	int ret;
>  
> +	dev_dbg(icd->parent, "%s: requested params: width = %d, height = %d\n",
> +		__func__, pix->width, pix->height);
> +
>  	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
>  	if (pixfmt && !xlate) {
>  		dev_warn(icd->parent, "Format %x not found\n", pixfmt);
> @@ -1163,6 +1394,19 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Store width and height returned by the sensor for resizing */
> +	pcdev->s_width = mf.width;
> +	pcdev->s_height = mf.height;

You don't need these in .try_fmt().

> +	dev_dbg(icd->parent, "%s: sensor params: width = %d, height = %d\n",
> +		__func__, pcdev->s_width, pcdev->s_height);
> +
> +	/* If the sensor does not support image size try PrP resizing */
> +	memset(pcdev->resizing, 0, sizeof(struct emma_prp_resize) << 1);

Same for sizeof()

> +	if (mf.width != pix->width || mf.height != pix->height) {
> +		if (mx2_emmaprp_resize(pcdev, &mf, pix) < 0)
> +			return -EINVAL;

.try_fmt() really shouldn't fail.

> +	}
> +
>  	if (mf.field == V4L2_FIELD_ANY)
>  		mf.field = V4L2_FIELD_NONE;
>  	/*
> @@ -1181,6 +1425,9 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,
>  	pix->field	= mf.field;
>  	pix->colorspace	= mf.colorspace;
>  
> +	dev_dbg(icd->parent, "%s: returned params: width = %d, height = %d\n",
> +		__func__, pix->width, pix->height);
> +
>  	return 0;
>  }
>  
> -- 
> 1.7.0.4

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Javier Martin Feb. 21, 2012, 8:17 a.m. UTC | #2
Hi Guennadi,
thank you for your review.

On 20 February 2012 15:13, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>> @@ -707,6 +732,74 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
>>       writel(prp->cfg.irq_flags, pcdev->base_emma + PRP_INTR_CNTL);
>>  }
>>
>> +static void mx2_prp_resize_commit(struct mx2_camera_dev *pcdev)
>> +{
>> +     int dir;
>> +
>> +     for (dir = RESIZE_DIR_H; dir <= RESIZE_DIR_V; dir++) {
>> +             unsigned char *s = pcdev->resizing[dir].s;
>> +             int len = pcdev->resizing[dir].len;
>> +             unsigned int coeff[2] = {0, 0};
>> +             unsigned int valid  = 0;
>> +             int i;
>> +
>> +             if (len == 0)
>> +                     continue;
>> +
>> +             for (i = RESIZE_NUM_MAX - 1; i >= 0; i--) {
>> +                     int j;
>> +
>> +                     j = i > 9 ? 1 : 0;
>> +                     coeff[j] = (coeff[j] << BC_COEF) |
>> +                     (s[i] & (SZ_COEF - 1));
>
> Please, remember to indent line continuations.

Yes, sorry.

>
>> +
>> +                     if (i == 5 || i == 15)
>> +                             coeff[j] <<= 1;
>> +
>> +                     valid = (valid << 1) | (s[i] >> BC_COEF);
>> +             }
>> +
>> +             valid |= PRP_RZ_VALID_TBL_LEN(len);
>> +
>> +             if (pcdev->resizing[dir].algo == RESIZE_ALGO_BILINEAR)
>> +                     valid |= PRP_RZ_VALID_BILINEAR;
>> +
>> +             if (pcdev->emma_prp->cfg.channel == 1) {
>
> Maybe put horizontal and vertical register addresses in an array too to
> avoid this "if?" Just an idea - if you don't think, the code would look
> nicer, just leave as is.

I don't know about that, we can't avoid the fact that the code would
look more compact but I think it would be less clear.

>> +                     if (dir == RESIZE_DIR_H) {
>> +                             writel(coeff[0], pcdev->base_emma +
>> +                                                     PRP_CH1_RZ_HORI_COEF1);
>> +                             writel(coeff[1], pcdev->base_emma +
>> +                                                     PRP_CH1_RZ_HORI_COEF2);
>> +                             writel(valid, pcdev->base_emma +
>> +                                                     PRP_CH1_RZ_HORI_VALID);
>> +                     } else {
>> +                             writel(coeff[0], pcdev->base_emma +
>> +                                                     PRP_CH1_RZ_VERT_COEF1);
>> +                             writel(coeff[1], pcdev->base_emma +
>> +                                                     PRP_CH1_RZ_VERT_COEF2);
>> +                             writel(valid, pcdev->base_emma +
>> +                                                     PRP_CH1_RZ_VERT_VALID);
>> +                     }
>> +             } else {
>> +                     if (dir == RESIZE_DIR_H) {
>> +                             writel(coeff[0], pcdev->base_emma +
>> +                                                     PRP_CH2_RZ_HORI_COEF1);
>> +                             writel(coeff[1], pcdev->base_emma +
>> +                                                     PRP_CH2_RZ_HORI_COEF2);
>> +                             writel(valid, pcdev->base_emma +
>> +                                                     PRP_CH2_RZ_HORI_VALID);
>> +                     } else {
>> +                             writel(coeff[0], pcdev->base_emma +
>> +                                                     PRP_CH2_RZ_VERT_COEF1);
>> +                             writel(coeff[1], pcdev->base_emma +
>> +                                                     PRP_CH2_RZ_VERT_COEF2);
>> +                             writel(valid, pcdev->base_emma +
>> +                                                     PRP_CH2_RZ_VERT_VALID);
>> +                     }
>> +             }
>> +     }
>> +}
>> +
>>  static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>>  {
>>       struct soc_camera_device *icd = soc_camera_from_vb2q(q);
>> @@ -773,6 +866,8 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>>               list_add_tail(&pcdev->buf_discard[1].queue,
>>                                     &pcdev->discard);
>>
>> +             mx2_prp_resize_commit(pcdev);
>> +
>>               mx27_camera_emma_buf_init(icd, bytesperline);
>>
>>               if (prp->cfg.channel == 1) {
>> @@ -1059,6 +1154,119 @@ static int mx2_camera_get_formats(struct soc_camera_device *icd,
>>       return formats;
>>  }
>>
>> +static int mx2_emmaprp_resize(struct mx2_camera_dev *pcdev,
>> +                           struct v4l2_mbus_framefmt *mf_in,
>> +                           struct v4l2_pix_format *pix_out)
>> +{
>> +     int num, den;
>> +     unsigned long m;
>> +     int i, dir;
>> +
>> +     for (dir = RESIZE_DIR_H; dir <= RESIZE_DIR_V; dir++) {
>> +             unsigned char *s = pcdev->resizing[dir].s;
>> +             int len = 0;
>> +             int in, out;
>> +
>> +             if (dir == RESIZE_DIR_H) {
>> +                     in = mf_in->width;
>> +                     out = pix_out->width;
>> +             } else {
>> +                     in = mf_in->height;
>> +                     out = pix_out->height;
>> +             }
>> +
>> +             if (in < out)
>> +                     return -EINVAL;
>> +             else if (in == out)
>> +                     continue;
>> +
>> +             /* Calculate ratio */
>> +             m = gcd(in, out);
>> +             num = in / m;
>> +             den = out / m;
>> +             if (num > RESIZE_NUM_MAX)
>> +                     return -EINVAL;
>> +
>> +             if ((num >= 2 * den) && (den == 1) &&
>> +                 (num < 9) && (!(num & 0x01))) {
>> +                     int sum = 0;
>> +                     int j;
>> +
>> +                     /* Average scaling for > 2:1 ratios */
>
> ">=" rather than ">"

You are right.

>> +                     /* Support can be added for num >=9 and odd values */
>
> So, this is only used for downscaling by 1/2, 1/4, 1/6, and 1/8?

Yes.

>> +
>> +                     pcdev->resizing[dir].algo = RESIZE_ALGO_AVERAGING;
>> +                     len = num;
>> +
>> +                     for (i = 0; i < (len / 2); i++)
>> +                             s[i] = 8;
>> +
>> +                     do {
>> +                             for (i = 0; i < (len / 2); i++) {
>> +                                     s[i] = s[i] >> 1;
>> +                                     sum = 0;
>> +                                     for (j = 0; j < (len / 2); j++)
>> +                                             sum += s[j];
>> +                                     if (sum == 4)
>> +                                             break;
>> +                             }
>> +                     } while (sum != 4);
>> +
>> +                     for (i = (len / 2); i < len; i++)
>> +                             s[i] = s[len - i - 1];
>> +
>> +                     s[len - 1] |= SZ_COEF;
>> +             } else {
>> +                     /* bilinear scaling for < 2:1 ratios */
>> +                     int v; /* overflow counter */
>> +                     int coeff, nxt; /* table output */
>> +                     int in_pos_inc = 2 * den;
>> +                     int out_pos = num;
>> +                     int out_pos_inc = 2 * num;
>> +                     int init_carry = num - den;
>> +                     int carry = init_carry;
>> +
>> +                     pcdev->resizing[dir].algo = RESIZE_ALGO_BILINEAR;
>> +                     v = den + in_pos_inc;
>> +                     do {
>> +                             coeff = v - out_pos;
>> +                             out_pos += out_pos_inc;
>> +                             carry += out_pos_inc;
>> +                             for (nxt = 0; v < out_pos; nxt++) {
>> +                                     v += in_pos_inc;
>> +                                     carry -= in_pos_inc;
>> +                             }
>> +
>> +                             if (len > RESIZE_NUM_MAX)
>> +                                     return -EINVAL;
>> +
>> +                             coeff = ((coeff << BC_COEF) +
>> +                                     (in_pos_inc >> 1)) / in_pos_inc;
>> +
>> +                             if (coeff >= (SZ_COEF - 1))
>> +                                     coeff--;
>> +
>> +                             coeff |= SZ_COEF;
>> +                             s[len] = (unsigned char)coeff;
>> +                             len++;
>> +
>> +                             for (i = 1; i < nxt; i++) {
>> +                                     if (len >= RESIZE_NUM_MAX)
>> +                                             return -EINVAL;
>> +                                     s[len] = 0;
>> +                                     len++;
>> +                             }
>> +                     } while (carry != init_carry);
>> +             }
>> +             pcdev->resizing[dir].len = len;
>> +             if (dir == RESIZE_DIR_H)
>> +                     mf_in->width = mf_in->width * den / num;
>> +             else
>> +                     mf_in->height = mf_in->height * den / num;
>
> Aren't your calculations exact, so that here you can just use
> pix_out->width and pix_out->height?

Yes, they are. I can save these operations. Thank you.

>> +     }
>> +     return 0;
>> +}
>> +
>>  static int mx2_camera_set_fmt(struct soc_camera_device *icd,
>>                              struct v4l2_format *f)
>>  {
>> @@ -1070,6 +1278,9 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
>>       struct v4l2_mbus_framefmt mf;
>>       int ret;
>>
>> +     dev_dbg(icd->parent, "%s: requested params: width = %d, height = %d\n",
>> +             __func__, pix->width, pix->height);
>> +
>>       xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
>>       if (!xlate) {
>>               dev_warn(icd->parent, "Format %x not found\n",
>> @@ -1087,6 +1298,18 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
>>       if (ret < 0 && ret != -ENOIOCTLCMD)
>>               return ret;
>>
>> +     /* Store width and height returned by the sensor for resizing */
>> +     pcdev->s_width = mf.width;
>> +     pcdev->s_height = mf.height;
>> +     dev_dbg(icd->parent, "%s: sensor params: width = %d, height = %d\n",
>> +             __func__, pcdev->s_width, pcdev->s_height);
>> +
>> +     memset(pcdev->resizing, 0, sizeof(struct emma_prp_resize) << 1);
>
> I think, just sizeof(pcdev->resizing) will do the trick.

No problem.

>> +     if (mf.width != pix->width || mf.height != pix->height) {
>> +             if (mx2_emmaprp_resize(pcdev, &mf, pix) < 0)
>> +                     return -EINVAL;
>
> Hmmm... This looks like a regression, not an improvement - you return more
> errors now, than without this resizing support. Wouldn't it be possible to
> fall back to the current behaviour if prp resizing is impossible?

You are right. I did this for testing purposes and forgot to update it.

>> +     }
>> +
>>       if (mf.code != xlate->code)
>>               return -EINVAL;
>>
>> @@ -1100,6 +1323,9 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
>>               pcdev->emma_prp = mx27_emma_prp_get_format(xlate->code,
>>                                               xlate->host_fmt->fourcc);
>>
>> +     dev_dbg(icd->parent, "%s: returned params: width = %d, height = %d\n",
>> +             __func__, pix->width, pix->height);
>> +
>>       return 0;
>>  }
>>
>> @@ -1109,11 +1335,16 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,
>>       struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>>       const struct soc_camera_format_xlate *xlate;
>>       struct v4l2_pix_format *pix = &f->fmt.pix;
>> -     struct v4l2_mbus_framefmt mf;
>>       __u32 pixfmt = pix->pixelformat;
>> +     struct v4l2_mbus_framefmt mf;
>
> This swap doesn't seem to be needed.

Fine, I'll fix it.

>> +     struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> +     struct mx2_camera_dev *pcdev = ici->priv;
>>       unsigned int width_limit;
>>       int ret;
>>
>> +     dev_dbg(icd->parent, "%s: requested params: width = %d, height = %d\n",
>> +             __func__, pix->width, pix->height);
>> +
>>       xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
>>       if (pixfmt && !xlate) {
>>               dev_warn(icd->parent, "Format %x not found\n", pixfmt);
>> @@ -1163,6 +1394,19 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,
>>       if (ret < 0)
>>               return ret;
>>
>> +     /* Store width and height returned by the sensor for resizing */
>> +     pcdev->s_width = mf.width;
>> +     pcdev->s_height = mf.height;
>
> You don't need these in .try_fmt().

Right.

>> +     dev_dbg(icd->parent, "%s: sensor params: width = %d, height = %d\n",
>> +             __func__, pcdev->s_width, pcdev->s_height);
>> +
>> +     /* If the sensor does not support image size try PrP resizing */
>> +     memset(pcdev->resizing, 0, sizeof(struct emma_prp_resize) << 1);
>
> Same for sizeof()
>
>> +     if (mf.width != pix->width || mf.height != pix->height) {
>> +             if (mx2_emmaprp_resize(pcdev, &mf, pix) < 0)
>> +                     return -EINVAL;
>
> .try_fmt() really shouldn't fail.

It's the same issue as with s_fmt, I will fix it.

I'll try to send a v2 tomorrow.

Regards.
  
Guennadi Liakhovetski Feb. 21, 2012, 8:39 a.m. UTC | #3
Hi Javier

One more thing occurred to me: I don't see anywhere in your patch checking 
for supported pixel (fourcc) formats. I don't think the PRP can resize 
arbitrary formats? Most likely these would be limited to some YUV, and, 
possibly, some RGB formats?

Thanks
Guennadi

On Tue, 21 Feb 2012, javier Martin wrote:

> Hi Guennadi,
> thank you for your review.
> 
> On 20 February 2012 15:13, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> >> @@ -707,6 +732,74 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
> >>       writel(prp->cfg.irq_flags, pcdev->base_emma + PRP_INTR_CNTL);
> >>  }
> >>
> >> +static void mx2_prp_resize_commit(struct mx2_camera_dev *pcdev)
> >> +{
> >> +     int dir;
> >> +
> >> +     for (dir = RESIZE_DIR_H; dir <= RESIZE_DIR_V; dir++) {
> >> +             unsigned char *s = pcdev->resizing[dir].s;
> >> +             int len = pcdev->resizing[dir].len;
> >> +             unsigned int coeff[2] = {0, 0};
> >> +             unsigned int valid  = 0;
> >> +             int i;
> >> +
> >> +             if (len == 0)
> >> +                     continue;
> >> +
> >> +             for (i = RESIZE_NUM_MAX - 1; i >= 0; i--) {
> >> +                     int j;
> >> +
> >> +                     j = i > 9 ? 1 : 0;
> >> +                     coeff[j] = (coeff[j] << BC_COEF) |
> >> +                     (s[i] & (SZ_COEF - 1));
> >
> > Please, remember to indent line continuations.
> 
> Yes, sorry.
> 
> >
> >> +
> >> +                     if (i == 5 || i == 15)
> >> +                             coeff[j] <<= 1;
> >> +
> >> +                     valid = (valid << 1) | (s[i] >> BC_COEF);
> >> +             }
> >> +
> >> +             valid |= PRP_RZ_VALID_TBL_LEN(len);
> >> +
> >> +             if (pcdev->resizing[dir].algo == RESIZE_ALGO_BILINEAR)
> >> +                     valid |= PRP_RZ_VALID_BILINEAR;
> >> +
> >> +             if (pcdev->emma_prp->cfg.channel == 1) {
> >
> > Maybe put horizontal and vertical register addresses in an array too to
> > avoid this "if?" Just an idea - if you don't think, the code would look
> > nicer, just leave as is.
> 
> I don't know about that, we can't avoid the fact that the code would
> look more compact but I think it would be less clear.
> 
> >> +                     if (dir == RESIZE_DIR_H) {
> >> +                             writel(coeff[0], pcdev->base_emma +
> >> +                                                     PRP_CH1_RZ_HORI_COEF1);
> >> +                             writel(coeff[1], pcdev->base_emma +
> >> +                                                     PRP_CH1_RZ_HORI_COEF2);
> >> +                             writel(valid, pcdev->base_emma +
> >> +                                                     PRP_CH1_RZ_HORI_VALID);
> >> +                     } else {
> >> +                             writel(coeff[0], pcdev->base_emma +
> >> +                                                     PRP_CH1_RZ_VERT_COEF1);
> >> +                             writel(coeff[1], pcdev->base_emma +
> >> +                                                     PRP_CH1_RZ_VERT_COEF2);
> >> +                             writel(valid, pcdev->base_emma +
> >> +                                                     PRP_CH1_RZ_VERT_VALID);
> >> +                     }
> >> +             } else {
> >> +                     if (dir == RESIZE_DIR_H) {
> >> +                             writel(coeff[0], pcdev->base_emma +
> >> +                                                     PRP_CH2_RZ_HORI_COEF1);
> >> +                             writel(coeff[1], pcdev->base_emma +
> >> +                                                     PRP_CH2_RZ_HORI_COEF2);
> >> +                             writel(valid, pcdev->base_emma +
> >> +                                                     PRP_CH2_RZ_HORI_VALID);
> >> +                     } else {
> >> +                             writel(coeff[0], pcdev->base_emma +
> >> +                                                     PRP_CH2_RZ_VERT_COEF1);
> >> +                             writel(coeff[1], pcdev->base_emma +
> >> +                                                     PRP_CH2_RZ_VERT_COEF2);
> >> +                             writel(valid, pcdev->base_emma +
> >> +                                                     PRP_CH2_RZ_VERT_VALID);
> >> +                     }
> >> +             }
> >> +     }
> >> +}
> >> +
> >>  static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
> >>  {
> >>       struct soc_camera_device *icd = soc_camera_from_vb2q(q);
> >> @@ -773,6 +866,8 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
> >>               list_add_tail(&pcdev->buf_discard[1].queue,
> >>                                     &pcdev->discard);
> >>
> >> +             mx2_prp_resize_commit(pcdev);
> >> +
> >>               mx27_camera_emma_buf_init(icd, bytesperline);
> >>
> >>               if (prp->cfg.channel == 1) {
> >> @@ -1059,6 +1154,119 @@ static int mx2_camera_get_formats(struct soc_camera_device *icd,
> >>       return formats;
> >>  }
> >>
> >> +static int mx2_emmaprp_resize(struct mx2_camera_dev *pcdev,
> >> +                           struct v4l2_mbus_framefmt *mf_in,
> >> +                           struct v4l2_pix_format *pix_out)
> >> +{
> >> +     int num, den;
> >> +     unsigned long m;
> >> +     int i, dir;
> >> +
> >> +     for (dir = RESIZE_DIR_H; dir <= RESIZE_DIR_V; dir++) {
> >> +             unsigned char *s = pcdev->resizing[dir].s;
> >> +             int len = 0;
> >> +             int in, out;
> >> +
> >> +             if (dir == RESIZE_DIR_H) {
> >> +                     in = mf_in->width;
> >> +                     out = pix_out->width;
> >> +             } else {
> >> +                     in = mf_in->height;
> >> +                     out = pix_out->height;
> >> +             }
> >> +
> >> +             if (in < out)
> >> +                     return -EINVAL;
> >> +             else if (in == out)
> >> +                     continue;
> >> +
> >> +             /* Calculate ratio */
> >> +             m = gcd(in, out);
> >> +             num = in / m;
> >> +             den = out / m;
> >> +             if (num > RESIZE_NUM_MAX)
> >> +                     return -EINVAL;
> >> +
> >> +             if ((num >= 2 * den) && (den == 1) &&
> >> +                 (num < 9) && (!(num & 0x01))) {
> >> +                     int sum = 0;
> >> +                     int j;
> >> +
> >> +                     /* Average scaling for > 2:1 ratios */
> >
> > ">=" rather than ">"
> 
> You are right.
> 
> >> +                     /* Support can be added for num >=9 and odd values */
> >
> > So, this is only used for downscaling by 1/2, 1/4, 1/6, and 1/8?
> 
> Yes.
> 
> >> +
> >> +                     pcdev->resizing[dir].algo = RESIZE_ALGO_AVERAGING;
> >> +                     len = num;
> >> +
> >> +                     for (i = 0; i < (len / 2); i++)
> >> +                             s[i] = 8;
> >> +
> >> +                     do {
> >> +                             for (i = 0; i < (len / 2); i++) {
> >> +                                     s[i] = s[i] >> 1;
> >> +                                     sum = 0;
> >> +                                     for (j = 0; j < (len / 2); j++)
> >> +                                             sum += s[j];
> >> +                                     if (sum == 4)
> >> +                                             break;
> >> +                             }
> >> +                     } while (sum != 4);
> >> +
> >> +                     for (i = (len / 2); i < len; i++)
> >> +                             s[i] = s[len - i - 1];
> >> +
> >> +                     s[len - 1] |= SZ_COEF;
> >> +             } else {
> >> +                     /* bilinear scaling for < 2:1 ratios */
> >> +                     int v; /* overflow counter */
> >> +                     int coeff, nxt; /* table output */
> >> +                     int in_pos_inc = 2 * den;
> >> +                     int out_pos = num;
> >> +                     int out_pos_inc = 2 * num;
> >> +                     int init_carry = num - den;
> >> +                     int carry = init_carry;
> >> +
> >> +                     pcdev->resizing[dir].algo = RESIZE_ALGO_BILINEAR;
> >> +                     v = den + in_pos_inc;
> >> +                     do {
> >> +                             coeff = v - out_pos;
> >> +                             out_pos += out_pos_inc;
> >> +                             carry += out_pos_inc;
> >> +                             for (nxt = 0; v < out_pos; nxt++) {
> >> +                                     v += in_pos_inc;
> >> +                                     carry -= in_pos_inc;
> >> +                             }
> >> +
> >> +                             if (len > RESIZE_NUM_MAX)
> >> +                                     return -EINVAL;
> >> +
> >> +                             coeff = ((coeff << BC_COEF) +
> >> +                                     (in_pos_inc >> 1)) / in_pos_inc;
> >> +
> >> +                             if (coeff >= (SZ_COEF - 1))
> >> +                                     coeff--;
> >> +
> >> +                             coeff |= SZ_COEF;
> >> +                             s[len] = (unsigned char)coeff;
> >> +                             len++;
> >> +
> >> +                             for (i = 1; i < nxt; i++) {
> >> +                                     if (len >= RESIZE_NUM_MAX)
> >> +                                             return -EINVAL;
> >> +                                     s[len] = 0;
> >> +                                     len++;
> >> +                             }
> >> +                     } while (carry != init_carry);
> >> +             }
> >> +             pcdev->resizing[dir].len = len;
> >> +             if (dir == RESIZE_DIR_H)
> >> +                     mf_in->width = mf_in->width * den / num;
> >> +             else
> >> +                     mf_in->height = mf_in->height * den / num;
> >
> > Aren't your calculations exact, so that here you can just use
> > pix_out->width and pix_out->height?
> 
> Yes, they are. I can save these operations. Thank you.
> 
> >> +     }
> >> +     return 0;
> >> +}
> >> +
> >>  static int mx2_camera_set_fmt(struct soc_camera_device *icd,
> >>                              struct v4l2_format *f)
> >>  {
> >> @@ -1070,6 +1278,9 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
> >>       struct v4l2_mbus_framefmt mf;
> >>       int ret;
> >>
> >> +     dev_dbg(icd->parent, "%s: requested params: width = %d, height = %d\n",
> >> +             __func__, pix->width, pix->height);
> >> +
> >>       xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
> >>       if (!xlate) {
> >>               dev_warn(icd->parent, "Format %x not found\n",
> >> @@ -1087,6 +1298,18 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
> >>       if (ret < 0 && ret != -ENOIOCTLCMD)
> >>               return ret;
> >>
> >> +     /* Store width and height returned by the sensor for resizing */
> >> +     pcdev->s_width = mf.width;
> >> +     pcdev->s_height = mf.height;
> >> +     dev_dbg(icd->parent, "%s: sensor params: width = %d, height = %d\n",
> >> +             __func__, pcdev->s_width, pcdev->s_height);
> >> +
> >> +     memset(pcdev->resizing, 0, sizeof(struct emma_prp_resize) << 1);
> >
> > I think, just sizeof(pcdev->resizing) will do the trick.
> 
> No problem.
> 
> >> +     if (mf.width != pix->width || mf.height != pix->height) {
> >> +             if (mx2_emmaprp_resize(pcdev, &mf, pix) < 0)
> >> +                     return -EINVAL;
> >
> > Hmmm... This looks like a regression, not an improvement - you return more
> > errors now, than without this resizing support. Wouldn't it be possible to
> > fall back to the current behaviour if prp resizing is impossible?
> 
> You are right. I did this for testing purposes and forgot to update it.
> 
> >> +     }
> >> +
> >>       if (mf.code != xlate->code)
> >>               return -EINVAL;
> >>
> >> @@ -1100,6 +1323,9 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
> >>               pcdev->emma_prp = mx27_emma_prp_get_format(xlate->code,
> >>                                               xlate->host_fmt->fourcc);
> >>
> >> +     dev_dbg(icd->parent, "%s: returned params: width = %d, height = %d\n",
> >> +             __func__, pix->width, pix->height);
> >> +
> >>       return 0;
> >>  }
> >>
> >> @@ -1109,11 +1335,16 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,
> >>       struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> >>       const struct soc_camera_format_xlate *xlate;
> >>       struct v4l2_pix_format *pix = &f->fmt.pix;
> >> -     struct v4l2_mbus_framefmt mf;
> >>       __u32 pixfmt = pix->pixelformat;
> >> +     struct v4l2_mbus_framefmt mf;
> >
> > This swap doesn't seem to be needed.
> 
> Fine, I'll fix it.
> 
> >> +     struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> >> +     struct mx2_camera_dev *pcdev = ici->priv;
> >>       unsigned int width_limit;
> >>       int ret;
> >>
> >> +     dev_dbg(icd->parent, "%s: requested params: width = %d, height = %d\n",
> >> +             __func__, pix->width, pix->height);
> >> +
> >>       xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
> >>       if (pixfmt && !xlate) {
> >>               dev_warn(icd->parent, "Format %x not found\n", pixfmt);
> >> @@ -1163,6 +1394,19 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,
> >>       if (ret < 0)
> >>               return ret;
> >>
> >> +     /* Store width and height returned by the sensor for resizing */
> >> +     pcdev->s_width = mf.width;
> >> +     pcdev->s_height = mf.height;
> >
> > You don't need these in .try_fmt().
> 
> Right.
> 
> >> +     dev_dbg(icd->parent, "%s: sensor params: width = %d, height = %d\n",
> >> +             __func__, pcdev->s_width, pcdev->s_height);
> >> +
> >> +     /* If the sensor does not support image size try PrP resizing */
> >> +     memset(pcdev->resizing, 0, sizeof(struct emma_prp_resize) << 1);
> >
> > Same for sizeof()
> >
> >> +     if (mf.width != pix->width || mf.height != pix->height) {
> >> +             if (mx2_emmaprp_resize(pcdev, &mf, pix) < 0)
> >> +                     return -EINVAL;
> >
> > .try_fmt() really shouldn't fail.
> 
> It's the same issue as with s_fmt, I will fix it.
> 
> I'll try to send a v2 tomorrow.
> 
> Regards.
> -- 
> Javier Martin
> Vista Silicon S.L.
> CDTUC - FASE C - Oficina S-345
> Avda de los Castros s/n
> 39005- Santander. Cantabria. Spain
> +34 942 25 32 60
> www.vista-silicon.com
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Javier Martin Feb. 21, 2012, 9:14 a.m. UTC | #4
On 21 February 2012 09:39, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Hi Javier
>
> One more thing occurred to me: I don't see anywhere in your patch checking
> for supported pixel (fourcc) formats. I don't think the PRP can resize
> arbitrary formats? Most likely these would be limited to some YUV, and,
> possibly, some RGB formats?

The PrP can resize every format which is supported as input by the eMMa.

Currently, the driver supports 2 input formats: RGB565 and YUV422
(YUYV)  (see mx27_emma_prp_table[]).

Since the commit of resizing registers is done in the stream_start
callback this makes sure that resizing won't be applied to unknown
formats.

Regards.
  
Guennadi Liakhovetski Feb. 21, 2012, 9:24 a.m. UTC | #5
On Tue, 21 Feb 2012, javier Martin wrote:

> On 21 February 2012 09:39, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Hi Javier
> >
> > One more thing occurred to me: I don't see anywhere in your patch checking
> > for supported pixel (fourcc) formats. I don't think the PRP can resize
> > arbitrary formats? Most likely these would be limited to some YUV, and,
> > possibly, some RGB formats?
> 
> The PrP can resize every format which is supported as input by the eMMa.
> 
> Currently, the driver supports 2 input formats: RGB565 and YUV422
> (YUYV)  (see mx27_emma_prp_table[]).

That's not how I understand it. The mx27_emma_prp_table[] array has 2 
entries: the first one is indeed configured for RGB565, and the second one 
is converting input YUV422 to output YUV420. But the former one is not 
really that specific format, rather it is a generic configuration used as 
a pass-through mode for generic 16-bit formats.

BTW, does that mean, that on i.MX27 the driver currently doesn't support 
8-bit formats like Bayer?

Thanks
Guennadi

> Since the commit of resizing registers is done in the stream_start
> callback this makes sure that resizing won't be applied to unknown
> formats.
> 
> Regards.
> -- 
> Javier Martin
> Vista Silicon S.L.
> CDTUC - FASE C - Oficina S-345
> Avda de los Castros s/n
> 39005- Santander. Cantabria. Spain
> +34 942 25 32 60
> www.vista-silicon.com

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Javier Martin Feb. 21, 2012, 9:41 a.m. UTC | #6
On 21 February 2012 10:24, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> On Tue, 21 Feb 2012, javier Martin wrote:
>
>> On 21 February 2012 09:39, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>> > Hi Javier
>> >
>> > One more thing occurred to me: I don't see anywhere in your patch checking
>> > for supported pixel (fourcc) formats. I don't think the PRP can resize
>> > arbitrary formats? Most likely these would be limited to some YUV, and,
>> > possibly, some RGB formats?
>>
>> The PrP can resize every format which is supported as input by the eMMa.
>>
>> Currently, the driver supports 2 input formats: RGB565 and YUV422
>> (YUYV)  (see mx27_emma_prp_table[]).
>
> That's not how I understand it. The mx27_emma_prp_table[] array has 2
> entries: the first one is indeed configured for RGB565, and the second one
> is converting input YUV422 to output YUV420. But the former one is not
> really that specific format, rather it is a generic configuration used as
> a pass-through mode for generic 16-bit formats.
>
> BTW, does that mean, that on i.MX27 the driver currently doesn't support
> 8-bit formats like Bayer?

According to the datasheet, the eMMa-PrP only accepts the following
input formats when capturing data form the CSI:

RGB 16 bpp
RGB 32 bpp (unpacked RGB888)
YUV 4:2:2 Pixel interleaved
YUV 4:4:4

But the driver only supports:
- RGB 16bpp which, as you say is used as pass-through mode for generic
16-bit formats.
- YUV 422 which is converted to YUV420.

I'm sorry, you are right. Since I only use the latter, I hadn't
noticed that the resizing engine could in fact have problems with
16bit pass-through mode depending on what 16bit format is really being
transfered.

What I can do is restricting the use of resizing only to the YUV422
case so that someone who is using the old pass-through mode can add
support for resizing later for this format.
  
Javier Martin Feb. 22, 2012, 12:14 p.m. UTC | #7
>>> @@ -1087,6 +1298,18 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
>>>       if (ret < 0 && ret != -ENOIOCTLCMD)
>>>               return ret;
>>>
>>> +     /* Store width and height returned by the sensor for resizing */
>>> +     pcdev->s_width = mf.width;
>>> +     pcdev->s_height = mf.height;
>>> +     dev_dbg(icd->parent, "%s: sensor params: width = %d, height = %d\n",
>>> +             __func__, pcdev->s_width, pcdev->s_height);
>>> +
>>> +     memset(pcdev->resizing, 0, sizeof(struct emma_prp_resize) << 1);
>>
>> I think, just sizeof(pcdev->resizing) will do the trick.

No, it doesn't work because pcdev->resizing is a pointer whose size is 4bytes.

I will just left this unchanged with your permission.

Regards.
  
Guennadi Liakhovetski Feb. 22, 2012, 12:26 p.m. UTC | #8
On Wed, 22 Feb 2012, javier Martin wrote:

> >>> @@ -1087,6 +1298,18 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
> >>>       if (ret < 0 && ret != -ENOIOCTLCMD)
> >>>               return ret;
> >>>
> >>> +     /* Store width and height returned by the sensor for resizing */
> >>> +     pcdev->s_width = mf.width;
> >>> +     pcdev->s_height = mf.height;
> >>> +     dev_dbg(icd->parent, "%s: sensor params: width = %d, height = %d\n",
> >>> +             __func__, pcdev->s_width, pcdev->s_height);
> >>> +
> >>> +     memset(pcdev->resizing, 0, sizeof(struct emma_prp_resize) << 1);
> >>
> >> I think, just sizeof(pcdev->resizing) will do the trick.
> 
> No, it doesn't work because pcdev->resizing is a pointer whose size is 
> 4bytes.

Sorry?

+	struct emma_prp_resize  resizing[2];

.resizing is an array of 2 "struct emma_prp_resize" objects.

Thanks
Guennadi

> I will just left this unchanged with your permission.
> 
> Regards.
> -- 
> Javier Martin
> Vista Silicon S.L.
> CDTUC - FASE C - Oficina S-345
> Avda de los Castros s/n
> 39005- Santander. Cantabria. Spain
> +34 942 25 32 60
> www.vista-silicon.com
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Patch

diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index 9e84368..6516ee4 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -19,6 +19,7 @@ 
 #include <linux/dma-mapping.h>
 #include <linux/errno.h>
 #include <linux/fs.h>
+#include <linux/gcd.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
@@ -204,10 +205,25 @@ 
 #define PRP_INTR_LBOVF		(1 << 7)
 #define PRP_INTR_CH2OVF		(1 << 8)
 
+/* Resizing registers */
+#define PRP_RZ_VALID_TBL_LEN(x)	((x) << 24)
+#define PRP_RZ_VALID_BILINEAR	(1 << 31)
+
 #define mx27_camera_emma(pcdev)	(cpu_is_mx27() && pcdev->use_emma)
 
 #define MAX_VIDEO_MEM	16
 
+#define RESIZE_NUM_MIN	1
+#define RESIZE_NUM_MAX	20
+#define BC_COEF		3
+#define SZ_COEF		(1 << BC_COEF)
+
+#define RESIZE_DIR_H	0
+#define RESIZE_DIR_V	1
+
+#define RESIZE_ALGO_BILINEAR 0
+#define RESIZE_ALGO_AVERAGING 1
+
 struct mx2_prp_cfg {
 	int channel;
 	u32 in_fmt;
@@ -217,6 +233,13 @@  struct mx2_prp_cfg {
 	u32 irq_flags;
 };
 
+/* prp resizing parameters */
+struct emma_prp_resize {
+	int		algo; /* type of algorithm used */
+	int		len; /* number of coefficients */
+	unsigned char	s[RESIZE_NUM_MAX]; /* table of coefficients */
+};
+
 /* prp configuration for a client-host fmt pair */
 struct mx2_fmt_cfg {
 	enum v4l2_mbus_pixelcode	in_fmt;
@@ -278,6 +301,8 @@  struct mx2_camera_dev {
 	dma_addr_t		discard_buffer_dma;
 	size_t			discard_size;
 	struct mx2_fmt_cfg	*emma_prp;
+	struct emma_prp_resize	resizing[2];
+	unsigned int		s_width, s_height;
 	u32			frame_count;
 	struct vb2_alloc_ctx	*alloc_ctx;
 };
@@ -687,7 +712,7 @@  static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
 	struct mx2_camera_dev *pcdev = ici->priv;
 	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
 
-	writel((icd->user_width << 16) | icd->user_height,
+	writel((pcdev->s_width << 16) | pcdev->s_height,
 	       pcdev->base_emma + PRP_SRC_FRAME_SIZE);
 	writel(prp->cfg.src_pixel,
 	       pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
@@ -707,6 +732,74 @@  static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
 	writel(prp->cfg.irq_flags, pcdev->base_emma + PRP_INTR_CNTL);
 }
 
+static void mx2_prp_resize_commit(struct mx2_camera_dev *pcdev)
+{
+	int dir;
+
+	for (dir = RESIZE_DIR_H; dir <= RESIZE_DIR_V; dir++) {
+		unsigned char *s = pcdev->resizing[dir].s;
+		int len = pcdev->resizing[dir].len;
+		unsigned int coeff[2] = {0, 0};
+		unsigned int valid  = 0;
+		int i;
+
+		if (len == 0)
+			continue;
+
+		for (i = RESIZE_NUM_MAX - 1; i >= 0; i--) {
+			int j;
+
+			j = i > 9 ? 1 : 0;
+			coeff[j] = (coeff[j] << BC_COEF) |
+			(s[i] & (SZ_COEF - 1));
+
+			if (i == 5 || i == 15)
+				coeff[j] <<= 1;
+
+			valid = (valid << 1) | (s[i] >> BC_COEF);
+		}
+
+		valid |= PRP_RZ_VALID_TBL_LEN(len);
+
+		if (pcdev->resizing[dir].algo == RESIZE_ALGO_BILINEAR)
+			valid |= PRP_RZ_VALID_BILINEAR;
+
+		if (pcdev->emma_prp->cfg.channel == 1) {
+			if (dir == RESIZE_DIR_H) {
+				writel(coeff[0], pcdev->base_emma +
+							PRP_CH1_RZ_HORI_COEF1);
+				writel(coeff[1], pcdev->base_emma +
+							PRP_CH1_RZ_HORI_COEF2);
+				writel(valid, pcdev->base_emma +
+							PRP_CH1_RZ_HORI_VALID);
+			} else {
+				writel(coeff[0], pcdev->base_emma +
+							PRP_CH1_RZ_VERT_COEF1);
+				writel(coeff[1], pcdev->base_emma +
+							PRP_CH1_RZ_VERT_COEF2);
+				writel(valid, pcdev->base_emma +
+							PRP_CH1_RZ_VERT_VALID);
+			}
+		} else {
+			if (dir == RESIZE_DIR_H) {
+				writel(coeff[0], pcdev->base_emma +
+							PRP_CH2_RZ_HORI_COEF1);
+				writel(coeff[1], pcdev->base_emma +
+							PRP_CH2_RZ_HORI_COEF2);
+				writel(valid, pcdev->base_emma +
+							PRP_CH2_RZ_HORI_VALID);
+			} else {
+				writel(coeff[0], pcdev->base_emma +
+							PRP_CH2_RZ_VERT_COEF1);
+				writel(coeff[1], pcdev->base_emma +
+							PRP_CH2_RZ_VERT_COEF2);
+				writel(valid, pcdev->base_emma +
+							PRP_CH2_RZ_VERT_VALID);
+			}
+		}
+	}
+}
+
 static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
 {
 	struct soc_camera_device *icd = soc_camera_from_vb2q(q);
@@ -773,6 +866,8 @@  static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
 		list_add_tail(&pcdev->buf_discard[1].queue,
 				      &pcdev->discard);
 
+		mx2_prp_resize_commit(pcdev);
+
 		mx27_camera_emma_buf_init(icd, bytesperline);
 
 		if (prp->cfg.channel == 1) {
@@ -1059,6 +1154,119 @@  static int mx2_camera_get_formats(struct soc_camera_device *icd,
 	return formats;
 }
 
+static int mx2_emmaprp_resize(struct mx2_camera_dev *pcdev,
+			      struct v4l2_mbus_framefmt *mf_in,
+			      struct v4l2_pix_format *pix_out)
+{
+	int num, den;
+	unsigned long m;
+	int i, dir;
+
+	for (dir = RESIZE_DIR_H; dir <= RESIZE_DIR_V; dir++) {
+		unsigned char *s = pcdev->resizing[dir].s;
+		int len = 0;
+		int in, out;
+
+		if (dir == RESIZE_DIR_H) {
+			in = mf_in->width;
+			out = pix_out->width;
+		} else {
+			in = mf_in->height;
+			out = pix_out->height;
+		}
+
+		if (in < out)
+			return -EINVAL;
+		else if (in == out)
+			continue;
+
+		/* Calculate ratio */
+		m = gcd(in, out);
+		num = in / m;
+		den = out / m;
+		if (num > RESIZE_NUM_MAX)
+			return -EINVAL;
+
+		if ((num >= 2 * den) && (den == 1) &&
+		    (num < 9) && (!(num & 0x01))) {
+			int sum = 0;
+			int j;
+
+			/* Average scaling for > 2:1 ratios */
+			/* Support can be added for num >=9 and odd values */
+
+			pcdev->resizing[dir].algo = RESIZE_ALGO_AVERAGING;
+			len = num;
+
+			for (i = 0; i < (len / 2); i++)
+				s[i] = 8;
+
+			do {
+				for (i = 0; i < (len / 2); i++) {
+					s[i] = s[i] >> 1;
+					sum = 0;
+					for (j = 0; j < (len / 2); j++)
+						sum += s[j];
+					if (sum == 4)
+						break;
+				}
+			} while (sum != 4);
+
+			for (i = (len / 2); i < len; i++)
+				s[i] = s[len - i - 1];
+
+			s[len - 1] |= SZ_COEF;
+		} else {
+			/* bilinear scaling for < 2:1 ratios */
+			int v; /* overflow counter */
+			int coeff, nxt; /* table output */
+			int in_pos_inc = 2 * den;
+			int out_pos = num;
+			int out_pos_inc = 2 * num;
+			int init_carry = num - den;
+			int carry = init_carry;
+
+			pcdev->resizing[dir].algo = RESIZE_ALGO_BILINEAR;
+			v = den + in_pos_inc;
+			do {
+				coeff = v - out_pos;
+				out_pos += out_pos_inc;
+				carry += out_pos_inc;
+				for (nxt = 0; v < out_pos; nxt++) {
+					v += in_pos_inc;
+					carry -= in_pos_inc;
+				}
+
+				if (len > RESIZE_NUM_MAX)
+					return -EINVAL;
+
+				coeff = ((coeff << BC_COEF) +
+					(in_pos_inc >> 1)) / in_pos_inc;
+
+				if (coeff >= (SZ_COEF - 1))
+					coeff--;
+
+				coeff |= SZ_COEF;
+				s[len] = (unsigned char)coeff;
+				len++;
+
+				for (i = 1; i < nxt; i++) {
+					if (len >= RESIZE_NUM_MAX)
+						return -EINVAL;
+					s[len] = 0;
+					len++;
+				}
+			} while (carry != init_carry);
+		}
+		pcdev->resizing[dir].len = len;
+		if (dir == RESIZE_DIR_H)
+			mf_in->width = mf_in->width * den / num;
+		else
+			mf_in->height = mf_in->height * den / num;
+	}
+	return 0;
+}
+
 static int mx2_camera_set_fmt(struct soc_camera_device *icd,
 			       struct v4l2_format *f)
 {
@@ -1070,6 +1278,9 @@  static int mx2_camera_set_fmt(struct soc_camera_device *icd,
 	struct v4l2_mbus_framefmt mf;
 	int ret;
 
+	dev_dbg(icd->parent, "%s: requested params: width = %d, height = %d\n",
+		__func__, pix->width, pix->height);
+
 	xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
 	if (!xlate) {
 		dev_warn(icd->parent, "Format %x not found\n",
@@ -1087,6 +1298,18 @@  static int mx2_camera_set_fmt(struct soc_camera_device *icd,
 	if (ret < 0 && ret != -ENOIOCTLCMD)
 		return ret;
 
+	/* Store width and height returned by the sensor for resizing */
+	pcdev->s_width = mf.width;
+	pcdev->s_height = mf.height;
+	dev_dbg(icd->parent, "%s: sensor params: width = %d, height = %d\n",
+		__func__, pcdev->s_width, pcdev->s_height);
+
+	memset(pcdev->resizing, 0, sizeof(struct emma_prp_resize) << 1);
+	if (mf.width != pix->width || mf.height != pix->height) {
+		if (mx2_emmaprp_resize(pcdev, &mf, pix) < 0)
+			return -EINVAL;
+	}
+
 	if (mf.code != xlate->code)
 		return -EINVAL;
 
@@ -1100,6 +1323,9 @@  static int mx2_camera_set_fmt(struct soc_camera_device *icd,
 		pcdev->emma_prp = mx27_emma_prp_get_format(xlate->code,
 						xlate->host_fmt->fourcc);
 
+	dev_dbg(icd->parent, "%s: returned params: width = %d, height = %d\n",
+		__func__, pix->width, pix->height);
+
 	return 0;
 }
 
@@ -1109,11 +1335,16 @@  static int mx2_camera_try_fmt(struct soc_camera_device *icd,
 	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
 	const struct soc_camera_format_xlate *xlate;
 	struct v4l2_pix_format *pix = &f->fmt.pix;
-	struct v4l2_mbus_framefmt mf;
 	__u32 pixfmt = pix->pixelformat;
+	struct v4l2_mbus_framefmt mf;
+	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
+	struct mx2_camera_dev *pcdev = ici->priv;
 	unsigned int width_limit;
 	int ret;
 
+	dev_dbg(icd->parent, "%s: requested params: width = %d, height = %d\n",
+		__func__, pix->width, pix->height);
+
 	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
 	if (pixfmt && !xlate) {
 		dev_warn(icd->parent, "Format %x not found\n", pixfmt);
@@ -1163,6 +1394,19 @@  static int mx2_camera_try_fmt(struct soc_camera_device *icd,
 	if (ret < 0)
 		return ret;
 
+	/* Store width and height returned by the sensor for resizing */
+	pcdev->s_width = mf.width;
+	pcdev->s_height = mf.height;
+	dev_dbg(icd->parent, "%s: sensor params: width = %d, height = %d\n",
+		__func__, pcdev->s_width, pcdev->s_height);
+
+	/* If the sensor does not support image size try PrP resizing */
+	memset(pcdev->resizing, 0, sizeof(struct emma_prp_resize) << 1);
+	if (mf.width != pix->width || mf.height != pix->height) {
+		if (mx2_emmaprp_resize(pcdev, &mf, pix) < 0)
+			return -EINVAL;
+	}
+
 	if (mf.field == V4L2_FIELD_ANY)
 		mf.field = V4L2_FIELD_NONE;
 	/*
@@ -1181,6 +1425,9 @@  static int mx2_camera_try_fmt(struct soc_camera_device *icd,
 	pix->field	= mf.field;
 	pix->colorspace	= mf.colorspace;
 
+	dev_dbg(icd->parent, "%s: returned params: width = %d, height = %d\n",
+		__func__, pix->width, pix->height);
+
 	return 0;
 }