| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <acc89e1c-8115-e585-3040-60fe03799300@gmail.com>
Date: Tue, 8 Jan 2019 11:14:37 -0800
From: Steve Longerbeam <slongerbeam@...il.com>
To: Hans Verkuil <hverkuil@...all.nl>, linux-media@...r.kernel.org
Cc: Philipp Zabel <p.zabel@...gutronix.de>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"open list:STAGING SUBSYSTEM" <devel@...verdev.osuosl.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] media: imx: queue subdev events to reachable video
devices
Hi Hans,
On 1/8/19 5:26 AM, Hans Verkuil wrote:
> On 12/09/18 20:57, Steve Longerbeam wrote:
>> From: Steve Longerbeam <slongerbeam@...il.com>
>>
>> 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.
Ok, I'll add more explanation.
>
>> Signed-off-by: Steve Longerbeam <slongerbeam@...il.com>
>> ---
>> drivers/staging/media/imx/imx-media-capture.c | 18 ++++++++++++++
>> drivers/staging/media/imx/imx-media-dev.c | 24 +++++++++++++++++++
>> 2 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
>> index b37e1186eb2f..4dfbe05d203e 100644
>> --- a/drivers/staging/media/imx/imx-media-capture.c
>> +++ b/drivers/staging/media/imx/imx-media-capture.c
>> @@ -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,
>> };
> 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.
>
>>
>> /*
>> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
>> index 4b344a4a3706..25e916562c66 100644
>> --- a/drivers/staging/media/imx/imx-media-dev.c
>> +++ b/drivers/staging/media/imx/imx-media-dev.c
>> @@ -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);
> 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.
> 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.
> 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
>
>> + }
>> +}
>> +
>> 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));
>>
>>
> Regards,
>
> Hans
Powered by blists - more mailing lists