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 <andrealm...@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;
>>>
>
