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@xxxxxxxxxxxxx>
>> ---
>> 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;
>>
