| Message ID | 20190424135642.31973-15-andrealmeid@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vimc: Add support for multiplanar formats | expand |
Hi André, Thanks for the patch, please see my comments below. On 4/24/19 10:56 AM, André Almeida wrote: > Create multiplanar kernel module parameter to define if the driver is > running in single planar or in multiplanar mode. Define the device > capabilities and format ioctls according to the multiplanar kernel > parameter. A device can't support both CAP_VIDEO_CAPTURE and > CAP_VIDEO_CAPTURE_MPLANAR at the same time. Add functions to handle > multiplanar format ioctls, calling the generic format ioctls functions > when possible.> > Signed-off-by: André Almeida <andrealmeid@collabora.com> > --- > Change in v3: > - Squash commit with "Add handler for multiplanar fmt ioctls" Was there any reason to squash those? Could you please unsquash it? so we can have one commit with the handlers and the last one adding the kernel parameter? > - Remove functions that only did the `IS_MULTIPLANAR(vcap)` check > - Assign ioctls according to device capabilities > > Changes in v2: > - Squash commits to create multiplanar module parameter and to define > the device capabilities > - Move the creation of the multiplanar parameter to the end of > history, so it's only added when all required changes are applied > > drivers/media/platform/vimc/vimc-capture.c | 78 +++++++++++++++++++--- > 1 file changed, 70 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c > index 2592ea982ff8..27caf14ddf43 100644 > --- a/drivers/media/platform/vimc/vimc-capture.c > +++ b/drivers/media/platform/vimc/vimc-capture.c > @@ -28,6 +28,10 @@ > > #define VIMC_CAP_DRV_NAME "vimc-capture" > > +static unsigned int multiplanar; > +module_param(multiplanar, uint, 0000); > +MODULE_PARM_DESC(multiplanar, "0 (default) creates a single planar device, 1 creates a multiplanar device."); > + > /* Checks if the device supports multiplanar capture */ > #define IS_MULTIPLANAR(vcap) \ > (vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE) > @@ -144,6 +148,10 @@ static void vimc_cap_get_format(struct vimc_ent_device *ved, > *fmt = vcap->format.fmt.pix; > } > > +/* > + * Functions to handle both single- and multi-planar VIDIOC FMT > + */ > + This comment could be added in commit 5 (where the single format comment was added) > static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv, > struct v4l2_format *f) > { > @@ -247,6 +255,41 @@ 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); > + > + /* Do not change the format while stream is on */ > + if (vb2_is_busy(&vcap->queue)) > + return -EBUSY; > + > + vimc_cap_try_fmt_vid_cap_mp(file, priv, f); I know the original code wasn't checking for errors in this func, you could add a check here it would be great. > + > + 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; > @@ -257,6 +300,7 @@ static bool vimc_cap_is_pixfmt_supported(u32 pixelformat) > return false; > } > > + > static int vimc_cap_enum_framesizes(struct file *file, void *fh, > struct v4l2_frmsizeenum *fsize) > { > @@ -287,13 +331,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, > @@ -529,6 +569,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; > @@ -560,7 +601,9 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master, > > /* Initialize the vb2 queue */ > q = &vcap->queue; > - q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > + q->type = multiplanar ? > + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE : > + V4L2_BUF_TYPE_VIDEO_CAPTURE; > q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR; > q->drv_priv = vcap; > q->buf_struct_size = sizeof(struct vimc_cap_buffer); > @@ -588,13 +631,32 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master, > dev_set_drvdata(comp, &vcap->ved); > vcap->dev = comp; > > + > /* Initialize the video_device struct */ > vdev = &vcap->vdev; > - vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; > + vdev->device_caps = (multiplanar ? > + V4L2_CAP_VIDEO_CAPTURE_MPLANE : > + V4L2_CAP_VIDEO_CAPTURE) | V4L2_CAP_STREAMING; > vdev->entity.ops = &vimc_cap_mops; > vdev->release = vimc_cap_release; > vdev->fops = &vimc_cap_fops; > - vdev->ioctl_ops = &vimc_cap_ioctl_ops; > + > + if (multiplanar) { Please, use the IS_MULTIPLANAR macro here. Helen > + 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; >
Hello Helen, Thanks for your review! On 4/24/19 6:32 PM, Helen Koike wrote: > Hi André, > > Thanks for the patch, please see my comments below. > > On 4/24/19 10:56 AM, André Almeida wrote: >> Create multiplanar kernel module parameter to define if the driver is >> running in single planar or in multiplanar mode. Define the device >> capabilities and format ioctls according to the multiplanar kernel >> parameter. A device can't support both CAP_VIDEO_CAPTURE and >> CAP_VIDEO_CAPTURE_MPLANAR at the same time. Add functions to handle >> multiplanar format ioctls, calling the generic format ioctls functions >> when possible.> >> Signed-off-by: André Almeida <andrealmeid@collabora.com> >> --- >> Change in v3: >> - Squash commit with "Add handler for multiplanar fmt ioctls" > Was there any reason to squash those? Could you please unsquash it? > so we can have one commit with the handlers and the last one adding the > kernel parameter? It was because if I add those functions to the code, it would give the warning of function defined but not used. I only use the multiplanar format ioctls when the multiplanar variable exists. >> - Remove functions that only did the `IS_MULTIPLANAR(vcap)` check >> - Assign ioctls according to device capabilities >> >> Changes in v2: >> - Squash commits to create multiplanar module parameter and to define >> the device capabilities >> - Move the creation of the multiplanar parameter to the end of >> history, so it's only added when all required changes are applied >> >> drivers/media/platform/vimc/vimc-capture.c | 78 +++++++++++++++++++--- >> 1 file changed, 70 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >> index 2592ea982ff8..27caf14ddf43 100644 >> --- a/drivers/media/platform/vimc/vimc-capture.c >> +++ b/drivers/media/platform/vimc/vimc-capture.c >> @@ -28,6 +28,10 @@ >> >> #define VIMC_CAP_DRV_NAME "vimc-capture" >> >> +static unsigned int multiplanar; >> +module_param(multiplanar, uint, 0000); >> +MODULE_PARM_DESC(multiplanar, "0 (default) creates a single planar device, 1 creates a multiplanar device."); >> + >> /* Checks if the device supports multiplanar capture */ >> #define IS_MULTIPLANAR(vcap) \ >> (vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE) >> @@ -144,6 +148,10 @@ static void vimc_cap_get_format(struct vimc_ent_device *ved, >> *fmt = vcap->format.fmt.pix; >> } >> >> +/* >> + * Functions to handle both single- and multi-planar VIDIOC FMT >> + */ >> + > This comment could be added in commit 5 (where the single format > comment was added) > >> static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv, >> struct v4l2_format *f) >> { >> @@ -247,6 +255,41 @@ 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); >> + >> + /* Do not change the format while stream is on */ >> + if (vb2_is_busy(&vcap->queue)) >> + return -EBUSY; >> + >> + vimc_cap_try_fmt_vid_cap_mp(file, priv, f); > I know the original code wasn't checking for errors in this func, you > could add a check here it would be great. What kind of error checking? Checking if the try format was successful? > >> + >> + 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; >> @@ -257,6 +300,7 @@ static bool vimc_cap_is_pixfmt_supported(u32 pixelformat) >> return false; >> } >> >> + >> static int vimc_cap_enum_framesizes(struct file *file, void *fh, >> struct v4l2_frmsizeenum *fsize) >> { >> @@ -287,13 +331,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, >> @@ -529,6 +569,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; >> @@ -560,7 +601,9 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master, >> >> /* Initialize the vb2 queue */ >> q = &vcap->queue; >> - q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> + q->type = multiplanar ? >> + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE : >> + V4L2_BUF_TYPE_VIDEO_CAPTURE; >> q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR; >> q->drv_priv = vcap; >> q->buf_struct_size = sizeof(struct vimc_cap_buffer); >> @@ -588,13 +631,32 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master, >> dev_set_drvdata(comp, &vcap->ved); >> vcap->dev = comp; >> >> + >> /* Initialize the video_device struct */ >> vdev = &vcap->vdev; >> - vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; >> + vdev->device_caps = (multiplanar ? >> + V4L2_CAP_VIDEO_CAPTURE_MPLANE : >> + V4L2_CAP_VIDEO_CAPTURE) | V4L2_CAP_STREAMING; >> vdev->entity.ops = &vimc_cap_mops; >> vdev->release = vimc_cap_release; >> vdev->fops = &vimc_cap_fops; >> - vdev->ioctl_ops = &vimc_cap_ioctl_ops; >> + >> + if (multiplanar) { > Please, use the IS_MULTIPLANAR macro here. The IS_MULTIPLANAR macro depends on the vcap->vdev.device_caps being assigned but vcap->vdev is only assigned on line 663, and to do this assignment, the vdev->ioctl_ops must be defined. André > Helen > >> + 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; >>
On 4/24/19 8:03 PM, André Almeida wrote: > Hello Helen, > > Thanks for your review! > > On 4/24/19 6:32 PM, Helen Koike wrote: >> Hi André, >> >> Thanks for the patch, please see my comments below. >> >> On 4/24/19 10:56 AM, André Almeida wrote: >>> Create multiplanar kernel module parameter to define if the driver is >>> running in single planar or in multiplanar mode. Define the device >>> capabilities and format ioctls according to the multiplanar kernel >>> parameter. A device can't support both CAP_VIDEO_CAPTURE and >>> CAP_VIDEO_CAPTURE_MPLANAR at the same time. Add functions to handle >>> multiplanar format ioctls, calling the generic format ioctls functions >>> when possible.> >>> Signed-off-by: André Almeida <andrealmeid@collabora.com> >>> --- >>> Change in v3: >>> - Squash commit with "Add handler for multiplanar fmt ioctls" >> Was there any reason to squash those? Could you please unsquash it? >> so we can have one commit with the handlers and the last one adding the >> kernel parameter? > > It was because if I add those functions to the code, it would give the > warning of function defined but not used. I only use the multiplanar > format ioctls when the multiplanar variable exists. > >>> - Remove functions that only did the `IS_MULTIPLANAR(vcap)` check >>> - Assign ioctls according to device capabilities >>> >>> Changes in v2: >>> - Squash commits to create multiplanar module parameter and to define >>> the device capabilities >>> - Move the creation of the multiplanar parameter to the end of >>> history, so it's only added when all required changes are applied >>> >>> drivers/media/platform/vimc/vimc-capture.c | 78 +++++++++++++++++++--- >>> 1 file changed, 70 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >>> index 2592ea982ff8..27caf14ddf43 100644 >>> --- a/drivers/media/platform/vimc/vimc-capture.c >>> +++ b/drivers/media/platform/vimc/vimc-capture.c >>> @@ -28,6 +28,10 @@ >>> >>> #define VIMC_CAP_DRV_NAME "vimc-capture" >>> >>> +static unsigned int multiplanar; >>> +module_param(multiplanar, uint, 0000); >>> +MODULE_PARM_DESC(multiplanar, "0 (default) creates a single planar device, 1 creates a multiplanar device."); >>> + >>> /* Checks if the device supports multiplanar capture */ >>> #define IS_MULTIPLANAR(vcap) \ >>> (vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE) >>> @@ -144,6 +148,10 @@ static void vimc_cap_get_format(struct vimc_ent_device *ved, >>> *fmt = vcap->format.fmt.pix; >>> } >>> >>> +/* >>> + * Functions to handle both single- and multi-planar VIDIOC FMT >>> + */ >>> + >> This comment could be added in commit 5 (where the single format >> comment was added) >> >>> static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv, >>> struct v4l2_format *f) >>> { >>> @@ -247,6 +255,41 @@ 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); >>> + >>> + /* Do not change the format while stream is on */ >>> + if (vb2_is_busy(&vcap->queue)) >>> + return -EBUSY; >>> + >>> + vimc_cap_try_fmt_vid_cap_mp(file, priv, f); >> I know the original code wasn't checking for errors in this func, you >> could add a check here it would be great. > What kind of error checking? Checking if the try format was successful? The return code of the function. >> >>> + >>> + 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; >>> @@ -257,6 +300,7 @@ static bool vimc_cap_is_pixfmt_supported(u32 pixelformat) >>> return false; >>> } >>> >>> + >>> static int vimc_cap_enum_framesizes(struct file *file, void *fh, >>> struct v4l2_frmsizeenum *fsize) >>> { >>> @@ -287,13 +331,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, >>> @@ -529,6 +569,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; >>> @@ -560,7 +601,9 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master, >>> >>> /* Initialize the vb2 queue */ >>> q = &vcap->queue; >>> - q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >>> + q->type = multiplanar ? >>> + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE : >>> + V4L2_BUF_TYPE_VIDEO_CAPTURE; >>> q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR; >>> q->drv_priv = vcap; >>> q->buf_struct_size = sizeof(struct vimc_cap_buffer); >>> @@ -588,13 +631,32 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master, >>> dev_set_drvdata(comp, &vcap->ved); >>> vcap->dev = comp; >>> >>> + >>> /* Initialize the video_device struct */ >>> vdev = &vcap->vdev; >>> - vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; >>> + vdev->device_caps = (multiplanar ? >>> + V4L2_CAP_VIDEO_CAPTURE_MPLANE : >>> + V4L2_CAP_VIDEO_CAPTURE) | V4L2_CAP_STREAMING; vcap->vdev.device_caps is assigned right here right? >>> vdev->entity.ops = &vimc_cap_mops; >>> vdev->release = vimc_cap_release; >>> vdev->fops = &vimc_cap_fops; >>> - vdev->ioctl_ops = &vimc_cap_ioctl_ops; >>> + >>> + if (multiplanar) { >> Please, use the IS_MULTIPLANAR macro here. > > The IS_MULTIPLANAR macro depends on the vcap->vdev.device_caps being > assigned but vcap->vdev is only assigned on line 663, and to do this > assignment, the vdev->ioctl_ops must be defined. But I see vcap->vdev.device_caps is being assigned just before this part, no? Helen > > André > >> Helen >> >>> + 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; >>> >
On 4/24/19 8:19 PM, Helen Koike wrote: > > On 4/24/19 8:03 PM, André Almeida wrote: >> Hello Helen, >> >> Thanks for your review! >> >> On 4/24/19 6:32 PM, Helen Koike wrote: >>> Hi André, >>> >>> Thanks for the patch, please see my comments below. >>> >>> On 4/24/19 10:56 AM, André Almeida wrote: >>>> Create multiplanar kernel module parameter to define if the driver is >>>> running in single planar or in multiplanar mode. Define the device >>>> capabilities and format ioctls according to the multiplanar kernel >>>> parameter. A device can't support both CAP_VIDEO_CAPTURE and >>>> CAP_VIDEO_CAPTURE_MPLANAR at the same time. Add functions to handle >>>> multiplanar format ioctls, calling the generic format ioctls functions >>>> when possible.> >>>> Signed-off-by: André Almeida <andrealmeid@collabora.com> >>>> --- >>>> Change in v3: >>>> - Squash commit with "Add handler for multiplanar fmt ioctls" >>> Was there any reason to squash those? Could you please unsquash it? >>> so we can have one commit with the handlers and the last one adding the >>> kernel parameter? >> It was because if I add those functions to the code, it would give the >> warning of function defined but not used. I only use the multiplanar >> format ioctls when the multiplanar variable exists. >> >>>> - Remove functions that only did the `IS_MULTIPLANAR(vcap)` check >>>> - Assign ioctls according to device capabilities >>>> >>>> Changes in v2: >>>> - Squash commits to create multiplanar module parameter and to define >>>> the device capabilities >>>> - Move the creation of the multiplanar parameter to the end of >>>> history, so it's only added when all required changes are applied >>>> >>>> drivers/media/platform/vimc/vimc-capture.c | 78 +++++++++++++++++++--- >>>> 1 file changed, 70 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >>>> index 2592ea982ff8..27caf14ddf43 100644 >>>> --- a/drivers/media/platform/vimc/vimc-capture.c >>>> +++ b/drivers/media/platform/vimc/vimc-capture.c >>>> @@ -28,6 +28,10 @@ >>>> >>>> #define VIMC_CAP_DRV_NAME "vimc-capture" >>>> >>>> +static unsigned int multiplanar; >>>> +module_param(multiplanar, uint, 0000); >>>> +MODULE_PARM_DESC(multiplanar, "0 (default) creates a single planar device, 1 creates a multiplanar device."); >>>> + >>>> /* Checks if the device supports multiplanar capture */ >>>> #define IS_MULTIPLANAR(vcap) \ >>>> (vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE) >>>> @@ -144,6 +148,10 @@ static void vimc_cap_get_format(struct vimc_ent_device *ved, >>>> *fmt = vcap->format.fmt.pix; >>>> } >>>> >>>> +/* >>>> + * Functions to handle both single- and multi-planar VIDIOC FMT >>>> + */ >>>> + >>> This comment could be added in commit 5 (where the single format >>> comment was added) >>> >>>> static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv, >>>> struct v4l2_format *f) >>>> { >>>> @@ -247,6 +255,41 @@ 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); >>>> + >>>> + /* Do not change the format while stream is on */ >>>> + if (vb2_is_busy(&vcap->queue)) >>>> + return -EBUSY; >>>> + >>>> + vimc_cap_try_fmt_vid_cap_mp(file, priv, f); >>> I know the original code wasn't checking for errors in this func, you >>> could add a check here it would be great. >> What kind of error checking? Checking if the try format was successful? > The return code of the function. > >>>> + >>>> + 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; >>>> @@ -257,6 +300,7 @@ static bool vimc_cap_is_pixfmt_supported(u32 pixelformat) >>>> return false; >>>> } >>>> >>>> + >>>> static int vimc_cap_enum_framesizes(struct file *file, void *fh, >>>> struct v4l2_frmsizeenum *fsize) >>>> { >>>> @@ -287,13 +331,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, >>>> @@ -529,6 +569,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; >>>> @@ -560,7 +601,9 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master, >>>> >>>> /* Initialize the vb2 queue */ >>>> q = &vcap->queue; >>>> - q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >>>> + q->type = multiplanar ? >>>> + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE : >>>> + V4L2_BUF_TYPE_VIDEO_CAPTURE; >>>> q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR; >>>> q->drv_priv = vcap; >>>> q->buf_struct_size = sizeof(struct vimc_cap_buffer); >>>> @@ -588,13 +631,32 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master, >>>> dev_set_drvdata(comp, &vcap->ved); >>>> vcap->dev = comp; >>>> >>>> + >>>> /* Initialize the video_device struct */ >>>> vdev = &vcap->vdev; >>>> - vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; >>>> + vdev->device_caps = (multiplanar ? >>>> + V4L2_CAP_VIDEO_CAPTURE_MPLANE : >>>> + V4L2_CAP_VIDEO_CAPTURE) | V4L2_CAP_STREAMING; > vcap->vdev.device_caps is assigned right here right? > >>>> vdev->entity.ops = &vimc_cap_mops; >>>> vdev->release = vimc_cap_release; >>>> vdev->fops = &vimc_cap_fops; >>>> - vdev->ioctl_ops = &vimc_cap_ioctl_ops; >>>> + >>>> + if (multiplanar) { >>> Please, use the IS_MULTIPLANAR macro here. >> The IS_MULTIPLANAR macro depends on the vcap->vdev.device_caps being >> assigned but vcap->vdev is only assigned on line 663, and to do this >> assignment, the vdev->ioctl_ops must be defined. > But I see vcap->vdev.device_caps is being assigned just before this > part, no? You are right, sorry for that :) > > Helen > >> André >> >>> Helen >>> >>>> + 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; >>>>
diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c index 2592ea982ff8..27caf14ddf43 100644 --- a/drivers/media/platform/vimc/vimc-capture.c +++ b/drivers/media/platform/vimc/vimc-capture.c @@ -28,6 +28,10 @@ #define VIMC_CAP_DRV_NAME "vimc-capture" +static unsigned int multiplanar; +module_param(multiplanar, uint, 0000); +MODULE_PARM_DESC(multiplanar, "0 (default) creates a single planar device, 1 creates a multiplanar device."); + /* Checks if the device supports multiplanar capture */ #define IS_MULTIPLANAR(vcap) \ (vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE) @@ -144,6 +148,10 @@ static void vimc_cap_get_format(struct vimc_ent_device *ved, *fmt = vcap->format.fmt.pix; } +/* + * Functions to handle both single- and multi-planar VIDIOC FMT + */ + static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format *f) { @@ -247,6 +255,41 @@ 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); + + /* Do not change the format while stream is on */ + if (vb2_is_busy(&vcap->queue)) + return -EBUSY; + + vimc_cap_try_fmt_vid_cap_mp(file, priv, f); + + 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; @@ -257,6 +300,7 @@ static bool vimc_cap_is_pixfmt_supported(u32 pixelformat) return false; } + static int vimc_cap_enum_framesizes(struct file *file, void *fh, struct v4l2_frmsizeenum *fsize) { @@ -287,13 +331,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, @@ -529,6 +569,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; @@ -560,7 +601,9 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master, /* Initialize the vb2 queue */ q = &vcap->queue; - q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; + q->type = multiplanar ? + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE : + V4L2_BUF_TYPE_VIDEO_CAPTURE; q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR; q->drv_priv = vcap; q->buf_struct_size = sizeof(struct vimc_cap_buffer); @@ -588,13 +631,32 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master, dev_set_drvdata(comp, &vcap->ved); vcap->dev = comp; + /* Initialize the video_device struct */ vdev = &vcap->vdev; - vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; + vdev->device_caps = (multiplanar ? + V4L2_CAP_VIDEO_CAPTURE_MPLANE : + V4L2_CAP_VIDEO_CAPTURE) | V4L2_CAP_STREAMING; vdev->entity.ops = &vimc_cap_mops; vdev->release = vimc_cap_release; vdev->fops = &vimc_cap_fops; - vdev->ioctl_ops = &vimc_cap_ioctl_ops; + + if (multiplanar) { + 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;
Create multiplanar kernel module parameter to define if the driver is running in single planar or in multiplanar mode. Define the device capabilities and format ioctls according to the multiplanar kernel parameter. A device can't support both CAP_VIDEO_CAPTURE and CAP_VIDEO_CAPTURE_MPLANAR at the same time. Add functions to handle multiplanar format ioctls, calling the generic format ioctls functions when possible. Signed-off-by: André Almeida <andrealmeid@collabora.com> --- Change in v3: - Squash commit with "Add handler for multiplanar fmt ioctls" - Remove functions that only did the `IS_MULTIPLANAR(vcap)` check - Assign ioctls according to device capabilities Changes in v2: - Squash commits to create multiplanar module parameter and to define the device capabilities - Move the creation of the multiplanar parameter to the end of history, so it's only added when all required changes are applied drivers/media/platform/vimc/vimc-capture.c | 78 +++++++++++++++++++--- 1 file changed, 70 insertions(+), 8 deletions(-)