media: imx: queue subdev events to reachable video devices

PATCH Superseded Steve Longerbeam 2 participants 3 messages Dec 9, 2018 – Jan 8, 2019

Subsystems (4)

Media (V4L/DVB) Arm i.MX and Layerscape Staging Media drivers for Freescale i.MX

lore

Share & tools

lore

Steve Longerbeam

author maintainer

2018-12-09 19:57 UTC #0 lore raw

PATCH 1/1 media: imx: queue subdev events to reachable video devices

From: Steve Longerbeam <[email protected]>

Forward events from a sub-device to its list of reachable video devices.

Note this will queue the event to a video device even if there is no actual _enabled_ media path from the sub-device to the video device. So a future improvement is to skip the video device if there is no enabled path to it from the sub-device. The entity->pipe pointer can't be used for this check because in imx-media a sub-device can be a member to more than one streaming pipeline at a time.

Signed-off-by Steve Longerbeam <[email protected]>

---
 drivers/staging/media/imx/imx-media-capture.c | 18 ++++++++++++++
 drivers/staging/media/imx/imx-media-dev.c     | 24 +++++++++++++++++++
 2 files changed, 42 insertions(+)

drivers/staging/media/imx/imx-media-capture.c +18 -0

index b37e1186eb2f..4dfbe05d203e 100644
@@ -335,6 +335,21 @@ static int capture_s_parm(struct file *file, void *fh,
 	return 0;
 }
 
+static int capture_subscribe_event(struct v4l2_fh *fh,
+				   const struct v4l2_event_subscription *sub)
+{
+	switch (sub->type) {
+	case V4L2_EVENT_IMX_FRAME_INTERVAL_ERROR:
+		return v4l2_event_subscribe(fh, sub, 0, NULL);
+	case V4L2_EVENT_SOURCE_CHANGE:
+		return v4l2_src_change_event_subscribe(fh, sub);
+	case V4L2_EVENT_CTRL:
+		return v4l2_ctrl_subscribe_event(fh, sub);
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct v4l2_ioctl_ops capture_ioctl_ops = {
 	.vidioc_querycap	= vidioc_querycap,

@@ -362,6 +377,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = {
 	.vidioc_expbuf		= vb2_ioctl_expbuf,
 	.vidioc_streamon	= vb2_ioctl_streamon,
 	.vidioc_streamoff	= vb2_ioctl_streamoff,
+
+	.vidioc_subscribe_event = capture_subscribe_event,
+	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
 };
 
 /*

drivers/staging/media/imx/imx-media-dev.c +24 -0

index 4b344a4a3706..25e916562c66 100644
@@ -442,6 +442,29 @@ static const struct media_device_ops imx_media_md_ops = {
 	.link_notify = imx_media_link_notify,
 };
 
+static void imx_media_notify(struct v4l2_subdev *sd,
+			     unsigned int notification,
+			     void *arg)
+{
+	struct media_entity *entity = &sd->entity;
+	int i;
+
+	if (notification != V4L2_DEVICE_NOTIFY_EVENT)
+		return;
+
+	for (i = 0; i < entity->num_pads; i++) {
+		struct media_pad *pad = &entity->pads[i];
+		struct imx_media_pad_vdev *pad_vdev;
+		struct list_head *pad_vdev_list;
+
+		pad_vdev_list = to_pad_vdev_list(sd, pad->index);
+		if (!pad_vdev_list)
+			continue;
+		list_for_each_entry(pad_vdev, pad_vdev_list, list)
+			v4l2_event_queue(pad_vdev->vdev->vfd, arg);
+	}
+}
+
 static int imx_media_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;

@@ -462,6 +485,7 @@ static int imx_media_probe(struct platform_device *pdev)
 	mutex_init(&imxmd->mutex);
 
 	imxmd->v4l2_dev.mdev = &imxmd->md;
+	imxmd->v4l2_dev.notify = imx_media_notify;
 	strscpy(imxmd->v4l2_dev.name, "imx-media",
 		sizeof(imxmd->v4l2_dev.name));

Signature · 1 line

2.17.1

Hans Verkuil

2019-01-08 13:26 UTC #1 lore raw

Quoted 11 lines · Steve Longerbeam

From: Steve Longerbeam <[email protected]>
Forward events from a sub-device to its list of reachable video devices.
Note this will queue the event to a video device even if there is no actual _enabled_ media path from the sub-device to the video device. So a future improvement is to skip the video device if there is no enabled path to it from the sub-device. The entity->pipe pointer can't be used for this check because in imx-media a sub-device can be a member to more than one streaming pipeline at a time.

You explain what you are doing, but I am missing an explanation of *why* this is needed.

Quoted 34 lines

Signed-off-by Steve Longerbeam <[email protected]>

---
 drivers/staging/media/imx/imx-media-capture.c | 18 ++++++++++++++
 drivers/staging/media/imx/imx-media-dev.c     | 24 +++++++++++++++++++
 2 files changed, 42 insertions(+)

drivers/staging/media/imx/imx-media-capture.c +15 -0

index b37e1186eb2f..4dfbe05d203e 100644
@@ -335,6 +335,21 @@ static int capture_s_parm(struct file *file, void *fh,
 	return 0;
 }
 
+static int capture_subscribe_event(struct v4l2_fh *fh,
+				   const struct v4l2_event_subscription *sub)
+{
+	switch (sub->type) {
+	case V4L2_EVENT_IMX_FRAME_INTERVAL_ERROR:
+		return v4l2_event_subscribe(fh, sub, 0, NULL);
+	case V4L2_EVENT_SOURCE_CHANGE:
+		return v4l2_src_change_event_subscribe(fh, sub);
+	case V4L2_EVENT_CTRL:
+		return v4l2_ctrl_subscribe_event(fh, sub);
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct v4l2_ioctl_ops capture_ioctl_ops = {
 	.vidioc_querycap	= vidioc_querycap,

drivers/staging/media/imx/imx-media-capture.c static const struct v4l2_ioctl_ops capture_ioctl_ops = { · line 383

Quoted 7 lines

drivers/staging/media/imx/imx-media-capture.c +3 -0

@@ -362,6 +377,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = {
 	.vidioc_expbuf		= vb2_ioctl_expbuf,
 	.vidioc_streamon	= vb2_ioctl_streamon,
 	.vidioc_streamoff	= vb2_ioctl_streamoff,
+
+	.vidioc_subscribe_event = capture_subscribe_event,
+	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
 };

This part of the patch adds event support to the capture device, can't this be split up into a separate patch? It seems to be useful in its own right.

Quoted 20 lines

drivers/staging/media/imx/imx-media-capture.c +0 -0

/*

drivers/staging/media/imx/imx-media-dev.c +10 -0

index 4b344a4a3706..25e916562c66 100644
@@ -442,6 +442,29 @@ static const struct media_device_ops imx_media_md_ops = {
 	.link_notify = imx_media_link_notify,
 };
 
+static void imx_media_notify(struct v4l2_subdev *sd,
+			     unsigned int notification,
+			     void *arg)
+{
+	struct media_entity *entity = &sd->entity;
+	int i;
+
+	if (notification != V4L2_DEVICE_NOTIFY_EVENT)
+		return;
+

drivers/staging/media/imx/imx-media-dev.c static const struct media_device_ops imx_media_md_ops = { · line 464

Quoted 10 lines

drivers/staging/media/imx/imx-media-dev.c +10 -0

+	for (i = 0; i < entity->num_pads; i++) {
+		struct media_pad *pad = &entity->pads[i];
+		struct imx_media_pad_vdev *pad_vdev;
+		struct list_head *pad_vdev_list;
+
+		pad_vdev_list = to_pad_vdev_list(sd, pad->index);
+		if (!pad_vdev_list)
+			continue;
+		list_for_each_entry(pad_vdev, pad_vdev_list, list)
+			v4l2_event_queue(pad_vdev->vdev->vfd, arg);

Which events do you want to forward? E.g. forwarding control events doesn't seem right, but other events may be useful. Are those events also appearing on the v4l-subdevX device? And if so, should they?

Quoted 8 lines

drivers/staging/media/imx/imx-media-dev.c +3 -0

+	}
+}
+
 static int imx_media_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;

drivers/staging/media/imx/imx-media-dev.c static int imx_media_probe(struct platform_device *pdev) · line 491

Quoted 7 lines

drivers/staging/media/imx/imx-media-dev.c +1 -0

@@ -462,6 +485,7 @@ static int imx_media_probe(struct platform_device *pdev)
 	mutex_init(&imxmd->mutex);
 
 	imxmd->v4l2_dev.mdev = &imxmd->md;
+	imxmd->v4l2_dev.notify = imx_media_notify;
 	strscpy(imxmd->v4l2_dev.name, "imx-media",
 		sizeof(imxmd->v4l2_dev.name));

Regards,

Hans

Steve Longerbeam

author maintainer

2019-01-08 19:14 UTC #2 lore raw

Hi Hans,

Quoted 6 lines · Hans Verkuil

Quoted 5 lines · Steve Longerbeam
From: Steve Longerbeam <[email protected]>
Forward events from a sub-device to its list of reachable video devices.

Quoted 8 lines

Quoted 6 lines
Note this will queue the event to a video device even if there is no actual _enabled_ media path from the sub-device to the video device. So a future improvement is to skip the video device if there is no enabled path to it from the sub-device. The entity->pipe pointer can't be used for this check because in imx-media a sub-device can be a member to more than one streaming pipeline at a time.
You explain what you are doing, but I am missing an explanation of *why* this is needed.

Ok, I'll add more explanation.

Quoted 33 lines

Signed-off-by Steve Longerbeam <[email protected]>

---
  drivers/staging/media/imx/imx-media-capture.c | 18 ++++++++++++++
  drivers/staging/media/imx/imx-media-dev.c     | 24 +++++++++++++++++++
  2 files changed, 42 insertions(+)

drivers/staging/media/imx/imx-media-capture.c +15 -0

index b37e1186eb2f..4dfbe05d203e 100644
@@ -335,6 +335,21 @@ static int capture_s_parm(struct file *file, void *fh,
  	return 0;
  }
 
+static int capture_subscribe_event(struct v4l2_fh *fh,
+				   const struct v4l2_event_subscription *sub)
+{
+	switch (sub->type) {
+	case V4L2_EVENT_IMX_FRAME_INTERVAL_ERROR:
+		return v4l2_event_subscribe(fh, sub, 0, NULL);
+	case V4L2_EVENT_SOURCE_CHANGE:
+		return v4l2_src_change_event_subscribe(fh, sub);
+	case V4L2_EVENT_CTRL:
+		return v4l2_ctrl_subscribe_event(fh, sub);
+	default:
+		return -EINVAL;
+	}
+}
+
  static const struct v4l2_ioctl_ops capture_ioctl_ops = {
  	.vidioc_querycap	= vidioc_querycap,

Quoted 10 lines

drivers/staging/media/imx/imx-media-capture.c static const struct v4l2_ioctl_ops capture_ioctl_ops = { · line 383
Quoted 8 lines
drivers/staging/media/imx/imx-media-capture.c +3 -0
@@ -362,6 +377,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = {
  	.vidioc_expbuf		= vb2_ioctl_expbuf,
  	.vidioc_streamon	= vb2_ioctl_streamon,
  	.vidioc_streamoff	= vb2_ioctl_streamoff,
+
+	.vidioc_subscribe_event = capture_subscribe_event,
+	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
  };
This part of the patch adds event support to the capture device, can't this be split up into a separate patch? It seems to be useful in its own right.

Ok, I will split this up. The first patch will add the (un)subscribe_event callbacks to the capture device, and the second patch will forward subdev events.

Quoted 24 lines

drivers/staging/media/imx/imx-media-capture.c +0 -0

/*

drivers/staging/media/imx/imx-media-dev.c +13 -0

index 4b344a4a3706..25e916562c66 100644
@@ -442,6 +442,29 @@ static const struct media_device_ops imx_media_md_ops = {
  	.link_notify = imx_media_link_notify,
  };
 
+static void imx_media_notify(struct v4l2_subdev *sd,
+			     unsigned int notification,
+			     void *arg)
+{
+	struct media_entity *entity = &sd->entity;
+	int i;
+
+	if (notification != V4L2_DEVICE_NOTIFY_EVENT)
+		return;
+
+	for (i = 0; i < entity->num_pads; i++) {
+		struct media_pad *pad = &entity->pads[i];
+		struct imx_media_pad_vdev *pad_vdev;

Quoted 8 lines

drivers/staging/media/imx/imx-media-dev.c static const struct media_device_ops imx_media_md_ops = { · line 464
Quoted 7 lines
drivers/staging/media/imx/imx-media-dev.c +7 -0
+		struct list_head *pad_vdev_list;
+
+		pad_vdev_list = to_pad_vdev_list(sd, pad->index);
+		if (!pad_vdev_list)
+			continue;
+		list_for_each_entry(pad_vdev, pad_vdev_list, list)
+			v4l2_event_queue(pad_vdev->vdev->vfd, arg);
Which events do you want to forward?

Shouldn't any/all subdev events be forwarded?

I would also prefer to allow userland to subscribe to any events that a subdevice might generate. In other words, imx-media will allow the user to subscribe to, and receive any events that might be generated by subdevices.

Quoted 2 lines

E.g. forwarding control events
doesn't seem right, but other events may be useful.

The imx capture devices inherit controls from the subdevices. If an inherited control is changed via the capture device, will _two_ control events be generated (one from the subdevice and one from the capture device)? Or will only one event get generated to the capture device? Same question goes when changing an inherited control from the subdevice nodes.

Quoted 2 lines

Are those events
also appearing on the v4l-subdevX device? And if so, should they?

How do we know what subdevices a future imx-based system might attach to the media graph? That's why I think it makes sense to forward any/all events from subdevices.

Steve

Quoted 19 lines

Quoted 9 lines
drivers/staging/media/imx/imx-media-dev.c +3 -0
+	}
+}
+
  static int imx_media_probe(struct platform_device *pdev)
  {
  	struct device *dev = &pdev->dev;
drivers/staging/media/imx/imx-media-dev.c static int imx_media_probe(struct platform_device *pdev) · line 491
Quoted 7 lines
drivers/staging/media/imx/imx-media-dev.c +1 -0
@@ -462,6 +485,7 @@ static int imx_media_probe(struct platform_device *pdev)
  	mutex_init(&imxmd->mutex);
 
  	imxmd->v4l2_dev.mdev = &imxmd->md;
+	imxmd->v4l2_dev.notify = imx_media_notify;
  	strscpy(imxmd->v4l2_dev.name, "imx-media",
  		sizeof(imxmd->v4l2_dev.name));
 
Regards,
Hans