cron job: media_tree daily build: ERRORS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Thu Sep 13 04:00:17 CEST 2018 media-tree git hash:78cf8c842c111df656c63b5d04997ea4e40ef26a media_build git hash: 73a39eb63460f29cbe9bc056ae0b05ce9e813b11 v4l-utils git hash: d26e4941419b05fcb2b6708ee32aef367c2ec4af edid-decode git hash: b2da1516df3cc2756bfe8d1fa06d7bf2562ba1f4 gcc version:i686-linux-gcc (GCC) 8.2.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.17.0-3-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-2.6.36.4-i686: WARNINGS linux-2.6.36.4-x86_64: ERRORS linux-2.6.37.6-i686: WARNINGS linux-2.6.37.6-x86_64: WARNINGS linux-2.6.38.8-i686: WARNINGS linux-2.6.38.8-x86_64: WARNINGS linux-2.6.39.4-i686: WARNINGS linux-2.6.39.4-x86_64: WARNINGS linux-3.0.101-i686: WARNINGS linux-3.0.101-x86_64: WARNINGS linux-3.1.10-i686: WARNINGS linux-3.1.10-x86_64: WARNINGS linux-3.2.102-i686: WARNINGS linux-3.2.102-x86_64: WARNINGS linux-3.3.8-i686: WARNINGS linux-3.3.8-x86_64: WARNINGS linux-3.4.113-i686: WARNINGS linux-3.4.113-x86_64: WARNINGS linux-3.5.7-i686: WARNINGS linux-3.5.7-x86_64: WARNINGS linux-3.6.11-i686: WARNINGS linux-3.6.11-x86_64: WARNINGS linux-3.7.10-i686: WARNINGS linux-3.7.10-x86_64: WARNINGS linux-3.8.13-i686: WARNINGS linux-3.8.13-x86_64: WARNINGS linux-3.9.11-i686: WARNINGS linux-3.9.11-x86_64: WARNINGS linux-3.10.108-i686: WARNINGS linux-3.10.108-x86_64: WARNINGS linux-3.11.10-i686: WARNINGS linux-3.11.10-x86_64: WARNINGS linux-3.12.74-i686: WARNINGS linux-3.12.74-x86_64: WARNINGS linux-3.13.11-i686: WARNINGS linux-3.13.11-x86_64: WARNINGS linux-3.14.79-i686: WARNINGS linux-3.14.79-x86_64: WARNINGS linux-3.15.10-i686: WARNINGS linux-3.15.10-x86_64: WARNINGS linux-3.16.57-i686: WARNINGS linux-3.16.57-x86_64: WARNINGS linux-3.17.8-i686: WARNINGS linux-3.17.8-x86_64: WARNINGS linux-3.18.119-i686: OK linux-3.18.119-x86_64: OK linux-3.19.8-i686: WARNINGS linux-3.19.8-x86_64: WARNINGS linux-4.0.9-i686: WARNINGS linux-4.0.9-x86_64: WARNINGS linux-4.1.52-i686: WARNINGS linux-4.1.52-x86_64: WARNINGS linux-4.2.8-i686: WARNINGS linux-4.2.8-x86_64: WARNINGS linux-4.3.6-i686: OK linux-4.3.6-x86_64: OK linux-4.4.152-i686: OK linux-4.4.152-x86_64: OK linux-4.5.7-i686: OK linux-4.5.7-x86_64: OK linux-4.6.7-i686: OK linux-4.6.7-x86_64: OK linux-4.7.10-i686: OK linux-4.7.10-x86_64: OK linux-4.8.17-i686: OK linux-4.8.17-x86_64: OK linux-4.9.124-i686: OK linux-4.9.124-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: OK linux-4.11.12-i686: OK linux-4.11.12-x86_64: OK linux-4.12.14-i686: OK linux-4.12.14-x86_64: OK linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.67-i686: OK linux-4.14.67-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.19-i686: OK linux-4.17.19-x86_64: OK linux-4.18.5-i686: OK linux-4.18.5-x86_64: OK linux-4.19-rc1-i686: OK linux-4.19-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Logs weren't copied as they are too large (2368 kB) The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
[PATCH v2 1/1] v4l: event: Prevent freeing event subscriptions while accessed
The event subscriptions are added to the subscribed event list while
holding a spinlock, but that lock is subsequently released while still
accessing the subscription object. This makes it possible to unsubscribe
the event --- and freeing the subscription object's memory --- while
the subscription object is simultaneously accessed.
Prevent this by adding a mutex to serialise the event subscription and
unsubscription. This also gives a guarantee to the callback ops that the
add op has returned before the del op is called.
This change also results in making the elems field less special:
subscriptions are only added to the event list once they are fully
initialised.
Signed-off-by: Sakari Ailus
---
since v1:
- Call the mutex field subscribe_lock instead.
- Move the field that is now subscribe_lock above the subscribed field the
write access to which it serialises.
- Improve documentation of the subscribe_lock field.
drivers/media/v4l2-core/v4l2-event.c | 35 ++-
drivers/media/v4l2-core/v4l2-fh.c| 2 ++
include/media/v4l2-fh.h | 4
3 files changed, 24 insertions(+), 17 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index 127fe6eb91d9..b76fd92e24c8 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
const struct v4l2_event *e
if (sev == NULL)
return;
- /*
-* If the event has been added to the fh->subscribed list, but its
-* add op has not completed yet elems will be 0, treat this as
-* not being subscribed.
-*/
- if (!sev->elems)
- return;
-
/* Increase event sequence number on fh. */
fh->sequence++;
@@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
struct v4l2_subscribed_event *sev, *found_ev;
unsigned long flags;
unsigned i;
+ int ret = 0;
if (sub->type == V4L2_EVENT_ALL)
return -EINVAL;
@@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
sev->flags = sub->flags;
sev->fh = fh;
sev->ops = ops;
+ sev->elems = elems;
+
+ mutex_lock(>subscribe_lock);
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (!found_ev)
- list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
+ /* Already listening */
kvfree(sev);
- return 0; /* Already listening */
+ goto out_unlock;
}
if (sev->ops && sev->ops->add) {
- int ret = sev->ops->add(sev, elems);
+ ret = sev->ops->add(sev, elems);
if (ret) {
- sev->ops = NULL;
- v4l2_event_unsubscribe(fh, sub);
- return ret;
+ kvfree(sev);
+ goto out_unlock;
}
}
/* Mark as ready for use */
- sev->elems = elems;
+ list_add(>list, >subscribed);
- return 0;
+out_unlock:
+ mutex_unlock(>subscribe_lock);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
@@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
return 0;
}
+ mutex_lock(>subscribe_lock);
+
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
@@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
if (sev && sev->ops && sev->ops->del)
sev->ops->del(sev);
+ mutex_unlock(>subscribe_lock);
+
kvfree(sev);
return 0;
diff --git a/drivers/media/v4l2-core/v4l2-fh.c
b/drivers/media/v4l2-core/v4l2-fh.c
index 3895999bf880..c91a7bd3ecfc 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device
*vdev)
INIT_LIST_HEAD(>available);
INIT_LIST_HEAD(>subscribed);
fh->sequence = -1;
+ mutex_init(>subscribe_lock);
}
EXPORT_SYMBOL_GPL(v4l2_fh_init);
@@ -90,6 +91,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
return;
v4l_disable_media_source(fh->vdev);
v4l2_event_unsubscribe_all(fh);
+ mutex_destroy(>subscribe_lock);
fh->vdev = NULL;
}
EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index ea73fef8bdc0..8586cfb49828 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -38,10 +38,13 @@ struct v4l2_ctrl_handler;
* @prio: priority of the file handler, as defined by v4l2_priority
*
* @wait: event' s wait queue
+ * @subscribe_lock: serialise
Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed
Hi Laurent, On Wed, Sep 12, 2018 at 02:57:20PM +0300, Laurent Pinchart wrote: > Hello, > > On Wednesday, 12 September 2018 13:00:57 EEST Sakari Ailus wrote: > > On Wed, Sep 12, 2018 at 11:27:35AM +0200, Hans Verkuil wrote: > > > On 09/12/18 10:52, Sakari Ailus wrote: > > >> The event subscriptions are added to the subscribed event list while > > >> holding a spinlock, but that lock is subsequently released while still > > >> accessing the subscription object. This makes it possible to unsubscribe > > >> the event --- and freeing the subscription object's memory --- while > > >> the subscription object is simultaneously accessed. > > > > > > Hmm, the (un)subscribe ioctls are serialized through the ioctl lock, > > > so this could only be a scenario with drivers that do not use this > > > lock. Off-hand the only driver I know that does this is uvc. > > > Unfortunately, > > > that's a rather popular one. > > > > On video nodes, perhaps. But how about sub-device nodes? Generally drivers > > tend to do locking themselves, whether or not that is the best for most > > drivers. > > I tend to agree with Sakari. Furthermore, having fine-grained locking is > better in my opinion than locking everything at the ioctl level, for drivers > that wish to do so. We should thus strive for self-contained locking in the > different helper libraries of V4L2. > > > >> Prevent this by adding a mutex to serialise the event subscription and > > >> unsubscription. This also gives a guarantee to the callback ops that the > > >> add op has returned before the del op is called. > > >> > > >> This change also results in making the elems field less special: > > >> subscriptions are only added to the event list once they are fully > > >> initialised. > > >> > > >> Signed-off-by: Sakari Ailus > > >> --- > > >> Hi folks, > > >> > > >> I noticed this while working to add support for media events. This seems > > >> like material for the stable trees. > > > > > > I'd say 'no need for this' if it wasn't for uvc. > > > > > >> drivers/media/v4l2-core/v4l2-event.c | 35 - > > >> drivers/media/v4l2-core/v4l2-fh.c| 2 ++ > > >> include/media/v4l2-fh.h | 4 > > >> 3 files changed, 24 insertions(+), 17 deletions(-) > > [snip] > > > >> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h > > >> index ea73fef8bdc0..1be45a5f6383 100644 > > >> --- a/include/media/v4l2-fh.h > > >> +++ b/include/media/v4l2-fh.h > > >> @@ -42,6 +42,9 @@ struct v4l2_ctrl_handler; > > >> * @available: list of events waiting to be dequeued > > >> * @navailable: number of available events at @available list > > >> * @sequence: event sequence number > > >> + * @mutex: hold event subscriptions during subscribing; > > >> + * guarantee that the add and del event callbacks are orderly > > >> called > > Could you try to describe what this mutex protects in terms of data ? The purpose actually changed a bit after I wrote the text. That could be the reason why it's sort of detached from the reality. :-) How about this: mutex: serialise changes to the subscribed list; guarantee that the add and del event callbacks are orderly called > > > >> + * > > Extra blank line ? A line that separates the event related fields from the m2m context. There's one above the event related ones, too. I didn't think it's worth mentioning that in the patch description. :-) -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v2 18/23] v4l: fwnode: Use media bus type for bus parser selection
Hi Jacopo, Thanks for the comments. On Wed, Sep 12, 2018 at 05:15:05PM +0200, jacopo mondi wrote: > Hi Sakari, > > On Mon, Aug 27, 2018 at 12:29:55PM +0300, Sakari Ailus wrote: > > Use the media bus types instead of the fwnode bus types internally. This > > is the interface to the drivers as well, making the use of the fwnode bus > > types more localised to the V4L2 fwnode framework. > > > > So basically now "v4l2_fwnode_bus_type" it is only used in a few > places in v4l2-fwnode and has to be kept in sync with the bus types > listed in the devicetree bindings documentation? Correct. > > Do you think it is still worth to keep around functions dealing with > that enum type as "v4l2_fwnode_bus_type_to_string()" ? > It is only used by a debug printout (without that much value added, as > we can print out the integer parsed from the DT). In all other cases > it can be converted to the corresponing v4l2_mbus_type immediately. One of the aims of this patchset is to make debugging easier. A string is more informative to the developers than a number, and for a few additional lines of code I think that is justifiable. I'll send v3 probably early tomorrow as I fixed a build issue; V4L2_MBUS_UNKNOWN was used in one patch that preceded its introduction. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v2 07/23] v4l: fwnode: Let the caller provide V4L2 fwnode endpoint
Hi Jacopo, On Wed, Sep 12, 2018 at 04:51:07PM +0200, jacopo mondi wrote: > Hi Sakari, > > On Mon, Aug 27, 2018 at 12:29:44PM +0300, Sakari Ailus wrote: > > Instead of allocating the V4L2 fwnode endpoint in > > v4l2_fwnode_endpoint_alloc_parse, let the caller to do this. This allows > > setting default parameters for the endpoint which is a very common need > > for drivers. > > > > Signed-off-by: Sakari Ailus > > --- > > drivers/media/i2c/ov2659.c | 14 +- > > drivers/media/i2c/smiapp/smiapp-core.c | 26 +- > > drivers/media/i2c/tc358743.c | 26 +- > > drivers/media/v4l2-core/v4l2-fwnode.c | 49 > > +- > > include/media/v4l2-fwnode.h| 10 --- > > 5 files changed, 60 insertions(+), 65 deletions(-) > > > > [snip] > > > -struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse( > > - struct fwnode_handle *fwnode) > > +int v4l2_fwnode_endpoint_alloc_parse( > > + struct fwnode_handle *fwnode, struct v4l2_fwnode_endpoint *vep) > > Looking at the resulting implementation of > "v4l2_fwnode_endpoint_alloc_parse" and "v4l2_fwnode_endpoint_parse" I > wonder if there's still value in keeping them separate... Now that in > both cases the caller has to provide an v4l2_fwnode_endpoint, isn't it > worth making a single function out of them, that behaves like > "alloc_parse" is doing nowadays (allocates vep->link_frequencies > conditionally on the presence of the "link-frequencies" property) ? The problem with that would be that the caller would have to know if there are variable length properties, i.e. the caller would always have to call v4l2_fwnode_endpoint_free() once it no longer needs them. For quite a few drivers this means immediately after calling the function which parsed them. I prefer to keep this simple for the drivers that need no such properties. > > Or is the size of the allocated vep relevant in the async subdevice > matching or registration process? I guess not, but I might be missing > something... It's not. The link frequencies are a pointer anyway. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed
Hi Hans,
On Wed, Sep 12, 2018 at 02:32:52PM +0200, Hans Verkuil wrote:
> On 09/12/18 12:00, Sakari Ailus wrote:
> > Hi Hans,
> >
> > Thanks for the quick review.
> >
> > On Wed, Sep 12, 2018 at 11:27:35AM +0200, Hans Verkuil wrote:
> >> On 09/12/18 10:52, Sakari Ailus wrote:
> >>> The event subscriptions are added to the subscribed event list while
> >>> holding a spinlock, but that lock is subsequently released while still
> >>> accessing the subscription object. This makes it possible to unsubscribe
> >>> the event --- and freeing the subscription object's memory --- while
> >>> the subscription object is simultaneously accessed.
> >>
> >> Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
> >> so this could only be a scenario with drivers that do not use this
> >> lock. Off-hand the only driver I know that does this is uvc. Unfortunately,
> >> that's a rather popular one.
> >
> > On video nodes, perhaps. But how about sub-device nodes? Generally drivers
> > tend to do locking themselves, whether or not that is the best for most
> > drivers.
>
> That's a whole different discussion :-)
Not entirely: the sub-device drivers do expose the sub-device event interface
to the user space and thus the bug affects those as well.
>
> I've never been convinced by this since 1) it's hard to prove correctness
> if drivers handle locking and 2) I've never seen any proof that it actually
> improves performance.
>
> Anyway, it's unrelated to this patch.
Yes, I agree on that.
>
> >
> >>
> >>>
> >>> Prevent this by adding a mutex to serialise the event subscription and
> >>> unsubscription. This also gives a guarantee to the callback ops that the
> >>> add op has returned before the del op is called.
> >>>
> >>> This change also results in making the elems field less special:
> >>> subscriptions are only added to the event list once they are fully
> >>> initialised.
> >>>
> >>> Signed-off-by: Sakari Ailus
> >>> ---
> >>> Hi folks,
> >>>
> >>> I noticed this while working to add support for media events. This seems
> >>> like material for the stable trees.
> >>
> >> I'd say 'no need for this' if it wasn't for uvc.
> >>
> >>>
> >>> drivers/media/v4l2-core/v4l2-event.c | 35
> >>> ++-
> >>> drivers/media/v4l2-core/v4l2-fh.c| 2 ++
> >>> include/media/v4l2-fh.h | 4
> >>> 3 files changed, 24 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> >>> b/drivers/media/v4l2-core/v4l2-event.c
> >>> index 127fe6eb91d9..74161f79e4d3 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-event.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-event.c
> >>> @@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh
> >>> *fh, const struct v4l2_event *e
> >>> if (sev == NULL)
> >>> return;
> >>>
> >>> - /*
> >>> - * If the event has been added to the fh->subscribed list, but its
> >>> - * add op has not completed yet elems will be 0, treat this as
> >>> - * not being subscribed.
> >>> - */
> >>> - if (!sev->elems)
> >>> - return;
> >>> -
> >>> /* Increase event sequence number on fh. */
> >>> fh->sequence++;
> >>>
> >>> @@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> >>> struct v4l2_subscribed_event *sev, *found_ev;
> >>> unsigned long flags;
> >>> unsigned i;
> >>> + int ret = 0;
> >>>
> >>> if (sub->type == V4L2_EVENT_ALL)
> >>> return -EINVAL;
> >>> @@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> >>> sev->flags = sub->flags;
> >>> sev->fh = fh;
> >>> sev->ops = ops;
> >>> + sev->elems = elems;
> >>> +
> >>> + mutex_lock(>mutex);
> >>>
> >>> spin_lock_irqsave(>vdev->fh_lock, flags);
> >>> found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> >>> - if (!found_ev)
> >>> - list_add(>list, >subscribed);
> >>> spin_unlock_irqrestore(>vdev->fh_lock, flags);
> >>>
> >>> if (found_ev) {
> >>> + /* Already listening */
> >>> kvfree(sev);
> >>> - return 0; /* Already listening */
> >>> + goto out_unlock;
> >>> }
> >>>
> >>> if (sev->ops && sev->ops->add) {
> >>> - int ret = sev->ops->add(sev, elems);
> >>> + ret = sev->ops->add(sev, elems);
> >>> if (ret) {
> >>> - sev->ops = NULL;
> >>> - v4l2_event_unsubscribe(fh, sub);
> >>> - return ret;
> >>> + kvfree(sev);
> >>> + goto out_unlock;
> >>> }
> >>> }
> >>>
> >>> /* Mark as ready for use */
> >>> - sev->elems = elems;
> >>> + list_add(>list, >subscribed);
> >>>
> >>> - return 0;
> >>> +out_unlock:
> >>> + mutex_unlock(>mutex);
> >>> +
> >>> + return ret;
> >>> }
> >>> EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> >>>
> >>> @@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> >>> return 0;
> >>> }
> >>>
> >>> + mutex_lock(>mutex);
> >>> +
> >>>
Re: [PATCH v2 18/23] v4l: fwnode: Use media bus type for bus parser selection
Hi Sakari,
On Mon, Aug 27, 2018 at 12:29:55PM +0300, Sakari Ailus wrote:
> Use the media bus types instead of the fwnode bus types internally. This
> is the interface to the drivers as well, making the use of the fwnode bus
> types more localised to the V4L2 fwnode framework.
>
So basically now "v4l2_fwnode_bus_type" it is only used in a few
places in v4l2-fwnode and has to be kept in sync with the bus types
listed in the devicetree bindings documentation?
Do you think it is still worth to keep around functions dealing with
that enum type as "v4l2_fwnode_bus_type_to_string()" ?
It is only used by a debug printout (without that much value added, as
we can print out the integer parsed from the DT). In all other cases
it can be converted to the corresponing v4l2_mbus_type immediately.
> Signed-off-by: Sakari Ailus
> ---
> drivers/media/v4l2-core/v4l2-fwnode.c | 100
> +++---
> 1 file changed, 80 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 6c5a76442667..d502abd7406b 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -42,9 +42,66 @@ enum v4l2_fwnode_bus_type {
> NR_OF_V4L2_FWNODE_BUS_TYPE,
> };
>
> +static const struct v4l2_fwnode_bus_conv {
> + enum v4l2_fwnode_bus_type fwnode_bus_type;
> + enum v4l2_mbus_type mbus_type;
> + const char *name;
> +} busses[] = {
> + {
> + V4L2_FWNODE_BUS_TYPE_GUESS,
> + V4L2_MBUS_UNKNOWN,
> + "not specified",
> + }, {
> + V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
> + V4L2_MBUS_CSI2_CPHY,
> + "MIPI CSI-2 C-PHY",
> + }, {
> + V4L2_FWNODE_BUS_TYPE_CSI1,
> + V4L2_MBUS_CSI1,
> + "MIPI CSI-1",
> + }, {
> + V4L2_FWNODE_BUS_TYPE_CCP2,
> + V4L2_MBUS_CCP2,
> + "compact camera port 2",
> + }, {
> + V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
> + V4L2_MBUS_CSI2_DPHY,
> + "MIPI CSI-2 D-PHY",
> + }, {
> + V4L2_FWNODE_BUS_TYPE_PARALLEL,
> + V4L2_MBUS_PARALLEL,
> + "parallel",
> + }, {
> + V4L2_FWNODE_BUS_TYPE_BT656,
> + V4L2_MBUS_BT656,
> + "Bt.656",
> + }
> +};
> +
> +static const struct v4l2_fwnode_bus_conv *
> +get_v4l2_fwnode_bus_conv_by_fwnode_bus(enum v4l2_fwnode_bus_type type)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(busses); i++)
> + if (busses[i].fwnode_bus_type == type)
> + return [i];
> +
> + return NULL;
> +}
> +
> +static enum v4l2_mbus_type
> +v4l2_fwnode_bus_type_to_mbus(enum v4l2_fwnode_bus_type type)
> +{
> + const struct v4l2_fwnode_bus_conv *conv =
> + get_v4l2_fwnode_bus_conv_by_fwnode_bus(type);
> +
> + return conv ? conv->mbus_type : V4L2_MBUS_UNKNOWN;
> +}
> +
> static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> struct v4l2_fwnode_endpoint *vep,
> -enum v4l2_fwnode_bus_type
> bus_type)
> +enum v4l2_mbus_type bus_type)
> {
> struct v4l2_fwnode_bus_mipi_csi2 *bus = >bus.mipi_csi2;
> bool have_clk_lane = false, have_data_lanes = false,
> @@ -58,7 +115,7 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct
> fwnode_handle *fwnode,
> u32 v;
> int rval;
>
> - if (bus_type == V4L2_FWNODE_BUS_TYPE_CSI2_DPHY) {
> + if (bus_type == V4L2_MBUS_CSI2_DPHY) {
> use_default_lane_mapping = true;
>
> num_data_lanes = min_t(u32, bus->num_data_lanes,
> @@ -134,7 +191,7 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct
> fwnode_handle *fwnode,
> flags |= V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
> }
>
> - if (bus_type == V4L2_FWNODE_BUS_TYPE_CSI2_DPHY || lanes_used ||
> + if (bus_type == V4L2_MBUS_CSI2_DPHY || lanes_used ||
> have_clk_lane || (flags & ~V4L2_MBUS_CSI2_CONTINUOUS_CLOCK)) {
> bus->flags = flags;
> vep->bus_type = V4L2_MBUS_CSI2_DPHY;
> @@ -177,7 +234,7 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct
> fwnode_handle *fwnode,
>
> static void v4l2_fwnode_endpoint_parse_parallel_bus(
> struct fwnode_handle *fwnode, struct v4l2_fwnode_endpoint *vep,
> - enum v4l2_fwnode_bus_type bus_type)
> + enum v4l2_mbus_type bus_type)
> {
> struct v4l2_fwnode_bus_parallel *bus = >bus.parallel;
> unsigned int flags = 0;
> @@ -274,11 +331,11 @@ static void v4l2_fwnode_endpoint_parse_parallel_bus(
> else
> vep->bus_type = V4L2_MBUS_BT656;
> break;
> - case V4L2_FWNODE_BUS_TYPE_PARALLEL:
> + case V4L2_MBUS_PARALLEL:
> vep->bus_type = V4L2_MBUS_PARALLEL;
>
Re: [PATCH v2 07/23] v4l: fwnode: Let the caller provide V4L2 fwnode endpoint
Hi Sakari,
On Mon, Aug 27, 2018 at 12:29:44PM +0300, Sakari Ailus wrote:
> Instead of allocating the V4L2 fwnode endpoint in
> v4l2_fwnode_endpoint_alloc_parse, let the caller to do this. This allows
> setting default parameters for the endpoint which is a very common need
> for drivers.
>
> Signed-off-by: Sakari Ailus
> ---
> drivers/media/i2c/ov2659.c | 14 +-
> drivers/media/i2c/smiapp/smiapp-core.c | 26 +-
> drivers/media/i2c/tc358743.c | 26 +-
> drivers/media/v4l2-core/v4l2-fwnode.c | 49
> +-
> include/media/v4l2-fwnode.h| 10 ---
> 5 files changed, 60 insertions(+), 65 deletions(-)
>
[snip]
> -struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
> - struct fwnode_handle *fwnode)
> +int v4l2_fwnode_endpoint_alloc_parse(
> + struct fwnode_handle *fwnode, struct v4l2_fwnode_endpoint *vep)
Looking at the resulting implementation of
"v4l2_fwnode_endpoint_alloc_parse" and "v4l2_fwnode_endpoint_parse" I
wonder if there's still value in keeping them separate... Now that in
both cases the caller has to provide an v4l2_fwnode_endpoint, isn't it
worth making a single function out of them, that behaves like
"alloc_parse" is doing nowadays (allocates vep->link_frequencies
conditionally on the presence of the "link-frequencies" property) ?
Or is the size of the allocated vep relevant in the async subdevice
matching or registration process? I guess not, but I might be missing
something...
Thanks
j
> {
> - struct v4l2_fwnode_endpoint *vep;
> int rval;
>
> - vep = kzalloc(sizeof(*vep), GFP_KERNEL);
> - if (!vep)
> - return ERR_PTR(-ENOMEM);
> -
> rval = __v4l2_fwnode_endpoint_parse(fwnode, vep);
> if (rval < 0)
> - goto out_err;
> + return rval;
>
> rval = fwnode_property_read_u64_array(fwnode, "link-frequencies",
> NULL, 0);
> @@ -316,18 +310,18 @@ struct v4l2_fwnode_endpoint
> *v4l2_fwnode_endpoint_alloc_parse(
> vep->link_frequencies =
> kmalloc_array(rval, sizeof(*vep->link_frequencies),
> GFP_KERNEL);
> - if (!vep->link_frequencies) {
> - rval = -ENOMEM;
> - goto out_err;
> - }
> + if (!vep->link_frequencies)
> + return -ENOMEM;
>
> vep->nr_of_link_frequencies = rval;
>
> rval = fwnode_property_read_u64_array(
> fwnode, "link-frequencies", vep->link_frequencies,
> vep->nr_of_link_frequencies);
> - if (rval < 0)
> - goto out_err;
> + if (rval < 0) {
> + v4l2_fwnode_endpoint_free(vep);
> + return rval;
> + }
>
> for (i = 0; i < vep->nr_of_link_frequencies; i++)
> pr_info("link-frequencies %u value %llu\n", i,
> @@ -336,11 +330,7 @@ struct v4l2_fwnode_endpoint
> *v4l2_fwnode_endpoint_alloc_parse(
>
> pr_debug("= end V4L2 endpoint properties\n");
>
> - return vep;
> -
> -out_err:
> - v4l2_fwnode_endpoint_free(vep);
> - return ERR_PTR(rval);
> + return 0;
> }
> EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_alloc_parse);
>
> @@ -392,9 +382,9 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd))
> {
> + struct v4l2_fwnode_endpoint vep = { .bus_type = V4L2_MBUS_UNKNOWN };
> struct v4l2_async_subdev *asd;
> - struct v4l2_fwnode_endpoint *vep;
> - int ret = 0;
> + int ret;
>
> asd = kzalloc(asd_struct_size, GFP_KERNEL);
> if (!asd)
> @@ -409,23 +399,22 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
> goto out_err;
> }
>
> - vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> - if (IS_ERR(vep)) {
> - ret = PTR_ERR(vep);
> + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, );
> + if (ret) {
> dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
>ret);
> goto out_err;
> }
>
> - ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
> + ret = parse_endpoint ? parse_endpoint(dev, , asd) : 0;
> if (ret == -ENOTCONN)
> - dev_dbg(dev, "ignoring port@%u/endpoint@%u\n", vep->base.port,
> - vep->base.id);
> + dev_dbg(dev, "ignoring port@%u/endpoint@%u\n", vep.base.port,
> + vep.base.id);
> else if (ret < 0)
> dev_warn(dev,
>"driver could not parse port@%u/endpoint@%u (%d)\n",
> - vep->base.port, vep->base.id, ret);
> - v4l2_fwnode_endpoint_free(vep);
> +
Question 16
Hi, We are waiting for your photos. Please send photos to hansre...@outlook.com for further editing. Our team is ready to edit your photos. Do you have photos for editing? We are team who can do following work for you. Cutting out your photos, or retouching if needed. It is for products photos or portrait photos, catalog photos. Please send photo editing work to hansre...@outlook.com Thanks, Jimmy Button Email: hansre...@outlook.com
Re: [git:media_tree/master] media: em28xx-audio: use GFP_KERNEL for memory allocation during init
On 2018-09-12 14:43:11 [+0200], To linux-media@vger.kernel.org wrote: > On 2018-09-12 12:00:36 [+], Mauro Carvalho Chehab wrote: > > This is an automatic generated email to let you know that the following > > patch were queued: > > The three patches you just enqueued are also in Greg's USB tree for the > next merge-window. Just ignore what I just wrote here. I didn't take my pills yet. Sebastian
Re: [git:media_tree/master] media: em28xx-audio: use GFP_KERNEL for memory allocation during init
On 2018-09-12 12:00:36 [+], Mauro Carvalho Chehab wrote: > This is an automatic generated email to let you know that the following patch > were queued: The three patches you just enqueued are also in Greg's USB tree for the next merge-window. Sebastian
Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed
On 09/12/18 12:00, Sakari Ailus wrote:
> Hi Hans,
>
> Thanks for the quick review.
>
> On Wed, Sep 12, 2018 at 11:27:35AM +0200, Hans Verkuil wrote:
>> On 09/12/18 10:52, Sakari Ailus wrote:
>>> The event subscriptions are added to the subscribed event list while
>>> holding a spinlock, but that lock is subsequently released while still
>>> accessing the subscription object. This makes it possible to unsubscribe
>>> the event --- and freeing the subscription object's memory --- while
>>> the subscription object is simultaneously accessed.
>>
>> Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
>> so this could only be a scenario with drivers that do not use this
>> lock. Off-hand the only driver I know that does this is uvc. Unfortunately,
>> that's a rather popular one.
>
> On video nodes, perhaps. But how about sub-device nodes? Generally drivers
> tend to do locking themselves, whether or not that is the best for most
> drivers.
That's a whole different discussion :-)
I've never been convinced by this since 1) it's hard to prove correctness
if drivers handle locking and 2) I've never seen any proof that it actually
improves performance.
Anyway, it's unrelated to this patch.
>
>>
>>>
>>> Prevent this by adding a mutex to serialise the event subscription and
>>> unsubscription. This also gives a guarantee to the callback ops that the
>>> add op has returned before the del op is called.
>>>
>>> This change also results in making the elems field less special:
>>> subscriptions are only added to the event list once they are fully
>>> initialised.
>>>
>>> Signed-off-by: Sakari Ailus
>>> ---
>>> Hi folks,
>>>
>>> I noticed this while working to add support for media events. This seems
>>> like material for the stable trees.
>>
>> I'd say 'no need for this' if it wasn't for uvc.
>>
>>>
>>> drivers/media/v4l2-core/v4l2-event.c | 35
>>> ++-
>>> drivers/media/v4l2-core/v4l2-fh.c| 2 ++
>>> include/media/v4l2-fh.h | 4
>>> 3 files changed, 24 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-event.c
>>> b/drivers/media/v4l2-core/v4l2-event.c
>>> index 127fe6eb91d9..74161f79e4d3 100644
>>> --- a/drivers/media/v4l2-core/v4l2-event.c
>>> +++ b/drivers/media/v4l2-core/v4l2-event.c
>>> @@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
>>> const struct v4l2_event *e
>>> if (sev == NULL)
>>> return;
>>>
>>> - /*
>>> -* If the event has been added to the fh->subscribed list, but its
>>> -* add op has not completed yet elems will be 0, treat this as
>>> -* not being subscribed.
>>> -*/
>>> - if (!sev->elems)
>>> - return;
>>> -
>>> /* Increase event sequence number on fh. */
>>> fh->sequence++;
>>>
>>> @@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>>> struct v4l2_subscribed_event *sev, *found_ev;
>>> unsigned long flags;
>>> unsigned i;
>>> + int ret = 0;
>>>
>>> if (sub->type == V4L2_EVENT_ALL)
>>> return -EINVAL;
>>> @@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>>> sev->flags = sub->flags;
>>> sev->fh = fh;
>>> sev->ops = ops;
>>> + sev->elems = elems;
>>> +
>>> + mutex_lock(>mutex);
>>>
>>> spin_lock_irqsave(>vdev->fh_lock, flags);
>>> found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
>>> - if (!found_ev)
>>> - list_add(>list, >subscribed);
>>> spin_unlock_irqrestore(>vdev->fh_lock, flags);
>>>
>>> if (found_ev) {
>>> + /* Already listening */
>>> kvfree(sev);
>>> - return 0; /* Already listening */
>>> + goto out_unlock;
>>> }
>>>
>>> if (sev->ops && sev->ops->add) {
>>> - int ret = sev->ops->add(sev, elems);
>>> + ret = sev->ops->add(sev, elems);
>>> if (ret) {
>>> - sev->ops = NULL;
>>> - v4l2_event_unsubscribe(fh, sub);
>>> - return ret;
>>> + kvfree(sev);
>>> + goto out_unlock;
>>> }
>>> }
>>>
>>> /* Mark as ready for use */
>>> - sev->elems = elems;
>>> + list_add(>list, >subscribed);
>>>
>>> - return 0;
>>> +out_unlock:
>>> + mutex_unlock(>mutex);
>>> +
>>> + return ret;
>>> }
>>> EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
>>>
>>> @@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>>> return 0;
>>> }
>>>
>>> + mutex_lock(>mutex);
>>> +
>>> spin_lock_irqsave(>vdev->fh_lock, flags);
>>>
>>> sev = v4l2_event_subscribed(fh, sub->type, sub->id);
>>> @@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>>> if (sev && sev->ops && sev->ops->del)
>>> sev->ops->del(sev);
>>>
>>> + mutex_unlock(>mutex);
>>> +
>>> kvfree(sev);
>>>
>>> return 0;
>>> diff --git a/drivers/media/v4l2-core/v4l2-fh.c
>>>
Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed
Hello,
On Wednesday, 12 September 2018 13:00:57 EEST Sakari Ailus wrote:
> On Wed, Sep 12, 2018 at 11:27:35AM +0200, Hans Verkuil wrote:
> > On 09/12/18 10:52, Sakari Ailus wrote:
> >> The event subscriptions are added to the subscribed event list while
> >> holding a spinlock, but that lock is subsequently released while still
> >> accessing the subscription object. This makes it possible to unsubscribe
> >> the event --- and freeing the subscription object's memory --- while
> >> the subscription object is simultaneously accessed.
> >
> > Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
> > so this could only be a scenario with drivers that do not use this
> > lock. Off-hand the only driver I know that does this is uvc.
> > Unfortunately,
> > that's a rather popular one.
>
> On video nodes, perhaps. But how about sub-device nodes? Generally drivers
> tend to do locking themselves, whether or not that is the best for most
> drivers.
I tend to agree with Sakari. Furthermore, having fine-grained locking is
better in my opinion than locking everything at the ioctl level, for drivers
that wish to do so. We should thus strive for self-contained locking in the
different helper libraries of V4L2.
> >> Prevent this by adding a mutex to serialise the event subscription and
> >> unsubscription. This also gives a guarantee to the callback ops that the
> >> add op has returned before the del op is called.
> >>
> >> This change also results in making the elems field less special:
> >> subscriptions are only added to the event list once they are fully
> >> initialised.
> >>
> >> Signed-off-by: Sakari Ailus
> >> ---
> >> Hi folks,
> >>
> >> I noticed this while working to add support for media events. This seems
> >> like material for the stable trees.
> >
> > I'd say 'no need for this' if it wasn't for uvc.
> >
> >> drivers/media/v4l2-core/v4l2-event.c | 35 -
> >> drivers/media/v4l2-core/v4l2-fh.c| 2 ++
> >> include/media/v4l2-fh.h | 4
> >> 3 files changed, 24 insertions(+), 17 deletions(-)
[snip]
> >> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> >> index ea73fef8bdc0..1be45a5f6383 100644
> >> --- a/include/media/v4l2-fh.h
> >> +++ b/include/media/v4l2-fh.h
> >> @@ -42,6 +42,9 @@ struct v4l2_ctrl_handler;
> >> * @available: list of events waiting to be dequeued
> >> * @navailable: number of available events at @available list
> >> * @sequence: event sequence number
> >> + * @mutex: hold event subscriptions during subscribing;
> >> + * guarantee that the add and del event callbacks are orderly
> >> called
Could you try to describe what this mutex protects in terms of data ?
> >> + *
Extra blank line ?
> >> * @m2m_ctx: pointer to v4l2_m2m_ctx
> >> */
> >>
> >> struct v4l2_fh {
> >> @@ -56,6 +59,7 @@ struct v4l2_fh {
> >>struct list_headavailable;
> >>unsigned intnavailable;
> >>u32 sequence;
> >> + struct mutexmutex;
> >
> > I don't like the name 'mutex'. Perhaps something more descriptive like:
> > 'subscribe_lock'?
> >
> >> #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
> >>
> >>struct v4l2_m2m_ctx *m2m_ctx;
> >
> > Overall I think this patch makes sense. The code is cleaner and easier to
> > follow. Just give 'mutex' a better name :-)
>
> How about "subscribe_mutex"? It's a mutex... "subscribe_lock" would use a
> similar convention elsewhere in V4L2 where mutexes are commonly called
> locks, so I'm certainly fine with that as well.
We indeed use lock for both mutexes and spinlocks. I have a slight preference
for the name lock myself, but I don't mind too much.
--
Regards,
Laurent Pinchart
Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed
Hi Hans,
Thanks for the quick review.
On Wed, Sep 12, 2018 at 11:27:35AM +0200, Hans Verkuil wrote:
> On 09/12/18 10:52, Sakari Ailus wrote:
> > The event subscriptions are added to the subscribed event list while
> > holding a spinlock, but that lock is subsequently released while still
> > accessing the subscription object. This makes it possible to unsubscribe
> > the event --- and freeing the subscription object's memory --- while
> > the subscription object is simultaneously accessed.
>
> Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
> so this could only be a scenario with drivers that do not use this
> lock. Off-hand the only driver I know that does this is uvc. Unfortunately,
> that's a rather popular one.
On video nodes, perhaps. But how about sub-device nodes? Generally drivers
tend to do locking themselves, whether or not that is the best for most
drivers.
>
> >
> > Prevent this by adding a mutex to serialise the event subscription and
> > unsubscription. This also gives a guarantee to the callback ops that the
> > add op has returned before the del op is called.
> >
> > This change also results in making the elems field less special:
> > subscriptions are only added to the event list once they are fully
> > initialised.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > Hi folks,
> >
> > I noticed this while working to add support for media events. This seems
> > like material for the stable trees.
>
> I'd say 'no need for this' if it wasn't for uvc.
>
> >
> > drivers/media/v4l2-core/v4l2-event.c | 35
> > ++-
> > drivers/media/v4l2-core/v4l2-fh.c| 2 ++
> > include/media/v4l2-fh.h | 4
> > 3 files changed, 24 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-event.c
> > b/drivers/media/v4l2-core/v4l2-event.c
> > index 127fe6eb91d9..74161f79e4d3 100644
> > --- a/drivers/media/v4l2-core/v4l2-event.c
> > +++ b/drivers/media/v4l2-core/v4l2-event.c
> > @@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> > const struct v4l2_event *e
> > if (sev == NULL)
> > return;
> >
> > - /*
> > -* If the event has been added to the fh->subscribed list, but its
> > -* add op has not completed yet elems will be 0, treat this as
> > -* not being subscribed.
> > -*/
> > - if (!sev->elems)
> > - return;
> > -
> > /* Increase event sequence number on fh. */
> > fh->sequence++;
> >
> > @@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> > struct v4l2_subscribed_event *sev, *found_ev;
> > unsigned long flags;
> > unsigned i;
> > + int ret = 0;
> >
> > if (sub->type == V4L2_EVENT_ALL)
> > return -EINVAL;
> > @@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> > sev->flags = sub->flags;
> > sev->fh = fh;
> > sev->ops = ops;
> > + sev->elems = elems;
> > +
> > + mutex_lock(>mutex);
> >
> > spin_lock_irqsave(>vdev->fh_lock, flags);
> > found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> > - if (!found_ev)
> > - list_add(>list, >subscribed);
> > spin_unlock_irqrestore(>vdev->fh_lock, flags);
> >
> > if (found_ev) {
> > + /* Already listening */
> > kvfree(sev);
> > - return 0; /* Already listening */
> > + goto out_unlock;
> > }
> >
> > if (sev->ops && sev->ops->add) {
> > - int ret = sev->ops->add(sev, elems);
> > + ret = sev->ops->add(sev, elems);
> > if (ret) {
> > - sev->ops = NULL;
> > - v4l2_event_unsubscribe(fh, sub);
> > - return ret;
> > + kvfree(sev);
> > + goto out_unlock;
> > }
> > }
> >
> > /* Mark as ready for use */
> > - sev->elems = elems;
> > + list_add(>list, >subscribed);
> >
> > - return 0;
> > +out_unlock:
> > + mutex_unlock(>mutex);
> > +
> > + return ret;
> > }
> > EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> >
> > @@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> > return 0;
> > }
> >
> > + mutex_lock(>mutex);
> > +
> > spin_lock_irqsave(>vdev->fh_lock, flags);
> >
> > sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> > @@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> > if (sev && sev->ops && sev->ops->del)
> > sev->ops->del(sev);
> >
> > + mutex_unlock(>mutex);
> > +
> > kvfree(sev);
> >
> > return 0;
> > diff --git a/drivers/media/v4l2-core/v4l2-fh.c
> > b/drivers/media/v4l2-core/v4l2-fh.c
> > index 3895999bf880..b017dafde907 100644
> > --- a/drivers/media/v4l2-core/v4l2-fh.c
> > +++ b/drivers/media/v4l2-core/v4l2-fh.c
> > @@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device
> > *vdev)
> > INIT_LIST_HEAD(>available);
> >
Re: [PATCH v4] vb2: check for sane values from queue_setup
On Tue, Sep 11, 2018 at 04:46:04PM +0200, Johan Fjeldtvedt wrote:
> Warn and return error from the reqbufs ioctl when driver sets 0 number
> of planes or 0 as plane sizes, as these values don't make any sense.
> Checking this here stops obviously wrong values from propagating
> further and causing various problems that are hard to trace back to
> either of these values being 0.
>
> v4: check num_planes, not num_buffers
>
> Signed-off-by: Johan Fjeldtvedt
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index f32ec7342ef0..cf2f93462a54 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -662,6 +662,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory
> memory,
> unsigned int num_buffers, allocated_buffers, num_planes = 0;
> unsigned plane_sizes[VB2_MAX_PLANES] = { };
> int ret;
> + int i;
unsigned int i;
And arrange the line above ret declaration. With that,
Acked-by: Sakari Ailus
>
> if (q->streaming) {
> dprintk(1, "streaming active\n");
> @@ -718,6 +719,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
> vb2_memory memory,
> if (ret)
> return ret;
>
> + /* Check that driver has set sane values */
> + if (WARN_ON(!num_planes))
> + return -EINVAL;
> +
> + for (i = 0; i < num_planes; i++)
> + if (WARN_ON(!plane_sizes[i]))
> + return -EINVAL;
> +
> /* Finally, allocate buffers and video memory */
> allocated_buffers =
> __vb2_queue_alloc(q, memory, num_buffers, num_planes,
> plane_sizes);
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v3] vb2: check for sane values from queue_setup
Hi,
On 09/11/2018 03:46 PM, Johan Fjeldtvedt wrote:
> Warn and return error from the reqbufs ioctl when driver sets 0 number
> of planes or 0 as plane sizes, as these values don't make any sense.
typo: planes -> buffers
> Checking this here stops obviously wrong values from propagating
> further and causing various problems that are hard to trace back to
> either of these values being 0.
>
> Signed-off-by: Johan Fjeldtvedt
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index f32ec7342ef0..5741e95e6af1 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -662,6 +662,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory
> memory,
> unsigned int num_buffers, allocated_buffers, num_planes = 0;
> unsigned plane_sizes[VB2_MAX_PLANES] = { };
> int ret;
> + int i;
>
> if (q->streaming) {
> dprintk(1, "streaming active\n");
> @@ -718,6 +719,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
> vb2_memory memory,
> if (ret)
> return ret;
>
> + /* Check that driver has set sane values */
> + if (WARN_ON(!num_buffers))
> + return -EINVAL;
> +
> + for (i = 0; i < num_buffers; i++)
> + if (WARN_ON(!plane_sizes[i]))
> + return -EINVAL;
> +
> /* Finally, allocate buffers and video memory */
> allocated_buffers =
> __vb2_queue_alloc(q, memory, num_buffers, num_planes,
> plane_sizes);
>
--
regards,
Stan
Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed
On 09/12/18 10:52, Sakari Ailus wrote:
> The event subscriptions are added to the subscribed event list while
> holding a spinlock, but that lock is subsequently released while still
> accessing the subscription object. This makes it possible to unsubscribe
> the event --- and freeing the subscription object's memory --- while
> the subscription object is simultaneously accessed.
Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
so this could only be a scenario with drivers that do not use this
lock. Off-hand the only driver I know that does this is uvc. Unfortunately,
that's a rather popular one.
>
> Prevent this by adding a mutex to serialise the event subscription and
> unsubscription. This also gives a guarantee to the callback ops that the
> add op has returned before the del op is called.
>
> This change also results in making the elems field less special:
> subscriptions are only added to the event list once they are fully
> initialised.
>
> Signed-off-by: Sakari Ailus
> ---
> Hi folks,
>
> I noticed this while working to add support for media events. This seems
> like material for the stable trees.
I'd say 'no need for this' if it wasn't for uvc.
>
> drivers/media/v4l2-core/v4l2-event.c | 35 ++-
> drivers/media/v4l2-core/v4l2-fh.c| 2 ++
> include/media/v4l2-fh.h | 4
> 3 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> b/drivers/media/v4l2-core/v4l2-event.c
> index 127fe6eb91d9..74161f79e4d3 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> const struct v4l2_event *e
> if (sev == NULL)
> return;
>
> - /*
> - * If the event has been added to the fh->subscribed list, but its
> - * add op has not completed yet elems will be 0, treat this as
> - * not being subscribed.
> - */
> - if (!sev->elems)
> - return;
> -
> /* Increase event sequence number on fh. */
> fh->sequence++;
>
> @@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> struct v4l2_subscribed_event *sev, *found_ev;
> unsigned long flags;
> unsigned i;
> + int ret = 0;
>
> if (sub->type == V4L2_EVENT_ALL)
> return -EINVAL;
> @@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> sev->flags = sub->flags;
> sev->fh = fh;
> sev->ops = ops;
> + sev->elems = elems;
> +
> + mutex_lock(>mutex);
>
> spin_lock_irqsave(>vdev->fh_lock, flags);
> found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> - if (!found_ev)
> - list_add(>list, >subscribed);
> spin_unlock_irqrestore(>vdev->fh_lock, flags);
>
> if (found_ev) {
> + /* Already listening */
> kvfree(sev);
> - return 0; /* Already listening */
> + goto out_unlock;
> }
>
> if (sev->ops && sev->ops->add) {
> - int ret = sev->ops->add(sev, elems);
> + ret = sev->ops->add(sev, elems);
> if (ret) {
> - sev->ops = NULL;
> - v4l2_event_unsubscribe(fh, sub);
> - return ret;
> + kvfree(sev);
> + goto out_unlock;
> }
> }
>
> /* Mark as ready for use */
> - sev->elems = elems;
> + list_add(>list, >subscribed);
>
> - return 0;
> +out_unlock:
> + mutex_unlock(>mutex);
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
>
> @@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> return 0;
> }
>
> + mutex_lock(>mutex);
> +
> spin_lock_irqsave(>vdev->fh_lock, flags);
>
> sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> @@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> if (sev && sev->ops && sev->ops->del)
> sev->ops->del(sev);
>
> + mutex_unlock(>mutex);
> +
> kvfree(sev);
>
> return 0;
> diff --git a/drivers/media/v4l2-core/v4l2-fh.c
> b/drivers/media/v4l2-core/v4l2-fh.c
> index 3895999bf880..b017dafde907 100644
> --- a/drivers/media/v4l2-core/v4l2-fh.c
> +++ b/drivers/media/v4l2-core/v4l2-fh.c
> @@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device
> *vdev)
> INIT_LIST_HEAD(>available);
> INIT_LIST_HEAD(>subscribed);
> fh->sequence = -1;
> + mutex_init(>mutex);
> }
> EXPORT_SYMBOL_GPL(v4l2_fh_init);
>
> @@ -90,6 +91,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
> return;
> v4l_disable_media_source(fh->vdev);
> v4l2_event_unsubscribe_all(fh);
> + mutex_destroy(>mutex);
> fh->vdev = NULL;
> }
> EXPORT_SYMBOL_GPL(v4l2_fh_exit);
> diff --git
[PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed
The event subscriptions are added to the subscribed event list while
holding a spinlock, but that lock is subsequently released while still
accessing the subscription object. This makes it possible to unsubscribe
the event --- and freeing the subscription object's memory --- while
the subscription object is simultaneously accessed.
Prevent this by adding a mutex to serialise the event subscription and
unsubscription. This also gives a guarantee to the callback ops that the
add op has returned before the del op is called.
This change also results in making the elems field less special:
subscriptions are only added to the event list once they are fully
initialised.
Signed-off-by: Sakari Ailus
---
Hi folks,
I noticed this while working to add support for media events. This seems
like material for the stable trees.
drivers/media/v4l2-core/v4l2-event.c | 35 ++-
drivers/media/v4l2-core/v4l2-fh.c| 2 ++
include/media/v4l2-fh.h | 4
3 files changed, 24 insertions(+), 17 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index 127fe6eb91d9..74161f79e4d3 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
const struct v4l2_event *e
if (sev == NULL)
return;
- /*
-* If the event has been added to the fh->subscribed list, but its
-* add op has not completed yet elems will be 0, treat this as
-* not being subscribed.
-*/
- if (!sev->elems)
- return;
-
/* Increase event sequence number on fh. */
fh->sequence++;
@@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
struct v4l2_subscribed_event *sev, *found_ev;
unsigned long flags;
unsigned i;
+ int ret = 0;
if (sub->type == V4L2_EVENT_ALL)
return -EINVAL;
@@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
sev->flags = sub->flags;
sev->fh = fh;
sev->ops = ops;
+ sev->elems = elems;
+
+ mutex_lock(>mutex);
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (!found_ev)
- list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
+ /* Already listening */
kvfree(sev);
- return 0; /* Already listening */
+ goto out_unlock;
}
if (sev->ops && sev->ops->add) {
- int ret = sev->ops->add(sev, elems);
+ ret = sev->ops->add(sev, elems);
if (ret) {
- sev->ops = NULL;
- v4l2_event_unsubscribe(fh, sub);
- return ret;
+ kvfree(sev);
+ goto out_unlock;
}
}
/* Mark as ready for use */
- sev->elems = elems;
+ list_add(>list, >subscribed);
- return 0;
+out_unlock:
+ mutex_unlock(>mutex);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
@@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
return 0;
}
+ mutex_lock(>mutex);
+
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
@@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
if (sev && sev->ops && sev->ops->del)
sev->ops->del(sev);
+ mutex_unlock(>mutex);
+
kvfree(sev);
return 0;
diff --git a/drivers/media/v4l2-core/v4l2-fh.c
b/drivers/media/v4l2-core/v4l2-fh.c
index 3895999bf880..b017dafde907 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device
*vdev)
INIT_LIST_HEAD(>available);
INIT_LIST_HEAD(>subscribed);
fh->sequence = -1;
+ mutex_init(>mutex);
}
EXPORT_SYMBOL_GPL(v4l2_fh_init);
@@ -90,6 +91,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
return;
v4l_disable_media_source(fh->vdev);
v4l2_event_unsubscribe_all(fh);
+ mutex_destroy(>mutex);
fh->vdev = NULL;
}
EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index ea73fef8bdc0..1be45a5f6383 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -42,6 +42,9 @@ struct v4l2_ctrl_handler;
* @available: list of events waiting to be dequeued
* @navailable: number of available events at @available list
* @sequence: event sequence number
+ * @mutex: hold event subscriptions during subscribing;
+ *guarantee that the add and del event callbacks are orderly called
+ *
* @m2m_ctx:
Re: [PATCH 3/3] media: replace strncpy() by strscpy()
On 09/10/2018 02:19 PM, Mauro Carvalho Chehab wrote:
> The strncpy() function is being deprecated upstream. Replace
> it by the safer strscpy().
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/dvb-frontends/as102_fe.c | 2 +-
> drivers/media/dvb-frontends/dib7000p.c | 3 ++-
> drivers/media/dvb-frontends/dib8000.c| 4 ++--
> drivers/media/dvb-frontends/dib9000.c| 6 --
> drivers/media/dvb-frontends/dvb-pll.c| 2 +-
> drivers/media/dvb-frontends/m88ds3103.c | 2 +-
> drivers/media/pci/bt8xx/dst.c| 3 ++-
> drivers/media/pci/mantis/mantis_i2c.c| 2 +-
> drivers/media/pci/saa7134/saa7134-go7007.c | 2 +-
> drivers/media/platform/am437x/am437x-vpfe.c | 2 +-
> drivers/media/platform/davinci/vpfe_capture.c| 2 +-
> drivers/media/platform/davinci/vpif_capture.c| 2 +-
> drivers/media/platform/davinci/vpif_display.c| 3 +--
> drivers/media/platform/exynos4-is/fimc-capture.c | 2 +-
> drivers/media/platform/exynos4-is/fimc-m2m.c | 2 +-
> drivers/media/platform/mtk-vpu/mtk_vpu.c | 2 +-
> drivers/media/platform/mx2_emmaprp.c | 4 ++--
> drivers/media/platform/s5p-g2d/g2d.c | 6 +++---
> drivers/media/platform/ti-vpe/vpe.c | 6 +++---
> drivers/media/platform/vicodec/vicodec-core.c| 4 ++--
> drivers/media/platform/vim2m.c | 4 ++--
> drivers/media/radio/si4713/si4713.c | 2 +-
> drivers/media/usb/go7007/go7007-usb.c| 16
> drivers/media/usb/go7007/go7007-v4l2.c | 2 +-
> drivers/media/usb/hdpvr/hdpvr-video.c| 9 +++--
> drivers/media/usb/pulse8-cec/pulse8-cec.c| 4 ++--
> drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 2 +-
> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 4 ++--
> drivers/staging/media/bcm2048/radio-bcm2048.c| 4 ++--
> drivers/staging/media/imx/imx-ic-common.c| 2 +-
> drivers/staging/media/imx/imx-media-vdic.c | 2 +-
> drivers/staging/media/zoran/zoran_driver.c | 10 +-
> 32 files changed, 61 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/media/platform/davinci/vpfe_capture.c
> b/drivers/media/platform/davinci/vpfe_capture.c
> index ea3ddd5a42bd..6f094dea6bc2 100644
> --- a/drivers/media/platform/davinci/vpfe_capture.c
> +++ b/drivers/media/platform/davinci/vpfe_capture.c
> @@ -1759,7 +1759,7 @@ static int vpfe_probe(struct platform_device *pdev)
>
> mutex_lock(_lock);
>
> - strncpy(ccdc_cfg->name, vpfe_cfg->ccdc, 32);
> + strscpy(ccdc_cfg->name, vpfe_cfg->ccdc, 32);
32 -> sizeof(ccdc_cfg->name)
> /* Get VINT0 irq resource */
> res1 = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> if (!res1) {
> diff --git a/drivers/media/platform/davinci/vpif_capture.c
> b/drivers/media/platform/davinci/vpif_capture.c
> index 62bced38db10..b246af6cc21f 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1254,7 +1254,7 @@ static int vpif_s_dv_timings(struct file *file, void
> *priv,
> } else {
> std_info->l5 = std_info->vsize - (bt->vfrontporch - 1);
> }
> - strncpy(std_info->name, "Custom timings BT656/1120", VPIF_MAX_NAME);
> + strscpy(std_info->name, "Custom timings BT656/1120", VPIF_MAX_NAME);
VPIF_MAX_NAME -> sizeof(std_info->name)
> std_info->width = bt->width;
> std_info->height = bt->height;
> std_info->frm_fmt = bt->interlaced ? 0 : 1;
> diff --git a/drivers/media/platform/davinci/vpif_display.c
> b/drivers/media/platform/davinci/vpif_display.c
> index 78eba66f4b2b..65f51ebef6b4 100644
> --- a/drivers/media/platform/davinci/vpif_display.c
> +++ b/drivers/media/platform/davinci/vpif_display.c
> @@ -987,8 +987,7 @@ static int vpif_s_dv_timings(struct file *file, void
> *priv,
> } else {
> std_info->l5 = std_info->vsize - (bt->vfrontporch - 1);
> }
> - strncpy(std_info->name, "Custom timings BT656/1120",
> - VPIF_MAX_NAME);
> + strscpy(std_info->name, "Custom timings BT656/1120", VPIF_MAX_NAME);
Ditto.
> std_info->width = bt->width;
> std_info->height = bt->height;
> std_info->frm_fmt = bt->interlaced ? 0 : 1;
> diff --git a/drivers/media/platform/mtk-vpu/mtk_vpu.c
> b/drivers/media/platform/mtk-vpu/mtk_vpu.c
> index f8d35e3ac1dc..9f4bca037b59 100644
> --- a/drivers/media/platform/mtk-vpu/mtk_vpu.c
> +++ b/drivers/media/platform/mtk-vpu/mtk_vpu.c
> @@ -615,7 +615,7 @@ static void vpu_init_ipi_handler(void *data, unsigned int
> len, void *priv)
> struct vpu_run *run = (struct vpu_run *)data;
>
> vpu->run.signaled = run->signaled;
> - strncpy(vpu->run.fw_ver, run->fw_ver, VPU_FW_VER_LEN);
> + strscpy(vpu->run.fw_ver, run->fw_ver, VPU_FW_VER_LEN);
VPU_FW_VER_LEN ->
Re: [PATCH v2 0/2] [media] Depth confidence pixel-format for Intel RealSense cameras
On 09/12/2018 08:42 AM, dorod...@gmail.com wrote: > From: Sergey Dorodnicov > > Define new fourcc describing depth sensor confidence data used in Intel > RealSense cameras. > Confidence information is stored as packed 4 bits per pixel single-plane > image. > The patches were tested on 4.18-rc2 and merged with media_tree/master. > Addressing code-review comments by Hans Verkuil and > Laurent Pinchart . > > Sergey Dorodnicov (2): > CNF4 fourcc for 4 bit-per-pixel packed depth confidence information > CNF4 pixel format for media subsystem > > Documentation/media/uapi/v4l/depth-formats.rst | 1 + > Documentation/media/uapi/v4l/pixfmt-cnf4.rst | 31 > ++ > drivers/media/usb/uvc/uvc_driver.c | 5 + > drivers/media/usb/uvc/uvcvideo.h | 3 +++ > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > include/uapi/linux/videodev2.h | 1 + > 6 files changed, 42 insertions(+) > create mode 100644 Documentation/media/uapi/v4l/pixfmt-cnf4.rst > Laurent, this looks good to me. Do you want to take this series or shall I? If you take it, then you can add my: Acked-by: Hans Verkuil to these patches. If you want me to take it, then I'll need your Ack of course. Regards, Hans
[PATCH v2 2/2] [media] CNF4 pixel format for media subsystem
From: Sergey Dorodnicov
Registering new GUID used by Intel RealSense cameras with fourcc CNF4,
encoding depth sensor confidence information for every pixel.
Signed-off-by: Sergey Dorodnicov
Signed-off-by: Evgeni Raikhel
---
drivers/media/usb/uvc/uvc_driver.c | 5 +
drivers/media/usb/uvc/uvcvideo.h | 3 +++
2 files changed, 8 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_driver.c
b/drivers/media/usb/uvc/uvc_driver.c
index d46dc43..19f129f 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -214,6 +214,11 @@ static struct uvc_format_desc uvc_fmts[] = {
.guid = UVC_GUID_FORMAT_INZI,
.fcc= V4L2_PIX_FMT_INZI,
},
+ {
+ .name = "4-bit Depth Confidence (Packed)",
+ .guid = UVC_GUID_FORMAT_CNF4,
+ .fcc= V4L2_PIX_FMT_CNF4,
+ },
};
/*
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index e5f5d84..779bab2 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -154,6 +154,9 @@
#define UVC_GUID_FORMAT_INVI \
{ 'I', 'N', 'V', 'I', 0xdb, 0x57, 0x49, 0x5e, \
0x8e, 0x3f, 0xf4, 0x79, 0x53, 0x2b, 0x94, 0x6f}
+#define UVC_GUID_FORMAT_CNF4 \
+ { 'C', ' ', ' ', ' ', 0x00, 0x00, 0x10, 0x00, \
+0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71}
#define UVC_GUID_FORMAT_D3DFMT_L8 \
{0x32, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, \
--
2.7.4
[PATCH v2 0/2] [media] Depth confidence pixel-format for Intel RealSense cameras
From: Sergey Dorodnicov Define new fourcc describing depth sensor confidence data used in Intel RealSense cameras. Confidence information is stored as packed 4 bits per pixel single-plane image. The patches were tested on 4.18-rc2 and merged with media_tree/master. Addressing code-review comments by Hans Verkuil and Laurent Pinchart . Sergey Dorodnicov (2): CNF4 fourcc for 4 bit-per-pixel packed depth confidence information CNF4 pixel format for media subsystem Documentation/media/uapi/v4l/depth-formats.rst | 1 + Documentation/media/uapi/v4l/pixfmt-cnf4.rst | 31 ++ drivers/media/usb/uvc/uvc_driver.c | 5 + drivers/media/usb/uvc/uvcvideo.h | 3 +++ drivers/media/v4l2-core/v4l2-ioctl.c | 1 + include/uapi/linux/videodev2.h | 1 + 6 files changed, 42 insertions(+) create mode 100644 Documentation/media/uapi/v4l/pixfmt-cnf4.rst -- 2.7.4
[PATCH v2 1/2] [media] CNF4 fourcc for 4 bit-per-pixel packed depth confidence information
From: Sergey Dorodnicov
Adding new fourcc CNF4 for 4 bit-per-pixel packed depth confidence
information provided by Intel RealSense cameras. Every two consecutive
pixels are packed into a single byte.
Signed-off-by: Sergey Dorodnicov
Signed-off-by: Evgeni Raikhel
---
Documentation/media/uapi/v4l/depth-formats.rst | 1 +
Documentation/media/uapi/v4l/pixfmt-cnf4.rst | 31 ++
drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
include/uapi/linux/videodev2.h | 1 +
4 files changed, 34 insertions(+)
create mode 100644 Documentation/media/uapi/v4l/pixfmt-cnf4.rst
diff --git a/Documentation/media/uapi/v4l/depth-formats.rst
b/Documentation/media/uapi/v4l/depth-formats.rst
index d1641e9..9533348 100644
--- a/Documentation/media/uapi/v4l/depth-formats.rst
+++ b/Documentation/media/uapi/v4l/depth-formats.rst
@@ -14,3 +14,4 @@ Depth data provides distance to points, mapped onto the image
plane
pixfmt-inzi
pixfmt-z16
+pixfmt-cnf4
diff --git a/Documentation/media/uapi/v4l/pixfmt-cnf4.rst
b/Documentation/media/uapi/v4l/pixfmt-cnf4.rst
new file mode 100644
index 000..8f46929
--- /dev/null
+++ b/Documentation/media/uapi/v4l/pixfmt-cnf4.rst
@@ -0,0 +1,31 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _V4L2-PIX-FMT-CNF4:
+
+**
+V4L2_PIX_FMT_CNF4 ('CNF4')
+**
+
+Depth sensor confidence information as a 4 bits per pixel packed array
+
+Description
+===
+
+Proprietary format used by Intel RealSense Depth cameras containing depth
+confidence information in range 0-15 with 0 indicating that the sensor was
+unable to resolve any signal and 15 indicating maximum level of confidence for
+the specific sensor (actual error margins might change from sensor to sensor).
+
+Every two consecutive pixels are packed into a single byte.
+Bits 0-3 of byte n refer to confidence value of depth pixel 2*n,
+bits 4-7 to confidence value of depth pixel 2*n+1.
+
+**Bit-packed representation.**
+
+.. flat-table::
+:header-rows: 0
+:stub-columns: 0
+:widths: 64 64
+
+* - Y'\ :sub:`01[3:0]`\ (bits 7--4) Y'\ :sub:`00[3:0]`\ (bits 3--0)
+ - Y'\ :sub:`03[3:0]`\ (bits 7--4) Y'\ :sub:`02[3:0]`\ (bits 3--0)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 54afc9c..f9aa8bd 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1189,6 +1189,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
case V4L2_PIX_FMT_Y12I: descr = "Interleaved 12-bit Greyscale";
break;
case V4L2_PIX_FMT_Z16: descr = "16-bit Depth"; break;
case V4L2_PIX_FMT_INZI: descr = "Planar 10:16 Greyscale Depth";
break;
+ case V4L2_PIX_FMT_CNF4: descr = "4-bit Depth Confidence
(Packed)"; break;
case V4L2_PIX_FMT_PAL8: descr = "8-bit Palette"; break;
case V4L2_PIX_FMT_UV8: descr = "8-bit Chrominance UV 4-4";
break;
case V4L2_PIX_FMT_YVU410: descr = "Planar YVU 4:1:0"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 622f047..2837c93 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -676,6 +676,7 @@ struct v4l2_pix_format {
#define V4L2_PIX_FMT_Z16 v4l2_fourcc('Z', '1', '6', ' ') /* Depth data
16-bit */
#define V4L2_PIX_FMT_MT21Cv4l2_fourcc('M', 'T', '2', '1') /* Mediatek
compressed block mode */
#define V4L2_PIX_FMT_INZI v4l2_fourcc('I', 'N', 'Z', 'I') /* Intel Planar
Greyscale 10-bit and Depth 16-bit */
+#define V4L2_PIX_FMT_CNF4 v4l2_fourcc('C', 'N', 'F', '4') /* Intel 4-bit
packed depth confidence information */
/* 10bit raw bayer packed, 32 bytes for every 25 pixels, last LSB 6 bits
unused */
#define V4L2_PIX_FMT_IPU3_SBGGR10 v4l2_fourcc('i', 'p', '3', 'b') /* IPU3
packed 10-bit BGGR bayer */
--
2.7.4
