[PATCH 0/2 v3] soc-camera: pixel format negotiation

[PATCH 0/2 v3] soc-camera: pixel format negotiation

[Guennadi Liakhovetski]

ok, v3 is rather symbolic, counting v1 my original version v2 the version 
from Robert, then this is approximately v3.

These two patches allow host and camera drivers to negoatiate supported 
pixel formats and provide a correct set of formats to the user.

I tried to follow what has been discussed during the previous two version 
rounds. But I had to test it, so, I implemented pxa-support too, sorry, 
Robert. sh_mobile_ceu_camera.c is unmodified so far, the fallback mode 
should provide backwards compatibility. camera drivers do not have to be 
modified either.

The complete stack (except these patches) until now I uploaded at

http://home.arcor.de/g.liakhovetski/v4l/20081118/

based on commit d3ac380b85fc3701c87580ee9ff934c65b8b779f of linux-next. I 
had to revert two ARM patches to get videobuf-dma-sg.c to compile, 
hopefully, it will get fixed some time...

Please, review, comment, test.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH 1/2 v3] soc-camera: pixel format negotiation - core support

[Guennadi Liakhovetski]

Allocate and fill a list of formats, supported by this specific 
camera-host combination. Use it for format enumeration. Take care to stay 
backwards-compatible.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

---

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 9db66a4..f5a1e5a 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -161,6 +161,56 @@ static int soc_camera_dqbuf(struct file *file, void *priv,
 	return videobuf_dqbuf(&icf->vb_vidq, p, file->f_flags & O_NONBLOCK);
 }
 
+static int soc_camera_init_user_formats(struct soc_camera_device *icd)
+{
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	int i, fmts = 0;
+
+	if (!ici->ops->get_formats)
+		/*
+		 * Fallback mode - the host will have to serve all
+		 * sensor-provided formats one-to-one to the user
+		 */
+		fmts = icd->num_formats;
+	else
+		/*
+		 * First pass - only count formats this host-sensor
+		 * configuration can provide
+		 */
+		for (i = 0; i < icd->num_formats; i++)
+			fmts += ici->ops->get_formats(icd, i, NULL);
+
+	if (!fmts)
+		return -ENXIO;
+
+	icd->user_formats = vmalloc(sizeof(struct soc_camera_data_format *) *
+				    fmts);
+	if (!icd->user_formats)
+		return -ENOMEM;
+
+	icd->num_user_formats = fmts;
+	fmts = 0;
+
+	dev_dbg(&icd->dev, "Found %d supported formats.\n", fmts);
+
+	/* Second pass - actually fill data formats */
+	for (i = 0; i < icd->num_formats; i++)
+		if (!ici->ops->get_formats)
+			icd->user_formats[i] = icd->formats + i;
+		else
+			fmts += ici->ops->get_formats(icd, i,
+						      &icd->user_formats[fmts]);
+
+	icd->current_fmt = icd->user_formats[0];
+
+	return 0;
+}
+
+static void soc_camera_free_user_formats(struct soc_camera_device *icd)
+{
+	vfree(icd->user_formats);
+}
+
 static int soc_camera_open(struct inode *inode, struct file *file)
 {
 	struct video_device *vdev;
@@ -197,10 +247,12 @@ static int soc_camera_open(struct inode *inode, struct file *file)
 
 	/* Now we really have to activate the camera */
 	if (icd->use_count == 1) {
+		ret = soc_camera_init_user_formats(icd);
+		if (ret < 0)
+			goto eiufmt;
 		ret = ici->ops->add(icd);
 		if (ret < 0) {
 			dev_err(&icd->dev, "Couldn't activate the camera: %d\n", ret);
-			icd->use_count--;
 			goto eiciadd;
 		}
 	}
@@ -216,6 +268,9 @@ static int soc_camera_open(struct inode *inode, struct file *file)
 
 	/* All errors are entered with the video_lock held */
 eiciadd:
+	soc_camera_free_user_formats(icd);
+eiufmt:
+	icd->use_count--;
 	module_put(ici->ops->owner);
 emgi:
 	module_put(icd->ops->owner);
@@ -234,8 +289,10 @@ static int soc_camera_close(struct inode *inode, struct file *file)
 
 	mutex_lock(&video_lock);
 	icd->use_count--;
-	if (!icd->use_count)
+	if (!icd->use_count) {
 		ici->ops->remove(icd);
+		soc_camera_free_user_formats(icd);
+	}
 	module_put(icd->ops->owner);
 	module_put(ici->ops->owner);
 	mutex_unlock(&video_lock);
@@ -329,13 +386,11 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 		return ret;
 	} else if (!icd->current_fmt ||
 		   icd->current_fmt->fourcc != f->fmt.pix.pixelformat) {
-		dev_err(&ici->dev, "Host driver hasn't set up current "
-			"format correctly!\n");
+		dev_err(&ici->dev,
+			"Host driver hasn't set up current format correctly!\n");
 		return -EINVAL;
 	}
 
-	/* buswidth may be further adjusted by the ici */
-	icd->buswidth		= icd->current_fmt->depth;
 	icd->width		= rect.width;
 	icd->height		= rect.height;
 	icf->vb_vidq.field	= f->fmt.pix.field;
@@ -359,10 +414,10 @@ static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
 
 	WARN_ON(priv != file->private_data);
 
-	if (f->index >= icd->num_formats)
+	if (f->index >= icd->num_user_formats)
 		return -EINVAL;
 
-	format = &icd->formats[f->index];
+	format = icd->user_formats[f->index];
 
 	strlcpy(f->description, format->name, sizeof(f->description));
 	f->pixelformat = format->fourcc;
@@ -919,8 +974,6 @@ int soc_camera_video_start(struct soc_camera_device *icd)
 	vdev->minor		= -1;
 	vdev->tvnorms		= V4L2_STD_UNKNOWN,
 
-	icd->current_fmt = &icd->formats[0];
-
 	err = video_register_device(vdev, VFL_TYPE_GRABBER, vdev->minor);
 	if (err < 0) {
 		dev_err(vdev->parent, "video_register_device failed\n");
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index dddaf45..d6333a0 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -41,6 +41,8 @@ struct soc_camera_device {
 	const struct soc_camera_data_format *current_fmt;
 	const struct soc_camera_data_format *formats;
 	int num_formats;
+	const struct soc_camera_data_format **user_formats;
+	int num_user_formats;
 	struct module *owner;
 	void *host_priv;		/* per-device host private data */
 	/* soc_camera.c private count. Only accessed with video_lock held */
@@ -65,8 +67,10 @@ struct soc_camera_host_ops {
 	struct module *owner;
 	int (*add)(struct soc_camera_device *);
 	void (*remove)(struct soc_camera_device *);
-	int (*suspend)(struct soc_camera_device *, pm_message_t state);
+	int (*suspend)(struct soc_camera_device *, pm_message_t);
 	int (*resume)(struct soc_camera_device *);
+	int (*get_formats)(struct soc_camera_device *, int,
+			   const struct soc_camera_data_format **);
 	int (*set_fmt)(struct soc_camera_device *, __u32, struct v4l2_rect *);
 	int (*try_fmt)(struct soc_camera_device *, struct v4l2_format *);
 	void (*init_videobuf)(struct videobuf_queue *,

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 1/2 v3] soc-camera: pixel format negotiation - core support

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Allocate and fill a list of formats, supported by this specific 
> camera-host combination. Use it for format enumeration. Take care to stay 
> backwards-compatible.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

So, the translation api is dead, long live user_format :)

I'm a bit disappointed, as the things I pointed out are missing :
 - host format and sensor format association for debug purpose
   (think about sensor developpers)
 - current format : we never know what will be done through the host by its
 pointer (I'm not thinking about end user, I'm still thinking about soc_camera
 point of view).

But anyway, that's life. My review of patch 2 will follow, this one looks fine
(though not tested yet).

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 1/2 v3] soc-camera: pixel format negotiation - core support

[Guennadi Liakhovetski]

On Wed, 19 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Allocate and fill a list of formats, supported by this specific 
> > camera-host combination. Use it for format enumeration. Take care to stay 
> > backwards-compatible.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> So, the translation api is dead, long live user_format :)
> 
> I'm a bit disappointed, as the things I pointed out are missing :
>  - host format and sensor format association for debug purpose
>    (think about sensor developpers)

Yes, it is missing, and I explained why I wasn't so keen on adding two new 
structs to a central module and a bunch of code only for debugging, which 
you only need while developing new camera host drivers. And as I 
implemented in pxa-camera, this debugging can easily be done in host 
drivers as required.

>  - current format : we never know what will be done through the host by its
>  pointer (I'm not thinking about end user, I'm still thinking about soc_camera
>  point of view).

Sorry, I do not quite understand your concern here. Are you unhappy, that 
host drivers ae now expected to assign the current_fmt pointer? But this 
has also been the case with your patch-series. Please, elaborate.

> But anyway, that's life. My review of patch 2 will follow, this one looks fine
> (though not tested yet).

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH 2/2 v3] pxa-camera: pixel format negotiation

[Guennadi Liakhovetski]

Use the new format-negotiation infrastructure, support all four YUV422 
packed and the planar formats.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

---

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 37afdfa..1bcdb5d 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -765,6 +765,9 @@ static int test_platform_param(struct pxa_camera_dev *pcdev,
 		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8))
 			return -EINVAL;
 		*flags |= SOCAM_DATAWIDTH_8;
+		break;
+	default:
+		return -EINVAL;
 	}
 
 	return 0;
@@ -823,12 +826,10 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	 * We fix bit-per-pixel equal to data-width... */
 	switch (common_flags & SOCAM_DATAWIDTH_MASK) {
 	case SOCAM_DATAWIDTH_10:
-		icd->buswidth = 10;
 		dw = 4;
 		bpp = 0x40;
 		break;
 	case SOCAM_DATAWIDTH_9:
-		icd->buswidth = 9;
 		dw = 3;
 		bpp = 0x20;
 		break;
@@ -836,7 +837,6 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 		/* Actually it can only be 8 now,
 		 * default is just to silence compiler warnings */
 	case SOCAM_DATAWIDTH_8:
-		icd->buswidth = 8;
 		dw = 2;
 		bpp = 0;
 	}
@@ -862,7 +862,17 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	case V4L2_PIX_FMT_YUV422P:
 		pcdev->channels = 3;
 		cicr1 |= CICR1_YCBCR_F;
+		/*
+		 * Normally, pxa bus wants as input UYVY format. We allow all
+		 * reorderings of the YUV422 format, as no processing is done,
+		 * and the YUV stream is just passed through without any
+		 * transformation. Note that UYVY is the only format that
+		 * should be used if pxa framebuffer Overlay2 is used.
+		 */
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_VYUY:
 	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_YVYU:
 		cicr1 |= CICR1_COLOR_SP_VAL(2);
 		break;
 	case V4L2_PIX_FMT_RGB555:
@@ -888,13 +898,14 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	return 0;
 }
 
-static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
+static int pxa_camera_try_bus_param(struct soc_camera_device *icd,
+				    unsigned char buswidth)
 {
 	struct soc_camera_host *ici =
 		to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	unsigned long bus_flags, camera_flags;
-	int ret = test_platform_param(pcdev, icd->buswidth, &bus_flags);
+	int ret = test_platform_param(pcdev, buswidth, &bus_flags);
 
 	if (ret < 0)
 		return ret;
@@ -904,25 +915,133 @@ static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	return soc_camera_bus_param_compatible(camera_flags, bus_flags) ? 0 : -EINVAL;
 }
 
+static const struct soc_camera_data_format pxa_camera_formats[] = {
+	{
+		.name		= "Planar YUV422 16 bit",
+		.depth		= 16,
+		.fourcc		= V4L2_PIX_FMT_YUV422P,
+		.colorspace	= V4L2_COLORSPACE_JPEG,
+	},
+};
+
+static bool depth_supported(struct soc_camera_device *icd, int depth)
+{
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
+
+	switch (depth) {
+	case 8:
+		return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8);
+	case 9:
+		return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_9);
+	case 10:
+		return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_10);
+	}
+	return false;
+}
+
+static int pxa_camera_get_formats(struct soc_camera_device *icd, int idx,
+				  const struct soc_camera_data_format **fmt)
+{
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
+	int formats = 0;
+
+	switch (icd->formats[idx].fourcc) {
+	case V4L2_PIX_FMT_UYVY:
+		formats++;
+		if (fmt && (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
+			*fmt++ = &pxa_camera_formats[0];
+			dev_dbg(&ici->dev, "Providing format %s using %s\n",
+				pxa_camera_formats[0].name,
+				icd->formats[idx].name);
+		}
+	case V4L2_PIX_FMT_VYUY:
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_YVYU:
+	case V4L2_PIX_FMT_RGB565:
+	case V4L2_PIX_FMT_RGB555:
+		formats++;
+		if (fmt && (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
+			*fmt++ = &icd->formats[idx];
+			dev_dbg(&ici->dev, "Providing format %s packed\n",
+				icd->formats[idx].name);
+		}
+		break;
+	default:
+		/* Generic pass-through */
+		if (depth_supported(icd, icd->formats[idx].depth)) {
+			formats++;
+			if (fmt) {
+				*fmt++ = &icd->formats[idx];
+				dev_dbg(&ici->dev,
+					"Providing format %s in pass-through mode\n",
+					icd->formats[idx].name);
+			}
+		}
+	}
+
+	return formats;
+}
+
 static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 			      __u32 pixfmt, struct v4l2_rect *rect)
 {
-	const struct soc_camera_data_format *cam_fmt;
-	int ret;
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
+	const struct soc_camera_data_format *host_fmt = NULL;
+	int ret, buswidth;
 
-	/*
-	 * TODO: find a suitable supported by the SoC output format, check
-	 * whether the sensor supports one of acceptable input formats.
-	 */
-	if (pixfmt) {
-		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
-		if (!cam_fmt)
+	switch (pixfmt) {
+	case V4L2_PIX_FMT_YUV422P:
+		host_fmt = &pxa_camera_formats[0];
+		pixfmt = V4L2_PIX_FMT_UYVY;
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_VYUY:
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_YVYU:
+	case V4L2_PIX_FMT_RGB565:
+	case V4L2_PIX_FMT_RGB555:
+		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
+			dev_warn(&ici->dev,
+				 "8 bit bus unsupported, but required for format %x\n",
+				 pixfmt);
+			return -EINVAL;
+		}
+
+		if (!host_fmt)
+			host_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
+
+		if (!host_fmt) {
+			dev_warn(&ici->dev, "Format %x not found\n", pixfmt);
 			return -EINVAL;
+		}
+		buswidth = 8;
+	case 0:				/* Only geometry change */
+		ret = icd->ops->set_fmt(icd, pixfmt, rect);
+		break;
+	default:
+		/* Generic pass-through */
+		host_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
+		if (!host_fmt || !depth_supported(icd, host_fmt->depth)) {
+			dev_warn(&ici->dev,
+				 "Format %x unsupported in pass-through mode\n",
+				 pixfmt);
+			return -EINVAL;
+		}
+
+		buswidth = host_fmt->depth;
+		ret = icd->ops->set_fmt(icd, pixfmt, rect);
 	}
 
-	ret = icd->ops->set_fmt(icd, pixfmt, rect);
-	if (pixfmt && !ret)
-		icd->current_fmt = cam_fmt;
+	if (ret < 0)
+		dev_warn(&ici->dev, "Failed to configure for format %x\n",
+			 pixfmt);
+
+	if (pixfmt && !ret) {
+		icd->buswidth = buswidth;
+		icd->current_fmt = host_fmt;
+	}
 
 	return ret;
 }
@@ -930,34 +1049,70 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 static int pxa_camera_try_fmt(struct soc_camera_device *icd,
 			      struct v4l2_format *f)
 {
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
 	const struct soc_camera_data_format *cam_fmt;
-	int ret = pxa_camera_try_bus_param(icd, f->fmt.pix.pixelformat);
+	struct v4l2_pix_format *pix = &f->fmt.pix;
+	__u32 pixfmt = pix->pixelformat;
+	unsigned char buswidth;
+	int ret;
 
-	if (ret < 0)
-		return ret;
+	switch (pixfmt) {
+	case V4L2_PIX_FMT_YUV422P:
+		pixfmt = V4L2_PIX_FMT_UYVY;
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_VYUY:
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_YVYU:
+	case V4L2_PIX_FMT_RGB565:
+	case V4L2_PIX_FMT_RGB555:
+		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
+			dev_warn(&ici->dev,
+				 "try-fmt: 8 bit bus unsupported for format %x\n",
+				 pixfmt);
+			return -EINVAL;
+		}
 
-	/*
-	 * TODO: find a suitable supported by the SoC output format, check
-	 * whether the sensor supports one of acceptable input formats.
-	 */
-	cam_fmt = soc_camera_format_by_fourcc(icd, f->fmt.pix.pixelformat);
-	if (!cam_fmt)
-		return -EINVAL;
+		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
+		if (!cam_fmt) {
+			dev_warn(&ici->dev, "try-fmt: format %x not found\n",
+				 pixfmt);
+			return -EINVAL;
+		}
+		buswidth = 8;
+		break;
+	default:
+		/* Generic pass-through */
+		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
+		if (!cam_fmt || !depth_supported(icd, cam_fmt->depth)) {
+			dev_warn(&ici->dev,
+				 "try-fmt: Format %x unsupported in pass-through\n",
+				 pixfmt);
+			return -EINVAL;
+		}
+		buswidth = cam_fmt->depth;
+	}
+
+	ret = pxa_camera_try_bus_param(icd, buswidth);
+
+	if (ret < 0) {
+		dev_warn(&ici->dev, "Incompatible bus parameters!\n");
+		return ret;
+	}
 
 	/* limit to pxa hardware capabilities */
-	if (f->fmt.pix.height < 32)
-		f->fmt.pix.height = 32;
-	if (f->fmt.pix.height > 2048)
-		f->fmt.pix.height = 2048;
-	if (f->fmt.pix.width < 48)
-		f->fmt.pix.width = 48;
-	if (f->fmt.pix.width > 2048)
-		f->fmt.pix.width = 2048;
-	f->fmt.pix.width &= ~0x01;
-
-	f->fmt.pix.bytesperline = f->fmt.pix.width *
-		DIV_ROUND_UP(cam_fmt->depth, 8);
-	f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
+	if (pix->height < 32)
+		pix->height = 32;
+	if (pix->height > 2048)
+		pix->height = 2048;
+	if (pix->width < 48)
+		pix->width = 48;
+	if (pix->width > 2048)
+		pix->width = 2048;
+	pix->width &= ~0x01;
+
+	pix->bytesperline = pix->width * DIV_ROUND_UP(cam_fmt->depth, 8);
+	pix->sizeimage = pix->height * pix->bytesperline;
 
 	/* limit to sensor capabilities */
 	return icd->ops->try_fmt(icd, f);
@@ -1068,6 +1223,7 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
 	.remove		= pxa_camera_remove_device,
 	.suspend	= pxa_camera_suspend,
 	.resume		= pxa_camera_resume,
+	.get_formats	= pxa_camera_get_formats,
 	.set_fmt	= pxa_camera_set_fmt,
 	.try_fmt	= pxa_camera_try_fmt,
 	.init_videobuf	= pxa_camera_init_videobuf,

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2 v3] pxa-camera: pixel format negotiation

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Use the new format-negotiation infrastructure, support all four YUV422 
> packed and the planar formats.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>
> ---
Hi Guennadi,

Please find my review here.

> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
> index 37afdfa..1bcdb5d 100644
> --- a/drivers/media/video/pxa_camera.c
> +++ b/drivers/media/video/pxa_camera.c
> @@ -765,6 +765,9 @@ static int test_platform_param(struct pxa_camera_dev *pcdev,
>  		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8))
>  			return -EINVAL;
>  		*flags |= SOCAM_DATAWIDTH_8;
> +		break;
> +	default:
> +		return -EINVAL;
If we're in pass-through mode, and depth is 16 (example: a today unknown RYB
format), we return -EINVAL. Is that on purpose ?

> -static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> +static int pxa_camera_try_bus_param(struct soc_camera_device *icd,
> +				    unsigned char buswidth)
>  {
>  	struct soc_camera_host *ici =
>  		to_soc_camera_host(icd->dev.parent);
>  	struct pxa_camera_dev *pcdev = ici->priv;
>  	unsigned long bus_flags, camera_flags;
> -	int ret = test_platform_param(pcdev, icd->buswidth, &bus_flags);
> +	int ret = test_platform_param(pcdev, buswidth, &bus_flags);
Why do we bother testing it ? If format negociation was done before, a format
asked for is necessarily available, otherwise it should have been removed at
format generation.

Likewise, is bus param matching necessary here, or should it be done at format
generation ? Can that be really be dynamic, or is it constrained by hardware,
and thus only necessary at startup, and not at each format try ?

<snip>
> +static int pxa_camera_get_formats(struct soc_camera_device *icd, int idx,
> +				  const struct soc_camera_data_format **fmt)
> +{
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> +	struct pxa_camera_dev *pcdev = ici->priv;
> +	int formats = 0;
> +
> +	switch (icd->formats[idx].fourcc) {
> +	case V4L2_PIX_FMT_UYVY:
> +		formats++;
> +		if (fmt && (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
> +			*fmt++ = &pxa_camera_formats[0];
> +			dev_dbg(&ici->dev, "Providing format %s using %s\n",
> +				pxa_camera_formats[0].name,
> +				icd->formats[idx].name);
> +		}
> +	case V4L2_PIX_FMT_VYUY:
> +	case V4L2_PIX_FMT_YUYV:
> +	case V4L2_PIX_FMT_YVYU:
> +	case V4L2_PIX_FMT_RGB565:
> +	case V4L2_PIX_FMT_RGB555:
> +		formats++;
> +		if (fmt && (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
> +			*fmt++ = &icd->formats[idx];
> +			dev_dbg(&ici->dev, "Providing format %s packed\n",
> +				icd->formats[idx].name);
> +		}
> +		break;
What if pcdev->platform_flags is 9 bits wide and sensor provides RGB565 ?
Variable formats will be incremented, but fmt will never be filled in. So there
will be holes in fmt. Shouldn't the formats++ depend on platform_flags &
PXA_CAMERA_DATAWIDTH_8 ?

> +	default:
> +		/* Generic pass-through */
> +		if (depth_supported(icd, icd->formats[idx].depth)) {
> +			formats++;
> +			if (fmt) {
> +				*fmt++ = &icd->formats[idx];
> +				dev_dbg(&ici->dev,
> +					"Providing format %s in pass-through mode\n",
> +					icd->formats[idx].name);
> +			}
> +		}
> +	}
Dito for formats++.

>  static int pxa_camera_set_fmt(struct soc_camera_device *icd,
>  			      __u32 pixfmt, struct v4l2_rect *rect)
<snip>
> +	case V4L2_PIX_FMT_UYVY:
> +	case V4L2_PIX_FMT_VYUY:
> +	case V4L2_PIX_FMT_YUYV:
> +	case V4L2_PIX_FMT_YVYU:
> +	case V4L2_PIX_FMT_RGB565:
> +	case V4L2_PIX_FMT_RGB555:
> +		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
> +			dev_warn(&ici->dev,
> +				 "8 bit bus unsupported, but required for format %x\n",
> +				 pixfmt);
> +			return -EINVAL;
Shouldn't that be already computed by format generation ?

> +		/* Generic pass-through */
> +		host_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
> +		if (!host_fmt || !depth_supported(icd, host_fmt->depth)) {
> +			dev_warn(&ici->dev,
> +				 "Format %x unsupported in pass-through mode\n",
> +				 pixfmt);
> +			return -EINVAL;
> +		}
Ditto.

> @@ -930,34 +1049,70 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
>  static int pxa_camera_try_fmt(struct soc_camera_device *icd,
>  			      struct v4l2_format *f)
>  {
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> +	struct pxa_camera_dev *pcdev = ici->priv;
>  	const struct soc_camera_data_format *cam_fmt;
> -	int ret = pxa_camera_try_bus_param(icd, f->fmt.pix.pixelformat);
> +	struct v4l2_pix_format *pix = &f->fmt.pix;
> +	__u32 pixfmt = pix->pixelformat;
> +	unsigned char buswidth;
> +	int ret;
>  
> -	if (ret < 0)
> -		return ret;
> +	switch (pixfmt) {
> +	case V4L2_PIX_FMT_YUV422P:
> +		pixfmt = V4L2_PIX_FMT_UYVY;
> +	case V4L2_PIX_FMT_UYVY:
> +	case V4L2_PIX_FMT_VYUY:
> +	case V4L2_PIX_FMT_YUYV:
> +	case V4L2_PIX_FMT_YVYU:
> +	case V4L2_PIX_FMT_RGB565:
> +	case V4L2_PIX_FMT_RGB555:
> +		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
> +			dev_warn(&ici->dev,
> +				 "try-fmt: 8 bit bus unsupported for format %x\n",
> +				 pixfmt);
> +			return -EINVAL;
> +		}
Ditto.

>  
> -	/*
> -	 * TODO: find a suitable supported by the SoC output format, check
> -	 * whether the sensor supports one of acceptable input formats.
> -	 */
> -	cam_fmt = soc_camera_format_by_fourcc(icd, f->fmt.pix.pixelformat);
> -	if (!cam_fmt)
> -		return -EINVAL;
> +		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
> +		if (!cam_fmt) {
> +			dev_warn(&ici->dev, "try-fmt: format %x not found\n",
> +				 pixfmt);
> +			return -EINVAL;
> +		}
> +		buswidth = 8;
> +		break;
> +	default:
> +		/* Generic pass-through */
> +		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
> +		if (!cam_fmt || !depth_supported(icd, cam_fmt->depth)) {
> +			dev_warn(&ici->dev,
> +				 "try-fmt: Format %x unsupported in pass-through\n",
> +				 pixfmt);
> +			return -EINVAL;
> +		}
> +		buswidth = cam_fmt->depth;
> +	}
> +
> +	ret = pxa_camera_try_bus_param(icd, buswidth);
Ditto.

All in all, I wonder why we need that many tests, and if we could reduce them at
format generation (under hypothesis that platform_flags are constant and sensor
flags are constant).

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2 v3] pxa-camera: pixel format negotiation

[Guennadi Liakhovetski]

On Wed, 19 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Use the new format-negotiation infrastructure, support all four YUV422 
> > packed and the planar formats.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >
> > ---
> Hi Guennadi,
> 
> Please find my review here.
> 
> > diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
> > index 37afdfa..1bcdb5d 100644
> > --- a/drivers/media/video/pxa_camera.c
> > +++ b/drivers/media/video/pxa_camera.c
> > @@ -765,6 +765,9 @@ static int test_platform_param(struct pxa_camera_dev *pcdev,
> >  		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8))
> >  			return -EINVAL;
> >  		*flags |= SOCAM_DATAWIDTH_8;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> If we're in pass-through mode, and depth is 16 (example: a today unknown RYB
> format), we return -EINVAL. Is that on purpose ?

Yes, I do not know how to pass a 16-bit format in a pass-through mode, and 
I don't have a test-case for it. Do you?

> > -static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> > +static int pxa_camera_try_bus_param(struct soc_camera_device *icd,
> > +				    unsigned char buswidth)
> >  {
> >  	struct soc_camera_host *ici =
> >  		to_soc_camera_host(icd->dev.parent);
> >  	struct pxa_camera_dev *pcdev = ici->priv;
> >  	unsigned long bus_flags, camera_flags;
> > -	int ret = test_platform_param(pcdev, icd->buswidth, &bus_flags);
> > +	int ret = test_platform_param(pcdev, buswidth, &bus_flags);
> Why do we bother testing it ? If format negociation was done before, a format
> asked for is necessarily available, otherwise it should have been removed at
> format generation.
> 
> Likewise, is bus param matching necessary here, or should it be done at format
> generation ? Can that be really be dynamic, or is it constrained by hardware,
> and thus only necessary at startup, and not at each format try ?

Hm, ok, so far I don't have any cases, where bus parameters can change at 
run-time. So, yes, I think, we could shift it into 
pxa_camera_get_formats().

> <snip>
> > +static int pxa_camera_get_formats(struct soc_camera_device *icd, int idx,
> > +				  const struct soc_camera_data_format **fmt)
> > +{
> > +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > +	struct pxa_camera_dev *pcdev = ici->priv;
> > +	int formats = 0;
> > +
> > +	switch (icd->formats[idx].fourcc) {
> > +	case V4L2_PIX_FMT_UYVY:
> > +		formats++;
> > +		if (fmt && (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
> > +			*fmt++ = &pxa_camera_formats[0];
> > +			dev_dbg(&ici->dev, "Providing format %s using %s\n",
> > +				pxa_camera_formats[0].name,
> > +				icd->formats[idx].name);
> > +		}
> > +	case V4L2_PIX_FMT_VYUY:
> > +	case V4L2_PIX_FMT_YUYV:
> > +	case V4L2_PIX_FMT_YVYU:
> > +	case V4L2_PIX_FMT_RGB565:
> > +	case V4L2_PIX_FMT_RGB555:
> > +		formats++;
> > +		if (fmt && (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
> > +			*fmt++ = &icd->formats[idx];
> > +			dev_dbg(&ici->dev, "Providing format %s packed\n",
> > +				icd->formats[idx].name);
> > +		}
> > +		break;
> What if pcdev->platform_flags is 9 bits wide and sensor provides RGB565 ?
> Variable formats will be incremented, but fmt will never be filled in. So there
> will be holes in fmt. Shouldn't the formats++ depend on platform_flags &
> PXA_CAMERA_DATAWIDTH_8 ?

Right, that's a bug, will fix, thanks. Same as above for 
V4L2_PIX_FMT_UYVY.

> > +	default:
> > +		/* Generic pass-through */
> > +		if (depth_supported(icd, icd->formats[idx].depth)) {
> > +			formats++;
> > +			if (fmt) {
> > +				*fmt++ = &icd->formats[idx];
> > +				dev_dbg(&ici->dev,
> > +					"Providing format %s in pass-through mode\n",
> > +					icd->formats[idx].name);
> > +			}
> > +		}
> > +	}
> Dito for formats++.

No, this looks correct - it first checks for depth_supported().

> >  static int pxa_camera_set_fmt(struct soc_camera_device *icd,
> >  			      __u32 pixfmt, struct v4l2_rect *rect)
> <snip>
> > +	case V4L2_PIX_FMT_UYVY:
> > +	case V4L2_PIX_FMT_VYUY:
> > +	case V4L2_PIX_FMT_YUYV:
> > +	case V4L2_PIX_FMT_YVYU:
> > +	case V4L2_PIX_FMT_RGB565:
> > +	case V4L2_PIX_FMT_RGB555:
> > +		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
> > +			dev_warn(&ici->dev,
> > +				 "8 bit bus unsupported, but required for format %x\n",
> > +				 pixfmt);
> > +			return -EINVAL;
> Shouldn't that be already computed by format generation ?
> 
> > +		/* Generic pass-through */
> > +		host_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
> > +		if (!host_fmt || !depth_supported(icd, host_fmt->depth)) {
> > +			dev_warn(&ici->dev,
> > +				 "Format %x unsupported in pass-through mode\n",
> > +				 pixfmt);
> > +			return -EINVAL;
> > +		}
> Ditto.

I know this code repeats, and it is not nice. But as I was writing it I 
didn't see another possibility. Or more precisely, I did see it, but I 
couldn't compare the two versions well without having at least one of them 
in code in front of my eyes:-) Now that I see it, I think, yes, there is a 
way to do this only once by using a translation struct similar to what you 
have proposed. Now _this_ would be a possibly important advantage, so it 
is useful not _only_ for debugging:-) But we would have to extend it with 
at least a buswidth. Like

	const struct soc_camera_data_format *cam_fmt;
	const struct soc_camera_data_format *host_fmt;
	unsigned char buswidth;

Now this _seems_ to provide the complete information so far... In 
pxa_camera_get_formats() we would

1. compute camera- and host-formats and buswidth
2. call pxa_camera_try_bus_param() to check bus-parameter compatibility

and then in try_fmt() and set_fmt() just traverse the list of translation 
structs and adjust geometry?

> > @@ -930,34 +1049,70 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
> >  static int pxa_camera_try_fmt(struct soc_camera_device *icd,
> >  			      struct v4l2_format *f)
> >  {
> > +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > +	struct pxa_camera_dev *pcdev = ici->priv;
> >  	const struct soc_camera_data_format *cam_fmt;
> > -	int ret = pxa_camera_try_bus_param(icd, f->fmt.pix.pixelformat);
> > +	struct v4l2_pix_format *pix = &f->fmt.pix;
> > +	__u32 pixfmt = pix->pixelformat;
> > +	unsigned char buswidth;
> > +	int ret;
> >  
> > -	if (ret < 0)
> > -		return ret;
> > +	switch (pixfmt) {
> > +	case V4L2_PIX_FMT_YUV422P:
> > +		pixfmt = V4L2_PIX_FMT_UYVY;
> > +	case V4L2_PIX_FMT_UYVY:
> > +	case V4L2_PIX_FMT_VYUY:
> > +	case V4L2_PIX_FMT_YUYV:
> > +	case V4L2_PIX_FMT_YVYU:
> > +	case V4L2_PIX_FMT_RGB565:
> > +	case V4L2_PIX_FMT_RGB555:
> > +		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
> > +			dev_warn(&ici->dev,
> > +				 "try-fmt: 8 bit bus unsupported for format %x\n",
> > +				 pixfmt);
> > +			return -EINVAL;
> > +		}
> Ditto.
> 
> >  
> > -	/*
> > -	 * TODO: find a suitable supported by the SoC output format, check
> > -	 * whether the sensor supports one of acceptable input formats.
> > -	 */
> > -	cam_fmt = soc_camera_format_by_fourcc(icd, f->fmt.pix.pixelformat);
> > -	if (!cam_fmt)
> > -		return -EINVAL;
> > +		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
> > +		if (!cam_fmt) {
> > +			dev_warn(&ici->dev, "try-fmt: format %x not found\n",
> > +				 pixfmt);
> > +			return -EINVAL;
> > +		}
> > +		buswidth = 8;
> > +		break;
> > +	default:
> > +		/* Generic pass-through */
> > +		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
> > +		if (!cam_fmt || !depth_supported(icd, cam_fmt->depth)) {
> > +			dev_warn(&ici->dev,
> > +				 "try-fmt: Format %x unsupported in pass-through\n",
> > +				 pixfmt);
> > +			return -EINVAL;
> > +		}
> > +		buswidth = cam_fmt->depth;
> > +	}
> > +
> > +	ret = pxa_camera_try_bus_param(icd, buswidth);
> Ditto.
> 
> All in all, I wonder why we need that many tests, and if we could reduce them at
> format generation (under hypothesis that platform_flags are constant and sensor
> flags are constant).

Ok, I propose you make the next round:-) I would be pleased if you base 
your new patches on these my two, and just replace the user_formats with a 
translation list, and modify pxa try_fmt() and set_fmt() as discussed 
above. I would be quite happy if you mark them "From: <you>". Or if you do 
not want to - let me know, I'll do it. And please do not make 13 patches 
this time:-) I think, two should be enough.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2 v3] pxa-camera: pixel format negotiation

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>> If we're in pass-through mode, and depth is 16 (example: a today unknown RYB
>> format), we return -EINVAL. Is that on purpose ?
>
> Yes, I do not know how to pass a 16-bit format in a pass-through mode, and 
> I don't have a test-case for it. Do you?
BYR2 I think (12bit Bayer in 16bit words), and Bxxx (10bit Bayer in 16bit
words).

And I can test the 10bit Bayer on 16bit words on mt9m111, and will do.

>> > -	int ret = test_platform_param(pcdev, icd->buswidth, &bus_flags);
>> > +	int ret = test_platform_param(pcdev, buswidth, &bus_flags);
<snip>
>> Likewise, is bus param matching necessary here, or should it be done at format
>> generation ? Can that be really be dynamic, or is it constrained by hardware,
>> and thus only necessary at startup, and not at each format try ?
>
> Hm, ok, so far I don't have any cases, where bus parameters can change at 
> run-time. So, yes, I think, we could shift it into 
> pxa_camera_get_formats().
Right. Roger.

>> What if pcdev->platform_flags is 9 bits wide and sensor provides RGB565 ?
>> Variable formats will be incremented, but fmt will never be filled in. So there
>> will be holes in fmt. Shouldn't the formats++ depend on platform_flags &
>> PXA_CAMERA_DATAWIDTH_8 ?
>
> Right, that's a bug, will fix, thanks. Same as above for 
> V4L2_PIX_FMT_UYVY.
OK.

>> > +	default:
>> > +		/* Generic pass-through */
>> > +		if (depth_supported(icd, icd->formats[idx].depth)) {
>> > +			formats++;
>> > +			if (fmt) {
<snip>
>
> No, this looks correct - it first checks for depth_supported().
You're right, sorry.

> I know this code repeats, and it is not nice. But as I was writing it I 
> didn't see another possibility. Or more precisely, I did see it, but I 
> couldn't compare the two versions well without having at least one of them 
> in code in front of my eyes:-) Now that I see it, I think, yes, there is a 
> way to do this only once by using a translation struct similar to what you 
> have proposed. Now _this_ would be a possibly important advantage, so it 
> is useful not _only_ for debugging:-) But we would have to extend it with 
> at least a buswidth. Like
>
> 	const struct soc_camera_data_format *cam_fmt;
> 	const struct soc_camera_data_format *host_fmt;
> 	unsigned char buswidth;
>
> Now this _seems_ to provide the complete information so far... In 
> pxa_camera_get_formats() we would
>
> 1. compute camera- and host-formats and buswidth
> 2. call pxa_camera_try_bus_param() to check bus-parameter compatibility
>
> and then in try_fmt() and set_fmt() just traverse the list of translation 
> structs and adjust geometry?
That sounds great. I'm on it.

>> All in all, I wonder why we need that many tests, and if we could reduce them at
>> format generation (under hypothesis that platform_flags are constant and sensor
>> flags are constant).
>
> Ok, I propose you make the next round:-) I would be pleased if you base 
> your new patches on these my two, and just replace the user_formats with a 
> translation list, and modify pxa try_fmt() and set_fmt() as discussed 
> above. I would be quite happy if you mark them "From: <you>". Or if you do 
> not want to - let me know, I'll do it. And please do not make 13 patches 
> this time:-) I think, two should be enough.
I'll be happy to make the next round.

Give me a couple of days, and I'll post the 2 patches, on top of your serie
(serie which will end with your 2 patches). After review, you can either merge
each one of them with yours, or take them apart.

Don't worry, I won't flood the list anymore :)

Cheers.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2 v3] pxa-camera: pixel format negotiation

[Guennadi Liakhovetski]

Hi Robert,

On Fri, 21 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> >> If we're in pass-through mode, and depth is 16 (example: a today unknown RYB
> >> format), we return -EINVAL. Is that on purpose ?
> >
> > Yes, I do not know how to pass a 16-bit format in a pass-through mode, and 
> > I don't have a test-case for it. Do you?
> BYR2 I think (12bit Bayer in 16bit words), and Bxxx (10bit Bayer in 16bit
> words).
> 
> And I can test the 10bit Bayer on 16bit words on mt9m111, and will do.

Wait, don't understand. 10-bit Bayer should have depth = 10, so it will 
pass. 12-bit Bayer will have depth 12 and will not pass, and I do not know 
how we can accept it on PXA27x.

> > I know this code repeats, and it is not nice. But as I was writing it I 
> > didn't see another possibility. Or more precisely, I did see it, but I 
> > couldn't compare the two versions well without having at least one of them 
> > in code in front of my eyes:-) Now that I see it, I think, yes, there is a 
> > way to do this only once by using a translation struct similar to what you 
> > have proposed. Now _this_ would be a possibly important advantage, so it 
> > is useful not _only_ for debugging:-) But we would have to extend it with 
> > at least a buswidth. Like
> >
> > 	const struct soc_camera_data_format *cam_fmt;
> > 	const struct soc_camera_data_format *host_fmt;
> > 	unsigned char buswidth;
> >
> > Now this _seems_ to provide the complete information so far... In 
> > pxa_camera_get_formats() we would
> >
> > 1. compute camera- and host-formats and buswidth
> > 2. call pxa_camera_try_bus_param() to check bus-parameter compatibility
> >
> > and then in try_fmt() and set_fmt() just traverse the list of translation 
> > structs and adjust geometry?
> That sounds great. I'm on it.
> 
> >> All in all, I wonder why we need that many tests, and if we could reduce them at
> >> format generation (under hypothesis that platform_flags are constant and sensor
> >> flags are constant).
> >
> > Ok, I propose you make the next round:-) I would be pleased if you base 
> > your new patches on these my two, and just replace the user_formats with a 
> > translation list, and modify pxa try_fmt() and set_fmt() as discussed 
> > above. I would be quite happy if you mark them "From: <you>". Or if you do 
> > not want to - let me know, I'll do it. And please do not make 13 patches 
> > this time:-) I think, two should be enough.
> I'll be happy to make the next round.
> 
> Give me a couple of days, and I'll post the 2 patches, on top of your serie
> (serie which will end with your 2 patches). After review, you can either merge
> each one of them with yours, or take them apart.
> 
> Don't worry, I won't flood the list anymore :)

Good, I think, we can use the next week, as long as Linus is scuba 
diving, to finish this transition:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2 v3] pxa-camera: pixel format negotiation

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>> > Yes, I do not know how to pass a 16-bit format in a pass-through mode, and 
>> > I don't have a test-case for it. Do you?
>> BYR2 I think (12bit Bayer in 16bit words), and Bxxx (10bit Bayer in 16bit
>> words).
>> 
>> And I can test the 10bit Bayer on 16bit words on mt9m111, and will do.
>
> Wait, don't understand. 10-bit Bayer should have depth = 10, so it will 
> pass. 12-bit Bayer will have depth 12 and will not pass, and I do not know 
> how we can accept it on PXA27x.
I should have been clearer.

It's called 8+2 bypass Bayer. Here is the layout :
 - first byte : <B9, B8, B7, B6, B5, B4, B3, B2>
 - second byte : <0, 0, 0, 0, 0, 0, B1, B0>
 => 2 bytes of 8 bits are sent over 8 bits of QIF interface
 => gives a Bayer Code of <0, 0, 0, 0, 0, 0, B9 - B0>

I think it is documented in Micron MT9M111 datasheet, table 6, page 14.
My understanding is that it has a buswidth=8, and depth=16. But I may be wrong,
have a look with your trained eye and tell me please.

> Good, I think, we can use the next week, as long as Linus is scuba 
> diving, to finish this transition:-)
Lucky him :) It's rainy, snowy here ... but ... it gives time to write code :)

Cheers.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2 v3] pxa-camera: pixel format negotiation

[Guennadi Liakhovetski]

On Fri, 21 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> >> > Yes, I do not know how to pass a 16-bit format in a pass-through mode, and 
> >> > I don't have a test-case for it. Do you?
> >> BYR2 I think (12bit Bayer in 16bit words), and Bxxx (10bit Bayer in 16bit
> >> words).
> >> 
> >> And I can test the 10bit Bayer on 16bit words on mt9m111, and will do.
> >
> > Wait, don't understand. 10-bit Bayer should have depth = 10, so it will 
> > pass. 12-bit Bayer will have depth 12 and will not pass, and I do not know 
> > how we can accept it on PXA27x.
> I should have been clearer.
> 
> It's called 8+2 bypass Bayer. Here is the layout :
>  - first byte : <B9, B8, B7, B6, B5, B4, B3, B2>
>  - second byte : <0, 0, 0, 0, 0, 0, B1, B0>
>  => 2 bytes of 8 bits are sent over 8 bits of QIF interface
>  => gives a Bayer Code of <0, 0, 0, 0, 0, 0, B9 - B0>
> 
> I think it is documented in Micron MT9M111 datasheet, table 6, page 14.
> My understanding is that it has a buswidth=8, and depth=16. But I may be wrong,
> have a look with your trained eye and tell me please.

I think we shouldn't (and possibly cannot) process this data in 
pass-through mode on pxa270. In raw mode pxa270 expects each pixel to only 
occupy one pixel clock. And we use icd->width and icd->height to configure 
PXA registers in pxa_camera_set_bus_param(). Whereas in this case we would 
have to lie to the PXA and configure it with, for example, the double 
line-width. I think, this way it could work. Then your horizontal sync 
would stay valid. So, I think, we have three options with this format:

1. Refuse to support this configuration, as PXA doesn't support 2 pixel 
clocks per pixel in raw mode

2. Extend the API even further to allow for different geometries on the 
sensor and on the controller. This, in fact, will anyway be required once 
we support scaling on host...

3. Create a special translation entry for this mode and abuse some 16-bit 
preprocessed format, like, e.g., RGB565. I _think_ this would work too, 
because, in the end, PXA doesn't know what colour it should be:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2 v3] pxa-camera: pixel format negotiation

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>> I think it is documented in Micron MT9M111 datasheet, table 6, page 14.
>> My understanding is that it has a buswidth=8, and depth=16. But I may be wrong,
>> have a look with your trained eye and tell me please.
>
> I think we shouldn't (and possibly cannot) process this data in 
> pass-through mode on pxa270. In raw mode pxa270 expects each pixel to only 
> occupy one pixel clock. And we use icd->width and icd->height to configure 
> PXA registers in pxa_camera_set_bus_param(). Whereas in this case we would 
> have to lie to the PXA and configure it with, for example, the double 
> line-width. I think, this way it could work. Then your horizontal sync 
> would stay valid. So, I think, we have three options with this format:
>
> 1. Refuse to support this configuration, as PXA doesn't support 2 pixel 
> clocks per pixel in raw mode
>
> 2. Extend the API even further to allow for different geometries on the 
> sensor and on the controller. This, in fact, will anyway be required once 
> we support scaling on host...
>
> 3. Create a special translation entry for this mode and abuse some 16-bit 
> preprocessed format, like, e.g., RGB565. I _think_ this would work too, 
> because, in the end, PXA doesn't know what colour it should be:-)

Wow, that's clearly stated :)

I like both solutions 2 and 3. In solution 3, on YUV variant would be nicer I
think, because pxa reorders RGB while packing it, whereas VYUY is just passed
through.
I would even prefer a bit solution 3, just slightly, to split true host scaling
from "simulated" host scaling.

Now, it's up to you, make a choice. This would be anyway for the next patch
serie, not this one. I think we should finish that one ASAP (and I'm a bit late)
to stabilize the API.

Cheers.

--
Robert

PS: I forgot ... now I aggree we shouldn't use pass-through mode for 16bit
exotic formats :)

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2 v3] pxa-camera: pixel format negotiation

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> I know this code repeats, and it is not nice. But as I was writing it I 
> didn't see another possibility. Or more precisely, I did see it, but I 
> couldn't compare the two versions well without having at least one of them 
> in code in front of my eyes:-) Now that I see it, I think, yes, there is a 
> way to do this only once by using a translation struct similar to what you 
> have proposed. Now _this_ would be a possibly important advantage, so it 
> is useful not _only_ for debugging:-) But we would have to extend it with 
> at least a buswidth. Like
>
> 	const struct soc_camera_data_format *cam_fmt;
> 	const struct soc_camera_data_format *host_fmt;
> 	unsigned char buswidth;
>
> Now this _seems_ to provide the complete information so far... In 
> pxa_camera_get_formats() we would
>
> 1. compute camera- and host-formats and buswidth
> 2. call pxa_camera_try_bus_param() to check bus-parameter compatibility
>
> and then in try_fmt() and set_fmt() just traverse the list of translation 
> structs and adjust geometry?

Hi Guennadi,

I began the work. I have a pending question here. Do you want to have the
translation structure fully contained into pxa_camera (in host_priv for
example), or do you want to replace the user formats by translation structure
(ie. soc_camera_init_user_formats() would generate a list of
soc_camera_format_translate instead of a list of soc_camera_data format) ?

I'm asking because in pxa_camera, there is no easy way to "guess" the size of
the array of translations. And as vmalloc() is done in
soc_camera_init_user_formats(), and allocates only soc_camera_data_format
structures, I see no easy way to generate the list of translations in
pxa_camera.c.

I thought I would modify soc_camera.c in this way :
static int soc_camera_init_user_formats(struct soc_camera_device *icd)
{
<snip>
-       icd->user_formats = vmalloc(sizeof(struct soc_camera_data_format *) *
-                                   fmts);
+       icd->user_formats =
+               vmalloc(sizeof(struct soc_camera_format_translate *) * fmts);
<snip>

Is that what you had in mind ? If not, how do you allocate the
soc_camera_format_translate array in pxa_camera.c ?

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2 v3] pxa-camera: pixel format negotiation

[Guennadi Liakhovetski]

On Sun, 23 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > I know this code repeats, and it is not nice. But as I was writing it I 
> > didn't see another possibility. Or more precisely, I did see it, but I 
> > couldn't compare the two versions well without having at least one of them 
> > in code in front of my eyes:-) Now that I see it, I think, yes, there is a 
> > way to do this only once by using a translation struct similar to what you 
> > have proposed. Now _this_ would be a possibly important advantage, so it 
> > is useful not _only_ for debugging:-) But we would have to extend it with 
> > at least a buswidth. Like
> >
> > 	const struct soc_camera_data_format *cam_fmt;
> > 	const struct soc_camera_data_format *host_fmt;
> > 	unsigned char buswidth;
> >
> > Now this _seems_ to provide the complete information so far... In 
> > pxa_camera_get_formats() we would
> >
> > 1. compute camera- and host-formats and buswidth
> > 2. call pxa_camera_try_bus_param() to check bus-parameter compatibility
> >
> > and then in try_fmt() and set_fmt() just traverse the list of translation 
> > structs and adjust geometry?
> 
> Hi Guennadi,
> 
> I began the work. I have a pending question here. Do you want to have the
> translation structure fully contained into pxa_camera (in host_priv for
> example), or do you want to replace the user formats by translation structure
> (ie. soc_camera_init_user_formats() would generate a list of
> soc_camera_format_translate instead of a list of soc_camera_data format) ?
> 
> I'm asking because in pxa_camera, there is no easy way to "guess" the size of
> the array of translations. And as vmalloc() is done in
> soc_camera_init_user_formats(), and allocates only soc_camera_data_format
> structures, I see no easy way to generate the list of translations in
> pxa_camera.c.
> 
> I thought I would modify soc_camera.c in this way :
> static int soc_camera_init_user_formats(struct soc_camera_device *icd)
> {
> <snip>
> -       icd->user_formats = vmalloc(sizeof(struct soc_camera_data_format *) *
> -                                   fmts);
> +       icd->user_formats =
> +               vmalloc(sizeof(struct soc_camera_format_translate *) * fmts);
> <snip>
> 
> Is that what you had in mind ?

Yes, exactly. he only thing, the name soc_camera_format_translate is too 
long... But I cannot think of a better one... maybe 
soc_camera_format_xlate just to make it a bit shorter? or format_match? 
Anyway, this will not be a reason to reject your patch:-) And I would 
prefer to have

+	x = vmalloc(y *

on one line, instead of splitting it like above. E.g., 

> +       icd->user_formats = vmalloc(fmts *
> +               sizeof(struct soc_camera_format_translate *));

but that's again just a matter of taste.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH 1/2] soc_camera: add format translation structure

[Robert Jarzmik]

Camera hosts rely on sensor formats available, as well as
host specific translations. We add a structure so that hosts
can define a translation table and use it for format check
and setup.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/soc_camera.c |   42 ++++++++++++++++++++++++++-----------
 include/media/soc_camera.h       |   23 ++++++++++++++++++--
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index f5a1e5a..c7c1ae5 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -47,6 +47,18 @@ const struct soc_camera_data_format *soc_camera_format_by_fourcc(
 }
 EXPORT_SYMBOL(soc_camera_format_by_fourcc);
 
+const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
+	struct soc_camera_device *icd, unsigned int fourcc)
+{
+	unsigned int i;
+
+	for (i = 0; i < icd->num_user_formats; i++)
+		if (icd->user_formats[i].host_fmt->fourcc == fourcc)
+			return icd->user_formats + i;
+	return NULL;
+}
+EXPORT_SYMBOL(soc_camera_xlate_by_fourcc);
+
 static int soc_camera_try_fmt_vid_cap(struct file *file, void *priv,
 				      struct v4l2_format *f)
 {
@@ -183,8 +195,8 @@ static int soc_camera_init_user_formats(struct soc_camera_device *icd)
 	if (!fmts)
 		return -ENXIO;
 
-	icd->user_formats = vmalloc(sizeof(struct soc_camera_data_format *) *
-				    fmts);
+	icd->user_formats =
+		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
 	if (!icd->user_formats)
 		return -ENOMEM;
 
@@ -195,13 +207,16 @@ static int soc_camera_init_user_formats(struct soc_camera_device *icd)
 
 	/* Second pass - actually fill data formats */
 	for (i = 0; i < icd->num_formats; i++)
-		if (!ici->ops->get_formats)
-			icd->user_formats[i] = icd->formats + i;
-		else
+		if (!ici->ops->get_formats) {
+			icd->user_formats[i].host_fmt = icd->formats + i;
+			icd->user_formats[i].cam_fmt = icd->formats + i;
+			icd->user_formats[i].buswidth = icd->formats[i].depth;
+		} else {
 			fmts += ici->ops->get_formats(icd, i,
 						      &icd->user_formats[fmts]);
+		}
 
-	icd->current_fmt = icd->user_formats[0];
+	icd->current_fmt = &icd->user_formats[0];
 
 	return 0;
 }
@@ -368,6 +383,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 	struct soc_camera_device *icd = icf->icd;
 	struct soc_camera_host *ici =
 		to_soc_camera_host(icd->dev.parent);
+	__u32 pixfmt = f->fmt.pix.pixelformat;
 	int ret;
 	struct v4l2_rect rect;
 
@@ -385,7 +401,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 	if (ret < 0) {
 		return ret;
 	} else if (!icd->current_fmt ||
-		   icd->current_fmt->fourcc != f->fmt.pix.pixelformat) {
+		   icd->current_fmt->host_fmt->fourcc != pixfmt) {
 		dev_err(&ici->dev,
 			"Host driver hasn't set up current format correctly!\n");
 		return -EINVAL;
@@ -402,7 +418,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 		icd->width, icd->height);
 
 	/* set physical bus parameters */
-	return ici->ops->set_bus_param(icd, f->fmt.pix.pixelformat);
+	return ici->ops->set_bus_param(icd, pixfmt);
 }
 
 static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
@@ -417,7 +433,7 @@ static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
 	if (f->index >= icd->num_user_formats)
 		return -EINVAL;
 
-	format = icd->user_formats[f->index];
+	format = icd->user_formats[f->index].host_fmt;
 
 	strlcpy(f->description, format->name, sizeof(f->description));
 	f->pixelformat = format->fourcc;
@@ -435,12 +451,12 @@ static int soc_camera_g_fmt_vid_cap(struct file *file, void *priv,
 	f->fmt.pix.width	= icd->width;
 	f->fmt.pix.height	= icd->height;
 	f->fmt.pix.field	= icf->vb_vidq.field;
-	f->fmt.pix.pixelformat	= icd->current_fmt->fourcc;
+	f->fmt.pix.pixelformat	= icd->current_fmt->host_fmt->fourcc;
 	f->fmt.pix.bytesperline	= f->fmt.pix.width *
-		DIV_ROUND_UP(icd->current_fmt->depth, 8);
+		DIV_ROUND_UP(icd->current_fmt->host_fmt->depth, 8);
 	f->fmt.pix.sizeimage	= f->fmt.pix.height * f->fmt.pix.bytesperline;
-	dev_dbg(&icd->dev, "current_fmt->fourcc: 0x%08x\n",
-		icd->current_fmt->fourcc);
+	dev_dbg(&icd->dev, "current_fmt->host_fmt->fourcc: 0x%08x\n",
+		icd->current_fmt->host_fmt->fourcc);
 	return 0;
 }
 
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index d6333a0..19fa2f7 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -38,10 +38,10 @@ struct soc_camera_device {
 	unsigned char buswidth;		/* See comment in .c */
 	struct soc_camera_ops *ops;
 	struct video_device *vdev;
-	const struct soc_camera_data_format *current_fmt;
+	const struct soc_camera_format_xlate *current_fmt;
 	const struct soc_camera_data_format *formats;
 	int num_formats;
-	const struct soc_camera_data_format **user_formats;
+	struct soc_camera_format_xlate *user_formats;
 	int num_user_formats;
 	struct module *owner;
 	void *host_priv;		/* per-device host private data */
@@ -70,7 +70,7 @@ struct soc_camera_host_ops {
 	int (*suspend)(struct soc_camera_device *, pm_message_t);
 	int (*resume)(struct soc_camera_device *);
 	int (*get_formats)(struct soc_camera_device *, int,
-			   const struct soc_camera_data_format **);
+			   struct soc_camera_format_xlate *);
 	int (*set_fmt)(struct soc_camera_device *, __u32, struct v4l2_rect *);
 	int (*try_fmt)(struct soc_camera_device *, struct v4l2_format *);
 	void (*init_videobuf)(struct videobuf_queue *,
@@ -111,6 +111,8 @@ extern void soc_camera_video_stop(struct soc_camera_device *icd);
 
 extern const struct soc_camera_data_format *soc_camera_format_by_fourcc(
 	struct soc_camera_device *icd, unsigned int fourcc);
+extern const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
+	struct soc_camera_device *icd, unsigned int fourcc);
 
 struct soc_camera_data_format {
 	const char *name;
@@ -119,6 +121,21 @@ struct soc_camera_data_format {
 	enum v4l2_colorspace colorspace;
 };
 
+/**
+ * struct soc_camera_format_xlate - match between host and sensor formats
+ * @cam_fmt: sensor format provided by the sensor
+ * @host_fmt: host format after host translation from cam_fmt
+ * @buswidth: bus width for this format
+ *
+ * Table of host and sensor formats matchings. A host can generate this list, in
+ * camera registation, and use it for format checks and format setup.
+ */
+struct soc_camera_format_xlate {
+	const struct soc_camera_data_format *cam_fmt;
+	const struct soc_camera_data_format *host_fmt;
+	unsigned char buswidth;
+};
+
 struct soc_camera_ops {
 	struct module *owner;
 	int (*probe)(struct soc_camera_device *);
-- 
1.5.6.5

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH 2/2] pxa_camera: use the new translation structure

[Robert Jarzmik]

The new translation structure enables to build the format
list with buswidth, depth, host format and camera format
checked, so that it's not done anymore on try_fmt nor
set_fmt.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/pxa_camera.c |  170 +++++++++++++++----------------------
 1 files changed, 69 insertions(+), 101 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 1bcdb5d..bdea201 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -156,7 +156,7 @@ static int pxa_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
 		*size += PAGE_ALIGN(icd->width * icd->height / 2); /* V pages */
 	} else {
 		*size = icd->width * icd->height *
-			((icd->current_fmt->depth + 7) >> 3);
+			((icd->current_fmt->host_fmt->depth + 7) >> 3);
 	}
 
 	if (0 == *count)
@@ -273,11 +273,11 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq,
 	 * the actual buffer is yours */
 	buf->inwork = 1;
 
-	if (buf->fmt	!= icd->current_fmt ||
+	if (buf->fmt	!= icd->current_fmt->host_fmt ||
 	    vb->width	!= icd->width ||
 	    vb->height	!= icd->height ||
 	    vb->field	!= field) {
-		buf->fmt	= icd->current_fmt;
+		buf->fmt	= icd->current_fmt->host_fmt;
 		vb->width	= icd->width;
 		vb->height	= icd->height;
 		vb->field	= field;
@@ -940,18 +940,44 @@ static bool depth_supported(struct soc_camera_device *icd, int depth)
 	return false;
 }
 
+static int required_buswidth(const struct soc_camera_data_format *fmt)
+{
+	switch (fmt->fourcc) {
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_VYUY:
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_YVYU:
+	case V4L2_PIX_FMT_RGB565:
+	case V4L2_PIX_FMT_RGB555:
+		return 8;
+	default:
+		return fmt->depth;
+	}
+}
+
 static int pxa_camera_get_formats(struct soc_camera_device *icd, int idx,
-				  const struct soc_camera_data_format **fmt)
+				  struct soc_camera_format_xlate *xlate)
 {
 	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
-	struct pxa_camera_dev *pcdev = ici->priv;
-	int formats = 0;
+	int formats = 0, buswidth, ret;
+
+	buswidth = required_buswidth(icd->formats + idx);
+
+	if (!depth_supported(icd, buswidth))
+		return 0;
+
+	ret = pxa_camera_try_bus_param(icd, buswidth);
+	if (ret < 0)
+		return 0;
 
 	switch (icd->formats[idx].fourcc) {
 	case V4L2_PIX_FMT_UYVY:
 		formats++;
-		if (fmt && (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
-			*fmt++ = &pxa_camera_formats[0];
+		if (xlate) {
+			xlate->host_fmt = &pxa_camera_formats[0];
+			xlate->cam_fmt = icd->formats + idx;
+			xlate->buswidth = buswidth;
+			xlate++;
 			dev_dbg(&ici->dev, "Providing format %s using %s\n",
 				pxa_camera_formats[0].name,
 				icd->formats[idx].name);
@@ -962,76 +988,57 @@ static int pxa_camera_get_formats(struct soc_camera_device *icd, int idx,
 	case V4L2_PIX_FMT_RGB565:
 	case V4L2_PIX_FMT_RGB555:
 		formats++;
-		if (fmt && (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
-			*fmt++ = &icd->formats[idx];
+		if (xlate) {
+			xlate->host_fmt = icd->formats + idx;
+			xlate->cam_fmt = icd->formats + idx;
+			xlate->buswidth = buswidth;
+			xlate++;
 			dev_dbg(&ici->dev, "Providing format %s packed\n",
 				icd->formats[idx].name);
 		}
 		break;
 	default:
 		/* Generic pass-through */
-		if (depth_supported(icd, icd->formats[idx].depth)) {
-			formats++;
-			if (fmt) {
-				*fmt++ = &icd->formats[idx];
-				dev_dbg(&ici->dev,
-					"Providing format %s in pass-through mode\n",
-					icd->formats[idx].name);
-			}
+		formats++;
+		if (xlate) {
+			xlate->host_fmt = icd->formats + idx;
+			xlate->cam_fmt = icd->formats + idx;
+			xlate->buswidth = icd->formats[idx].depth;
+			xlate++;
+			dev_dbg(&ici->dev,
+				"Providing format %s in pass-through mode\n",
+				icd->formats[idx].name);
 		}
 	}
 
 	return formats;
 }
 
+
 static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 			      __u32 pixfmt, struct v4l2_rect *rect)
 {
 	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
-	struct pxa_camera_dev *pcdev = ici->priv;
-	const struct soc_camera_data_format *host_fmt = NULL;
+	const struct soc_camera_data_format *host_fmt, *cam_fmt = NULL;
+	const struct soc_camera_format_xlate *xlate;
 	int ret, buswidth;
 
-	switch (pixfmt) {
-	case V4L2_PIX_FMT_YUV422P:
-		host_fmt = &pxa_camera_formats[0];
-		pixfmt = V4L2_PIX_FMT_UYVY;
-	case V4L2_PIX_FMT_UYVY:
-	case V4L2_PIX_FMT_VYUY:
-	case V4L2_PIX_FMT_YUYV:
-	case V4L2_PIX_FMT_YVYU:
-	case V4L2_PIX_FMT_RGB565:
-	case V4L2_PIX_FMT_RGB555:
-		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
-			dev_warn(&ici->dev,
-				 "8 bit bus unsupported, but required for format %x\n",
-				 pixfmt);
-			return -EINVAL;
-		}
+	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
+	if (!xlate) {
+		dev_warn(&ici->dev, "Format %x not found\n", pixfmt);
+		return -EINVAL;
+	}
 
-		if (!host_fmt)
-			host_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
+	buswidth = xlate->buswidth;
+	host_fmt = xlate->host_fmt;
+	cam_fmt = xlate->cam_fmt;
 
-		if (!host_fmt) {
-			dev_warn(&ici->dev, "Format %x not found\n", pixfmt);
-			return -EINVAL;
-		}
-		buswidth = 8;
+	switch (pixfmt) {
 	case 0:				/* Only geometry change */
 		ret = icd->ops->set_fmt(icd, pixfmt, rect);
 		break;
 	default:
-		/* Generic pass-through */
-		host_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
-		if (!host_fmt || !depth_supported(icd, host_fmt->depth)) {
-			dev_warn(&ici->dev,
-				 "Format %x unsupported in pass-through mode\n",
-				 pixfmt);
-			return -EINVAL;
-		}
-
-		buswidth = host_fmt->depth;
-		ret = icd->ops->set_fmt(icd, pixfmt, rect);
+		ret = icd->ops->set_fmt(icd, cam_fmt->fourcc, rect);
 	}
 
 	if (ret < 0)
@@ -1040,7 +1047,7 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 
 	if (pixfmt && !ret) {
 		icd->buswidth = buswidth;
-		icd->current_fmt = host_fmt;
+		icd->current_fmt = xlate;
 	}
 
 	return ret;
@@ -1050,54 +1057,14 @@ static int pxa_camera_try_fmt(struct soc_camera_device *icd,
 			      struct v4l2_format *f)
 {
 	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
-	struct pxa_camera_dev *pcdev = ici->priv;
-	const struct soc_camera_data_format *cam_fmt;
+	const struct soc_camera_format_xlate *xlate;
 	struct v4l2_pix_format *pix = &f->fmt.pix;
 	__u32 pixfmt = pix->pixelformat;
-	unsigned char buswidth;
-	int ret;
-
-	switch (pixfmt) {
-	case V4L2_PIX_FMT_YUV422P:
-		pixfmt = V4L2_PIX_FMT_UYVY;
-	case V4L2_PIX_FMT_UYVY:
-	case V4L2_PIX_FMT_VYUY:
-	case V4L2_PIX_FMT_YUYV:
-	case V4L2_PIX_FMT_YVYU:
-	case V4L2_PIX_FMT_RGB565:
-	case V4L2_PIX_FMT_RGB555:
-		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)) {
-			dev_warn(&ici->dev,
-				 "try-fmt: 8 bit bus unsupported for format %x\n",
-				 pixfmt);
-			return -EINVAL;
-		}
 
-		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
-		if (!cam_fmt) {
-			dev_warn(&ici->dev, "try-fmt: format %x not found\n",
-				 pixfmt);
-			return -EINVAL;
-		}
-		buswidth = 8;
-		break;
-	default:
-		/* Generic pass-through */
-		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
-		if (!cam_fmt || !depth_supported(icd, cam_fmt->depth)) {
-			dev_warn(&ici->dev,
-				 "try-fmt: Format %x unsupported in pass-through\n",
-				 pixfmt);
-			return -EINVAL;
-		}
-		buswidth = cam_fmt->depth;
-	}
-
-	ret = pxa_camera_try_bus_param(icd, buswidth);
-
-	if (ret < 0) {
-		dev_warn(&ici->dev, "Incompatible bus parameters!\n");
-		return ret;
+	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
+	if (!xlate) {
+		dev_warn(&ici->dev, "Format %x not found\n", pixfmt);
+		return -EINVAL;
 	}
 
 	/* limit to pxa hardware capabilities */
@@ -1111,7 +1078,8 @@ static int pxa_camera_try_fmt(struct soc_camera_device *icd,
 		pix->width = 2048;
 	pix->width &= ~0x01;
 
-	pix->bytesperline = pix->width * DIV_ROUND_UP(cam_fmt->depth, 8);
+	pix->bytesperline = pix->width *
+		DIV_ROUND_UP(xlate->host_fmt->depth, 8);
 	pix->sizeimage = pix->height * pix->bytesperline;
 
 	/* limit to sensor capabilities */
-- 
1.5.6.5

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2] pxa_camera: use the new translation structure

[Guennadi Liakhovetski]

On Mon, 24 Nov 2008, Robert Jarzmik wrote:

> The new translation structure enables to build the format
> list with buswidth, depth, host format and camera format
> checked, so that it's not done anymore on try_fmt nor
> set_fmt.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

Ok, this one looks good to me. Only two small nitpicks, the first one was 
actually my mistake in the beginning:

>  static int pxa_camera_get_formats(struct soc_camera_device *icd, int idx,
> -				  const struct soc_camera_data_format **fmt)
> +				  struct soc_camera_format_xlate *xlate)
>  {
>  	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> -	struct pxa_camera_dev *pcdev = ici->priv;
> -	int formats = 0;
> +	int formats = 0, buswidth, ret;
> +
> +	buswidth = required_buswidth(icd->formats + idx);
> +
> +	if (!depth_supported(icd, buswidth))

I think, this function would be better named buswicth_supported().

>  		}
>  	}
>  
>  	return formats;
>  }
>  
> +
>  static int pxa_camera_set_fmt(struct soc_camera_device *icd,
>  			      __u32 pixfmt, struct v4l2_rect *rect)
>  {
>  	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);

Let's stay at one blank line between functions:-)


So, just please revert the current_fmt change and submit these two patches 
integrating mine into them.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2] pxa_camera: use the new translation structure

[Guennadi Liakhovetski]

On Fri, 28 Nov 2008, Guennadi Liakhovetski wrote:

> On Mon, 24 Nov 2008, Robert Jarzmik wrote:
> 
> > +	if (!depth_supported(icd, buswidth))
> 
> I think, this function would be better named buswicth_supported().

...even better buswidth_supported:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 2/2] pxa_camera: use the new translation structure

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>> I think, this function would be better named buswicth_supported().
>
> ...even better buswidth_supported:-)
Yep. Tomorrow, with your comments on soc_camera, I'll repost the 2 patches.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 1/2] soc_camera: add format translation structure

[Guennadi Liakhovetski]

On Mon, 24 Nov 2008, Robert Jarzmik wrote:

> Camera hosts rely on sensor formats available, as well as
> host specific translations. We add a structure so that hosts
> can define a translation table and use it for format check
> and setup.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/video/soc_camera.c |   42 ++++++++++++++++++++++++++-----------
>  include/media/soc_camera.h       |   23 ++++++++++++++++++--
>  2 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
> index f5a1e5a..c7c1ae5 100644
> --- a/drivers/media/video/soc_camera.c
> +++ b/drivers/media/video/soc_camera.c
> @@ -47,6 +47,18 @@ const struct soc_camera_data_format *soc_camera_format_by_fourcc(
>  }
>  EXPORT_SYMBOL(soc_camera_format_by_fourcc);
>  
> +const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
> +	struct soc_camera_device *icd, unsigned int fourcc)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < icd->num_user_formats; i++)
> +		if (icd->user_formats[i].host_fmt->fourcc == fourcc)
> +			return icd->user_formats + i;
> +	return NULL;
> +}
> +EXPORT_SYMBOL(soc_camera_xlate_by_fourcc);
> +
>  static int soc_camera_try_fmt_vid_cap(struct file *file, void *priv,
>  				      struct v4l2_format *f)
>  {
> @@ -183,8 +195,8 @@ static int soc_camera_init_user_formats(struct soc_camera_device *icd)
>  	if (!fmts)
>  		return -ENXIO;
>  
> -	icd->user_formats = vmalloc(sizeof(struct soc_camera_data_format *) *
> -				    fmts);
> +	icd->user_formats =
> +		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
>  	if (!icd->user_formats)
>  		return -ENOMEM;
>  
> @@ -195,13 +207,16 @@ static int soc_camera_init_user_formats(struct soc_camera_device *icd)
>  
>  	/* Second pass - actually fill data formats */
>  	for (i = 0; i < icd->num_formats; i++)
> -		if (!ici->ops->get_formats)
> -			icd->user_formats[i] = icd->formats + i;
> -		else
> +		if (!ici->ops->get_formats) {
> +			icd->user_formats[i].host_fmt = icd->formats + i;
> +			icd->user_formats[i].cam_fmt = icd->formats + i;
> +			icd->user_formats[i].buswidth = icd->formats[i].depth;
> +		} else {
>  			fmts += ici->ops->get_formats(icd, i,
>  						      &icd->user_formats[fmts]);
> +		}
>  
> -	icd->current_fmt = icd->user_formats[0];
> +	icd->current_fmt = &icd->user_formats[0];

Well, no. You cannot do this - not in this patch. In general, I guess, you 
want current_fmt to point to the xlate object for debugging, etc. But this 
has to be a separate patch, changing the define in the header, 
soc_camera.c and _all_ host-drivers, including SuperH, which you left 
broken with your two patches. So, please leave current_fmt at its old 
meaning for these two patches. We can convert it later - if we really want 
to.

>  
>  	return 0;
>  }
> @@ -368,6 +383,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
>  	struct soc_camera_device *icd = icf->icd;
>  	struct soc_camera_host *ici =
>  		to_soc_camera_host(icd->dev.parent);
> +	__u32 pixfmt = f->fmt.pix.pixelformat;
>  	int ret;
>  	struct v4l2_rect rect;
>  
> @@ -385,7 +401,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
>  	if (ret < 0) {
>  		return ret;
>  	} else if (!icd->current_fmt ||
> -		   icd->current_fmt->fourcc != f->fmt.pix.pixelformat) {
> +		   icd->current_fmt->host_fmt->fourcc != pixfmt) {
>  		dev_err(&ici->dev,
>  			"Host driver hasn't set up current format correctly!\n");
>  		return -EINVAL;
> @@ -402,7 +418,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
>  		icd->width, icd->height);
>  
>  	/* set physical bus parameters */
> -	return ici->ops->set_bus_param(icd, f->fmt.pix.pixelformat);
> +	return ici->ops->set_bus_param(icd, pixfmt);
>  }
>  
>  static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
> @@ -417,7 +433,7 @@ static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
>  	if (f->index >= icd->num_user_formats)
>  		return -EINVAL;
>  
> -	format = icd->user_formats[f->index];
> +	format = icd->user_formats[f->index].host_fmt;
>  
>  	strlcpy(f->description, format->name, sizeof(f->description));
>  	f->pixelformat = format->fourcc;
> @@ -435,12 +451,12 @@ static int soc_camera_g_fmt_vid_cap(struct file *file, void *priv,
>  	f->fmt.pix.width	= icd->width;
>  	f->fmt.pix.height	= icd->height;
>  	f->fmt.pix.field	= icf->vb_vidq.field;
> -	f->fmt.pix.pixelformat	= icd->current_fmt->fourcc;
> +	f->fmt.pix.pixelformat	= icd->current_fmt->host_fmt->fourcc;
>  	f->fmt.pix.bytesperline	= f->fmt.pix.width *
> -		DIV_ROUND_UP(icd->current_fmt->depth, 8);
> +		DIV_ROUND_UP(icd->current_fmt->host_fmt->depth, 8);
>  	f->fmt.pix.sizeimage	= f->fmt.pix.height * f->fmt.pix.bytesperline;
> -	dev_dbg(&icd->dev, "current_fmt->fourcc: 0x%08x\n",
> -		icd->current_fmt->fourcc);
> +	dev_dbg(&icd->dev, "current_fmt->host_fmt->fourcc: 0x%08x\n",
> +		icd->current_fmt->host_fmt->fourcc);
>  	return 0;
>  }
>  
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index d6333a0..19fa2f7 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -38,10 +38,10 @@ struct soc_camera_device {
>  	unsigned char buswidth;		/* See comment in .c */
>  	struct soc_camera_ops *ops;
>  	struct video_device *vdev;
> -	const struct soc_camera_data_format *current_fmt;
> +	const struct soc_camera_format_xlate *current_fmt;
>  	const struct soc_camera_data_format *formats;
>  	int num_formats;
> -	const struct soc_camera_data_format **user_formats;
> +	struct soc_camera_format_xlate *user_formats;
>  	int num_user_formats;
>  	struct module *owner;
>  	void *host_priv;		/* per-device host private data */
> @@ -70,7 +70,7 @@ struct soc_camera_host_ops {
>  	int (*suspend)(struct soc_camera_device *, pm_message_t);
>  	int (*resume)(struct soc_camera_device *);
>  	int (*get_formats)(struct soc_camera_device *, int,
> -			   const struct soc_camera_data_format **);
> +			   struct soc_camera_format_xlate *);
>  	int (*set_fmt)(struct soc_camera_device *, __u32, struct v4l2_rect *);
>  	int (*try_fmt)(struct soc_camera_device *, struct v4l2_format *);
>  	void (*init_videobuf)(struct videobuf_queue *,
> @@ -111,6 +111,8 @@ extern void soc_camera_video_stop(struct soc_camera_device *icd);
>  
>  extern const struct soc_camera_data_format *soc_camera_format_by_fourcc(
>  	struct soc_camera_device *icd, unsigned int fourcc);
> +extern const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
> +	struct soc_camera_device *icd, unsigned int fourcc);
>  
>  struct soc_camera_data_format {
>  	const char *name;
> @@ -119,6 +121,21 @@ struct soc_camera_data_format {
>  	enum v4l2_colorspace colorspace;
>  };
>  
> +/**
> + * struct soc_camera_format_xlate - match between host and sensor formats
> + * @cam_fmt: sensor format provided by the sensor
> + * @host_fmt: host format after host translation from cam_fmt
> + * @buswidth: bus width for this format
> + *
> + * Table of host and sensor formats matchings. A host can generate this list, in
> + * camera registation, and use it for format checks and format setup.
> + */

This comment doesn't look quite right - this is not a table, this is just 
one element thereof. And "host can generate this list" is also not quite 
precise - the list is generated by the soc_camera.c, the host can override 
the default one-to-one mapping.

> +struct soc_camera_format_xlate {
> +	const struct soc_camera_data_format *cam_fmt;
> +	const struct soc_camera_data_format *host_fmt;
> +	unsigned char buswidth;
> +};
> +
>  struct soc_camera_ops {
>  	struct module *owner;
>  	int (*probe)(struct soc_camera_device *);
> -- 
> 1.5.6.5

Otherwise looks ok. I would suggest you remove the current_fmt change, fix 
the comment and submit integrated into my previous patch - not as 
incremental.

A review to the pxa-patch will follow later...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 1/2] soc_camera: add format translation structure

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Well, no. You cannot do this - not in this patch. In general, I guess, you 
> want current_fmt to point to the xlate object for debugging, etc. But this 
> has to be a separate patch, changing the define in the header, 
> soc_camera.c and _all_ host-drivers, including SuperH, which you left 
> broken with your two patches. So, please leave current_fmt at its old 
> meaning for these two patches. We can convert it later - if we really want 
> to.
As you wish. I reverted that.

> This comment doesn't look quite right - this is not a table, this is just 
> one element thereof. And "host can generate this list" is also not quite 
> precise - the list is generated by the soc_camera.c, the host can override 
> the default one-to-one mapping.
Right.

>
> Otherwise looks ok. I would suggest you remove the current_fmt change, fix 
> the comment and submit integrated into my previous patch - not as 
> incremental.
>
> A review to the pxa-patch will follow later...
All right. I'm waiting for the second review to post both patches
amended. Meanwhile, I'll always keep the 2 patches state here :
http://belgarath.falguerolles.org/download/for_guennadi/

This is the place where the 2 patches are, in their newest form (the merged
version from you and me). I took the liberty to cosign the patches, up to you to
remove either mine, or yours, or rewrite a part of the patch, to your will.

Cheers.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 1/2] soc_camera: add format translation structure

[Guennadi Liakhovetski]

On Thu, 27 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Well, no. You cannot do this - not in this patch. In general, I guess, you 
> > want current_fmt to point to the xlate object for debugging, etc. But this 
> > has to be a separate patch, changing the define in the header, 
> > soc_camera.c and _all_ host-drivers, including SuperH, which you left 
> > broken with your two patches. So, please leave current_fmt at its old 
> > meaning for these two patches. We can convert it later - if we really want 
> > to.
> As you wish. I reverted that.

No, this is not just my wish, this is a requirement - we should not break 
compilation. If we change API we can either

1. change API and all users in one patch

or

2. add new API, in following patches slowly convert existing users, at 
last remove the old API.

> > A review to the pxa-patch will follow later...
> All right. I'm waiting for the second review to post both patches
> amended. Meanwhile, I'll always keep the 2 patches state here :
> http://belgarath.falguerolles.org/download/for_guennadi/
> 
> This is the place where the 2 patches are, in their newest form (the merged
> version from you and me). I took the liberty to cosign the patches, up to you to
> remove either mine, or yours, or rewrite a part of the patch, to your will.

In your 0001-soc-camera-pixel-format-negotiation-core-support.patch patch 
there one place doesn't look right:

+	icd->current_fmt = &icd->formats[0];

I think, we really want

+	icd->current_fmt = icd->user_formats[0].host_fmt;

icd->formats[0] might not be supported at all. As for Sobs - I anyway have 
to add mine as I pull them via the soc-camera tree. And as you submit them 
you'll use your "From:" line, right?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v4 1/2] soc-camera: pixel format negotiation - core support

[Guennadi Liakhovetski]

Hi Robert,

this is what I have written:

> +	icd->current_fmt = icd->user_formats[0].host_fmt;

and this is what you've written:

On Sat, 29 Nov 2008, Robert Jarzmik wrote:

> +	icd->current_fmt = &icd->user_formats[0].host_fmt;

I think, this is different. Further I think your version would produce a 
compiler warning like this:

drivers/media/video/soc_camera.c: In function 'soc_camera_init_user_formats':
drivers/media/video/soc_camera.c:219: warning: assignment from incompatible pointer type

and is indeed wrong. Please fix and please test your patches before 
submitting - compile and run. Or am I wrong?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v4 1/2] soc-camera: pixel format negotiation - core support

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> I think, this is different. Further I think your version would produce a 
> compiler warning like this:
>
> drivers/media/video/soc_camera.c: In function 'soc_camera_init_user_formats':
> drivers/media/video/soc_camera.c:219: warning: assignment from incompatible pointer type
True. Didn't see it, sorry.

> and is indeed wrong. Please fix and please test your patches before 
> submitting - compile and run. Or am I wrong?
I _did_ test my patch. Even with the error, the capture_example from v4l tree
didn't trigger a bug.

--
Robert

>From f34e1b0354a8f176e235d6a8aca4bf0a0efcab2c Mon Sep 17 00:00:00 2001
From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Date: Tue, 18 Nov 2008 20:25:48 +0100
Subject: [PATCH v4 1/2] soc-camera: pixel format negotiation - core support

Allocate and fill a list of formats, supported by this specific
camera-host combination. Use it for format enumeration. Take care to stay
backwards-compatible.

Camera hosts rely on sensor formats available, as well as
host specific translations. We add a structure so that hosts
can define a translation table and use it for format check
and setup.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/soc_camera.c |   93 +++++++++++++++++++++++++++++++++-----
 include/media/soc_camera.h       |   25 ++++++++++-
 2 files changed, 105 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 9db66a4..5e48c2c 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -47,6 +47,18 @@ const struct soc_camera_data_format *soc_camera_format_by_fourcc(
 }
 EXPORT_SYMBOL(soc_camera_format_by_fourcc);
 
+const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
+	struct soc_camera_device *icd, unsigned int fourcc)
+{
+	unsigned int i;
+
+	for (i = 0; i < icd->num_user_formats; i++)
+		if (icd->user_formats[i].host_fmt->fourcc == fourcc)
+			return icd->user_formats + i;
+	return NULL;
+}
+EXPORT_SYMBOL(soc_camera_xlate_by_fourcc);
+
 static int soc_camera_try_fmt_vid_cap(struct file *file, void *priv,
 				      struct v4l2_format *f)
 {
@@ -161,6 +173,59 @@ static int soc_camera_dqbuf(struct file *file, void *priv,
 	return videobuf_dqbuf(&icf->vb_vidq, p, file->f_flags & O_NONBLOCK);
 }
 
+static int soc_camera_init_user_formats(struct soc_camera_device *icd)
+{
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	int i, fmts = 0;
+
+	if (!ici->ops->get_formats)
+		/*
+		 * Fallback mode - the host will have to serve all
+		 * sensor-provided formats one-to-one to the user
+		 */
+		fmts = icd->num_formats;
+	else
+		/*
+		 * First pass - only count formats this host-sensor
+		 * configuration can provide
+		 */
+		for (i = 0; i < icd->num_formats; i++)
+			fmts += ici->ops->get_formats(icd, i, NULL);
+
+	if (!fmts)
+		return -ENXIO;
+
+	icd->user_formats =
+		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
+	if (!icd->user_formats)
+		return -ENOMEM;
+
+	icd->num_user_formats = fmts;
+	fmts = 0;
+
+	dev_dbg(&icd->dev, "Found %d supported formats.\n", fmts);
+
+	/* Second pass - actually fill data formats */
+	for (i = 0; i < icd->num_formats; i++)
+		if (!ici->ops->get_formats) {
+			icd->user_formats[i].host_fmt = icd->formats + i;
+			icd->user_formats[i].cam_fmt = icd->formats + i;
+			icd->user_formats[i].buswidth = icd->formats[i].depth;
+		} else {
+			fmts += ici->ops->get_formats(icd, i,
+						      &icd->user_formats[fmts]);
+		}
+
+	icd->current_fmt = icd->user_formats[0].host_fmt;
+
+	return 0;
+}
+
+static void soc_camera_free_user_formats(struct soc_camera_device *icd)
+{
+	vfree(icd->user_formats);
+}
+
 static int soc_camera_open(struct inode *inode, struct file *file)
 {
 	struct video_device *vdev;
@@ -197,10 +262,12 @@ static int soc_camera_open(struct inode *inode, struct file *file)
 
 	/* Now we really have to activate the camera */
 	if (icd->use_count == 1) {
+		ret = soc_camera_init_user_formats(icd);
+		if (ret < 0)
+			goto eiufmt;
 		ret = ici->ops->add(icd);
 		if (ret < 0) {
 			dev_err(&icd->dev, "Couldn't activate the camera: %d\n", ret);
-			icd->use_count--;
 			goto eiciadd;
 		}
 	}
@@ -216,6 +283,9 @@ static int soc_camera_open(struct inode *inode, struct file *file)
 
 	/* All errors are entered with the video_lock held */
 eiciadd:
+	soc_camera_free_user_formats(icd);
+eiufmt:
+	icd->use_count--;
 	module_put(ici->ops->owner);
 emgi:
 	module_put(icd->ops->owner);
@@ -234,8 +304,10 @@ static int soc_camera_close(struct inode *inode, struct file *file)
 
 	mutex_lock(&video_lock);
 	icd->use_count--;
-	if (!icd->use_count)
+	if (!icd->use_count) {
 		ici->ops->remove(icd);
+		soc_camera_free_user_formats(icd);
+	}
 	module_put(icd->ops->owner);
 	module_put(ici->ops->owner);
 	mutex_unlock(&video_lock);
@@ -311,6 +383,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 	struct soc_camera_device *icd = icf->icd;
 	struct soc_camera_host *ici =
 		to_soc_camera_host(icd->dev.parent);
+	__u32 pixfmt = f->fmt.pix.pixelformat;
 	int ret;
 	struct v4l2_rect rect;
 
@@ -328,14 +401,12 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 	if (ret < 0) {
 		return ret;
 	} else if (!icd->current_fmt ||
-		   icd->current_fmt->fourcc != f->fmt.pix.pixelformat) {
-		dev_err(&ici->dev, "Host driver hasn't set up current "
-			"format correctly!\n");
+		   icd->current_fmt->fourcc != pixfmt) {
+		dev_err(&ici->dev,
+			"Host driver hasn't set up current format correctly!\n");
 		return -EINVAL;
 	}
 
-	/* buswidth may be further adjusted by the ici */
-	icd->buswidth		= icd->current_fmt->depth;
 	icd->width		= rect.width;
 	icd->height		= rect.height;
 	icf->vb_vidq.field	= f->fmt.pix.field;
@@ -347,7 +418,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 		icd->width, icd->height);
 
 	/* set physical bus parameters */
-	return ici->ops->set_bus_param(icd, f->fmt.pix.pixelformat);
+	return ici->ops->set_bus_param(icd, pixfmt);
 }
 
 static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
@@ -359,10 +430,10 @@ static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
 
 	WARN_ON(priv != file->private_data);
 
-	if (f->index >= icd->num_formats)
+	if (f->index >= icd->num_user_formats)
 		return -EINVAL;
 
-	format = &icd->formats[f->index];
+	format = icd->user_formats[f->index].host_fmt;
 
 	strlcpy(f->description, format->name, sizeof(f->description));
 	f->pixelformat = format->fourcc;
@@ -919,8 +990,6 @@ int soc_camera_video_start(struct soc_camera_device *icd)
 	vdev->minor		= -1;
 	vdev->tvnorms		= V4L2_STD_UNKNOWN,
 
-	icd->current_fmt = &icd->formats[0];
-
 	err = video_register_device(vdev, VFL_TYPE_GRABBER, vdev->minor);
 	if (err < 0) {
 		dev_err(vdev->parent, "video_register_device failed\n");
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index dddaf45..da57ffd 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -41,6 +41,8 @@ struct soc_camera_device {
 	const struct soc_camera_data_format *current_fmt;
 	const struct soc_camera_data_format *formats;
 	int num_formats;
+	struct soc_camera_format_xlate *user_formats;
+	int num_user_formats;
 	struct module *owner;
 	void *host_priv;		/* per-device host private data */
 	/* soc_camera.c private count. Only accessed with video_lock held */
@@ -65,8 +67,10 @@ struct soc_camera_host_ops {
 	struct module *owner;
 	int (*add)(struct soc_camera_device *);
 	void (*remove)(struct soc_camera_device *);
-	int (*suspend)(struct soc_camera_device *, pm_message_t state);
+	int (*suspend)(struct soc_camera_device *, pm_message_t);
 	int (*resume)(struct soc_camera_device *);
+	int (*get_formats)(struct soc_camera_device *, int,
+			   struct soc_camera_format_xlate *);
 	int (*set_fmt)(struct soc_camera_device *, __u32, struct v4l2_rect *);
 	int (*try_fmt)(struct soc_camera_device *, struct v4l2_format *);
 	void (*init_videobuf)(struct videobuf_queue *,
@@ -107,6 +111,8 @@ extern void soc_camera_video_stop(struct soc_camera_device *icd);
 
 extern const struct soc_camera_data_format *soc_camera_format_by_fourcc(
 	struct soc_camera_device *icd, unsigned int fourcc);
+extern const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
+	struct soc_camera_device *icd, unsigned int fourcc);
 
 struct soc_camera_data_format {
 	const char *name;
@@ -115,6 +121,23 @@ struct soc_camera_data_format {
 	enum v4l2_colorspace colorspace;
 };
 
+/**
+ * struct soc_camera_format_xlate - match between host and sensor formats
+ * @cam_fmt: sensor format provided by the sensor
+ * @host_fmt: host format after host translation from cam_fmt
+ * @buswidth: bus width for this format
+ *
+ * Host and sensor translation structure. Used in table of host and sensor
+ * formats matchings in soc_camera_device. A host can override the generic list
+ * generation by implementing get_formats(), and use it for format checks and
+ * format setup.
+ */
+struct soc_camera_format_xlate {
+	const struct soc_camera_data_format *cam_fmt;
+	const struct soc_camera_data_format *host_fmt;
+	unsigned char buswidth;
+};
+
 struct soc_camera_ops {
 	struct module *owner;
 	int (*probe)(struct soc_camera_device *);
-- 
1.5.6.5

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH v4 1/2] soc-camera: pixel format negotiation - core support

[Robert Jarzmik]

From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Allocate and fill a list of formats, supported by this specific
camera-host combination. Use it for format enumeration. Take care to stay
backwards-compatible.

Camera hosts rely on sensor formats available, as well as
host specific translations. We add a structure so that hosts
can define a translation table and use it for format check
and setup.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/soc_camera.c |   93 +++++++++++++++++++++++++++++++++-----
 include/media/soc_camera.h       |   25 ++++++++++-
 2 files changed, 105 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 9db66a4..aa604ef 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -47,6 +47,18 @@ const struct soc_camera_data_format *soc_camera_format_by_fourcc(
 }
 EXPORT_SYMBOL(soc_camera_format_by_fourcc);
 
+const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
+	struct soc_camera_device *icd, unsigned int fourcc)
+{
+	unsigned int i;
+
+	for (i = 0; i < icd->num_user_formats; i++)
+		if (icd->user_formats[i].host_fmt->fourcc == fourcc)
+			return icd->user_formats + i;
+	return NULL;
+}
+EXPORT_SYMBOL(soc_camera_xlate_by_fourcc);
+
 static int soc_camera_try_fmt_vid_cap(struct file *file, void *priv,
 				      struct v4l2_format *f)
 {
@@ -161,6 +173,59 @@ static int soc_camera_dqbuf(struct file *file, void *priv,
 	return videobuf_dqbuf(&icf->vb_vidq, p, file->f_flags & O_NONBLOCK);
 }
 
+static int soc_camera_init_user_formats(struct soc_camera_device *icd)
+{
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	int i, fmts = 0;
+
+	if (!ici->ops->get_formats)
+		/*
+		 * Fallback mode - the host will have to serve all
+		 * sensor-provided formats one-to-one to the user
+		 */
+		fmts = icd->num_formats;
+	else
+		/*
+		 * First pass - only count formats this host-sensor
+		 * configuration can provide
+		 */
+		for (i = 0; i < icd->num_formats; i++)
+			fmts += ici->ops->get_formats(icd, i, NULL);
+
+	if (!fmts)
+		return -ENXIO;
+
+	icd->user_formats =
+		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
+	if (!icd->user_formats)
+		return -ENOMEM;
+
+	icd->num_user_formats = fmts;
+	fmts = 0;
+
+	dev_dbg(&icd->dev, "Found %d supported formats.\n", fmts);
+
+	/* Second pass - actually fill data formats */
+	for (i = 0; i < icd->num_formats; i++)
+		if (!ici->ops->get_formats) {
+			icd->user_formats[i].host_fmt = icd->formats + i;
+			icd->user_formats[i].cam_fmt = icd->formats + i;
+			icd->user_formats[i].buswidth = icd->formats[i].depth;
+		} else {
+			fmts += ici->ops->get_formats(icd, i,
+						      &icd->user_formats[fmts]);
+		}
+
+	icd->current_fmt = &icd->user_formats[0].host_fmt;
+
+	return 0;
+}
+
+static void soc_camera_free_user_formats(struct soc_camera_device *icd)
+{
+	vfree(icd->user_formats);
+}
+
 static int soc_camera_open(struct inode *inode, struct file *file)
 {
 	struct video_device *vdev;
@@ -197,10 +262,12 @@ static int soc_camera_open(struct inode *inode, struct file *file)
 
 	/* Now we really have to activate the camera */
 	if (icd->use_count == 1) {
+		ret = soc_camera_init_user_formats(icd);
+		if (ret < 0)
+			goto eiufmt;
 		ret = ici->ops->add(icd);
 		if (ret < 0) {
 			dev_err(&icd->dev, "Couldn't activate the camera: %d\n", ret);
-			icd->use_count--;
 			goto eiciadd;
 		}
 	}
@@ -216,6 +283,9 @@ static int soc_camera_open(struct inode *inode, struct file *file)
 
 	/* All errors are entered with the video_lock held */
 eiciadd:
+	soc_camera_free_user_formats(icd);
+eiufmt:
+	icd->use_count--;
 	module_put(ici->ops->owner);
 emgi:
 	module_put(icd->ops->owner);
@@ -234,8 +304,10 @@ static int soc_camera_close(struct inode *inode, struct file *file)
 
 	mutex_lock(&video_lock);
 	icd->use_count--;
-	if (!icd->use_count)
+	if (!icd->use_count) {
 		ici->ops->remove(icd);
+		soc_camera_free_user_formats(icd);
+	}
 	module_put(icd->ops->owner);
 	module_put(ici->ops->owner);
 	mutex_unlock(&video_lock);
@@ -311,6 +383,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 	struct soc_camera_device *icd = icf->icd;
 	struct soc_camera_host *ici =
 		to_soc_camera_host(icd->dev.parent);
+	__u32 pixfmt = f->fmt.pix.pixelformat;
 	int ret;
 	struct v4l2_rect rect;
 
@@ -328,14 +401,12 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 	if (ret < 0) {
 		return ret;
 	} else if (!icd->current_fmt ||
-		   icd->current_fmt->fourcc != f->fmt.pix.pixelformat) {
-		dev_err(&ici->dev, "Host driver hasn't set up current "
-			"format correctly!\n");
+		   icd->current_fmt->fourcc != pixfmt) {
+		dev_err(&ici->dev,
+			"Host driver hasn't set up current format correctly!\n");
 		return -EINVAL;
 	}
 
-	/* buswidth may be further adjusted by the ici */
-	icd->buswidth		= icd->current_fmt->depth;
 	icd->width		= rect.width;
 	icd->height		= rect.height;
 	icf->vb_vidq.field	= f->fmt.pix.field;
@@ -347,7 +418,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 		icd->width, icd->height);
 
 	/* set physical bus parameters */
-	return ici->ops->set_bus_param(icd, f->fmt.pix.pixelformat);
+	return ici->ops->set_bus_param(icd, pixfmt);
 }
 
 static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
@@ -359,10 +430,10 @@ static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
 
 	WARN_ON(priv != file->private_data);
 
-	if (f->index >= icd->num_formats)
+	if (f->index >= icd->num_user_formats)
 		return -EINVAL;
 
-	format = &icd->formats[f->index];
+	format = icd->user_formats[f->index].host_fmt;
 
 	strlcpy(f->description, format->name, sizeof(f->description));
 	f->pixelformat = format->fourcc;
@@ -919,8 +990,6 @@ int soc_camera_video_start(struct soc_camera_device *icd)
 	vdev->minor		= -1;
 	vdev->tvnorms		= V4L2_STD_UNKNOWN,
 
-	icd->current_fmt = &icd->formats[0];
-
 	err = video_register_device(vdev, VFL_TYPE_GRABBER, vdev->minor);
 	if (err < 0) {
 		dev_err(vdev->parent, "video_register_device failed\n");
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index dddaf45..da57ffd 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -41,6 +41,8 @@ struct soc_camera_device {
 	const struct soc_camera_data_format *current_fmt;
 	const struct soc_camera_data_format *formats;
 	int num_formats;
+	struct soc_camera_format_xlate *user_formats;
+	int num_user_formats;
 	struct module *owner;
 	void *host_priv;		/* per-device host private data */
 	/* soc_camera.c private count. Only accessed with video_lock held */
@@ -65,8 +67,10 @@ struct soc_camera_host_ops {
 	struct module *owner;
 	int (*add)(struct soc_camera_device *);
 	void (*remove)(struct soc_camera_device *);
-	int (*suspend)(struct soc_camera_device *, pm_message_t state);
+	int (*suspend)(struct soc_camera_device *, pm_message_t);
 	int (*resume)(struct soc_camera_device *);
+	int (*get_formats)(struct soc_camera_device *, int,
+			   struct soc_camera_format_xlate *);
 	int (*set_fmt)(struct soc_camera_device *, __u32, struct v4l2_rect *);
 	int (*try_fmt)(struct soc_camera_device *, struct v4l2_format *);
 	void (*init_videobuf)(struct videobuf_queue *,
@@ -107,6 +111,8 @@ extern void soc_camera_video_stop(struct soc_camera_device *icd);
 
 extern const struct soc_camera_data_format *soc_camera_format_by_fourcc(
 	struct soc_camera_device *icd, unsigned int fourcc);
+extern const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
+	struct soc_camera_device *icd, unsigned int fourcc);
 
 struct soc_camera_data_format {
 	const char *name;
@@ -115,6 +121,23 @@ struct soc_camera_data_format {
 	enum v4l2_colorspace colorspace;
 };
 
+/**
+ * struct soc_camera_format_xlate - match between host and sensor formats
+ * @cam_fmt: sensor format provided by the sensor
+ * @host_fmt: host format after host translation from cam_fmt
+ * @buswidth: bus width for this format
+ *
+ * Host and sensor translation structure. Used in table of host and sensor
+ * formats matchings in soc_camera_device. A host can override the generic list
+ * generation by implementing get_formats(), and use it for format checks and
+ * format setup.
+ */
+struct soc_camera_format_xlate {
+	const struct soc_camera_data_format *cam_fmt;
+	const struct soc_camera_data_format *host_fmt;
+	unsigned char buswidth;
+};
+
 struct soc_camera_ops {
 	struct module *owner;
 	int (*probe)(struct soc_camera_device *);
-- 
1.5.6.5

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH v4 2/2] pxa-camera: pixel format negotiation

[Robert Jarzmik]

From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Use the new format-negotiation infrastructure, support all four YUV422
packed and the planar formats.

The new translation structure enables to build the format
list with buswidth, depth, host format and camera format
checked, so that it's not done anymore on try_fmt nor
set_fmt.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/pxa_camera.c |  207 ++++++++++++++++++++++++++++++--------
 1 files changed, 165 insertions(+), 42 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 37afdfa..8219a6c 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -765,6 +765,9 @@ static int test_platform_param(struct pxa_camera_dev *pcdev,
 		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8))
 			return -EINVAL;
 		*flags |= SOCAM_DATAWIDTH_8;
+		break;
+	default:
+		return -EINVAL;
 	}
 
 	return 0;
@@ -823,12 +826,10 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	 * We fix bit-per-pixel equal to data-width... */
 	switch (common_flags & SOCAM_DATAWIDTH_MASK) {
 	case SOCAM_DATAWIDTH_10:
-		icd->buswidth = 10;
 		dw = 4;
 		bpp = 0x40;
 		break;
 	case SOCAM_DATAWIDTH_9:
-		icd->buswidth = 9;
 		dw = 3;
 		bpp = 0x20;
 		break;
@@ -836,7 +837,6 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 		/* Actually it can only be 8 now,
 		 * default is just to silence compiler warnings */
 	case SOCAM_DATAWIDTH_8:
-		icd->buswidth = 8;
 		dw = 2;
 		bpp = 0;
 	}
@@ -862,7 +862,17 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	case V4L2_PIX_FMT_YUV422P:
 		pcdev->channels = 3;
 		cicr1 |= CICR1_YCBCR_F;
+		/*
+		 * Normally, pxa bus wants as input UYVY format. We allow all
+		 * reorderings of the YUV422 format, as no processing is done,
+		 * and the YUV stream is just passed through without any
+		 * transformation. Note that UYVY is the only format that
+		 * should be used if pxa framebuffer Overlay2 is used.
+		 */
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_VYUY:
 	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_YVYU:
 		cicr1 |= CICR1_COLOR_SP_VAL(2);
 		break;
 	case V4L2_PIX_FMT_RGB555:
@@ -888,13 +898,14 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	return 0;
 }
 
-static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
+static int pxa_camera_try_bus_param(struct soc_camera_device *icd,
+				    unsigned char buswidth)
 {
 	struct soc_camera_host *ici =
 		to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	unsigned long bus_flags, camera_flags;
-	int ret = test_platform_param(pcdev, icd->buswidth, &bus_flags);
+	int ret = test_platform_param(pcdev, buswidth, &bus_flags);
 
 	if (ret < 0)
 		return ret;
@@ -904,25 +915,139 @@ static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	return soc_camera_bus_param_compatible(camera_flags, bus_flags) ? 0 : -EINVAL;
 }
 
+static const struct soc_camera_data_format pxa_camera_formats[] = {
+	{
+		.name		= "Planar YUV422 16 bit",
+		.depth		= 16,
+		.fourcc		= V4L2_PIX_FMT_YUV422P,
+		.colorspace	= V4L2_COLORSPACE_JPEG,
+	},
+};
+
+static bool buswidth_supported(struct soc_camera_device *icd, int depth)
+{
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
+
+	switch (depth) {
+	case 8:
+		return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8);
+	case 9:
+		return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_9);
+	case 10:
+		return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_10);
+	}
+	return false;
+}
+
+static int required_buswidth(const struct soc_camera_data_format *fmt)
+{
+	switch (fmt->fourcc) {
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_VYUY:
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_YVYU:
+	case V4L2_PIX_FMT_RGB565:
+	case V4L2_PIX_FMT_RGB555:
+		return 8;
+	default:
+		return fmt->depth;
+	}
+}
+
+static int pxa_camera_get_formats(struct soc_camera_device *icd, int idx,
+				  struct soc_camera_format_xlate *xlate)
+{
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	int formats = 0, buswidth, ret;
+
+	buswidth = required_buswidth(icd->formats + idx);
+
+	if (!buswidth_supported(icd, buswidth))
+		return 0;
+
+	ret = pxa_camera_try_bus_param(icd, buswidth);
+	if (ret < 0)
+		return 0;
+
+	switch (icd->formats[idx].fourcc) {
+	case V4L2_PIX_FMT_UYVY:
+		formats++;
+		if (xlate) {
+			xlate->host_fmt = &pxa_camera_formats[0];
+			xlate->cam_fmt = icd->formats + idx;
+			xlate->buswidth = buswidth;
+			xlate++;
+			dev_dbg(&ici->dev, "Providing format %s using %s\n",
+				pxa_camera_formats[0].name,
+				icd->formats[idx].name);
+		}
+	case V4L2_PIX_FMT_VYUY:
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_YVYU:
+	case V4L2_PIX_FMT_RGB565:
+	case V4L2_PIX_FMT_RGB555:
+		formats++;
+		if (xlate) {
+			xlate->host_fmt = icd->formats + idx;
+			xlate->cam_fmt = icd->formats + idx;
+			xlate->buswidth = buswidth;
+			xlate++;
+			dev_dbg(&ici->dev, "Providing format %s packed\n",
+				icd->formats[idx].name);
+		}
+		break;
+	default:
+		/* Generic pass-through */
+		formats++;
+		if (xlate) {
+			xlate->host_fmt = icd->formats + idx;
+			xlate->cam_fmt = icd->formats + idx;
+			xlate->buswidth = icd->formats[idx].depth;
+			xlate++;
+			dev_dbg(&ici->dev,
+				"Providing format %s in pass-through mode\n",
+				icd->formats[idx].name);
+		}
+	}
+
+	return formats;
+}
+
 static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 			      __u32 pixfmt, struct v4l2_rect *rect)
 {
-	const struct soc_camera_data_format *cam_fmt;
-	int ret;
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	const struct soc_camera_data_format *host_fmt, *cam_fmt = NULL;
+	const struct soc_camera_format_xlate *xlate;
+	int ret, buswidth;
 
-	/*
-	 * TODO: find a suitable supported by the SoC output format, check
-	 * whether the sensor supports one of acceptable input formats.
-	 */
-	if (pixfmt) {
-		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
-		if (!cam_fmt)
-			return -EINVAL;
+	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
+	if (!xlate) {
+		dev_warn(&ici->dev, "Format %x not found\n", pixfmt);
+		return -EINVAL;
 	}
 
-	ret = icd->ops->set_fmt(icd, pixfmt, rect);
-	if (pixfmt && !ret)
-		icd->current_fmt = cam_fmt;
+	buswidth = xlate->buswidth;
+	host_fmt = xlate->host_fmt;
+	cam_fmt = xlate->cam_fmt;
+
+	switch (pixfmt) {
+	case 0:				/* Only geometry change */
+		ret = icd->ops->set_fmt(icd, pixfmt, rect);
+		break;
+	default:
+		ret = icd->ops->set_fmt(icd, cam_fmt->fourcc, rect);
+	}
+
+	if (ret < 0)
+		dev_warn(&ici->dev, "Failed to configure for format %x\n",
+			 pixfmt);
+
+	if (pixfmt && !ret) {
+		icd->buswidth = buswidth;
+		icd->current_fmt = host_fmt;
+	}
 
 	return ret;
 }
@@ -930,34 +1055,31 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 static int pxa_camera_try_fmt(struct soc_camera_device *icd,
 			      struct v4l2_format *f)
 {
-	const struct soc_camera_data_format *cam_fmt;
-	int ret = pxa_camera_try_bus_param(icd, f->fmt.pix.pixelformat);
-
-	if (ret < 0)
-		return ret;
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	const struct soc_camera_format_xlate *xlate;
+	struct v4l2_pix_format *pix = &f->fmt.pix;
+	__u32 pixfmt = pix->pixelformat;
 
-	/*
-	 * TODO: find a suitable supported by the SoC output format, check
-	 * whether the sensor supports one of acceptable input formats.
-	 */
-	cam_fmt = soc_camera_format_by_fourcc(icd, f->fmt.pix.pixelformat);
-	if (!cam_fmt)
+	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
+	if (!xlate) {
+		dev_warn(&ici->dev, "Format %x not found\n", pixfmt);
 		return -EINVAL;
+	}
 
 	/* limit to pxa hardware capabilities */
-	if (f->fmt.pix.height < 32)
-		f->fmt.pix.height = 32;
-	if (f->fmt.pix.height > 2048)
-		f->fmt.pix.height = 2048;
-	if (f->fmt.pix.width < 48)
-		f->fmt.pix.width = 48;
-	if (f->fmt.pix.width > 2048)
-		f->fmt.pix.width = 2048;
-	f->fmt.pix.width &= ~0x01;
-
-	f->fmt.pix.bytesperline = f->fmt.pix.width *
-		DIV_ROUND_UP(cam_fmt->depth, 8);
-	f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
+	if (pix->height < 32)
+		pix->height = 32;
+	if (pix->height > 2048)
+		pix->height = 2048;
+	if (pix->width < 48)
+		pix->width = 48;
+	if (pix->width > 2048)
+		pix->width = 2048;
+	pix->width &= ~0x01;
+
+	pix->bytesperline = pix->width *
+		DIV_ROUND_UP(xlate->host_fmt->depth, 8);
+	pix->sizeimage = pix->height * pix->bytesperline;
 
 	/* limit to sensor capabilities */
 	return icd->ops->try_fmt(icd, f);
@@ -1068,6 +1190,7 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
 	.remove		= pxa_camera_remove_device,
 	.suspend	= pxa_camera_suspend,
 	.resume		= pxa_camera_resume,
+	.get_formats	= pxa_camera_get_formats,
 	.set_fmt	= pxa_camera_set_fmt,
 	.try_fmt	= pxa_camera_try_fmt,
 	.init_videobuf	= pxa_camera_init_videobuf,
-- 
1.5.6.5

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v4 2/2] pxa-camera: pixel format negotiation

[Guennadi Liakhovetski]

Heh, couldn't wait until tomorrow?:-)

On Sat, 29 Nov 2008, Robert Jarzmik wrote:

> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

I haven't looked at the patches yet - will do tomorrow or on monday, but - 
I actually wanted them to be "from you" - that's why I suggested you to do 
the final version. Otherwise I would have done that myself - don't want to 
steal your work. But if you insist, I think, we can share - I make 1/2 
(spc-camera) "from me" and 2/2 (pxa) "from you" - agree?

Thanks
Guennadi

> 
> Use the new format-negotiation infrastructure, support all four YUV422
> packed and the planar formats.
> 
> The new translation structure enables to build the format
> list with buswidth, depth, host format and camera format
> checked, so that it's not done anymore on try_fmt nor
> set_fmt.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/video/pxa_camera.c |  207 ++++++++++++++++++++++++++++++--------
>  1 files changed, 165 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
> index 37afdfa..8219a6c 100644
> --- a/drivers/media/video/pxa_camera.c
> +++ b/drivers/media/video/pxa_camera.c
> @@ -765,6 +765,9 @@ static int test_platform_param(struct pxa_camera_dev *pcdev,
>  		if (!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8))
>  			return -EINVAL;
>  		*flags |= SOCAM_DATAWIDTH_8;
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
>  	return 0;
> @@ -823,12 +826,10 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
>  	 * We fix bit-per-pixel equal to data-width... */
>  	switch (common_flags & SOCAM_DATAWIDTH_MASK) {
>  	case SOCAM_DATAWIDTH_10:
> -		icd->buswidth = 10;
>  		dw = 4;
>  		bpp = 0x40;
>  		break;
>  	case SOCAM_DATAWIDTH_9:
> -		icd->buswidth = 9;
>  		dw = 3;
>  		bpp = 0x20;
>  		break;
> @@ -836,7 +837,6 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
>  		/* Actually it can only be 8 now,
>  		 * default is just to silence compiler warnings */
>  	case SOCAM_DATAWIDTH_8:
> -		icd->buswidth = 8;
>  		dw = 2;
>  		bpp = 0;
>  	}
> @@ -862,7 +862,17 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
>  	case V4L2_PIX_FMT_YUV422P:
>  		pcdev->channels = 3;
>  		cicr1 |= CICR1_YCBCR_F;
> +		/*
> +		 * Normally, pxa bus wants as input UYVY format. We allow all
> +		 * reorderings of the YUV422 format, as no processing is done,
> +		 * and the YUV stream is just passed through without any
> +		 * transformation. Note that UYVY is the only format that
> +		 * should be used if pxa framebuffer Overlay2 is used.
> +		 */
> +	case V4L2_PIX_FMT_UYVY:
> +	case V4L2_PIX_FMT_VYUY:
>  	case V4L2_PIX_FMT_YUYV:
> +	case V4L2_PIX_FMT_YVYU:
>  		cicr1 |= CICR1_COLOR_SP_VAL(2);
>  		break;
>  	case V4L2_PIX_FMT_RGB555:
> @@ -888,13 +898,14 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
>  	return 0;
>  }
>  
> -static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> +static int pxa_camera_try_bus_param(struct soc_camera_device *icd,
> +				    unsigned char buswidth)
>  {
>  	struct soc_camera_host *ici =
>  		to_soc_camera_host(icd->dev.parent);
>  	struct pxa_camera_dev *pcdev = ici->priv;
>  	unsigned long bus_flags, camera_flags;
> -	int ret = test_platform_param(pcdev, icd->buswidth, &bus_flags);
> +	int ret = test_platform_param(pcdev, buswidth, &bus_flags);
>  
>  	if (ret < 0)
>  		return ret;
> @@ -904,25 +915,139 @@ static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
>  	return soc_camera_bus_param_compatible(camera_flags, bus_flags) ? 0 : -EINVAL;
>  }
>  
> +static const struct soc_camera_data_format pxa_camera_formats[] = {
> +	{
> +		.name		= "Planar YUV422 16 bit",
> +		.depth		= 16,
> +		.fourcc		= V4L2_PIX_FMT_YUV422P,
> +		.colorspace	= V4L2_COLORSPACE_JPEG,
> +	},
> +};
> +
> +static bool buswidth_supported(struct soc_camera_device *icd, int depth)
> +{
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> +	struct pxa_camera_dev *pcdev = ici->priv;
> +
> +	switch (depth) {
> +	case 8:
> +		return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8);
> +	case 9:
> +		return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_9);
> +	case 10:
> +		return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_10);
> +	}
> +	return false;
> +}
> +
> +static int required_buswidth(const struct soc_camera_data_format *fmt)
> +{
> +	switch (fmt->fourcc) {
> +	case V4L2_PIX_FMT_UYVY:
> +	case V4L2_PIX_FMT_VYUY:
> +	case V4L2_PIX_FMT_YUYV:
> +	case V4L2_PIX_FMT_YVYU:
> +	case V4L2_PIX_FMT_RGB565:
> +	case V4L2_PIX_FMT_RGB555:
> +		return 8;
> +	default:
> +		return fmt->depth;
> +	}
> +}
> +
> +static int pxa_camera_get_formats(struct soc_camera_device *icd, int idx,
> +				  struct soc_camera_format_xlate *xlate)
> +{
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> +	int formats = 0, buswidth, ret;
> +
> +	buswidth = required_buswidth(icd->formats + idx);
> +
> +	if (!buswidth_supported(icd, buswidth))
> +		return 0;
> +
> +	ret = pxa_camera_try_bus_param(icd, buswidth);
> +	if (ret < 0)
> +		return 0;
> +
> +	switch (icd->formats[idx].fourcc) {
> +	case V4L2_PIX_FMT_UYVY:
> +		formats++;
> +		if (xlate) {
> +			xlate->host_fmt = &pxa_camera_formats[0];
> +			xlate->cam_fmt = icd->formats + idx;
> +			xlate->buswidth = buswidth;
> +			xlate++;
> +			dev_dbg(&ici->dev, "Providing format %s using %s\n",
> +				pxa_camera_formats[0].name,
> +				icd->formats[idx].name);
> +		}
> +	case V4L2_PIX_FMT_VYUY:
> +	case V4L2_PIX_FMT_YUYV:
> +	case V4L2_PIX_FMT_YVYU:
> +	case V4L2_PIX_FMT_RGB565:
> +	case V4L2_PIX_FMT_RGB555:
> +		formats++;
> +		if (xlate) {
> +			xlate->host_fmt = icd->formats + idx;
> +			xlate->cam_fmt = icd->formats + idx;
> +			xlate->buswidth = buswidth;
> +			xlate++;
> +			dev_dbg(&ici->dev, "Providing format %s packed\n",
> +				icd->formats[idx].name);
> +		}
> +		break;
> +	default:
> +		/* Generic pass-through */
> +		formats++;
> +		if (xlate) {
> +			xlate->host_fmt = icd->formats + idx;
> +			xlate->cam_fmt = icd->formats + idx;
> +			xlate->buswidth = icd->formats[idx].depth;
> +			xlate++;
> +			dev_dbg(&ici->dev,
> +				"Providing format %s in pass-through mode\n",
> +				icd->formats[idx].name);
> +		}
> +	}
> +
> +	return formats;
> +}
> +
>  static int pxa_camera_set_fmt(struct soc_camera_device *icd,
>  			      __u32 pixfmt, struct v4l2_rect *rect)
>  {
> -	const struct soc_camera_data_format *cam_fmt;
> -	int ret;
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> +	const struct soc_camera_data_format *host_fmt, *cam_fmt = NULL;
> +	const struct soc_camera_format_xlate *xlate;
> +	int ret, buswidth;
>  
> -	/*
> -	 * TODO: find a suitable supported by the SoC output format, check
> -	 * whether the sensor supports one of acceptable input formats.
> -	 */
> -	if (pixfmt) {
> -		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
> -		if (!cam_fmt)
> -			return -EINVAL;
> +	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
> +	if (!xlate) {
> +		dev_warn(&ici->dev, "Format %x not found\n", pixfmt);
> +		return -EINVAL;
>  	}
>  
> -	ret = icd->ops->set_fmt(icd, pixfmt, rect);
> -	if (pixfmt && !ret)
> -		icd->current_fmt = cam_fmt;
> +	buswidth = xlate->buswidth;
> +	host_fmt = xlate->host_fmt;
> +	cam_fmt = xlate->cam_fmt;
> +
> +	switch (pixfmt) {
> +	case 0:				/* Only geometry change */
> +		ret = icd->ops->set_fmt(icd, pixfmt, rect);
> +		break;
> +	default:
> +		ret = icd->ops->set_fmt(icd, cam_fmt->fourcc, rect);
> +	}
> +
> +	if (ret < 0)
> +		dev_warn(&ici->dev, "Failed to configure for format %x\n",
> +			 pixfmt);
> +
> +	if (pixfmt && !ret) {
> +		icd->buswidth = buswidth;
> +		icd->current_fmt = host_fmt;
> +	}
>  
>  	return ret;
>  }
> @@ -930,34 +1055,31 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
>  static int pxa_camera_try_fmt(struct soc_camera_device *icd,
>  			      struct v4l2_format *f)
>  {
> -	const struct soc_camera_data_format *cam_fmt;
> -	int ret = pxa_camera_try_bus_param(icd, f->fmt.pix.pixelformat);
> -
> -	if (ret < 0)
> -		return ret;
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> +	const struct soc_camera_format_xlate *xlate;
> +	struct v4l2_pix_format *pix = &f->fmt.pix;
> +	__u32 pixfmt = pix->pixelformat;
>  
> -	/*
> -	 * TODO: find a suitable supported by the SoC output format, check
> -	 * whether the sensor supports one of acceptable input formats.
> -	 */
> -	cam_fmt = soc_camera_format_by_fourcc(icd, f->fmt.pix.pixelformat);
> -	if (!cam_fmt)
> +	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
> +	if (!xlate) {
> +		dev_warn(&ici->dev, "Format %x not found\n", pixfmt);
>  		return -EINVAL;
> +	}
>  
>  	/* limit to pxa hardware capabilities */
> -	if (f->fmt.pix.height < 32)
> -		f->fmt.pix.height = 32;
> -	if (f->fmt.pix.height > 2048)
> -		f->fmt.pix.height = 2048;
> -	if (f->fmt.pix.width < 48)
> -		f->fmt.pix.width = 48;
> -	if (f->fmt.pix.width > 2048)
> -		f->fmt.pix.width = 2048;
> -	f->fmt.pix.width &= ~0x01;
> -
> -	f->fmt.pix.bytesperline = f->fmt.pix.width *
> -		DIV_ROUND_UP(cam_fmt->depth, 8);
> -	f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
> +	if (pix->height < 32)
> +		pix->height = 32;
> +	if (pix->height > 2048)
> +		pix->height = 2048;
> +	if (pix->width < 48)
> +		pix->width = 48;
> +	if (pix->width > 2048)
> +		pix->width = 2048;
> +	pix->width &= ~0x01;
> +
> +	pix->bytesperline = pix->width *
> +		DIV_ROUND_UP(xlate->host_fmt->depth, 8);
> +	pix->sizeimage = pix->height * pix->bytesperline;
>  
>  	/* limit to sensor capabilities */
>  	return icd->ops->try_fmt(icd, f);
> @@ -1068,6 +1190,7 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
>  	.remove		= pxa_camera_remove_device,
>  	.suspend	= pxa_camera_suspend,
>  	.resume		= pxa_camera_resume,
> +	.get_formats	= pxa_camera_get_formats,
>  	.set_fmt	= pxa_camera_set_fmt,
>  	.try_fmt	= pxa_camera_try_fmt,
>  	.init_videobuf	= pxa_camera_init_videobuf,
> -- 
> 1.5.6.5
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list