Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

Hi Laurent,

On 01/19/2017 12:43 PM, Todor Tomov wrote:
> Hi Laurent,
> 
> Thank you for the detailed review.
> 
> On 12/05/2016 05:22 PM, Laurent Pinchart wrote:
>> Hi Todor,
>>
>> Thank you for the patch.
>>
>> On Friday 25 Nov 2016 16:57:20 Todor Tomov wrote:
>>> These files handle the video device nodes of the camss driver.
>>
>> camss is a quite generic, I'm a bit concerned about claiming that acronym in 
>> the global kernel namespace. Would it be too long if we prefixed symbols 
>> with 
>> msm_camss instead ?
> 
> Ok. Are you concerned about camss_enable_clocks() and camss_disable_clocks() 
> or
> you have something else in mind too?

Could you please add more details about this?

> 
>>
>>> Signed-off-by: Todor Tomov 
>>> ---
>>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 ++
>>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>>>  2 files changed, 664 insertions(+)
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>>
>>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c
>>> b/drivers/media/platform/qcom/camss-8x16/video.c new file mode 100644
>>> index 000..0bf8ea9
>>> --- /dev/null
>>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c



>>> +/* 
>>> + * V4L2 file operations
>>> + */
>>> +
>>> +/*
>>> + * video_init_format - Helper function to initialize format
>>> + *
>>> + * Initialize all pad formats with default values.
>>> + */
>>> +static int video_init_format(struct file *file, void *fh)
>>> +{
>>> +   struct v4l2_format format;
>>> +
>>> +   memset(, 0, sizeof(format));
>>> +   format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +
>>> +   return video_s_fmt(file, fh, );
>>
>> This will set the active format every time you open the device node, I don't 
>> think that's what you want.
> 
> Well, actually this is what I wanted. I wanted to keep in sync the pixel 
> format
> on the video node and the media bus format on the subdev node (i.e. the pixel
> format will be always correct for the current media bus format). For the 
> current
> version there is a direct correspondence between the pixel format and the 
> media
> format so this will work I think. For the future there might be multiple pixel
> formats for one media bus format and a second open of the video node could 
> reset
> the pixel format to unwanted value so this will need a change. I'm wondering 
> about
> (and still not able to find) a good moment/event when to perform the 
> initialization
> of the format on the video node. As it gets the current format from the subdev
> node, the moment of the registration will be too early as the media link is 
> still
> not created. But after that I couldn't find a suitable callback/event where 
> to do
> it. If you can share any idea about this, please do :)

I still haven't found a better solution for this. If you have something in mind,
please share.

> 
>>
>>> +}
>>> +
>>> +static int video_open(struct file *file)
>>> +{
>>> +   struct video_device *vdev = video_devdata(file);
>>> +   struct camss_video *video = video_drvdata(file);
>>> +   struct camss_video_fh *handle;
>>> +   int ret;
>>> +
>>> +   handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>>> +   if (handle == NULL)
>>> +   return -ENOMEM;
>>> +
>>> +   v4l2_fh_init(>vfh, video->vdev);
>>> +   v4l2_fh_add(>vfh);
>>> +
>>> +   handle->video = video;
>>> +   file->private_data = >vfh;
>>> +
>>> +   ret = v4l2_pipeline_pm_use(>entity, 1);
>>> +   if (ret < 0) {
>>> +   dev_err(video->camss->dev, "Failed to power up pipeline\n");
>>> +   goto error_pm_use;
>>> +   }
>>> +
>>> +   ret = video_init_format(file, >vfh);
>>> +   if (ret < 0) {
>>> +   dev_err(video->camss->dev, "Failed to init format\n");
>>> +   goto error_init_format;
>>> +   }
>>> +
>>> +   return 0;
>>> +
>>> +error_init_format:
>>> +   v4l2_pipeline_pm_use(>entity, 0);
>>> +
>>> +error_pm_use:
>>> +   v4l2_fh_del(>vfh);
>>> +   kfree(handle);
>>> +
>>> +   return ret;
>>> +}

-- 
Best regards,
Todor Tomov

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

Hi Laurent,

On 01/19/2017 12:43 PM, Todor Tomov wrote:
> Hi Laurent,
> 
> Thank you for the detailed review.
> 
> On 12/05/2016 05:22 PM, Laurent Pinchart wrote:
>> Hi Todor,
>>
>> Thank you for the patch.
>>
>> On Friday 25 Nov 2016 16:57:20 Todor Tomov wrote:
>>> These files handle the video device nodes of the camss driver.
>>
>> camss is a quite generic, I'm a bit concerned about claiming that acronym in 
>> the global kernel namespace. Would it be too long if we prefixed symbols 
>> with 
>> msm_camss instead ?
> 
> Ok. Are you concerned about camss_enable_clocks() and camss_disable_clocks() 
> or
> you have something else in mind too?

Could you please add more details about this?

> 
>>
>>> Signed-off-by: Todor Tomov 
>>> ---
>>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 ++
>>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>>>  2 files changed, 664 insertions(+)
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>>
>>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c
>>> b/drivers/media/platform/qcom/camss-8x16/video.c new file mode 100644
>>> index 000..0bf8ea9
>>> --- /dev/null
>>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c



>>> +/* 
>>> + * V4L2 file operations
>>> + */
>>> +
>>> +/*
>>> + * video_init_format - Helper function to initialize format
>>> + *
>>> + * Initialize all pad formats with default values.
>>> + */
>>> +static int video_init_format(struct file *file, void *fh)
>>> +{
>>> +   struct v4l2_format format;
>>> +
>>> +   memset(, 0, sizeof(format));
>>> +   format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +
>>> +   return video_s_fmt(file, fh, );
>>
>> This will set the active format every time you open the device node, I don't 
>> think that's what you want.
> 
> Well, actually this is what I wanted. I wanted to keep in sync the pixel 
> format
> on the video node and the media bus format on the subdev node (i.e. the pixel
> format will be always correct for the current media bus format). For the 
> current
> version there is a direct correspondence between the pixel format and the 
> media
> format so this will work I think. For the future there might be multiple pixel
> formats for one media bus format and a second open of the video node could 
> reset
> the pixel format to unwanted value so this will need a change. I'm wondering 
> about
> (and still not able to find) a good moment/event when to perform the 
> initialization
> of the format on the video node. As it gets the current format from the subdev
> node, the moment of the registration will be too early as the media link is 
> still
> not created. But after that I couldn't find a suitable callback/event where 
> to do
> it. If you can share any idea about this, please do :)

I still haven't found a better solution for this. If you have something in mind,
please share.

> 
>>
>>> +}
>>> +
>>> +static int video_open(struct file *file)
>>> +{
>>> +   struct video_device *vdev = video_devdata(file);
>>> +   struct camss_video *video = video_drvdata(file);
>>> +   struct camss_video_fh *handle;
>>> +   int ret;
>>> +
>>> +   handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>>> +   if (handle == NULL)
>>> +   return -ENOMEM;
>>> +
>>> +   v4l2_fh_init(>vfh, video->vdev);
>>> +   v4l2_fh_add(>vfh);
>>> +
>>> +   handle->video = video;
>>> +   file->private_data = >vfh;
>>> +
>>> +   ret = v4l2_pipeline_pm_use(>entity, 1);
>>> +   if (ret < 0) {
>>> +   dev_err(video->camss->dev, "Failed to power up pipeline\n");
>>> +   goto error_pm_use;
>>> +   }
>>> +
>>> +   ret = video_init_format(file, >vfh);
>>> +   if (ret < 0) {
>>> +   dev_err(video->camss->dev, "Failed to init format\n");
>>> +   goto error_init_format;
>>> +   }
>>> +
>>> +   return 0;
>>> +
>>> +error_init_format:
>>> +   v4l2_pipeline_pm_use(>entity, 0);
>>> +
>>> +error_pm_use:
>>> +   v4l2_fh_del(>vfh);
>>> +   kfree(handle);
>>> +
>>> +   return ret;
>>> +}

-- 
Best regards,
Todor Tomov

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

Hi Laurent,

Thank you for the detailed review.

On 12/05/2016 05:22 PM, Laurent Pinchart wrote:
> Hi Todor,
> 
> Thank you for the patch.
> 
> On Friday 25 Nov 2016 16:57:20 Todor Tomov wrote:
>> These files handle the video device nodes of the camss driver.
> 
> camss is a quite generic, I'm a bit concerned about claiming that acronym in 
> the global kernel namespace. Would it be too long if we prefixed symbols with 
> msm_camss instead ?

Ok. Are you concerned about camss_enable_clocks() and camss_disable_clocks() or
you have something else in mind too?

> 
>> Signed-off-by: Todor Tomov 
>> ---
>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 ++
>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>>  2 files changed, 664 insertions(+)
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>
>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c
>> b/drivers/media/platform/qcom/camss-8x16/video.c new file mode 100644
>> index 000..0bf8ea9
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
>> @@ -0,0 +1,597 @@
>> +/*
>> + * video.c
>> + *
>> + * Qualcomm MSM Camera Subsystem - V4L2 device node
>> + *
>> + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
>> + * Copyright (C) 2015-2016 Linaro Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "video.h"
>> +#include "camss.h"
>> +
>> +/*
>> + * struct format_info - ISP media bus format information
>> + * @code: V4L2 media bus format code
>> + * @pixelformat: V4L2 pixel format FCC identifier
>> + * @bpp: Bits per pixel when stored in memory
>> + */
>> +static const struct format_info {
>> +u32 code;
>> +u32 pixelformat;
>> +unsigned int bpp;
>> +} formats[] = {
>> +{ MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY, 16 },
>> +{ MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY, 16 },
>> +{ MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV, 16 },
>> +{ MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU, 16 },
>> +{ MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8 },
>> +{ MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8 },
>> +{ MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8 },
>> +{ MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8 },
>> +{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P, 10 },
>> +{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P, 10 },
>> +{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P, 10 },
>> +{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P, 10 },
>> +{ MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 },
>> +{ MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P, 12 },
>> +{ MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P, 12 },
>> +{ MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 }
>> +};
>> +
>> +/*
>> ---
>> -- + * Helper functions
>> + */
>> +
>> +/*
>> + * video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
>> + * @mbus: v4l2_mbus_framefmt format (input)
>> + * @pix: v4l2_pix_format format (output)
>> + *
>> + * Fill the output pix structure with information from the input mbus
>> format.
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +static unsigned int video_mbus_to_pix(const struct v4l2_mbus_framefmt
>> *mbus,
>> +  struct v4l2_pix_format *pix)
>> +{
>> +unsigned int i;
>> +
>> +memset(pix, 0, sizeof(*pix));
>> +pix->width = mbus->width;
>> +pix->height = mbus->height;
>> +
>> +for (i = 0; i < ARRAY_SIZE(formats); ++i) {
>> +if (formats[i].code == mbus->code)
>> +break;
>> +}
>> +
>> +if (WARN_ON(i == ARRAY_SIZE(formats)))
>> +return -EINVAL;
>> +
>> +pix->pixelformat = formats[i].pixelformat;
>> +pix->bytesperline = pix->width * formats[i].bpp / 8;
>> +pix->bytesperline = ALIGN(pix->bytesperline, 8);
> 
> Does the hardware support padding at the end of lines ? If so it would be 
> useful to use the maximum of the computed and requested by userspace values 
> (up to the maximum size of the padding supported by the hardware).
> 
>> +pix->sizeimage = pix->bytesperline * pix->height;
> 
> Similarly, should we support padding at the end of the image ?

Yes, the 

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

Hi Laurent,

Thank you for the detailed review.

On 12/05/2016 05:22 PM, Laurent Pinchart wrote:
> Hi Todor,
> 
> Thank you for the patch.
> 
> On Friday 25 Nov 2016 16:57:20 Todor Tomov wrote:
>> These files handle the video device nodes of the camss driver.
> 
> camss is a quite generic, I'm a bit concerned about claiming that acronym in 
> the global kernel namespace. Would it be too long if we prefixed symbols with 
> msm_camss instead ?

Ok. Are you concerned about camss_enable_clocks() and camss_disable_clocks() or
you have something else in mind too?

> 
>> Signed-off-by: Todor Tomov 
>> ---
>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 ++
>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>>  2 files changed, 664 insertions(+)
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>
>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c
>> b/drivers/media/platform/qcom/camss-8x16/video.c new file mode 100644
>> index 000..0bf8ea9
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
>> @@ -0,0 +1,597 @@
>> +/*
>> + * video.c
>> + *
>> + * Qualcomm MSM Camera Subsystem - V4L2 device node
>> + *
>> + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
>> + * Copyright (C) 2015-2016 Linaro Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "video.h"
>> +#include "camss.h"
>> +
>> +/*
>> + * struct format_info - ISP media bus format information
>> + * @code: V4L2 media bus format code
>> + * @pixelformat: V4L2 pixel format FCC identifier
>> + * @bpp: Bits per pixel when stored in memory
>> + */
>> +static const struct format_info {
>> +u32 code;
>> +u32 pixelformat;
>> +unsigned int bpp;
>> +} formats[] = {
>> +{ MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY, 16 },
>> +{ MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY, 16 },
>> +{ MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV, 16 },
>> +{ MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU, 16 },
>> +{ MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8 },
>> +{ MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8 },
>> +{ MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8 },
>> +{ MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8 },
>> +{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P, 10 },
>> +{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P, 10 },
>> +{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P, 10 },
>> +{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P, 10 },
>> +{ MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 },
>> +{ MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P, 12 },
>> +{ MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P, 12 },
>> +{ MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 }
>> +};
>> +
>> +/*
>> ---
>> -- + * Helper functions
>> + */
>> +
>> +/*
>> + * video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
>> + * @mbus: v4l2_mbus_framefmt format (input)
>> + * @pix: v4l2_pix_format format (output)
>> + *
>> + * Fill the output pix structure with information from the input mbus
>> format.
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +static unsigned int video_mbus_to_pix(const struct v4l2_mbus_framefmt
>> *mbus,
>> +  struct v4l2_pix_format *pix)
>> +{
>> +unsigned int i;
>> +
>> +memset(pix, 0, sizeof(*pix));
>> +pix->width = mbus->width;
>> +pix->height = mbus->height;
>> +
>> +for (i = 0; i < ARRAY_SIZE(formats); ++i) {
>> +if (formats[i].code == mbus->code)
>> +break;
>> +}
>> +
>> +if (WARN_ON(i == ARRAY_SIZE(formats)))
>> +return -EINVAL;
>> +
>> +pix->pixelformat = formats[i].pixelformat;
>> +pix->bytesperline = pix->width * formats[i].bpp / 8;
>> +pix->bytesperline = ALIGN(pix->bytesperline, 8);
> 
> Does the hardware support padding at the end of lines ? If so it would be 
> useful to use the maximum of the computed and requested by userspace values 
> (up to the maximum size of the padding supported by the hardware).
> 
>> +pix->sizeimage = pix->bytesperline * pix->height;
> 
> Similarly, should we support padding at the end of the image ?

Yes, the hardware supports horizontal 

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

Hi Laurent, Hans,

On 12/05/2016 05:25 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 05 Dec 2016 16:02:55 Hans Verkuil wrote:
>> On 12/05/2016 03:45 PM, Laurent Pinchart wrote:
>>> On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote:
 On 11/25/2016 03:57 PM, Todor Tomov wrote:
> These files handle the video device nodes of the camss driver.
>
> Signed-off-by: Todor Tomov 
> ---
>
>  drivers/media/platform/qcom/camss-8x16/video.c | 597 +
>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>  2 files changed, 664 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>>
>>> [snip]
>>>
> +int msm_video_register(struct camss_video *video, struct v4l2_device
> *v4l2_dev,
> +const char *name)
> +{
> + struct media_pad *pad = >pad;
> + struct video_device *vdev;
> + struct vb2_queue *q;
> + int ret;
> +
> + vdev = video_device_alloc();
> + if (vdev == NULL) {
> + dev_err(v4l2_dev->dev, "Failed to allocate video
> device\n");
> + return -ENOMEM;
> + }
> +
> + video->vdev = vdev;
> +
> + q = >vb2_q;
> + q->drv_priv = video;
> + q->mem_ops = _dma_contig_memops;
> + q->ops = _video_vb2_q_ops;
> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + q->io_modes = VB2_MMAP;

 Add modes VB2_DMABUF and VB2_READ. These are for free, so why not?
 Especially DMABUF is of course very desirable to have.
>>>
>>> I certainly agree with VB2_DMABUF, but I wouldn't expose VB2_READ. read()
>>> for this kind of device is inefficient and we should encourage userspace
>>> application to move away from it (and certainly make it very clear that
>>> new applications should not use read() with this device).
>>
>> VB2_READ is very nice when debugging (no need to program a stream I/O
>> application first)
> 
> There's at least v4l2-ctl and yavta that can (and should) be used for 
> debugging ;-)
> 
>> and useful when you want to pipe the video.
> 
> Except that you lose frame boundaries. It's really a poor man's solution that 
> should never be used in any "real" application. I'd rather add an option to 
> v4l2-ctl to output the video stream to stdout (and/or stderr).
> 
>> Nobody uses read() in 'regular' applications since it is obviously
>> inefficient, but I don't see that as a reason not to offer VB2_READ.
>>
>> I don't think this is something anyone will ever abuse,
> 
> Famous last words ;-)
> 
>> and it is a nice feature to have (and for free as well).
> 

Thank you for the discussion over this. Both of you have reasonable arguments
about the read i/o.
I'd say that you cannot always save a person from himself. I'll add VB2_READ.

-- 
Best regards,
Todor Tomov

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

Hi Laurent, Hans,

On 12/05/2016 05:25 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 05 Dec 2016 16:02:55 Hans Verkuil wrote:
>> On 12/05/2016 03:45 PM, Laurent Pinchart wrote:
>>> On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote:
 On 11/25/2016 03:57 PM, Todor Tomov wrote:
> These files handle the video device nodes of the camss driver.
>
> Signed-off-by: Todor Tomov 
> ---
>
>  drivers/media/platform/qcom/camss-8x16/video.c | 597 +
>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>  2 files changed, 664 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>>
>>> [snip]
>>>
> +int msm_video_register(struct camss_video *video, struct v4l2_device
> *v4l2_dev,
> +const char *name)
> +{
> + struct media_pad *pad = >pad;
> + struct video_device *vdev;
> + struct vb2_queue *q;
> + int ret;
> +
> + vdev = video_device_alloc();
> + if (vdev == NULL) {
> + dev_err(v4l2_dev->dev, "Failed to allocate video
> device\n");
> + return -ENOMEM;
> + }
> +
> + video->vdev = vdev;
> +
> + q = >vb2_q;
> + q->drv_priv = video;
> + q->mem_ops = _dma_contig_memops;
> + q->ops = _video_vb2_q_ops;
> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + q->io_modes = VB2_MMAP;

 Add modes VB2_DMABUF and VB2_READ. These are for free, so why not?
 Especially DMABUF is of course very desirable to have.
>>>
>>> I certainly agree with VB2_DMABUF, but I wouldn't expose VB2_READ. read()
>>> for this kind of device is inefficient and we should encourage userspace
>>> application to move away from it (and certainly make it very clear that
>>> new applications should not use read() with this device).
>>
>> VB2_READ is very nice when debugging (no need to program a stream I/O
>> application first)
> 
> There's at least v4l2-ctl and yavta that can (and should) be used for 
> debugging ;-)
> 
>> and useful when you want to pipe the video.
> 
> Except that you lose frame boundaries. It's really a poor man's solution that 
> should never be used in any "real" application. I'd rather add an option to 
> v4l2-ctl to output the video stream to stdout (and/or stderr).
> 
>> Nobody uses read() in 'regular' applications since it is obviously
>> inefficient, but I don't see that as a reason not to offer VB2_READ.
>>
>> I don't think this is something anyone will ever abuse,
> 
> Famous last words ;-)
> 
>> and it is a nice feature to have (and for free as well).
> 

Thank you for the discussion over this. Both of you have reasonable arguments
about the read i/o.
I'd say that you cannot always save a person from himself. I'll add VB2_READ.

-- 
Best regards,
Todor Tomov

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

Hi Hans,

Thank you for the extensive review.

On 12/05/2016 03:44 PM, Hans Verkuil wrote:
> A few comments below:
> 
> On 11/25/2016 03:57 PM, Todor Tomov wrote:
>> These files handle the video device nodes of the camss driver.
>>
>> Signed-off-by: Todor Tomov 
>> ---
>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 
>> +
>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>>  2 files changed, 664 insertions(+)
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>
>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c 
>> b/drivers/media/platform/qcom/camss-8x16/video.c
>> new file mode 100644
>> index 000..0bf8ea9
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
>> @@ -0,0 +1,597 @@
> 
> 
> 
>> +static int video_start_streaming(struct vb2_queue *q, unsigned int count)
>> +{
>> +struct camss_video *video = vb2_get_drv_priv(q);
>> +struct video_device *vdev = video->vdev;
>> +struct media_entity *entity;
>> +struct media_pad *pad;
>> +struct v4l2_subdev *subdev;
>> +int ret;
>> +
>> +ret = media_entity_pipeline_start(>entity, >pipe);
>> +if (ret < 0)
>> +return ret;
>> +
>> +ret = video_check_format(video);
>> +if (ret < 0)
>> +goto error;
>> +
>> +entity = >entity;
>> +while (1) {
>> +pad = >pads[0];
>> +if (!(pad->flags & MEDIA_PAD_FL_SINK))
>> +break;
>> +
>> +pad = media_entity_remote_pad(pad);
>> +if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
>> +break;
>> +
>> +entity = pad->entity;
>> +subdev = media_entity_to_v4l2_subdev(entity);
>> +
>> +ret = v4l2_subdev_call(subdev, video, s_stream, 1);
>> +if (ret < 0 && ret != -ENOIOCTLCMD)
>> +goto error;
>> +}
>> +
>> +return 0;
>> +
>> +error:
>> +media_entity_pipeline_stop(>entity);
>> +
> 
> On error all queued buffers must be returned with state VB2_STATE_QUEUED.
> 
> Basically the same as 'camss_video_call(video, flush_buffers);', just with
> a different state.

Ok, I'll add this.

> 
>> +return ret;
>> +}
> 
> 
> 
>> +static int video_querycap(struct file *file, void *fh,
>> +  struct v4l2_capability *cap)
>> +{
>> +strlcpy(cap->driver, "qcom-camss", sizeof(cap->driver));
>> +strlcpy(cap->card, "Qualcomm Camera Subsystem", sizeof(cap->card));
>> +strlcpy(cap->bus_info, "platform:qcom-camss", sizeof(cap->bus_info));
>> +cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
>> +V4L2_CAP_DEVICE_CAPS;
>> +cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> 
> Don't set capabilities and device_caps here. Instead fill in the struct 
> video_device
> device_caps field and the v4l2 core will take care of cap->capabilities and
> cap->device_caps.

Ok.

> 
>> +
>> +return 0;
>> +}
>> +
>> +static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc 
>> *f)
>> +{
>> +struct camss_video *video = video_drvdata(file);
>> +struct v4l2_format format;
>> +int ret;
>> +
>> +if (f->type != video->type)
>> +return -EINVAL;
>> +
>> +if (f->index)
>> +return -EINVAL;
>> +
>> +ret = video_get_subdev_format(video, );
>> +if (ret < 0)
>> +return ret;
>> +
>> +f->pixelformat = format.fmt.pix.pixelformat;
>> +
>> +return 0;
>> +}
>> +
>> +static int video_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
>> +{
>> +struct camss_video *video = video_drvdata(file);
>> +
>> +if (f->type != video->type)
>> +return -EINVAL;
>> +
>> +*f = video->active_fmt;
>> +
>> +return 0;
>> +}
>> +
>> +static int video_s_fmt(struct file *file, void *fh, struct v4l2_format *f)
>> +{
>> +struct camss_video *video = video_drvdata(file);
>> +int ret;
>> +
>> +if (f->type != video->type)
>> +return -EINVAL;
>> +
>> +ret = video_get_subdev_format(video, f);
>> +if (ret < 0)
>> +return ret;
>> +
>> +video->active_fmt = *f;
>> +
>> +return 0;
>> +}
>> +
>> +static int video_try_fmt(struct file *file, void *fh, struct v4l2_format *f)
>> +{
>> +struct camss_video *video = video_drvdata(file);
>> +
>> +if (f->type != video->type)
>> +return -EINVAL;
>> +
>> +return video_get_subdev_format(video, f);
>> +}
>> +
>> +static int video_enum_input(struct file *file, void *fh,
>> +struct v4l2_input *input)
>> +{
>> +if (input->index > 0)
>> +return -EINVAL;
>> +
>> +strlcpy(input->name, "camera", sizeof(input->name));
>> +input->type = V4L2_INPUT_TYPE_CAMERA;
>> +
>> +return 0;
>> +}
>> +
>> +static int 

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

Hi Hans,

Thank you for the extensive review.

On 12/05/2016 03:44 PM, Hans Verkuil wrote:
> A few comments below:
> 
> On 11/25/2016 03:57 PM, Todor Tomov wrote:
>> These files handle the video device nodes of the camss driver.
>>
>> Signed-off-by: Todor Tomov 
>> ---
>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 
>> +
>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>>  2 files changed, 664 insertions(+)
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>
>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c 
>> b/drivers/media/platform/qcom/camss-8x16/video.c
>> new file mode 100644
>> index 000..0bf8ea9
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
>> @@ -0,0 +1,597 @@
> 
> 
> 
>> +static int video_start_streaming(struct vb2_queue *q, unsigned int count)
>> +{
>> +struct camss_video *video = vb2_get_drv_priv(q);
>> +struct video_device *vdev = video->vdev;
>> +struct media_entity *entity;
>> +struct media_pad *pad;
>> +struct v4l2_subdev *subdev;
>> +int ret;
>> +
>> +ret = media_entity_pipeline_start(>entity, >pipe);
>> +if (ret < 0)
>> +return ret;
>> +
>> +ret = video_check_format(video);
>> +if (ret < 0)
>> +goto error;
>> +
>> +entity = >entity;
>> +while (1) {
>> +pad = >pads[0];
>> +if (!(pad->flags & MEDIA_PAD_FL_SINK))
>> +break;
>> +
>> +pad = media_entity_remote_pad(pad);
>> +if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
>> +break;
>> +
>> +entity = pad->entity;
>> +subdev = media_entity_to_v4l2_subdev(entity);
>> +
>> +ret = v4l2_subdev_call(subdev, video, s_stream, 1);
>> +if (ret < 0 && ret != -ENOIOCTLCMD)
>> +goto error;
>> +}
>> +
>> +return 0;
>> +
>> +error:
>> +media_entity_pipeline_stop(>entity);
>> +
> 
> On error all queued buffers must be returned with state VB2_STATE_QUEUED.
> 
> Basically the same as 'camss_video_call(video, flush_buffers);', just with
> a different state.

Ok, I'll add this.

> 
>> +return ret;
>> +}
> 
> 
> 
>> +static int video_querycap(struct file *file, void *fh,
>> +  struct v4l2_capability *cap)
>> +{
>> +strlcpy(cap->driver, "qcom-camss", sizeof(cap->driver));
>> +strlcpy(cap->card, "Qualcomm Camera Subsystem", sizeof(cap->card));
>> +strlcpy(cap->bus_info, "platform:qcom-camss", sizeof(cap->bus_info));
>> +cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
>> +V4L2_CAP_DEVICE_CAPS;
>> +cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> 
> Don't set capabilities and device_caps here. Instead fill in the struct 
> video_device
> device_caps field and the v4l2 core will take care of cap->capabilities and
> cap->device_caps.

Ok.

> 
>> +
>> +return 0;
>> +}
>> +
>> +static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc 
>> *f)
>> +{
>> +struct camss_video *video = video_drvdata(file);
>> +struct v4l2_format format;
>> +int ret;
>> +
>> +if (f->type != video->type)
>> +return -EINVAL;
>> +
>> +if (f->index)
>> +return -EINVAL;
>> +
>> +ret = video_get_subdev_format(video, );
>> +if (ret < 0)
>> +return ret;
>> +
>> +f->pixelformat = format.fmt.pix.pixelformat;
>> +
>> +return 0;
>> +}
>> +
>> +static int video_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
>> +{
>> +struct camss_video *video = video_drvdata(file);
>> +
>> +if (f->type != video->type)
>> +return -EINVAL;
>> +
>> +*f = video->active_fmt;
>> +
>> +return 0;
>> +}
>> +
>> +static int video_s_fmt(struct file *file, void *fh, struct v4l2_format *f)
>> +{
>> +struct camss_video *video = video_drvdata(file);
>> +int ret;
>> +
>> +if (f->type != video->type)
>> +return -EINVAL;
>> +
>> +ret = video_get_subdev_format(video, f);
>> +if (ret < 0)
>> +return ret;
>> +
>> +video->active_fmt = *f;
>> +
>> +return 0;
>> +}
>> +
>> +static int video_try_fmt(struct file *file, void *fh, struct v4l2_format *f)
>> +{
>> +struct camss_video *video = video_drvdata(file);
>> +
>> +if (f->type != video->type)
>> +return -EINVAL;
>> +
>> +return video_get_subdev_format(video, f);
>> +}
>> +
>> +static int video_enum_input(struct file *file, void *fh,
>> +struct v4l2_input *input)
>> +{
>> +if (input->index > 0)
>> +return -EINVAL;
>> +
>> +strlcpy(input->name, "camera", sizeof(input->name));
>> +input->type = V4L2_INPUT_TYPE_CAMERA;
>> +
>> +return 0;
>> +}
>> +
>> +static int video_g_input(struct file *file, 

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Laurent Pinchart]

Hi Hans,

On Monday 05 Dec 2016 16:02:55 Hans Verkuil wrote:
> On 12/05/2016 03:45 PM, Laurent Pinchart wrote:
> > On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote:
> >> On 11/25/2016 03:57 PM, Todor Tomov wrote:
> >>> These files handle the video device nodes of the camss driver.
> >>> 
> >>> Signed-off-by: Todor Tomov 
> >>> ---
> >>> 
> >>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 +
> >>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
> >>>  2 files changed, 664 insertions(+)
> >>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
> >>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
> > 
> > [snip]
> > 
> >>> +int msm_video_register(struct camss_video *video, struct v4l2_device
> >>> *v4l2_dev,
> >>> +const char *name)
> >>> +{
> >>> + struct media_pad *pad = >pad;
> >>> + struct video_device *vdev;
> >>> + struct vb2_queue *q;
> >>> + int ret;
> >>> +
> >>> + vdev = video_device_alloc();
> >>> + if (vdev == NULL) {
> >>> + dev_err(v4l2_dev->dev, "Failed to allocate video
> >>> device\n");
> >>> + return -ENOMEM;
> >>> + }
> >>> +
> >>> + video->vdev = vdev;
> >>> +
> >>> + q = >vb2_q;
> >>> + q->drv_priv = video;
> >>> + q->mem_ops = _dma_contig_memops;
> >>> + q->ops = _video_vb2_q_ops;
> >>> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >>> + q->io_modes = VB2_MMAP;
> >> 
> >> Add modes VB2_DMABUF and VB2_READ. These are for free, so why not?
> >> Especially DMABUF is of course very desirable to have.
> > 
> > I certainly agree with VB2_DMABUF, but I wouldn't expose VB2_READ. read()
> > for this kind of device is inefficient and we should encourage userspace
> > application to move away from it (and certainly make it very clear that
> > new applications should not use read() with this device).
> 
> VB2_READ is very nice when debugging (no need to program a stream I/O
> application first)

There's at least v4l2-ctl and yavta that can (and should) be used for 
debugging ;-)

> and useful when you want to pipe the video.

Except that you lose frame boundaries. It's really a poor man's solution that 
should never be used in any "real" application. I'd rather add an option to 
v4l2-ctl to output the video stream to stdout (and/or stderr).

> Nobody uses read() in 'regular' applications since it is obviously
> inefficient, but I don't see that as a reason not to offer VB2_READ.
> 
> I don't think this is something anyone will ever abuse,

Famous last words ;-)

> and it is a nice feature to have (and for free as well).

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Laurent Pinchart]

Hi Hans,

On Monday 05 Dec 2016 16:02:55 Hans Verkuil wrote:
> On 12/05/2016 03:45 PM, Laurent Pinchart wrote:
> > On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote:
> >> On 11/25/2016 03:57 PM, Todor Tomov wrote:
> >>> These files handle the video device nodes of the camss driver.
> >>> 
> >>> Signed-off-by: Todor Tomov 
> >>> ---
> >>> 
> >>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 +
> >>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
> >>>  2 files changed, 664 insertions(+)
> >>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
> >>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
> > 
> > [snip]
> > 
> >>> +int msm_video_register(struct camss_video *video, struct v4l2_device
> >>> *v4l2_dev,
> >>> +const char *name)
> >>> +{
> >>> + struct media_pad *pad = >pad;
> >>> + struct video_device *vdev;
> >>> + struct vb2_queue *q;
> >>> + int ret;
> >>> +
> >>> + vdev = video_device_alloc();
> >>> + if (vdev == NULL) {
> >>> + dev_err(v4l2_dev->dev, "Failed to allocate video
> >>> device\n");
> >>> + return -ENOMEM;
> >>> + }
> >>> +
> >>> + video->vdev = vdev;
> >>> +
> >>> + q = >vb2_q;
> >>> + q->drv_priv = video;
> >>> + q->mem_ops = _dma_contig_memops;
> >>> + q->ops = _video_vb2_q_ops;
> >>> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >>> + q->io_modes = VB2_MMAP;
> >> 
> >> Add modes VB2_DMABUF and VB2_READ. These are for free, so why not?
> >> Especially DMABUF is of course very desirable to have.
> > 
> > I certainly agree with VB2_DMABUF, but I wouldn't expose VB2_READ. read()
> > for this kind of device is inefficient and we should encourage userspace
> > application to move away from it (and certainly make it very clear that
> > new applications should not use read() with this device).
> 
> VB2_READ is very nice when debugging (no need to program a stream I/O
> application first)

There's at least v4l2-ctl and yavta that can (and should) be used for 
debugging ;-)

> and useful when you want to pipe the video.

Except that you lose frame boundaries. It's really a poor man's solution that 
should never be used in any "real" application. I'd rather add an option to 
v4l2-ctl to output the video stream to stdout (and/or stderr).

> Nobody uses read() in 'regular' applications since it is obviously
> inefficient, but I don't see that as a reason not to offer VB2_READ.
> 
> I don't think this is something anyone will ever abuse,

Famous last words ;-)

> and it is a nice feature to have (and for free as well).

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Laurent Pinchart]

Hi Todor,

Thank you for the patch.

On Friday 25 Nov 2016 16:57:20 Todor Tomov wrote:
> These files handle the video device nodes of the camss driver.

camss is a quite generic, I'm a bit concerned about claiming that acronym in 
the global kernel namespace. Would it be too long if we prefixed symbols with 
msm_camss instead ?

> Signed-off-by: Todor Tomov 
> ---
>  drivers/media/platform/qcom/camss-8x16/video.c | 597 ++
>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>  2 files changed, 664 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
> 
> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c
> b/drivers/media/platform/qcom/camss-8x16/video.c new file mode 100644
> index 000..0bf8ea9
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
> @@ -0,0 +1,597 @@
> +/*
> + * video.c
> + *
> + * Qualcomm MSM Camera Subsystem - V4L2 device node
> + *
> + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2015-2016 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "video.h"
> +#include "camss.h"
> +
> +/*
> + * struct format_info - ISP media bus format information
> + * @code: V4L2 media bus format code
> + * @pixelformat: V4L2 pixel format FCC identifier
> + * @bpp: Bits per pixel when stored in memory
> + */
> +static const struct format_info {
> + u32 code;
> + u32 pixelformat;
> + unsigned int bpp;
> +} formats[] = {
> + { MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY, 16 },
> + { MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY, 16 },
> + { MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV, 16 },
> + { MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU, 16 },
> + { MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8 },
> + { MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8 },
> + { MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8 },
> + { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8 },
> + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P, 10 },
> + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P, 10 },
> + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P, 10 },
> + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P, 10 },
> + { MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 },
> + { MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P, 12 },
> + { MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P, 12 },
> + { MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 }
> +};
> +
> +/*
> ---
> -- + * Helper functions
> + */
> +
> +/*
> + * video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
> + * @mbus: v4l2_mbus_framefmt format (input)
> + * @pix: v4l2_pix_format format (output)
> + *
> + * Fill the output pix structure with information from the input mbus
> format.
> + *
> + * Return 0 on success or a negative error code otherwise
> + */
> +static unsigned int video_mbus_to_pix(const struct v4l2_mbus_framefmt
> *mbus,
> +   struct v4l2_pix_format *pix)
> +{
> + unsigned int i;
> +
> + memset(pix, 0, sizeof(*pix));
> + pix->width = mbus->width;
> + pix->height = mbus->height;
> +
> + for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> + if (formats[i].code == mbus->code)
> + break;
> + }
> +
> + if (WARN_ON(i == ARRAY_SIZE(formats)))
> + return -EINVAL;
> +
> + pix->pixelformat = formats[i].pixelformat;
> + pix->bytesperline = pix->width * formats[i].bpp / 8;
> + pix->bytesperline = ALIGN(pix->bytesperline, 8);

Does the hardware support padding at the end of lines ? If so it would be 
useful to use the maximum of the computed and requested by userspace values 
(up to the maximum size of the padding supported by the hardware).

> + pix->sizeimage = pix->bytesperline * pix->height;

Similarly, should we support padding at the end of the image ?

> + pix->colorspace = mbus->colorspace;
> + pix->field = mbus->field;
> +
> + return 0;
> +}
> +
> +static struct v4l2_subdev *video_remote_subdev(struct camss_video *video,
> +u32 *pad)
> +{
> + struct media_pad *remote;
> +
> + remote = media_entity_remote_pad(>pad);
> +

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Laurent Pinchart]

Hi Todor,

Thank you for the patch.

On Friday 25 Nov 2016 16:57:20 Todor Tomov wrote:
> These files handle the video device nodes of the camss driver.

camss is a quite generic, I'm a bit concerned about claiming that acronym in 
the global kernel namespace. Would it be too long if we prefixed symbols with 
msm_camss instead ?

> Signed-off-by: Todor Tomov 
> ---
>  drivers/media/platform/qcom/camss-8x16/video.c | 597 ++
>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>  2 files changed, 664 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
> 
> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c
> b/drivers/media/platform/qcom/camss-8x16/video.c new file mode 100644
> index 000..0bf8ea9
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
> @@ -0,0 +1,597 @@
> +/*
> + * video.c
> + *
> + * Qualcomm MSM Camera Subsystem - V4L2 device node
> + *
> + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2015-2016 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "video.h"
> +#include "camss.h"
> +
> +/*
> + * struct format_info - ISP media bus format information
> + * @code: V4L2 media bus format code
> + * @pixelformat: V4L2 pixel format FCC identifier
> + * @bpp: Bits per pixel when stored in memory
> + */
> +static const struct format_info {
> + u32 code;
> + u32 pixelformat;
> + unsigned int bpp;
> +} formats[] = {
> + { MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY, 16 },
> + { MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY, 16 },
> + { MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV, 16 },
> + { MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU, 16 },
> + { MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8 },
> + { MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8 },
> + { MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8 },
> + { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8 },
> + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P, 10 },
> + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P, 10 },
> + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P, 10 },
> + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P, 10 },
> + { MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 },
> + { MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P, 12 },
> + { MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P, 12 },
> + { MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 }
> +};
> +
> +/*
> ---
> -- + * Helper functions
> + */
> +
> +/*
> + * video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
> + * @mbus: v4l2_mbus_framefmt format (input)
> + * @pix: v4l2_pix_format format (output)
> + *
> + * Fill the output pix structure with information from the input mbus
> format.
> + *
> + * Return 0 on success or a negative error code otherwise
> + */
> +static unsigned int video_mbus_to_pix(const struct v4l2_mbus_framefmt
> *mbus,
> +   struct v4l2_pix_format *pix)
> +{
> + unsigned int i;
> +
> + memset(pix, 0, sizeof(*pix));
> + pix->width = mbus->width;
> + pix->height = mbus->height;
> +
> + for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> + if (formats[i].code == mbus->code)
> + break;
> + }
> +
> + if (WARN_ON(i == ARRAY_SIZE(formats)))
> + return -EINVAL;
> +
> + pix->pixelformat = formats[i].pixelformat;
> + pix->bytesperline = pix->width * formats[i].bpp / 8;
> + pix->bytesperline = ALIGN(pix->bytesperline, 8);

Does the hardware support padding at the end of lines ? If so it would be 
useful to use the maximum of the computed and requested by userspace values 
(up to the maximum size of the padding supported by the hardware).

> + pix->sizeimage = pix->bytesperline * pix->height;

Similarly, should we support padding at the end of the image ?

> + pix->colorspace = mbus->colorspace;
> + pix->field = mbus->field;
> +
> + return 0;
> +}
> +
> +static struct v4l2_subdev *video_remote_subdev(struct camss_video *video,
> +u32 *pad)
> +{
> + struct media_pad *remote;
> +
> + remote = media_entity_remote_pad(>pad);
> +
> + if (!remote || 

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Hans Verkuil]

On 12/05/2016 03:45 PM, Laurent Pinchart wrote:
> Hello,
> 
> On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote:
>> On 11/25/2016 03:57 PM, Todor Tomov wrote:
>>> These files handle the video device nodes of the camss driver.
>>>
>>> Signed-off-by: Todor Tomov 
>>> ---
>>>
>>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 
>>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>>>  2 files changed, 664 insertions(+)
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
> 
> [snip]
> 
>>> +int msm_video_register(struct camss_video *video, struct v4l2_device
>>> *v4l2_dev,
>>> +const char *name)
>>> +{
>>> + struct media_pad *pad = >pad;
>>> + struct video_device *vdev;
>>> + struct vb2_queue *q;
>>> + int ret;
>>> +
>>> + vdev = video_device_alloc();
>>> + if (vdev == NULL) {
>>> + dev_err(v4l2_dev->dev, "Failed to allocate video device\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + video->vdev = vdev;
>>> +
>>> + q = >vb2_q;
>>> + q->drv_priv = video;
>>> + q->mem_ops = _dma_contig_memops;
>>> + q->ops = _video_vb2_q_ops;
>>> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> + q->io_modes = VB2_MMAP;
>>
>> Add modes VB2_DMABUF and VB2_READ. These are for free, so why not?
>> Especially DMABUF is of course very desirable to have.
> 
> I certainly agree with VB2_DMABUF, but I wouldn't expose VB2_READ. read() for 
> this kind of device is inefficient and we should encourage userspace 
> application to move away from it (and certainly make it very clear that new 
> applications should not use read() with this device).

VB2_READ is very nice when debugging (no need to program a stream I/O 
application first)
and useful when you want to pipe the video.

Nobody uses read() in 'regular' applications since it is obviously inefficient, 
but
I don't see that as a reason not to offer VB2_READ.

I don't think this is something anyone will ever abuse, and it is a nice feature
to have (and for free as well).

Regards,

Hans

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Hans Verkuil]

On 12/05/2016 03:45 PM, Laurent Pinchart wrote:
> Hello,
> 
> On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote:
>> On 11/25/2016 03:57 PM, Todor Tomov wrote:
>>> These files handle the video device nodes of the camss driver.
>>>
>>> Signed-off-by: Todor Tomov 
>>> ---
>>>
>>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 
>>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>>>  2 files changed, 664 insertions(+)
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
> 
> [snip]
> 
>>> +int msm_video_register(struct camss_video *video, struct v4l2_device
>>> *v4l2_dev,
>>> +const char *name)
>>> +{
>>> + struct media_pad *pad = >pad;
>>> + struct video_device *vdev;
>>> + struct vb2_queue *q;
>>> + int ret;
>>> +
>>> + vdev = video_device_alloc();
>>> + if (vdev == NULL) {
>>> + dev_err(v4l2_dev->dev, "Failed to allocate video device\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + video->vdev = vdev;
>>> +
>>> + q = >vb2_q;
>>> + q->drv_priv = video;
>>> + q->mem_ops = _dma_contig_memops;
>>> + q->ops = _video_vb2_q_ops;
>>> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> + q->io_modes = VB2_MMAP;
>>
>> Add modes VB2_DMABUF and VB2_READ. These are for free, so why not?
>> Especially DMABUF is of course very desirable to have.
> 
> I certainly agree with VB2_DMABUF, but I wouldn't expose VB2_READ. read() for 
> this kind of device is inefficient and we should encourage userspace 
> application to move away from it (and certainly make it very clear that new 
> applications should not use read() with this device).

VB2_READ is very nice when debugging (no need to program a stream I/O 
application first)
and useful when you want to pipe the video.

Nobody uses read() in 'regular' applications since it is obviously inefficient, 
but
I don't see that as a reason not to offer VB2_READ.

I don't think this is something anyone will ever abuse, and it is a nice feature
to have (and for free as well).

Regards,

Hans

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Laurent Pinchart]

Hello,

On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote:
> On 11/25/2016 03:57 PM, Todor Tomov wrote:
> > These files handle the video device nodes of the camss driver.
> >
> > Signed-off-by: Todor Tomov 
> > ---
> >
> >  drivers/media/platform/qcom/camss-8x16/video.c | 597 
> >  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
> >  2 files changed, 664 insertions(+)
> >  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
> >  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h

[snip]

> > +int msm_video_register(struct camss_video *video, struct v4l2_device
> > *v4l2_dev,
> > +const char *name)
> > +{
> > + struct media_pad *pad = >pad;
> > + struct video_device *vdev;
> > + struct vb2_queue *q;
> > + int ret;
> > +
> > + vdev = video_device_alloc();
> > + if (vdev == NULL) {
> > + dev_err(v4l2_dev->dev, "Failed to allocate video device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + video->vdev = vdev;
> > +
> > + q = >vb2_q;
> > + q->drv_priv = video;
> > + q->mem_ops = _dma_contig_memops;
> > + q->ops = _video_vb2_q_ops;
> > + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > + q->io_modes = VB2_MMAP;
> 
> Add modes VB2_DMABUF and VB2_READ. These are for free, so why not?
> Especially DMABUF is of course very desirable to have.

I certainly agree with VB2_DMABUF, but I wouldn't expose VB2_READ. read() for 
this kind of device is inefficient and we should encourage userspace 
application to move away from it (and certainly make it very clear that new 
applications should not use read() with this device).

> > + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > + q->buf_struct_size = sizeof(struct camss_buffer);
> > + q->dev = video->camss->dev;
> > + ret = vb2_queue_init(q);
> > + if (ret < 0) {
> > + dev_err(v4l2_dev->dev, "Failed to init vb2 queue\n");
> > + return ret;
> > + }
> > +
> > + pad->flags = MEDIA_PAD_FL_SINK;
> > + ret = media_entity_pads_init(>entity, 1, pad);
> > + if (ret < 0) {
> > + dev_err(v4l2_dev->dev, "Failed to init video entity\n");
> > + goto error_media_init;
> > + }
> > +
> > + vdev->fops = _vid_fops;
> > + vdev->ioctl_ops = _vid_ioctl_ops;
> > + vdev->release = video_device_release;
> > + vdev->v4l2_dev = v4l2_dev;
> > + vdev->vfl_dir = VFL_DIR_RX;
> > + vdev->queue = >vb2_q;
> 
> As mentioned in querycap: set vdev->device_caps here.
> 
> > + strlcpy(vdev->name, name, sizeof(vdev->name));
> > +
> > + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> > + if (ret < 0) {
> > + dev_err(v4l2_dev->dev, "Failed to register video device\n");
> > + goto error_video_register;
> > + }
> > +
> > + video_set_drvdata(vdev, video);
> > +
> > + return 0;
> > +
> > +error_video_register:
> > + media_entity_cleanup(>entity);
> > +error_media_init:
> > + vb2_queue_release(>vb2_q);
> > +
> > + return ret;
> > +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Laurent Pinchart]

Hello,

On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote:
> On 11/25/2016 03:57 PM, Todor Tomov wrote:
> > These files handle the video device nodes of the camss driver.
> >
> > Signed-off-by: Todor Tomov 
> > ---
> >
> >  drivers/media/platform/qcom/camss-8x16/video.c | 597 
> >  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
> >  2 files changed, 664 insertions(+)
> >  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
> >  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h

[snip]

> > +int msm_video_register(struct camss_video *video, struct v4l2_device
> > *v4l2_dev,
> > +const char *name)
> > +{
> > + struct media_pad *pad = >pad;
> > + struct video_device *vdev;
> > + struct vb2_queue *q;
> > + int ret;
> > +
> > + vdev = video_device_alloc();
> > + if (vdev == NULL) {
> > + dev_err(v4l2_dev->dev, "Failed to allocate video device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + video->vdev = vdev;
> > +
> > + q = >vb2_q;
> > + q->drv_priv = video;
> > + q->mem_ops = _dma_contig_memops;
> > + q->ops = _video_vb2_q_ops;
> > + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > + q->io_modes = VB2_MMAP;
> 
> Add modes VB2_DMABUF and VB2_READ. These are for free, so why not?
> Especially DMABUF is of course very desirable to have.

I certainly agree with VB2_DMABUF, but I wouldn't expose VB2_READ. read() for 
this kind of device is inefficient and we should encourage userspace 
application to move away from it (and certainly make it very clear that new 
applications should not use read() with this device).

> > + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > + q->buf_struct_size = sizeof(struct camss_buffer);
> > + q->dev = video->camss->dev;
> > + ret = vb2_queue_init(q);
> > + if (ret < 0) {
> > + dev_err(v4l2_dev->dev, "Failed to init vb2 queue\n");
> > + return ret;
> > + }
> > +
> > + pad->flags = MEDIA_PAD_FL_SINK;
> > + ret = media_entity_pads_init(>entity, 1, pad);
> > + if (ret < 0) {
> > + dev_err(v4l2_dev->dev, "Failed to init video entity\n");
> > + goto error_media_init;
> > + }
> > +
> > + vdev->fops = _vid_fops;
> > + vdev->ioctl_ops = _vid_ioctl_ops;
> > + vdev->release = video_device_release;
> > + vdev->v4l2_dev = v4l2_dev;
> > + vdev->vfl_dir = VFL_DIR_RX;
> > + vdev->queue = >vb2_q;
> 
> As mentioned in querycap: set vdev->device_caps here.
> 
> > + strlcpy(vdev->name, name, sizeof(vdev->name));
> > +
> > + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> > + if (ret < 0) {
> > + dev_err(v4l2_dev->dev, "Failed to register video device\n");
> > + goto error_video_register;
> > + }
> > +
> > + video_set_drvdata(vdev, video);
> > +
> > + return 0;
> > +
> > +error_video_register:
> > + media_entity_cleanup(>entity);
> > +error_media_init:
> > + vb2_queue_release(>vb2_q);
> > +
> > + return ret;
> > +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Laurent Pinchart]

Hi Hans,

On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote:
> > +static int video_querycap(struct file *file, void *fh,
> > +   struct v4l2_capability *cap)
> > +{
> > + strlcpy(cap->driver, "qcom-camss", sizeof(cap->driver));
> > + strlcpy(cap->card, "Qualcomm Camera Subsystem", sizeof(cap->card));
> > + strlcpy(cap->bus_info, "platform:qcom-camss",
> > sizeof(cap->bus_info));
> > + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
> > + V4L2_CAP_DEVICE_CAPS
> > ;
> > + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> 
> Don't set capabilities and device_caps here. Instead fill in the struct
> video_device device_caps field and the v4l2 core will take care of
> cap->capabilities and cap->device_caps.

Time to add this to checkpatch.pl ? :-)

> > +
> > + return 0;
> > +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Laurent Pinchart]

Hi Hans,

On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote:
> > +static int video_querycap(struct file *file, void *fh,
> > +   struct v4l2_capability *cap)
> > +{
> > + strlcpy(cap->driver, "qcom-camss", sizeof(cap->driver));
> > + strlcpy(cap->card, "Qualcomm Camera Subsystem", sizeof(cap->card));
> > + strlcpy(cap->bus_info, "platform:qcom-camss",
> > sizeof(cap->bus_info));
> > + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
> > + V4L2_CAP_DEVICE_CAPS
> > ;
> > + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> 
> Don't set capabilities and device_caps here. Instead fill in the struct
> video_device device_caps field and the v4l2 core will take care of
> cap->capabilities and cap->device_caps.

Time to add this to checkpatch.pl ? :-)

> > +
> > + return 0;
> > +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Hans Verkuil]

A few comments below:

On 11/25/2016 03:57 PM, Todor Tomov wrote:
> These files handle the video device nodes of the camss driver.
> 
> Signed-off-by: Todor Tomov 
> ---
>  drivers/media/platform/qcom/camss-8x16/video.c | 597 
> +
>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>  2 files changed, 664 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
> 
> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c 
> b/drivers/media/platform/qcom/camss-8x16/video.c
> new file mode 100644
> index 000..0bf8ea9
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
> @@ -0,0 +1,597 @@



> +static int video_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> + struct camss_video *video = vb2_get_drv_priv(q);
> + struct video_device *vdev = video->vdev;
> + struct media_entity *entity;
> + struct media_pad *pad;
> + struct v4l2_subdev *subdev;
> + int ret;
> +
> + ret = media_entity_pipeline_start(>entity, >pipe);
> + if (ret < 0)
> + return ret;
> +
> + ret = video_check_format(video);
> + if (ret < 0)
> + goto error;
> +
> + entity = >entity;
> + while (1) {
> + pad = >pads[0];
> + if (!(pad->flags & MEDIA_PAD_FL_SINK))
> + break;
> +
> + pad = media_entity_remote_pad(pad);
> + if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> + break;
> +
> + entity = pad->entity;
> + subdev = media_entity_to_v4l2_subdev(entity);
> +
> + ret = v4l2_subdev_call(subdev, video, s_stream, 1);
> + if (ret < 0 && ret != -ENOIOCTLCMD)
> + goto error;
> + }
> +
> + return 0;
> +
> +error:
> + media_entity_pipeline_stop(>entity);
> +

On error all queued buffers must be returned with state VB2_STATE_QUEUED.

Basically the same as 'camss_video_call(video, flush_buffers);', just with
a different state.

> + return ret;
> +}



> +static int video_querycap(struct file *file, void *fh,
> +   struct v4l2_capability *cap)
> +{
> + strlcpy(cap->driver, "qcom-camss", sizeof(cap->driver));
> + strlcpy(cap->card, "Qualcomm Camera Subsystem", sizeof(cap->card));
> + strlcpy(cap->bus_info, "platform:qcom-camss", sizeof(cap->bus_info));
> + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
> + V4L2_CAP_DEVICE_CAPS;
> + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;

Don't set capabilities and device_caps here. Instead fill in the struct 
video_device
device_caps field and the v4l2 core will take care of cap->capabilities and
cap->device_caps.

> +
> + return 0;
> +}
> +
> +static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc 
> *f)
> +{
> + struct camss_video *video = video_drvdata(file);
> + struct v4l2_format format;
> + int ret;
> +
> + if (f->type != video->type)
> + return -EINVAL;
> +
> + if (f->index)
> + return -EINVAL;
> +
> + ret = video_get_subdev_format(video, );
> + if (ret < 0)
> + return ret;
> +
> + f->pixelformat = format.fmt.pix.pixelformat;
> +
> + return 0;
> +}
> +
> +static int video_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
> +{
> + struct camss_video *video = video_drvdata(file);
> +
> + if (f->type != video->type)
> + return -EINVAL;
> +
> + *f = video->active_fmt;
> +
> + return 0;
> +}
> +
> +static int video_s_fmt(struct file *file, void *fh, struct v4l2_format *f)
> +{
> + struct camss_video *video = video_drvdata(file);
> + int ret;
> +
> + if (f->type != video->type)
> + return -EINVAL;
> +
> + ret = video_get_subdev_format(video, f);
> + if (ret < 0)
> + return ret;
> +
> + video->active_fmt = *f;
> +
> + return 0;
> +}
> +
> +static int video_try_fmt(struct file *file, void *fh, struct v4l2_format *f)
> +{
> + struct camss_video *video = video_drvdata(file);
> +
> + if (f->type != video->type)
> + return -EINVAL;
> +
> + return video_get_subdev_format(video, f);
> +}
> +
> +static int video_enum_input(struct file *file, void *fh,
> + struct v4l2_input *input)
> +{
> + if (input->index > 0)
> + return -EINVAL;
> +
> + strlcpy(input->name, "camera", sizeof(input->name));
> + input->type = V4L2_INPUT_TYPE_CAMERA;
> +
> + return 0;
> +}
> +
> +static int video_g_input(struct file *file, void *fh, unsigned int *input)
> +{
> + *input = 0;
> +
> + return 0;
> +}
> +
> +static int video_s_input(struct file *file, void *fh, unsigned int input)
> +{
> + return input == 0 ? 0 : -EINVAL;

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Hans Verkuil]

A few comments below:

On 11/25/2016 03:57 PM, Todor Tomov wrote:
> These files handle the video device nodes of the camss driver.
> 
> Signed-off-by: Todor Tomov 
> ---
>  drivers/media/platform/qcom/camss-8x16/video.c | 597 
> +
>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>  2 files changed, 664 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
> 
> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c 
> b/drivers/media/platform/qcom/camss-8x16/video.c
> new file mode 100644
> index 000..0bf8ea9
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
> @@ -0,0 +1,597 @@



> +static int video_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> + struct camss_video *video = vb2_get_drv_priv(q);
> + struct video_device *vdev = video->vdev;
> + struct media_entity *entity;
> + struct media_pad *pad;
> + struct v4l2_subdev *subdev;
> + int ret;
> +
> + ret = media_entity_pipeline_start(>entity, >pipe);
> + if (ret < 0)
> + return ret;
> +
> + ret = video_check_format(video);
> + if (ret < 0)
> + goto error;
> +
> + entity = >entity;
> + while (1) {
> + pad = >pads[0];
> + if (!(pad->flags & MEDIA_PAD_FL_SINK))
> + break;
> +
> + pad = media_entity_remote_pad(pad);
> + if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> + break;
> +
> + entity = pad->entity;
> + subdev = media_entity_to_v4l2_subdev(entity);
> +
> + ret = v4l2_subdev_call(subdev, video, s_stream, 1);
> + if (ret < 0 && ret != -ENOIOCTLCMD)
> + goto error;
> + }
> +
> + return 0;
> +
> +error:
> + media_entity_pipeline_stop(>entity);
> +

On error all queued buffers must be returned with state VB2_STATE_QUEUED.

Basically the same as 'camss_video_call(video, flush_buffers);', just with
a different state.

> + return ret;
> +}



> +static int video_querycap(struct file *file, void *fh,
> +   struct v4l2_capability *cap)
> +{
> + strlcpy(cap->driver, "qcom-camss", sizeof(cap->driver));
> + strlcpy(cap->card, "Qualcomm Camera Subsystem", sizeof(cap->card));
> + strlcpy(cap->bus_info, "platform:qcom-camss", sizeof(cap->bus_info));
> + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
> + V4L2_CAP_DEVICE_CAPS;
> + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;

Don't set capabilities and device_caps here. Instead fill in the struct 
video_device
device_caps field and the v4l2 core will take care of cap->capabilities and
cap->device_caps.

> +
> + return 0;
> +}
> +
> +static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc 
> *f)
> +{
> + struct camss_video *video = video_drvdata(file);
> + struct v4l2_format format;
> + int ret;
> +
> + if (f->type != video->type)
> + return -EINVAL;
> +
> + if (f->index)
> + return -EINVAL;
> +
> + ret = video_get_subdev_format(video, );
> + if (ret < 0)
> + return ret;
> +
> + f->pixelformat = format.fmt.pix.pixelformat;
> +
> + return 0;
> +}
> +
> +static int video_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
> +{
> + struct camss_video *video = video_drvdata(file);
> +
> + if (f->type != video->type)
> + return -EINVAL;
> +
> + *f = video->active_fmt;
> +
> + return 0;
> +}
> +
> +static int video_s_fmt(struct file *file, void *fh, struct v4l2_format *f)
> +{
> + struct camss_video *video = video_drvdata(file);
> + int ret;
> +
> + if (f->type != video->type)
> + return -EINVAL;
> +
> + ret = video_get_subdev_format(video, f);
> + if (ret < 0)
> + return ret;
> +
> + video->active_fmt = *f;
> +
> + return 0;
> +}
> +
> +static int video_try_fmt(struct file *file, void *fh, struct v4l2_format *f)
> +{
> + struct camss_video *video = video_drvdata(file);
> +
> + if (f->type != video->type)
> + return -EINVAL;
> +
> + return video_get_subdev_format(video, f);
> +}
> +
> +static int video_enum_input(struct file *file, void *fh,
> + struct v4l2_input *input)
> +{
> + if (input->index > 0)
> + return -EINVAL;
> +
> + strlcpy(input->name, "camera", sizeof(input->name));
> + input->type = V4L2_INPUT_TYPE_CAMERA;
> +
> + return 0;
> +}
> +
> +static int video_g_input(struct file *file, void *fh, unsigned int *input)
> +{
> + *input = 0;
> +
> + return 0;
> +}
> +
> +static int video_s_input(struct file *file, void *fh, unsigned int input)
> +{
> + return input == 0 ? 0 : -EINVAL;
> +}
> +
> +static 

[PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

These files handle the video device nodes of the camss driver.

Signed-off-by: Todor Tomov 
---
 drivers/media/platform/qcom/camss-8x16/video.c | 597 +
 drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
 2 files changed, 664 insertions(+)
 create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
 create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h

diff --git a/drivers/media/platform/qcom/camss-8x16/video.c 
b/drivers/media/platform/qcom/camss-8x16/video.c
new file mode 100644
index 000..0bf8ea9
--- /dev/null
+++ b/drivers/media/platform/qcom/camss-8x16/video.c
@@ -0,0 +1,597 @@
+/*
+ * video.c
+ *
+ * Qualcomm MSM Camera Subsystem - V4L2 device node
+ *
+ * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2015-2016 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "video.h"
+#include "camss.h"
+
+/*
+ * struct format_info - ISP media bus format information
+ * @code: V4L2 media bus format code
+ * @pixelformat: V4L2 pixel format FCC identifier
+ * @bpp: Bits per pixel when stored in memory
+ */
+static const struct format_info {
+   u32 code;
+   u32 pixelformat;
+   unsigned int bpp;
+} formats[] = {
+   { MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY, 16 },
+   { MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY, 16 },
+   { MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV, 16 },
+   { MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU, 16 },
+   { MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8 },
+   { MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8 },
+   { MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8 },
+   { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8 },
+   { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P, 10 },
+   { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P, 10 },
+   { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P, 10 },
+   { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P, 10 },
+   { MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 },
+   { MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P, 12 },
+   { MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P, 12 },
+   { MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 }
+};
+
+/* 
-
+ * Helper functions
+ */
+
+/*
+ * video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
+ * @mbus: v4l2_mbus_framefmt format (input)
+ * @pix: v4l2_pix_format format (output)
+ *
+ * Fill the output pix structure with information from the input mbus format.
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static unsigned int video_mbus_to_pix(const struct v4l2_mbus_framefmt *mbus,
+ struct v4l2_pix_format *pix)
+{
+   unsigned int i;
+
+   memset(pix, 0, sizeof(*pix));
+   pix->width = mbus->width;
+   pix->height = mbus->height;
+
+   for (i = 0; i < ARRAY_SIZE(formats); ++i) {
+   if (formats[i].code == mbus->code)
+   break;
+   }
+
+   if (WARN_ON(i == ARRAY_SIZE(formats)))
+   return -EINVAL;
+
+   pix->pixelformat = formats[i].pixelformat;
+   pix->bytesperline = pix->width * formats[i].bpp / 8;
+   pix->bytesperline = ALIGN(pix->bytesperline, 8);
+   pix->sizeimage = pix->bytesperline * pix->height;
+   pix->colorspace = mbus->colorspace;
+   pix->field = mbus->field;
+
+   return 0;
+}
+
+static struct v4l2_subdev *video_remote_subdev(struct camss_video *video,
+  u32 *pad)
+{
+   struct media_pad *remote;
+
+   remote = media_entity_remote_pad(>pad);
+
+   if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
+   return NULL;
+
+   if (pad)
+   *pad = remote->index;
+
+   return media_entity_to_v4l2_subdev(remote->entity);
+}
+
+static int video_get_subdev_format(struct camss_video *video,
+  struct v4l2_format *format)
+{
+   struct v4l2_subdev_format fmt;
+   struct v4l2_subdev *subdev;
+   u32 pad;
+   int ret;
+
+   subdev = video_remote_subdev(video, );
+   if (subdev == NULL)
+   return -EINVAL;
+
+   fmt.pad = pad;
+   fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+
+   ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, );
+ 

[PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

These files handle the video device nodes of the camss driver.

Signed-off-by: Todor Tomov 
---
 drivers/media/platform/qcom/camss-8x16/video.c | 597 +
 drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
 2 files changed, 664 insertions(+)
 create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
 create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h

diff --git a/drivers/media/platform/qcom/camss-8x16/video.c 
b/drivers/media/platform/qcom/camss-8x16/video.c
new file mode 100644
index 000..0bf8ea9
--- /dev/null
+++ b/drivers/media/platform/qcom/camss-8x16/video.c
@@ -0,0 +1,597 @@
+/*
+ * video.c
+ *
+ * Qualcomm MSM Camera Subsystem - V4L2 device node
+ *
+ * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2015-2016 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "video.h"
+#include "camss.h"
+
+/*
+ * struct format_info - ISP media bus format information
+ * @code: V4L2 media bus format code
+ * @pixelformat: V4L2 pixel format FCC identifier
+ * @bpp: Bits per pixel when stored in memory
+ */
+static const struct format_info {
+   u32 code;
+   u32 pixelformat;
+   unsigned int bpp;
+} formats[] = {
+   { MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY, 16 },
+   { MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY, 16 },
+   { MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV, 16 },
+   { MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU, 16 },
+   { MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8 },
+   { MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8 },
+   { MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8 },
+   { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8 },
+   { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P, 10 },
+   { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P, 10 },
+   { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P, 10 },
+   { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P, 10 },
+   { MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 },
+   { MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P, 12 },
+   { MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P, 12 },
+   { MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 }
+};
+
+/* 
-
+ * Helper functions
+ */
+
+/*
+ * video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
+ * @mbus: v4l2_mbus_framefmt format (input)
+ * @pix: v4l2_pix_format format (output)
+ *
+ * Fill the output pix structure with information from the input mbus format.
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static unsigned int video_mbus_to_pix(const struct v4l2_mbus_framefmt *mbus,
+ struct v4l2_pix_format *pix)
+{
+   unsigned int i;
+
+   memset(pix, 0, sizeof(*pix));
+   pix->width = mbus->width;
+   pix->height = mbus->height;
+
+   for (i = 0; i < ARRAY_SIZE(formats); ++i) {
+   if (formats[i].code == mbus->code)
+   break;
+   }
+
+   if (WARN_ON(i == ARRAY_SIZE(formats)))
+   return -EINVAL;
+
+   pix->pixelformat = formats[i].pixelformat;
+   pix->bytesperline = pix->width * formats[i].bpp / 8;
+   pix->bytesperline = ALIGN(pix->bytesperline, 8);
+   pix->sizeimage = pix->bytesperline * pix->height;
+   pix->colorspace = mbus->colorspace;
+   pix->field = mbus->field;
+
+   return 0;
+}
+
+static struct v4l2_subdev *video_remote_subdev(struct camss_video *video,
+  u32 *pad)
+{
+   struct media_pad *remote;
+
+   remote = media_entity_remote_pad(>pad);
+
+   if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
+   return NULL;
+
+   if (pad)
+   *pad = remote->index;
+
+   return media_entity_to_v4l2_subdev(remote->entity);
+}
+
+static int video_get_subdev_format(struct camss_video *video,
+  struct v4l2_format *format)
+{
+   struct v4l2_subdev_format fmt;
+   struct v4l2_subdev *subdev;
+   u32 pad;
+   int ret;
+
+   subdev = video_remote_subdev(video, );
+   if (subdev == NULL)
+   return -EINVAL;
+
+   fmt.pad = pad;
+   fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+
+   ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, );
+   if (ret)
+