diff mbox series

[v5,06/14] media: vimc: cap: Add handler for multiplanar fmt ioctls

Message ID 20190426195114.5002-7-andrealmeid@collabora.com (mailing list archive)
State New, archived
Headers show
Series media: vimc: Add support for multiplanar formats | expand

Commit Message

André Almeida April 26, 2019, 7:51 p.m. UTC
Add functions to handle multiplanar format ioctls, calling
the generic format ioctls functions when possible.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
---
Change in v5: none

Change in v4:
- Split from multiplanar parameter commit
- Use `IS_MULTIPLANAR` instead of `multiplanar`
- Move "Functions to handle..." to commit 5

Change in v3:
- Remove functions that only did the `IS_MULTIPLANAR(vcap)` check
- Squash with multiplanar parameter commit

Change in v2:
- Fix alignment

 drivers/media/platform/vimc/vimc-capture.c | 63 +++++++++++++++++++---
 1 file changed, 57 insertions(+), 6 deletions(-)

Comments

Hans Verkuil April 29, 2019, 10:09 a.m. UTC | #1
On 4/26/19 9:51 PM, André Almeida wrote:
> Add functions to handle multiplanar format ioctls, calling
> the generic format ioctls functions when possible.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> ---
> Change in v5: none
> 
> Change in v4:
> - Split from multiplanar parameter commit
> - Use `IS_MULTIPLANAR` instead of `multiplanar`
> - Move "Functions to handle..." to commit 5
> 
> Change in v3:
> - Remove functions that only did the `IS_MULTIPLANAR(vcap)` check
> - Squash with multiplanar parameter commit
> 
> Change in v2:
> - Fix alignment
> 
>  drivers/media/platform/vimc/vimc-capture.c | 63 +++++++++++++++++++---
>  1 file changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index fa0d75eb06e8..652d3d08bc50 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -211,6 +211,44 @@ static int vimc_cap_s_fmt_vid_cap_sp(struct file *file, void *priv,
>  	return 0;
>  }
>  
> +/*
> + * VIDIOC handlers for multi-planar formats
> + */
> +static int vimc_cap_s_fmt_vid_cap_mp(struct file *file, void *priv,
> +				     struct v4l2_format *f)
> +{
> +	struct vimc_cap_device *vcap = video_drvdata(file);
> +	int ret;
> +
> +	/* Do not change the format while stream is on */
> +	if (vb2_is_busy(&vcap->queue))
> +		return -EBUSY;
> +
> +	ret = vimc_cap_try_fmt_vid_cap_mp(file, priv, f);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(vcap->dev, "%s: format update: "
> +		"old:%dx%d (0x%x, %d, %d, %d, %d) "
> +		"new:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name,
> +		/* old */
> +		vcap->format.fmt.pix_mp.width, vcap->format.fmt.pix_mp.height,
> +		vcap->format.fmt.pix_mp.pixelformat,
> +		vcap->format.fmt.pix_mp.colorspace,
> +		vcap->format.fmt.pix_mp.quantization,
> +		vcap->format.fmt.pix_mp.xfer_func,
> +		vcap->format.fmt.pix_mp.ycbcr_enc,
> +		/* new */
> +		f->fmt.pix_mp.width, f->fmt.pix_mp.height,
> +		f->fmt.pix_mp.pixelformat, f->fmt.pix_mp.colorspace,
> +		f->fmt.pix_mp.quantization, f->fmt.pix_mp.xfer_func,
> +		f->fmt.pix_mp.ycbcr_enc);
> +
> +	vcap->format = *f;
> +
> +	return 0;
> +}
> +
>  static bool vimc_cap_is_pixfmt_supported(u32 pixelformat)
>  {
>  	unsigned int i;
> @@ -251,13 +289,9 @@ static const struct v4l2_file_operations vimc_cap_fops = {
>  	.mmap           = vb2_fop_mmap,
>  };
>  
> -static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = {
> +static struct v4l2_ioctl_ops vimc_cap_ioctl_ops = {
>  	.vidioc_querycap = vimc_cap_querycap,
>  
> -	.vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap,
> -	.vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap_sp,
> -	.vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap_sp,
> -	.vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap,
>  	.vidioc_enum_framesizes = vimc_cap_enum_framesizes,
>  
>  	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> @@ -455,6 +489,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  {
>  	struct v4l2_device *v4l2_dev = master_data;
>  	struct vimc_platform_data *pdata = comp->platform_data;
> +	struct v4l2_ioctl_ops *ioctl_ops = &vimc_cap_ioctl_ops;
>  	struct vimc_cap_device *vcap;
>  	struct video_device *vdev;
>  	struct vb2_queue *q;
> @@ -527,7 +562,23 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  	vdev->entity.ops = &vimc_cap_mops;
>  	vdev->release = vimc_cap_release;
>  	vdev->fops = &vimc_cap_fops;
> -	vdev->ioctl_ops = &vimc_cap_ioctl_ops;
> +
> +	if (IS_MULTIPLANAR(vcap)) {
> +		ioctl_ops->vidioc_g_fmt_vid_cap_mplane = vimc_cap_g_fmt_vid_cap;
> +		ioctl_ops->vidioc_s_fmt_vid_cap_mplane =
> +						vimc_cap_s_fmt_vid_cap_mp;
> +		ioctl_ops->vidioc_try_fmt_vid_cap_mplane =
> +						vimc_cap_try_fmt_vid_cap_mp;
> +		ioctl_ops->vidioc_enum_fmt_vid_cap_mplane =
> +						vimc_cap_enum_fmt_vid_cap;
> +	} else {
> +		ioctl_ops->vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap;
> +		ioctl_ops->vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap_sp;
> +		ioctl_ops->vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap_sp;
> +		ioctl_ops->vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap;
> +	}

I don't really like this approach. It is good practice to keep vimc_cap_ioctl_ops
a const struct.

Instead do the same thing that vivid does: see e.g. vidioc_s_fmt_vid_cap and
vidioc_s_fmt_vid_cap_mplane in vivid-vid-cap.c.

Regards,

	Hans

> +
> +	vdev->ioctl_ops = ioctl_ops;
>  	vdev->lock = &vcap->lock;
>  	vdev->queue = q;
>  	vdev->v4l2_dev = v4l2_dev;
>
diff mbox series

Patch

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index fa0d75eb06e8..652d3d08bc50 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -211,6 +211,44 @@  static int vimc_cap_s_fmt_vid_cap_sp(struct file *file, void *priv,
 	return 0;
 }
 
+/*
+ * VIDIOC handlers for multi-planar formats
+ */
+static int vimc_cap_s_fmt_vid_cap_mp(struct file *file, void *priv,
+				     struct v4l2_format *f)
+{
+	struct vimc_cap_device *vcap = video_drvdata(file);
+	int ret;
+
+	/* Do not change the format while stream is on */
+	if (vb2_is_busy(&vcap->queue))
+		return -EBUSY;
+
+	ret = vimc_cap_try_fmt_vid_cap_mp(file, priv, f);
+	if (ret)
+		return ret;
+
+	dev_dbg(vcap->dev, "%s: format update: "
+		"old:%dx%d (0x%x, %d, %d, %d, %d) "
+		"new:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name,
+		/* old */
+		vcap->format.fmt.pix_mp.width, vcap->format.fmt.pix_mp.height,
+		vcap->format.fmt.pix_mp.pixelformat,
+		vcap->format.fmt.pix_mp.colorspace,
+		vcap->format.fmt.pix_mp.quantization,
+		vcap->format.fmt.pix_mp.xfer_func,
+		vcap->format.fmt.pix_mp.ycbcr_enc,
+		/* new */
+		f->fmt.pix_mp.width, f->fmt.pix_mp.height,
+		f->fmt.pix_mp.pixelformat, f->fmt.pix_mp.colorspace,
+		f->fmt.pix_mp.quantization, f->fmt.pix_mp.xfer_func,
+		f->fmt.pix_mp.ycbcr_enc);
+
+	vcap->format = *f;
+
+	return 0;
+}
+
 static bool vimc_cap_is_pixfmt_supported(u32 pixelformat)
 {
 	unsigned int i;
@@ -251,13 +289,9 @@  static const struct v4l2_file_operations vimc_cap_fops = {
 	.mmap           = vb2_fop_mmap,
 };
 
-static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = {
+static struct v4l2_ioctl_ops vimc_cap_ioctl_ops = {
 	.vidioc_querycap = vimc_cap_querycap,
 
-	.vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap,
-	.vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap_sp,
-	.vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap_sp,
-	.vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap,
 	.vidioc_enum_framesizes = vimc_cap_enum_framesizes,
 
 	.vidioc_reqbufs = vb2_ioctl_reqbufs,
@@ -455,6 +489,7 @@  static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 {
 	struct v4l2_device *v4l2_dev = master_data;
 	struct vimc_platform_data *pdata = comp->platform_data;
+	struct v4l2_ioctl_ops *ioctl_ops = &vimc_cap_ioctl_ops;
 	struct vimc_cap_device *vcap;
 	struct video_device *vdev;
 	struct vb2_queue *q;
@@ -527,7 +562,23 @@  static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	vdev->entity.ops = &vimc_cap_mops;
 	vdev->release = vimc_cap_release;
 	vdev->fops = &vimc_cap_fops;
-	vdev->ioctl_ops = &vimc_cap_ioctl_ops;
+
+	if (IS_MULTIPLANAR(vcap)) {
+		ioctl_ops->vidioc_g_fmt_vid_cap_mplane = vimc_cap_g_fmt_vid_cap;
+		ioctl_ops->vidioc_s_fmt_vid_cap_mplane =
+						vimc_cap_s_fmt_vid_cap_mp;
+		ioctl_ops->vidioc_try_fmt_vid_cap_mplane =
+						vimc_cap_try_fmt_vid_cap_mp;
+		ioctl_ops->vidioc_enum_fmt_vid_cap_mplane =
+						vimc_cap_enum_fmt_vid_cap;
+	} else {
+		ioctl_ops->vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap;
+		ioctl_ops->vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap_sp;
+		ioctl_ops->vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap_sp;
+		ioctl_ops->vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap;
+	}
+
+	vdev->ioctl_ops = ioctl_ops;
 	vdev->lock = &vcap->lock;
 	vdev->queue = q;
 	vdev->v4l2_dev = v4l2_dev;