Re: [PATCH 2/9] ARM: lager: add i2c1, i2c2 pins
On 16/06/14 13:33, Sergei Shtylyov wrote: Hello. On 06/15/2014 11:56 PM, Ben Dooks wrote: Add pinctrl definitions for i2c1 and i2c2 busses on the Lager board to ensure these are setup correctly at initialisation time. The i2c0 and i2c3 busses are connected to single function pins. Signed-off-by: Ben Dooks ben.do...@codethink.co.uk Likewise, this as been already merged by Simon. Ah, they had not been merged when I took the branch for this around -rc8 time. I will look at changing the necessary bits for the vin in the DT and re-sub them as a new series for Simon to look at merging. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius -- 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 2/9] ARM: lager: add i2c1, i2c2 pins
On Wed, Jun 18, 2014 at 08:27:37AM +0100, Ben Dooks wrote: On 16/06/14 13:33, Sergei Shtylyov wrote: Hello. On 06/15/2014 11:56 PM, Ben Dooks wrote: Add pinctrl definitions for i2c1 and i2c2 busses on the Lager board to ensure these are setup correctly at initialisation time. The i2c0 and i2c3 busses are connected to single function pins. Signed-off-by: Ben Dooks ben.do...@codethink.co.uk Likewise, this as been already merged by Simon. Ah, they had not been merged when I took the branch for this around -rc8 time. I will look at changing the necessary bits for the vin in the DT and re-sub them as a new series for Simon to look at merging. Thanks, Ben. -- 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: [alsa-devel] [PATCH 1/3] sound: Add a quirk to enforce period_bytes
Mauro Carvalho Chehab wrote: Let's see the au0828 case: 48 kHz, 2 bytes/sample, 2 channels, 256 maxpacksize, 1 ms URB interval (bInterval = 1). In this case, there is 192 bytes per 1ms period. The device's clock and the bus clock are not synchronized, so there will be _approximately_ 192 bytes per USB frame. Let's assume that the period was set to 3456, with corresponds to a latency of 18 ms. In this case, as NUM_URBS = 12, There is no symbol named NUM_URBS. it means that the transfer buffer will be set to its maximum value of 3072 bytes per URB pack (12 * 256) The number of URBs is not the same as the number of packets per URB. and the URB transfer_callback will be called on every 16 ms. It will be called once per millisecond. So, what ... definitely not ... happens is: - after 16 ms, the first 3072 bytes arrive. The next packet will take another 16ms to arrive; - after 2 ms, underrun, as the period_size was not filled yet. Regards, Clemens -- 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: [alsa-devel] [PATCH 1/3] sound: Add a quirk to enforce period_bytes
Mauro Carvalho Chehab wrote: Both xawtv and tvtime use the same code for audio: http://git.linuxtv.org/cgit.cgi/xawtv3.git/tree/common/alsa_stream.c There's an algorithm there that gets the period size form both the capture and the playback cards, trying to find a minimum period that would work properly for both. Why are you trying to match period sizes? The sample clocks won't be synchronized anyway, so it is not possible to force them to happen at the same time. Please note that for playback devices, the latency is the same as the buffer length, while for capture device, the latency is the same as the _period_ length. Therefore, it does not make sense to put an upper limit on the size of the capture buffer. I do not think it is a good idea to stop the capture device when it overruns. Regards, Clemens -- 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
cron job: media_tree daily build: OK
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: Wed Jun 18 10:38:55 CEST 2014 git branch: test git hash: 2ac6f6305efef8c10994be48b20723cc70654189 gcc version:i686-linux-gcc (GCC) 4.8.2 sparse version: v0.5.0-14-gf11dd94 host hardware: x86_64 host os:3.14-5.slh.5-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-exynos: OK linux-git-arm-mx: OK linux-git-arm-omap: OK linux-git-arm-omap1: OK linux-git-arm-pxa: OK linux-git-blackfin: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.31.14-i686: OK linux-2.6.32.27-i686: OK linux-2.6.33.7-i686: OK linux-2.6.34.7-i686: OK linux-2.6.35.9-i686: OK linux-2.6.36.4-i686: OK linux-2.6.37.6-i686: OK linux-2.6.38.8-i686: OK linux-2.6.39.4-i686: OK linux-3.0.60-i686: OK linux-3.1.10-i686: OK linux-3.2.37-i686: OK linux-3.3.8-i686: OK linux-3.4.27-i686: OK linux-3.5.7-i686: OK linux-3.6.11-i686: OK linux-3.7.4-i686: OK linux-3.8-i686: OK linux-3.9.2-i686: OK linux-3.10.1-i686: OK linux-3.11.1-i686: OK linux-3.12-i686: OK linux-3.13-i686: OK linux-3.14-i686: OK linux-3.15-i686: OK linux-3.16-rc1-i686: OK linux-2.6.31.14-x86_64: OK linux-2.6.32.27-x86_64: OK linux-2.6.33.7-x86_64: OK linux-2.6.34.7-x86_64: OK linux-2.6.35.9-x86_64: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-x86_64: OK linux-3.0.60-x86_64: OK linux-3.1.10-x86_64: OK linux-3.2.37-x86_64: OK linux-3.3.8-x86_64: OK linux-3.4.27-x86_64: OK linux-3.5.7-x86_64: OK linux-3.6.11-x86_64: OK linux-3.7.4-x86_64: OK linux-3.8-x86_64: OK linux-3.9.2-x86_64: OK linux-3.10.1-x86_64: OK linux-3.11.1-x86_64: OK linux-3.12-x86_64: OK linux-3.13-x86_64: OK linux-3.14-x86_64: OK linux-3.15-x86_64: OK linux-3.16-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- 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
[REPOST PATCH 2/8] seqno-fence: Hardware dma-buf implementation of fencing (v5)
This type of fence can be used with hardware synchronization for simple
hardware that can block execution until the condition
(dma_buf[offset] - value) = 0 has been met when WAIT_GEQUAL is used,
or (dma_buf[offset] != 0) has been met when WAIT_NONZERO is set.
A software fallback still has to be provided in case the fence is used
with a device that doesn't support this mechanism. It is useful to expose
this for graphics cards that have an op to support this.
Some cards like i915 can export those, but don't have an option to wait,
so they need the software fallback.
I extended the original patch by Rob Clark.
v1: Original
v2: Renamed from bikeshed to seqno, moved into dma-fence.c since
not much was left of the file. Lots of documentation added.
v3: Use fence_ops instead of custom callbacks. Moved to own file
to avoid circular dependency between dma-buf.h and fence.h
v4: Add spinlock pointer to seqno_fence_init
v5: Add condition member to allow wait for != 0.
Fix small style errors pointed out by checkpatch.
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
Reviewed-by: Rob Clark robdcl...@gmail.com #v4
---
Documentation/DocBook/device-drivers.tmpl |1
drivers/base/fence.c | 52 +
include/linux/seqno-fence.h | 119 +
3 files changed, 172 insertions(+)
create mode 100644 include/linux/seqno-fence.h
diff --git a/Documentation/DocBook/device-drivers.tmpl
b/Documentation/DocBook/device-drivers.tmpl
index 7eef81069d1b..6ca7a11fb893 100644
--- a/Documentation/DocBook/device-drivers.tmpl
+++ b/Documentation/DocBook/device-drivers.tmpl
@@ -131,6 +131,7 @@ X!Edrivers/base/interface.c
!Edrivers/base/dma-buf.c
!Edrivers/base/fence.c
!Iinclude/linux/fence.h
+!Iinclude/linux/seqno-fence.h
!Edrivers/base/reservation.c
!Iinclude/linux/reservation.h
!Edrivers/base/dma-coherent.c
diff --git a/drivers/base/fence.c b/drivers/base/fence.c
index 1da7f4d6542a..752a2dfa505f 100644
--- a/drivers/base/fence.c
+++ b/drivers/base/fence.c
@@ -25,6 +25,7 @@
#include linux/export.h
#include linux/atomic.h
#include linux/fence.h
+#include linux/seqno-fence.h
#define CREATE_TRACE_POINTS
#include trace/events/fence.h
@@ -414,3 +415,54 @@ __fence_init(struct fence *fence, const struct fence_ops
*ops,
trace_fence_init(fence);
}
EXPORT_SYMBOL(__fence_init);
+
+static const char *seqno_fence_get_driver_name(struct fence *fence)
+{
+ struct seqno_fence *seqno_fence = to_seqno_fence(fence);
+ return seqno_fence-ops-get_driver_name(fence);
+}
+
+static const char *seqno_fence_get_timeline_name(struct fence *fence)
+{
+ struct seqno_fence *seqno_fence = to_seqno_fence(fence);
+ return seqno_fence-ops-get_timeline_name(fence);
+}
+
+static bool seqno_enable_signaling(struct fence *fence)
+{
+ struct seqno_fence *seqno_fence = to_seqno_fence(fence);
+ return seqno_fence-ops-enable_signaling(fence);
+}
+
+static bool seqno_signaled(struct fence *fence)
+{
+ struct seqno_fence *seqno_fence = to_seqno_fence(fence);
+ return seqno_fence-ops-signaled seqno_fence-ops-signaled(fence);
+}
+
+static void seqno_release(struct fence *fence)
+{
+ struct seqno_fence *f = to_seqno_fence(fence);
+
+ dma_buf_put(f-sync_buf);
+ if (f-ops-release)
+ f-ops-release(fence);
+ else
+ kfree(f);
+}
+
+static long seqno_wait(struct fence *fence, bool intr, signed long timeout)
+{
+ struct seqno_fence *f = to_seqno_fence(fence);
+ return f-ops-wait(fence, intr, timeout);
+}
+
+const struct fence_ops seqno_fence_ops = {
+ .get_driver_name = seqno_fence_get_driver_name,
+ .get_timeline_name = seqno_fence_get_timeline_name,
+ .enable_signaling = seqno_enable_signaling,
+ .signaled = seqno_signaled,
+ .wait = seqno_wait,
+ .release = seqno_release,
+};
+EXPORT_SYMBOL(seqno_fence_ops);
diff --git a/include/linux/seqno-fence.h b/include/linux/seqno-fence.h
new file mode 100644
index ..b4d4aad3cadc
--- /dev/null
+++ b/include/linux/seqno-fence.h
@@ -0,0 +1,119 @@
+/*
+ * seqno-fence, using a dma-buf to synchronize fencing
+ *
+ * Copyright (C) 2012 Texas Instruments
+ * Copyright (C) 2012 Canonical Ltd
+ * Authors:
+ * Rob Clark robdcl...@gmail.com
+ * Maarten Lankhorst maarten.lankho...@canonical.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see
[REPOST PATCH 6/8] dma-buf: add poll support, v3
Thanks to Fengguang Wu for spotting a missing static cast.
v2:
- Kill unused variable need_shared.
v3:
- Clarify the BUG() in dma_buf_release some more. (Rob Clark)
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
---
drivers/base/dma-buf.c | 108 +++
include/linux/dma-buf.h | 12 +
2 files changed, 120 insertions(+)
diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
index cd40ca22911f..25e8c4165936 100644
--- a/drivers/base/dma-buf.c
+++ b/drivers/base/dma-buf.c
@@ -30,6 +30,7 @@
#include linux/export.h
#include linux/debugfs.h
#include linux/seq_file.h
+#include linux/poll.h
#include linux/reservation.h
static inline int is_dma_buf_file(struct file *);
@@ -52,6 +53,16 @@ static int dma_buf_release(struct inode *inode, struct file
*file)
BUG_ON(dmabuf-vmapping_counter);
+ /*
+* Any fences that a dma-buf poll can wait on should be signaled
+* before releasing dma-buf. This is the responsibility of each
+* driver that uses the reservation objects.
+*
+* If you hit this BUG() it means someone dropped their ref to the
+* dma-buf while still having pending operation to the buffer.
+*/
+ BUG_ON(dmabuf-cb_shared.active || dmabuf-cb_excl.active);
+
dmabuf-ops-release(dmabuf);
mutex_lock(db_list.lock);
@@ -108,10 +119,103 @@ static loff_t dma_buf_llseek(struct file *file, loff_t
offset, int whence)
return base + offset;
}
+static void dma_buf_poll_cb(struct fence *fence, struct fence_cb *cb)
+{
+ struct dma_buf_poll_cb_t *dcb = (struct dma_buf_poll_cb_t *)cb;
+ unsigned long flags;
+
+ spin_lock_irqsave(dcb-poll-lock, flags);
+ wake_up_locked_poll(dcb-poll, dcb-active);
+ dcb-active = 0;
+ spin_unlock_irqrestore(dcb-poll-lock, flags);
+}
+
+static unsigned int dma_buf_poll(struct file *file, poll_table *poll)
+{
+ struct dma_buf *dmabuf;
+ struct reservation_object *resv;
+ unsigned long events;
+
+ dmabuf = file-private_data;
+ if (!dmabuf || !dmabuf-resv)
+ return POLLERR;
+
+ resv = dmabuf-resv;
+
+ poll_wait(file, dmabuf-poll, poll);
+
+ events = poll_requested_events(poll) (POLLIN | POLLOUT);
+ if (!events)
+ return 0;
+
+ ww_mutex_lock(resv-lock, NULL);
+
+ if (resv-fence_excl (!(events POLLOUT) ||
+resv-fence_shared_count == 0)) {
+ struct dma_buf_poll_cb_t *dcb = dmabuf-cb_excl;
+ unsigned long pevents = POLLIN;
+
+ if (resv-fence_shared_count == 0)
+ pevents |= POLLOUT;
+
+ spin_lock_irq(dmabuf-poll.lock);
+ if (dcb-active) {
+ dcb-active |= pevents;
+ events = ~pevents;
+ } else
+ dcb-active = pevents;
+ spin_unlock_irq(dmabuf-poll.lock);
+
+ if (events pevents) {
+ if (!fence_add_callback(resv-fence_excl,
+ dcb-cb, dma_buf_poll_cb))
+ events = ~pevents;
+ else
+ /*
+* No callback queued, wake up any additional
+* waiters.
+*/
+ dma_buf_poll_cb(NULL, dcb-cb);
+ }
+ }
+
+ if ((events POLLOUT) resv-fence_shared_count 0) {
+ struct dma_buf_poll_cb_t *dcb = dmabuf-cb_shared;
+ int i;
+
+ /* Only queue a new callback if no event has fired yet */
+ spin_lock_irq(dmabuf-poll.lock);
+ if (dcb-active)
+ events = ~POLLOUT;
+ else
+ dcb-active = POLLOUT;
+ spin_unlock_irq(dmabuf-poll.lock);
+
+ if (!(events POLLOUT))
+ goto out;
+
+ for (i = 0; i resv-fence_shared_count; ++i)
+ if (!fence_add_callback(resv-fence_shared[i],
+ dcb-cb, dma_buf_poll_cb)) {
+ events = ~POLLOUT;
+ break;
+ }
+
+ /* No callback queued, wake up any additional waiters. */
+ if (i == resv-fence_shared_count)
+ dma_buf_poll_cb(NULL, dcb-cb);
+ }
+
+out:
+ ww_mutex_unlock(resv-lock);
+ return events;
+}
+
static const struct file_operations dma_buf_fops = {
.release= dma_buf_release,
.mmap = dma_buf_mmap_internal,
.llseek = dma_buf_llseek,
+ .poll = dma_buf_poll,
};
/*
@@ -171,6 +275,10 @@ struct dma_buf
[REPOST PATCH 4/8] android: convert sync to fence api, v5
Just to show it's easy.
Android syncpoints can be mapped to a timeline. This removes the need
to maintain a separate api for synchronization. I've left the android
trace events in place, but the core fence events should already be
sufficient for debugging.
v2:
- Call fence_remove_callback in sync_fence_free if not all fences have fired.
v3:
- Merge Colin Cross' bugfixes, and the android fence merge optimization.
v4:
- Merge with the upstream fixes.
v5:
- Fix small style issues pointed out by Thomas Hellstrom.
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
Acked-by: John Stultz john.stu...@linaro.org
---
drivers/staging/android/Kconfig |1
drivers/staging/android/Makefile |2
drivers/staging/android/sw_sync.c|6
drivers/staging/android/sync.c | 913 +++---
drivers/staging/android/sync.h | 79 ++-
drivers/staging/android/sync_debug.c | 247 +
drivers/staging/android/trace/sync.h | 12
7 files changed, 609 insertions(+), 651 deletions(-)
create mode 100644 drivers/staging/android/sync_debug.c
diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
index 99e484f845f2..51607e9aa049 100644
--- a/drivers/staging/android/Kconfig
+++ b/drivers/staging/android/Kconfig
@@ -88,6 +88,7 @@ config SYNC
bool Synchronization framework
default n
select ANON_INODES
+ select DMA_SHARED_BUFFER
---help---
This option enables the framework for synchronization between multiple
drivers. Sync implementations can take advantage of hardware
diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index 0a01e1914905..517ad5ffa429 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -9,5 +9,5 @@ obj-$(CONFIG_ANDROID_TIMED_OUTPUT) += timed_output.o
obj-$(CONFIG_ANDROID_TIMED_GPIO) += timed_gpio.o
obj-$(CONFIG_ANDROID_LOW_MEMORY_KILLER)+= lowmemorykiller.o
obj-$(CONFIG_ANDROID_INTF_ALARM_DEV) += alarm-dev.o
-obj-$(CONFIG_SYNC) += sync.o
+obj-$(CONFIG_SYNC) += sync.o sync_debug.o
obj-$(CONFIG_SW_SYNC) += sw_sync.o
diff --git a/drivers/staging/android/sw_sync.c
b/drivers/staging/android/sw_sync.c
index 12a136ec1cec..a76db3ff87cb 100644
--- a/drivers/staging/android/sw_sync.c
+++ b/drivers/staging/android/sw_sync.c
@@ -50,7 +50,7 @@ static struct sync_pt *sw_sync_pt_dup(struct sync_pt *sync_pt)
{
struct sw_sync_pt *pt = (struct sw_sync_pt *) sync_pt;
struct sw_sync_timeline *obj =
- (struct sw_sync_timeline *)sync_pt-parent;
+ (struct sw_sync_timeline *)sync_pt_parent(sync_pt);
return (struct sync_pt *) sw_sync_pt_create(obj, pt-value);
}
@@ -59,7 +59,7 @@ static int sw_sync_pt_has_signaled(struct sync_pt *sync_pt)
{
struct sw_sync_pt *pt = (struct sw_sync_pt *)sync_pt;
struct sw_sync_timeline *obj =
- (struct sw_sync_timeline *)sync_pt-parent;
+ (struct sw_sync_timeline *)sync_pt_parent(sync_pt);
return sw_sync_cmp(obj-value, pt-value) = 0;
}
@@ -97,7 +97,6 @@ static void sw_sync_pt_value_str(struct sync_pt *sync_pt,
char *str, int size)
{
struct sw_sync_pt *pt = (struct sw_sync_pt *)sync_pt;
-
snprintf(str, size, %d, pt-value);
}
@@ -157,7 +156,6 @@ static int sw_sync_open(struct inode *inode, struct file
*file)
static int sw_sync_release(struct inode *inode, struct file *file)
{
struct sw_sync_timeline *obj = file-private_data;
-
sync_timeline_destroy(obj-obj);
return 0;
}
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index 18174f7c871c..70b09b5001ba 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -31,22 +31,13 @@
#define CREATE_TRACE_POINTS
#include trace/sync.h
-static void sync_fence_signal_pt(struct sync_pt *pt);
-static int _sync_pt_has_signaled(struct sync_pt *pt);
-static void sync_fence_free(struct kref *kref);
-static void sync_dump(void);
-
-static LIST_HEAD(sync_timeline_list_head);
-static DEFINE_SPINLOCK(sync_timeline_list_lock);
-
-static LIST_HEAD(sync_fence_list_head);
-static DEFINE_SPINLOCK(sync_fence_list_lock);
+static const struct fence_ops android_fence_ops;
+static const struct file_operations sync_fence_fops;
struct sync_timeline *sync_timeline_create(const struct sync_timeline_ops *ops,
int size, const char *name)
{
struct sync_timeline *obj;
- unsigned long flags;
if (size sizeof(struct sync_timeline))
return NULL;
@@ -57,17 +48,14 @@ struct sync_timeline *sync_timeline_create(const struct
sync_timeline_ops *ops,
kref_init(obj-kref);
obj-ops = ops;
+ obj-context = fence_context_alloc(1);
[REPOST PATCH 5/8] reservation: add support for fences to enable cross-device synchronisation
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
Reviewed-by: Rob Clark robdcl...@gmail.com
---
include/linux/reservation.h | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/include/linux/reservation.h b/include/linux/reservation.h
index 813dae960ebd..f3f57460a205 100644
--- a/include/linux/reservation.h
+++ b/include/linux/reservation.h
@@ -6,7 +6,7 @@
* Copyright (C) 2012 Texas Instruments
*
* Authors:
- * Rob Clark rob.cl...@linaro.org
+ * Rob Clark robdcl...@gmail.com
* Maarten Lankhorst maarten.lankho...@canonical.com
* Thomas Hellstrom thellstrom-at-vmware-dot-com
*
@@ -40,22 +40,40 @@
#define _LINUX_RESERVATION_H
#include linux/ww_mutex.h
+#include linux/fence.h
+#include linux/slab.h
extern struct ww_class reservation_ww_class;
struct reservation_object {
struct ww_mutex lock;
+
+ struct fence *fence_excl;
+ struct fence **fence_shared;
+ u32 fence_shared_count, fence_shared_max;
};
static inline void
reservation_object_init(struct reservation_object *obj)
{
ww_mutex_init(obj-lock, reservation_ww_class);
+
+ obj-fence_shared_count = obj-fence_shared_max = 0;
+ obj-fence_shared = NULL;
+ obj-fence_excl = NULL;
}
static inline void
reservation_object_fini(struct reservation_object *obj)
{
+ int i;
+
+ if (obj-fence_excl)
+ fence_put(obj-fence_excl);
+ for (i = 0; i obj-fence_shared_count; ++i)
+ fence_put(obj-fence_shared[i]);
+ kfree(obj-fence_shared);
+
ww_mutex_destroy(obj-lock);
}
--
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
[REPOST PATCH 0/8] fence synchronization patches
The following series implements fence and converts dma-buf and android sync to use it. Patch 5 and 6 add support for polling to dma-buf, blocking until all fences are signaled. Patch 7 and 8 provide some helpers, and allow use of RCU in the reservation api. The helpers make it easier to convert ttm, and make dealing with rcu less painful. Patches slightly updated to fix compilation with armada and new atomic primitives, but otherwise identical. --- Maarten Lankhorst (8): fence: dma-buf cross-device synchronization (v17) seqno-fence: Hardware dma-buf implementation of fencing (v5) dma-buf: use reservation objects android: convert sync to fence api, v5 reservation: add support for fences to enable cross-device synchronisation dma-buf: add poll support, v3 reservation: update api and add some helpers reservation: add suppport for read-only access using rcu Documentation/DocBook/device-drivers.tmpl |3 drivers/base/Kconfig |9 drivers/base/Makefile |2 drivers/base/dma-buf.c | 168 drivers/base/fence.c | 468 drivers/base/reservation.c | 440 drivers/gpu/drm/armada/armada_gem.c|2 drivers/gpu/drm/drm_prime.c|8 drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |2 drivers/gpu/drm/i915/i915_gem_dmabuf.c |3 drivers/gpu/drm/nouveau/nouveau_drm.c |1 drivers/gpu/drm/nouveau/nouveau_gem.h |1 drivers/gpu/drm/nouveau/nouveau_prime.c|7 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |2 drivers/gpu/drm/radeon/radeon_drv.c|2 drivers/gpu/drm/radeon/radeon_prime.c |8 drivers/gpu/drm/tegra/gem.c|2 drivers/gpu/drm/ttm/ttm_object.c |2 drivers/media/v4l2-core/videobuf2-dma-contig.c |2 drivers/staging/android/Kconfig|1 drivers/staging/android/Makefile |2 drivers/staging/android/ion/ion.c |3 drivers/staging/android/sw_sync.c |6 drivers/staging/android/sync.c | 913 drivers/staging/android/sync.h | 79 +- drivers/staging/android/sync_debug.c | 247 ++ drivers/staging/android/trace/sync.h | 12 include/drm/drmP.h |3 include/linux/dma-buf.h| 21 - include/linux/fence.h | 355 + include/linux/reservation.h| 82 ++ include/linux/seqno-fence.h| 119 +++ include/trace/events/fence.h | 128 +++ 33 files changed, 2435 insertions(+), 668 deletions(-) create mode 100644 drivers/base/fence.c create mode 100644 drivers/staging/android/sync_debug.c create mode 100644 include/linux/fence.h create mode 100644 include/linux/seqno-fence.h create mode 100644 include/trace/events/fence.h -- Signature -- 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
[REPOST PATCH 7/8] reservation: update api and add some helpers
Move the list of shared fences to a struct, and return it in
reservation_object_get_list().
Add reservation_object_get_excl to get the exclusive fence.
Add reservation_object_reserve_shared(), which reserves space
in the reservation_object for 1 more shared fence.
reservation_object_add_shared_fence() and
reservation_object_add_excl_fence() are used to assign a new
fence to a reservation_object pointer, to complete a reservation.
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
Changes since v1:
- Add reservation_object_get_excl, reorder code a bit.
---
drivers/base/dma-buf.c | 35 +++---
drivers/base/fence.c|4 +
drivers/base/reservation.c | 156 +++
include/linux/fence.h |6 ++
include/linux/reservation.h | 56 ++-
5 files changed, 236 insertions(+), 21 deletions(-)
diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
index 25e8c4165936..cb8379dfeed5 100644
--- a/drivers/base/dma-buf.c
+++ b/drivers/base/dma-buf.c
@@ -134,7 +134,10 @@ static unsigned int dma_buf_poll(struct file *file,
poll_table *poll)
{
struct dma_buf *dmabuf;
struct reservation_object *resv;
+ struct reservation_object_list *fobj;
+ struct fence *fence_excl;
unsigned long events;
+ unsigned shared_count;
dmabuf = file-private_data;
if (!dmabuf || !dmabuf-resv)
@@ -150,12 +153,18 @@ static unsigned int dma_buf_poll(struct file *file,
poll_table *poll)
ww_mutex_lock(resv-lock, NULL);
- if (resv-fence_excl (!(events POLLOUT) ||
-resv-fence_shared_count == 0)) {
+ fobj = resv-fence;
+ if (!fobj)
+ goto out;
+
+ shared_count = fobj-shared_count;
+ fence_excl = resv-fence_excl;
+
+ if (fence_excl (!(events POLLOUT) || shared_count == 0)) {
struct dma_buf_poll_cb_t *dcb = dmabuf-cb_excl;
unsigned long pevents = POLLIN;
- if (resv-fence_shared_count == 0)
+ if (shared_count == 0)
pevents |= POLLOUT;
spin_lock_irq(dmabuf-poll.lock);
@@ -167,19 +176,20 @@ static unsigned int dma_buf_poll(struct file *file,
poll_table *poll)
spin_unlock_irq(dmabuf-poll.lock);
if (events pevents) {
- if (!fence_add_callback(resv-fence_excl,
- dcb-cb, dma_buf_poll_cb))
+ if (!fence_add_callback(fence_excl, dcb-cb,
+ dma_buf_poll_cb)) {
events = ~pevents;
- else
+ } else {
/*
* No callback queued, wake up any additional
* waiters.
*/
dma_buf_poll_cb(NULL, dcb-cb);
+ }
}
}
- if ((events POLLOUT) resv-fence_shared_count 0) {
+ if ((events POLLOUT) shared_count 0) {
struct dma_buf_poll_cb_t *dcb = dmabuf-cb_shared;
int i;
@@ -194,15 +204,18 @@ static unsigned int dma_buf_poll(struct file *file,
poll_table *poll)
if (!(events POLLOUT))
goto out;
- for (i = 0; i resv-fence_shared_count; ++i)
- if (!fence_add_callback(resv-fence_shared[i],
- dcb-cb, dma_buf_poll_cb)) {
+ for (i = 0; i shared_count; ++i) {
+ struct fence *fence = fobj-shared[i];
+
+ if (!fence_add_callback(fence, dcb-cb,
+ dma_buf_poll_cb)) {
events = ~POLLOUT;
break;
}
+ }
/* No callback queued, wake up any additional waiters. */
- if (i == resv-fence_shared_count)
+ if (i == shared_count)
dma_buf_poll_cb(NULL, dcb-cb);
}
diff --git a/drivers/base/fence.c b/drivers/base/fence.c
index 752a2dfa505f..74d1f7bcb467 100644
--- a/drivers/base/fence.c
+++ b/drivers/base/fence.c
@@ -170,7 +170,7 @@ void release_fence(struct kref *kref)
if (fence-ops-release)
fence-ops-release(fence);
else
- kfree(fence);
+ free_fence(fence);
}
EXPORT_SYMBOL(release_fence);
@@ -448,7 +448,7 @@ static void seqno_release(struct fence *fence)
if (f-ops-release)
f-ops-release(fence);
else
- kfree(f);
+ free_fence(fence);
}
static long seqno_wait(struct fence *fence, bool intr, signed long timeout)
diff --git
[REPOST PATCH 8/8] reservation: add suppport for read-only access using rcu
This adds 4 more functions to deal with rcu.
reservation_object_get_fences_rcu() will obtain the list of shared
and exclusive fences without obtaining the ww_mutex.
reservation_object_wait_timeout_rcu() will wait on all fences of the
reservation_object, without obtaining the ww_mutex.
reservation_object_test_signaled_rcu() will test if all fences of the
reservation_object are signaled without using the ww_mutex.
reservation_object_get_excl() is added because touching the fence_excl
member directly will trigger a sparse warning.
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
Reviewed-By: Thomas Hellstrom thellst...@vmware.com
---
drivers/base/dma-buf.c | 47 +-
drivers/base/reservation.c | 336 ---
include/linux/fence.h | 20 ++-
include/linux/reservation.h | 52 +--
4 files changed, 400 insertions(+), 55 deletions(-)
diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
index cb8379dfeed5..f3014c448e1e 100644
--- a/drivers/base/dma-buf.c
+++ b/drivers/base/dma-buf.c
@@ -137,7 +137,7 @@ static unsigned int dma_buf_poll(struct file *file,
poll_table *poll)
struct reservation_object_list *fobj;
struct fence *fence_excl;
unsigned long events;
- unsigned shared_count;
+ unsigned shared_count, seq;
dmabuf = file-private_data;
if (!dmabuf || !dmabuf-resv)
@@ -151,14 +151,20 @@ static unsigned int dma_buf_poll(struct file *file,
poll_table *poll)
if (!events)
return 0;
- ww_mutex_lock(resv-lock, NULL);
+retry:
+ seq = read_seqcount_begin(resv-seq);
+ rcu_read_lock();
- fobj = resv-fence;
- if (!fobj)
- goto out;
-
- shared_count = fobj-shared_count;
- fence_excl = resv-fence_excl;
+ fobj = rcu_dereference(resv-fence);
+ if (fobj)
+ shared_count = fobj-shared_count;
+ else
+ shared_count = 0;
+ fence_excl = rcu_dereference(resv-fence_excl);
+ if (read_seqcount_retry(resv-seq, seq)) {
+ rcu_read_unlock();
+ goto retry;
+ }
if (fence_excl (!(events POLLOUT) || shared_count == 0)) {
struct dma_buf_poll_cb_t *dcb = dmabuf-cb_excl;
@@ -176,14 +182,20 @@ static unsigned int dma_buf_poll(struct file *file,
poll_table *poll)
spin_unlock_irq(dmabuf-poll.lock);
if (events pevents) {
- if (!fence_add_callback(fence_excl, dcb-cb,
+ if (!fence_get_rcu(fence_excl)) {
+ /* force a recheck */
+ events = ~pevents;
+ dma_buf_poll_cb(NULL, dcb-cb);
+ } else if (!fence_add_callback(fence_excl, dcb-cb,
dma_buf_poll_cb)) {
events = ~pevents;
+ fence_put(fence_excl);
} else {
/*
* No callback queued, wake up any additional
* waiters.
*/
+ fence_put(fence_excl);
dma_buf_poll_cb(NULL, dcb-cb);
}
}
@@ -205,13 +217,26 @@ static unsigned int dma_buf_poll(struct file *file,
poll_table *poll)
goto out;
for (i = 0; i shared_count; ++i) {
- struct fence *fence = fobj-shared[i];
+ struct fence *fence = rcu_dereference(fobj-shared[i]);
+ if (!fence_get_rcu(fence)) {
+ /*
+* fence refcount dropped to zero, this means
+* that fobj has been freed
+*
+* call dma_buf_poll_cb and force a recheck!
+*/
+ events = ~POLLOUT;
+ dma_buf_poll_cb(NULL, dcb-cb);
+ break;
+ }
if (!fence_add_callback(fence, dcb-cb,
dma_buf_poll_cb)) {
+ fence_put(fence);
events = ~POLLOUT;
break;
}
+ fence_put(fence);
}
/* No callback queued, wake up any additional waiters. */
@@ -220,7 +245,7 @@ static unsigned int dma_buf_poll(struct file *file,
poll_table *poll)
}
out:
- ww_mutex_unlock(resv-lock);
+ rcu_read_unlock();
return events;
}
diff --git a/drivers/base/reservation.c
[REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
A fence can be attached to a buffer which is being filled or consumed
by hw, to allow userspace to pass the buffer without waiting to another
device. For example, userspace can call page_flip ioctl to display the
next frame of graphics after kicking the GPU but while the GPU is still
rendering. The display device sharing the buffer with the GPU would
attach a callback to get notified when the GPU's rendering-complete IRQ
fires, to update the scan-out address of the display, without having to
wake up userspace.
A driver must allocate a fence context for each execution ring that can
run in parallel. The function for this takes an argument with how many
contexts to allocate:
+ fence_context_alloc()
A fence is transient, one-shot deal. It is allocated and attached
to one or more dma-buf's. When the one that attached it is done, with
the pending operation, it can signal the fence:
+ fence_signal()
To have a rough approximation whether a fence is fired, call:
+ fence_is_signaled()
The dma-buf-mgr handles tracking, and waiting on, the fences associated
with a dma-buf.
The one pending on the fence can add an async callback:
+ fence_add_callback()
The callback can optionally be cancelled with:
+ fence_remove_callback()
To wait synchronously, optionally with a timeout:
+ fence_wait()
+ fence_wait_timeout()
When emitting a fence, call:
+ trace_fence_emit()
To annotate that a fence is blocking on another fence, call:
+ trace_fence_annotate_wait_on(fence, on_fence)
A default software-only implementation is provided, which can be used
by drivers attaching a fence to a buffer when they have no other means
for hw sync. But a memory backed fence is also envisioned, because it
is common that GPU's can write to, or poll on some memory location for
synchronization. For example:
fence = custom_get_fence(...);
if ((seqno_fence = to_seqno_fence(fence)) != NULL) {
dma_buf *fence_buf = seqno_fence-sync_buf;
get_dma_buf(fence_buf);
... tell the hw the memory location to wait ...
custom_wait_on(fence_buf, seqno_fence-seqno_ofs, fence-seqno);
} else {
/* fall-back to sw sync * /
fence_add_callback(fence, my_cb);
}
On SoC platforms, if some other hw mechanism is provided for synchronizing
between IP blocks, it could be supported as an alternate implementation
with it's own fence ops in a similar way.
enable_signaling callback is used to provide sw signaling in case a cpu
waiter is requested or no compatible hardware signaling could be used.
The intention is to provide a userspace interface (presumably via eventfd)
later, to be used in conjunction with dma-buf's mmap support for sw access
to buffers (or for userspace apps that would prefer to do their own
synchronization).
v1: Original
v2: After discussion w/ danvet and mlankhorst on #dri-devel, we decided
that dma-fence didn't need to care about the sw-hw signaling path
(it can be handled same as sw-sw case), and therefore the fence-ops
can be simplified and more handled in the core. So remove the signal,
add_callback, cancel_callback, and wait ops, and replace with a simple
enable_signaling() op which can be used to inform a fence supporting
hw-hw signaling that one or more devices which do not support hw
signaling are waiting (and therefore it should enable an irq or do
whatever is necessary in order that the CPU is notified when the
fence is passed).
v3: Fix locking fail in attach_fence() and get_fence()
v4: Remove tie-in w/ dma-buf.. after discussion w/ danvet and mlankorst
we decided that we need to be able to attach one fence to N dma-buf's,
so using the list_head in dma-fence struct would be problematic.
v5: [ Maarten Lankhorst ] Updated for dma-bikeshed-fence and dma-buf-manager.
v6: [ Maarten Lankhorst ] I removed dma_fence_cancel_callback and some comments
about checking if fence fired or not. This is broken by design.
waitqueue_active during destruction is now fatal, since the signaller
should be holding a reference in enable_signalling until it signalled
the fence. Pass the original dma_fence_cb along, and call __remove_wait
in the dma_fence_callback handler, so that no cleanup needs to be
performed.
v7: [ Maarten Lankhorst ] Set cb-func and only enable sw signaling if
fence wasn't signaled yet, for example for hardware fences that may
choose to signal blindly.
v8: [ Maarten Lankhorst ] Tons of tiny fixes, moved __dma_fence_init to
header and fixed include mess. dma-fence.h now includes dma-buf.h
All members are now initialized, so kmalloc can be used for
allocating a dma-fence. More documentation added.
v9: Change compiler bitfields to flags, change return type of
enable_signaling to bool. Rework dma_fence_wait. Added
dma_fence_is_signaled and dma_fence_wait_timeout.
s/dma// and change exports to non GPL. Added fence_is_signaled and
fence_enable_sw_signaling calls, add ability to override default
[REPOST PATCH 3/8] dma-buf: use reservation objects
This allows reservation objects to be used in dma-buf. it's required
for implementing polling support on the fences that belong to a dma-buf.
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
Acked-by: Mauro Carvalho Chehab m.che...@samsung.com #drivers/media/v4l2-core/
Acked-by: Thomas Hellstrom thellst...@vmware.com #drivers/gpu/drm/ttm
Signed-off-by: Vincent Stehlé vincent.ste...@laposte.net
#drivers/gpu/drm/armada/
---
drivers/base/dma-buf.c | 22 --
drivers/gpu/drm/armada/armada_gem.c|2 +-
drivers/gpu/drm/drm_prime.c|8 +++-
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |2 +-
drivers/gpu/drm/i915/i915_gem_dmabuf.c |3 ++-
drivers/gpu/drm/nouveau/nouveau_drm.c |1 +
drivers/gpu/drm/nouveau/nouveau_gem.h |1 +
drivers/gpu/drm/nouveau/nouveau_prime.c|7 +++
drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |2 +-
drivers/gpu/drm/radeon/radeon_drv.c|2 ++
drivers/gpu/drm/radeon/radeon_prime.c |8
drivers/gpu/drm/tegra/gem.c|2 +-
drivers/gpu/drm/ttm/ttm_object.c |2 +-
drivers/media/v4l2-core/videobuf2-dma-contig.c |2 +-
drivers/staging/android/ion/ion.c |3 ++-
include/drm/drmP.h |3 +++
include/linux/dma-buf.h|9 ++---
17 files changed, 65 insertions(+), 14 deletions(-)
diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
index 840c7fa80983..cd40ca22911f 100644
--- a/drivers/base/dma-buf.c
+++ b/drivers/base/dma-buf.c
@@ -25,10 +25,12 @@
#include linux/fs.h
#include linux/slab.h
#include linux/dma-buf.h
+#include linux/fence.h
#include linux/anon_inodes.h
#include linux/export.h
#include linux/debugfs.h
#include linux/seq_file.h
+#include linux/reservation.h
static inline int is_dma_buf_file(struct file *);
@@ -56,6 +58,9 @@ static int dma_buf_release(struct inode *inode, struct file
*file)
list_del(dmabuf-list_node);
mutex_unlock(db_list.lock);
+ if (dmabuf-resv == (struct reservation_object *)dmabuf[1])
+ reservation_object_fini(dmabuf-resv);
+
kfree(dmabuf);
return 0;
}
@@ -128,6 +133,7 @@ static inline int is_dma_buf_file(struct file *file)
* @size: [in]Size of the buffer
* @flags: [in]mode flags for the file.
* @exp_name: [in]name of the exporting module - useful for debugging.
+ * @resv: [in]reservation-object, NULL to allocate default one.
*
* Returns, on success, a newly created dma_buf object, which wraps the
* supplied private data and operations for dma_buf_ops. On either missing
@@ -135,10 +141,17 @@ static inline int is_dma_buf_file(struct file *file)
*
*/
struct dma_buf *dma_buf_export_named(void *priv, const struct dma_buf_ops *ops,
- size_t size, int flags, const char *exp_name)
+ size_t size, int flags, const char *exp_name,
+ struct reservation_object *resv)
{
struct dma_buf *dmabuf;
struct file *file;
+ size_t alloc_size = sizeof(struct dma_buf);
+ if (!resv)
+ alloc_size += sizeof(struct reservation_object);
+ else
+ /* prevent dma_buf[1] == dma_buf-resv */
+ alloc_size += 1;
if (WARN_ON(!priv || !ops
|| !ops-map_dma_buf
@@ -150,7 +163,7 @@ struct dma_buf *dma_buf_export_named(void *priv, const
struct dma_buf_ops *ops,
return ERR_PTR(-EINVAL);
}
- dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
+ dmabuf = kzalloc(alloc_size, GFP_KERNEL);
if (dmabuf == NULL)
return ERR_PTR(-ENOMEM);
@@ -158,6 +171,11 @@ struct dma_buf *dma_buf_export_named(void *priv, const
struct dma_buf_ops *ops,
dmabuf-ops = ops;
dmabuf-size = size;
dmabuf-exp_name = exp_name;
+ if (!resv) {
+ resv = (struct reservation_object *)dmabuf[1];
+ reservation_object_init(resv);
+ }
+ dmabuf-resv = resv;
file = anon_inode_getfile(dmabuf, dma_buf_fops, dmabuf, flags);
if (IS_ERR(file)) {
diff --git a/drivers/gpu/drm/armada/armada_gem.c
b/drivers/gpu/drm/armada/armada_gem.c
index bb9b642d8485..7496f55611a5 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -539,7 +539,7 @@ armada_gem_prime_export(struct drm_device *dev, struct
drm_gem_object *obj,
int flags)
{
return dma_buf_export(obj, armada_gem_prime_dmabuf_ops, obj-size,
- O_RDWR);
+ O_RDWR, NULL);
}
struct drm_gem_object *
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 304ca8cacbc4..99d578bad17e 100644
---
Re: [PATCH RESEND] libv4lconvert: Fix a regression when converting from Y10B
Hi,
On 06/16/2014 05:00 PM, Antonio Ospite wrote:
Fix a regression introduced in commit
efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too
short source buffer before accessing it).
The old code:
case V4L2_PIX_FMT_Y10BPACK:
...
if (result == 0 src_size (width * height * 10 / 8)) {
V4LCONVERT_ERR(short y10b data frame\n);
errno = EPIPE;
result = -1;
}
...
meant to say If the conversion was *successful* _but_ the frame size
was invalid, then take the error path, but in
efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was
misunderstood and v4lconvert_convert_pixfmt() was made to return an
error even in the case of a successful conversion from Y10B.
Fix the check, and now print only the message letting the errno and the
result from the conversion routines to be propagated to the caller.
Signed-off-by: Antonio Ospite a...@ao2.it
Cc: Gregor Jasny gja...@googlemail.com
Thanks for the patch, but: ...
---
lib/libv4lconvert/libv4lconvert.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/lib/libv4lconvert/libv4lconvert.c
b/lib/libv4lconvert/libv4lconvert.c
index c49d30d..50d6906 100644
--- a/lib/libv4lconvert/libv4lconvert.c
+++ b/lib/libv4lconvert/libv4lconvert.c
@@ -1052,11 +1052,8 @@ static int v4lconvert_convert_pixfmt(struct
v4lconvert_data *data,
width, height);
break;
}
- if (result == 0) {
+ if (result != 0)
V4LCONVERT_ERR(y10b conversion failed\n);
- errno = EPIPE;
- result = -1;
- }
break;
case V4L2_PIX_FMT_RGB565:
Why print a message here at all in the != 0 case? In the old code before commit
efc29f1764 you did not print an error when v4lconvert_y10b_to_... failed, so
I assume that that already does a V4LCONVERT_ERR in that case. So why do it a
second time with a less precise error message here?
So I believe that the proper fix would be to just remove the entire block
instead
of flipping the test and keeping the V4LCONVERT_ERR. Please send a new version
with this fixed, then I'll merge it asap.
Regards,
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
Re: [PATCH RESEND] libv4lconvert: Fix a regression when converting from Y10B
Hi,
On 06/18/2014 01:43 PM, Hans de Goede wrote:
Hi,
On 06/16/2014 05:00 PM, Antonio Ospite wrote:
Fix a regression introduced in commit
efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too
short source buffer before accessing it).
The old code:
case V4L2_PIX_FMT_Y10BPACK:
...
if (result == 0 src_size (width * height * 10 / 8)) {
V4LCONVERT_ERR(short y10b data frame\n);
errno = EPIPE;
result = -1;
}
...
meant to say If the conversion was *successful* _but_ the frame size
was invalid, then take the error path, but in
efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was
misunderstood and v4lconvert_convert_pixfmt() was made to return an
error even in the case of a successful conversion from Y10B.
Fix the check, and now print only the message letting the errno and the
result from the conversion routines to be propagated to the caller.
Signed-off-by: Antonio Ospite a...@ao2.it
Cc: Gregor Jasny gja...@googlemail.com
Thanks for the patch, but: ...
---
lib/libv4lconvert/libv4lconvert.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/lib/libv4lconvert/libv4lconvert.c
b/lib/libv4lconvert/libv4lconvert.c
index c49d30d..50d6906 100644
--- a/lib/libv4lconvert/libv4lconvert.c
+++ b/lib/libv4lconvert/libv4lconvert.c
@@ -1052,11 +1052,8 @@ static int v4lconvert_convert_pixfmt(struct
v4lconvert_data *data,
width, height);
break;
}
-if (result == 0) {
+if (result != 0)
V4LCONVERT_ERR(y10b conversion failed\n);
-errno = EPIPE;
-result = -1;
-}
break;
case V4L2_PIX_FMT_RGB565:
Why print a message here at all in the != 0 case? In the old code before
commit
efc29f1764 you did not print an error when v4lconvert_y10b_to_... failed, so
I assume that that already does a V4LCONVERT_ERR in that case. So why do it a
second time with a less precise error message here?
So I believe that the proper fix would be to just remove the entire block
instead
of flipping the test and keeping the V4LCONVERT_ERR. Please send a new version
with this fixed, then I'll merge it asap.
Scrap that, I decided I might just as well fix this bit myself, so I've just
pushed an updated patch completely removing the second check from the
V4L2_PIX_FMT_Y10BPACK case.
Regards,
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
We offer all purpose loan at 3% interest rate
We offer all purpose loan at 3% interest rate. Contact Us for more details by Email:santanderfinancegr...@gmail.com -- 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] Add patch to allow compilation on versions 3.5 with CONFIG_OF
Doesn't this produce compiler warnings since the static v4l2_of_parse_* functions are now never called? Those functions are inside the #if 0, so the compiler never sees them. I would patch v4l2-core/Makefile instead to just never compile v4l2-of.o. Because the final Makefile is generated by a script, I didn't think you actually could address it at the make prepare phase. That said, now I see that I could have patched linux/drivers/media/vl2-core/Makefile as you suggested. I don't have time to regenerate/resubmit at this point. Take the patch if you want it, feel free to recreate the patch slightly differently with no discernible benefit, or eventually I will get around to it the next time I'm working on that tree. Cheers, Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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 RESEND] libv4lconvert: Fix a regression when converting from Y10B
On Wed, 18 Jun 2014 13:46:10 +0200
Hans de Goede hdego...@redhat.com wrote:
Hi,
On 06/18/2014 01:43 PM, Hans de Goede wrote:
Hi,
On 06/16/2014 05:00 PM, Antonio Ospite wrote:
Fix a regression introduced in commit
efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too
short source buffer before accessing it).
The old code:
case V4L2_PIX_FMT_Y10BPACK:
...
if (result == 0 src_size (width * height * 10 / 8)) {
V4LCONVERT_ERR(short y10b data frame\n);
errno = EPIPE;
result = -1;
}
...
meant to say If the conversion was *successful* _but_ the frame size
was invalid, then take the error path, but in
efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was
misunderstood and v4lconvert_convert_pixfmt() was made to return an
error even in the case of a successful conversion from Y10B.
Fix the check, and now print only the message letting the errno and the
result from the conversion routines to be propagated to the caller.
Signed-off-by: Antonio Ospite a...@ao2.it
Cc: Gregor Jasny gja...@googlemail.com
Thanks for the patch, but: ...
---
lib/libv4lconvert/libv4lconvert.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/lib/libv4lconvert/libv4lconvert.c
b/lib/libv4lconvert/libv4lconvert.c
index c49d30d..50d6906 100644
--- a/lib/libv4lconvert/libv4lconvert.c
+++ b/lib/libv4lconvert/libv4lconvert.c
@@ -1052,11 +1052,8 @@ static int v4lconvert_convert_pixfmt(struct
v4lconvert_data *data,
width, height);
break;
}
- if (result == 0) {
+ if (result != 0)
V4LCONVERT_ERR(y10b conversion failed\n);
- errno = EPIPE;
- result = -1;
- }
break;
case V4L2_PIX_FMT_RGB565:
Why print a message here at all in the != 0 case? In the old code before
commit
efc29f1764 you did not print an error when v4lconvert_y10b_to_... failed, so
I assume that that already does a V4LCONVERT_ERR in that case. So why do it
a
second time with a less precise error message here?
The one from v4lconvert_oom_error(), yes, which is generic, it does not
tell _where_ the failure was.
So I believe that the proper fix would be to just remove the entire block
instead
of flipping the test and keeping the V4LCONVERT_ERR. Please send a new
version
with this fixed, then I'll merge it asap.
Scrap that, I decided I might just as well fix this bit myself, so I've just
pushed an updated patch completely removing the second check from the
V4L2_PIX_FMT_Y10BPACK case.
The rationale behind leaving the message was:
1. The conversion routines are called even in the case of short
frames (BTW that is true for any format, not just for Y10B).
2. The conversion routines from Y10B are not in place, they
allocate a temporary buffer, so they may fail themselves.
with this in mind I saw the second message as an _additional_ error
indication to the user (useful in case of short frame _and_ conversion
failure) rather than a less precise one. However, you are right that
this additional error message was not in the original code before
efc29f1764, so your patch is perfectly fine by me.
Thanks for merging it.
BTW, comments about 1.?
What's the idea behind calling the conversion routines even for short
frames?
Ciao ciao,
Antonio
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
--
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 RESEND] libv4lconvert: Fix a regression when converting from Y10B
Hi,
On 06/18/2014 03:23 PM, Antonio Ospite wrote:
On Wed, 18 Jun 2014 13:46:10 +0200
Hans de Goede hdego...@redhat.com wrote:
Hi,
On 06/18/2014 01:43 PM, Hans de Goede wrote:
Hi,
On 06/16/2014 05:00 PM, Antonio Ospite wrote:
Fix a regression introduced in commit
efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too
short source buffer before accessing it).
The old code:
case V4L2_PIX_FMT_Y10BPACK:
...
if (result == 0 src_size (width * height * 10 / 8)) {
V4LCONVERT_ERR(short y10b data frame\n);
errno = EPIPE;
result = -1;
}
...
meant to say If the conversion was *successful* _but_ the frame size
was invalid, then take the error path, but in
efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was
misunderstood and v4lconvert_convert_pixfmt() was made to return an
error even in the case of a successful conversion from Y10B.
Fix the check, and now print only the message letting the errno and the
result from the conversion routines to be propagated to the caller.
Signed-off-by: Antonio Ospite a...@ao2.it
Cc: Gregor Jasny gja...@googlemail.com
Thanks for the patch, but: ...
---
lib/libv4lconvert/libv4lconvert.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/lib/libv4lconvert/libv4lconvert.c
b/lib/libv4lconvert/libv4lconvert.c
index c49d30d..50d6906 100644
--- a/lib/libv4lconvert/libv4lconvert.c
+++ b/lib/libv4lconvert/libv4lconvert.c
@@ -1052,11 +1052,8 @@ static int v4lconvert_convert_pixfmt(struct
v4lconvert_data *data,
width, height);
break;
}
- if (result == 0) {
+ if (result != 0)
V4LCONVERT_ERR(y10b conversion failed\n);
- errno = EPIPE;
- result = -1;
- }
break;
case V4L2_PIX_FMT_RGB565:
Why print a message here at all in the != 0 case? In the old code before
commit
efc29f1764 you did not print an error when v4lconvert_y10b_to_... failed, so
I assume that that already does a V4LCONVERT_ERR in that case. So why do it
a
second time with a less precise error message here?
The one from v4lconvert_oom_error(), yes, which is generic, it does not
tell _where_ the failure was.
So I believe that the proper fix would be to just remove the entire block
instead
of flipping the test and keeping the V4LCONVERT_ERR. Please send a new
version
with this fixed, then I'll merge it asap.
Scrap that, I decided I might just as well fix this bit myself, so I've just
pushed an updated patch completely removing the second check from the
V4L2_PIX_FMT_Y10BPACK case.
The rationale behind leaving the message was:
1. The conversion routines are called even in the case of short
frames (BTW that is true for any format, not just for Y10B).
2. The conversion routines from Y10B are not in place, they
allocate a temporary buffer, so they may fail themselves.
Right, and this already does a V4LCONVERT_ERR, which will override any
error msg stored earlier.
with this in mind I saw the second message as an _additional_ error
indication to the user (useful in case of short frame _and_ conversion
failure) rather than a less precise one. However, you are right that
this additional error message was not in the original code before
efc29f1764, so your patch is perfectly fine by me.
Thanks for merging it.
BTW, comments about 1.?
What's the idea behind calling the conversion routines even for short
frames?
For short frames the higher layer (libv4l2) will retry up to 3 times and then
just return whatever it did get. The src_size is the amount of available bytes
in the source buffer, the actual source buffer is pre-allocated and is always
large enough, so in case of 3 consecutive short frames we convert whatever we
did get + whatever data there was already in the buffer for the rest of the
frame and return that to the user.
This is useful since if the vsync timing is off between bridge and sensor,
we often miss some lines at the bottom. So by converting what ever we do get we
end up returning a frame with a mostly complete picture + 2 lines of garbage at
the bottom at 1/3th of the framerate because of the retries.
Ideally this would never happen, but it does and in this case actually showing
the broken picture and allowing the user to take a screenshot of this and
attach it to a bug report makes things a whole lot easier to debug. And in this
case the camera is even still somewhat usable by the user this way.
Likewise in other cases where the driver consistently feeds us short frames,
it can be quite helpful to actually see the contents of the short frame
for debugging purposes.
Regards,
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
Re: [PATCH RESEND] libv4lconvert: Fix a regression when converting from Y10B
On Wed, 18 Jun 2014 15:59:13 +0200 Hans de Goede hdego...@redhat.com wrote: Hi, On 06/18/2014 03:23 PM, Antonio Ospite wrote: On Wed, 18 Jun 2014 13:46:10 +0200 Hans de Goede hdego...@redhat.com wrote: Hi, On 06/18/2014 01:43 PM, Hans de Goede wrote: Hi, On 06/16/2014 05:00 PM, Antonio Ospite wrote: [...] Why print a message here at all in the != 0 case? In the old code before commit efc29f1764 you did not print an error when v4lconvert_y10b_to_... failed, so I assume that that already does a V4LCONVERT_ERR in that case. So why do it a second time with a less precise error message here? The one from v4lconvert_oom_error(), yes, which is generic, it does not tell _where_ the failure was. So I believe that the proper fix would be to just remove the entire block instead of flipping the test and keeping the V4LCONVERT_ERR. Please send a new version with this fixed, then I'll merge it asap. Scrap that, I decided I might just as well fix this bit myself, so I've just pushed an updated patch completely removing the second check from the V4L2_PIX_FMT_Y10BPACK case. The rationale behind leaving the message was: 1. The conversion routines are called even in the case of short frames (BTW that is true for any format, not just for Y10B). 2. The conversion routines from Y10B are not in place, they allocate a temporary buffer, so they may fail themselves. Right, and this already does a V4LCONVERT_ERR, which will override any error msg stored earlier. I see now, I was overlooking how V4LCONVERT_ERR() works. with this in mind I saw the second message as an _additional_ error indication to the user (useful in case of short frame _and_ conversion failure) rather than a less precise one. However, you are right that this additional error message was not in the original code before efc29f1764, so your patch is perfectly fine by me. Thanks for merging it. BTW, comments about 1.? What's the idea behind calling the conversion routines even for short frames? For short frames the higher layer (libv4l2) will retry up to 3 times and then just return whatever it did get. The src_size is the amount of available bytes in the source buffer, the actual source buffer is pre-allocated and is always large enough, so in case of 3 consecutive short frames we convert whatever we did get + whatever data there was already in the buffer for the rest of the frame and return that to the user. This is useful since if the vsync timing is off between bridge and sensor, we often miss some lines at the bottom. So by converting what ever we do get we end up returning a frame with a mostly complete picture + 2 lines of garbage at the bottom at 1/3th of the framerate because of the retries. Ideally this would never happen, but it does and in this case actually showing the broken picture and allowing the user to take a screenshot of this and attach it to a bug report makes things a whole lot easier to debug. And in this case the camera is even still somewhat usable by the user this way. Likewise in other cases where the driver consistently feeds us short frames, it can be quite helpful to actually see the contents of the short frame for debugging purposes. Regards, Hans Thanks for the explanation. Ciao, Antonio P.S. can we please have commit fff7e0eaae9734aa1f0a4e0fadef0d8c5c41b1e8 cherry-picked in the stable-1.0 branch? -- Antonio Ospite http://ao2.it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing? -- 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] Documentation: Fix DocBook build with relative $(srctree)
After commits 890676c6 (kbuild: Use relative path when building in the source tree) and 9da0763b (kbuild: Use relative path when building in a subdir of the source tree), the $(srctree) variable can be a relative path. This breaks Documentation/DocBook/media/Makefile, because it tries to create symlinks from a subdirectory of the object tree to the source tree. Fix this by using a full path in this case. Reported-by: Randy Dunlap rdun...@infradead.org Signed-off-by: Michal Marek mma...@suse.cz --- Documentation/DocBook/media/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/DocBook/media/Makefile b/Documentation/DocBook/media/Makefile index 1d27f0a..639e748 100644 --- a/Documentation/DocBook/media/Makefile +++ b/Documentation/DocBook/media/Makefile @@ -202,8 +202,8 @@ $(MEDIA_OBJ_DIR)/%: $(MEDIA_SRC_DIR)/%.b64 $(MEDIA_OBJ_DIR)/v4l2.xml: $(OBJIMGFILES) @$($(quiet)gen_xml) - @(ln -sf $(MEDIA_SRC_DIR)/v4l/*xml $(MEDIA_OBJ_DIR)/) - @(ln -sf $(MEDIA_SRC_DIR)/dvb/*xml $(MEDIA_OBJ_DIR)/) + @(ln -sf `cd $(MEDIA_SRC_DIR) /bin/pwd`/v4l/*xml $(MEDIA_OBJ_DIR)/) + @(ln -sf `cd $(MEDIA_SRC_DIR) /bin/pwd`/dvb/*xml $(MEDIA_OBJ_DIR)/) $(MEDIA_OBJ_DIR)/videodev2.h.xml: $(srctree)/include/uapi/linux/videodev2.h $(MEDIA_OBJ_DIR)/v4l2.xml @$($(quiet)gen_xml) -- 1.9.2 -- 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] Documentation: Fix DocBook build with relative $(srctree)
On 06/18/14 08:27, Michal Marek wrote: After commits 890676c6 (kbuild: Use relative path when building in the source tree) and 9da0763b (kbuild: Use relative path when building in a subdir of the source tree), the $(srctree) variable can be a relative path. This breaks Documentation/DocBook/media/Makefile, because it tries to create symlinks from a subdirectory of the object tree to the source tree. Fix this by using a full path in this case. Reported-by: Randy Dunlap rdun...@infradead.org Signed-off-by: Michal Marek mma...@suse.cz Thanks. Acked-by: Randy Dunlap rdun...@infradead.org Tested-by: Randy Dunlap rdun...@infradead.org Please merge to Linus sooner instead of later. --- Documentation/DocBook/media/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/DocBook/media/Makefile b/Documentation/DocBook/media/Makefile index 1d27f0a..639e748 100644 --- a/Documentation/DocBook/media/Makefile +++ b/Documentation/DocBook/media/Makefile @@ -202,8 +202,8 @@ $(MEDIA_OBJ_DIR)/%: $(MEDIA_SRC_DIR)/%.b64 $(MEDIA_OBJ_DIR)/v4l2.xml: $(OBJIMGFILES) @$($(quiet)gen_xml) - @(ln -sf $(MEDIA_SRC_DIR)/v4l/*xml $(MEDIA_OBJ_DIR)/) - @(ln -sf $(MEDIA_SRC_DIR)/dvb/*xml $(MEDIA_OBJ_DIR)/) + @(ln -sf `cd $(MEDIA_SRC_DIR) /bin/pwd`/v4l/*xml $(MEDIA_OBJ_DIR)/) + @(ln -sf `cd $(MEDIA_SRC_DIR) /bin/pwd`/dvb/*xml $(MEDIA_OBJ_DIR)/) $(MEDIA_OBJ_DIR)/videodev2.h.xml: $(srctree)/include/uapi/linux/videodev2.h $(MEDIA_OBJ_DIR)/v4l2.xml @$($(quiet)gen_xml) -- ~Randy -- 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
Contact me for more details.
Hi friend. I want to transfer USD5.5Million into your account Contact me for more details. -- 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 1/1] media: saa7134: remove if based on uninitialized variable
Variable b is not initialized. Only with a small chance it has random value 0xFF. Remove if statement based on this value. Signed-off-by: Heinrich Schuchardt xypron.g...@gmx.de --- drivers/media/pci/saa7134/saa7134-input.c | 4 1 file changed, 4 deletions(-) diff --git a/drivers/media/pci/saa7134/saa7134-input.c b/drivers/media/pci/saa7134/saa7134-input.c index 6f43126..1c56f2ab 100644 --- a/drivers/media/pci/saa7134/saa7134-input.c +++ b/drivers/media/pci/saa7134/saa7134-input.c @@ -132,10 +132,6 @@ static int get_key_flydvb_trio(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) if (0x4 ~gpio) return 0; /* No button press */ - /* No button press - only before first key pressed */ - if (b == 0xFF) - return 0; - /* poll IR chip */ /* weak up the IR chip */ b = 0; -- 2.0.0 -- 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 1/1] media: dib9000: avoid out of bound access
The current test to avoid out of bound access to mb[] is insufficient. For len = 19 non-existent mb[10] will be accessed. A check in the for loop is insufficient to avoid out of bound access in dib9000_mbx_send_attr. Signed-off-by: Heinrich Schuchardt xypron.g...@gmx.de --- drivers/media/dvb-frontends/dib9000.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/media/dvb-frontends/dib9000.c b/drivers/media/dvb-frontends/dib9000.c index e540cfb..6a71917 100644 --- a/drivers/media/dvb-frontends/dib9000.c +++ b/drivers/media/dvb-frontends/dib9000.c @@ -1040,10 +1040,13 @@ static int dib9000_risc_apb_access_write(struct dib9000_state *state, u32 addres if (address = 1024 || !state-platform.risc.fw_is_running) return -EINVAL; + if (len 18) + return -EINVAL; + /* dprintk( APB access thru wr fw %d %x, address, attribute); */ mb[0] = (unsigned short)address; - for (i = 0; i len i 20; i += 2) + for (i = 0; i len; i += 2) mb[1 + (i / 2)] = (b[i] 8 | b[i + 1]); dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len / 2, attribute); -- 2.0.0 -- 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 1/1] [media] v4l: omap4iss: configuration using uninitialized variable
Variable reg is not initialized.
Random values are written to OMAP4 ISS registers if !ctx-eof_enabled.
Signed-off-by: Heinrich Schuchardt xypron.g...@gmx.de
---
drivers/staging/media/omap4iss/iss_csi2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/omap4iss/iss_csi2.c
b/drivers/staging/media/omap4iss/iss_csi2.c
index bf8a657..9ae4871 100644
--- a/drivers/staging/media/omap4iss/iss_csi2.c
+++ b/drivers/staging/media/omap4iss/iss_csi2.c
@@ -317,7 +317,7 @@ static void csi2_ctx_enable(struct iss_csi2_device *csi2,
u8 ctxnum, u8 enable)
static void csi2_ctx_config(struct iss_csi2_device *csi2,
struct iss_csi2_ctx_cfg *ctx)
{
- u32 reg;
+ u32 reg = 0;
/* Set up CSI2_CTx_CTRL1 */
if (ctx-eof_enabled)
--
2.0.0
--
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 1/1] media: dib9000: avoid out of bound access
On Wed, Jun 18, 2014 at 3:02 PM, Heinrich Schuchardt xypron.g...@gmx.de wrote: The current test to avoid out of bound access to mb[] is insufficient. For len = 19 non-existent mb[10] will be accessed. A check in the for loop is insufficient to avoid out of bound access in dib9000_mbx_send_attr. Signed-off-by: Heinrich Schuchardt xypron.g...@gmx.de --- drivers/media/dvb-frontends/dib9000.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/media/dvb-frontends/dib9000.c b/drivers/media/dvb-frontends/dib9000.c index e540cfb..6a71917 100644 --- a/drivers/media/dvb-frontends/dib9000.c +++ b/drivers/media/dvb-frontends/dib9000.c @@ -1040,10 +1040,13 @@ static int dib9000_risc_apb_access_write(struct dib9000_state *state, u32 addres if (address = 1024 || !state-platform.risc.fw_is_running) return -EINVAL; + if (len 18) + return -EINVAL; + /* dprintk( APB access thru wr fw %d %x, address, attribute); */ mb[0] = (unsigned short)address; - for (i = 0; i len i 20; i += 2) + for (i = 0; i len; i += 2) mb[1 + (i / 2)] = (b[i] 8 | b[i + 1]); Good catch on the mb[] access! However, I think there is still more to fix since b[i + 1] can read past the end of b: Say b is defined as u8 b[3]. Passing len 3 means the second loop, with i==2 will access b[2] and b[3], the latter is out of range. -Kees dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len / 2, attribute); -- 2.0.0 -- Kees Cook Chrome OS Security -- 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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
+#define CREATE_TRACE_POINTS
+#include trace/events/fence.h
+
+EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
+EXPORT_TRACEPOINT_SYMBOL(fence_emit);
Are you really willing to live with these as tracepoints for forever?
What is the use of them in debugging? Was it just for debugging the
fence code, or for something else?
+/**
+ * fence_context_alloc - allocate an array of fence contexts
+ * @num: [in]amount of contexts to allocate
+ *
+ * This function will return the first index of the number of fences
allocated.
+ * The fence context is used for setting fence-context to a unique number.
+ */
+unsigned fence_context_alloc(unsigned num)
+{
+ BUG_ON(!num);
+ return atomic_add_return(num, fence_context_counter) - num;
+}
+EXPORT_SYMBOL(fence_context_alloc);
EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here.
Traditionally all of the driver core exports have been with this
marking, any objection to making that change here as well?
+int __fence_signal(struct fence *fence)
+{
+ struct fence_cb *cur, *tmp;
+ int ret = 0;
+
+ if (WARN_ON(!fence))
+ return -EINVAL;
+
+ if (!ktime_to_ns(fence-timestamp)) {
+ fence-timestamp = ktime_get();
+ smp_mb__before_atomic();
+ }
+
+ if (test_and_set_bit(FENCE_FLAG_SIGNALED_BIT, fence-flags)) {
+ ret = -EINVAL;
+
+ /*
+ * we might have raced with the unlocked fence_signal,
+ * still run through all callbacks
+ */
+ } else
+ trace_fence_signaled(fence);
+
+ list_for_each_entry_safe(cur, tmp, fence-cb_list, node) {
+ list_del_init(cur-node);
+ cur-func(fence, cur);
+ }
+ return ret;
+}
+EXPORT_SYMBOL(__fence_signal);
Don't export a function with __ in front of it, you are saying that an
internal function is somehow valid for everyone else to call? Why?
You aren't even documenting the function, so who knows how to use it?
+/**
+ * fence_signal - signal completion of a fence
+ * @fence: the fence to signal
+ *
+ * Signal completion for software callbacks on a fence, this will unblock
+ * fence_wait() calls and run all the callbacks added with
+ * fence_add_callback(). Can be called multiple times, but since a fence
+ * can only go from unsignaled to signaled state, it will only be effective
+ * the first time.
+ */
+int fence_signal(struct fence *fence)
+{
+ unsigned long flags;
+
+ if (!fence)
+ return -EINVAL;
+
+ if (!ktime_to_ns(fence-timestamp)) {
+ fence-timestamp = ktime_get();
+ smp_mb__before_atomic();
+ }
+
+ if (test_and_set_bit(FENCE_FLAG_SIGNALED_BIT, fence-flags))
+ return -EINVAL;
+
+ trace_fence_signaled(fence);
+
+ if (test_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, fence-flags)) {
+ struct fence_cb *cur, *tmp;
+
+ spin_lock_irqsave(fence-lock, flags);
+ list_for_each_entry_safe(cur, tmp, fence-cb_list, node) {
+ list_del_init(cur-node);
+ cur-func(fence, cur);
+ }
+ spin_unlock_irqrestore(fence-lock, flags);
+ }
+ return 0;
+}
+EXPORT_SYMBOL(fence_signal);
So, why should I use this over __fence_signal? What is the difference?
Am I missing some documentation somewhere?
+void release_fence(struct kref *kref)
+{
+ struct fence *fence =
+ container_of(kref, struct fence, refcount);
+
+ trace_fence_destroy(fence);
+
+ BUG_ON(!list_empty(fence-cb_list));
+
+ if (fence-ops-release)
+ fence-ops-release(fence);
+ else
+ kfree(fence);
+}
+EXPORT_SYMBOL(release_fence);
fence_release() makes it more unified with the other functions in this
file, right?
+/**
+ * fence_default_wait - default sleep until the fence gets signaled
+ * or until timeout elapses
+ * @fence: [in]the fence to wait on
+ * @intr:[in]if true, do an interruptible wait
+ * @timeout: [in]timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ *
+ * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
+ * remaining timeout in jiffies on success.
+ */
+long
Shouldn't this be signed to be explicit?
+fence_default_wait(struct fence *fence, bool intr, signed long timeout)
+{
+ struct default_wait_cb cb;
+ unsigned long flags;
+ long ret = timeout;
+ bool was_set;
+
+ if (test_bit(FENCE_FLAG_SIGNALED_BIT, fence-flags))
+ return timeout;
+
+ spin_lock_irqsave(fence-lock, flags);
+
+ if (intr signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ goto out;
+ }
+
+ was_set = test_and_set_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, fence-flags);
+
+ if
Re: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote: + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. I don't like this paragraph in all of the files, but if you insist that some lawyer wants it there, I'll live with it... + * You should have received a copy of the GNU General Public License along with + * this program. If not, see http://www.gnu.org/licenses/. That's just not needed at all and is fluff. Please remove. -- 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: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote: Just to show it's easy. Android syncpoints can be mapped to a timeline. This removes the need to maintain a separate api for synchronization. I've left the android trace events in place, but the core fence events should already be sufficient for debugging. v2: - Call fence_remove_callback in sync_fence_free if not all fences have fired. v3: - Merge Colin Cross' bugfixes, and the android fence merge optimization. v4: - Merge with the upstream fixes. v5: - Fix small style issues pointed out by Thomas Hellstrom. Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com Acked-by: John Stultz john.stu...@linaro.org --- drivers/staging/android/Kconfig |1 drivers/staging/android/Makefile |2 drivers/staging/android/sw_sync.c|6 drivers/staging/android/sync.c | 913 +++--- drivers/staging/android/sync.h | 79 ++- drivers/staging/android/sync_debug.c | 247 + drivers/staging/android/trace/sync.h | 12 7 files changed, 609 insertions(+), 651 deletions(-) create mode 100644 drivers/staging/android/sync_debug.c With these changes, can we pull the android sync logic out of drivers/staging/ now? thanks, greg k-h -- 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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
A fence can be attached to a buffer which is being filled or consumed
by hw, to allow userspace to pass the buffer without waiting to another
device. For example, userspace can call page_flip ioctl to display the
next frame of graphics after kicking the GPU but while the GPU is still
rendering. The display device sharing the buffer with the GPU would
attach a callback to get notified when the GPU's rendering-complete IRQ
fires, to update the scan-out address of the display, without having to
wake up userspace.
A driver must allocate a fence context for each execution ring that can
run in parallel. The function for this takes an argument with how many
contexts to allocate:
+ fence_context_alloc()
A fence is transient, one-shot deal. It is allocated and attached
to one or more dma-buf's. When the one that attached it is done, with
the pending operation, it can signal the fence:
+ fence_signal()
To have a rough approximation whether a fence is fired, call:
+ fence_is_signaled()
The dma-buf-mgr handles tracking, and waiting on, the fences associated
with a dma-buf.
The one pending on the fence can add an async callback:
+ fence_add_callback()
The callback can optionally be cancelled with:
+ fence_remove_callback()
To wait synchronously, optionally with a timeout:
+ fence_wait()
+ fence_wait_timeout()
When emitting a fence, call:
+ trace_fence_emit()
To annotate that a fence is blocking on another fence, call:
+ trace_fence_annotate_wait_on(fence, on_fence)
A default software-only implementation is provided, which can be used
by drivers attaching a fence to a buffer when they have no other means
for hw sync. But a memory backed fence is also envisioned, because it
is common that GPU's can write to, or poll on some memory location for
synchronization. For example:
fence = custom_get_fence(...);
if ((seqno_fence = to_seqno_fence(fence)) != NULL) {
dma_buf *fence_buf = seqno_fence-sync_buf;
get_dma_buf(fence_buf);
... tell the hw the memory location to wait ...
custom_wait_on(fence_buf, seqno_fence-seqno_ofs, fence-seqno);
} else {
/* fall-back to sw sync * /
fence_add_callback(fence, my_cb);
}
On SoC platforms, if some other hw mechanism is provided for synchronizing
between IP blocks, it could be supported as an alternate implementation
with it's own fence ops in a similar way.
enable_signaling callback is used to provide sw signaling in case a cpu
waiter is requested or no compatible hardware signaling could be used.
The intention is to provide a userspace interface (presumably via eventfd)
later, to be used in conjunction with dma-buf's mmap support for sw access
to buffers (or for userspace apps that would prefer to do their own
synchronization).
v1: Original
v2: After discussion w/ danvet and mlankhorst on #dri-devel, we decided
that dma-fence didn't need to care about the sw-hw signaling path
(it can be handled same as sw-sw case), and therefore the fence-ops
can be simplified and more handled in the core. So remove the signal,
add_callback, cancel_callback, and wait ops, and replace with a simple
enable_signaling() op which can be used to inform a fence supporting
hw-hw signaling that one or more devices which do not support hw
signaling are waiting (and therefore it should enable an irq or do
whatever is necessary in order that the CPU is notified when the
fence is passed).
v3: Fix locking fail in attach_fence() and get_fence()
v4: Remove tie-in w/ dma-buf.. after discussion w/ danvet and mlankorst
we decided that we need to be able to attach one fence to N dma-buf's,
so using the list_head in dma-fence struct would be problematic.
v5: [ Maarten Lankhorst ] Updated for dma-bikeshed-fence and dma-buf-manager.
v6: [ Maarten Lankhorst ] I removed dma_fence_cancel_callback and some
comments
about checking if fence fired or not. This is broken by design.
waitqueue_active during destruction is now fatal, since the signaller
should be holding a reference in enable_signalling until it signalled
the fence. Pass the original dma_fence_cb along, and call __remove_wait
in the dma_fence_callback handler, so that no cleanup needs to be
performed.
v7: [ Maarten Lankhorst ] Set cb-func and only enable sw signaling if
fence wasn't signaled yet, for example for hardware fences that may
choose to signal blindly.
v8: [ Maarten Lankhorst ] Tons of tiny fixes, moved __dma_fence_init to
header and fixed include mess. dma-fence.h now includes dma-buf.h
All members are now initialized, so kmalloc can be used for
allocating a dma-fence. More documentation added.
v9: Change compiler bitfields to flags, change return type of
enable_signaling to bool. Rework dma_fence_wait. Added
Re: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Wed, Jun 18, 2014 at 9:13 PM, Greg KH gre...@linuxfoundation.org wrote:
On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
+#define CREATE_TRACE_POINTS
+#include trace/events/fence.h
+
+EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
+EXPORT_TRACEPOINT_SYMBOL(fence_emit);
Are you really willing to live with these as tracepoints for forever?
What is the use of them in debugging? Was it just for debugging the
fence code, or for something else?
fwiw, the goal is something like this:
http://people.freedesktop.org/~robclark/perf-supertuxkart.svg
but without needing to make perf understand each driver's custom trace events
(from:
http://bloggingthemonkey.blogspot.com/2013/09/freedreno-update-moar-fps.html
)
BR,
-R
+/**
+ * fence_context_alloc - allocate an array of fence contexts
+ * @num: [in]amount of contexts to allocate
+ *
+ * This function will return the first index of the number of fences
allocated.
+ * The fence context is used for setting fence-context to a unique number.
+ */
+unsigned fence_context_alloc(unsigned num)
+{
+ BUG_ON(!num);
+ return atomic_add_return(num, fence_context_counter) - num;
+}
+EXPORT_SYMBOL(fence_context_alloc);
EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here.
Traditionally all of the driver core exports have been with this
marking, any objection to making that change here as well?
+int __fence_signal(struct fence *fence)
+{
+ struct fence_cb *cur, *tmp;
+ int ret = 0;
+
+ if (WARN_ON(!fence))
+ return -EINVAL;
+
+ if (!ktime_to_ns(fence-timestamp)) {
+ fence-timestamp = ktime_get();
+ smp_mb__before_atomic();
+ }
+
+ if (test_and_set_bit(FENCE_FLAG_SIGNALED_BIT, fence-flags)) {
+ ret = -EINVAL;
+
+ /*
+ * we might have raced with the unlocked fence_signal,
+ * still run through all callbacks
+ */
+ } else
+ trace_fence_signaled(fence);
+
+ list_for_each_entry_safe(cur, tmp, fence-cb_list, node) {
+ list_del_init(cur-node);
+ cur-func(fence, cur);
+ }
+ return ret;
+}
+EXPORT_SYMBOL(__fence_signal);
Don't export a function with __ in front of it, you are saying that an
internal function is somehow valid for everyone else to call? Why?
You aren't even documenting the function, so who knows how to use it?
+/**
+ * fence_signal - signal completion of a fence
+ * @fence: the fence to signal
+ *
+ * Signal completion for software callbacks on a fence, this will unblock
+ * fence_wait() calls and run all the callbacks added with
+ * fence_add_callback(). Can be called multiple times, but since a fence
+ * can only go from unsignaled to signaled state, it will only be effective
+ * the first time.
+ */
+int fence_signal(struct fence *fence)
+{
+ unsigned long flags;
+
+ if (!fence)
+ return -EINVAL;
+
+ if (!ktime_to_ns(fence-timestamp)) {
+ fence-timestamp = ktime_get();
+ smp_mb__before_atomic();
+ }
+
+ if (test_and_set_bit(FENCE_FLAG_SIGNALED_BIT, fence-flags))
+ return -EINVAL;
+
+ trace_fence_signaled(fence);
+
+ if (test_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, fence-flags)) {
+ struct fence_cb *cur, *tmp;
+
+ spin_lock_irqsave(fence-lock, flags);
+ list_for_each_entry_safe(cur, tmp, fence-cb_list, node) {
+ list_del_init(cur-node);
+ cur-func(fence, cur);
+ }
+ spin_unlock_irqrestore(fence-lock, flags);
+ }
+ return 0;
+}
+EXPORT_SYMBOL(fence_signal);
So, why should I use this over __fence_signal? What is the difference?
Am I missing some documentation somewhere?
+void release_fence(struct kref *kref)
+{
+ struct fence *fence =
+ container_of(kref, struct fence, refcount);
+
+ trace_fence_destroy(fence);
+
+ BUG_ON(!list_empty(fence-cb_list));
+
+ if (fence-ops-release)
+ fence-ops-release(fence);
+ else
+ kfree(fence);
+}
+EXPORT_SYMBOL(release_fence);
fence_release() makes it more unified with the other functions in this
file, right?
+/**
+ * fence_default_wait - default sleep until the fence gets signaled
+ * or until timeout elapses
+ * @fence: [in]the fence to wait on
+ * @intr:[in]if true, do an interruptible wait
+ * @timeout: [in]timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ *
+ * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
+ * remaining timeout in jiffies on success.
+ */
+long
Shouldn't this be signed to be explicit?
+fence_default_wait(struct fence *fence, bool intr, signed long timeout)
+{
+ struct default_wait_cb cb;
+ unsigned long flags;
+ long ret = timeout;
+ bool was_set;
+
+
Re: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Wed, Jun 18, 2014 at 9:16 PM, Greg KH gre...@linuxfoundation.org wrote:
On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
A fence can be attached to a buffer which is being filled or consumed
by hw, to allow userspace to pass the buffer without waiting to another
device. For example, userspace can call page_flip ioctl to display the
next frame of graphics after kicking the GPU but while the GPU is still
rendering. The display device sharing the buffer with the GPU would
attach a callback to get notified when the GPU's rendering-complete IRQ
fires, to update the scan-out address of the display, without having to
wake up userspace.
A driver must allocate a fence context for each execution ring that can
run in parallel. The function for this takes an argument with how many
contexts to allocate:
+ fence_context_alloc()
A fence is transient, one-shot deal. It is allocated and attached
to one or more dma-buf's. When the one that attached it is done, with
the pending operation, it can signal the fence:
+ fence_signal()
To have a rough approximation whether a fence is fired, call:
+ fence_is_signaled()
The dma-buf-mgr handles tracking, and waiting on, the fences associated
with a dma-buf.
The one pending on the fence can add an async callback:
+ fence_add_callback()
The callback can optionally be cancelled with:
+ fence_remove_callback()
To wait synchronously, optionally with a timeout:
+ fence_wait()
+ fence_wait_timeout()
When emitting a fence, call:
+ trace_fence_emit()
To annotate that a fence is blocking on another fence, call:
+ trace_fence_annotate_wait_on(fence, on_fence)
A default software-only implementation is provided, which can be used
by drivers attaching a fence to a buffer when they have no other means
for hw sync. But a memory backed fence is also envisioned, because it
is common that GPU's can write to, or poll on some memory location for
synchronization. For example:
fence = custom_get_fence(...);
if ((seqno_fence = to_seqno_fence(fence)) != NULL) {
dma_buf *fence_buf = seqno_fence-sync_buf;
get_dma_buf(fence_buf);
... tell the hw the memory location to wait ...
custom_wait_on(fence_buf, seqno_fence-seqno_ofs, fence-seqno);
} else {
/* fall-back to sw sync * /
fence_add_callback(fence, my_cb);
}
On SoC platforms, if some other hw mechanism is provided for synchronizing
between IP blocks, it could be supported as an alternate implementation
with it's own fence ops in a similar way.
enable_signaling callback is used to provide sw signaling in case a cpu
waiter is requested or no compatible hardware signaling could be used.
The intention is to provide a userspace interface (presumably via eventfd)
later, to be used in conjunction with dma-buf's mmap support for sw access
to buffers (or for userspace apps that would prefer to do their own
synchronization).
v1: Original
v2: After discussion w/ danvet and mlankhorst on #dri-devel, we decided
that dma-fence didn't need to care about the sw-hw signaling path
(it can be handled same as sw-sw case), and therefore the fence-ops
can be simplified and more handled in the core. So remove the signal,
add_callback, cancel_callback, and wait ops, and replace with a simple
enable_signaling() op which can be used to inform a fence supporting
hw-hw signaling that one or more devices which do not support hw
signaling are waiting (and therefore it should enable an irq or do
whatever is necessary in order that the CPU is notified when the
fence is passed).
v3: Fix locking fail in attach_fence() and get_fence()
v4: Remove tie-in w/ dma-buf.. after discussion w/ danvet and mlankorst
we decided that we need to be able to attach one fence to N dma-buf's,
so using the list_head in dma-fence struct would be problematic.
v5: [ Maarten Lankhorst ] Updated for dma-bikeshed-fence and dma-buf-manager.
v6: [ Maarten Lankhorst ] I removed dma_fence_cancel_callback and some
comments
about checking if fence fired or not. This is broken by design.
waitqueue_active during destruction is now fatal, since the signaller
should be holding a reference in enable_signalling until it signalled
the fence. Pass the original dma_fence_cb along, and call __remove_wait
in the dma_fence_callback handler, so that no cleanup needs to be
performed.
v7: [ Maarten Lankhorst ] Set cb-func and only enable sw signaling if
fence wasn't signaled yet, for example for hardware fences that may
choose to signal blindly.
v8: [ Maarten Lankhorst ] Tons of tiny fixes, moved __dma_fence_init to
header and fixed include mess. dma-fence.h now includes dma-buf.h
All members are now initialized, so kmalloc can be used for
allocating a dma-fence. More documentation added.
v9: Change compiler bitfields to flags, change return type of
enable_signaling
Re: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Wed, Jun 18, 2014 at 09:23:06PM -0400, Rob Clark wrote: On Wed, Jun 18, 2014 at 9:13 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote: +#define CREATE_TRACE_POINTS +#include trace/events/fence.h + +EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on); +EXPORT_TRACEPOINT_SYMBOL(fence_emit); Are you really willing to live with these as tracepoints for forever? What is the use of them in debugging? Was it just for debugging the fence code, or for something else? fwiw, the goal is something like this: http://people.freedesktop.org/~robclark/perf-supertuxkart.svg but without needing to make perf understand each driver's custom trace events (from: http://bloggingthemonkey.blogspot.com/2013/09/freedreno-update-moar-fps.html ) Will these tracepoints provide something like that? If so, great, but I want to make sure as these now become a user/kernel ABI that you can not break. thanks, greg k-h -- 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 1/1] media: dib9000: avoid out of bound access
On 19.06.2014 01:50, Kees Cook wrote: On Wed, Jun 18, 2014 at 3:02 PM, Heinrich Schuchardt xypron.g...@gmx.de wrote: The current test to avoid out of bound access to mb[] is insufficient. For len = 19 non-existent mb[10] will be accessed. A check in the for loop is insufficient to avoid out of bound access in dib9000_mbx_send_attr. Signed-off-by: Heinrich Schuchardt xypron.g...@gmx.de --- drivers/media/dvb-frontends/dib9000.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/media/dvb-frontends/dib9000.c b/drivers/media/dvb-frontends/dib9000.c index e540cfb..6a71917 100644 --- a/drivers/media/dvb-frontends/dib9000.c +++ b/drivers/media/dvb-frontends/dib9000.c @@ -1040,10 +1040,13 @@ static int dib9000_risc_apb_access_write(struct dib9000_state *state, u32 addres if (address = 1024 || !state-platform.risc.fw_is_running) return -EINVAL; + if (len 18) + return -EINVAL; + /* dprintk( APB access thru wr fw %d %x, address, attribute); */ mb[0] = (unsigned short)address; - for (i = 0; i len i 20; i += 2) + for (i = 0; i len; i += 2) mb[1 + (i / 2)] = (b[i] 8 | b[i + 1]); Good catch on the mb[] access! However, I think there is still more to fix since b[i + 1] can read past the end of b: Say b is defined as u8 b[3]. Passing len 3 means the second loop, with i==2 will access b[2] and b[3], the latter is out of range. b[] and len are provided by the caller of dib9000_risc_apb_access_write. dib9000_risc_apb_access_write cannot verify if the length of b[] matches len at all. Currently dib9000_risc_apb_access_write cannot handle odd values of len. This holds even true if b[] has been padded with zero to an even length: For odd values of len the last byte is not passed to dib9000_mbx_send_attr. What is left unclear is how odd values of len should be handled correctly: Should the caller provide a b[] padded with 0 to the next even number of bytes, or should dib9000_risc_apb_access_write take care not to read more then len bytes, or should odd values of len cause an error EINVAL. From what I read in the coding one source of the value of len is tuner_attach(), which is called from outside the dib9000 driver. Heinrich -Kees dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len / 2, attribute); -- 2.0.0 -- 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 1/1] media: dib9000: avoid out of bound access
On Wed, Jun 18, 2014 at 6:41 PM, Heinrich Schuchardt xypron.g...@gmx.de wrote:
On 19.06.2014 01:50, Kees Cook wrote:
On Wed, Jun 18, 2014 at 3:02 PM, Heinrich Schuchardt xypron.g...@gmx.de
wrote:
The current test to avoid out of bound access to mb[] is insufficient.
For len = 19 non-existent mb[10] will be accessed.
A check in the for loop is insufficient to avoid out of bound access in
dib9000_mbx_send_attr.
Signed-off-by: Heinrich Schuchardt xypron.g...@gmx.de
---
drivers/media/dvb-frontends/dib9000.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/media/dvb-frontends/dib9000.c
b/drivers/media/dvb-frontends/dib9000.c
index e540cfb..6a71917 100644
--- a/drivers/media/dvb-frontends/dib9000.c
+++ b/drivers/media/dvb-frontends/dib9000.c
@@ -1040,10 +1040,13 @@ static int dib9000_risc_apb_access_write(struct
dib9000_state *state, u32 addres
if (address = 1024 || !state-platform.risc.fw_is_running)
return -EINVAL;
+ if (len 18)
+ return -EINVAL;
+
/* dprintk( APB access thru wr fw %d %x, address, attribute);
*/
mb[0] = (unsigned short)address;
- for (i = 0; i len i 20; i += 2)
+ for (i = 0; i len; i += 2)
mb[1 + (i / 2)] = (b[i] 8 | b[i + 1]);
Good catch on the mb[] access! However, I think there is still more to
fix since b[i + 1] can read past the end of b: Say b is defined as u8
b[3]. Passing len 3 means the second loop, with i==2 will access b[2]
and b[3], the latter is out of range.
b[] and len are provided by the caller of dib9000_risc_apb_access_write.
dib9000_risc_apb_access_write cannot verify if the length of b[] matches len
at all.
Currently dib9000_risc_apb_access_write cannot handle odd values of len.
This holds even true if b[] has been padded with zero to an even length: For
odd values of len the last byte is not passed to dib9000_mbx_send_attr.
What is left unclear is how odd values of len should be handled correctly:
Should the caller provide a b[] padded with 0 to the next even number of
bytes,
or should dib9000_risc_apb_access_write take care not to read more then len
bytes,
or should odd values of len cause an error EINVAL.
From what I read in the coding one source of the value of len is
tuner_attach(), which is called from outside the dib9000 driver.
How about:
for (i = 0; i len; i += 2) {
u16 val = b[i] 8;
if (i + 1 len)
val |= b[i + 1];
mb[1 + (i / 2)] = val;
That's defensive, and would have the same effect of callers doing the padding.
-Kees
Heinrich
-Kees
dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len /
2, attribute);
--
2.0.0
--
Kees Cook
Chrome OS Security
--
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
cron job: media_tree daily build: OK
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 Jun 19 04:00:18 CEST 2014 git branch: test git hash: 2ac6f6305efef8c10994be48b20723cc70654189 gcc version:i686-linux-gcc (GCC) 4.8.2 sparse version: v0.5.0-14-gf11dd94 host hardware: x86_64 host os:3.14-5.slh.5-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-exynos: OK linux-git-arm-mx: OK linux-git-arm-omap: OK linux-git-arm-omap1: OK linux-git-arm-pxa: OK linux-git-blackfin: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.31.14-i686: OK linux-2.6.32.27-i686: OK linux-2.6.33.7-i686: OK linux-2.6.34.7-i686: OK linux-2.6.35.9-i686: OK linux-2.6.36.4-i686: OK linux-2.6.37.6-i686: OK linux-2.6.38.8-i686: OK linux-2.6.39.4-i686: OK linux-3.0.60-i686: OK linux-3.1.10-i686: OK linux-3.2.37-i686: OK linux-3.3.8-i686: OK linux-3.4.27-i686: OK linux-3.5.7-i686: OK linux-3.6.11-i686: OK linux-3.7.4-i686: OK linux-3.8-i686: OK linux-3.9.2-i686: OK linux-3.10.1-i686: OK linux-3.11.1-i686: OK linux-3.12-i686: OK linux-3.13-i686: OK linux-3.14-i686: OK linux-3.15-i686: OK linux-3.16-rc1-i686: OK linux-2.6.31.14-x86_64: OK linux-2.6.32.27-x86_64: OK linux-2.6.33.7-x86_64: OK linux-2.6.34.7-x86_64: OK linux-2.6.35.9-x86_64: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-x86_64: OK linux-3.0.60-x86_64: OK linux-3.1.10-x86_64: OK linux-3.2.37-x86_64: OK linux-3.3.8-x86_64: OK linux-3.4.27-x86_64: OK linux-3.5.7-x86_64: OK linux-3.6.11-x86_64: OK linux-3.7.4-x86_64: OK linux-3.8-x86_64: OK linux-3.9.2-x86_64: OK linux-3.10.1-x86_64: OK linux-3.11.1-x86_64: OK linux-3.12-x86_64: OK linux-3.13-x86_64: OK linux-3.14-x86_64: OK linux-3.15-x86_64: OK linux-3.16-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- 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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
Hi Greg,
On 19 June 2014 06:55, Rob Clark robdcl...@gmail.com wrote:
On Wed, Jun 18, 2014 at 9:16 PM, Greg KH gre...@linuxfoundation.org wrote:
On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
A fence can be attached to a buffer which is being filled or consumed
by hw, to allow userspace to pass the buffer without waiting to another
device. For example, userspace can call page_flip ioctl to display the
next frame of graphics after kicking the GPU but while the GPU is still
rendering. The display device sharing the buffer with the GPU would
attach a callback to get notified when the GPU's rendering-complete IRQ
fires, to update the scan-out address of the display, without having to
wake up userspace.
A driver must allocate a fence context for each execution ring that can
run in parallel. The function for this takes an argument with how many
contexts to allocate:
+ fence_context_alloc()
A fence is transient, one-shot deal. It is allocated and attached
to one or more dma-buf's. When the one that attached it is done, with
the pending operation, it can signal the fence:
+ fence_signal()
To have a rough approximation whether a fence is fired, call:
+ fence_is_signaled()
The dma-buf-mgr handles tracking, and waiting on, the fences associated
with a dma-buf.
The one pending on the fence can add an async callback:
+ fence_add_callback()
The callback can optionally be cancelled with:
+ fence_remove_callback()
To wait synchronously, optionally with a timeout:
+ fence_wait()
+ fence_wait_timeout()
When emitting a fence, call:
+ trace_fence_emit()
To annotate that a fence is blocking on another fence, call:
+ trace_fence_annotate_wait_on(fence, on_fence)
A default software-only implementation is provided, which can be used
by drivers attaching a fence to a buffer when they have no other means
for hw sync. But a memory backed fence is also envisioned, because it
is common that GPU's can write to, or poll on some memory location for
synchronization. For example:
fence = custom_get_fence(...);
if ((seqno_fence = to_seqno_fence(fence)) != NULL) {
dma_buf *fence_buf = seqno_fence-sync_buf;
get_dma_buf(fence_buf);
... tell the hw the memory location to wait ...
custom_wait_on(fence_buf, seqno_fence-seqno_ofs, fence-seqno);
} else {
/* fall-back to sw sync * /
fence_add_callback(fence, my_cb);
}
On SoC platforms, if some other hw mechanism is provided for synchronizing
between IP blocks, it could be supported as an alternate implementation
with it's own fence ops in a similar way.
enable_signaling callback is used to provide sw signaling in case a cpu
waiter is requested or no compatible hardware signaling could be used.
The intention is to provide a userspace interface (presumably via eventfd)
later, to be used in conjunction with dma-buf's mmap support for sw access
to buffers (or for userspace apps that would prefer to do their own
synchronization).
v1: Original
v2: After discussion w/ danvet and mlankhorst on #dri-devel, we decided
that dma-fence didn't need to care about the sw-hw signaling path
(it can be handled same as sw-sw case), and therefore the fence-ops
can be simplified and more handled in the core. So remove the signal,
add_callback, cancel_callback, and wait ops, and replace with a simple
enable_signaling() op which can be used to inform a fence supporting
hw-hw signaling that one or more devices which do not support hw
signaling are waiting (and therefore it should enable an irq or do
whatever is necessary in order that the CPU is notified when the
fence is passed).
v3: Fix locking fail in attach_fence() and get_fence()
v4: Remove tie-in w/ dma-buf.. after discussion w/ danvet and mlankorst
we decided that we need to be able to attach one fence to N dma-buf's,
so using the list_head in dma-fence struct would be problematic.
v5: [ Maarten Lankhorst ] Updated for dma-bikeshed-fence and
dma-buf-manager.
v6: [ Maarten Lankhorst ] I removed dma_fence_cancel_callback and some
comments
about checking if fence fired or not. This is broken by design.
waitqueue_active during destruction is now fatal, since the signaller
should be holding a reference in enable_signalling until it signalled
the fence. Pass the original dma_fence_cb along, and call __remove_wait
in the dma_fence_callback handler, so that no cleanup needs to be
performed.
v7: [ Maarten Lankhorst ] Set cb-func and only enable sw signaling if
fence wasn't signaled yet, for example for hardware fences that may
choose to signal blindly.
v8: [ Maarten Lankhorst ] Tons of tiny fixes, moved __dma_fence_init to
header and fixed include mess. dma-fence.h now includes dma-buf.h
All members are now initialized, so kmalloc can be used for
allocating a dma-fence. More documentation added.
v9: Change
Re: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, Jun 19, 2014 at 09:57:35AM +0530, Sumit Semwal wrote:
Hi Greg,
On 19 June 2014 06:55, Rob Clark robdcl...@gmail.com wrote:
On Wed, Jun 18, 2014 at 9:16 PM, Greg KH gre...@linuxfoundation.org wrote:
On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
A fence can be attached to a buffer which is being filled or consumed
by hw, to allow userspace to pass the buffer without waiting to another
device. For example, userspace can call page_flip ioctl to display the
next frame of graphics after kicking the GPU but while the GPU is still
rendering. The display device sharing the buffer with the GPU would
attach a callback to get notified when the GPU's rendering-complete IRQ
fires, to update the scan-out address of the display, without having to
wake up userspace.
A driver must allocate a fence context for each execution ring that can
run in parallel. The function for this takes an argument with how many
contexts to allocate:
+ fence_context_alloc()
A fence is transient, one-shot deal. It is allocated and attached
to one or more dma-buf's. When the one that attached it is done, with
the pending operation, it can signal the fence:
+ fence_signal()
To have a rough approximation whether a fence is fired, call:
+ fence_is_signaled()
The dma-buf-mgr handles tracking, and waiting on, the fences associated
with a dma-buf.
The one pending on the fence can add an async callback:
+ fence_add_callback()
The callback can optionally be cancelled with:
+ fence_remove_callback()
To wait synchronously, optionally with a timeout:
+ fence_wait()
+ fence_wait_timeout()
When emitting a fence, call:
+ trace_fence_emit()
To annotate that a fence is blocking on another fence, call:
+ trace_fence_annotate_wait_on(fence, on_fence)
A default software-only implementation is provided, which can be used
by drivers attaching a fence to a buffer when they have no other means
for hw sync. But a memory backed fence is also envisioned, because it
is common that GPU's can write to, or poll on some memory location for
synchronization. For example:
fence = custom_get_fence(...);
if ((seqno_fence = to_seqno_fence(fence)) != NULL) {
dma_buf *fence_buf = seqno_fence-sync_buf;
get_dma_buf(fence_buf);
... tell the hw the memory location to wait ...
custom_wait_on(fence_buf, seqno_fence-seqno_ofs, fence-seqno);
} else {
/* fall-back to sw sync * /
fence_add_callback(fence, my_cb);
}
On SoC platforms, if some other hw mechanism is provided for synchronizing
between IP blocks, it could be supported as an alternate implementation
with it's own fence ops in a similar way.
enable_signaling callback is used to provide sw signaling in case a cpu
waiter is requested or no compatible hardware signaling could be used.
The intention is to provide a userspace interface (presumably via eventfd)
later, to be used in conjunction with dma-buf's mmap support for sw access
to buffers (or for userspace apps that would prefer to do their own
synchronization).
v1: Original
v2: After discussion w/ danvet and mlankhorst on #dri-devel, we decided
that dma-fence didn't need to care about the sw-hw signaling path
(it can be handled same as sw-sw case), and therefore the fence-ops
can be simplified and more handled in the core. So remove the signal,
add_callback, cancel_callback, and wait ops, and replace with a simple
enable_signaling() op which can be used to inform a fence supporting
hw-hw signaling that one or more devices which do not support hw
signaling are waiting (and therefore it should enable an irq or do
whatever is necessary in order that the CPU is notified when the
fence is passed).
v3: Fix locking fail in attach_fence() and get_fence()
v4: Remove tie-in w/ dma-buf.. after discussion w/ danvet and mlankorst
we decided that we need to be able to attach one fence to N dma-buf's,
so using the list_head in dma-fence struct would be problematic.
v5: [ Maarten Lankhorst ] Updated for dma-bikeshed-fence and
dma-buf-manager.
v6: [ Maarten Lankhorst ] I removed dma_fence_cancel_callback and some
comments
about checking if fence fired or not. This is broken by design.
waitqueue_active during destruction is now fatal, since the signaller
should be holding a reference in enable_signalling until it signalled
the fence. Pass the original dma_fence_cb along, and call
__remove_wait
in the dma_fence_callback handler, so that no cleanup needs to be
performed.
v7: [ Maarten Lankhorst ] Set cb-func and only enable sw signaling if
fence wasn't signaled yet, for example for hardware fences that may
choose to signal blindly.
v8: [ Maarten Lankhorst ] Tons of tiny fixes, moved __dma_fence_init to
header and fixed include
Re: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On 19 June 2014 10:24, Greg KH gre...@linuxfoundation.org wrote: On Thu, Jun 19, 2014 at 09:57:35AM +0530, Sumit Semwal wrote: Hi Greg, Who is going to sign up to maintain this code? (hint, it's not me...) that would be Sumit (dma-buf tree).. probably we should move fence/reservation/dma-buf into drivers/dma-buf (or something approximately like that) Yes, that would be me - it might be better to create a new directory as suggested above (drivers/dma-buf). That's fine with me, there is going to be more than just one file in there, right? :) greg k-h Certainly atleast 3 :) ~sumit -- 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
