Re: [PATCH v6] [media] vimc: Virtual Media Controller core, capture and sensor

2017-03-27 Thread Sakari Ailus 
Hi Helen,

Please see my comments below.

Helen Koike wrote:
> On 2017-01-25 11:03 AM, Sakari Ailus wrote:
...
>>> + * the videobuf2 framework will allocate this struct based on
>>> + * buf_struct_size and use the first sizeof(struct vb2_buffer)
>>> bytes of
>>> + * memory as a vb2_buffer
>>> + */
>>> +struct vb2_v4l2_buffer vb2;
>>> +struct list_head list;
>>> +};
>>> +
>>> +static int vimc_cap_querycap(struct file *file, void *priv,
>>> + struct v4l2_capability *cap)
>>> +{
>>> +struct vimc_cap_device *vcap = video_drvdata(file);
>>> +
>>> +strlcpy(cap->driver, KBUILD_MODNAME, sizeof(cap->driver));
>>> +strlcpy(cap->card, KBUILD_MODNAME, sizeof(cap->card));
>>> +snprintf(cap->bus_info, sizeof(cap->bus_info),
>>> + "platform:%s", vcap->v4l2_dev->name);
>>> +
>>> +return 0;
>>> +}
>>> +
>>> +static int vimc_cap_enum_input(struct file *file, void *priv,
>>> +   struct v4l2_input *i)
>>> +{
>>> +/* We only have one input */
>>> +if (i->index > 0)
>>> +return -EINVAL;
>>> +
>>> +i->type = V4L2_INPUT_TYPE_CAMERA;
>>> +strlcpy(i->name, "VIMC capture", sizeof(i->name));
>>
>> Isn't this (*INPUT IOCTLs) something that should be handled in a
>> sub-device
>> driver, such as a TV tuner?
> 
> 
> Can the ioctl VIDIOC_ENUMINPUT enumerate no inputs at all? Can I just
> return -EINVAL here in G_INPUT and S_INPUT as well?
> I thought I had to enumerate at least one input, and between
> V4L2_INPUT_TYPE_TUNER and V4L2_INPUT_TYPE_CAMERA, this last
> one seems more appropriated

I don't think other drivers that provide MC interface do this on video
nodes either. The VIMC driver could know what's connected to it, but
generally that's not the case.

> 
> 
>>
>>> +
>>> +return 0;
>>> +}
>>> +
>>> +static int vimc_cap_g_input(struct file *file, void *priv, unsigned
>>> int *i)
>>> +{
>>> +/* We only have one input */
>>> +*i = 0;
>>> +return 0;
>>> +}
>>> +
>>> +static int vimc_cap_s_input(struct file *file, void *priv, unsigned
>>> int i)
>>> +{
>>> +/* We only have one input */
>>> +return i ? -EINVAL : 0;
>>> +}
>>> +
>>> +static int vimc_cap_fmt_vid_cap(struct file *file, void *priv,
>>> +  struct v4l2_format *f)
>>> +{
>>> +struct vimc_cap_device *vcap = video_drvdata(file);
>>> +
>>> +f->fmt.pix = vcap->format;
>>> +
>>> +return 0;
>>> +}
>>> +
>>> +static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
>>> + struct v4l2_fmtdesc *f)
>>> +{
>>> +struct vimc_cap_device *vcap = video_drvdata(file);
>>> +
>>> +if (f->index > 0)
>>> +return -EINVAL;
>>> +
>>> +/* We only support one format for now */
>>> +f->pixelformat = vcap->format.pixelformat;
>>> +
>>> +return 0;
>>> +}
>>> +
>>> +static const struct v4l2_file_operations vimc_cap_fops = {
>>> +.owner= THIS_MODULE,
>>> +.open= v4l2_fh_open,
>>> +.release= vb2_fop_release,
>>> +.read   = vb2_fop_read,
>>> +.poll= vb2_fop_poll,
>>> +.unlocked_ioctl = video_ioctl2,
>>> +.mmap   = vb2_fop_mmap,
>>> +};
>>> +
>>> +static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = {
>>> +.vidioc_querycap = vimc_cap_querycap,
>>> +
>>> +.vidioc_enum_input = vimc_cap_enum_input,
>>> +.vidioc_g_input = vimc_cap_g_input,
>>> +.vidioc_s_input = vimc_cap_s_input,
>>> +
>>> +.vidioc_g_fmt_vid_cap = vimc_cap_fmt_vid_cap,
>>> +.vidioc_s_fmt_vid_cap = vimc_cap_fmt_vid_cap,
>>> +.vidioc_try_fmt_vid_cap = vimc_cap_fmt_vid_cap,
>>> +.vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap,
>>> +
>>> +.vidioc_reqbufs = vb2_ioctl_reqbufs,
>>> +.vidioc_create_bufs = vb2_ioctl_create_bufs,
>>> +.vidioc_querybuf = vb2_ioctl_querybuf,
>>> +.vidioc_qbuf = vb2_ioctl_qbuf,
>>> +.vidioc_dqbuf = vb2_ioctl_dqbuf,
>>> +.vidioc_expbuf = vb2_ioctl_expbuf,
>>> +.vidioc_streamon = vb2_ioctl_streamon,
>>> +.vidioc_streamoff = vb2_ioctl_streamoff,
>>> +};
>>> +
>>> +static void vimc_cap_return_all_buffers(struct vimc_cap_device *vcap,
>>> +enum vb2_buffer_state state)
>>> +{
>>> +struct vimc_cap_buffer *vbuf, *node;
>>> +
>>> +spin_lock(>qlock);
>>> +
>>> +list_for_each_entry_safe(vbuf, node, >buf_list, list) {
>>> +vb2_buffer_done(>vb2.vb2_buf, state);
>>> +list_del(>list);
>>> +}
>>> +
>>> +spin_unlock(>qlock);
>>> +}
>>> +
>>> +static int vimc_cap_pipeline_s_stream(struct vimc_cap_device *vcap,
>>> int enable)
>>> +{
>>> +int ret;
>>> +struct media_pad *pad;
>>> +struct media_entity *entity;
>>> +struct v4l2_subdev *sd;
>>> +
>>> +/* Start the stream in the subdevice direct connected */
>>> +entity = >vdev.entity;
>>> +pad = media_entity_remote_pad(>pads[0]);
>>
>> You could use vcap->vdev.entity.pads here, without assigning to
>> entity. Then
>> entity would only be used to refer to the

Re: [PATCH v6] [media] vimc: Virtual Media Controller core, capture and sensor

2017-03-24 Thread Helen Koike 

Hi Sakari,

Thanks for your review, I have some questions below:

On 2017-01-25 11:03 AM, Sakari Ailus wrote:

Hi Helen,

My apologies for the long review time!

Please see my comments below.

On Sun, Sep 04, 2016 at 05:02:18PM -0300, Helen Koike wrote:

From: Helen Fornazier 

First version of the Virtual Media Controller.
Add a simple version of the core of the driver, the capture and
sensor nodes in the topology, generating a grey image in a hardcoded
format.

Signed-off-by: Helen Koike 

---

Patch based in media/master tree, and available here:
https://github.com/helen-fornazier/opw-staging/tree/vimc/devel/vpu

Changes since v5:
- Fix message "Entity type for entity Sensor A was not initialized!"
  by initializing the sensor entity.function after the calling
  v4l2_subded_init
- populate device_caps in vimc_cap_create instead of in
  vimc_cap_querycap
- Fix typo in vimc-core.c s/de/the

Changes since v4:
- coding style fixes
- remove BUG_ON
- change copyright to 2016
- depens on VIDEO_V4L2_SUBDEV_API instead of select
- remove assignement of V4L2_CAP_DEVICE_CAPS
- s/vimc_cap_g_fmt_vid_cap/vimc_cap_fmt_vid_cap
- fix vimc_cap_queue_setup declaration type
- remove wrong buffer size check and add it in vimc_cap_buffer_prepare
- vimc_cap_create: remove unecessary check if v4l2_dev or v4l2_dev->dev is null
- vimc_cap_create: only allow a single pad
- vimc_sen_create: only allow source pads, remove unecessary source pads
checks in vimc_thread_sen

Changes since v3: fix rmmod crash and built-in compile
- Re-order unregister calls in vimc_device_unregister function (remove
rmmod issue)
- Call media_device_unregister_entity in vimc_raw_destroy
- Add depends on VIDEO_DEV && VIDEO_V4L2 and select VIDEOBUF2_VMALLOC
- Check *nplanes in queue_setup (this remove v4l2-compliance fail)
- Include  in vimc-sensor.c
- Move include of  from vimc-core.c to vimc-core.h
- Generate 60 frames per sec instead of 1 in the sensor

Changes since v2: update with current media master tree
- Add struct media_pipeline in vimc_cap_device
- Use vb2_v4l2_buffer instead of vb2_buffer
- Typos
- Remove usage of entity->type and use entity->function instead
- Remove fmt argument from queue setup
- Use ktime_get_ns instead of v4l2_get_timestamp
- Iterate over link's list using list_for_each_entry
- Use media_device_{init, cleanup}
- Use entity->use_count to keep track of entities instead of the old
entity->id
- Replace media_entity_init by media_entity_pads_init
---
 drivers/media/platform/Kconfig |   2 +
 drivers/media/platform/Makefile|   1 +
 drivers/media/platform/vimc/Kconfig|   7 +
 drivers/media/platform/vimc/Makefile   |   3 +
 drivers/media/platform/vimc/vimc-capture.c | 553 ++
 drivers/media/platform/vimc/vimc-capture.h |  28 ++
 drivers/media/platform/vimc/vimc-core.c| 600 +
 drivers/media/platform/vimc/vimc-core.h|  57 +++
 drivers/media/platform/vimc/vimc-sensor.c  | 279 ++
 drivers/media/platform/vimc/vimc-sensor.h  |  28 ++
 10 files changed, 1558 insertions(+)
 create mode 100644 drivers/media/platform/vimc/Kconfig
 create mode 100644 drivers/media/platform/vimc/Makefile
 create mode 100644 drivers/media/platform/vimc/vimc-capture.c
 create mode 100644 drivers/media/platform/vimc/vimc-capture.h
 create mode 100644 drivers/media/platform/vimc/vimc-core.c
 create mode 100644 drivers/media/platform/vimc/vimc-core.h
 create mode 100644 drivers/media/platform/vimc/vimc-sensor.c
 create mode 100644 drivers/media/platform/vimc/vimc-sensor.h

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 46f14dd..4a0577f 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -329,6 +329,8 @@ menuconfig V4L_TEST_DRIVERS

 if V4L_TEST_DRIVERS

+source "drivers/media/platform/vimc/Kconfig"
+
 source "drivers/media/platform/vivid/Kconfig"

 config VIDEO_VIM2M
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 536d1d8..dd4f658 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_VIDEO_OMAP3) += omap3isp/

 obj-$(CONFIG_VIDEO_VIU) += fsl-viu.o

+obj-$(CONFIG_VIDEO_VIMC)   += vimc/
 obj-$(CONFIG_VIDEO_VIVID)  += vivid/
 obj-$(CONFIG_VIDEO_VIM2M)  += vim2m.o

diff --git a/drivers/media/platform/vimc/Kconfig 
b/drivers/media/platform/vimc/Kconfig
new file mode 100644
index 000..b48819c
--- /dev/null
+++ b/drivers/media/platform/vimc/Kconfig
@@ -0,0 +1,7 @@
+config VIDEO_VIMC
+   tristate "Virtual Media Controller Driver (VIMC)"
+   depends on VIDEO_DEV && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   select VIDEOBUF2_VMALLOC
+   default n
+   ---help---
+ Skeleton driver for Virtual Media Controller


A bit more verbose description would be nice.


diff --git

Re: [PATCH v6] [media] vimc: Virtual Media Controller core, capture and sensor

2017-03-10 Thread Helen Koike 

Hi Hans,

On 2017-03-10 10:08 AM, Hans Verkuil wrote:

Hi Helen,

On 11/01/17 02:30, Helen Koike wrote:
>
> Thank you for the review, I'll update the patch accordingly and re-submit it.
>
> Helen

Do you know when you have a v7 ready?


Thanks for your interest. I don't have the next version entirely ready yet but 
I'll work on it to send this next version asap and the other patches I have on 
top of it as well.



We really need a vimc driver so people without hardware can actually fiddle 
around
with the MC.

See also my rant here (not directed at you!):

https://www.spinics.net/lists/kernel/msg2462009.html

Regards,

Hans


Helen

Re: [PATCH v6] [media] vimc: Virtual Media Controller core, capture and sensor

2017-03-10 Thread Hans Verkuil 
Hi Helen,

On 11/01/17 02:30, Helen Koike wrote:
> 
> Thank you for the review, I'll update the patch accordingly and re-submit it.
> 
> Helen

Do you know when you have a v7 ready?

We really need a vimc driver so people without hardware can actually fiddle 
around
with the MC.

See also my rant here (not directed at you!):

https://www.spinics.net/lists/kernel/msg2462009.html

Regards,

Hans

Re: [PATCH v6] [media] vimc: Virtual Media Controller core, capture and sensor

2017-01-25 Thread Mauro Carvalho Chehab 
I won't be doing a review on it yet... I'll try to do it on the next
version. Just a quick notice:

Em Wed, 25 Jan 2017 15:03:46 +0200
Sakari Ailus  escreveu:

> > + * Copyright (C) 2016 Helen Koike F.   
> 
> 2017 might be used now.

Please keep 2016 as the year you did the initial development. It
makes sense to change it to:
2016-2017

In order to reflect that you're also doing some work on it this year,
but preserving the initial date is important, IMHO.

-- 
Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] [media] vimc: Virtual Media Controller core, capture and sensor

2017-01-25 Thread Sakari Ailus 
Hi Helen,

My apologies for the long review time!

Please see my comments below.

On Sun, Sep 04, 2016 at 05:02:18PM -0300, Helen Koike wrote:
> From: Helen Fornazier 
> 
> First version of the Virtual Media Controller.
> Add a simple version of the core of the driver, the capture and
> sensor nodes in the topology, generating a grey image in a hardcoded
> format.
> 
> Signed-off-by: Helen Koike 
> 
> ---
> 
> Patch based in media/master tree, and available here:
> https://github.com/helen-fornazier/opw-staging/tree/vimc/devel/vpu
> 
> Changes since v5:
> - Fix message "Entity type for entity Sensor A was not initialized!"
>   by initializing the sensor entity.function after the calling
>   v4l2_subded_init
> - populate device_caps in vimc_cap_create instead of in
>   vimc_cap_querycap
> - Fix typo in vimc-core.c s/de/the
> 
> Changes since v4:
> - coding style fixes
> - remove BUG_ON
> - change copyright to 2016
> - depens on VIDEO_V4L2_SUBDEV_API instead of select
> - remove assignement of V4L2_CAP_DEVICE_CAPS
> - s/vimc_cap_g_fmt_vid_cap/vimc_cap_fmt_vid_cap
> - fix vimc_cap_queue_setup declaration type
> - remove wrong buffer size check and add it in vimc_cap_buffer_prepare
> - vimc_cap_create: remove unecessary check if v4l2_dev or v4l2_dev->dev is 
> null
> - vimc_cap_create: only allow a single pad
> - vimc_sen_create: only allow source pads, remove unecessary source pads
> checks in vimc_thread_sen
> 
> Changes since v3: fix rmmod crash and built-in compile
> - Re-order unregister calls in vimc_device_unregister function (remove
> rmmod issue)
> - Call media_device_unregister_entity in vimc_raw_destroy
> - Add depends on VIDEO_DEV && VIDEO_V4L2 and select VIDEOBUF2_VMALLOC
> - Check *nplanes in queue_setup (this remove v4l2-compliance fail)
> - Include  in vimc-sensor.c
> - Move include of  from vimc-core.c to vimc-core.h
> - Generate 60 frames per sec instead of 1 in the sensor
> 
> Changes since v2: update with current media master tree
> - Add struct media_pipeline in vimc_cap_device
> - Use vb2_v4l2_buffer instead of vb2_buffer
> - Typos
> - Remove usage of entity->type and use entity->function instead
> - Remove fmt argument from queue setup
> - Use ktime_get_ns instead of v4l2_get_timestamp
> - Iterate over link's list using list_for_each_entry
> - Use media_device_{init, cleanup}
> - Use entity->use_count to keep track of entities instead of the old
> entity->id
> - Replace media_entity_init by media_entity_pads_init
> ---
>  drivers/media/platform/Kconfig |   2 +
>  drivers/media/platform/Makefile|   1 +
>  drivers/media/platform/vimc/Kconfig|   7 +
>  drivers/media/platform/vimc/Makefile   |   3 +
>  drivers/media/platform/vimc/vimc-capture.c | 553 ++
>  drivers/media/platform/vimc/vimc-capture.h |  28 ++
>  drivers/media/platform/vimc/vimc-core.c| 600 
> +
>  drivers/media/platform/vimc/vimc-core.h|  57 +++
>  drivers/media/platform/vimc/vimc-sensor.c  | 279 ++
>  drivers/media/platform/vimc/vimc-sensor.h  |  28 ++
>  10 files changed, 1558 insertions(+)
>  create mode 100644 drivers/media/platform/vimc/Kconfig
>  create mode 100644 drivers/media/platform/vimc/Makefile
>  create mode 100644 drivers/media/platform/vimc/vimc-capture.c
>  create mode 100644 drivers/media/platform/vimc/vimc-capture.h
>  create mode 100644 drivers/media/platform/vimc/vimc-core.c
>  create mode 100644 drivers/media/platform/vimc/vimc-core.h
>  create mode 100644 drivers/media/platform/vimc/vimc-sensor.c
>  create mode 100644 drivers/media/platform/vimc/vimc-sensor.h
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 46f14dd..4a0577f 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -329,6 +329,8 @@ menuconfig V4L_TEST_DRIVERS
>  
>  if V4L_TEST_DRIVERS
>  
> +source "drivers/media/platform/vimc/Kconfig"
> +
>  source "drivers/media/platform/vivid/Kconfig"
>  
>  config VIDEO_VIM2M
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 536d1d8..dd4f658 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_VIDEO_OMAP3)   += omap3isp/
>  
>  obj-$(CONFIG_VIDEO_VIU) += fsl-viu.o
>  
> +obj-$(CONFIG_VIDEO_VIMC) += vimc/
>  obj-$(CONFIG_VIDEO_VIVID)+= vivid/
>  obj-$(CONFIG_VIDEO_VIM2M)+= vim2m.o
>  
> diff --git a/drivers/media/platform/vimc/Kconfig 
> b/drivers/media/platform/vimc/Kconfig
> new file mode 100644
> index 000..b48819c
> --- /dev/null
> +++ b/drivers/media/platform/vimc/Kconfig
> @@ -0,0 +1,7 @@
> +config VIDEO_VIMC
> + tristate "Virtual Media Controller Driver (VIMC)"
> + depends on VIDEO_DEV && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + select VIDEOBUF2_VMALLOC
> + default n
> + ---help---
> +   Skeleton

Re: [PATCH v6] [media] vimc: Virtual Media Controller core, capture and sensor

2017-01-10 Thread Helen Koike 

Hi Laurent,

On 2017-01-10 04:54 PM, Laurent Pinchart wrote:

Hi Helen,

(CC'ing Sakari as there's a question specifically for him)

Thank you for the patch, and so sorry for the late review.

On Sunday 04 Sep 2016 17:02:18 Helen Koike wrote:

From: Helen Fornazier 

First version of the Virtual Media Controller.
Add a simple version of the core of the driver, the capture and
sensor nodes in the topology, generating a grey image in a hardcoded
format.

Signed-off-by: Helen Koike 


I've reviewed the whole patch but haven't had time to test it. I've also
skipped the items marked as TODO or FIXME as they're obviously not ready yet
:-) Overall this looks good to me, all the issues are minor.


---

Patch based in media/master tree, and available here:
https://github.com/helen-fornazier/opw-staging/tree/vimc/devel/vpu

Changes since v5:
- Fix message "Entity type for entity Sensor A was not initialized!"
  by initializing the sensor entity.function after the calling
  v4l2_subded_init
- populate device_caps in vimc_cap_create instead of in
  vimc_cap_querycap
- Fix typo in vimc-core.c s/de/the

Changes since v4:
- coding style fixes
- remove BUG_ON
- change copyright to 2016
- depens on VIDEO_V4L2_SUBDEV_API instead of select
- remove assignement of V4L2_CAP_DEVICE_CAPS
- s/vimc_cap_g_fmt_vid_cap/vimc_cap_fmt_vid_cap
- fix vimc_cap_queue_setup declaration type
- remove wrong buffer size check and add it in vimc_cap_buffer_prepare
- vimc_cap_create: remove unecessary check if v4l2_dev or v4l2_dev->dev is
null - vimc_cap_create: only allow a single pad
- vimc_sen_create: only allow source pads, remove unecessary source pads
checks in vimc_thread_sen

Changes since v3: fix rmmod crash and built-in compile
- Re-order unregister calls in vimc_device_unregister function (remove
rmmod issue)
- Call media_device_unregister_entity in vimc_raw_destroy
- Add depends on VIDEO_DEV && VIDEO_V4L2 and select VIDEOBUF2_VMALLOC
- Check *nplanes in queue_setup (this remove v4l2-compliance fail)
- Include  in vimc-sensor.c
- Move include of  from vimc-core.c to vimc-core.h
- Generate 60 frames per sec instead of 1 in the sensor

Changes since v2: update with current media master tree
- Add struct media_pipeline in vimc_cap_device
- Use vb2_v4l2_buffer instead of vb2_buffer
- Typos
- Remove usage of entity->type and use entity->function instead
- Remove fmt argument from queue setup
- Use ktime_get_ns instead of v4l2_get_timestamp
- Iterate over link's list using list_for_each_entry
- Use media_device_{init, cleanup}
- Use entity->use_count to keep track of entities instead of the old
entity->id
- Replace media_entity_init by media_entity_pads_init
---
 drivers/media/platform/Kconfig |   2 +
 drivers/media/platform/Makefile|   1 +
 drivers/media/platform/vimc/Kconfig|   7 +
 drivers/media/platform/vimc/Makefile   |   3 +
 drivers/media/platform/vimc/vimc-capture.c | 553 ++
 drivers/media/platform/vimc/vimc-capture.h |  28 ++
 drivers/media/platform/vimc/vimc-core.c| 600 ++
 drivers/media/platform/vimc/vimc-core.h|  57 +++
 drivers/media/platform/vimc/vimc-sensor.c  | 279 ++
 drivers/media/platform/vimc/vimc-sensor.h  |  28 ++
 10 files changed, 1558 insertions(+)
 create mode 100644 drivers/media/platform/vimc/Kconfig
 create mode 100644 drivers/media/platform/vimc/Makefile
 create mode 100644 drivers/media/platform/vimc/vimc-capture.c
 create mode 100644 drivers/media/platform/vimc/vimc-capture.h
 create mode 100644 drivers/media/platform/vimc/vimc-core.c
 create mode 100644 drivers/media/platform/vimc/vimc-core.h
 create mode 100644 drivers/media/platform/vimc/vimc-sensor.c
 create mode 100644 drivers/media/platform/vimc/vimc-sensor.h


[snip]


diff --git a/drivers/media/platform/vimc/vimc-capture.c
b/drivers/media/platform/vimc/vimc-capture.c new file mode 100644
index 000..b7636cf
--- /dev/null
+++ b/drivers/media/platform/vimc/vimc-capture.c


[snip]


+static void vimc_cap_return_all_buffers(struct vimc_cap_device *vcap,
+   enum vb2_buffer_state state)
+{
+   struct vimc_cap_buffer *vbuf, *node;
+
+   spin_lock(>qlock);
+
+   list_for_each_entry_safe(vbuf, node, >buf_list, list) {
+   vb2_buffer_done(>vb2.vb2_buf, state);
+   list_del(>list);


It shouldn't matter given that you protect this with a spinlock, but moving
the list_del() above makes the code flow follow a safer order.


+   }
+
+   spin_unlock(>qlock);
+}
+
+static int vimc_cap_pipeline_s_stream(struct vimc_cap_device *vcap, int
enable)
+{
+   int ret;
+   struct media_pad *pad;
+   struct media_entity *entity;
+   struct v4l2_subdev *sd;
+
+   /* Start the stream in the subdevice direct connected */
+   entity = >vdev.entity;
+   pad = media_entity_remote_pad(>pads[0]);
+
+   /* If we are

Re: [PATCH v6] [media] vimc: Virtual Media Controller core, capture and sensor

2017-01-10 Thread Laurent Pinchart 
Hi Helen,

(CC'ing Sakari as there's a question specifically for him)

Thank you for the patch, and so sorry for the late review. 

On Sunday 04 Sep 2016 17:02:18 Helen Koike wrote:
> From: Helen Fornazier 
> 
> First version of the Virtual Media Controller.
> Add a simple version of the core of the driver, the capture and
> sensor nodes in the topology, generating a grey image in a hardcoded
> format.
> 
> Signed-off-by: Helen Koike 

I've reviewed the whole patch but haven't had time to test it. I've also 
skipped the items marked as TODO or FIXME as they're obviously not ready yet 
:-) Overall this looks good to me, all the issues are minor.

> ---
> 
> Patch based in media/master tree, and available here:
> https://github.com/helen-fornazier/opw-staging/tree/vimc/devel/vpu
> 
> Changes since v5:
> - Fix message "Entity type for entity Sensor A was not initialized!"
>   by initializing the sensor entity.function after the calling
>   v4l2_subded_init
> - populate device_caps in vimc_cap_create instead of in
>   vimc_cap_querycap
> - Fix typo in vimc-core.c s/de/the
> 
> Changes since v4:
> - coding style fixes
> - remove BUG_ON
> - change copyright to 2016
> - depens on VIDEO_V4L2_SUBDEV_API instead of select
> - remove assignement of V4L2_CAP_DEVICE_CAPS
> - s/vimc_cap_g_fmt_vid_cap/vimc_cap_fmt_vid_cap
> - fix vimc_cap_queue_setup declaration type
> - remove wrong buffer size check and add it in vimc_cap_buffer_prepare
> - vimc_cap_create: remove unecessary check if v4l2_dev or v4l2_dev->dev is
> null - vimc_cap_create: only allow a single pad
> - vimc_sen_create: only allow source pads, remove unecessary source pads
> checks in vimc_thread_sen
> 
> Changes since v3: fix rmmod crash and built-in compile
> - Re-order unregister calls in vimc_device_unregister function (remove
> rmmod issue)
> - Call media_device_unregister_entity in vimc_raw_destroy
> - Add depends on VIDEO_DEV && VIDEO_V4L2 and select VIDEOBUF2_VMALLOC
> - Check *nplanes in queue_setup (this remove v4l2-compliance fail)
> - Include  in vimc-sensor.c
> - Move include of  from vimc-core.c to vimc-core.h
> - Generate 60 frames per sec instead of 1 in the sensor
> 
> Changes since v2: update with current media master tree
> - Add struct media_pipeline in vimc_cap_device
> - Use vb2_v4l2_buffer instead of vb2_buffer
> - Typos
> - Remove usage of entity->type and use entity->function instead
> - Remove fmt argument from queue setup
> - Use ktime_get_ns instead of v4l2_get_timestamp
> - Iterate over link's list using list_for_each_entry
> - Use media_device_{init, cleanup}
> - Use entity->use_count to keep track of entities instead of the old
> entity->id
> - Replace media_entity_init by media_entity_pads_init
> ---
>  drivers/media/platform/Kconfig |   2 +
>  drivers/media/platform/Makefile|   1 +
>  drivers/media/platform/vimc/Kconfig|   7 +
>  drivers/media/platform/vimc/Makefile   |   3 +
>  drivers/media/platform/vimc/vimc-capture.c | 553 ++
>  drivers/media/platform/vimc/vimc-capture.h |  28 ++
>  drivers/media/platform/vimc/vimc-core.c| 600 ++
>  drivers/media/platform/vimc/vimc-core.h|  57 +++
>  drivers/media/platform/vimc/vimc-sensor.c  | 279 ++
>  drivers/media/platform/vimc/vimc-sensor.h  |  28 ++
>  10 files changed, 1558 insertions(+)
>  create mode 100644 drivers/media/platform/vimc/Kconfig
>  create mode 100644 drivers/media/platform/vimc/Makefile
>  create mode 100644 drivers/media/platform/vimc/vimc-capture.c
>  create mode 100644 drivers/media/platform/vimc/vimc-capture.h
>  create mode 100644 drivers/media/platform/vimc/vimc-core.c
>  create mode 100644 drivers/media/platform/vimc/vimc-core.h
>  create mode 100644 drivers/media/platform/vimc/vimc-sensor.c
>  create mode 100644 drivers/media/platform/vimc/vimc-sensor.h

[snip]

> diff --git a/drivers/media/platform/vimc/vimc-capture.c
> b/drivers/media/platform/vimc/vimc-capture.c new file mode 100644
> index 000..b7636cf
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc-capture.c

[snip]

> +static void vimc_cap_return_all_buffers(struct vimc_cap_device *vcap,
> + enum vb2_buffer_state state)
> +{
> + struct vimc_cap_buffer *vbuf, *node;
> +
> + spin_lock(>qlock);
> +
> + list_for_each_entry_safe(vbuf, node, >buf_list, list) {
> + vb2_buffer_done(>vb2.vb2_buf, state);
> + list_del(>list);

It shouldn't matter given that you protect this with a spinlock, but moving 
the list_del() above makes the code flow follow a safer order.

> + }
> +
> + spin_unlock(>qlock);
> +}
> +
> +static int vimc_cap_pipeline_s_stream(struct vimc_cap_device *vcap, int
> enable)
> +{
> + int ret;
> + struct media_pad *pad;
> + struct media_entity *entity;
> + struct v4l2_subdev *sd;
> +
> + /* Start the stream in the subdevice

Re: [PATCH v6] [media] vimc: Virtual Media Controller core, capture and sensor

2016-09-06 Thread Hans Verkuil 

On 09/04/16 22:02, Helen Koike wrote:

From: Helen Fornazier 

First version of the Virtual Media Controller.
Add a simple version of the core of the driver, the capture and
sensor nodes in the topology, generating a grey image in a hardcoded
format.

Signed-off-by: Helen Koike 



One thing is missing: a MAINTAINERS entry. Can you make a separate patch
updating the MAINTAINERS file?

Thanks!

Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6] [media] vimc: Virtual Media Controller core, capture and sensor

2016-09-04 Thread Helen Koike 
From: Helen Fornazier 

First version of the Virtual Media Controller.
Add a simple version of the core of the driver, the capture and
sensor nodes in the topology, generating a grey image in a hardcoded
format.

Signed-off-by: Helen Koike 

---

Patch based in media/master tree, and available here:
https://github.com/helen-fornazier/opw-staging/tree/vimc/devel/vpu

Changes since v5:
- Fix message "Entity type for entity Sensor A was not initialized!"
  by initializing the sensor entity.function after the calling
  v4l2_subded_init
- populate device_caps in vimc_cap_create instead of in
  vimc_cap_querycap
- Fix typo in vimc-core.c s/de/the

Changes since v4:
- coding style fixes
- remove BUG_ON
- change copyright to 2016
- depens on VIDEO_V4L2_SUBDEV_API instead of select
- remove assignement of V4L2_CAP_DEVICE_CAPS
- s/vimc_cap_g_fmt_vid_cap/vimc_cap_fmt_vid_cap
- fix vimc_cap_queue_setup declaration type
- remove wrong buffer size check and add it in vimc_cap_buffer_prepare
- vimc_cap_create: remove unecessary check if v4l2_dev or v4l2_dev->dev is null
- vimc_cap_create: only allow a single pad
- vimc_sen_create: only allow source pads, remove unecessary source pads
checks in vimc_thread_sen

Changes since v3: fix rmmod crash and built-in compile
- Re-order unregister calls in vimc_device_unregister function (remove
rmmod issue)
- Call media_device_unregister_entity in vimc_raw_destroy
- Add depends on VIDEO_DEV && VIDEO_V4L2 and select VIDEOBUF2_VMALLOC
- Check *nplanes in queue_setup (this remove v4l2-compliance fail)
- Include  in vimc-sensor.c
- Move include of  from vimc-core.c to vimc-core.h
- Generate 60 frames per sec instead of 1 in the sensor

Changes since v2: update with current media master tree
- Add struct media_pipeline in vimc_cap_device
- Use vb2_v4l2_buffer instead of vb2_buffer
- Typos
- Remove usage of entity->type and use entity->function instead
- Remove fmt argument from queue setup
- Use ktime_get_ns instead of v4l2_get_timestamp
- Iterate over link's list using list_for_each_entry
- Use media_device_{init, cleanup}
- Use entity->use_count to keep track of entities instead of the old
entity->id
- Replace media_entity_init by media_entity_pads_init
---
 drivers/media/platform/Kconfig |   2 +
 drivers/media/platform/Makefile|   1 +
 drivers/media/platform/vimc/Kconfig|   7 +
 drivers/media/platform/vimc/Makefile   |   3 +
 drivers/media/platform/vimc/vimc-capture.c | 553 ++
 drivers/media/platform/vimc/vimc-capture.h |  28 ++
 drivers/media/platform/vimc/vimc-core.c| 600 +
 drivers/media/platform/vimc/vimc-core.h|  57 +++
 drivers/media/platform/vimc/vimc-sensor.c  | 279 ++
 drivers/media/platform/vimc/vimc-sensor.h  |  28 ++
 10 files changed, 1558 insertions(+)
 create mode 100644 drivers/media/platform/vimc/Kconfig
 create mode 100644 drivers/media/platform/vimc/Makefile
 create mode 100644 drivers/media/platform/vimc/vimc-capture.c
 create mode 100644 drivers/media/platform/vimc/vimc-capture.h
 create mode 100644 drivers/media/platform/vimc/vimc-core.c
 create mode 100644 drivers/media/platform/vimc/vimc-core.h
 create mode 100644 drivers/media/platform/vimc/vimc-sensor.c
 create mode 100644 drivers/media/platform/vimc/vimc-sensor.h

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 46f14dd..4a0577f 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -329,6 +329,8 @@ menuconfig V4L_TEST_DRIVERS
 
 if V4L_TEST_DRIVERS
 
+source "drivers/media/platform/vimc/Kconfig"
+
 source "drivers/media/platform/vivid/Kconfig"
 
 config VIDEO_VIM2M
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 536d1d8..dd4f658 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_VIDEO_OMAP3) += omap3isp/
 
 obj-$(CONFIG_VIDEO_VIU) += fsl-viu.o
 
+obj-$(CONFIG_VIDEO_VIMC)   += vimc/
 obj-$(CONFIG_VIDEO_VIVID)  += vivid/
 obj-$(CONFIG_VIDEO_VIM2M)  += vim2m.o
 
diff --git a/drivers/media/platform/vimc/Kconfig 
b/drivers/media/platform/vimc/Kconfig
new file mode 100644
index 000..b48819c
--- /dev/null
+++ b/drivers/media/platform/vimc/Kconfig
@@ -0,0 +1,7 @@
+config VIDEO_VIMC
+   tristate "Virtual Media Controller Driver (VIMC)"
+   depends on VIDEO_DEV && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   select VIDEOBUF2_VMALLOC
+   default n
+   ---help---
+ Skeleton driver for Virtual Media Controller
diff --git a/drivers/media/platform/vimc/Makefile 
b/drivers/media/platform/vimc/Makefile
new file mode 100644
index 000..c45195e
--- /dev/null
+++ b/drivers/media/platform/vimc/Makefile
@@ -0,0 +1,3 @@
+vimc-objs := vimc-core.o vimc-capture.o vimc-sensor.o
+
+obj-$(CONFIG_VIDEO_VIMC) += vimc.o
diff --git